diff --git a/services/real-time/app/js/AuthorizationManager.js b/services/real-time/app/js/AuthorizationManager.js index 01e99810bb..261e2e91ee 100644 --- a/services/real-time/app/js/AuthorizationManager.js +++ b/services/real-time/app/js/AuthorizationManager.js @@ -5,7 +5,7 @@ module.exports = AuthorizationManager = { assertClientCanViewProject(client, callback) { AuthorizationManager._assertClientHasPrivilegeLevel( client, - ['readOnly', 'readAndWrite', 'owner'], + ['readOnly', 'readAndWrite', 'review', 'owner'], callback ) }, diff --git a/services/real-time/test/unit/js/AuthorizationManagerTests.js b/services/real-time/test/unit/js/AuthorizationManagerTests.js index e2d296624a..57882eaacc 100644 --- a/services/real-time/test/unit/js/AuthorizationManagerTests.js +++ b/services/real-time/test/unit/js/AuthorizationManagerTests.js @@ -47,6 +47,17 @@ describe('AuthorizationManager', function () { ) }) + it('should allow the review privilegeLevel', function (done) { + this.client.ol_context.privilege_level = 'review' + return this.AuthorizationManager.assertClientCanViewProject( + this.client, + error => { + expect(error).to.be.null + return done() + } + ) + }) + it('should allow the owner privilegeLevel', function (done) { this.client.ol_context.privilege_level = 'owner' return this.AuthorizationManager.assertClientCanViewProject( 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..c2e1163b65 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 } @@ -222,9 +224,10 @@ async function getPublicShareTokens(userId, projectId) { // token access has been disabled. async function getProjectsUserIsMemberOf(userId, fields) { const limit = pLimit(2) - const [readAndWrite, readOnly, tokenReadAndWrite, tokenReadOnly] = + const [readAndWrite, review, readOnly, tokenReadAndWrite, tokenReadOnly] = await Promise.all([ limit(() => Project.find({ collaberator_refs: userId }, fields).exec()), + limit(() => Project.find({ reviewer_refs: userId }, fields).exec()), limit(() => Project.find({ readOnly_refs: userId }, fields).exec()), limit(() => Project.find( @@ -245,7 +248,7 @@ async function getProjectsUserIsMemberOf(userId, fields) { ).exec() ), ]) - return { readAndWrite, readOnly, tokenReadAndWrite, tokenReadOnly } + return { readAndWrite, review, readOnly, tokenReadAndWrite, tokenReadOnly } } // This function returns all the projects that a user is a member of, regardless of @@ -324,7 +327,8 @@ function _getMemberIdsWithPrivilegeLevelsFromFields( tokenAccessIds, tokenAccessReadOnlyIds, publicAccessLevel, - pendingEditorIds + pendingEditorIds, + reviewerIds ) { const members = [] members.push({ @@ -365,6 +369,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..7247c9170d 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({ @@ -237,20 +284,39 @@ async function setCollaboratorPrivilegeLevel( // collaborator const query = { _id: projectId, - $or: [{ collaberator_refs: userId }, { readOnly_refs: userId }], + $or: [ + { collaberator_refs: userId }, + { readOnly_refs: userId }, + { reviewer_refs: userId }, + ], } let update switch (privilegeLevel) { case PrivilegeLevels.READ_AND_WRITE: { update = { - $pull: { readOnly_refs: userId, pendingEditor_refs: userId }, + $pull: { + readOnly_refs: userId, + pendingEditor_refs: userId, + reviewer_refs: userId, + }, $addToSet: { collaberator_refs: userId }, } break } + case PrivilegeLevels.REVIEW: { + update = { + $pull: { + readOnly_refs: userId, + pendingEditor_refs: userId, + collaberator_refs: userId, + }, + $addToSet: { reviewer_refs: userId }, + } + break + } case PrivilegeLevels.READ_ONLY: { update = { - $pull: { collaberator_refs: userId }, + $pull: { collaberator_refs: userId, reviewer_refs: userId }, $addToSet: { readOnly_refs: userId }, } if (pendingEditor) { diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsRouter.mjs b/services/web/app/src/Features/Collaborators/CollaboratorsRouter.mjs index 4ff5097124..a057e1d8fe 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsRouter.mjs +++ b/services/web/app/src/Features/Collaborators/CollaboratorsRouter.mjs @@ -50,7 +50,11 @@ export default { }), body: Joi.object({ privilegeLevel: Joi.string() - .valid(PrivilegeLevels.READ_ONLY, PrivilegeLevels.READ_AND_WRITE) + .valid( + PrivilegeLevels.READ_ONLY, + PrivilegeLevels.READ_AND_WRITE, + PrivilegeLevels.REVIEW + ) .required(), }), }), diff --git a/services/web/app/src/Features/Helpers/AuthorizationHelper.js b/services/web/app/src/Features/Helpers/AuthorizationHelper.js index 8369f2d321..d8e6bd932d 100644 --- a/services/web/app/src/Features/Helpers/AuthorizationHelper.js +++ b/services/web/app/src/Features/Helpers/AuthorizationHelper.js @@ -1,7 +1,14 @@ const { UserSchema } = require('../../models/User') +const SplitTestHandler = require('../SplitTests/SplitTestHandler') +const ProjectGetter = require('../Project/ProjectGetter') +const { callbackify } = require('@overleaf/promise-utils') module.exports = { hasAnyStaffAccess, + isReviewerRoleEnabled: callbackify(isReviewerRoleEnabled), + promises: { + isReviewerRoleEnabled, + }, } function hasAnyStaffAccess(user) { @@ -14,3 +21,28 @@ function hasAnyStaffAccess(user) { } return false } + +async function isReviewerRoleEnabled(userId, projectId) { + const project = await ProjectGetter.promises.getProject(projectId, { + reviewer_refs: 1, + owner_ref: 1, + }) + + // if there are reviewers, it means the role is enabled + if (Object.keys(project.reviewer_refs || {}).length > 0) { + return true + } + + // if there are no reviewers, check split test from project owner + if (project.owner_ref === userId) { + const reviewerRoleAssigment = + await SplitTestHandler.promises.getAssignmentForUser( + userId, + 'reviewer-role' + ) + + return reviewerRoleAssigment.variant === 'enabled' + } + + return false +} diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index 1f44051c32..c6ab071a75 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', ].filter(Boolean) const getUserValues = async userId => @@ -894,8 +895,14 @@ const _ProjectController = { }, _buildProjectList(allProjects, userId) { let project - const { owned, readAndWrite, readOnly, tokenReadAndWrite, tokenReadOnly } = - allProjects + const { + owned, + review, + readAndWrite, + readOnly, + tokenReadAndWrite, + tokenReadOnly, + } = allProjects const projects = [] for (project of owned) { projects.push( @@ -918,6 +925,16 @@ const _ProjectController = { ) ) } + for (project of review) { + projects.push( + ProjectController._buildProjectViewModel( + project, + 'review', + Sources.INVITE, + userId + ) + ) + } for (project of readOnly) { projects.push( ProjectController._buildProjectViewModel( diff --git a/services/web/app/src/Features/Project/ProjectGetter.js b/services/web/app/src/Features/Project/ProjectGetter.js index 55e400e0f5..7edf08d7a2 100644 --- a/services/web/app/src/Features/Project/ProjectGetter.js +++ b/services/web/app/src/Features/Project/ProjectGetter.js @@ -102,6 +102,7 @@ const ProjectGetter = { readOnly: projects.readOnly || [], tokenReadAndWrite: projects.tokenReadAndWrite || [], tokenReadOnly: projects.tokenReadOnly || [], + review: projects.review || [], } // Remove duplicate projects. The order of result values is determined by the order they occur. diff --git a/services/web/app/src/Features/Project/ProjectListController.mjs b/services/web/app/src/Features/Project/ProjectListController.mjs index 8c173e0f6d..95c34e049b 100644 --- a/services/web/app/src/Features/Project/ProjectListController.mjs +++ b/services/web/app/src/Features/Project/ProjectListController.mjs @@ -549,8 +549,14 @@ async function _getProjects( * @private */ function _formatProjects(projects, userId) { - const { owned, readAndWrite, readOnly, tokenReadAndWrite, tokenReadOnly } = - projects + const { + owned, + review, + readAndWrite, + readOnly, + tokenReadAndWrite, + tokenReadOnly, + } = projects const formattedProjects = /** @type {Project[]} **/ [] for (const project of owned) { @@ -564,6 +570,11 @@ function _formatProjects(projects, userId) { _formatProjectInfo(project, 'readWrite', Sources.INVITE, userId) ) } + for (const project of review) { + formattedProjects.push( + _formatProjectInfo(project, 'review', Sources.INVITE, userId) + ) + } for (const project of readOnly) { formattedProjects.push( _formatProjectInfo(project, 'readOnly', Sources.INVITE, userId) 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 c57f04bcc6..aa77aa94df 100644 --- a/services/web/frontend/extracted-translations.json +++ b/services/web/frontend/extracted-translations.json @@ -171,6 +171,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": "", @@ -1280,6 +1281,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/context/review-panel/hooks/use-review-panel-state.ts b/services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts index 082f466824..be12f5c3f9 100644 --- a/services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts +++ b/services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts @@ -814,7 +814,10 @@ function useReviewPanelState(): ReviewPanel.ReviewPanelState { } const members = project.members ?? [] for (const member of members) { - if (member.privileges === 'readAndWrite') { + if ( + member.privileges === 'readAndWrite' || + member.privileges === 'review' + ) { if (!trackChangesState[member._id]) { // An added member will have track changes enabled if track changes is on for everyone setUserTCState( 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') && ( + + )}    @@ -237,6 +238,7 @@ function SelectPrivilege({ (): Privilege[] => [ { 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/frontend/js/shared/context/types/project-context.tsx b/services/web/frontend/js/shared/context/types/project-context.tsx index 7eedd18654..556ba8d296 100644 --- a/services/web/frontend/js/shared/context/types/project-context.tsx +++ b/services/web/frontend/js/shared/context/types/project-context.tsx @@ -3,7 +3,7 @@ import { PublicAccessLevel } from '../../../../../types/public-access-level' export type ProjectContextMember = { _id: UserId - privileges: 'readOnly' | 'readAndWrite' + privileges: 'readOnly' | 'readAndWrite' | 'review' email: string first_name: string last_name: string diff --git a/services/web/locales/en.json b/services/web/locales/en.json index c0ef3860d5..72ca8bf3bf 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -233,6 +233,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", @@ -1804,6 +1805,7 @@ "revert_pending_plan_change": "Revert scheduled plan change", "review": "Review", "review_your_peers_work": "Review your peers’ work", + "reviewer": "Reviewer", "revoke": "Revoke", "revoke_invite": "Revoke Invite", "right": "Right", diff --git a/services/web/test/acceptance/src/SharingTests.mjs b/services/web/test/acceptance/src/SharingTests.mjs index aa537d76b6..f644c39180 100644 --- a/services/web/test/acceptance/src/SharingTests.mjs +++ b/services/web/test/acceptance/src/SharingTests.mjs @@ -8,12 +8,15 @@ describe('Sharing', function () { this.ownerSession = new User() this.collaboratorSession = new User() this.strangerSession = new User() + this.reviewerSession = new User() await this.ownerSession.login() await this.collaboratorSession.login() await this.strangerSession.login() + await this.reviewerSession.login() this.owner = await this.ownerSession.get() this.collaborator = await this.collaboratorSession.get() this.stranger = await this.strangerSession.get() + this.reviewer = await this.reviewerSession.get() this.projectId = await this.ownerSession.createProject('Test project') }) @@ -35,6 +38,19 @@ describe('Sharing', function () { const project = await this.ownerSession.getProject(this.projectId) expect(project.collaberator_refs).to.deep.equal([this.collaborator._id]) expect(project.readOnly_refs).to.deep.equal([]) + expect(project.reviewer_refs).to.deep.equal([]) + }) + + it('sets the privilege level to review', async function () { + await this.ownerSession.setCollaboratorInfo( + this.projectId, + this.collaborator._id, + { privilegeLevel: 'review' } + ) + const project = await this.ownerSession.getProject(this.projectId) + expect(project.reviewer_refs).to.deep.equal([this.collaborator._id]) + expect(project.collaberator_refs).to.deep.equal([]) + expect(project.readOnly_refs).to.deep.equal([]) }) it('treats setting the privilege to read-only as a noop', async function () { @@ -45,6 +61,7 @@ describe('Sharing', function () { ) const project = await this.ownerSession.getProject(this.projectId) expect(project.collaberator_refs).to.deep.equal([]) + expect(project.reviewer_refs).to.deep.equal([]) expect(project.readOnly_refs).to.deep.equal([this.collaborator._id]) }) @@ -96,7 +113,40 @@ describe('Sharing', function () { ) const project = await this.ownerSession.getProject(this.projectId) expect(project.collaberator_refs).to.deep.equal([]) + expect(project.reviewer_refs).to.deep.equal([]) expect(project.readOnly_refs).to.deep.equal([this.collaborator._id]) }) }) + + describe('with reviewer collaborator', function () { + beforeEach(async function () { + await this.ownerSession.addUserToProject( + this.projectId, + this.reviewer, + 'review' + ) + }) + + it('prevents non-owners to set the privilege level', async function () { + await expect( + this.collaboratorSession.setCollaboratorInfo( + this.projectId, + this.reviewer._id, + { privilegeLevel: 'review' } + ) + ).to.be.rejectedWith(/failed: status=403 /) + }) + + it('sets the privilege level to read-only', async function () { + await this.ownerSession.setCollaboratorInfo( + this.projectId, + this.reviewer._id, + { privilegeLevel: 'readOnly' } + ) + const project = await this.ownerSession.getProject(this.projectId) + expect(project.collaberator_refs).to.deep.equal([]) + expect(project.reviewer_refs).to.deep.equal([]) + expect(project.readOnly_refs).to.deep.equal([this.reviewer._id]) + }) + }) }) diff --git a/services/web/test/acceptance/src/helpers/User.js b/services/web/test/acceptance/src/helpers/User.js index fa4ab677ee..75a8d287ac 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/services/web/test/acceptance/src/helpers/User.js @@ -968,6 +968,10 @@ class User { updateOp = { $addToSet: { readOnly_refs: user._id, pendingEditor_refs: user._id }, } + } else if (privileges === 'review') { + updateOp = { + $addToSet: { reviewer_refs: user._id }, + } } db.projects.updateOne({ _id: new ObjectId(projectId) }, updateOp, callback) } diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsGetterTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsGetterTests.js index 2bb3e5907c..0547ef4062 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( @@ -289,6 +319,11 @@ describe('CollaboratorsGetter', function () { .withArgs({ readOnly_refs: this.userId }, this.fields) .chain('exec') .resolves(['mock-read-only-project-1', 'mock-read-only-project-2']) + + this.ProjectMock.expects('find') + .withArgs({ reviewer_refs: this.userId }, this.fields) + .chain('exec') + .resolves(['mock-review-project-1', 'mock-review-project-2']) this.ProjectMock.expects('find') .withArgs( { @@ -337,6 +372,7 @@ describe('CollaboratorsGetter', function () { 'mock-token-read-only-project-1', 'mock-token-read-only-project-2', ], + review: ['mock-review-project-1', 'mock-review-project-2'], }) }) }) @@ -352,15 +388,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 +411,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..8e14590b0f 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, @@ -546,12 +590,14 @@ describe('CollaboratorsHandler', function () { $or: [ { collaberator_refs: this.userId }, { readOnly_refs: this.userId }, + { reviewer_refs: this.userId }, ], }, { $pull: { collaberator_refs: this.userId, pendingEditor_refs: this.userId, + reviewer_refs: this.userId, }, $addToSet: { readOnly_refs: this.userId }, } @@ -573,12 +619,14 @@ describe('CollaboratorsHandler', function () { $or: [ { collaberator_refs: this.userId }, { readOnly_refs: this.userId }, + { reviewer_refs: this.userId }, ], }, { $addToSet: { collaberator_refs: this.userId }, $pull: { readOnly_refs: this.userId, + reviewer_refs: this.userId, pendingEditor_refs: this.userId, }, } @@ -592,6 +640,35 @@ describe('CollaboratorsHandler', function () { ) }) + it('sets a collaborator to reviewer', async function () { + this.ProjectMock.expects('updateOne') + .withArgs( + { + _id: this.projectId, + $or: [ + { collaberator_refs: this.userId }, + { readOnly_refs: this.userId }, + { reviewer_refs: this.userId }, + ], + }, + { + $addToSet: { reviewer_refs: this.userId }, + $pull: { + readOnly_refs: this.userId, + collaberator_refs: this.userId, + pendingEditor_refs: this.userId, + }, + } + ) + .chain('exec') + .resolves({ matchedCount: 1 }) + await this.CollaboratorsHandler.promises.setCollaboratorPrivilegeLevel( + this.projectId, + this.userId, + 'review' + ) + }) + it('sets a collaborator to read-only as a pendingEditor', async function () { this.ProjectMock.expects('updateOne') .withArgs( @@ -600,6 +677,7 @@ describe('CollaboratorsHandler', function () { $or: [ { collaberator_refs: this.userId }, { readOnly_refs: this.userId }, + { reviewer_refs: this.userId }, ], }, { @@ -609,6 +687,7 @@ describe('CollaboratorsHandler', function () { }, $pull: { collaberator_refs: this.userId, + reviewer_refs: this.userId, }, } ) diff --git a/services/web/test/unit/src/HelperFiles/AuthorizationHelperTests.js b/services/web/test/unit/src/HelperFiles/AuthorizationHelperTests.js index 7a887e2beb..4aa21ef7a9 100644 --- a/services/web/test/unit/src/HelperFiles/AuthorizationHelperTests.js +++ b/services/web/test/unit/src/HelperFiles/AuthorizationHelperTests.js @@ -25,6 +25,10 @@ describe('AuthorizationHelper', function () { }, }, }, + '../Project/ProjectGetter': (this.ProjectGetter = { promises: {} }), + '../SplitTests/SplitTestHandler': (this.SplitTestHandler = { + promises: {}, + }), }, }) }) @@ -59,4 +63,76 @@ describe('AuthorizationHelper', function () { expect(this.AuthorizationHelper.hasAnyStaffAccess(user)).to.be.false }) }) + + describe('isReviewerRoleEnabled', function () { + it('with no reviewers and no split test', async function () { + this.ProjectGetter.promises.getProject = sinon.stub().resolves({ + reviewer_refs: {}, + owner_ref: 'ownerId', + }) + this.SplitTestHandler.promises.getAssignmentForUser = sinon + .stub() + .resolves({ + variant: 'disabled', + }) + expect( + await this.AuthorizationHelper.promises.isReviewerRoleEnabled( + 'userId', + 'projectId' + ) + ).to.be.false + }) + + it('with no reviewers and enabled split test', async function () { + this.ProjectGetter.promises.getProject = sinon.stub().resolves({ + reviewer_refs: {}, + owner_ref: 'userId', + }) + this.SplitTestHandler.promises.getAssignmentForUser = sinon + .stub() + .resolves({ + variant: 'enabled', + }) + expect( + await this.AuthorizationHelper.promises.isReviewerRoleEnabled( + 'userId', + 'projectId' + ) + ).to.be.true + }) + + it('with reviewers and disabled split test', async function () { + this.ProjectGetter.promises.getProject = sinon.stub().resolves({ + reviewer_refs: [{ $oid: 'userId' }], + }) + this.SplitTestHandler.promises.getAssignmentForUser = sinon + .stub() + .resolves({ + variant: 'default', + }) + expect( + await this.AuthorizationHelper.promises.isReviewerRoleEnabled( + 'userId', + 'projectId' + ) + ).to.be.true + }) + + it('with reviewers and enabled split test', async function () { + this.ProjectGetter.promises.getProject = sinon.stub().resolves({ + reviewer_refs: [{ $oid: 'userId' }], + }) + this.SplitTestHandler.promises.getAssignmentForUser = sinon + .stub() + .resolves({ + variant: 'enabled', + }) + expect( + await this.AuthorizationHelper.promises.isReviewerRoleEnabled( + 'userId', + 'projectId' + ) + ).to.be.true + }) + }) }) diff --git a/services/web/test/unit/src/Project/ProjectGetterTests.js b/services/web/test/unit/src/Project/ProjectGetterTests.js index 1aecd4ab11..8f58b8a10b 100644 --- a/services/web/test/unit/src/Project/ProjectGetterTests.js +++ b/services/web/test/unit/src/Project/ProjectGetterTests.js @@ -275,6 +275,7 @@ describe('ProjectGetter', function () { this.fields = { mock: 'fields' } this.projectOwned = { _id: 'mock-owned-projects' } this.projectRW = { _id: 'mock-rw-projects' } + this.projectReview = { _id: 'mock-review-projects' } this.projectRO = { _id: 'mock-ro-projects' } this.projectTokenRW = { _id: 'mock-token-rw-projects' } this.projectTokenRO = { _id: 'mock-token-ro-projects' } @@ -289,6 +290,7 @@ describe('ProjectGetter', function () { readOnly: [this.projectRO], tokenReadAndWrite: [this.projectTokenRW], tokenReadOnly: [this.projectTokenRO], + review: [this.projectReview], }) const projects = await this.ProjectGetter.promises.findAllUsersProjects( this.userId, @@ -301,6 +303,7 @@ describe('ProjectGetter', function () { readOnly: [this.projectRO], tokenReadAndWrite: [this.projectTokenRW], tokenReadOnly: [this.projectTokenRO], + review: [this.projectReview], }) }) @@ -314,6 +317,7 @@ describe('ProjectGetter', function () { this.projectTokenRO, this.projectRO, ], + review: [this.projectReview], }) const projects = await this.ProjectGetter.promises.findAllUsersProjects( this.userId, @@ -326,6 +330,7 @@ describe('ProjectGetter', function () { readOnly: [this.projectRO], tokenReadAndWrite: [this.projectTokenRW], tokenReadOnly: [this.projectTokenRO], + review: [this.projectReview], }) }) }) diff --git a/services/web/test/unit/src/Project/ProjectListControllerTests.mjs b/services/web/test/unit/src/Project/ProjectListControllerTests.mjs index 478845f846..74065db753 100644 --- a/services/web/test/unit/src/Project/ProjectListControllerTests.mjs +++ b/services/web/test/unit/src/Project/ProjectListControllerTests.mjs @@ -219,12 +219,14 @@ describe('ProjectListController', function () { this.readOnly = [{ _id: 3, lastUpdated: 3, owner_ref: 'user-1' }] this.tokenReadAndWrite = [{ _id: 6, lastUpdated: 5, owner_ref: 'user-4' }] this.tokenReadOnly = [{ _id: 7, lastUpdated: 4, owner_ref: 'user-5' }] + this.review = [{ _id: 8, lastUpdated: 4, owner_ref: 'user-6' }] this.allProjects = { owned: this.projects, readAndWrite: this.readAndWrite, readOnly: this.readOnly, tokenReadAndWrite: this.tokenReadAndWrite, tokenReadOnly: this.tokenReadOnly, + review: this.review, } this.ProjectGetter.promises.findAllUsersProjects.resolves( @@ -279,7 +281,8 @@ describe('ProjectListController', function () { this.readAndWrite.length + this.readOnly.length + this.tokenReadAndWrite.length + - this.tokenReadOnly.length + this.tokenReadOnly.length + + this.review.length ) done() } @@ -719,12 +722,14 @@ describe('ProjectListController', function () { { _id: 6, lastUpdated: 5, owner_ref: 'user-4' }, // Also in tokenReadAndWrite { _id: 7, lastUpdated: 4, owner_ref: 'user-5' }, ] + this.review = [{ _id: 8, lastUpdated: 5, owner_ref: 'user-6' }] this.allProjects = { owned: this.projects, readAndWrite: this.readAndWrite, readOnly: this.readOnly, tokenReadAndWrite: this.tokenReadAndWrite, tokenReadOnly: this.tokenReadOnly, + review: this.review, } this.ProjectGetter.promises.findAllUsersProjects.resolves( @@ -747,7 +752,8 @@ describe('ProjectListController', function () { this.readAndWrite.length + this.readOnly.length + this.tokenReadAndWrite.length + - this.tokenReadOnly.length - + this.tokenReadOnly.length + + this.review.length - 1 ) done()