From 2f926ce09e2fba337d5817dc08ae7c3654f55ed4 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Thu, 7 Sep 2023 10:16:02 +0100 Subject: [PATCH] Merge pull request #14685 from overleaf/bg-refactor-institutions-controller refactor InstitutionsController and unit tests GitOrigin-RevId: 947733b276fd3a5597baa0e95cd0ecca4853611f --- .../Institutions/InstitutionsController.js | 66 +------- .../Institutions/InstitutionsManager.js | 62 +++++++ .../InstitutionsControllerTests.js | 155 ------------------ .../Institutions/InstitutionsManagerTests.js | 120 ++++++++++++++ 4 files changed, 183 insertions(+), 220 deletions(-) delete mode 100644 services/web/test/unit/src/Institutions/InstitutionsControllerTests.js diff --git a/services/web/app/src/Features/Institutions/InstitutionsController.js b/services/web/app/src/Features/Institutions/InstitutionsController.js index c83d9993d6..6d6de30f85 100644 --- a/services/web/app/src/Features/Institutions/InstitutionsController.js +++ b/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 - ) - } - ) -} diff --git a/services/web/app/src/Features/Institutions/InstitutionsManager.js b/services/web/app/src/Features/Institutions/InstitutionsManager.js index 6b4be74127..baa8d0f34c 100644 --- a/services/web/app/src/Features/Institutions/InstitutionsManager.js +++ b/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 diff --git a/services/web/test/unit/src/Institutions/InstitutionsControllerTests.js b/services/web/test/unit/src/Institutions/InstitutionsControllerTests.js deleted file mode 100644 index 13169a8e91..0000000000 --- a/services/web/test/unit/src/Institutions/InstitutionsControllerTests.js +++ /dev/null @@ -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 - ) - }) - }) -}) diff --git a/services/web/test/unit/src/Institutions/InstitutionsManagerTests.js b/services/web/test/unit/src/Institutions/InstitutionsManagerTests.js index d38242335a..05928f8f23 100644 --- a/services/web/test/unit/src/Institutions/InstitutionsManagerTests.js +++ b/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() + }) + }) + }) + }) })