Merge pull request #3705 from overleaf/ae-refactor-clone-modal

Refactor "Copy Project" modal

GitOrigin-RevId: b6039d57c29c04a7cd20dd4a2ab5216d485c05ed

services/web/app/views/project/editor/left-menu.pug

@@ -46,6 +46,7 @@ aside#left-menu.full-size(
 					handle-hide="handleHide"
 					project-id="projectId"
 					project-name="projectName"
+					open-project="openProject"
 					show="show"
 				)
 

services/web/frontend/js/features/clone-project-modal/components/clone-project-modal-content.js

@@ -1,52 +1,49 @@
-import React, { useMemo, useState } from 'react'
+import React from 'react'
+import PropTypes from 'prop-types'
+import { Trans } from 'react-i18next'
 import {
   Modal,
   Alert,
   Button,
-  FormGroup,
   ControlLabel,
-  FormControl
+  FormControl,
+  FormGroup
 } from 'react-bootstrap'
-import PropTypes from 'prop-types'
-import { useTranslation } from 'react-i18next'
+import AccessibleModal from '../../../shared/components/accessible-modal'
 
-function CloneProjectModalContent({
-  cloneProject,
-  projectName = '',
-  error,
+export default function CloneProjectModalContent({
+  animation = true,
+  show,
   cancel,
-  inFlight
+  handleSubmit,
+  clonedProjectName,
+  setClonedProjectName,
+  error,
+  inFlight,
+  valid
 }) {
-  const { t } = useTranslation()
-
-  const [clonedProjectName, setClonedProjectName] = useState(
-    `${projectName} (Copy)`
-  )
-
-  const valid = useMemo(() => !!clonedProjectName, [clonedProjectName])
-
-  function handleSubmit(event) {
-    event.preventDefault()
-    if (valid) {
-      cloneProject(clonedProjectName)
-    }
-  }
-
   return (
-    <>
+    <AccessibleModal
+      animation={animation}
+      show={show}
+      onHide={cancel}
+      id="clone-project-modal"
+    >
       <Modal.Header closeButton>
-        <Modal.Title>{t('copy_project')}</Modal.Title>
+        <Modal.Title>
+          <Trans i18nKey="copy_project" />
+        </Modal.Title>
       </Modal.Header>
 
       <Modal.Body>
         <form id="clone-project-form" onSubmit={handleSubmit}>
           <FormGroup>
-            <ControlLabel htmlFor="cloned-project-name">
-              {t('new_name')}
+            <ControlLabel htmlFor="clone-project-form-name">
+              <Trans i18nKey="new_name" />
             </ControlLabel>
 
             <FormControl
-              id="cloned-project-name"
+              id="clone-project-form-name"
               type="text"
               placeholder="New Project Name"
               required
@@ -58,14 +55,18 @@ function CloneProjectModalContent({
 
         {error && (
           <Alert bsStyle="danger">
-            {error.message || t('generic_something_went_wrong')}
+            {error.length ? (
+              error
+            ) : (
+              <Trans i18nKey="generic_something_went_wrong" />
+            )}
           </Alert>
         )}
       </Modal.Body>
 
       <Modal.Footer>
         <Button type="button" disabled={inFlight} onClick={cancel}>
-          {t('cancel')}
+          <Trans i18nKey="cancel" />
         </Button>
 
         <Button
@@ -74,24 +75,26 @@ function CloneProjectModalContent({
           bsStyle="primary"
           disabled={inFlight || !valid}
         >
-          {inFlight ? <span>{t('copying')}…</span> : <span>{t('copy')}</span>}
+          {inFlight ? (
+            <>
+              <Trans i18nKey="copying" />…
+            </>
+          ) : (
+            <Trans i18nKey="copy" />
+          )}
         </Button>
       </Modal.Footer>
-    </>
+    </AccessibleModal>
   )
 }
-
 CloneProjectModalContent.propTypes = {
-  cloneProject: PropTypes.func.isRequired,
-  error: PropTypes.oneOfType([
-    PropTypes.bool,
-    PropTypes.shape({
-      message: PropTypes.string
-    })
-  ]),
+  animation: PropTypes.bool,
+  show: PropTypes.bool.isRequired,
   cancel: PropTypes.func.isRequired,
+  handleSubmit: PropTypes.func.isRequired,
+  clonedProjectName: PropTypes.string,
+  setClonedProjectName: PropTypes.func.isRequired,
+  error: PropTypes.oneOfType([PropTypes.bool, PropTypes.string]),
   inFlight: PropTypes.bool.isRequired,
-  projectName: PropTypes.string
+  valid: PropTypes.bool.isRequired
 }
-
-export default CloneProjectModalContent

services/web/frontend/js/features/clone-project-modal/components/clone-project-modal.js

@@ -1,54 +1,30 @@
-import React, { useCallback, useEffect, useState } from 'react'
-import { Modal } from 'react-bootstrap'
+import React, { useCallback, useEffect, useMemo, useState } from 'react'
 import PropTypes from 'prop-types'
+import { cloneProject } from '../utils/api'
 import CloneProjectModalContent from './clone-project-modal-content'
 
-function CloneProjectModal({ handleHide, show, projectId, projectName }) {
+function CloneProjectModal({
+  show,
+  handleHide,
+  projectId,
+  projectName = '',
+  openProject
+}) {
   const [inFlight, setInFlight] = useState(false)
   const [error, setError] = useState()
+  const [clonedProjectName, setClonedProjectName] = useState('')
 
-  // reset error when the modal is opened
+  // set the cloned project name when the modal opens
   useEffect(() => {
     if (show) {
-      setError(undefined)
+      setClonedProjectName(`${projectName} (Copy)`)
     }
-  }, [show])
-
-  // clone the project when the form is submitted
-  const cloneProject = useCallback(
-    cloneName => {
-      setInFlight(true)
+  }, [show, projectName])
 
-      fetch(`/project/${projectId}/clone`, {
-        method: 'POST',
-        body: JSON.stringify({
-          _csrf: window.csrfToken,
-          projectName: cloneName
-        }),
-        headers: {
-          'Content-Type': 'application/json'
-        }
-      })
-        .then(async response => {
-          if (response.ok) {
-            const { project_id: clonedProjectId } = await response.json()
-            window.location.assign(`/project/${clonedProjectId}`)
-          } else {
-            if (response.status === 400) {
-              setError({ message: await response.text() })
-            } else {
-              setError(true)
-            }
-            setInFlight(false)
-          }
-        })
-        .catch(() => {
-          setError(true)
-          setInFlight(false)
-        })
-    },
-    [projectId]
-  )
+  // reset error when the modal is opened
+  useEffect(() => {
+    setError(undefined)
+  }, [])
 
   // close the modal if not in flight
   const cancel = useCallback(() => {
@@ -57,16 +33,49 @@ function CloneProjectModal({ handleHide, show, projectId, projectName }) {
     }
   }, [handleHide, inFlight])
 
+  // valid if the cloned project has a name
+  const valid = useMemo(() => !!clonedProjectName, [clonedProjectName])
+
+  // form submission: clone the project if the name is valid
+  const handleSubmit = event => {
+    event.preventDefault()
+
+    if (!valid) {
+      return
+    }
+
+    setError(false)
+    setInFlight(true)
+
+    // clone the project
+    cloneProject(projectId, clonedProjectName)
+      .then(data => {
+        // open the cloned project
+        openProject(data.project_id)
+      })
+      .catch(({ response, data }) => {
+        if (response.status === 400) {
+          setError(data.message)
+        } else {
+          setError(true)
+        }
+      })
+      .finally(() => {
+        setInFlight(false)
+      })
+  }
+
   return (
-    <Modal show={show} onHide={cancel}>
-      <CloneProjectModalContent
-        cloneProject={cloneProject}
-        error={error}
-        cancel={cancel}
-        inFlight={inFlight}
-        projectName={projectName}
-      />
-    </Modal>
+    <CloneProjectModalContent
+      show={show}
+      cancel={cancel}
+      inFlight={inFlight}
+      valid={valid}
+      error={error}
+      clonedProjectName={clonedProjectName}
+      setClonedProjectName={setClonedProjectName}
+      handleSubmit={handleSubmit}
+    />
   )
 }
 
@@ -74,6 +83,7 @@ CloneProjectModal.propTypes = {
   handleHide: PropTypes.func.isRequired,
   projectId: PropTypes.string.isRequired,
   projectName: PropTypes.string,
+  openProject: PropTypes.func.isRequired,
   show: PropTypes.bool.isRequired
 }
 

services/web/frontend/js/features/clone-project-modal/controllers/left-menu-clone-project-modal-controller.js

@@ -18,6 +18,10 @@ export default App.controller('LeftMenuCloneProjectModalController', function(
     })
   }
 
+  $scope.openProject = projectId => {
+    window.location.assign(`/project/${projectId}`)
+  }
+
   $scope.openCloneProjectModal = () => {
     $scope.$applyAsync(() => {
       const { project } = ide.$scope

services/web/frontend/js/features/clone-project-modal/utils/api.js

@@ -0,0 +1,9 @@
+import { postJSON } from '../../../infrastructure/fetch-json'
+
+export function cloneProject(projectId, cloneName) {
+  return postJSON(`/project/${projectId}/clone`, {
+    body: {
+      projectName: cloneName
+    }
+  })
+}

services/web/frontend/stories/clone-project-modal-content.stories.js

@@ -0,0 +1,46 @@
+import React from 'react'
+
+import CloneProjectModalContent from '../js/features/clone-project-modal/components/clone-project-modal-content'
+
+export const Basic = args => {
+  return <CloneProjectModalContent {...args} />
+}
+
+export const Invalid = args => {
+  return (
+    <CloneProjectModalContent {...args} clonedProjectName="" valid={false} />
+  )
+}
+
+export const Inflight = args => {
+  return <CloneProjectModalContent {...args} inFlight />
+}
+
+export const GenericError = args => {
+  return <CloneProjectModalContent {...args} error />
+}
+
+export const SpecificError = args => {
+  return (
+    <CloneProjectModalContent {...args} error="There was a specific error" />
+  )
+}
+
+export default {
+  title: 'Clone Project Modal / Content',
+  component: CloneProjectModalContent,
+  args: {
+    animation: false,
+    projectId: 'original-project',
+    clonedProjectName: 'Project Title',
+    show: true,
+    error: false,
+    inFlight: false,
+    valid: true
+  },
+  argTypes: {
+    cancel: { action: 'cancel' },
+    handleSubmit: { action: 'submit' },
+    setClonedProjectName: { action: 'set project name' }
+  }
+}

services/web/frontend/stories/clone-project-modal.stories.js

@@ -1,67 +1,67 @@
 import React from 'react'
+import fetchMock from 'fetch-mock'
+import PropTypes from 'prop-types'
 
-import CloneProjectModalContent from '../js/features/clone-project-modal/components/clone-project-modal-content'
+import CloneProjectModal from '../js/features/clone-project-modal/components/clone-project-modal'
 
-// NOTE: CloneProjectModalContent is wrapped in modal classes, without modal behaviours
+export const Interactive = ({
+  mockResponse = 200,
+  mockResponseDelay = 500,
+  ...args
+}) => {
+  fetchMock.restore().post(
+    'express:/project/:projectId/clone',
+    () => {
+      switch (mockResponse) {
+        case 400:
+          return { status: 400, body: 'The project name is not valid' }
 
-export const Form = args => (
-  <div className="modal-dialog">
-    <div className="modal-content">
-      <CloneProjectModalContent {...args} />
-    </div>
-  </div>
-)
-Form.args = {
-  inFlight: false,
-  error: false
-}
-
-export const Loading = args => (
-  <div className="modal-dialog">
-    <div className="modal-content">
-      <CloneProjectModalContent {...args} />
-    </div>
-  </div>
-)
-Loading.args = {
-  inFlight: true,
-  error: false
-}
+        default:
+          return mockResponse
+      }
+    },
+    { delay: mockResponseDelay }
+  )
 
-export const LoadingError = args => (
-  <div className="modal-dialog">
-    <div className="modal-content">
-      <CloneProjectModalContent {...args} />
-    </div>
-  </div>
-)
-LoadingError.args = {
-  inFlight: false,
-  error: true
+  return <CloneProjectModal {...args} />
 }
-
-export const LoadingErrorMessage = args => (
-  <div className="modal-dialog">
-    <div className="modal-content">
-      <CloneProjectModalContent {...args} />
-    </div>
-  </div>
-)
-LoadingErrorMessage.args = {
-  inFlight: false,
-  error: {
-    message: 'The chosen project name is already in use'
-  }
+Interactive.propTypes = {
+  mockResponse: PropTypes.number,
+  mockResponseDelay: PropTypes.number
 }
 
 export default {
   title: 'Clone Project Modal',
-  component: CloneProjectModalContent,
+  component: CloneProjectModal,
   args: {
-    projectName: 'Project Title'
+    projectId: 'original-project',
+    projectName: 'Project Title',
+    show: true
   },
   argTypes: {
-    cloneProject: { action: 'cloneProject' },
-    cancel: { action: 'cancel' }
+    handleHide: { action: 'close modal' },
+    openProject: { action: 'open project' },
+    mockResponse: {
+      name: 'Mock Response Status',
+      type: { name: 'number', required: false },
+      description: 'The status code that should be returned by the mock server',
+      defaultValue: 200,
+      control: {
+        type: 'radio',
+        options: [200, 500, 400]
+      }
+    },
+    mockResponseDelay: {
+      name: 'Mock Response Delay',
+      type: { name: 'number', required: false },
+      description: 'The delay before returning a response from the mock server',
+      defaultValue: 500,
+      control: {
+        type: 'range',
+        min: 0,
+        max: 2500,
+        step: 250
+      }
+    }
   }
 }

services/web/test/frontend/features/clone-project-modal/components/clone-project-modal.test.js

@@ -1,69 +1,84 @@
 import React from 'react'
-import { fireEvent, render, screen } from '@testing-library/react'
+import { fireEvent, render, screen, waitFor } from '@testing-library/react'
 import { expect } from 'chai'
-import CloneProjectModalContent from '../../../../../frontend/js/features/clone-project-modal/components/clone-project-modal-content'
 import CloneProjectModal from '../../../../../frontend/js/features/clone-project-modal/components/clone-project-modal'
 import sinon from 'sinon'
 import fetchMock from 'fetch-mock'
 
-const cancel = sinon.stub()
-const cloneProject = sinon.stub()
-const handleHide = sinon.stub()
-
 describe('<CloneProjectModal />', function() {
   afterEach(function() {
     fetchMock.reset()
   })
 
-  it('posts the generated project name', async function() {
-    const matcher = 'express:/project/:projectId/clone'
+  const modalProps = {
+    handleHide: sinon.stub(),
+    projectId: 'project-1',
+    projectName: 'Test Project',
+    openProject: sinon.stub(),
+    show: true
+  }
 
-    fetchMock.postOnce(
-      matcher,
-      () => {
-        return {
-          project_id: 'test'
-        }
-      },
+  it('renders the translated modal title', async function() {
+    render(<CloneProjectModal {...modalProps} />)
+
+    await screen.findByText('Copy Project')
+  })
+
+  it('posts the generated project name', async function() {
+    fetchMock.post(
+      'express:/project/:projectId/clone',
       {
-        body: {
-          projectName: 'A Project (Copy)'
-        }
-      }
+        status: 200,
+        body: { project_id: modalProps.projectId }
+      },
+      { delay: 10 }
     )
 
-    render(
-      <CloneProjectModal
-        handleHide={handleHide}
-        projectId="project-1"
-        projectName="A Project"
-        show
-      />
-    )
+    const openProject = sinon.stub()
+
+    render(<CloneProjectModal {...modalProps} openProject={openProject} />)
+
+    const cancelButton = await screen.findByRole('button', { name: 'Cancel' })
+    expect(cancelButton.disabled).to.be.false
 
-    const button = await screen.findByRole('button', {
-      name: 'Copy',
-      hidden: true // TODO: this shouldn't be needed
+    const submitButton = await screen.findByRole('button', { name: 'Copy' })
+    expect(submitButton.disabled).to.be.false
+
+    const input = await screen.getByLabelText('New Name')
+
+    fireEvent.change(input, {
+      target: { value: '' }
     })
+    expect(submitButton.disabled).to.be.true
 
-    const cancelButton = await screen.findByRole('button', {
-      name: 'Cancel',
-      hidden: true // TODO: this shouldn't be needed
+    fireEvent.change(input, {
+      target: { value: 'A Cloned Project' }
     })
+    expect(submitButton.disabled).to.be.false
 
-    expect(button.disabled).to.be.false
-    expect(cancelButton.disabled).to.be.false
+    fireEvent.click(submitButton)
+    expect(submitButton.disabled).to.be.true
 
-    fireEvent.click(button)
+    await fetchMock.flush(true)
+    expect(fetchMock.done()).to.be.true
+    const [url, options] = fetchMock.lastCall(
+      'express:/project/:projectId/clone'
+    )
+    expect(url).to.equal('/project/project-1/clone')
 
-    expect(fetchMock.done(matcher)).to.be.true
-    // TODO: window.location?
+    expect(JSON.parse(options.body)).to.deep.equal({
+      projectName: 'A Cloned Project'
+    })
+
+    expect(openProject).to.be.calledOnce
 
     const errorMessage = screen.queryByText('Sorry, something went wrong')
     expect(errorMessage).to.be.null
 
-    expect(button.disabled).to.be.true
-    expect(cancelButton.disabled).to.be.true
+    await waitFor(() => {
+      expect(submitButton.disabled).to.be.false
+      expect(cancelButton.disabled).to.be.false
+    })
   })
 
   it('handles a generic error response', async function() {
@@ -74,31 +89,20 @@ describe('<CloneProjectModal />', function() {
       body: 'There was an error!'
     })
 
-    render(
-      <CloneProjectModal
-        handleHide={handleHide}
-        projectId="project-2"
-        projectName="A Project"
-        show
-      />
-    )
-
-    const button = await screen.findByRole('button', {
-      name: 'Copy',
-      hidden: true // TODO: this shouldn't be needed
-    })
+    const openProject = sinon.stub()
 
-    const cancelButton = await screen.findByRole('button', {
-      name: 'Cancel',
-      hidden: true // TODO: this shouldn't be needed
-    })
+    render(<CloneProjectModal {...modalProps} openProject={openProject} />)
 
+    const button = await screen.findByRole('button', { name: 'Copy' })
     expect(button.disabled).to.be.false
+
+    const cancelButton = await screen.findByRole('button', { name: 'Cancel' })
     expect(cancelButton.disabled).to.be.false
 
     fireEvent.click(button)
 
     expect(fetchMock.done(matcher)).to.be.true
+    expect(openProject).not.to.be.called
 
     await screen.findByText('Sorry, something went wrong')
 
@@ -114,31 +118,21 @@ describe('<CloneProjectModal />', function() {
       body: 'There was an error!'
     })
 
-    render(
-      <CloneProjectModal
-        handleHide={handleHide}
-        projectId="project-3"
-        projectName="A Project"
-        show
-      />
-    )
+    const openProject = sinon.stub()
 
-    const button = await screen.findByRole('button', {
-      name: 'Copy',
-      hidden: true // TODO: this shouldn't be needed
-    })
-
-    const cancelButton = await screen.findByRole('button', {
-      name: 'Cancel',
-      hidden: true // TODO: this shouldn't be needed
-    })
+    render(<CloneProjectModal {...modalProps} openProject={openProject} />)
 
+    const button = await screen.findByRole('button', { name: 'Copy' })
     expect(button.disabled).to.be.false
+
+    const cancelButton = await screen.findByRole('button', { name: 'Cancel' })
     expect(cancelButton.disabled).to.be.false
 
     fireEvent.click(button)
+    await fetchMock.flush(true)
 
     expect(fetchMock.done(matcher)).to.be.true
+    expect(openProject).not.to.be.called
 
     await screen.findByText('There was an error!')
 
@@ -146,92 +140,3 @@ describe('<CloneProjectModal />', function() {
     expect(cancelButton.disabled).to.be.false
   })
 })
-
-describe('<CloneProjectModalContent />', function() {
-  it('renders the translated modal title', async function() {
-    render(
-      <CloneProjectModalContent
-        cloneProject={cloneProject}
-        cancel={cancel}
-        inFlight={false}
-      />
-    )
-
-    await screen.findByText('Copy Project')
-  })
-
-  it('shows the copy button', async function() {
-    render(
-      <CloneProjectModalContent
-        cloneProject={cloneProject}
-        cancel={cancel}
-        inFlight={false}
-      />
-    )
-
-    const button = await screen.findByRole('button', { name: 'Copy' })
-
-    expect(button.disabled).to.be.false
-  })
-
-  it('disables the copy button when loading', async function() {
-    render(
-      <CloneProjectModalContent
-        cloneProject={cloneProject}
-        cancel={cancel}
-        inFlight
-      />
-    )
-
-    const button = await screen.findByText(
-      (content, element) =>
-        element.nodeName === 'BUTTON' &&
-        element.textContent.trim().match(/^Copying…$/)
-    )
-
-    expect(button.disabled).to.be.true
-  })
-
-  it('renders a generic error message', async function() {
-    render(
-      <CloneProjectModalContent
-        cloneProject={cloneProject}
-        cancel={cancel}
-        inFlight={false}
-        error
-      />
-    )
-
-    await screen.findByText('Sorry, something went wrong')
-  })
-
-  it('renders a specific error message', async function() {
-    render(
-      <CloneProjectModalContent
-        cloneProject={cloneProject}
-        cancel={cancel}
-        inFlight={false}
-        error={{
-          message: 'Uh oh!'
-        }}
-      />
-    )
-
-    await screen.findByText('Uh oh!')
-  })
-
-  it('displays a project name', async function() {
-    render(
-      <CloneProjectModalContent
-        cloneProject={cloneProject}
-        cancel={cancel}
-        inFlight={false}
-        projectName="A copy of a project"
-      />
-    )
-
-    const input = await screen.getByLabelText('New Name')
-
-    expect(input.value).to.equal('A copy of a project (Copy)')
-  })
-})