Set track changes state permissions for reviewer role (#22345)

* Support for adding reviewer role

* added collaboratorsGetter tests

* emit toggle-track-changes when reviewer is added

* Add reviewer in change privilege level handler

GitOrigin-RevId: 6a03d2355b3fb7d7b755ed1d3ab1a080126cd2dc

services/web/app/src/Features/Collaborators/CollaboratorsHandler.js

@@ -284,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) {

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(),
         }),
       }),

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
+}

services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/edit-member.tsx

@@ -92,6 +92,7 @@ export default function EditMember({
       )
     } else if (
       newPrivileges === 'readAndWrite' ||
+      newPrivileges === 'review' ||
       newPrivileges === 'readOnly'
     ) {
       monitorRequest(() =>

services/web/locales/en.json

@@ -1824,6 +1824,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",

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])
+    })
+  })
 })

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)
   }

services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js

@@ -590,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 },
           }
@@ -617,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,
             },
           }
@@ -636,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(
@@ -644,6 +677,7 @@ describe('CollaboratorsHandler', function () {
             $or: [
               { collaberator_refs: this.userId },
               { readOnly_refs: this.userId },
+              { reviewer_refs: this.userId },
             ],
           },
           {
@@ -653,6 +687,7 @@ describe('CollaboratorsHandler', function () {
             },
             $pull: {
               collaberator_refs: this.userId,
+              reviewer_refs: this.userId,
             },
           }
         )

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
+    })
+  })
 })