From f39ca200b9c387386a5dd975161a2744f873fe32 Mon Sep 17 00:00:00 2001 From: Domagoj Kriskovic Date: Mon, 9 Dec 2024 12:37:54 +0100 Subject: [PATCH] Support for adding reviewer role (#22314) * Support for adding reviewer role * added collaboratorsGetter tests * emit toggle-track-changes when reviewer is added GitOrigin-RevId: 5b831102d561f5dbc4b23ab9f6dc63e3a87ea103 --- .../Authorization/AuthorizationManager.js | 1 + .../Features/Authorization/PrivilegeLevels.js | 1 + .../Collaborators/CollaboratorsGetter.js | 14 +++++- .../Collaborators/CollaboratorsHandler.js | 49 ++++++++++++++++++- .../src/Features/Project/ProjectController.js | 1 + .../TokenAccess/TokenAccessController.mjs | 1 + services/web/app/src/models/Project.js | 1 + .../web/frontend/extracted-translations.json | 2 + .../ide-react/context/permissions-context.tsx | 8 +++ .../features/ide-react/types/permissions.ts | 2 +- .../components/member-privileges.jsx | 3 ++ .../add-collaborators.jsx | 4 ++ .../restricted-link-sharing/edit-member.tsx | 1 + .../member-privileges.jsx | 3 ++ services/web/locales/en.json | 1 + .../Collaborators/CollaboratorsGetterTests.js | 39 +++++++++++++++ .../CollaboratorsHandlerTests.js | 44 +++++++++++++++++ 17 files changed, 171 insertions(+), 4 deletions(-) diff --git a/services/web/app/src/Features/Authorization/AuthorizationManager.js b/services/web/app/src/Features/Authorization/AuthorizationManager.js index 28de66047f..565788c3ba 100644 --- a/services/web/app/src/Features/Authorization/AuthorizationManager.js +++ b/services/web/app/src/Features/Authorization/AuthorizationManager.js @@ -186,6 +186,7 @@ async function canUserReadProject(userId, projectId, token) { PrivilegeLevels.OWNER, PrivilegeLevels.READ_AND_WRITE, PrivilegeLevels.READ_ONLY, + PrivilegeLevels.REVIEW, ].includes(privilegeLevel) } diff --git a/services/web/app/src/Features/Authorization/PrivilegeLevels.js b/services/web/app/src/Features/Authorization/PrivilegeLevels.js index a598f3d04d..ff03cf8762 100644 --- a/services/web/app/src/Features/Authorization/PrivilegeLevels.js +++ b/services/web/app/src/Features/Authorization/PrivilegeLevels.js @@ -2,6 +2,7 @@ const PrivilegeLevels = { NONE: false, READ_ONLY: 'readOnly', READ_AND_WRITE: 'readAndWrite', + REVIEW: 'review', OWNER: 'owner', } diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js b/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js index e37e18f443..130cf72a8a 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js @@ -61,6 +61,7 @@ async function getMemberIdsWithPrivilegeLevels(projectId) { tokenAccessReadAndWrite_refs: 1, publicAccesLevel: 1, pendingEditor_refs: 1, + reviewer_refs: 1, }) if (!project) { throw new Errors.NotFoundError(`no project found with id ${projectId}`) @@ -72,7 +73,8 @@ async function getMemberIdsWithPrivilegeLevels(projectId) { project.tokenAccessReadAndWrite_refs, project.tokenAccessReadOnly_refs, project.publicAccesLevel, - project.pendingEditor_refs + project.pendingEditor_refs, + project.reviewer_refs ) return memberIds } @@ -324,7 +326,8 @@ function _getMemberIdsWithPrivilegeLevelsFromFields( tokenAccessIds, tokenAccessReadOnlyIds, publicAccessLevel, - pendingEditorIds + pendingEditorIds, + reviewerIds ) { const members = [] members.push({ @@ -365,6 +368,13 @@ function _getMemberIdsWithPrivilegeLevelsFromFields( }) } } + for (const memberId of reviewerIds || []) { + members.push({ + id: memberId.toString(), + privilegeLevel: PrivilegeLevels.REVIEW, + source: Sources.INVITE, + }) + } return members } diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js b/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js index 10dd76dd7a..2a696beaff 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js @@ -10,6 +10,7 @@ const TpdsProjectFlusher = require('../ThirdPartyDataStore/TpdsProjectFlusher') const CollaboratorsGetter = require('./CollaboratorsGetter') const Errors = require('../Errors/Errors') const TpdsUpdateSender = require('../ThirdPartyDataStore/TpdsUpdateSender') +const EditorRealTimeController = require('../Editor/EditorRealTimeController') module.exports = { userIsTokenMember: callbackify(userIsTokenMember), @@ -48,6 +49,7 @@ async function removeUserFromProject(projectId, userId) { $set: { archived }, $pull: { collaberator_refs: userId, + reviewer_refs: userId, readOnly_refs: userId, pendingEditor_refs: userId, tokenAccessReadOnly_refs: userId, @@ -63,6 +65,7 @@ async function removeUserFromProject(projectId, userId) { $pull: { collaberator_refs: userId, readOnly_refs: userId, + reviewer_refs: userId, pendingEditor_refs: userId, tokenAccessReadOnly_refs: userId, tokenAccessReadAndWrite_refs: userId, @@ -109,6 +112,8 @@ async function addUserIdToProject( name: 1, collaberator_refs: 1, readOnly_refs: 1, + reviewer_refs: 1, + track_changes: 1, }) let level let existingUsers = project.collaberator_refs || [] @@ -132,6 +137,9 @@ async function addUserIdToProject( { privileges: 'readOnly', userId, projectId, pendingEditor }, 'adding user' ) + } else if (privilegeLevel === PrivilegeLevels.REVIEW) { + level = { reviewer_refs: userId } + logger.debug({ privileges: 'reviewer', userId, projectId }, 'adding user') } else { throw new Error(`unknown privilegeLevel: ${privilegeLevel}`) } @@ -140,7 +148,46 @@ async function addUserIdToProject( ContactManager.addContact(addingUserId, userId, () => {}) } - await Project.updateOne({ _id: projectId }, { $addToSet: level }).exec() + if (privilegeLevel === PrivilegeLevels.REVIEW) { + const trackChanges = + typeof project.track_changes === 'object' ? project.track_changes : {} + + trackChanges[userId] = true + + if (project.track_changes === true) { + // track changes are enabled for all + // we need to convert it to explicit format + const members = + await CollaboratorsGetter.promises.getMemberIdsWithPrivilegeLevels( + project + ) + + for (const { id, privilegeLevel } of members) { + if ( + [ + PrivilegeLevels.OWNER, + PrivilegeLevels.READ_AND_WRITE, + PrivilegeLevels.REVIEW, + ].includes(privilegeLevel) + ) { + trackChanges[id] = true + } + } + } + + await Project.updateOne( + { _id: projectId }, + { track_changes: trackChanges, $addToSet: level } + ).exec() + + EditorRealTimeController.emitToRoom( + projectId, + 'toggle-track-changes', + trackChanges + ) + } else { + await Project.updateOne({ _id: projectId }, { $addToSet: level }).exec() + } // Ensure there is a dedicated folder for this "new" project. await TpdsUpdateSender.promises.createProject({ diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index 7df20bbfef..c24de1132c 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -353,6 +353,7 @@ const _ProjectController = { 'default-visual-for-beginners', 'hotjar', 'ai-add-on', + 'reviewer-role', 'papers-integration', ].filter(Boolean) diff --git a/services/web/app/src/Features/TokenAccess/TokenAccessController.mjs b/services/web/app/src/Features/TokenAccess/TokenAccessController.mjs index e95ff4c963..fa94b0673e 100644 --- a/services/web/app/src/Features/TokenAccess/TokenAccessController.mjs +++ b/services/web/app/src/Features/TokenAccess/TokenAccessController.mjs @@ -25,6 +25,7 @@ import LimitationsManager from '../Subscription/LimitationsManager.js' const orderedPrivilegeLevels = [ PrivilegeLevels.NONE, PrivilegeLevels.READ_ONLY, + PrivilegeLevels.REVIEW, PrivilegeLevels.READ_AND_WRITE, PrivilegeLevels.OWNER, ] diff --git a/services/web/app/src/models/Project.js b/services/web/app/src/models/Project.js index 249ee9979c..1555b471a4 100644 --- a/services/web/app/src/models/Project.js +++ b/services/web/app/src/models/Project.js @@ -38,6 +38,7 @@ const ProjectSchema = new Schema( active: { type: Boolean, default: true }, owner_ref: { type: ObjectId, ref: 'User' }, collaberator_refs: [{ type: ObjectId, ref: 'User' }], + reviewer_refs: [{ type: ObjectId, ref: 'User' }], readOnly_refs: [{ type: ObjectId, ref: 'User' }], pendingEditor_refs: [{ type: ObjectId, ref: 'User' }], rootDoc_id: { type: ObjectId }, diff --git a/services/web/frontend/extracted-translations.json b/services/web/frontend/extracted-translations.json index 5013677a5a..9059e253c0 100644 --- a/services/web/frontend/extracted-translations.json +++ b/services/web/frontend/extracted-translations.json @@ -175,6 +175,7 @@ "can_link_institution_email_acct_to_institution_acct": "", "can_link_your_institution_acct_2": "", "can_now_relink_dropbox": "", + "can_review": "", "can_view": "", "cancel": "", "cancel_add_on": "", @@ -1289,6 +1290,7 @@ "revert_pending_plan_change": "", "review": "", "review_your_peers_work": "", + "reviewer": "", "revoke": "", "revoke_invite": "", "right": "", diff --git a/services/web/frontend/js/features/ide-react/context/permissions-context.tsx b/services/web/frontend/js/features/ide-react/context/permissions-context.tsx index f21c2bb21c..b58d859791 100644 --- a/services/web/frontend/js/features/ide-react/context/permissions-context.tsx +++ b/services/web/frontend/js/features/ide-react/context/permissions-context.tsx @@ -21,6 +21,12 @@ const permissionsMap: DeepReadonly> = { admin: false, comment: true, }, + review: { + read: true, + write: true, + admin: false, + comment: true, + }, readAndWrite: { read: true, write: true, @@ -38,12 +44,14 @@ const permissionsMap: DeepReadonly> = { const anonymousPermissionsMap: typeof permissionsMap = { readOnly: { ...permissionsMap.readOnly, comment: false }, readAndWrite: { ...permissionsMap.readAndWrite, comment: false }, + review: { ...permissionsMap.review, comment: false }, owner: { ...permissionsMap.owner, comment: false }, } const linkSharingWarningPermissionsMap: typeof permissionsMap = { readOnly: { ...permissionsMap.readOnly, comment: false }, readAndWrite: permissionsMap.readAndWrite, + review: permissionsMap.review, owner: permissionsMap.owner, } diff --git a/services/web/frontend/js/features/ide-react/types/permissions.ts b/services/web/frontend/js/features/ide-react/types/permissions.ts index 531f4d3e79..0234c21e65 100644 --- a/services/web/frontend/js/features/ide-react/types/permissions.ts +++ b/services/web/frontend/js/features/ide-react/types/permissions.ts @@ -5,4 +5,4 @@ export type Permissions = { comment: boolean } -export type PermissionsLevel = 'owner' | 'readAndWrite' | 'readOnly' +export type PermissionsLevel = 'owner' | 'readAndWrite' | 'review' | 'readOnly' diff --git a/services/web/frontend/js/features/share-project-modal/components/member-privileges.jsx b/services/web/frontend/js/features/share-project-modal/components/member-privileges.jsx index d49a9ea479..a968bfc5fd 100644 --- a/services/web/frontend/js/features/share-project-modal/components/member-privileges.jsx +++ b/services/web/frontend/js/features/share-project-modal/components/member-privileges.jsx @@ -11,6 +11,9 @@ export default function MemberPrivileges({ privileges }) { case 'readOnly': return t('read_only') + case 'review': + return t('can_review') + default: return null } diff --git a/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/add-collaborators.jsx b/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/add-collaborators.jsx index 9e62f19404..0b64e9143b 100644 --- a/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/add-collaborators.jsx +++ b/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/add-collaborators.jsx @@ -14,6 +14,7 @@ import OLForm from '@/features/ui/components/ol/ol-form' import OLFormGroup from '@/features/ui/components/ol/ol-form-group' import OLFormSelect from '@/features/ui/components/ol/ol-form-select' import OLButton from '@/features/ui/components/ol/ol-button' +import { isSplitTestEnabled } from '@/utils/splitTestUtils' export default function AddCollaborators({ readOnly }) { const [privileges, setPrivileges] = useState('readAndWrite') @@ -173,6 +174,9 @@ export default function AddCollaborators({ readOnly }) { {t('can_edit')} + {isSplitTestEnabled('reviewer-role') && ( + + )}    [ { key: 'owner', label: t('make_owner') }, { key: 'readAndWrite', label: t('editor') }, + { key: 'review', label: t('reviewer') }, { key: 'readOnly', label: t('viewer') }, { key: 'removeAccess', label: t('remove_access') }, ], diff --git a/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/member-privileges.jsx b/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/member-privileges.jsx index d49a9ea479..a968bfc5fd 100644 --- a/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/member-privileges.jsx +++ b/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/member-privileges.jsx @@ -11,6 +11,9 @@ export default function MemberPrivileges({ privileges }) { case 'readOnly': return t('read_only') + case 'review': + return t('can_review') + default: return null } diff --git a/services/web/locales/en.json b/services/web/locales/en.json index fa0d0d536e..5541f67d88 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -239,6 +239,7 @@ "can_link_institution_email_to_login": "You can link your __email__ __appName__ account to your __institutionName__ account, which will allow you to log in to __appName__ through your institution and will reconfirm your institutional email address.", "can_link_your_institution_acct_2": "You can now <0>link your <0>__appName__ account to your <0>__institutionName__ institutional account.", "can_now_relink_dropbox": "You can now <0>relink your Dropbox account.", + "can_review": "Can review", "can_view": "Can view", "cancel": "Cancel", "cancel_add_on": "Cancel add-on", diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsGetterTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsGetterTests.js index 2bb3e5907c..1f09acdc78 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsGetterTests.js +++ b/services/web/test/unit/src/Collaborators/CollaboratorsGetterTests.js @@ -20,6 +20,8 @@ describe('CollaboratorsGetter', function () { this.pendingEditorRef = new ObjectId() this.readWriteRef1 = new ObjectId() this.readWriteRef2 = new ObjectId() + this.reviewer1Ref = new ObjectId() + this.reviewer2Ref = new ObjectId() this.readOnlyTokenRef = new ObjectId() this.readWriteTokenRef = new ObjectId() this.nonMemberRef = new ObjectId() @@ -33,6 +35,7 @@ describe('CollaboratorsGetter', function () { ], pendingEditor_refs: [this.pendingEditorRef], collaberator_refs: [this.readWriteRef1, this.readWriteRef2], + reviewer_refs: [this.reviewer1Ref, this.reviewer2Ref], tokenAccessReadAndWrite_refs: [this.readWriteTokenRef], tokenAccessReadOnly_refs: [this.readOnlyTokenRef], publicAccesLevel: 'tokenBased', @@ -121,6 +124,16 @@ describe('CollaboratorsGetter', function () { privilegeLevel: 'readAndWrite', source: 'token', }, + { + id: this.reviewer1Ref.toString(), + privilegeLevel: 'review', + source: 'invite', + }, + { + id: this.reviewer2Ref.toString(), + privilegeLevel: 'review', + source: 'invite', + }, ]) }) }) @@ -154,6 +167,8 @@ describe('CollaboratorsGetter', function () { this.pendingEditorRef.toString(), this.readWriteTokenRef.toString(), this.readOnlyTokenRef.toString(), + this.reviewer1Ref.toString(), + this.reviewer2Ref.toString(), ]) }) }) @@ -171,6 +186,8 @@ describe('CollaboratorsGetter', function () { this.readWriteRef1.toString(), this.readWriteRef2.toString(), this.pendingEditorRef.toString(), + this.reviewer1Ref.toString(), + this.reviewer2Ref.toString(), ]) }) }) @@ -189,6 +206,9 @@ describe('CollaboratorsGetter', function () { this.UserGetter.promises.getUser .withArgs(this.readWriteTokenRef.toString()) .resolves({ _id: this.readWriteTokenRef }) + this.UserGetter.promises.getUser + .withArgs(this.reviewer1Ref.toString()) + .resolves({ _id: this.reviewer1Ref }) }) it('should return an array of invited members with their privilege levels', async function () { @@ -199,6 +219,7 @@ describe('CollaboratorsGetter', function () { expect(result).to.have.deep.members([ { user: { _id: this.readOnlyRef1 }, privilegeLevel: 'readOnly' }, { user: { _id: this.readWriteRef2 }, privilegeLevel: 'readAndWrite' }, + { user: { _id: this.reviewer1Ref }, privilegeLevel: 'review' }, ]) }) }) @@ -213,6 +234,15 @@ describe('CollaboratorsGetter', function () { expect(level).to.equal('readOnly') }) + it('should return review privilege level', async function () { + const level = + await this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel( + this.reviewer1Ref, + this.project._id + ) + expect(level).to.equal('review') + }) + it('should return false if the member has no privilege level', async function () { const level = await this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel( @@ -352,15 +382,21 @@ describe('CollaboratorsGetter', function () { _id: this.readWriteRef1, email: 'readwrite@example.com', } + this.reviewUser = { + _id: this.reviewer1Ref, + email: 'review@example.com', + } this.members = [ { user: this.owningUser, privilegeLevel: 'owner' }, { user: this.readWriteUser, privilegeLevel: 'readAndWrite' }, + { user: this.reviewUser, privilegeLevel: 'review' }, ] this.views = { owner: this.owningUser, ownerFeatures: this.owningUser.features, members: [ { _id: this.readWriteUser._id, email: this.readWriteUser.email }, + { _id: this.reviewUser._id, email: this.reviewUser.email }, ], } this.UserGetter.promises.getUser @@ -369,6 +405,9 @@ describe('CollaboratorsGetter', function () { this.UserGetter.promises.getUser .withArgs(this.readWriteUser._id.toString()) .resolves(this.readWriteUser) + this.UserGetter.promises.getUser + .withArgs(this.reviewUser._id.toString()) + .resolves(this.reviewUser) this.ProjectEditorHandler.buildOwnerAndMembersViews.returns(this.views) this.result = await this.CollaboratorsGetter.promises.getAllInvitedMembers( diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js index a36b438578..aec7ab0e98 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js +++ b/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js @@ -65,8 +65,10 @@ describe('CollaboratorsHandler', function () { this.CollaboratorsGetter = { promises: { dangerouslyGetAllProjectsUserIsMemberOf: sinon.stub(), + getMemberIdsWithPrivilegeLevels: sinon.stub().resolves([]), }, } + this.EditorRealTimeController = { emitToRoom: sinon.stub() } this.CollaboratorsHandler = SandboxedModule.require(MODULE_PATH, { requires: { '../User/UserGetter': this.UserGetter, @@ -76,6 +78,7 @@ describe('CollaboratorsHandler', function () { '../ThirdPartyDataStore/TpdsUpdateSender': this.TpdsUpdateSender, '../Project/ProjectGetter': this.ProjectGetter, '../Project/ProjectHelper': this.ProjectHelper, + '../Editor/EditorRealTimeController': this.EditorRealTimeController, './CollaboratorsGetter': this.CollaboratorsGetter, }, }) @@ -105,6 +108,7 @@ describe('CollaboratorsHandler', function () { { $pull: { collaberator_refs: this.userId, + reviewer_refs: this.userId, readOnly_refs: this.userId, pendingEditor_refs: this.userId, tokenAccessReadOnly_refs: this.userId, @@ -148,6 +152,7 @@ describe('CollaboratorsHandler', function () { }, $pull: { collaberator_refs: this.userId, + reviewer_refs: this.userId, readOnly_refs: this.userId, pendingEditor_refs: this.userId, tokenAccessReadOnly_refs: this.userId, @@ -183,6 +188,7 @@ describe('CollaboratorsHandler', function () { { $pull: { collaberator_refs: this.userId, + reviewer_refs: this.userId, readOnly_refs: this.userId, pendingEditor_refs: this.userId, tokenAccessReadOnly_refs: this.userId, @@ -302,6 +308,43 @@ describe('CollaboratorsHandler', function () { }) }) + describe('as reviewer', function () { + beforeEach(async function () { + this.ProjectMock.expects('updateOne') + .withArgs( + { + _id: this.project._id, + }, + { + track_changes: { [this.userId]: true }, + $addToSet: { reviewer_refs: this.userId }, + } + ) + .chain('exec') + .resolves() + await this.CollaboratorsHandler.promises.addUserIdToProject( + this.project._id, + this.addingUserId, + this.userId, + 'review' + ) + }) + + it('should update the client with new track changes settings', function () { + return this.EditorRealTimeController.emitToRoom + .calledWith(this.project._id, 'toggle-track-changes', { + [this.userId]: true, + }) + .should.equal(true) + }) + + it('should flush the project to the TPDS', function () { + expect( + this.TpdsProjectFlusher.promises.flushProjectToTpds + ).to.have.been.calledWith(this.project._id) + }) + }) + describe('with invalid privilegeLevel', function () { it('should call the callback with an error', async function () { await expect( @@ -405,6 +448,7 @@ describe('CollaboratorsHandler', function () { { $pull: { collaberator_refs: this.userId, + reviewer_refs: this.userId, readOnly_refs: this.userId, pendingEditor_refs: this.userId, tokenAccessReadOnly_refs: this.userId,