Merge pull request #14685 from overleaf/bg-refactor-institutions-controller

refactor InstitutionsController and unit tests

GitOrigin-RevId: 947733b276fd3a5597baa0e95cd0ecca4853611f

services/web/app/src/Features/Institutions/InstitutionsController.js

@@ -1,9 +1,4 @@
-const OError = require('@overleaf/o-error')
-const UserGetter = require('../User/UserGetter')
-const { addAffiliation } = require('../Institutions/InstitutionsAPI')
-const FeaturesUpdater = require('../Subscription/FeaturesUpdater')
-const async = require('async')
-const ASYNC_AFFILIATIONS_LIMIT = 10
+const { affiliateUsers } = require('./InstitutionsManager')
 
 module.exports = {
   confirmDomain(req, res, next) {
@@ -16,62 +11,3 @@ module.exports = {
     })
   },
 }
-
-function affiliateUsers(hostname, callback) {
-  const reversedHostname = hostname.trim().split('').reverse().join('')
-  UserGetter.getInstitutionUsersByHostname(hostname, (error, users) => {
-    if (error) {
-      OError.tag(error, 'problem fetching users by hostname')
-      return callback(error)
-    }
-
-    async.mapLimit(
-      users,
-      ASYNC_AFFILIATIONS_LIMIT,
-      (user, innerCallback) => {
-        affiliateUserByReversedHostname(user, reversedHostname, innerCallback)
-      },
-      callback
-    )
-  })
-}
-
-function affiliateUserByReversedHostname(user, reversedHostname, callback) {
-  const matchingEmails = user.emails.filter(
-    email => email.reversedHostname === reversedHostname
-  )
-  async.mapSeries(
-    matchingEmails,
-    (email, innerCallback) => {
-      addAffiliation(
-        user._id,
-        email.email,
-        {
-          confirmedAt: email.confirmedAt,
-          entitlement:
-            email.samlIdentifier && email.samlIdentifier.hasEntitlement,
-        },
-        error => {
-          if (error) {
-            OError.tag(
-              error,
-              'problem adding affiliation while confirming hostname'
-            )
-            return innerCallback(error)
-          }
-          innerCallback()
-        }
-      )
-    },
-    err => {
-      if (err) {
-        return callback(err)
-      }
-      FeaturesUpdater.refreshFeatures(
-        user._id,
-        'affiliate-user-by-reversed-hostname',
-        callback
-      )
-    }
-  )
-}

services/web/app/src/Features/Institutions/InstitutionsManager.js

@@ -7,6 +7,7 @@ const { fetchJson } = require('@overleaf/fetch-utils')
 const {
   getInstitutionAffiliations,
   getConfirmedInstitutionAffiliations,
+  addAffiliation,
   promises: InstitutionsAPIPromises,
 } = require('./InstitutionsAPI')
 const FeaturesUpdater = require('../Subscription/FeaturesUpdater')
@@ -17,6 +18,7 @@ const NotificationsHandler = require('../Notifications/NotificationsHandler')
 const SubscriptionLocator = require('../Subscription/SubscriptionLocator')
 const { Institution } = require('../../models/Institution')
 const { Subscription } = require('../../models/Subscription')
+const OError = require('@overleaf/o-error')
 
 const ASYNC_LIMIT = parseInt(process.env.ASYNC_LIMIT, 10) || 5
 
@@ -249,6 +251,25 @@ const InstitutionsManager = {
         .exec(callback)
     })
   },
+
+  affiliateUsers(hostname, callback) {
+    const reversedHostname = hostname.trim().split('').reverse().join('')
+    UserGetter.getInstitutionUsersByHostname(hostname, (error, users) => {
+      if (error) {
+        OError.tag(error, 'problem fetching users by hostname')
+        return callback(error)
+      }
+
+      async.mapLimit(
+        users,
+        ASYNC_LIMIT,
+        (user, innerCallback) => {
+          affiliateUserByReversedHostname(user, reversedHostname, innerCallback)
+        },
+        callback
+      )
+    })
+  },
 }
 
 const fetchInstitutionAndAffiliations = (institutionId, callback) =>
