From 379d9ac41ce881aaf361b46034726acfd84e757d Mon Sep 17 00:00:00 2001 From: Jessica Lawshe Date: Mon, 16 Aug 2021 08:08:16 -0500 Subject: [PATCH] Merge pull request #4659 from overleaf/jel-em-duplicate-subscription-message One Pro message per current commons institutions GitOrigin-RevId: 21b74af8b9da1b6647a0384115f5d268b80e4bdf --- .../Institutions/InstitutionsGetter.js | 17 +++++++ .../Subscription/SubscriptionController.js | 11 ++--- .../SubscriptionViewModelBuilder.js | 12 ++--- .../web/app/views/subscriptions/dashboard.pug | 3 +- .../dashboard/_institution_memberships.pug | 24 +++++---- .../Institutions/InstitutionsGetterTests.js | 49 +++++++++++++++++++ 6 files changed, 86 insertions(+), 30 deletions(-) diff --git a/services/web/app/src/Features/Institutions/InstitutionsGetter.js b/services/web/app/src/Features/Institutions/InstitutionsGetter.js index 312e2df3f7..c55fa716b5 100644 --- a/services/web/app/src/Features/Institutions/InstitutionsGetter.js +++ b/services/web/app/src/Features/Institutions/InstitutionsGetter.js @@ -29,6 +29,19 @@ async function getCurrentInstitutionIds(userId) { return [...institutionIds] } +async function getCurrentInstitutionsWithLicence(userId) { + // current are those confirmed and not with lapsed reconfirmations + // only 1 record returned per current institution + const institutions = {} + const currentAffiliations = await _getCurrentAffiliations(userId) + currentAffiliations.forEach(affiliation => { + if (affiliation.licence && affiliation.licence !== 'free') { + institutions[affiliation.institution.id] = affiliation.institution + } + }) + return Object.values(institutions) +} + const InstitutionsGetter = { getConfirmedAffiliations(userId, callback) { UserGetter.getUserFullEmails(userId, function (error, emailsData) { @@ -51,6 +64,9 @@ const InstitutionsGetter = { }, getCurrentInstitutionIds: callbackify(getCurrentInstitutionIds), + getCurrentInstitutionsWithLicence: callbackify( + getCurrentInstitutionsWithLicence + ), getManagedInstitutions(userId, callback) { UserMembershipsHandler.getEntitiesByUser( @@ -63,6 +79,7 @@ const InstitutionsGetter = { InstitutionsGetter.promises = { getCurrentInstitutionIds, + getCurrentInstitutionsWithLicence, } module.exports = InstitutionsGetter diff --git a/services/web/app/src/Features/Subscription/SubscriptionController.js b/services/web/app/src/Features/Subscription/SubscriptionController.js index d13059cfb1..19864fb2bb 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionController.js +++ b/services/web/app/src/Features/Subscription/SubscriptionController.js @@ -19,7 +19,6 @@ const AnalyticsManager = require('../Analytics/AnalyticsManager') const RecurlyEventHandler = require('./RecurlyEventHandler') const { expressify } = require('../../util/promises') const OError = require('@overleaf/o-error') -const _ = require('lodash') const SUBSCRIPTION_PAGE_SPLIT_TEST = 'subscription-page' @@ -103,7 +102,7 @@ async function userSubscriptionPage(req, res) { personalSubscription, memberGroupSubscriptions, managedGroupSubscriptions, - confirmedMemberAffiliations, + currentInstitutionsWithLicence, managedInstitutions, managedPublishers, v1SubscriptionStatus, @@ -121,11 +120,7 @@ async function userSubscriptionPage(req, res) { personalSubscription || hasSubscription || (memberGroupSubscriptions && memberGroupSubscriptions.length > 0) || - (confirmedMemberAffiliations && - confirmedMemberAffiliations.length > 0 && - _.find(confirmedMemberAffiliations, affiliation => { - return affiliation.licence && affiliation.licence !== 'free' - })) + currentInstitutionsWithLicence.length > 0 ) { AnalyticsManager.recordEvent(user._id, 'subscription-page-view') } else { @@ -163,10 +158,10 @@ async function userSubscriptionPage(req, res) { personalSubscription, memberGroupSubscriptions, managedGroupSubscriptions, - confirmedMemberAffiliations, managedInstitutions, managedPublishers, v1SubscriptionStatus, + currentInstitutionsWithLicence, } res.render('subscriptions/dashboard', data) } diff --git a/services/web/app/src/Features/Subscription/SubscriptionViewModelBuilder.js b/services/web/app/src/Features/Subscription/SubscriptionViewModelBuilder.js index c7c0d922c6..ca1a67e204 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionViewModelBuilder.js +++ b/services/web/app/src/Features/Subscription/SubscriptionViewModelBuilder.js @@ -88,8 +88,8 @@ function buildUsersSubscriptionViewModel(user, callback) { managedGroupSubscriptions(cb) { SubscriptionLocator.getManagedGroupSubscriptions(user, cb) }, - confirmedMemberAffiliations(cb) { - InstitutionsGetter.getConfirmedAffiliations(user._id, cb) + currentInstitutionsWithLicence(cb) { + InstitutionsGetter.getCurrentInstitutionsWithLicence(user._id, cb) }, managedInstitutions(cb) { InstitutionsGetter.getManagedInstitutions(user._id, cb) @@ -117,7 +117,7 @@ function buildUsersSubscriptionViewModel(user, callback) { personalSubscription, memberGroupSubscriptions, managedGroupSubscriptions, - confirmedMemberAffiliations, + currentInstitutionsWithLicence, managedInstitutions, managedPublishers, v1SubscriptionStatus, @@ -125,15 +125,13 @@ function buildUsersSubscriptionViewModel(user, callback) { recurlyCoupons, plan, } = results + if (memberGroupSubscriptions == null) { memberGroupSubscriptions = [] } if (managedGroupSubscriptions == null) { managedGroupSubscriptions = [] } - if (confirmedMemberAffiliations == null) { - confirmedMemberAffiliations = [] - } if (managedInstitutions == null) { managedInstitutions = [] } @@ -269,7 +267,7 @@ function buildUsersSubscriptionViewModel(user, callback) { personalSubscription, managedGroupSubscriptions, memberGroupSubscriptions, - confirmedMemberAffiliations, + currentInstitutionsWithLicence, managedInstitutions, managedPublishers, v1SubscriptionStatus, diff --git a/services/web/app/views/subscriptions/dashboard.pug b/services/web/app/views/subscriptions/dashboard.pug index 574e902548..7688648c79 100644 --- a/services/web/app/views/subscriptions/dashboard.pug +++ b/services/web/app/views/subscriptions/dashboard.pug @@ -43,8 +43,7 @@ block content -hasDisplayedSubscription = true include ./dashboard/_group_memberships - if (confirmedMemberAffiliations && confirmedMemberAffiliations.length > 0) - include ./dashboard/_institution_memberships + include ./dashboard/_institution_memberships if (v1SubscriptionStatus) include ./dashboard/_v1_subscription_status diff --git a/services/web/app/views/subscriptions/dashboard/_institution_memberships.pug b/services/web/app/views/subscriptions/dashboard/_institution_memberships.pug index abe80a0cc3..9556100c51 100644 --- a/services/web/app/views/subscriptions/dashboard/_institution_memberships.pug +++ b/services/web/app/views/subscriptions/dashboard/_institution_memberships.pug @@ -1,13 +1,11 @@ -each affiliation in confirmedMemberAffiliations - if (affiliation.licence && affiliation.licence != 'free') - -hasDisplayedSubscription = true - p - | You have a - | - strong(ng-non-bindable) Professional - | - | #{settings.appName} account as a confirmed member of - | - strong(ng-non-bindable)= affiliation.institution.name - hr - +each institution in currentInstitutionsWithLicence + -hasDisplayedSubscription = true + p + | You have a + | + strong(ng-non-bindable) Professional + | + | #{settings.appName} account as a confirmed member of + | + strong(ng-non-bindable)= institution.name + hr diff --git a/services/web/test/unit/src/Institutions/InstitutionsGetterTests.js b/services/web/test/unit/src/Institutions/InstitutionsGetterTests.js index c992ee87db..9972f290d1 100644 --- a/services/web/test/unit/src/Institutions/InstitutionsGetterTests.js +++ b/services/web/test/unit/src/Institutions/InstitutionsGetterTests.js @@ -37,6 +37,36 @@ describe('InstitutionsGetter', function () { pastReconfirmDate: true, }, } + this.licencedAffiliation = { + confirmedAt: new Date(), + affiliation: { + licence: 'pro_plus', + institution: { id: 777, confirmed: true }, + pastReconfirmDate: false, + }, + } + this.licencedAffiliationPastReconfirmation = { + confirmedAt: new Date('2000-01-01'), + affiliation: { + licence: 'pro_plus', + institution: { id: 888, confirmed: true }, + pastReconfirmDate: true, + }, + } + this.unconfirmedEmailLicensedAffiliation = { + confirmedAt: null, + affiliation: { + licence: 'pro_plus', + institution: { id: 123, confirmed: true, pastReconfirmDate: false }, + }, + } + this.unconfirmedDomainLicensedAffiliation = { + confirmedAt: new Date(), + affiliation: { + licence: 'pro_plus', + institution: { id: 789, confirmed: false, pastReconfirmDate: false }, + }, + } this.userEmails = [ { confirmedAt: null, @@ -90,4 +120,23 @@ describe('InstitutionsGetter', function () { expect(e.message).to.equal('oops') }) }) + + describe('getCurrentInstitutionsWithLicence', function () { + it('returns one result per institution and filters out affiliations without license', async function () { + this.UserGetter.promises.getUserFullEmails.resolves([ + this.licencedAffiliation, + this.licencedAffiliation, + this.licencedAffiliationPastReconfirmation, + this.confirmedAffiliation, + this.unconfirmedDomainLicensedAffiliation, + this.unconfirmedEmailLicensedAffiliation, + ]) + const institutions = await this.InstitutionsGetter.promises.getCurrentInstitutionsWithLicence( + this.userId + ) + expect(institutions.map(institution => institution.id)).to.deep.equal([ + this.licencedAffiliation.affiliation.institution.id, + ]) + }) + }) })