From 7430d7f558633c7fd2e06bc6072e172237586f85 Mon Sep 17 00:00:00 2001 From: Miguel Serrano Date: Thu, 1 Jul 2021 13:42:30 +0200 Subject: [PATCH] Merge pull request #4287 from overleaf/jpa-no-analytics-queues-server-ce-pro [misc] do not set up analytics queues in Server CE/Pro GitOrigin-RevId: 61a62f0ff7f04d5206845e01c68097229a5954fd --- .../User/UserOnboardingEmailManager.js | 9 +- .../UserPostRegistrationAnalyticsManager.js | 9 +- .../User/UserOnboardingEmailManagerTests.js | 58 ++++-- ...erPostRegistrationAnalyticsManagerTests.js | 172 ++++++++++-------- 4 files changed, 158 insertions(+), 90 deletions(-) diff --git a/services/web/app/src/Features/User/UserOnboardingEmailManager.js b/services/web/app/src/Features/User/UserOnboardingEmailManager.js index 350c40aab4..fb990a525d 100644 --- a/services/web/app/src/Features/User/UserOnboardingEmailManager.js +++ b/services/web/app/src/Features/User/UserOnboardingEmailManager.js @@ -1,3 +1,4 @@ +const Features = require('../../infrastructure/Features') const Queues = require('../../infrastructure/Queues') const EmailHandler = require('../Email/EmailHandler') const UserUpdater = require('./UserUpdater') @@ -34,4 +35,10 @@ class UserOnboardingEmailManager { } } -module.exports = new UserOnboardingEmailManager() +class NoopManager { + async scheduleOnboardingEmail() {} +} + +module.exports = Features.hasFeature('saas') + ? new UserOnboardingEmailManager() + : new NoopManager() diff --git a/services/web/app/src/Features/User/UserPostRegistrationAnalyticsManager.js b/services/web/app/src/Features/User/UserPostRegistrationAnalyticsManager.js index a7a288017a..d378d9257d 100644 --- a/services/web/app/src/Features/User/UserPostRegistrationAnalyticsManager.js +++ b/services/web/app/src/Features/User/UserPostRegistrationAnalyticsManager.js @@ -4,6 +4,7 @@ const { promises: InstitutionsAPIPromises, } = require('../Institutions/InstitutionsAPI') const AnalyticsManager = require('../Analytics/AnalyticsManager') +const Features = require('../../infrastructure/Features') const ONE_DAY_MS = 24 * 60 * 60 * 1000 @@ -47,4 +48,10 @@ async function checkAffiliations(userId) { } } -module.exports = new UserPostRegistrationAnalyticsManager() +class NoopManager { + async schedulePostRegistrationAnalytics() {} +} + +module.exports = Features.hasFeature('saas') + ? new UserPostRegistrationAnalyticsManager() + : new NoopManager() diff --git a/services/web/test/unit/src/User/UserOnboardingEmailManagerTests.js b/services/web/test/unit/src/User/UserOnboardingEmailManagerTests.js index 0776b0eb9e..dc3fb69d31 100644 --- a/services/web/test/unit/src/User/UserOnboardingEmailManagerTests.js +++ b/services/web/test/unit/src/User/UserOnboardingEmailManagerTests.js @@ -1,6 +1,7 @@ const SandboxedModule = require('sandboxed-module') const path = require('path') const sinon = require('sinon') +const { expect } = require('chai') const MODULE_PATH = path.join( __dirname, @@ -17,11 +18,10 @@ describe('UserOnboardingEmailManager', function () { this.queueProcessFunction = callback }, } - const self = this this.Queues = { - getOnboardingEmailsQueue: () => { - return self.onboardingEmailsQueue - }, + getOnboardingEmailsQueue: sinon + .stub() + .returns(this.onboardingEmailsQueue), } this.UserGetter = { promises: { @@ -41,21 +41,49 @@ describe('UserOnboardingEmailManager', function () { updateUser: sinon.stub().resolves(), }, } + this.Features = { + hasFeature: sinon.stub(), + } this.request = sinon.stub().yields() - this.UserOnboardingEmailManager = SandboxedModule.require(MODULE_PATH, { - globals: { - console: console, - }, - requires: { - '../../infrastructure/Queues': this.Queues, - '../Email/EmailHandler': this.EmailHandler, - './UserGetter': this.UserGetter, - './UserUpdater': this.UserUpdater, - }, + + this.init = isSAAS => { + this.Features.hasFeature.withArgs('saas').returns(isSAAS) + this.UserOnboardingEmailManager = SandboxedModule.require(MODULE_PATH, { + globals: { + console: console, + }, + requires: { + '../../infrastructure/Features': this.Features, + '../../infrastructure/Queues': this.Queues, + '../Email/EmailHandler': this.EmailHandler, + './UserGetter': this.UserGetter, + './UserUpdater': this.UserUpdater, + }, + }) + } + }) + + describe('in Server CE/Pro', function () { + beforeEach(function () { + this.init(false) + }) + + it('should not create any queue', function () { + expect(this.Queues.getOnboardingEmailsQueue).to.not.have.been.called + }) + it('should not schedule any email', function () { + this.UserOnboardingEmailManager.scheduleOnboardingEmail({ + _id: this.fakeUserId, + }) + expect(this.onboardingEmailsQueue.add).to.not.have.been.called }) }) - describe('schedule email', function () { + describe('schedule email in SAAS', function () { + beforeEach(function () { + this.init(true) + }) + it('should schedule delayed job on queue', function () { this.UserOnboardingEmailManager.scheduleOnboardingEmail({ _id: this.fakeUserId, diff --git a/services/web/test/unit/src/User/UserPostRegistrationAnalyticsManagerTests.js b/services/web/test/unit/src/User/UserPostRegistrationAnalyticsManagerTests.js index 8b33c537d6..16be0c0d0d 100644 --- a/services/web/test/unit/src/User/UserPostRegistrationAnalyticsManagerTests.js +++ b/services/web/test/unit/src/User/UserPostRegistrationAnalyticsManagerTests.js @@ -1,6 +1,7 @@ const SandboxedModule = require('sandboxed-module') const path = require('path') const sinon = require('sinon') +const { expect } = require('chai') const MODULE_PATH = path.join( __dirname, @@ -16,11 +17,10 @@ describe('UserPostRegistrationAnalyticsManager', function () { this.queueProcessFunction = callback }, } - const self = this this.Queues = { - getPostRegistrationAnalyticsQueue: () => { - return self.postRegistrationAnalyticsQueue - }, + getPostRegistrationAnalyticsQueue: sinon + .stub() + .returns(this.postRegistrationAnalyticsQueue), } this.UserGetter = { promises: { @@ -35,88 +35,114 @@ describe('UserPostRegistrationAnalyticsManager', function () { this.AnalyticsManager = { setUserProperty: sinon.stub().resolves(), } - this.UserPostRegistrationAnalyticsManager = SandboxedModule.require( - MODULE_PATH, - { - globals: { - console: console, - }, - requires: { - '../../infrastructure/Queues': this.Queues, - './UserGetter': this.UserGetter, - '../Institutions/InstitutionsAPI': this.InstitutionsAPI, - '../Analytics/AnalyticsManager': this.AnalyticsManager, - }, - } - ) - }) - - describe('schedule jobs', function () { - it('should schedule delayed job on queue', function () { - this.UserPostRegistrationAnalyticsManager.schedulePostRegistrationAnalytics( + this.Features = { + hasFeature: sinon.stub().returns(true), + } + this.init = isSAAS => { + this.Features.hasFeature.withArgs('saas').returns(isSAAS) + this.UserPostRegistrationAnalyticsManager = SandboxedModule.require( + MODULE_PATH, { - _id: this.fakeUserId, + requires: { + '../../infrastructure/Features': this.Features, + '../../infrastructure/Queues': this.Queues, + './UserGetter': this.UserGetter, + '../Institutions/InstitutionsAPI': this.InstitutionsAPI, + '../Analytics/AnalyticsManager': this.AnalyticsManager, + }, } ) - sinon.assert.calledWithMatch( - this.postRegistrationAnalyticsQueue.add, - { userId: this.fakeUserId }, - { delay: 24 * 60 * 60 * 1000 } + } + }) + + describe('in Server CE/Pro', function () { + beforeEach(function () { + this.init(false) + }) + + it('should schedule delayed job on queue', function () { + this.UserPostRegistrationAnalyticsManager.schedulePostRegistrationAnalytics( + { _id: this.fakeUserId } ) + expect(this.Queues.getPostRegistrationAnalyticsQueue).to.not.have.been + .called + expect(this.postRegistrationAnalyticsQueue.add).to.not.have.been.called }) }) - describe('process jobs', function () { - it('stops without errors if user is not found', async function () { - this.UserGetter.promises.getUser.resolves(null) - await this.queueProcessFunction({ data: { userId: this.fakeUserId } }) - sinon.assert.calledWith(this.UserGetter.promises.getUser, { - _id: this.fakeUserId, + describe('in SAAS', function () { + beforeEach(function () { + this.init(true) + }) + describe('schedule jobs in SAAS', function () { + it('should schedule delayed job on queue', function () { + this.UserPostRegistrationAnalyticsManager.schedulePostRegistrationAnalytics( + { + _id: this.fakeUserId, + } + ) + sinon.assert.calledWithMatch( + this.postRegistrationAnalyticsQueue.add, + { userId: this.fakeUserId }, + { delay: 24 * 60 * 60 * 1000 } + ) }) - sinon.assert.notCalled(this.InstitutionsAPI.promises.getUserAffiliations) - sinon.assert.notCalled(this.AnalyticsManager.setUserProperty) }) - it('sets user property if user has commons account affiliationd', async function () { - this.InstitutionsAPI.promises.getUserAffiliations.resolves([ - {}, - { - institution: { - commonsAccount: true, - }, - }, - { - institution: { - commonsAccount: false, - }, - }, - ]) - await this.queueProcessFunction({ data: { userId: this.fakeUserId } }) - sinon.assert.calledWith(this.UserGetter.promises.getUser, { - _id: this.fakeUserId, + describe('process jobs', function () { + it('stops without errors if user is not found', async function () { + this.UserGetter.promises.getUser.resolves(null) + await this.queueProcessFunction({ data: { userId: this.fakeUserId } }) + sinon.assert.calledWith(this.UserGetter.promises.getUser, { + _id: this.fakeUserId, + }) + sinon.assert.notCalled( + this.InstitutionsAPI.promises.getUserAffiliations + ) + sinon.assert.notCalled(this.AnalyticsManager.setUserProperty) }) - sinon.assert.calledWith( - this.InstitutionsAPI.promises.getUserAffiliations, - this.fakeUserId - ) - sinon.assert.calledWith( - this.AnalyticsManager.setUserProperty, - this.fakeUserId, - 'registered-from-commons-account', - true - ) - }) - it('does not set user property if user has no commons account affiliation', async function () { - this.InstitutionsAPI.promises.getUserAffiliations.resolves([ - { - institution: { - commonsAccount: false, + it('sets user property if user has commons account affiliationd', async function () { + this.InstitutionsAPI.promises.getUserAffiliations.resolves([ + {}, + { + institution: { + commonsAccount: true, + }, }, - }, - ]) - await this.queueProcessFunction({ data: { userId: this.fakeUserId } }) - sinon.assert.notCalled(this.AnalyticsManager.setUserProperty) + { + institution: { + commonsAccount: false, + }, + }, + ]) + await this.queueProcessFunction({ data: { userId: this.fakeUserId } }) + sinon.assert.calledWith(this.UserGetter.promises.getUser, { + _id: this.fakeUserId, + }) + sinon.assert.calledWith( + this.InstitutionsAPI.promises.getUserAffiliations, + this.fakeUserId + ) + sinon.assert.calledWith( + this.AnalyticsManager.setUserProperty, + this.fakeUserId, + 'registered-from-commons-account', + true + ) + }) + + it('does not set user property if user has no commons account affiliation', async function () { + this.InstitutionsAPI.promises.getUserAffiliations.resolves([ + { + institution: { + commonsAccount: false, + }, + }, + ]) + await this.queueProcessFunction({ data: { userId: this.fakeUserId } }) + sinon.assert.notCalled(this.AnalyticsManager.setUserProperty) + }) }) }) })