Merge pull request #2844 from overleaf/ta-admin-affiliations

Improve Admin Affiliations UI

GitOrigin-RevId: 1e3bcb425e1cb8463b2c3c7bbc757ab444b391a3

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

@@ -1,71 +1,43 @@
-/* eslint-disable
-    handle-callback-err,
-    max-len,
-    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
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 let InstitutionsFeatures
-const InstitutionsGetter = require('./InstitutionsGetter')
+const UserGetter = require('../User/UserGetter')
 const PlansLocator = require('../Subscription/PlansLocator')
 const Settings = require('settings-sharelatex')
-const logger = require('logger-sharelatex')
 
 module.exports = InstitutionsFeatures = {
   getInstitutionsFeatures(userId, callback) {
-    if (callback == null) {
-      callback = function(error, features) {}
-    }
-    return InstitutionsFeatures.getInstitutionsPlan(userId, function(
-      error,
-      plan
-    ) {
-      if (error != null) {
+    InstitutionsFeatures.getInstitutionsPlan(userId, function(error, planCode) {
+      if (error) {
         return callback(error)
       }
-      plan = PlansLocator.findLocalPlanInSettings(plan)
-      return callback(null, (plan != null ? plan.features : undefined) || {})
+      const plan = planCode && PlansLocator.findLocalPlanInSettings(planCode)
+      const features = plan && plan.features
+      callback(null, features || {})
     })
   },
 
   getInstitutionsPlan(userId, callback) {
-    if (callback == null) {
-      callback = function(error, plan) {}
-    }
-    return InstitutionsFeatures.hasLicence(userId, function(error, hasLicence) {
-      if (error != null) {
+    InstitutionsFeatures.hasLicence(userId, function(error, hasLicence) {
+      if (error) {
         return callback(error)
       }
       if (!hasLicence) {
         return callback(null, null)
       }
-      return callback(null, Settings.institutionPlanCode)
+      callback(null, Settings.institutionPlanCode)
     })
   },
 
   hasLicence(userId, callback) {
-    if (callback == null) {
-      callback = function(error, hasLicence) {}
-    }
-    return InstitutionsGetter.getConfirmedAffiliations(userId, function(
-      error,
-      affiliations
-    ) {
-      if (error != null) {
+    UserGetter.getUserFullEmails(userId, function(error, emailsData) {
+      if (error) {
         return callback(error)
       }
 
-      const hasLicence = affiliations.some(
-        affiliation => affiliation.licence && affiliation.licence !== 'free'
+      const hasLicence = emailsData.some(
+        emailData => emailData.emailHasInstitutionLicence
       )
 
-      return callback(null, hasLicence)
+      callback(null, hasLicence)
     })
   }
 }

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

@@ -0,0 +1,24 @@
+function emailHasLicence(emailData) {
+  if (!emailData.confirmedAt) {
+    return false
+  }
+  if (!emailData.affiliation) {
+    return false
+  }
+  const affiliation = emailData.affiliation
+  const institution = affiliation.institution
+  if (!institution) {
+    return false
+  }
+  if (!institution.confirmed) {
+    return false
+  }
+  if (!affiliation.licence) {
+    return false
+  }
+  return affiliation.licence !== 'free'
+}
+
+module.exports = {
+  emailHasLicence
+}

services/web/app/src/Features/User/UserGetter.js

@@ -5,6 +5,7 @@ const { db } = mongojs
 const { ObjectId } = mongojs
 const { promisifyAll } = require('../../util/promises')
 const { getUserAffiliations } = require('../Institutions/InstitutionsAPI')
+const InstitutionsHelper = require('../Institutions/InstitutionsHelper')
 const Errors = require('../Errors/Errors')
 const Features = require('../../infrastructure/Features')
 
@@ -37,7 +38,10 @@ const UserGetter = {
   },
 
   getUserFullEmails(userId, callback) {
-    this.getUser(userId, { email: 1, emails: 1 }, function(error, user) {
+    this.getUser(userId, { email: 1, emails: 1, samlIdentifiers: 1 }, function(
+      error,
+      user
+    ) {
       if (error) {
         return callback(error)
       }
@@ -46,7 +50,10 @@ const UserGetter = {
       }
 
       if (!Features.hasFeature('affiliations')) {
-        return callback(null, decorateFullEmails(user.email, user.emails, []))
+        return callback(
+          null,
+          decorateFullEmails(user.email, user.emails, [], [])
+        )
       }
 
       getUserAffiliations(userId, function(error, affiliationsData) {
@@ -55,7 +62,12 @@ const UserGetter = {
         }
         callback(
           null,
-          decorateFullEmails(user.email, user.emails || [], affiliationsData)
+          decorateFullEmails(
+            user.email,
+            user.emails || [],
+            affiliationsData,
+            user.samlIdentifiers || []
+          )
         )
       })
     })
@@ -147,7 +159,12 @@ const UserGetter = {
   }
 }
 
-var decorateFullEmails = (defaultEmail, emailsData, affiliationsData) =>
+var decorateFullEmails = (
+  defaultEmail,
+  emailsData,
+  affiliationsData,
+  samlIdentifiers
+) =>
   emailsData.map(function(emailData) {
     emailData.default = emailData.email === defaultEmail
 
@@ -167,6 +184,18 @@ var decorateFullEmails = (defaultEmail, emailsData, affiliationsData) =>
       emailsData.affiliation = null
     }
 
+    if (emailData.samlProviderId) {
+      emailData.samlIdentifier = samlIdentifiers.find(
+        samlIdentifier => samlIdentifier.providerId === emailData.samlProviderId
+      )
+    } else {
+      emailsData.samlIdentifier = null
+    }
+
+    emailData.emailHasInstitutionLicence = InstitutionsHelper.emailHasLicence(
+      emailData
+    )
+
     return emailData
   })
 ;[

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

@@ -0,0 +1,65 @@
+const { expect } = require('chai')
+const path = require('path')
+const InstitutionsHelper = require(path.join(
+  __dirname,
+  '/../../../../app/src/Features/Institutions/InstitutionsHelper'
+))
+
+describe('InstitutionsHelper', function() {
+  describe('emailHasLicence', function() {
+    it('returns licence', function() {
+      const emailHasLicence = InstitutionsHelper.emailHasLicence({
+        confirmedAt: new Date(),
+        affiliation: {
+          institution: { confirmed: true },
+          licence: 'pro_plus'
+        }
+      })
+      expect(emailHasLicence).to.be.true
+    })
+
+    it('returns false if licence is free', function() {
+      const emailHasLicence = InstitutionsHelper.emailHasLicence({
+        confirmedAt: new Date(),
+        affiliation: {
+          institution: { confirmed: true },
+          licence: 'free'
+        }
+      })
+      expect(emailHasLicence).to.be.false
+    })
+
+    it('returns false if licence is null', function() {
+      const emailHasLicence = InstitutionsHelper.emailHasLicence({
+        confirmedAt: new Date(),
+        affiliation: {
+          institution: { confirmed: true },
+          licence: null
+        }
+      })
+      expect(emailHasLicence).to.be.false
+    })
+
+    it('returns false if institution is not confirmed', function() {
+      const emailHasLicence = InstitutionsHelper.emailHasLicence({
+        confirmedAt: new Date(),
+        affiliation: {
+          institution: { confirmed: false },
+          licence: 'pro_plus'
+        }
+      })
+      expect(emailHasLicence).to.be.false
+    })
+
+    it('returns false if email is not confirmed', function() {
+      const emailHasLicence = InstitutionsHelper.emailHasLicence({
+        confirmedAt: null,
+        affiliation: {
+          institution: { confirmed: true },
+          licence: 'pro_plus'
+        }
+      })
+      expect(emailHasLicence).to.be.false
+    })
+  })
+})

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

@@ -22,7 +22,7 @@ const modulePath = require('path').join(
 
 describe('InstitutionsFeatures', function() {
   beforeEach(function() {
-    this.InstitutionsGetter = { getConfirmedAffiliations: sinon.stub() }
+    this.UserGetter = { getUserFullEmails: sinon.stub() }
     this.PlansLocator = { findLocalPlanInSettings: sinon.stub() }
     this.institutionPlanCode = 'institution_plan_code'
     this.InstitutionsFeatures = SandboxedModule.require(modulePath, {
@@ -30,7 +30,7 @@ describe('InstitutionsFeatures', function() {
         console: console
       },
       requires: {
-        './InstitutionsGetter': this.InstitutionsGetter,
+        '../User/UserGetter': this.UserGetter,
         '../Subscription/PlansLocator': this.PlansLocator,
         'settings-sharelatex': {
           institutionPlanCode: this.institutionPlanCode
@@ -47,7 +47,7 @@ describe('InstitutionsFeatures', function() {
 
   describe('hasLicence', function() {
     it('should handle error', function(done) {
-      this.InstitutionsGetter.getConfirmedAffiliations.yields(new Error('Nope'))
+      this.UserGetter.getUserFullEmails.yields(new Error('Nope'))
       return this.InstitutionsFeatures.hasLicence(
         this.userId,
         (error, hasLicence) => {
@@ -57,28 +57,9 @@ describe('InstitutionsFeatures', function() {
       )
     })
 
-    it('should return false if user has no confirmed affiliations', function(done) {
-      const affiliations = []
-      this.InstitutionsGetter.getConfirmedAffiliations.yields(
-        null,
-        affiliations
-      )
-      return this.InstitutionsFeatures.hasLicence(
-        this.userId,
-        (error, hasLicence) => {
-          expect(error).to.not.exist
-          expect(hasLicence).to.be.false
-          return done()
-        }
-      )
-    })
-
     it('should return false if user has no paid affiliations', function(done) {
-      const affiliations = [{ licence: 'free' }]
-      this.InstitutionsGetter.getConfirmedAffiliations.yields(
-        null,
-        affiliations
-      )
+      const emailData = [{ emailHasInstitutionLicence: false }]
+      this.UserGetter.getUserFullEmails.yields(null, emailData)
       return this.InstitutionsFeatures.hasLicence(
         this.userId,
         (error, hasLicence) => {
@@ -90,16 +71,11 @@ describe('InstitutionsFeatures', function() {
     })
 
     it('should return true if user has confirmed paid affiliation', function(done) {
-      const affiliations = [
-        { licence: 'pro_plus' },
-        { licence: 'free' },
-        { licence: 'pro' },
-        { licence: null }
+      const emailData = [
+        { emailHasInstitutionLicence: true },
+        { emailHasInstitutionLicence: false }
       ]
-      this.InstitutionsGetter.getConfirmedAffiliations.yields(
-        null,
-        affiliations
-      )
+      this.UserGetter.getUserFullEmails.yields(null, emailData)
       return this.InstitutionsFeatures.hasLicence(
         this.userId,
         (error, hasLicence) => {

services/web/test/unit/src/User/UserGetterTests.js

@@ -17,7 +17,11 @@ describe('UserGetter', function() {
       _id: '12390i',
       email: 'email2@foo.bar',
       emails: [
-        { email: 'email1@foo.bar', reversedHostname: 'rab.oof' },
+        {
+          email: 'email1@foo.bar',
+          reversedHostname: 'rab.oof',
+          confirmedAt: new Date()
+        },
         { email: 'email2@foo.bar', reversedHostname: 'rab.oof' }
       ]
     }
@@ -85,7 +89,7 @@ describe('UserGetter', function() {
       this.UserGetter.getUser = sinon
         .stub()
         .callsArgWith(2, null, this.fakeUser)
-      const projection = { email: 1, emails: 1 }
+      const projection = { email: 1, emails: 1, samlIdentifiers: 1 }
       this.UserGetter.getUserFullEmails(
         this.fakeUser._id,
         (error, fullEmails) => {
@@ -111,11 +115,14 @@ describe('UserGetter', function() {
             {
               email: 'email1@foo.bar',
               reversedHostname: 'rab.oof',
+              confirmedAt: this.fakeUser.emails[0].confirmedAt,
+              emailHasInstitutionLicence: false,
               default: false
             },
             {
               email: 'email2@foo.bar',
               reversedHostname: 'rab.oof',
+              emailHasInstitutionLicence: false,
               default: true
             }
           ])
@@ -135,7 +142,11 @@ describe('UserGetter', function() {
           department: 'Maths',
           inferred: false,
           licence: 'pro_plus',
-          institution: { name: 'University Name', isUniversity: true }
+          institution: {
+            name: 'University Name',
+            isUniversity: true,
+            confirmed: true
+          }
         }
       ]
       this.getUserAffiliations.callsArgWith(1, null, affiliationsData)
@@ -147,7 +158,9 @@ describe('UserGetter', function() {
             {
               email: 'email1@foo.bar',
               reversedHostname: 'rab.oof',
+              confirmedAt: this.fakeUser.emails[0].confirmedAt,
               default: false,
+              emailHasInstitutionLicence: true,
               affiliation: {
                 institution: affiliationsData[0].institution,
                 inferred: affiliationsData[0].inferred,
@@ -159,6 +172,42 @@ describe('UserGetter', function() {
             {
               email: 'email2@foo.bar',
               reversedHostname: 'rab.oof',
+              emailHasInstitutionLicence: false,
+              default: true
+            }
+          ])
+          done()
+        }
+      )
+    })
+
+    it('should merge SAML identifier', function(done) {
+      const fakeSamlIdentifiers = [
+        { providerId: 'saml_id', exteranlUserId: 'whatever' }
+      ]
+      const fakeUserWithSaml = this.fakeUser
+      fakeUserWithSaml.emails[0].samlProviderId = 'saml_id'
+      fakeUserWithSaml.samlIdentifiers = fakeSamlIdentifiers
+      this.UserGetter.getUser = sinon.stub().yields(null, this.fakeUser)
+      this.getUserAffiliations.callsArgWith(1, null, [])
+      this.UserGetter.getUserFullEmails(
+        this.fakeUser._id,
+        (error, fullEmails) => {
+          expect(error).to.not.exist
+          assert.deepEqual(fullEmails, [
+            {
+              email: 'email1@foo.bar',
+              reversedHostname: 'rab.oof',
+              confirmedAt: this.fakeUser.emails[0].confirmedAt,
+              default: false,
+              emailHasInstitutionLicence: false,
+              samlProviderId: 'saml_id',
+              samlIdentifier: fakeSamlIdentifiers[0]
+            },
+            {
+              email: 'email2@foo.bar',
+              reversedHostname: 'rab.oof',
+              emailHasInstitutionLicence: false,
               default: true
             }
           ])
@@ -175,7 +224,7 @@ describe('UserGetter', function() {
       this.UserGetter.getUser = sinon
         .stub()
         .callsArgWith(2, null, this.fakeUser)
-      const projection = { email: 1, emails: 1 }
+      const projection = { email: 1, emails: 1, samlIdentifiers: 1 }
       this.UserGetter.getUserFullEmails(
         this.fakeUser._id,
         (error, fullEmails) => {