Merge pull request #24357 from overleaf/em-reviewers-limit

Count reviewers towards the collaborator limit in the backend GitOrigin-RevId: 352fa37023ffbb032e23a474a81897ca871fee69
2025-03-24 10:51:14 +00:00
parent e4dae982d2
commit 1fb94dee18
6 changed files with 301 additions and 269 deletions
@@ -1,5 +1,4 @@
 const SandboxedModule = require('sandboxed-module')
-const assert = require('assert')
 const sinon = require('sinon')
 const { expect } = require('chai')
 const modulePath = require('path').join(
@@ -50,13 +49,14 @@ describe('LimitationsManager', function () {

    this.CollaboratorsGetter = {
      promises: {
-        getInvitedEditCollaboratorCount: sinon.stub().resolves(),
+        getInvitedEditCollaboratorCount: sinon.stub().resolves(0),
+        getMemberIdPrivilegeLevel: sinon.stub(),
      },
    }

    this.CollaboratorsInviteGetter = {
      promises: {
-        getEditInviteCount: sinon.stub().resolves(),
+        getEditInviteCount: sinon.stub().resolves(0),
      },
    }

@@ -76,76 +76,64 @@ describe('LimitationsManager', function () {

  describe('allowedNumberOfCollaboratorsInProject', function () {
    describe('when the project is owned by a user without a subscription', function () {
-      beforeEach(function (done) {
+      beforeEach(function () {
        this.Settings.defaultFeatures = { collaborators: 23 }
        this.project.owner_ref = this.userId
        delete this.user.features
-        this.callback = sinon.stub().callsFake(() => done())
-        this.LimitationsManager.allowedNumberOfCollaboratorsInProject(
-          this.projectId,
-          this.callback
-        )
      })

-      it('should return the default number', function () {
-        this.callback
-          .calledWith(null, this.Settings.defaultFeatures.collaborators)
-          .should.equal(true)
+      it('should return the default number', async function () {
+        const result =
+          await this.LimitationsManager.promises.allowedNumberOfCollaboratorsInProject(
+            this.projectId
+          )
+        expect(result).to.equal(this.Settings.defaultFeatures.collaborators)
      })
    })

    describe('when the project is owned by a user with a subscription', function () {
-      beforeEach(function (done) {
+      beforeEach(function () {
        this.project.owner_ref = this.userId
        this.user.features = { collaborators: 21 }
-        this.callback = sinon.stub().callsFake(() => done())
-        this.LimitationsManager.allowedNumberOfCollaboratorsInProject(
-          this.projectId,
-          this.callback
-        )
      })

-      it('should return the number of collaborators the user is allowed', function () {
-        this.callback
-          .calledWith(null, this.user.features.collaborators)
-          .should.equal(true)
+      it('should return the number of collaborators the user is allowed', async function () {
+        const result =
+          await this.LimitationsManager.promises.allowedNumberOfCollaboratorsInProject(
+            this.projectId
+          )
+        expect(result).to.equal(this.user.features.collaborators)
      })
    })
  })

  describe('allowedNumberOfCollaboratorsForUser', function () {
    describe('when the user has no features', function () {
-      beforeEach(function (done) {
+      beforeEach(function () {
        this.Settings.defaultFeatures = { collaborators: 23 }
        delete this.user.features
-        this.callback = sinon.stub().callsFake(() => done())
-        this.LimitationsManager.allowedNumberOfCollaboratorsForUser(
-          this.userId,
-          this.callback
-        )
      })

-      it('should return the default number', function () {
-        this.callback
-          .calledWith(null, this.Settings.defaultFeatures.collaborators)
-          .should.equal(true)
+      it('should return the default number', async function () {
+        const result =
+          await this.LimitationsManager.promises.allowedNumberOfCollaboratorsForUser(
+            this.userId
+          )
+        expect(result).to.equal(this.Settings.defaultFeatures.collaborators)
      })
    })

    describe('when the user has features', function () {
-      beforeEach(function (done) {
+      beforeEach(async function () {
        this.user.features = { collaborators: 21 }
-        this.callback = sinon.stub().callsFake(() => done())
-        this.LimitationsManager.allowedNumberOfCollaboratorsForUser(
-          this.userId,
-          this.callback
-        )
+        this.result =
+          await this.LimitationsManager.promises.allowedNumberOfCollaboratorsForUser(
+            this.userId
+          )
      })

      it('should return the number of collaborators the user is allowed', function () {
-        this.callback
-          .calledWith(null, this.user.features.collaborators)
-          .should.equal(true)
+        expect(this.result).to.equal(this.user.features.collaborators)
      })
    })
  })
@@ -222,7 +210,7 @@ describe('LimitationsManager', function () {

  describe('canAddXEditCollaborators', function () {
    describe('when the project has fewer collaborators than allowed', function () {
-      beforeEach(function (done) {
+      beforeEach(function () {
        this.current_number = 1
        this.user.features.collaborators = 2
        this.invite_count = 0
@@ -231,21 +219,20 @@ describe('LimitationsManager', function () {
        this.CollaboratorsInviteGetter.promises.getEditInviteCount = sinon
          .stub()
          .resolves(this.invite_count)
-        this.callback = sinon.stub().callsFake(() => done())
-        this.LimitationsManager.canAddXEditCollaborators(
-          this.projectId,
-          1,
-          this.callback
-        )
      })

-      it('should return true', function () {
-        this.callback.calledWith(null, true).should.equal(true)
+      it('should return true', async function () {
+        const result =
+          await this.LimitationsManager.promises.canAddXEditCollaborators(
+            this.projectId,
+            1
+          )
+        expect(result).to.be.true
      })
    })

    describe('when the project has fewer collaborators and invites than allowed', function () {
-      beforeEach(function (done) {
+      beforeEach(function () {
        this.current_number = 1
        this.user.features.collaborators = 4
        this.invite_count = 1
@@ -254,21 +241,20 @@ describe('LimitationsManager', function () {
        this.CollaboratorsInviteGetter.promises.getEditInviteCount = sinon
          .stub()
          .resolves(this.invite_count)
-        this.callback = sinon.stub().callsFake(() => done())
-        this.LimitationsManager.canAddXEditCollaborators(
-          this.projectId,
-          1,
-          this.callback
-        )
      })

-      it('should return true', function () {
-        this.callback.calledWith(null, true).should.equal(true)
+      it('should return true', async function () {
+        const result =
+          await this.LimitationsManager.promises.canAddXEditCollaborators(
+            this.projectId,
+            1
+          )
+        expect(result).to.be.true
      })
    })

    describe('when the project has fewer collaborators than allowed but I want to add more than allowed', function () {
-      beforeEach(function (done) {
+      beforeEach(function () {
        this.current_number = 1
        this.user.features.collaborators = 2
        this.invite_count = 0
@@ -277,21 +263,20 @@ describe('LimitationsManager', function () {
        this.CollaboratorsInviteGetter.promises.getEditInviteCount = sinon
          .stub()
          .resolves(this.invite_count)
-        this.callback = sinon.stub().callsFake(() => done())
-        this.LimitationsManager.canAddXEditCollaborators(
-          this.projectId,
-          2,
-          this.callback
-        )
      })

-      it('should return false', function () {
-        this.callback.calledWith(null, false).should.equal(true)
+      it('should return false', async function () {
+        const result =
+          await this.LimitationsManager.promises.canAddXEditCollaborators(
+            this.projectId,
+            2
+          )
+        expect(result).to.be.false
      })
    })

    describe('when the project has more collaborators than allowed', function () {
-      beforeEach(function (done) {
+      beforeEach(function () {
        this.current_number = 3
        this.user.features.collaborators = 2
        this.invite_count = 0
@@ -300,21 +285,20 @@ describe('LimitationsManager', function () {
        this.CollaboratorsInviteGetter.promises.getEditInviteCount = sinon
          .stub()
          .resolves(this.invite_count)
-        this.callback = sinon.stub().callsFake(() => done())
-        this.LimitationsManager.canAddXEditCollaborators(
-          this.projectId,
-          1,
-          this.callback
-        )
      })

-      it('should return false', function () {
-        this.callback.calledWith(null, false).should.equal(true)
+      it('should return false', async function () {
+        const result =
+          await this.LimitationsManager.promises.canAddXEditCollaborators(
+            this.projectId,
+            1
+          )
+        expect(result).to.be.false
      })
    })

    describe('when the project has infinite collaborators', function () {
-      beforeEach(function (done) {
+      beforeEach(function () {
        this.current_number = 100
        this.user.features.collaborators = -1
        this.invite_count = 0
@@ -323,21 +307,20 @@ describe('LimitationsManager', function () {
        this.CollaboratorsInviteGetter.promises.getEditInviteCount = sinon
          .stub()
          .resolves(this.invite_count)
-        this.callback = sinon.stub().callsFake(() => done())
-        this.LimitationsManager.canAddXEditCollaborators(
-          this.projectId,
-          1,
-          this.callback
-        )
      })

-      it('should return true', function () {
-        this.callback.calledWith(null, true).should.equal(true)
+      it('should return true', async function () {
+        const result =
+          await this.LimitationsManager.promises.canAddXEditCollaborators(
+            this.projectId,
+            1
+          )
+        expect(result).to.be.true
      })
    })

    describe('when the project has more invites than allowed', function () {
-      beforeEach(function (done) {
+      beforeEach(function () {
        this.current_number = 0
        this.user.features.collaborators = 2
        this.invite_count = 2
@@ -346,21 +329,20 @@ describe('LimitationsManager', function () {
        this.CollaboratorsInviteGetter.promises.getEditInviteCount = sinon
          .stub()
          .resolves(this.invite_count)
-        this.callback = sinon.stub().callsFake(() => done())
-        this.LimitationsManager.canAddXEditCollaborators(
-          this.projectId,
-          1,
-          this.callback
-        )
      })

-      it('should return false', function () {
-        this.callback.calledWith(null, false).should.equal(true)
+      it('should return false', async function () {
+        const result =
+          await this.LimitationsManager.promises.canAddXEditCollaborators(
+            this.projectId,
+            1
+          )
+        expect(result).to.be.false
      })
    })

    describe('when the project has more invites and collaborators than allowed', function () {
-      beforeEach(function (done) {
+      beforeEach(function () {
        this.current_number = 1
        this.user.features.collaborators = 2
        this.invite_count = 1
@@ -369,16 +351,67 @@ describe('LimitationsManager', function () {
        this.CollaboratorsInviteGetter.promises.getEditInviteCount = sinon
          .stub()
          .resolves(this.invite_count)
-        this.callback = sinon.stub().callsFake(() => done())
-        this.LimitationsManager.canAddXEditCollaborators(
-          this.projectId,
-          1,
-          this.callback
+      })
+
+      it('should return false', async function () {
+        const result =
+          await this.LimitationsManager.promises.canAddXEditCollaborators(
+            this.projectId,
+            1
+          )
+        expect(result).to.be.false
+      })
+    })
+  })
+
+  describe('canChangeCollaboratorPrivilegeLevel', function () {
+    beforeEach(function () {
+      this.collaboratorId = 'collaborator-id'
+      this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel.resolves(
+        'readOnly'
+      )
+    })
+
+    describe("when the limit hasn't been reached", function () {
+      it('accepts changing a viewer to an editor', async function () {
+        const result =
+          await this.LimitationsManager.promises.canChangeCollaboratorPrivilegeLevel(
+            this.projectId,
+            this.collaboratorId,
+            'readAndWrite'
+          )
+        expect(result).to.be.true
+      })
+    })
+
+    describe('when the limit has been reached', function () {
+      beforeEach(function () {
+        this.CollaboratorsGetter.promises.getInvitedEditCollaboratorCount.resolves(
+          1
        )
      })

-      it('should return false', function () {
-        this.callback.calledWith(null, false).should.equal(true)
+      it('accepts changing a reviewer to an editor', async function () {
+        this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel.resolves(
+          'review'
+        )
+        const result =
+          await this.LimitationsManager.promises.canChangeCollaboratorPrivilegeLevel(
+            this.projectId,
+            this.collaboratorId,
+            'readAndWrite'
+          )
+        expect(result).to.be.true
+      })
+
+      it('rejects changing a viewer to a reviewer', async function () {
+        const result =
+          await this.LimitationsManager.promises.canChangeCollaboratorPrivilegeLevel(
+            this.projectId,
+            this.collaboratorId,
+            'review'
+          )
+        expect(result).to.be.false
      })
    })
  })
@@ -390,60 +423,39 @@ describe('LimitationsManager', function () {
        .resolves()
    })

-    it('should return true if the recurly token is set', function (done) {
+    it('should return true if the recurly token is set', async function () {
      this.SubscriptionLocator.promises.getUsersSubscription = sinon
        .stub()
        .resolves({
          recurlySubscription_id: '1234',
        })
-      this.LimitationsManager.userHasSubscription(
-        this.user,
-        (err, hasSubscription) => {
-          assert.equal(err, null)
-          hasSubscription.should.equal(true)
-          done()
-        }
-      )
+      const { hasSubscription } =
+        await this.LimitationsManager.promises.userHasSubscription(this.user)
+      expect(hasSubscription).to.be.true
    })

-    it('should return false if the recurly token is not set', function (done) {
+    it('should return false if the recurly token is not set', async function () {
      this.SubscriptionLocator.promises.getUsersSubscription.resolves({})
-      this.subscription = {}
-      this.LimitationsManager.userHasSubscription(
-        this.user,
-        (err, hasSubscription) => {
-          assert.equal(err, null)
-          hasSubscription.should.equal(false)
-          done()
-        }
-      )
+      const { hasSubscription } =
+        await this.LimitationsManager.promises.userHasSubscription(this.user)
+      expect(hasSubscription).to.be.false
    })

-    it('should return false if the subscription is undefined', function (done) {
+    it('should return false if the subscription is undefined', async function () {
      this.SubscriptionLocator.promises.getUsersSubscription.resolves()
-      this.LimitationsManager.userHasSubscription(
-        this.user,
-        (err, hasSubscription) => {
-          assert.equal(err, null)
-          hasSubscription.should.equal(false)
-          done()
-        }
-      )
+      const { hasSubscription } =
+        await this.LimitationsManager.promises.userHasSubscription(this.user)
+      expect(hasSubscription).to.be.false
    })

-    it('should return the subscription', function (done) {
+    it('should return the subscription', async function () {
      const stubbedSubscription = { freeTrial: {}, token: '' }
      this.SubscriptionLocator.promises.getUsersSubscription.resolves(
        stubbedSubscription
      )
-      this.LimitationsManager.userHasSubscription(
-        this.user,
-        (err, hasSubOrIsGroupMember, subscription) => {
-          assert.equal(err, null)
-          subscription.should.deep.equal(stubbedSubscription)
-          done()
-        }
-      )
+      const { subscription } =
+        await this.LimitationsManager.promises.userHasSubscription(this.user)
+      expect(subscription).to.deep.equal(stubbedSubscription)
    })

    describe('when user has a custom account', function () {
@@ -454,26 +466,16 @@ describe('LimitationsManager', function () {
        )
      })

-      it('should return true', function (done) {
-        this.LimitationsManager.userHasSubscription(
-          this.user,
-          (err, hasSubscription, subscription) => {
-            assert.equal(err, null)
-            hasSubscription.should.equal(true)
-            done()
-          }
-        )
+      it('should return true', async function () {
+        const { hasSubscription } =
+          await this.LimitationsManager.promises.userHasSubscription(this.user)
+        expect(hasSubscription).to.be.true
      })

-      it('should return the subscription', function (done) {
-        this.LimitationsManager.userHasSubscription(
-          this.user,
-          (err, hasSubscription, subscription) => {
-            assert.equal(err, null)
-            subscription.should.deep.equal(this.fakeSubscription)
-            done()
-          }
-        )
+      it('should return the subscription', async function () {
+        const { subscription } =
+          await this.LimitationsManager.promises.userHasSubscription(this.user)
+        expect(subscription).to.deep.equal(this.fakeSubscription)
      })
    })
  })
@@ -485,32 +487,26 @@ describe('LimitationsManager', function () {
        .resolves()
    })

-    it('should return false if there are no groups subcriptions', function (done) {
+    it('should return false if there are no groups subcriptions', async function () {
      this.SubscriptionLocator.promises.getMemberSubscriptions.resolves([])
-      this.LimitationsManager.userIsMemberOfGroupSubscription(
-        this.user,
-        (err, isMember) => {
-          assert.equal(err, null)
-          isMember.should.equal(false)
-          done()
-        }
-      )
+      const { isMember } =
+        await this.LimitationsManager.promises.userIsMemberOfGroupSubscription(
+          this.user
+        )
+      expect(isMember).to.be.false
    })

-    it('should return true if there are no groups subcriptions', function (done) {
-      let subscriptions
+    it('should return true if there are no groups subcriptions', async function () {
+      const subscriptions = ['mock-subscription']
      this.SubscriptionLocator.promises.getMemberSubscriptions.resolves(
-        (subscriptions = ['mock-subscription'])
-      )
-      this.LimitationsManager.userIsMemberOfGroupSubscription(
-        this.user,
-        (err, isMember, retSubscriptions) => {
-          assert.equal(err, null)
-          isMember.should.equal(true)
-          retSubscriptions.should.equal(subscriptions)
-          done()
-        }
+        subscriptions
      )
+      const { isMember, subscriptions: retSubscriptions } =
+        await this.LimitationsManager.promises.userIsMemberOfGroupSubscription(
+          this.user
+        )
+      expect(isMember).to.be.true
+      expect(retSubscriptions).to.deep.equal(subscriptions)
    })
  })

@@ -524,54 +520,36 @@ describe('LimitationsManager', function () {
        .resolves(null)
    })

-    it('should return true if userIsMemberOfGroupSubscription', function (done) {
+    it('should return true if userIsMemberOfGroupSubscription', async function () {
      this.SubscriptionLocator.promises.getMemberSubscriptions = sinon
        .stub()
        .resolves([{ _id: '123' }])
-      this.LimitationsManager.hasPaidSubscription(
-        this.user,
-        (err, hasSubOrIsGroupMember) => {
-          assert.equal(err, null)
-          hasSubOrIsGroupMember.should.equal(true)
-          done()
-        }
-      )
+      const { hasPaidSubscription } =
+        await this.LimitationsManager.promises.hasPaidSubscription(this.user)
+      expect(hasPaidSubscription).to.be.true
    })

-    it('should return true if userHasSubscription', function (done) {
+    it('should return true if userHasSubscription', async function () {
      this.SubscriptionLocator.promises.getUsersSubscription = sinon
        .stub()
        .resolves({ recurlySubscription_id: '123' })
-      this.LimitationsManager.hasPaidSubscription(
-        this.user,
-        (err, hasSubOrIsGroupMember) => {
-          assert.equal(err, null)
-          hasSubOrIsGroupMember.should.equal(true)
-          done()
-        }
-      )
+      const { hasPaidSubscription } =
+        await this.LimitationsManager.promises.hasPaidSubscription(this.user)
+      expect(hasPaidSubscription).to.be.true
    })

-    it('should return false if none are true', function (done) {
-      this.LimitationsManager.hasPaidSubscription(
-        this.user,
-        (err, hasSubOrIsGroupMember) => {
-          assert.equal(err, null)
-          hasSubOrIsGroupMember.should.equal(false)
-          done()
-        }
-      )
+    it('should return false if none are true', async function () {
+      const { hasPaidSubscription } =
+        await this.LimitationsManager.promises.hasPaidSubscription(this.user)
+      expect(hasPaidSubscription).to.be.false
    })

-    it('should have userHasSubscriptionOrIsGroupMember alias', function (done) {
-      this.LimitationsManager.userHasSubscriptionOrIsGroupMember(
-        this.user,
-        (err, hasSubOrIsGroupMember) => {
-          assert.equal(err, null)
-          hasSubOrIsGroupMember.should.equal(false)
-          done()
-        }
-      )
+    it('should have userHasSubscriptionOrIsGroupMember alias', async function () {
+      const { hasPaidSubscription } =
+        await this.LimitationsManager.promises.userHasSubscriptionOrIsGroupMember(
+          this.user
+        )
+      expect(hasPaidSubscription).to.be.false
    })
  })

@@ -587,48 +565,39 @@ describe('LimitationsManager', function () {
      }
    })

-    it('should return true if the limit is hit (including members and invites)', function (done) {
+    it('should return true if the limit is hit (including members and invites)', async function () {
      this.SubscriptionLocator.promises.getSubscription.resolves(
        this.subscription
      )
-      this.LimitationsManager.hasGroupMembersLimitReached(
-        this.subscriptionId,
-        (err, limitReached) => {
-          assert.equal(err, null)
-          limitReached.should.equal(true)
-          done()
-        }
-      )
+      const { limitReached } =
+        await this.LimitationsManager.promises.hasGroupMembersLimitReached(
+          this.subscriptionId
+        )
+      expect(limitReached).to.be.true
    })

-    it('should return false if the limit is not hit (including members and invites)', function (done) {
+    it('should return false if the limit is not hit (including members and invites)', async function () {
      this.subscription.membersLimit = 4
      this.SubscriptionLocator.promises.getSubscription.resolves(
        this.subscription
      )
-      this.LimitationsManager.hasGroupMembersLimitReached(
-        this.subscriptionId,
-        (err, limitReached) => {
-          assert.equal(err, null)
-          limitReached.should.equal(false)
-          done()
-        }
-      )
+      const { limitReached } =
+        await this.LimitationsManager.promises.hasGroupMembersLimitReached(
+          this.subscriptionId
+        )
+      expect(limitReached).to.be.false
    })

-    it('should return true if the limit has been exceded (including members and invites)', function (done) {
+    it('should return true if the limit has been exceded (including members and invites)', async function () {
      this.subscription.membersLimit = 2
      this.SubscriptionLocator.promises.getSubscription.resolves(
        this.subscription
      )
-      this.LimitationsManager.hasGroupMembersLimitReached(
-        this.subscriptionId,
-        (err, limitReached) => {
-          assert.equal(err, null)
-          limitReached.should.equal(true)
-          done()
-        }
-      )
+      const { limitReached } =
+        await this.LimitationsManager.promises.hasGroupMembersLimitReached(
+          this.subscriptionId
+        )
+      expect(limitReached).to.be.true
    })
  })
 })