From d0d28524a2ae55ad6c78380157809f7668bdd1e0 Mon Sep 17 00:00:00 2001 From: Alf Eaton <75253002+aeaton-overleaf@users.noreply.github.com> Date: Wed, 31 Mar 2021 11:44:20 +0100 Subject: [PATCH] Switch to `useScopeValue` for project data in Share modal (#3823) GitOrigin-RevId: f82170c241c59cf7b66fea7e1471004e46ab3547 --- .../web/app/views/project/editor/header.pug | 2 - .../components/edit-member.js | 7 +- .../components/share-project-modal.js | 21 +- .../react-share-project-modal-controller.js | 35 +-- .../shared/context/util/scope-value-hook.js | 3 +- .../stories/share-project-modal.stories.js | 43 +++- .../components/share-project-modal.test.js | 217 ++++++++++-------- 7 files changed, 181 insertions(+), 147 deletions(-) diff --git a/services/web/app/views/project/editor/header.pug b/services/web/app/views/project/editor/header.pug index 47d3c2bf03..651fcef9d0 100644 --- a/services/web/app/views/project/editor/header.pug +++ b/services/web/app/views/project/editor/header.pug @@ -129,8 +129,6 @@ header.toolbar.toolbar-header.toolbar-with-labels( handle-hide="handleHide" show="show" is-admin="isAdmin" - project="clonedProject" - update-project="updateProject" ) != moduleIncludes('publish:button', locals) diff --git a/services/web/frontend/js/features/share-project-modal/components/edit-member.js b/services/web/frontend/js/features/share-project-modal/components/edit-member.js index 4d084c36a4..1b29bb3e8f 100644 --- a/services/web/frontend/js/features/share-project-modal/components/edit-member.js +++ b/services/web/frontend/js/features/share-project-modal/components/edit-member.js @@ -1,4 +1,4 @@ -import React, { useState } from 'react' +import React, { useState, useEffect } from 'react' import PropTypes from 'prop-types' import { Trans, useTranslation } from 'react-i18next' import { @@ -25,6 +25,11 @@ export default function EditMember({ member }) { setConfirmingOwnershipTransfer ] = useState(false) + // update the local state if the member's privileges change externally + useEffect(() => { + setPrivileges(member.privileges) + }, [member.privileges]) + const { updateProject, monitorRequest } = useShareProjectContext() const project = useProjectContext() diff --git a/services/web/frontend/js/features/share-project-modal/components/share-project-modal.js b/services/web/frontend/js/features/share-project-modal/components/share-project-modal.js index 16e5c18059..6fe99ff5fc 100644 --- a/services/web/frontend/js/features/share-project-modal/components/share-project-modal.js +++ b/services/web/frontend/js/features/share-project-modal/components/share-project-modal.js @@ -7,6 +7,7 @@ import React, { } from 'react' import PropTypes from 'prop-types' import ShareProjectModalContent from './share-project-modal-content' +import useScopeValue from '../../../shared/context/util/scope-value-hook' const ShareProjectContext = createContext() @@ -87,12 +88,13 @@ export default function ShareProjectModal({ animation = true, isAdmin, eventTracking, - project, - updateProject + ide }) { const [inFlight, setInFlight] = useState(false) const [error, setError] = useState() + const [project, setProject] = useScopeValue('project', ide.$scope, true) + // reset error when the modal is opened useEffect(() => { if (show) { @@ -129,6 +131,14 @@ export default function ShareProjectModal({ return promise }, []) + // merge the new data with the old project data + const updateProject = useCallback( + data => { + setProject(project => Object.assign(project, data)) + }, + [setProject] + ) + if (!project) { return null } @@ -162,10 +172,11 @@ ShareProjectModal.propTypes = { animation: PropTypes.bool, handleHide: PropTypes.func.isRequired, isAdmin: PropTypes.bool.isRequired, - project: projectShape, + ide: PropTypes.shape({ + $scope: PropTypes.object.isRequired + }).isRequired, show: PropTypes.bool.isRequired, eventTracking: PropTypes.shape({ sendMB: PropTypes.func.isRequired - }), - updateProject: PropTypes.func.isRequired + }) } diff --git a/services/web/frontend/js/features/share-project-modal/controllers/react-share-project-modal-controller.js b/services/web/frontend/js/features/share-project-modal/controllers/react-share-project-modal-controller.js index 4f193269ec..f7585b0494 100644 --- a/services/web/frontend/js/features/share-project-modal/controllers/react-share-project-modal-controller.js +++ b/services/web/frontend/js/features/share-project-modal/controllers/react-share-project-modal-controller.js @@ -1,11 +1,13 @@ import App from '../../../base' import { react2angular } from 'react2angular' -import cloneDeep from 'lodash/cloneDeep' import ShareProjectModal from '../components/share-project-modal' import { listProjectInvites, listProjectMembers } from '../utils/api' -App.component('shareProjectModal', react2angular(ShareProjectModal)) +App.component( + 'shareProjectModal', + react2angular(ShareProjectModal, undefined, ['ide']) +) export default App.controller('ReactShareProjectModalController', function( $scope, @@ -15,49 +17,20 @@ export default App.controller('ReactShareProjectModalController', function( $scope.isAdmin = false $scope.show = false - let deregisterProjectWatch - - // deep watch $scope.project for changes - function registerProjectWatch() { - deregisterProjectWatch = $scope.$watch( - 'project', - project => { - $scope.clonedProject = cloneDeep(project) - }, - true - ) - } - $scope.handleHide = () => { $scope.$applyAsync(() => { $scope.show = false - if (deregisterProjectWatch) { - deregisterProjectWatch() - } }) } $scope.openShareProjectModal = isAdmin => { eventTracking.sendMBOnce('ide-open-share-modal-once') $scope.$applyAsync(() => { - registerProjectWatch() - $scope.isAdmin = isAdmin $scope.show = true }) } - // update $scope.project with new data - $scope.updateProject = data => { - if (!$scope.project) { - return - } - - $scope.$applyAsync(() => { - Object.assign($scope.project, data) - }) - } - /* tokens */ ide.socket.on('project:tokens:changed', data => { diff --git a/services/web/frontend/js/shared/context/util/scope-value-hook.js b/services/web/frontend/js/shared/context/util/scope-value-hook.js index 4c233ab6d1..c1e6b8fcbc 100644 --- a/services/web/frontend/js/shared/context/util/scope-value-hook.js +++ b/services/web/frontend/js/shared/context/util/scope-value-hook.js @@ -8,6 +8,7 @@ import _ from 'lodash' * * @param {string} path - dot '.' path of a property in `sourceScope`. * @param {object} $scope - Angular $scope containing the value to bind. + * @param {boolean} deep * @returns {[any, function]} - Binded value and setter function tuple. */ export default function useScopeValue(path, $scope, deep = false) { @@ -17,7 +18,7 @@ export default function useScopeValue(path, $scope, deep = false) { return $scope.$watch( path, newValue => { - setValue(newValue) + setValue(deep ? _.cloneDeep(newValue) : newValue) }, deep ) diff --git a/services/web/frontend/stories/share-project-modal.stories.js b/services/web/frontend/stories/share-project-modal.stories.js index 7e1e929c50..9ff960669a 100644 --- a/services/web/frontend/stories/share-project-modal.stories.js +++ b/services/web/frontend/stories/share-project-modal.stories.js @@ -73,6 +73,16 @@ const setupFetchMock = () => { }) } +const ideWithProject = project => { + return { + $scope: { + $watch: () => () => {}, + $applyAsync: () => {}, + project + } + } +} + export const LinkSharingOff = args => { setupFetchMock() @@ -81,7 +91,7 @@ export const LinkSharingOff = args => { publicAccesLevel: 'private' } - return + return } export const LinkSharingOn = args => { @@ -92,7 +102,7 @@ export const LinkSharingOn = args => { publicAccesLevel: 'tokenBased' } - return + return } export const LinkSharingLoading = args => { @@ -104,7 +114,7 @@ export const LinkSharingLoading = args => { tokens: undefined } - return + return } export const NonAdminLinkSharingOff = args => { @@ -113,7 +123,13 @@ export const NonAdminLinkSharingOff = args => { publicAccesLevel: 'private' } - return + return ( + + ) } export const NonAdminLinkSharingOn = args => { @@ -122,7 +138,13 @@ export const NonAdminLinkSharingOn = args => { publicAccesLevel: 'tokenBased' } - return + return ( + + ) } export const RestrictedTokenMember = args => { @@ -139,7 +161,7 @@ export const RestrictedTokenMember = args => { publicAccesLevel: 'tokenBased' } - return + return } export const LegacyLinkSharingReadAndWrite = args => { @@ -150,7 +172,7 @@ export const LegacyLinkSharingReadAndWrite = args => { publicAccesLevel: 'readAndWrite' } - return + return } export const LegacyLinkSharingReadOnly = args => { @@ -161,7 +183,7 @@ export const LegacyLinkSharingReadOnly = args => { publicAccesLevel: 'readOnly' } - return + return } export const LimitedCollaborators = args => { @@ -175,7 +197,7 @@ export const LimitedCollaborators = args => { } } - return + return } const project = { @@ -232,8 +254,7 @@ export default { animation: false, isAdmin: true, user: {}, - project, - updateProject: () => null + project }, argTypes: { handleHide: { action: 'hide' } diff --git a/services/web/test/frontend/features/share-project-modal/components/share-project-modal.test.js b/services/web/test/frontend/features/share-project-modal/components/share-project-modal.test.js index 490d6795b8..7c15484785 100644 --- a/services/web/test/frontend/features/share-project-modal/components/share-project-modal.test.js +++ b/services/web/test/frontend/features/share-project-modal/components/share-project-modal.test.js @@ -2,12 +2,12 @@ import { expect } from 'chai' import sinon from 'sinon' import React from 'react' import { - act, cleanup, render, screen, fireEvent, - waitFor + waitFor, + waitForElementToBeRemoved } from '@testing-library/react' import fetchMock from 'fetch-mock' import ShareProjectModal from '../../../../../frontend/js/features/share-project-modal/components/share-project-modal' @@ -69,12 +69,21 @@ describe('', function() { } ] + const ideWithProject = project => { + return { + $scope: { + $watch: () => () => {}, + $applyAsync: () => {}, + project + } + } + } + const modalProps = { + ide: ideWithProject(project), show: true, isAdmin: true, - project, - handleHide: sinon.stub(), - updateProject: sinon.stub() + handleHide: sinon.stub() } const originalExposedSettings = window.ExposedSettings @@ -122,7 +131,7 @@ describe('', function() { render( ) @@ -141,7 +150,7 @@ describe('', function() { render( ) @@ -158,7 +167,7 @@ describe('', function() { render( ) @@ -172,7 +181,7 @@ describe('', function() { render( ) @@ -195,7 +204,11 @@ describe('', function() { const { rerender } = render( ) @@ -207,7 +220,11 @@ describe('', function() { rerender( ) @@ -222,17 +239,21 @@ describe('', function() { .null expect(screen.queryByRole('button', { name: 'Resend' })).to.be.null - // render as non-admin, link sharing off: actions should be missing and message should be present + // render as non-admin (non-owner), link sharing off: actions should be missing and message should be present rerender( ) await screen.findByText( - 'To add more collaborators or turn on link sharing, please ask the project owner' + 'To change access permissions, please ask the project owner' ) expect(screen.queryByRole('button', { name: 'Turn off link sharing' })).to @@ -248,7 +269,7 @@ describe('', function() { render( ) @@ -296,12 +317,12 @@ describe('', function() { render( ) @@ -340,11 +361,11 @@ describe('', function() { render( ) @@ -353,11 +374,9 @@ describe('', function() { }) const resendButton = screen.getByRole('button', { name: 'Resend' }) + fireEvent.click(resendButton) - await act(async () => { - await fireEvent.click(resendButton) - expect(closeButton.disabled).to.be.true - }) + await waitFor(() => expect(closeButton.disabled).to.be.true) expect(fetchMock.done()).to.be.true expect(closeButton.disabled).to.be.false @@ -377,11 +396,11 @@ describe('', function() { render( ) @@ -390,11 +409,8 @@ describe('', function() { }) const revokeButton = screen.getByRole('button', { name: 'Revoke' }) - - await act(async () => { - await fireEvent.click(revokeButton) - expect(closeButton.disabled).to.be.true - }) + fireEvent.click(revokeButton) + await waitFor(() => expect(closeButton.disabled).to.be.true) expect(fetchMock.done()).to.be.true expect(closeButton.disabled).to.be.false @@ -414,11 +430,11 @@ describe('', function() { render( ) @@ -433,13 +449,11 @@ describe('', function() { const changeButton = screen.getByRole('button', { name: 'Change' }) - await act(async () => { - await fireEvent.click(changeButton) - expect(closeButton.disabled).to.be.true + fireEvent.click(changeButton) + await waitFor(() => expect(closeButton.disabled).to.be.true) - const { body } = fetchMock.lastOptions() - expect(JSON.parse(body)).to.deep.equal({ privilegeLevel: 'readAndWrite' }) - }) + const { body } = fetchMock.lastOptions() + expect(JSON.parse(body)).to.deep.equal({ privilegeLevel: 'readAndWrite' }) expect(fetchMock.done()).to.be.true expect(closeButton.disabled).to.be.false @@ -459,11 +473,11 @@ describe('', function() { render( ) @@ -473,13 +487,16 @@ describe('', function() { name: 'Remove from project' }) - act(() => { - removeButton.click() - }) + fireEvent.click(removeButton) + + const url = fetchMock.lastUrl() + expect(url).to.equal('/project/test-project/users/member-viewer') expect(fetchMock.done()).to.be.true - // TODO: once the project data is updated, assert that the member has been removed + await waitForElementToBeRemoved(() => + screen.queryByText('member-viewer@example.com') + ) }) it('changes member privileges to owner with confirmation', async function() { @@ -496,11 +513,11 @@ describe('', function() { render( ) @@ -519,20 +536,16 @@ describe('', function() { ) }) - const confirmButton = screen.getByRole('button', { - name: 'Change owner', - hidden: true - }) - const reloadStub = sinon.stub(locationModule, 'reload') - await act(async () => { - await fireEvent.click(confirmButton) - expect(confirmButton.disabled).to.be.true - - const { body } = fetchMock.lastOptions() - expect(JSON.parse(body)).to.deep.equal({ user_id: 'member-viewer' }) + const confirmButton = screen.getByRole('button', { + name: 'Change owner' }) + fireEvent.click(confirmButton) + await waitFor(() => expect(confirmButton.disabled).to.be.true) + + const { body } = fetchMock.lastOptions() + expect(JSON.parse(body)).to.deep.equal({ user_id: 'member-viewer' }) expect(fetchMock.done()).to.be.true expect(reloadStub.calledOnce).to.be.true @@ -540,39 +553,13 @@ describe('', function() { }) it('sends invites to input email addresses', async function() { - // TODO: can't use this as the value of useProjectContext doesn't get updated - // let mergedProject = { - // ...project, - // publicAccesLevel: 'tokenBased' - // } - // - // const updateProject = value => { - // mergedProject = { ...mergedProject, ...value } - // - // rerender( - // - // ) - // } - // - // const { rerender } = render( - // - // ) - render( ) @@ -661,13 +648,13 @@ describe('', function() { render( ) @@ -684,10 +671,10 @@ describe('', function() { render( ) @@ -703,13 +690,11 @@ describe('', function() { const submitButton = screen.getByRole('button', { name: 'Share' }) const respondWithError = async function(errorReason) { - await act(async () => { - inputElement.focus() - await fireEvent.change(inputElement, { - target: { value: 'invited-author-1@example.com' } - }) - inputElement.blur() + inputElement.focus() + fireEvent.change(inputElement, { + target: { value: 'invited-author-1@example.com' } }) + inputElement.blur() fetchMock.postOnce( 'express:/project/:projectId/invite', @@ -748,5 +733,45 @@ describe('', function() { ) }) - // TODO: add test for switching between token-access and private, once project data is in React context + it('handles switching between access levels', async function() { + fetchMock.post('express:/project/:projectId/settings/admin', 204) + + render( + + ) + + await screen.findByText( + 'Link sharing is off, only invited users can view this project.' + ) + + const enableButton = await screen.findByRole('button', { + name: 'Turn on link sharing' + }) + fireEvent.click(enableButton) + await waitFor(() => expect(enableButton.disabled).to.be.true) + + const { body: tokenBody } = fetchMock.lastOptions() + expect(JSON.parse(tokenBody)).to.deep.equal({ + publicAccessLevel: 'tokenBased' + }) + + await screen.findByText('Link sharing is on') + const disableButton = await screen.findByRole('button', { + name: 'Turn off link sharing' + }) + fireEvent.click(disableButton) + await waitFor(() => expect(disableButton.disabled).to.be.true) + + const { body: privateBody } = fetchMock.lastOptions() + expect(JSON.parse(privateBody)).to.deep.equal({ + publicAccessLevel: 'private' + }) + + await screen.findByText( + 'Link sharing is off, only invited users can view this project.' + ) + }) })