@@ -359,7 +380,48 @@ async function fetchV1Data(institution) {
   }
 }
 
+function affiliateUserByReversedHostname(user, reversedHostname, callback) {
+  const matchingEmails = user.emails.filter(
+    email => email.reversedHostname === reversedHostname
+  )
+  async.mapSeries(
+    matchingEmails,
+    (email, innerCallback) => {
+      addAffiliation(
+        user._id,
+        email.email,
+        {
+          confirmedAt: email.confirmedAt,
+          entitlement:
+            email.samlIdentifier && email.samlIdentifier.hasEntitlement,
+        },
+        error => {
+          if (error) {
+            OError.tag(
+              error,
+              'problem adding affiliation while confirming hostname'
+            )
+            return innerCallback(error)
+          }
+          innerCallback()
+        }
+      )
+    },
+    err => {
+      if (err) {
+        return callback(err)
+      }
+      FeaturesUpdater.refreshFeatures(
+        user._id,
+        'affiliate-user-by-reversed-hostname',
+        callback
+      )
+    }
+  )
+}
+
 InstitutionsManager.promises = {
+  affiliateUsers: promisify(InstitutionsManager.affiliateUsers),
   checkInstitutionUsers,
   clearInstitutionNotifications: promisify(
     InstitutionsManager.clearInstitutionNotifications

services/web/test/unit/src/Institutions/InstitutionsControllerTests.js

@@ -1,155 +0,0 @@
-/* eslint-disable
-    max-len,
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-const SandboxedModule = require('sandboxed-module')
-const assert = require('assert')
-const { expect } = require('chai')
-const path = require('path')
-const sinon = require('sinon')
-const modulePath = path.join(
-  __dirname,
-  '../../../../app/src/Features/Institutions/InstitutionsController'
-)
-
-describe('InstitutionsController', function () {
-  beforeEach(function () {
-    this.host = 'mit.edu'.split('').reverse().join('')
-    this.stubbedUser1 = {
-      _id: '3131231',
-      name: 'bob',
-      email: 'hello@world.com',
-      emails: [
-        { email: 'stubb1@mit.edu', reversedHostname: this.host },
-        { email: 'test@test.com', reversedHostname: 'test.com' },
-        { email: 'another@mit.edu', reversedHostname: this.host },
-      ],
-    }
-    this.stubbedUser1DecoratedEmails = [
-      {
-        email: 'stubb1@mit.edu',
-        reversedHostname: this.host,
-        samlIdentifier: { hasEntitlement: false },
-      },
-      { email: 'test@test.com', reversedHostname: 'test.com' },
-      {
-        email: 'another@mit.edu',
-        reversedHostname: this.host,
-        samlIdentifier: { hasEntitlement: true },
-      },
-    ]
-    this.stubbedUser2 = {
-      _id: '3131232',
-      name: 'test',
-      email: 'hello2@world.com',
-      emails: [{ email: 'subb2@mit.edu', reversedHostname: this.host }],
-    }
-    this.stubbedUser2DecoratedEmails = [
-      {
-        email: 'subb2@mit.edu',
-        reversedHostname: this.host,
-      },
-    ]
-
-    this.getInstitutionUsersByHostname = sinon.stub().yields(null, [
-      {
-        _id: this.stubbedUser1._id,
-        emails: this.stubbedUser1DecoratedEmails,
-      },
-      {
-        _id: this.stubbedUser2._id,
-        emails: this.stubbedUser2DecoratedEmails,
-      },
-    ])
-    this.addAffiliation = sinon.stub().callsArgWith(3, null)
-    this.refreshFeatures = sinon.stub().yields(null)
-    this.InstitutionsController = SandboxedModule.require(modulePath, {
-      requires: {
-        '../User/UserGetter': {
-          getInstitutionUsersByHostname: this.getInstitutionUsersByHostname,
-        },
-        '../Institutions/InstitutionsAPI': {
-          addAffiliation: this.addAffiliation,
-        },
-        '../Subscription/FeaturesUpdater': {
-          refreshFeatures: this.refreshFeatures,
-        },
-      },
-    })
-
-    this.req = { body: { hostname: 'mit.edu' } }
-
-    this.res = {
-      send: sinon.stub(),
-      json: sinon.stub(),
-    }
-    return (this.next = sinon.stub())
-  })
-
-  describe('affiliateUsers', function () {
-    it('should add affiliations for matching users', function (done) {
-      this.res.sendStatus = code => {
-        code.should.equal(200)
-        this.getInstitutionUsersByHostname.calledOnce.should.equal(true)
-        this.addAffiliation.calledThrice.should.equal(true)
-        this.addAffiliation
-          .calledWithMatch(
-            this.stubbedUser1._id,
-            this.stubbedUser1.emails[0].email,
-            { entitlement: false }
-          )
-          .should.equal(true)
-        this.addAffiliation
-          .calledWithMatch(
-            this.stubbedUser1._id,
-            this.stubbedUser1.emails[2].email,
-            { entitlement: true }
-          )
-          .should.equal(true)
-        this.addAffiliation
-          .calledWithMatch(
-            this.stubbedUser2._id,
-            this.stubbedUser2.emails[0].email,
-            { entitlement: undefined }
-          )
-          .should.equal(true)
-        this.refreshFeatures
-          .calledWith(this.stubbedUser1._id)
-          .should.equal(true)
-        this.refreshFeatures
-          .calledWith(this.stubbedUser2._id)
-          .should.equal(true)
-        this.refreshFeatures.should.have.been.calledTwice
-        return done()
-      }
-      this.next.callsFake(done)
-      return this.InstitutionsController.confirmDomain(
-        this.req,
-        this.res,
-        this.next
-      )
-    })
-
-    it('should return errors if last affiliation cannot be added', function (done) {
-      this.addAffiliation.onCall(2).callsArgWith(3, new Error('error'))
-      this.next = error => {
-        expect(error).to.exist
-        this.getInstitutionUsersByHostname.calledOnce.should.equal(true)
-        return done()
-      }
-      return this.InstitutionsController.confirmDomain(
-        this.req,
-        this.res,
-        this.next
-      )
-    })
-  })
-})

services/web/test/unit/src/Institutions/InstitutionsManagerTests.js

@@ -14,6 +14,7 @@ describe('InstitutionsManager', function () {
     this.user = {}
     this.getInstitutionAffiliations = sinon.stub()
     this.getConfirmedInstitutionAffiliations = sinon.stub()
+    this.addAffiliation = sinon.stub().callsArgWith(3, null)
     this.refreshFeatures = sinon.stub().yields()
     this.users = [
       { _id: 'lapsed', features: {} },
@@ -92,12 +93,26 @@ describe('InstitutionsManager', function () {
       with_confirmed_email: 2, // 1 non entitled SSO + 1 email user
     }
 
+    // this.InstitutionsController = SandboxedModule.require(modulePath, {
+    //   requires: {
+    //     '../User/UserGetter': {
+    //       getInstitutionUsersByHostname: this.getInstitutionUsersByHostname,
+    //     },
+    //     '../Institutions/InstitutionsAPI': {
+    //       addAffiliation: this.addAffiliation,
+    //     },
+    //     '../Subscription/FeaturesUpdater': {
+    //       refreshFeatures: this.refreshFeatures,
+    //     },
+    //   },
+
     this.InstitutionsManager = SandboxedModule.require(modulePath, {
       requires: {
         './InstitutionsAPI': {
           getInstitutionAffiliations: this.getInstitutionAffiliations,
           getConfirmedInstitutionAffiliations:
             this.getConfirmedInstitutionAffiliations,
+          addAffiliation: this.addAffiliation,
           promises: {
             getInstitutionAffiliations:
               (this.getInstitutionAffiliationsPromise = sinon
@@ -125,6 +140,9 @@ describe('InstitutionsManager', function () {
         '../../models/Institution': this.InstitutionModel,
         '../../models/Subscription': SubscriptionModel,
         mongodb: this.Mongo,
+        '@overleaf/settings': {
+          features: { professional: { 'test-feature': true } },
+        },
       },
     })
   })
@@ -355,4 +373,106 @@ describe('InstitutionsManager', function () {
       )
     })
   })
+
+  describe('addAffiliations', function () {
+    beforeEach(function () {
+      this.host = 'mit.edu'.split('').reverse().join('')
+      this.stubbedUser1 = {
+        _id: '3131231',
+        name: 'bob',
+        email: 'hello@world.com',
+        emails: [
+          { email: 'stubb1@mit.edu', reversedHostname: this.host },
+          { email: 'test@test.com', reversedHostname: 'test.com' },
+          { email: 'another@mit.edu', reversedHostname: this.host },
+        ],
+      }
+      this.stubbedUser1DecoratedEmails = [
+        {
+          email: 'stubb1@mit.edu',
+          reversedHostname: this.host,
+          samlIdentifier: { hasEntitlement: false },
+        },
+        { email: 'test@test.com', reversedHostname: 'test.com' },
+        {
+          email: 'another@mit.edu',
+          reversedHostname: this.host,
+          samlIdentifier: { hasEntitlement: true },
+        },
+      ]
+      this.stubbedUser2 = {
+        _id: '3131232',
+        name: 'test',
+        email: 'hello2@world.com',
+        emails: [{ email: 'subb2@mit.edu', reversedHostname: this.host }],
+      }
+      this.stubbedUser2DecoratedEmails = [
+        {
+          email: 'subb2@mit.edu',
+          reversedHostname: this.host,
+        },
+      ]
+
+      this.getInstitutionUsersByHostname = sinon.stub().yields(null, [
+        {
+          _id: this.stubbedUser1._id,
+          emails: this.stubbedUser1DecoratedEmails,
+        },
+        {
+          _id: this.stubbedUser2._id,
+          emails: this.stubbedUser2DecoratedEmails,
+        },
+      ])
+      this.UserGetter.getInstitutionUsersByHostname =
+        this.getInstitutionUsersByHostname
+    })
+
+    describe('affiliateUsers', function () {
+      it('should add affiliations for matching users', function (done) {
+        this.InstitutionsManager.affiliateUsers('mit.edu', error => {
+          expect(error).not.to.exist
+          this.getInstitutionUsersByHostname.calledOnce.should.equal(true)
+          this.addAffiliation.calledThrice.should.equal(true)
+          this.addAffiliation
+            .calledWithMatch(
+              this.stubbedUser1._id,
+              this.stubbedUser1.emails[0].email,
+              { entitlement: false }
+            )
+            .should.equal(true)
+          this.addAffiliation
+            .calledWithMatch(
+              this.stubbedUser1._id,
+              this.stubbedUser1.emails[2].email,
+              { entitlement: true }
+            )
+            .should.equal(true)
+          this.addAffiliation
+            .calledWithMatch(
+              this.stubbedUser2._id,
+              this.stubbedUser2.emails[0].email,
+              { entitlement: undefined }
+            )
+            .should.equal(true)
+          this.refreshFeatures
+            .calledWith(this.stubbedUser1._id)
+            .should.equal(true)
+          this.refreshFeatures
+            .calledWith(this.stubbedUser2._id)
+            .should.equal(true)
+          this.refreshFeatures.should.have.been.calledTwice
+          done()
+        })
+      })
+
+      it('should return errors if last affiliation cannot be added', function (done) {
+        this.addAffiliation.onCall(2).callsArgWith(3, new Error('error'))
+        this.InstitutionsManager.affiliateUsers('mit.edu', error => {
+          expect(error).to.exist
+          this.getInstitutionUsersByHostname.calledOnce.should.equal(true)
+          done()
+        })
+      })
+    })
+  })
 })