From 5af039eef05d7864ee47665da782b8aaf4a81949 Mon Sep 17 00:00:00 2001 From: Alexandre Bourdin Date: Thu, 10 Jun 2021 10:13:06 +0200 Subject: [PATCH] Merge pull request #4113 from overleaf/ta-post-registration-analytics Add Post Registration Analytics Job GitOrigin-RevId: f0d83eeea2e32915782e916cb40a768d5c1b6116 --- .../web/app/src/Features/User/UserCreator.js | 4 + .../UserPostRegistrationAnalyticsManager.js | 50 +++++++ services/web/app/src/infrastructure/Queues.js | 5 + .../test/unit/src/User/UserCreatorTests.js | 16 ++- ...erPostRegistrationAnalyticsManagerTests.js | 122 ++++++++++++++++++ 5 files changed, 195 insertions(+), 2 deletions(-) create mode 100644 services/web/app/src/Features/User/UserPostRegistrationAnalyticsManager.js create mode 100644 services/web/test/unit/src/User/UserPostRegistrationAnalyticsManagerTests.js diff --git a/services/web/app/src/Features/User/UserCreator.js b/services/web/app/src/Features/User/UserCreator.js index ae915bb9ba..95f6917b89 100644 --- a/services/web/app/src/Features/User/UserCreator.js +++ b/services/web/app/src/Features/User/UserCreator.js @@ -8,6 +8,7 @@ const UserGetter = require('./UserGetter') const UserUpdater = require('./UserUpdater') const Analytics = require('../Analytics/AnalyticsManager') const UserOnboardingEmailQueueManager = require('./UserOnboardingEmailManager') +const UserPostRegistrationAnalyticsManager = require('./UserPostRegistrationAnalyticsManager') const OError = require('@overleaf/o-error') async function _addAffiliation(user, affiliationOptions) { @@ -89,6 +90,9 @@ async function createNewUser(attributes, options = {}) { if (Features.hasFeature('saas')) { try { await UserOnboardingEmailQueueManager.scheduleOnboardingEmail(user) + await UserPostRegistrationAnalyticsManager.schedulePostRegistrationAnalytics( + user + ) } catch (error) { logger.error( OError.tag(error, 'Failed to schedule sending of onboarding email', { diff --git a/services/web/app/src/Features/User/UserPostRegistrationAnalyticsManager.js b/services/web/app/src/Features/User/UserPostRegistrationAnalyticsManager.js new file mode 100644 index 0000000000..a7a288017a --- /dev/null +++ b/services/web/app/src/Features/User/UserPostRegistrationAnalyticsManager.js @@ -0,0 +1,50 @@ +const Queues = require('../../infrastructure/Queues') +const UserGetter = require('./UserGetter') +const { + promises: InstitutionsAPIPromises, +} = require('../Institutions/InstitutionsAPI') +const AnalyticsManager = require('../Analytics/AnalyticsManager') + +const ONE_DAY_MS = 24 * 60 * 60 * 1000 + +class UserPostRegistrationAnalyticsManager { + constructor() { + this.queue = Queues.getPostRegistrationAnalyticsQueue() + this.queue.process(async job => { + const { userId } = job.data + await postRegistrationAnalytics(userId) + }) + } + + async schedulePostRegistrationAnalytics(user) { + await this.queue.add({ userId: user._id }, { delay: ONE_DAY_MS }) + } +} + +async function postRegistrationAnalytics(userId) { + const user = await UserGetter.promises.getUser({ _id: userId }, { email: 1 }) + if (!user) { + return + } + await checkAffiliations(userId) +} + +async function checkAffiliations(userId) { + const affiliationsData = await InstitutionsAPIPromises.getUserAffiliations( + userId + ) + const hasCommonsAccountAffiliation = affiliationsData.some( + affiliationData => + affiliationData.institution && affiliationData.institution.commonsAccount + ) + + if (hasCommonsAccountAffiliation) { + await AnalyticsManager.setUserProperty( + userId, + 'registered-from-commons-account', + true + ) + } +} + +module.exports = new UserPostRegistrationAnalyticsManager() diff --git a/services/web/app/src/infrastructure/Queues.js b/services/web/app/src/infrastructure/Queues.js index 2bd7e5e667..8bfd88b9d1 100644 --- a/services/web/app/src/infrastructure/Queues.js +++ b/services/web/app/src/infrastructure/Queues.js @@ -31,6 +31,10 @@ function getOnboardingEmailsQueue() { return getOrCreateQueue('emails-onboarding') } +function getPostRegistrationAnalyticsQueue() { + return getOrCreateQueue('post-registration-analytics') +} + function getOrCreateQueue(queueName, defaultJobOptions) { if (!queues[queueName]) { queues[queueName] = new Queue(queueName, { @@ -54,4 +58,5 @@ module.exports = { getAnalyticsEditingSessionsQueue, getAnalyticsUserPropertiesQueue, getOnboardingEmailsQueue, + getPostRegistrationAnalyticsQueue, } diff --git a/services/web/test/unit/src/User/UserCreatorTests.js b/services/web/test/unit/src/User/UserCreatorTests.js index 579e1b16c1..82cbffd64c 100644 --- a/services/web/test/unit/src/User/UserCreatorTests.js +++ b/services/web/test/unit/src/User/UserCreatorTests.js @@ -48,6 +48,9 @@ describe('UserCreator', function () { './UserOnboardingEmailManager': (this.UserOnboardingEmailManager = { scheduleOnboardingEmail: sinon.stub(), }), + './UserPostRegistrationAnalyticsManager': (this.UserPostRegistrationAnalyticsManager = { + schedulePostRegistrationAnalytics: sinon.stub(), + }), }, }) @@ -279,7 +282,7 @@ describe('UserCreator', function () { ) }) - it('should schedule an onboarding email on registration with saas feature', async function () { + it('should schedule post registration jobs on registration with saas feature', async function () { this.Features.hasFeature = sinon.stub().withArgs('saas').returns(true) const user = await this.UserCreator.promises.createNewUser({ email: this.email, @@ -289,14 +292,23 @@ describe('UserCreator', function () { this.UserOnboardingEmailManager.scheduleOnboardingEmail, user ) + sinon.assert.calledWith( + this.UserPostRegistrationAnalyticsManager + .schedulePostRegistrationAnalytics, + user + ) }) - it('should not add schedule onboarding email when without saas feature', async function () { + it('should not schedule post registration checks when without saas feature', async function () { const attributes = { email: this.email } await this.UserCreator.promises.createNewUser(attributes) sinon.assert.notCalled( this.UserOnboardingEmailManager.scheduleOnboardingEmail ) + sinon.assert.notCalled( + this.UserPostRegistrationAnalyticsManager + .schedulePostRegistrationAnalytics + ) }) }) }) diff --git a/services/web/test/unit/src/User/UserPostRegistrationAnalyticsManagerTests.js b/services/web/test/unit/src/User/UserPostRegistrationAnalyticsManagerTests.js new file mode 100644 index 0000000000..8b33c537d6 --- /dev/null +++ b/services/web/test/unit/src/User/UserPostRegistrationAnalyticsManagerTests.js @@ -0,0 +1,122 @@ +const SandboxedModule = require('sandboxed-module') +const path = require('path') +const sinon = require('sinon') + +const MODULE_PATH = path.join( + __dirname, + '../../../../app/src/Features/User/UserPostRegistrationAnalyticsManager' +) + +describe('UserPostRegistrationAnalyticsManager', function () { + beforeEach(function () { + this.fakeUserId = '123abc' + this.postRegistrationAnalyticsQueue = { + add: sinon.stub().resolves(), + process: callback => { + this.queueProcessFunction = callback + }, + } + const self = this + this.Queues = { + getPostRegistrationAnalyticsQueue: () => { + return self.postRegistrationAnalyticsQueue + }, + } + this.UserGetter = { + promises: { + getUser: sinon.stub().resolves({ _id: this.fakeUserId }), + }, + } + this.InstitutionsAPI = { + promises: { + getUserAffiliations: sinon.stub().resolves([]), + }, + } + 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( + { + _id: this.fakeUserId, + } + ) + sinon.assert.calledWithMatch( + this.postRegistrationAnalyticsQueue.add, + { userId: this.fakeUserId }, + { delay: 24 * 60 * 60 * 1000 } + ) + }) + }) + + 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) + }) + + 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, + }) + 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) + }) + }) +})