diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js b/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js index 7247c9170d..2a696beaff 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js @@ -284,39 +284,20 @@ async function setCollaboratorPrivilegeLevel( // collaborator const query = { _id: projectId, - $or: [ - { collaberator_refs: userId }, - { readOnly_refs: userId }, - { reviewer_refs: userId }, - ], + $or: [{ collaberator_refs: userId }, { readOnly_refs: userId }], } let update switch (privilegeLevel) { case PrivilegeLevels.READ_AND_WRITE: { update = { - $pull: { - readOnly_refs: userId, - pendingEditor_refs: userId, - reviewer_refs: userId, - }, + $pull: { readOnly_refs: userId, pendingEditor_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, reviewer_refs: userId }, + $pull: { collaberator_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 a057e1d8fe..4ff5097124 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsRouter.mjs +++ b/services/web/app/src/Features/Collaborators/CollaboratorsRouter.mjs @@ -50,11 +50,7 @@ export default { }), body: Joi.object({ privilegeLevel: Joi.string() - .valid( - PrivilegeLevels.READ_ONLY, - PrivilegeLevels.READ_AND_WRITE, - PrivilegeLevels.REVIEW - ) + .valid(PrivilegeLevels.READ_ONLY, PrivilegeLevels.READ_AND_WRITE) .required(), }), }), diff --git a/services/web/app/src/Features/Helpers/AuthorizationHelper.js b/services/web/app/src/Features/Helpers/AuthorizationHelper.js index d8e6bd932d..8369f2d321 100644 --- a/services/web/app/src/Features/Helpers/AuthorizationHelper.js +++ b/services/web/app/src/Features/Helpers/AuthorizationHelper.js @@ -1,14 +1,7 @@ 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) { @@ -21,28 +14,3 @@ 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/frontend/js/features/share-project-modal/components/restricted-link-sharing/edit-member.tsx b/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/edit-member.tsx index 635073b840..1189176b42 100644 --- a/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/edit-member.tsx +++ b/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/edit-member.tsx @@ -92,7 +92,6 @@ export default function EditMember({ ) } else if ( newPrivileges === 'readAndWrite' || - newPrivileges === 'review' || newPrivileges === 'readOnly' ) { monitorRequest(() => diff --git a/services/web/locales/en.json b/services/web/locales/en.json index bd125a15ec..3a1b73bda6 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -1824,7 +1824,6 @@ "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 f644c39180..aa537d76b6 100644 --- a/services/web/test/acceptance/src/SharingTests.mjs +++ b/services/web/test/acceptance/src/SharingTests.mjs @@ -8,15 +8,12 @@ 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') }) @@ -38,19 +35,6 @@ 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 () { @@ -61,7 +45,6 @@ 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]) }) @@ -113,40 +96,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]) }) }) - - 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 75a8d287ac..fa4ab677ee 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/services/web/test/acceptance/src/helpers/User.js @@ -968,10 +968,6 @@ 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/CollaboratorsHandlerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js index 8e14590b0f..aec7ab0e98 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js +++ b/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js @@ -590,14 +590,12 @@ 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 }, } @@ -619,14 +617,12 @@ 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, }, } @@ -640,35 +636,6 @@ 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( @@ -677,7 +644,6 @@ describe('CollaboratorsHandler', function () { $or: [ { collaberator_refs: this.userId }, { readOnly_refs: this.userId }, - { reviewer_refs: this.userId }, ], }, { @@ -687,7 +653,6 @@ 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 4aa21ef7a9..7a887e2beb 100644 --- a/services/web/test/unit/src/HelperFiles/AuthorizationHelperTests.js +++ b/services/web/test/unit/src/HelperFiles/AuthorizationHelperTests.js @@ -25,10 +25,6 @@ describe('AuthorizationHelper', function () { }, }, }, - '../Project/ProjectGetter': (this.ProjectGetter = { promises: {} }), - '../SplitTests/SplitTestHandler': (this.SplitTestHandler = { - promises: {}, - }), }, }) }) @@ -63,76 +59,4 @@ 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 - }) - }) })