From 6a0da3d204990c23c3eb614eacc092826100df7b Mon Sep 17 00:00:00 2001 From: Jessica Lawshe Date: Tue, 25 Jan 2022 09:22:08 -0600 Subject: [PATCH] Merge pull request #6375 from overleaf/jel-reconfirm-check [web] Use v1 date for reconfirm notification check GitOrigin-RevId: e14f1b6a1a6ab629628858d962a3757a6078cf79 --- .../web/app/src/Features/User/UserGetter.js | 22 +- .../test/acceptance/src/helpers/UserHelper.js | 4 + .../test/acceptance/src/mocks/MockV1Api.js | 29 ++- .../Institutions/InstitutionsGetterTests.js | 45 +++- .../web/test/unit/src/User/UserGetterTests.js | 230 +++++++++++++----- 5 files changed, 238 insertions(+), 92 deletions(-) diff --git a/services/web/app/src/Features/User/UserGetter.js b/services/web/app/src/Features/User/UserGetter.js index 66d2f8ceec..3b211278e6 100644 --- a/services/web/app/src/Features/User/UserGetter.js +++ b/services/web/app/src/Features/User/UserGetter.js @@ -42,28 +42,17 @@ function _pastReconfirmDate(lastDayToReconfirm) { return moment(lastDayToReconfirm).isBefore() } -function _emailInReconfirmNotificationPeriod( - lastDayToReconfirm, - cachedPastReconfirmDate -) { +function _emailInReconfirmNotificationPeriod(cachedLastDayToReconfirm) { const globalReconfirmPeriod = settings.reconfirmNotificationDays - if (!globalReconfirmPeriod || !lastDayToReconfirm) return false + if (!globalReconfirmPeriod || !cachedLastDayToReconfirm) return false - const notificationStarts = moment(lastDayToReconfirm).subtract( + const notificationStarts = moment(cachedLastDayToReconfirm).subtract( globalReconfirmPeriod, 'days' ) - let inNotificationPeriod = moment().isAfter(notificationStarts) - - if (!inNotificationPeriod && cachedPastReconfirmDate) { - // show notification if cached date is past, - // even if non-cached date is not past - inNotificationPeriod = true - } - - return inNotificationPeriod + return moment().isAfter(notificationStarts) } async function getUserFullEmails(userId) { @@ -271,8 +260,7 @@ const decorateFullEmails = ( } const pastReconfirmDate = _pastReconfirmDate(lastDayToReconfirm) const inReconfirmNotificationPeriod = _emailInReconfirmNotificationPeriod( - lastDayToReconfirm, - cachedPastReconfirmDate + cachedLastDayToReconfirm ) emailData.affiliation = { institution, diff --git a/services/web/test/acceptance/src/helpers/UserHelper.js b/services/web/test/acceptance/src/helpers/UserHelper.js index 401c504f8a..1b0e632192 100644 --- a/services/web/test/acceptance/src/helpers/UserHelper.js +++ b/services/web/test/acceptance/src/helpers/UserHelper.js @@ -1,6 +1,7 @@ const { expect } = require('chai') const AuthenticationManager = require('../../../../app/src/Features/Authentication/AuthenticationManager') const Settings = require('@overleaf/settings') +const InstitutionsAPI = require('../../../../app/src/Features/Institutions/InstitutionsAPI') const UserCreator = require('../../../../app/src/Features/User/UserCreator') const UserGetter = require('../../../../app/src/Features/User/UserGetter') const UserUpdater = require('../../../../app/src/Features/User/UserUpdater') @@ -335,6 +336,9 @@ class UserHelper { }, } await UserUpdater.promises.updateUser(query, update) + await InstitutionsAPI.promises.addAffiliation(userId, email, { + confirmedAt: date, + }) } async changeConfirmedToNotificationPeriod( diff --git a/services/web/test/acceptance/src/mocks/MockV1Api.js b/services/web/test/acceptance/src/mocks/MockV1Api.js index e970e64b08..73271a42ac 100644 --- a/services/web/test/acceptance/src/mocks/MockV1Api.js +++ b/services/web/test/acceptance/src/mocks/MockV1Api.js @@ -1,4 +1,5 @@ const AbstractMockApi = require('./AbstractMockApi') +const moment = require('moment') const sinon = require('sinon') class MockV1Api extends AbstractMockApi { @@ -91,7 +92,7 @@ class MockV1Api extends AbstractMockApi { ) } - addAffiliation(userId, email, entitlement) { + addAffiliation(userId, email, entitlement, confirmedAt) { let newAffiliation = true const institution = {} if (!email) return @@ -133,6 +134,17 @@ class MockV1Api extends AbstractMockApi { } }) } + + if (confirmedAt) { + this.affiliations[userId].forEach(affiliation => { + if (affiliation.email === email) { + if (!affiliation.cached_confirmed_at) { + affiliation.cached_confirmed_at = confirmedAt + } + affiliation.cached_reconfirmed_at = confirmedAt + } + }) + } } setDocExported(token, info) { @@ -235,6 +247,18 @@ class MockV1Api extends AbstractMockApi { ) { affiliation.licence = 'pro_plus' } + + if ( + institutionData.maxConfirmationMonths && + affiliation.cached_reconfirmed_at + ) { + const lastDayToReconfirm = moment( + affiliation.cached_reconfirmed_at + ).add(institutionData.maxConfirmationMonths, 'months') + affiliation.last_day_to_reconfirm = lastDayToReconfirm.toDate() + affiliation.past_reconfirm_date = lastDayToReconfirm.isBefore() + } + return affiliation } ) @@ -245,7 +269,8 @@ class MockV1Api extends AbstractMockApi { this.addAffiliation( req.params.userId, req.body.email, - req.body.entitlement + req.body.entitlement, + req.body.confirmedAt ) res.sendStatus(201) }) diff --git a/services/web/test/unit/src/Institutions/InstitutionsGetterTests.js b/services/web/test/unit/src/Institutions/InstitutionsGetterTests.js index 14285a1c7d..0d82b5d218 100644 --- a/services/web/test/unit/src/Institutions/InstitutionsGetterTests.js +++ b/services/web/test/unit/src/Institutions/InstitutionsGetterTests.js @@ -29,6 +29,7 @@ describe('InstitutionsGetter', function () { confirmedAt: new Date(), affiliation: { institution: { id: 456, confirmed: true }, + cachedPastReconfirmDate: false, pastReconfirmDate: false, }, } @@ -36,6 +37,7 @@ describe('InstitutionsGetter', function () { confirmedAt: new Date('2000-01-01'), affiliation: { institution: { id: 135, confirmed: true }, + cachedPastReconfirmDate: true, pastReconfirmDate: true, }, } @@ -44,6 +46,7 @@ describe('InstitutionsGetter', function () { affiliation: { licence: 'pro_plus', institution: { id: 777, confirmed: true }, + cachedPastReconfirmDate: false, pastReconfirmDate: false, }, } @@ -52,6 +55,7 @@ describe('InstitutionsGetter', function () { affiliation: { licence: 'pro_plus', institution: { id: 888, confirmed: true }, + cachedPastReconfirmDate: true, pastReconfirmDate: true, }, } @@ -59,35 +63,64 @@ describe('InstitutionsGetter', function () { confirmedAt: null, affiliation: { licence: 'pro_plus', - institution: { id: 123, confirmed: true, pastReconfirmDate: false }, + institution: { + id: 123, + confirmed: true, + cachedPastReconfirmDate: false, + pastReconfirmDate: false, + }, }, } this.unconfirmedDomainLicensedAffiliation = { confirmedAt: new Date(), affiliation: { licence: 'pro_plus', - institution: { id: 789, confirmed: false, pastReconfirmDate: false }, + institution: { + id: 789, + confirmed: false, + cachedPastReconfirmDate: false, + pastReconfirmDate: false, + }, }, } this.userEmails = [ { confirmedAt: null, affiliation: { - institution: { id: 123, confirmed: true, pastReconfirmDate: false }, + institution: { + id: 123, + confirmed: true, + cachedPastReconfirmDate: false, + pastReconfirmDate: false, + }, }, }, this.confirmedAffiliation, this.confirmedAffiliation, this.confirmedAffiliationPastReconfirmation, - { confirmedAt: new Date(), affiliation: null, pastReconfirmDate: false }, { confirmedAt: new Date(), - affiliation: { institution: null, pastReconfirmDate: false }, + affiliation: null, + cachedPastReconfirmDate: false, + pastReconfirmDate: false, }, { confirmedAt: new Date(), affiliation: { - institution: { id: 789, confirmed: false, pastReconfirmDate: false }, + institution: null, + cachedPastReconfirmDate: false, + pastReconfirmDate: false, + }, + }, + { + confirmedAt: new Date(), + affiliation: { + institution: { + id: 789, + confirmed: false, + cachedPastReconfirmDate: false, + pastReconfirmDate: false, + }, }, }, ] diff --git a/services/web/test/unit/src/User/UserGetterTests.js b/services/web/test/unit/src/User/UserGetterTests.js index 6c6ddabbea..bbe237e75b 100644 --- a/services/web/test/unit/src/User/UserGetterTests.js +++ b/services/web/test/unit/src/User/UserGetterTests.js @@ -328,6 +328,19 @@ describe('UserGetter', function () { }, ] it('should flag inReconfirmNotificationPeriod for all affiliations in period', function (done) { + const { maxConfirmationMonths } = institutionNonSSO + const confirmed1 = moment() + .subtract(maxConfirmationMonths + 2, 'months') + .toDate() + const lastDayToReconfirm1 = moment(confirmed1) + .add(maxConfirmationMonths, 'months') + .toDate() + const confirmed2 = moment() + .subtract(maxConfirmationMonths + 1, 'months') + .toDate() + const lastDayToReconfirm2 = moment(confirmed2) + .add(maxConfirmationMonths, 'months') + .toDate() const user = { _id: '12390i', email: email1, @@ -335,27 +348,20 @@ describe('UserGetter', function () { { email: email1, reversedHostname: 'moc.noitailiffa-elpmaxe', - confirmedAt: moment() - .subtract( - institutionNonSSO.maxConfirmationMonths + 2, - 'months' - ) - .toDate(), + confirmedAt: confirmed1, default: true, }, { email: email2, reversedHostname: 'moc.noitailiffa-elpmaxe', - confirmedAt: moment() - .subtract( - institutionNonSSO.maxConfirmationMonths + 1, - 'months' - ) - .toDate(), + confirmedAt: confirmed2, }, ], } - this.getUserAffiliations.resolves(affiliationsData) + const affiliations = [...affiliationsData] + affiliations[0].last_day_to_reconfirm = lastDayToReconfirm1 + affiliations[1].last_day_to_reconfirm = lastDayToReconfirm2 + this.getUserAffiliations.resolves(affiliations) this.UserGetter.promises.getUser = sinon.stub().resolves(user) this.UserGetter.getUserFullEmails( this.fakeUser._id, @@ -372,10 +378,18 @@ describe('UserGetter', function () { ) }) it('should not flag affiliations outside of notification period', function (done) { - const aboutToBeWithinPeriod = moment() - .subtract(institutionNonSSO.maxConfirmationMonths, 'months') + const { maxConfirmationMonths } = institutionNonSSO + const confirmed1 = new Date() + const lastDayToReconfirm1 = moment(confirmed1) + .add(maxConfirmationMonths, 'months') + .toDate() + const confirmed2 = moment() + .subtract(maxConfirmationMonths, 'months') .add(15, 'days') .toDate() // expires in 15 days + const lastDayToReconfirm2 = moment(confirmed2) + .add(maxConfirmationMonths, 'months') + .toDate() const user = { _id: '12390i', email: email1, @@ -383,17 +397,20 @@ describe('UserGetter', function () { { email: email1, reversedHostname: 'moc.noitailiffa-elpmaxe', - confirmedAt: new Date(), + confirmedAt: confirmed1, default: true, }, { email: email2, reversedHostname: 'moc.noitailiffa-elpmaxe', - confirmedAt: aboutToBeWithinPeriod, + confirmedAt: confirmed2, }, ], } - this.getUserAffiliations.resolves(affiliationsData) + const affiliations = [...affiliationsData] + affiliations[0].last_day_to_reconfirm = lastDayToReconfirm1 + affiliations[1].last_day_to_reconfirm = lastDayToReconfirm2 + this.getUserAffiliations.resolves(affiliations) this.UserGetter.promises.getUser = sinon.stub().resolves(user) this.UserGetter.getUserFullEmails( this.fakeUser._id, @@ -413,37 +430,14 @@ describe('UserGetter', function () { describe('SSO institutions', function () { it('should flag only linked email, if in notification period', function (done) { + const { maxConfirmationMonths } = institutionSSO const email1 = 'email1@sso.bar' const email2 = 'email2@sso.bar' const email3 = 'email3@sso.bar' - - const affiliationsData = [ - { - email: email1, - role: 'Prof', - department: 'Maths', - inferred: false, - licence: 'pro_plus', - institution: institutionSSO, - }, - { - email: email2, - role: 'Prof', - department: 'Maths', - inferred: false, - licence: 'pro_plus', - institution: institutionSSO, - }, - { - email: email3, - role: 'Prof', - department: 'Maths', - inferred: false, - licence: 'pro_plus', - institution: institutionSSO, - }, - ] - + const reconfirmedAt = new Date('2019-09-24') + const lastDayToReconfirm = moment(reconfirmedAt) + .add(maxConfirmationMonths, 'months') + .toDate() const user = { _id: '12390i', email: email1, @@ -452,21 +446,21 @@ describe('UserGetter', function () { email: email1, reversedHostname: 'rab.oss', confirmedAt: new Date('2019-09-24'), - reconfirmedAt: new Date('2019-09-24'), + reconfirmedAt: reconfirmedAt, default: true, }, { email: email2, reversedHostname: 'rab.oss', confirmedAt: new Date('2019-09-24'), - reconfirmedAt: new Date('2019-09-24'), + reconfirmedAt: reconfirmedAt, samlProviderId: institutionSSO.id, }, { email: email3, reversedHostname: 'rab.oss', confirmedAt: new Date('2019-09-24'), - reconfirmedAt: new Date('2019-09-24'), + reconfirmedAt: reconfirmedAt, }, ], samlIdentifiers: [ @@ -476,7 +470,36 @@ describe('UserGetter', function () { }, ], } - this.getUserAffiliations.resolves(affiliationsData) + const affiliations = [ + { + email: email1, + role: 'Prof', + department: 'Maths!', + inferred: false, + licence: 'pro_plus', + institution: institutionSSO, + last_day_to_reconfirm: lastDayToReconfirm, + }, + { + email: email2, + role: 'Prof', + department: 'Maths', + inferred: false, + licence: 'pro_plus', + institution: institutionSSO, + last_day_to_reconfirm: lastDayToReconfirm, + }, + { + email: email3, + role: 'Prof', + department: 'Maths', + inferred: false, + licence: 'pro_plus', + institution: institutionSSO, + last_day_to_reconfirm: lastDayToReconfirm, + }, + ] + this.getUserAffiliations.resolves(affiliations) this.UserGetter.promises.getUser = sinon.stub().resolves(user) this.UserGetter.getUserFullEmails( this.fakeUser._id, @@ -499,10 +522,19 @@ describe('UserGetter', function () { describe('multiple institution affiliations', function () { it('should flag each institution', function (done) { + const { maxConfirmationMonths } = institutionSSO const email1 = 'email1@sso.bar' const email2 = 'email2@sso.bar' const email3 = 'email3@foo.bar' const email4 = 'email4@foo.bar' + const confirmed1 = new Date('2019-09-24T20:25:08.503Z') + const lastDayToReconfirm1 = moment(confirmed1) + .add(maxConfirmationMonths, 'months') + .toDate() + const confirmed2 = new Date('2019-10-24T20:25:08.503Z') + const lastDayToReconfirm2 = moment(confirmed2) + .add(maxConfirmationMonths, 'months') + .toDate() const affiliationsData = [ { @@ -512,6 +544,7 @@ describe('UserGetter', function () { inferred: false, licence: 'pro_plus', institution: institutionSSO, + last_day_to_reconfirm: lastDayToReconfirm1, }, { email: email2, @@ -520,6 +553,7 @@ describe('UserGetter', function () { inferred: false, licence: 'pro_plus', institution: institutionSSO, + last_day_to_reconfirm: lastDayToReconfirm1, }, { email: email3, @@ -528,6 +562,7 @@ describe('UserGetter', function () { inferred: false, licence: 'pro_plus', institution: institutionNonSSO, + last_day_to_reconfirm: lastDayToReconfirm2, }, { email: email4, @@ -536,6 +571,7 @@ describe('UserGetter', function () { inferred: false, licence: 'pro_plus', institution: institutionNonSSO, + last_day_to_reconfirm: lastDayToReconfirm1, }, ] const user = { @@ -545,24 +581,24 @@ describe('UserGetter', function () { { email: email1, reversedHostname: 'rab.oss', - confirmedAt: '2019-09-24T20:25:08.503Z', + confirmedAt: confirmed1, default: true, }, { email: email2, reversedHostname: 'rab.oss', - confirmedAt: new Date('2019-09-24T20:25:08.503Z'), + confirmedAt: confirmed1, samlProviderId: institutionSSO.id, }, { email: email3, reversedHostname: 'rab.oof', - confirmedAt: new Date('2019-10-24T20:25:08.503Z'), + confirmedAt: confirmed2, }, { email: email4, reversedHostname: 'rab.oof', - confirmedAt: new Date('2019-09-24T20:25:08.503Z'), + confirmedAt: confirmed1, }, ], samlIdentifiers: [ @@ -572,6 +608,7 @@ describe('UserGetter', function () { }, ], } + this.getUserAffiliations.resolves(affiliationsData) this.UserGetter.promises.getUser = sinon.stub().resolves(user) this.UserGetter.getUserFullEmails( @@ -598,9 +635,31 @@ describe('UserGetter', function () { describe('reconfirmedAt', function () { it('only use confirmedAt when no reconfirmedAt', function (done) { + const { maxConfirmationMonths } = institutionSSO const email1 = 'email1@foo.bar' + const reconfirmed1 = moment().subtract( + maxConfirmationMonths * 3, + 'months' + ) + const lastDayToReconfirm1 = moment(reconfirmed1) + .add(maxConfirmationMonths, 'months') + .toDate() const email2 = 'email2@foo.bar' + const reconfirmed2 = moment().subtract( + maxConfirmationMonths * 2, + 'months' + ) + const lastDayToReconfirm2 = moment(reconfirmed2) + .add(maxConfirmationMonths, 'months') + .toDate() const email3 = 'email3@foo.bar' + const reconfirmed3 = moment().subtract( + maxConfirmationMonths * 4, + 'months' + ) + const lastDayToReconfirm3 = moment(reconfirmed3) + .add(maxConfirmationMonths, 'months') + .toDate() const affiliationsData = [ { @@ -610,6 +669,7 @@ describe('UserGetter', function () { inferred: false, licence: 'pro_plus', institution: institutionNonSSO, + last_day_to_reconfirm: lastDayToReconfirm1, }, { email: email2, @@ -618,6 +678,7 @@ describe('UserGetter', function () { inferred: false, licence: 'pro_plus', institution: institutionNonSSO, + last_day_to_reconfirm: lastDayToReconfirm2, }, { email: email3, @@ -626,6 +687,7 @@ describe('UserGetter', function () { inferred: false, licence: 'pro_plus', institution: institutionNonSSO, + last_day_to_reconfirm: lastDayToReconfirm3, }, ] const user = { @@ -639,10 +701,7 @@ describe('UserGetter', function () { institutionSSO.maxConfirmationMonths * 2, 'months' ), - reconfirmedAt: moment().subtract( - institutionSSO.maxConfirmationMonths * 3, - 'months' - ), + reconfirmedAt: reconfirmed1, default: true, }, { @@ -652,10 +711,7 @@ describe('UserGetter', function () { institutionSSO.maxConfirmationMonths * 3, 'months' ), - reconfirmedAt: moment().subtract( - institutionSSO.maxConfirmationMonths * 2, - 'months' - ), + reconfirmedAt: reconfirmed2, }, { email: email3, @@ -664,10 +720,7 @@ describe('UserGetter', function () { institutionSSO.maxConfirmationMonths * 4, 'months' ), - reconfirmedAt: moment().subtract( - institutionSSO.maxConfirmationMonths * 4, - 'months' - ), + reconfirmedAt: reconfirmed3, }, ], } @@ -695,8 +748,9 @@ describe('UserGetter', function () { describe('before reconfirmation period expires and within reconfirmation notification period', function () { const email = 'leonard@example-affiliation.com' it('should flag the email', function (done) { + const { maxConfirmationMonths } = institutionNonSSO const confirmedAt = moment() - .subtract(institutionNonSSO.maxConfirmationMonths, 'months') + .subtract(maxConfirmationMonths, 'months') .subtract(14, 'days') .toDate() // expires in 14 days const affiliationsData = [ @@ -707,6 +761,9 @@ describe('UserGetter', function () { inferred: false, licence: 'pro_plus', institution: institutionNonSSO, + last_day_to_reconfirm: moment(confirmedAt) + .add(maxConfirmationMonths, 'months') + .toDate(), }, ] const user = { @@ -844,6 +901,45 @@ describe('UserGetter', function () { ) }) + it('should flag to show notification if v1 shows as reconfirmation upcoming but v2 does not', function (done) { + const email = 'abc123@test.com' + const { maxConfirmationMonths } = institutionNonSSO + const affiliationsData = [ + { + email, + licence: 'free', + institution: institutionNonSSO, + last_day_to_reconfirm: moment() + .subtract(maxConfirmationMonths, 'months') + .add(3, 'day') + .toDate(), + }, + ] + const user = { + _id: '12390i', + email, + emails: [ + { + email, + confirmedAt: new Date(), + default: true, + }, + ], + } + this.getUserAffiliations.resolves(affiliationsData) + this.UserGetter.promises.getUser = sinon.stub().resolves(user) + this.UserGetter.getUserFullEmails( + this.fakeUser._id, + (error, fullEmails) => { + expect(error).to.not.exist + expect( + fullEmails[0].affiliation.inReconfirmNotificationPeriod + ).to.equal(true) + done() + } + ) + }) + describe('cachedLastDayToReconfirm', function () { const email = 'abc123@test.com' const confirmedAt = new Date('2019-07-11T18:25:01.639Z')