From 5b8de28250bac3bfe41b6e5c75ea35884e0b0212 Mon Sep 17 00:00:00 2001 From: Alexandre Bourdin Date: Thu, 9 Sep 2021 19:03:29 +0200 Subject: [PATCH] Merge pull request #5050 from overleaf/revert-4639-ab-web-mono-analytics-id Revert "Analytics ID support" GitOrigin-RevId: cc5da762ba1bafcbcea65ed0dd86342896b6d1eb --- .../Features/Analytics/AnalyticsController.js | 8 +- .../Features/Analytics/AnalyticsManager.js | 177 ++++-------------- .../AnalyticsRegistrationSourceHelper.js | 8 +- .../Analytics/UserAnalyticsIdCache.js | 26 --- .../AuthenticationController.js | 15 +- .../CollaboratorsInviteController.js | 12 +- .../src/Features/Project/ProjectController.js | 2 +- .../Project/ProjectCreationHandler.js | 10 +- .../Features/SplitTests/SplitTestHandler.js | 2 +- .../Features/SplitTests/SplitTestV2Handler.js | 153 +++------------ .../Features/Subscription/FeaturesUpdater.js | 2 +- .../Subscription/RecurlyEventHandler.js | 112 ++++------- .../Subscription/SubscriptionController.js | 2 +- .../Subscription/SubscriptionUpdater.js | 2 +- .../TokenAccess/TokenAccessHandler.js | 8 +- .../web/app/src/Features/User/UserCreator.js | 12 +- .../UserPostRegistrationAnalyticsManager.js | 2 +- .../Features/User/UserRegistrationHandler.js | 3 +- services/web/app/src/infrastructure/Server.js | 5 - services/web/app/src/models/User.js | 1 - .../server-ce-scripts/scripts/create-user.js | 2 +- .../src/Analytics/AnalyticsControllerTests.js | 30 ++- .../src/Analytics/AnalyticsManagerTests.js | 21 +-- .../AuthenticationControllerTests.js | 20 +- .../CollaboratorsInviteControllerTests.js | 2 +- .../src/Project/ProjectControllerTests.js | 2 +- .../src/Subscription/FeaturesUpdaterTests.js | 6 +- .../Subscription/RecurlyEventHandlerTests.js | 52 ++--- .../SubscriptionControllerTests.js | 5 +- .../Subscription/SubscriptionHandlerTests.js | 2 +- .../Subscription/SubscriptionUpdaterTests.js | 14 +- .../TokenAccess/TokenAccessHandlerTests.js | 7 +- .../test/unit/src/User/UserControllerTests.js | 8 +- .../test/unit/src/User/UserCreatorTests.js | 8 +- ...erPostRegistrationAnalyticsManagerTests.js | 12 +- .../src/User/UserRegistrationHandlerTests.js | 53 +++--- 36 files changed, 249 insertions(+), 557 deletions(-) delete mode 100644 services/web/app/src/Features/Analytics/UserAnalyticsIdCache.js diff --git a/services/web/app/src/Features/Analytics/AnalyticsController.js b/services/web/app/src/Features/Analytics/AnalyticsController.js index f50470a2b3..f18cee429d 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsController.js +++ b/services/web/app/src/Features/Analytics/AnalyticsController.js @@ -30,12 +30,10 @@ module.exports = { if (!Features.hasFeature('analytics')) { return res.sendStatus(202) } + const userId = + SessionManager.getLoggedInUserId(req.session) || req.sessionID delete req.body._csrf - AnalyticsManager.recordEventForSession( - req.session, - req.params.event, - req.body - ) + AnalyticsManager.recordEvent(userId, req.params.event, req.body) res.sendStatus(202) }, } diff --git a/services/web/app/src/Features/Analytics/AnalyticsManager.js b/services/web/app/src/Features/Analytics/AnalyticsManager.js index 2437d001ed..278ea4691c 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsManager.js +++ b/services/web/app/src/Features/Analytics/AnalyticsManager.js @@ -1,32 +1,21 @@ -const SessionManager = require('../Authentication/SessionManager') -const UserAnalyticsIdCache = require('./UserAnalyticsIdCache') const Settings = require('@overleaf/settings') const Metrics = require('../../infrastructure/Metrics') const Queues = require('../../infrastructure/Queues') -const uuid = require('uuid') -const _ = require('lodash') -const { expressify } = require('../../util/promises') const analyticsEventsQueue = Queues.getAnalyticsEventsQueue() const analyticsEditingSessionsQueue = Queues.getAnalyticsEditingSessionsQueue() const analyticsUserPropertiesQueue = Queues.getAnalyticsUserPropertiesQueue() -const ONE_MINUTE_MS = 60 * 1000 - -function identifyUser(userId, analyticsId, isNewUser) { - if (!userId || !analyticsId) { +function identifyUser(userId, oldUserId) { + if (!userId || !oldUserId) { return } - if (_isAnalyticsDisabled() || _isSmokeTestUser(userId)) { + if (isAnalyticsDisabled() || isSmokeTestUser(userId)) { return } Metrics.analyticsQueue.inc({ status: 'adding', event_type: 'identify' }) analyticsEventsQueue - .add( - 'identify', - { userId, analyticsId, isNewUser }, - { delay: ONE_MINUTE_MS } - ) + .add('identify', { userId, oldUserId }) .then(() => { Metrics.analyticsQueue.inc({ status: 'added', event_type: 'identify' }) }) @@ -35,81 +24,29 @@ function identifyUser(userId, analyticsId, isNewUser) { }) } -async function recordEventForUser(userId, event, segmentation) { +function recordEvent(userId, event, segmentation) { if (!userId) { return } - if (_isAnalyticsDisabled() || _isSmokeTestUser(userId)) { + if (isAnalyticsDisabled() || isSmokeTestUser(userId)) { return } - const analyticsId = await UserAnalyticsIdCache.get(userId) - if (analyticsId) { - _recordEvent({ analyticsId, userId, event, segmentation, isLoggedIn: true }) - } -} - -function recordEventForSession(session, event, segmentation) { - const { analyticsId, userId } = getIdsFromSession(session) - if (!analyticsId) { - return - } - if (_isAnalyticsDisabled() || _isSmokeTestUser(userId)) { - return - } - _recordEvent({ - analyticsId, - userId, - event, - segmentation, - isLoggedIn: !!userId, - }) -} - -async function setUserPropertyForUser(userId, propertyName, propertyValue) { - if (_isAnalyticsDisabled() || _isSmokeTestUser(userId)) { - return - } - - _checkPropertyValue(propertyValue) - - const analyticsId = await UserAnalyticsIdCache.get(userId) - if (analyticsId) { - _setUserProperty({ analyticsId, propertyName, propertyValue }) - } -} - -async function setUserPropertyForAnalyticsId( - analyticsId, - propertyName, - propertyValue -) { - if (_isAnalyticsDisabled()) { - return - } - - _checkPropertyValue(propertyValue) - - _setUserProperty({ analyticsId, propertyName, propertyValue }) -} - -async function setUserPropertyForSession(session, propertyName, propertyValue) { - const { analyticsId, userId } = getIdsFromSession(session) - if (_isAnalyticsDisabled() || _isSmokeTestUser(userId)) { - return - } - - _checkPropertyValue(propertyValue) - - if (analyticsId) { - _setUserProperty({ analyticsId, propertyName, propertyValue }) - } + Metrics.analyticsQueue.inc({ status: 'adding', event_type: 'event' }) + analyticsEventsQueue + .add('event', { userId, event, segmentation }) + .then(() => { + Metrics.analyticsQueue.inc({ status: 'added', event_type: 'event' }) + }) + .catch(() => { + Metrics.analyticsQueue.inc({ status: 'error', event_type: 'event' }) + }) } function updateEditingSession(userId, projectId, countryCode) { if (!userId) { return } - if (_isAnalyticsDisabled() || _isSmokeTestUser(userId)) { + if (isAnalyticsDisabled() || isSmokeTestUser(userId)) { return } Metrics.analyticsQueue.inc({ @@ -132,36 +69,26 @@ function updateEditingSession(userId, projectId, countryCode) { }) } -function _recordEvent( - { analyticsId, userId, event, segmentation, isLoggedIn }, - { delay } = {} -) { - Metrics.analyticsQueue.inc({ status: 'adding', event_type: 'event' }) - analyticsEventsQueue - .add( - 'event', - { analyticsId, userId, event, segmentation, isLoggedIn }, - { delay } - ) - .then(() => { - Metrics.analyticsQueue.inc({ status: 'added', event_type: 'event' }) - }) - .catch(() => { - Metrics.analyticsQueue.inc({ status: 'error', event_type: 'event' }) - }) -} +function setUserProperty(userId, propertyName, propertyValue) { + if (!userId) { + return + } + if (isAnalyticsDisabled() || isSmokeTestUser(userId)) { + return + } + + if (propertyValue === undefined) { + throw new Error( + 'propertyValue cannot be undefined, use null to unset a property' + ) + } -function _setUserProperty({ analyticsId, propertyName, propertyValue }) { Metrics.analyticsQueue.inc({ status: 'adding', event_type: 'user-property', }) analyticsUserPropertiesQueue - .add({ - analyticsId, - propertyName, - propertyValue, - }) + .add({ userId, propertyName, propertyValue }) .then(() => { Metrics.analyticsQueue.inc({ status: 'added', @@ -176,54 +103,18 @@ function _setUserProperty({ analyticsId, propertyName, propertyValue }) { }) } -function _isSmokeTestUser(userId) { +function isSmokeTestUser(userId) { const smokeTestUserId = Settings.smokeTest && Settings.smokeTest.userId return smokeTestUserId != null && userId.toString() === smokeTestUserId } -function _isAnalyticsDisabled() { +function isAnalyticsDisabled() { return !(Settings.analytics && Settings.analytics.enabled) } -function _checkPropertyValue(propertyValue) { - if (propertyValue === undefined) { - throw new Error( - 'propertyValue cannot be undefined, use null to unset a property' - ) - } -} - -function getIdsFromSession(session) { - const analyticsId = _.get(session, ['analyticsId']) - const userId = SessionManager.getLoggedInUserId(session) - return { analyticsId, userId } -} - -async function analyticsIdMiddleware(req, res, next) { - const session = req.session - const sessionUser = SessionManager.getSessionUser(session) - if (session.analyticsId) { - if (sessionUser && session.analyticsId !== sessionUser.analyticsId) { - session.analyticsId = sessionUser.analyticsId - } - } else { - if (sessionUser) { - session.analyticsId = sessionUser.analyticsId - } else { - session.analyticsId = uuid.v4() - } - } - next() -} - module.exports = { identifyUser, - recordEventForSession, - recordEventForUser, - setUserPropertyForUser, - setUserPropertyForSession, - setUserPropertyForAnalyticsId, + recordEvent, updateEditingSession, - getIdsFromSession, - analyticsIdMiddleware: expressify(analyticsIdMiddleware), + setUserProperty, } diff --git a/services/web/app/src/Features/Analytics/AnalyticsRegistrationSourceHelper.js b/services/web/app/src/Features/Analytics/AnalyticsRegistrationSourceHelper.js index fa31d62234..c29f91caad 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsRegistrationSourceHelper.js +++ b/services/web/app/src/Features/Analytics/AnalyticsRegistrationSourceHelper.js @@ -79,7 +79,7 @@ function addUserProperties(userId, session) { } if (session.referal_id) { - AnalyticsManager.setUserPropertyForUser( + AnalyticsManager.setUserProperty( userId, `registered-from-bonus-scheme`, true @@ -87,7 +87,7 @@ function addUserProperties(userId, session) { } if (session.required_login_for) { - AnalyticsManager.setUserPropertyForUser( + AnalyticsManager.setUserProperty( userId, `registered-from-${session.required_login_for}`, true @@ -96,7 +96,7 @@ function addUserProperties(userId, session) { if (session.inbound) { if (session.inbound.referrer) { - AnalyticsManager.setUserPropertyForUser( + AnalyticsManager.setUserProperty( userId, `registered-from-referrer-${session.inbound.referrer.medium}`, session.inbound.referrer.detail || 'other' @@ -106,7 +106,7 @@ function addUserProperties(userId, session) { if (session.inbound.utm) { for (const utmKey of UTM_KEYS) { if (session.inbound.utm[utmKey]) { - AnalyticsManager.setUserPropertyForUser( + AnalyticsManager.setUserProperty( userId, `registered-from-${utmKey.replace('_', '-')}`, session.inbound.utm[utmKey] diff --git a/services/web/app/src/Features/Analytics/UserAnalyticsIdCache.js b/services/web/app/src/Features/Analytics/UserAnalyticsIdCache.js deleted file mode 100644 index c665f68bad..0000000000 --- a/services/web/app/src/Features/Analytics/UserAnalyticsIdCache.js +++ /dev/null @@ -1,26 +0,0 @@ -const UserGetter = require('../User/UserGetter') -const { CacheLoader } = require('cache-flow') - -class UserAnalyticsIdCache extends CacheLoader { - constructor() { - super('user-analytics-id', { - expirationTime: 60, - maxSize: 10000, - }) - } - - async load(userId) { - const user = await UserGetter.promises.getUser(userId, { analyticsId: 1 }) - if (user) { - return user.analyticsId || user._id - } - } - - keyToString(userId) { - if (userId) { - return userId.toString() - } - } -} - -module.exports = new UserAnalyticsIdCache() diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index 6064a88b98..bbd0d4c543 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -48,7 +48,6 @@ const AuthenticationController = { ip_address: user._login_req_ip, must_reconfirm: user.must_reconfirm, v1_id: user.overleaf != null ? user.overleaf.id : undefined, - analyticsId: user.analyticsId || user._id, } callback(null, lightUser) }, @@ -83,9 +82,6 @@ const AuthenticationController = { return res.redirect('/login') } // OAuth2 'state' mismatch - const anonymousAnalyticsId = req.session.analyticsId - const isNewUser = req.session.justRegistered || false - const Modules = require('../../infrastructure/Modules') Modules.hooks.fire( 'preFinishLogin', @@ -110,7 +106,7 @@ const AuthenticationController = { const redir = AuthenticationController._getRedirectFromSession(req) || '/project' - _loginAsyncHandlers(req, user, anonymousAnalyticsId, isNewUser) + _loginAsyncHandlers(req, user) const userId = user._id UserAuditLogHandler.addEntry(userId, 'login', userId, req.ip, err => { if (err) { @@ -490,8 +486,7 @@ function _afterLoginSessionSetup(req, user, callback) { }) }) } - -function _loginAsyncHandlers(req, user, anonymousAnalyticsId, isNewUser) { +function _loginAsyncHandlers(req, user) { UserHandler.setupLoginData(user, err => { if (err != null) { logger.warn({ err }, 'error setting up login data') @@ -500,14 +495,12 @@ function _loginAsyncHandlers(req, user, anonymousAnalyticsId, isNewUser) { LoginRateLimiter.recordSuccessfulLogin(user.email) AuthenticationController._recordSuccessfulLogin(user._id) AuthenticationController.ipMatchCheck(req, user) - Analytics.recordEventForUser(user._id, 'user-logged-in') - Analytics.identifyUser(user._id, anonymousAnalyticsId, isNewUser) - + Analytics.recordEvent(user._id, 'user-logged-in') + Analytics.identifyUser(user._id, req.sessionID) logger.log( { email: user.email, user_id: user._id.toString() }, 'successful log in' ) - req.session.justLoggedIn = true // capture the request ip for use when creating the session return (user._login_req_ip = req.ip) diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js b/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js index 4c5a2b8d22..9a5383f5f7 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js @@ -377,14 +377,10 @@ module.exports = CollaboratorsInviteController = { 'project:membership:changed', { invites: true, members: true } ) - AnalyticsManager.recordEventForUser( - currentUser._id, - 'project-invite-accept', - { - projectId, - userId: currentUser._id, - } - ) + AnalyticsManager.recordEvent(currentUser._id, 'project-invite-accept', { + projectId, + userId: currentUser._id, + }) if (req.xhr) { return res.sendStatus(204) // Done async via project page notification } else { diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index 93a08b9701..e3bca01b9c 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -780,7 +780,7 @@ const ProjectController = { metrics.inc(metricName) if (userId) { - AnalyticsManager.recordEventForUser(userId, 'project-opened', { + AnalyticsManager.recordEvent(userId, 'project-opened', { projectId: project._id, }) } diff --git a/services/web/app/src/Features/Project/ProjectCreationHandler.js b/services/web/app/src/Features/Project/ProjectCreationHandler.js index 8b28d09923..be8729cc0a 100644 --- a/services/web/app/src/Features/Project/ProjectCreationHandler.js +++ b/services/web/app/src/Features/Project/ProjectCreationHandler.js @@ -35,12 +35,12 @@ async function createBlankProject(ownerId, projectName, attributes = {}) { const isImport = attributes && attributes.overleaf const project = await _createBlankProject(ownerId, projectName, attributes) if (isImport) { - AnalyticsManager.recordEventForUser(ownerId, 'project-imported', { + AnalyticsManager.recordEvent(ownerId, 'project-imported', { projectId: project._id, attributes, }) } else { - AnalyticsManager.recordEventForUser(ownerId, 'project-created', { + AnalyticsManager.recordEvent(ownerId, 'project-created', { projectId: project._id, attributes, }) @@ -50,7 +50,7 @@ async function createBlankProject(ownerId, projectName, attributes = {}) { async function createProjectFromSnippet(ownerId, projectName, docLines) { const project = await _createBlankProject(ownerId, projectName) - AnalyticsManager.recordEventForUser(ownerId, 'project-created', { + AnalyticsManager.recordEvent(ownerId, 'project-created', { projectId: project._id, }) await _createRootDoc(project, ownerId, docLines) @@ -63,7 +63,7 @@ async function createBasicProject(ownerId, projectName) { const docLines = await _buildTemplate('mainbasic.tex', ownerId, projectName) await _createRootDoc(project, ownerId, docLines) - AnalyticsManager.recordEventForUser(ownerId, 'project-created', { + AnalyticsManager.recordEvent(ownerId, 'project-created', { projectId: project._id, }) @@ -75,7 +75,7 @@ async function createExampleProject(ownerId, projectName) { await _addExampleProjectFiles(ownerId, projectName, project) - AnalyticsManager.recordEventForUser(ownerId, 'project-created', { + AnalyticsManager.recordEvent(ownerId, 'project-created', { projectId: project._id, }) diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index 989e21b14f..0a08aad2de 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -96,7 +96,7 @@ async function assignUserToVariant(userId, splitTest) { [`splitTests.${splitTest.id}`]: selectedVariant, }, }) - AnalyticsManager.setUserPropertyForUser( + AnalyticsManager.setUserProperty( userId, `split-test-${splitTest.id}`, selectedVariant diff --git a/services/web/app/src/Features/SplitTests/SplitTestV2Handler.js b/services/web/app/src/Features/SplitTests/SplitTestV2Handler.js index 7516d6499b..6d32618809 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestV2Handler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestV2Handler.js @@ -1,7 +1,6 @@ const UserGetter = require('../User/UserGetter') const UserUpdater = require('../User/UserUpdater') const AnalyticsManager = require('../Analytics/AnalyticsManager') -const UserAnalyticsIdCache = require('../Analytics/UserAnalyticsIdCache') const crypto = require('crypto') const _ = require('lodash') const { callbackify } = require('util') @@ -25,7 +24,7 @@ const BETA_PHASE = 'beta' * // execute the default behaviour (control group) * } * // then record an event - * AnalyticsManager.recordEventForUser(userId, 'example-project-created', { + * AnalyticsManager.recordEvent(userId, 'example-project-created', { * projectId: project._id, * ...assignment.analytics.segmentation * }) @@ -36,50 +35,8 @@ const BETA_PHASE = 'beta' * @returns {Promise<{variant: string, analytics: {segmentation: {splitTest: string, variant: string, phase: string, versionNumber: number}|{}}}>} */ async function getAssignment(userId, splitTestName, options) { - if (!userId) { - return { variant: 'default', analytics: { segmentation: {} } } - } - const analyticsId = await UserAnalyticsIdCache.get(userId) - return _getAssignment(analyticsId, userId, undefined, splitTestName, options) -} - -/** - * Get the assignment of a user to a split test by their session. - * - * @example - * // Assign user and record an event - * - * const assignment = await SplitTestV2Handler.getAssignment(req.session, 'example-project') - * if (assignment.variant === 'awesome-new-version') { - * // execute my awesome change - * } - * else { - * // execute the default behaviour (control group) - * } - * // then record an event - * AnalyticsManager.recordEventForSession(req.session, 'example-project-created', { - * projectId: project._id, - * ...assignment.analytics.segmentation - * }) - * - * @param session the request session - * @param splitTestName the unique name of the split test - * @param options {Object} - for test purposes only, to force the synchronous update of the user's profile - * @returns {Promise<{variant: string, analytics: {segmentation: {splitTest: string, variant: string, phase: string, versionNumber: number}|{}}}>} - */ -async function getAssignmentForSession(session, splitTestName, options) { - const { userId, analyticsId } = AnalyticsManager.getIdsFromSession(session) - return _getAssignment(analyticsId, userId, session, splitTestName, options) -} - -async function _getAssignment( - analyticsId, - userId, - session, - splitTestName, - options -) { const splitTest = await splitTestCache.get(splitTestName) + if (splitTest) { const currentVersion = splitTest.getCurrentVersion() if (currentVersion.active) { @@ -88,12 +45,10 @@ async function _getAssignment( selectedVariantName, phase, versionNumber, - } = await _getAssignmentMetadata(analyticsId, userId, splitTest) + } = await _getAssignmentMetadata(userId, splitTest) if (activeForUser) { const assignmentConfig = { userId, - analyticsId, - session, splitTestName, variantName: selectedVariantName, phase, @@ -134,44 +89,21 @@ async function assignInLocalsContext(res, userId, splitTestName, options) { res.locals.splitTestVariants[splitTestName] = assignment.variant } -async function assignInLocalsContextForSession( - res, - session, - splitTestName, - options -) { - const assignment = await getAssignmentForSession( - session, - splitTestName, - options - ) - if (!res.locals.splitTestVariants) { - res.locals.splitTestVariants = {} - } - res.locals.splitTestVariants[splitTestName] = assignment.variant -} - -async function _getAssignmentMetadata(analyticsId, userId, splitTest) { +async function _getAssignmentMetadata(userId, splitTest) { const currentVersion = splitTest.getCurrentVersion() const phase = currentVersion.phase if ([ALPHA_PHASE, BETA_PHASE].includes(phase)) { - if (userId) { - const user = await _getUser(userId) - if ( - (phase === ALPHA_PHASE && !(user && user.alphaProgram)) || - (phase === BETA_PHASE && !(user && user.betaProgram)) - ) { - return { - activeForUser: false, - } - } - } else { + const user = await _getUser(userId) + if ( + (phase === ALPHA_PHASE && !(user && user.alphaProgram)) || + (phase === BETA_PHASE && !(user && user.betaProgram)) + ) { return { activeForUser: false, } } } - const percentile = _getPercentile(analyticsId, splitTest.name, phase) + const percentile = _getPercentile(userId, splitTest.name, phase) const selectedVariantName = _getVariantFromPercentile( currentVersion.variants, percentile @@ -184,10 +116,10 @@ async function _getAssignmentMetadata(analyticsId, userId, splitTest) { } } -function _getPercentile(analyticsId, splitTestName, splitTestPhase) { +function _getPercentile(userId, splitTestName, splitTestPhase) { const hash = crypto .createHash('md5') - .update(analyticsId + splitTestName + splitTestPhase) + .update(userId + splitTestName + splitTestPhase) .digest('hex') const hashPrefix = hash.substr(0, 8) return Math.floor( @@ -207,52 +139,29 @@ function _getVariantFromPercentile(variants, percentile) { async function _updateVariantAssignment({ userId, - analyticsId, - session, splitTestName, phase, versionNumber, variantName, }) { - const persistedAssignment = { - variantName, - versionNumber, - phase, - assignedAt: new Date(), - } - if (userId) { - const user = await _getUser(userId) - if (user) { - const assignedSplitTests = user.splitTests || [] - const assignmentLog = assignedSplitTests[splitTestName] || [] - const existingAssignment = _.find(assignmentLog, { versionNumber }) - if (!existingAssignment) { - await UserUpdater.promises.updateUser(userId, { - $addToSet: { - [`splitTests.${splitTestName}`]: persistedAssignment, - }, - }) - AnalyticsManager.setUserPropertyForAnalyticsId( - analyticsId, - `split-test-${splitTestName}-${versionNumber}`, - variantName - ) - } - } - } else if (session) { - if (!session.splitTests) { - session.splitTests = {} - } - if (!session.splitTests[splitTestName]) { - session.splitTests[splitTestName] = [] - } - const existingAssignment = _.find(session.splitTests[splitTestName], { - versionNumber, - }) + const user = await _getUser(userId) + if (user) { + const assignedSplitTests = user.splitTests || [] + const assignmentLog = assignedSplitTests[splitTestName] || [] + const existingAssignment = _.find(assignmentLog, { versionNumber }) if (!existingAssignment) { - session.splitTests[splitTestName].push(persistedAssignment) - AnalyticsManager.setUserPropertyForAnalyticsId( - analyticsId, + await UserUpdater.promises.updateUser(userId, { + $addToSet: { + [`splitTests.${splitTestName}`]: { + variantName, + versionNumber, + phase, + assignedAt: new Date(), + }, + }, + }) + AnalyticsManager.setUserProperty( + userId, `split-test-${splitTestName}-${versionNumber}`, variantName ) @@ -270,13 +179,9 @@ async function _getUser(id) { module.exports = { getAssignment: callbackify(getAssignment), - getAssignmentForSession: callbackify(getAssignmentForSession), assignInLocalsContext: callbackify(assignInLocalsContext), - assignInLocalsContextForSession: callbackify(assignInLocalsContextForSession), promises: { getAssignment, - getAssignmentForSession, assignInLocalsContext, - assignInLocalsContextForSession, }, } diff --git a/services/web/app/src/Features/Subscription/FeaturesUpdater.js b/services/web/app/src/Features/Subscription/FeaturesUpdater.js index b9af77dbac..563ec45488 100644 --- a/services/web/app/src/Features/Subscription/FeaturesUpdater.js +++ b/services/web/app/src/Features/Subscription/FeaturesUpdater.js @@ -28,7 +28,7 @@ const FeaturesUpdater = { const matchedFeatureSet = FeaturesUpdater._getMatchedFeatureSet( features ) - AnalyticsManager.setUserPropertyForUser( + AnalyticsManager.setUserProperty( userId, 'feature-set', matchedFeatureSet diff --git a/services/web/app/src/Features/Subscription/RecurlyEventHandler.js b/services/web/app/src/Features/Subscription/RecurlyEventHandler.js index ea977a8a35..f21815b051 100644 --- a/services/web/app/src/Features/Subscription/RecurlyEventHandler.js +++ b/services/web/app/src/Features/Subscription/RecurlyEventHandler.js @@ -39,133 +39,85 @@ function sendRecurlyAnalyticsEvent(event, eventData) { } } -async function _sendSubscriptionStartedEvent(eventData) { +function _sendSubscriptionStartedEvent(eventData) { const userId = _getUserId(eventData) const { planCode, quantity, state, isTrial } = _getSubscriptionData(eventData) - AnalyticsManager.recordEventForUser(userId, 'subscription-started', { + AnalyticsManager.recordEvent(userId, 'subscription-started', { plan_code: planCode, quantity, is_trial: isTrial, }) - AnalyticsManager.setUserPropertyForUser( - userId, - 'subscription-plan-code', - planCode - ) - AnalyticsManager.setUserPropertyForUser(userId, 'subscription-state', state) - AnalyticsManager.setUserPropertyForUser( - userId, - 'subscription-is-trial', - isTrial - ) + AnalyticsManager.setUserProperty(userId, 'subscription-plan-code', planCode) + AnalyticsManager.setUserProperty(userId, 'subscription-state', state) + AnalyticsManager.setUserProperty(userId, 'subscription-is-trial', isTrial) } -async function _sendSubscriptionUpdatedEvent(eventData) { +function _sendSubscriptionUpdatedEvent(eventData) { const userId = _getUserId(eventData) const { planCode, quantity, state, isTrial } = _getSubscriptionData(eventData) - AnalyticsManager.recordEventForUser(userId, 'subscription-updated', { + AnalyticsManager.recordEvent(userId, 'subscription-updated', { plan_code: planCode, quantity, }) - AnalyticsManager.setUserPropertyForUser( - userId, - 'subscription-plan-code', - planCode - ) - AnalyticsManager.setUserPropertyForUser(userId, 'subscription-state', state) - AnalyticsManager.setUserPropertyForUser( - userId, - 'subscription-is-trial', - isTrial - ) + AnalyticsManager.setUserProperty(userId, 'subscription-plan-code', planCode) + AnalyticsManager.setUserProperty(userId, 'subscription-state', state) + AnalyticsManager.setUserProperty(userId, 'subscription-is-trial', isTrial) } -async function _sendSubscriptionCancelledEvent(eventData) { +function _sendSubscriptionCancelledEvent(eventData) { const userId = _getUserId(eventData) const { planCode, quantity, state, isTrial } = _getSubscriptionData(eventData) - AnalyticsManager.recordEventForUser(userId, 'subscription-cancelled', { + AnalyticsManager.recordEvent(userId, 'subscription-cancelled', { plan_code: planCode, quantity, is_trial: isTrial, }) - AnalyticsManager.setUserPropertyForUser(userId, 'subscription-state', state) - AnalyticsManager.setUserPropertyForUser( - userId, - 'subscription-is-trial', - isTrial - ) + AnalyticsManager.setUserProperty(userId, 'subscription-state', state) + AnalyticsManager.setUserProperty(userId, 'subscription-is-trial', isTrial) } -async function _sendSubscriptionExpiredEvent(eventData) { +function _sendSubscriptionExpiredEvent(eventData) { const userId = _getUserId(eventData) const { planCode, quantity, state, isTrial } = _getSubscriptionData(eventData) - AnalyticsManager.recordEventForUser(userId, 'subscription-expired', { + AnalyticsManager.recordEvent(userId, 'subscription-expired', { plan_code: planCode, quantity, is_trial: isTrial, }) - AnalyticsManager.setUserPropertyForUser( - userId, - 'subscription-plan-code', - planCode - ) - AnalyticsManager.setUserPropertyForUser(userId, 'subscription-state', state) - AnalyticsManager.setUserPropertyForUser( - userId, - 'subscription-is-trial', - isTrial - ) + AnalyticsManager.setUserProperty(userId, 'subscription-plan-code', planCode) + AnalyticsManager.setUserProperty(userId, 'subscription-state', state) + AnalyticsManager.setUserProperty(userId, 'subscription-is-trial', isTrial) } -async function _sendSubscriptionRenewedEvent(eventData) { +function _sendSubscriptionRenewedEvent(eventData) { const userId = _getUserId(eventData) const { planCode, quantity, state, isTrial } = _getSubscriptionData(eventData) - AnalyticsManager.recordEventForUser(userId, 'subscription-renewed', { + AnalyticsManager.recordEvent(userId, 'subscription-renewed', { plan_code: planCode, quantity, is_trial: isTrial, }) - AnalyticsManager.setUserPropertyForUser( - userId, - 'subscription-plan-code', - planCode - ) - AnalyticsManager.setUserPropertyForUser(userId, 'subscription-state', state) - AnalyticsManager.setUserPropertyForUser( - userId, - 'subscription-is-trial', - isTrial - ) + AnalyticsManager.setUserProperty(userId, 'subscription-plan-code', planCode) + AnalyticsManager.setUserProperty(userId, 'subscription-state', state) + AnalyticsManager.setUserProperty(userId, 'subscription-is-trial', isTrial) } -async function _sendSubscriptionReactivatedEvent(eventData) { +function _sendSubscriptionReactivatedEvent(eventData) { const userId = _getUserId(eventData) const { planCode, quantity, state, isTrial } = _getSubscriptionData(eventData) - AnalyticsManager.recordEventForUser(userId, 'subscription-reactivated', { + AnalyticsManager.recordEvent(userId, 'subscription-reactivated', { plan_code: planCode, quantity, }) - AnalyticsManager.setUserPropertyForUser( - userId, - 'subscription-plan-code', - planCode - ) - AnalyticsManager.setUserPropertyForUser(userId, 'subscription-state', state) - AnalyticsManager.setUserPropertyForUser( - userId, - 'subscription-is-trial', - isTrial - ) + AnalyticsManager.setUserProperty(userId, 'subscription-plan-code', planCode) + AnalyticsManager.setUserProperty(userId, 'subscription-state', state) + AnalyticsManager.setUserProperty(userId, 'subscription-is-trial', isTrial) } -async function _sendInvoicePaidEvent(eventData) { +function _sendInvoicePaidEvent(eventData) { const userId = _getUserId(eventData) - AnalyticsManager.recordEventForUser(userId, 'subscription-invoice-collected') - AnalyticsManager.setUserPropertyForUser( - userId, - 'subscription-is-trial', - false - ) + AnalyticsManager.recordEvent(userId, 'subscription-invoice-collected') + AnalyticsManager.setUserProperty(userId, 'subscription-is-trial', false) } function _getUserId(eventData) { diff --git a/services/web/app/src/Features/Subscription/SubscriptionController.js b/services/web/app/src/Features/Subscription/SubscriptionController.js index e1dabb55bc..3aa6f919e6 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionController.js +++ b/services/web/app/src/Features/Subscription/SubscriptionController.js @@ -116,7 +116,7 @@ async function userSubscriptionPage(req, res) { personalSubscription ? personalSubscription.plan : undefined ) - AnalyticsManager.recordEventForSession(req.session, 'subscription-page-view') + AnalyticsManager.recordEvent(user._id, 'subscription-page-view') const data = { title: 'your_subscription', diff --git a/services/web/app/src/Features/Subscription/SubscriptionUpdater.js b/services/web/app/src/Features/Subscription/SubscriptionUpdater.js index b15e496c2c..050dd9a0d6 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionUpdater.js +++ b/services/web/app/src/Features/Subscription/SubscriptionUpdater.js @@ -360,7 +360,7 @@ async function _sendUserGroupPlanCodeUserProperty(userId) { bestFeatures = plan.features } } - AnalyticsManager.setUserPropertyForUser( + AnalyticsManager.setUserProperty( userId, 'group-subscription-plan-code', bestPlanCode diff --git a/services/web/app/src/Features/TokenAccess/TokenAccessHandler.js b/services/web/app/src/Features/TokenAccess/TokenAccessHandler.js index 8f9a63ab42..d97cb81c04 100644 --- a/services/web/app/src/Features/TokenAccess/TokenAccessHandler.js +++ b/services/web/app/src/Features/TokenAccess/TokenAccessHandler.js @@ -164,9 +164,7 @@ const TokenAccessHandler = { addReadOnlyUserToProject(userId, projectId, callback) { userId = ObjectId(userId.toString()) projectId = ObjectId(projectId.toString()) - Analytics.recordEventForUser(userId, 'project-joined', { - mode: 'read-only', - }) + Analytics.recordEvent(userId, 'project-joined', { mode: 'read-only' }) Project.updateOne( { _id: projectId, @@ -181,9 +179,7 @@ const TokenAccessHandler = { addReadAndWriteUserToProject(userId, projectId, callback) { userId = ObjectId(userId.toString()) projectId = ObjectId(projectId.toString()) - Analytics.recordEventForUser(userId, 'project-joined', { - mode: 'read-write', - }) + Analytics.recordEvent(userId, 'project-joined', { mode: 'read-write' }) Project.updateOne( { _id: projectId, diff --git a/services/web/app/src/Features/User/UserCreator.js b/services/web/app/src/Features/User/UserCreator.js index 828e0e7f83..5bb5ba5574 100644 --- a/services/web/app/src/Features/User/UserCreator.js +++ b/services/web/app/src/Features/User/UserCreator.js @@ -84,16 +84,8 @@ async function createNewUser(attributes, options = {}) { } } - await Analytics.recordEventForUser(user._id, 'user-registered') - await Analytics.setUserPropertyForUser(user._id, 'created-at', new Date()) - await Analytics.setUserPropertyForUser(user._id, 'user-id', user._id) - if (attributes.analyticsId) { - await Analytics.setUserPropertyForUser( - user._id, - 'analytics-id', - attributes.analyticsId - ) - } + Analytics.recordEvent(user._id, 'user-registered') + Analytics.setUserProperty(user._id, 'created-at', new Date()) if (Features.hasFeature('saas')) { try { diff --git a/services/web/app/src/Features/User/UserPostRegistrationAnalyticsManager.js b/services/web/app/src/Features/User/UserPostRegistrationAnalyticsManager.js index 389cac3198..c8dc5b9aed 100644 --- a/services/web/app/src/Features/User/UserPostRegistrationAnalyticsManager.js +++ b/services/web/app/src/Features/User/UserPostRegistrationAnalyticsManager.js @@ -32,7 +32,7 @@ async function checkAffiliations(userId) { ) if (hasCommonsAccountAffiliation) { - await AnalyticsManager.setUserPropertyForUser( + await AnalyticsManager.setUserProperty( userId, 'registered-from-commons-account', true diff --git a/services/web/app/src/Features/User/UserRegistrationHandler.js b/services/web/app/src/Features/User/UserRegistrationHandler.js index a45b6dbcc8..d935e4cd7b 100644 --- a/services/web/app/src/Features/User/UserRegistrationHandler.js +++ b/services/web/app/src/Features/User/UserRegistrationHandler.js @@ -8,6 +8,7 @@ const logger = require('logger-sharelatex') const crypto = require('crypto') const EmailHandler = require('../Email/EmailHandler') const OneTimeTokenHandler = require('../Security/OneTimeTokenHandler') +const Analytics = require('../Analytics/AnalyticsManager') const settings = require('@overleaf/settings') const EmailHelper = require('../Helpers/EmailHelper') @@ -30,7 +31,6 @@ const UserRegistrationHandler = { email: userDetails.email, first_name: userDetails.first_name, last_name: userDetails.last_name, - analyticsId: userDetails.analyticsId, }, {}, callback @@ -87,6 +87,7 @@ const UserRegistrationHandler = { }, // this can be slow, just fire it off ], error => { + Analytics.recordEvent(user._id, 'user-registered') callback(error, user) } ) diff --git a/services/web/app/src/infrastructure/Server.js b/services/web/app/src/infrastructure/Server.js index 199c52a071..7e30f77237 100644 --- a/services/web/app/src/infrastructure/Server.js +++ b/services/web/app/src/infrastructure/Server.js @@ -15,7 +15,6 @@ const sessionsRedisClient = UserSessionsRedis.client() const SessionAutostartMiddleware = require('./SessionAutostartMiddleware') const SessionStoreManager = require('./SessionStoreManager') -const AnalyticsManager = require('../Features/Analytics/AnalyticsManager') const session = require('express-session') const RedisStore = require('connect-redis')(session) const bodyParser = require('./BodyParserWrapper') @@ -33,7 +32,6 @@ const ProxyManager = require('./ProxyManager') const translations = require('./Translations') const Modules = require('./Modules') const Views = require('./Views') -const Features = require('./Features') const ErrorController = require('../Features/Errors/ErrorController') const HttpErrorHandler = require('../Features/Errors/HttpErrorHandler') @@ -127,9 +125,6 @@ webRouter.use( rolling: true, }) ) -if (Features.hasFeature('saas')) { - webRouter.use(AnalyticsManager.analyticsIdMiddleware) -} // patch the session store to generate a validation token for every new session SessionStoreManager.enableValidationToken(sessionStore) diff --git a/services/web/app/src/models/User.js b/services/web/app/src/models/User.js index 5a4e396792..4ba8a810f2 100644 --- a/services/web/app/src/models/User.js +++ b/services/web/app/src/models/User.js @@ -166,7 +166,6 @@ const UserSchema = new Schema({ onboardingEmailSentAt: { type: Date }, auditLog: [AuditLogEntrySchema], splitTests: Schema.Types.Mixed, - analyticsId: { type: String }, }) exports.User = mongoose.model('User', UserSchema) diff --git a/services/web/modules/server-ce-scripts/scripts/create-user.js b/services/web/modules/server-ce-scripts/scripts/create-user.js index 031c78fff2..994ad874ec 100644 --- a/services/web/modules/server-ce-scripts/scripts/create-user.js +++ b/services/web/modules/server-ce-scripts/scripts/create-user.js @@ -18,7 +18,7 @@ async function main() { await new Promise((resolve, reject) => { UserRegistrationHandler.registerNewUserAndSendActivationEmail( - email, + email, (error, user, setNewPasswordUrl) => { if (error) { return reject(error) diff --git a/services/web/test/unit/src/Analytics/AnalyticsControllerTests.js b/services/web/test/unit/src/Analytics/AnalyticsControllerTests.js index 9a29cffbe7..686400614f 100644 --- a/services/web/test/unit/src/Analytics/AnalyticsControllerTests.js +++ b/services/web/test/unit/src/Analytics/AnalyticsControllerTests.js @@ -12,7 +12,7 @@ describe('AnalyticsController', function () { this.AnalyticsManager = { updateEditingSession: sinon.stub(), - recordEventForSession: sinon.stub(), + recordEvent: sinon.stub(), } this.Features = { @@ -42,7 +42,6 @@ describe('AnalyticsController', function () { params: { projectId: 'a project id', }, - session: {}, } this.GeoIpLookup.getDetails = sinon .stub() @@ -79,18 +78,35 @@ describe('AnalyticsController', function () { delete this.expectedData._csrf }) - it('should use the session', function (done) { + it('should use the user_id', function (done) { + this.SessionManager.getLoggedInUserId.returns('1234') this.controller.recordEvent(this.req, this.res) - this.AnalyticsManager.recordEventForSession - .calledWith(this.req.session, this.req.params.event, this.expectedData) + this.AnalyticsManager.recordEvent + .calledWith('1234', this.req.params.event, this.expectedData) + .should.equal(true) + done() + }) + + it('should use the session id', function (done) { + this.controller.recordEvent(this.req, this.res) + this.AnalyticsManager.recordEvent + .calledWith( + this.req.sessionID, + this.req.params.event, + this.expectedData + ) .should.equal(true) done() }) it('should remove the CSRF token before sending to the manager', function (done) { this.controller.recordEvent(this.req, this.res) - this.AnalyticsManager.recordEventForSession - .calledWith(this.req.session, this.req.params.event, this.expectedData) + this.AnalyticsManager.recordEvent + .calledWith( + this.req.sessionID, + this.req.params.event, + this.expectedData + ) .should.equal(true) done() }) diff --git a/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js b/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js index d041c6e617..ed08641a7b 100644 --- a/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js +++ b/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js @@ -10,7 +10,6 @@ const MODULE_PATH = path.join( describe('AnalyticsManager', function () { beforeEach(function () { this.fakeUserId = '123abc' - this.analyticsId = '123456' this.Settings = { analytics: { enabled: true }, } @@ -51,9 +50,6 @@ describe('AnalyticsManager', function () { requires: { '@overleaf/settings': this.Settings, '../../infrastructure/Queues': this.Queues, - './UserAnalyticsIdCache': (this.UserAnalyticsIdCache = { - get: sinon.stub().resolves(this.analyticsId), - }), }, }) }) @@ -74,26 +70,21 @@ describe('AnalyticsManager', function () { describe('queues the appropriate message for', function () { it('identifyUser', function () { - const analyticsId = '456def' - this.AnalyticsManager.identifyUser(this.fakeUserId, analyticsId) + const oldUserId = '456def' + this.AnalyticsManager.identifyUser(this.fakeUserId, oldUserId) sinon.assert.calledWithMatch(this.analyticsEventsQueue.add, 'identify', { userId: this.fakeUserId, - analyticsId, + oldUserId, }) }) - it('recordEventForUser', async function () { + it('recordEvent', function () { const event = 'fake-event' - await this.AnalyticsManager.recordEventForUser( - this.fakeUserId, - event, - null - ) + this.AnalyticsManager.recordEvent(this.fakeUserId, event, null) sinon.assert.calledWithMatch(this.analyticsEventsQueue.add, 'event', { - analyticsId: this.analyticsId, event, + userId: this.fakeUserId, segmentation: null, - isLoggedIn: true, }) }) diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index 8ec78545a1..834d0ba725 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -28,7 +28,6 @@ describe('AuthenticationController', function () { this.res = new MockResponse() this.callback = sinon.stub() this.next = sinon.stub() - this.req.session.analyticsId = 'abc-123' this.AuthenticationController = SandboxedModule.require(modulePath, { requires: { @@ -54,9 +53,8 @@ describe('AuthenticationController', function () { setupLoginData: sinon.stub(), }), '../Analytics/AnalyticsManager': (this.AnalyticsManager = { - recordEventForUser: sinon.stub(), + recordEvent: sinon.stub(), identifyUser: sinon.stub(), - getIdsFromSession: sinon.stub().returns({ userId: this.user._id }), }), '../../infrastructure/SessionStoreManager': (this.SessionStoreManager = {}), '@overleaf/settings': (this.Settings = { @@ -1238,11 +1236,9 @@ describe('AuthenticationController', function () { }) it('should call identifyUser', function () { - sinon.assert.calledWith( - this.AnalyticsManager.identifyUser, - this.user._id, - this.req.session.analyticsId - ) + this.AnalyticsManager.identifyUser + .calledWith(this.user._id, this.req.sessionID) + .should.equal(true) }) it('should setup the user data in the background', function () { @@ -1275,11 +1271,9 @@ describe('AuthenticationController', function () { }) it('should track the login event', function () { - sinon.assert.calledWith( - this.AnalyticsManager.recordEventForUser, - this.user._id, - 'user-logged-in' - ) + this.AnalyticsManager.recordEvent + .calledWith(this.user._id, 'user-logged-in') + .should.equal(true) }) }) }) diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsInviteControllerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsInviteControllerTests.js index 7f04d0ad03..c70a7d48fa 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsInviteControllerTests.js +++ b/services/web/test/unit/src/Collaborators/CollaboratorsInviteControllerTests.js @@ -24,7 +24,7 @@ const { ObjectId } = require('mongodb') describe('CollaboratorsInviteController', function () { beforeEach(function () { this.user = { _id: 'id' } - this.AnalyticsManger = { recordEventForUser: sinon.stub() } + this.AnalyticsManger = { recordEvent: sinon.stub() } this.sendingUser = null this.AuthenticationController = { getSessionUser: req => { diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index fa0fdc158b..f111801131 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -173,7 +173,7 @@ describe('ProjectController', function () { .BrandVariationsHandler, '../ThirdPartyDataStore/TpdsProjectFlusher': this.TpdsProjectFlusher, '../../models/Project': {}, - '../Analytics/AnalyticsManager': { recordEventForUser: () => {} }, + '../Analytics/AnalyticsManager': { recordEvent: () => {} }, '../../infrastructure/Modules': { hooks: { fire: sinon.stub().yields(null, []) }, }, diff --git a/services/web/test/unit/src/Subscription/FeaturesUpdaterTests.js b/services/web/test/unit/src/Subscription/FeaturesUpdaterTests.js index 40b27b5773..c4facd1b1d 100644 --- a/services/web/test/unit/src/Subscription/FeaturesUpdaterTests.js +++ b/services/web/test/unit/src/Subscription/FeaturesUpdaterTests.js @@ -40,7 +40,7 @@ describe('FeaturesUpdater', function () { '../Institutions/InstitutionsFeatures': (this.InstitutionsFeatures = {}), '../User/UserGetter': (this.UserGetter = {}), '../Analytics/AnalyticsManager': (this.AnalyticsManager = { - setUserPropertyForUser: sinon.stub(), + setUserProperty: sinon.stub(), }), '../../infrastructure/Modules': (this.Modules = { hooks: { fire: sinon.stub() }, @@ -182,7 +182,7 @@ describe('FeaturesUpdater', function () { ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.user_id, 'feature-set', 'personal' @@ -201,7 +201,7 @@ describe('FeaturesUpdater', function () { ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.user_id, 'feature-set', 'mixed' diff --git a/services/web/test/unit/src/Subscription/RecurlyEventHandlerTests.js b/services/web/test/unit/src/Subscription/RecurlyEventHandlerTests.js index 1c1cb18564..59492c0018 100644 --- a/services/web/test/unit/src/Subscription/RecurlyEventHandlerTests.js +++ b/services/web/test/unit/src/Subscription/RecurlyEventHandlerTests.js @@ -27,8 +27,8 @@ describe('RecurlyEventHandler', function () { this.RecurlyEventHandler = SandboxedModule.require(modulePath, { requires: { '../Analytics/AnalyticsManager': (this.AnalyticsManager = { - recordEventForUser: sinon.stub(), - setUserPropertyForUser: sinon.stub(), + recordEvent: sinon.stub(), + setUserProperty: sinon.stub(), }), }, }) @@ -40,7 +40,7 @@ describe('RecurlyEventHandler', function () { this.eventData ) sinon.assert.calledWith( - this.AnalyticsManager.recordEventForUser, + this.AnalyticsManager.recordEvent, this.userId, 'subscription-started', { @@ -50,19 +50,19 @@ describe('RecurlyEventHandler', function () { } ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.userId, 'subscription-plan-code', this.planCode ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.userId, 'subscription-state', 'active' ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.userId, 'subscription-is-trial', true @@ -83,7 +83,7 @@ describe('RecurlyEventHandler', function () { this.eventData ) sinon.assert.calledWith( - this.AnalyticsManager.recordEventForUser, + this.AnalyticsManager.recordEvent, this.userId, 'subscription-started', { @@ -93,13 +93,13 @@ describe('RecurlyEventHandler', function () { } ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.userId, 'subscription-state', 'active' ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.userId, 'subscription-is-trial', false @@ -114,7 +114,7 @@ describe('RecurlyEventHandler', function () { this.eventData ) sinon.assert.calledWith( - this.AnalyticsManager.recordEventForUser, + this.AnalyticsManager.recordEvent, this.userId, 'subscription-updated', { @@ -123,19 +123,19 @@ describe('RecurlyEventHandler', function () { } ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.userId, 'subscription-plan-code', this.planCode ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.userId, 'subscription-state', 'active' ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.userId, 'subscription-is-trial', true @@ -149,7 +149,7 @@ describe('RecurlyEventHandler', function () { this.eventData ) sinon.assert.calledWith( - this.AnalyticsManager.recordEventForUser, + this.AnalyticsManager.recordEvent, this.userId, 'subscription-cancelled', { @@ -159,13 +159,13 @@ describe('RecurlyEventHandler', function () { } ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.userId, 'subscription-state', 'cancelled' ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.userId, 'subscription-is-trial', true @@ -179,7 +179,7 @@ describe('RecurlyEventHandler', function () { this.eventData ) sinon.assert.calledWith( - this.AnalyticsManager.recordEventForUser, + this.AnalyticsManager.recordEvent, this.userId, 'subscription-expired', { @@ -189,19 +189,19 @@ describe('RecurlyEventHandler', function () { } ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.userId, 'subscription-plan-code', this.planCode ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.userId, 'subscription-state', 'expired' ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.userId, 'subscription-is-trial', true @@ -214,7 +214,7 @@ describe('RecurlyEventHandler', function () { this.eventData ) sinon.assert.calledWith( - this.AnalyticsManager.recordEventForUser, + this.AnalyticsManager.recordEvent, this.userId, 'subscription-renewed', { @@ -231,7 +231,7 @@ describe('RecurlyEventHandler', function () { this.eventData ) sinon.assert.calledWith( - this.AnalyticsManager.recordEventForUser, + this.AnalyticsManager.recordEvent, this.userId, 'subscription-reactivated', { @@ -255,7 +255,7 @@ describe('RecurlyEventHandler', function () { } ) sinon.assert.calledWith( - this.AnalyticsManager.recordEventForUser, + this.AnalyticsManager.recordEvent, this.userId, 'subscription-invoice-collected' ) @@ -274,7 +274,7 @@ describe('RecurlyEventHandler', function () { }, } ) - sinon.assert.notCalled(this.AnalyticsManager.recordEventForUser) + sinon.assert.notCalled(this.AnalyticsManager.recordEvent) }) it('with closed_invoice_notification', function () { @@ -291,7 +291,7 @@ describe('RecurlyEventHandler', function () { } ) sinon.assert.calledWith( - this.AnalyticsManager.recordEventForUser, + this.AnalyticsManager.recordEvent, this.userId, 'subscription-invoice-collected' ) @@ -310,6 +310,6 @@ describe('RecurlyEventHandler', function () { }, } ) - sinon.assert.notCalled(this.AnalyticsManager.recordEventForUser) + sinon.assert.notCalled(this.AnalyticsManager.recordEvent) }) }) diff --git a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js b/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js index 27d80343d3..1f3e03388a 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js @@ -141,9 +141,8 @@ describe('SubscriptionController', function () { }), './Errors': SubscriptionErrors, '../Analytics/AnalyticsManager': (this.AnalyticsManager = { - recordEventForUser: sinon.stub(), - recordEventForSession: sinon.stub(), - setUserPropertyForUser: sinon.stub(), + recordEvent: sinon.stub(), + setUserProperty: sinon.stub(), }), '../SplitTests/SplitTestHandler': (this.SplitTestHandler = { getTestSegmentation: () => {}, diff --git a/services/web/test/unit/src/Subscription/SubscriptionHandlerTests.js b/services/web/test/unit/src/Subscription/SubscriptionHandlerTests.js index a9671559f0..2da7c0cfd7 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionHandlerTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionHandlerTests.js @@ -108,7 +108,7 @@ describe('SubscriptionHandler', function () { this.EmailHandler = { sendEmail: sinon.stub() } - this.AnalyticsManager = { recordEventForUser: sinon.stub() } + this.AnalyticsManager = { recordEvent: sinon.stub() } this.PlansLocator = { findLocalPlanInSettings: sinon.stub().returns({ planCode: 'plan' }), diff --git a/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js b/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js index b4c41c0671..5c6535b712 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js @@ -105,7 +105,7 @@ describe('SubscriptionUpdater', function () { findOneAndUpdate: sinon.stub().yields(), } this.AnalyticsManager = { - setUserPropertyForUser: sinon.stub(), + setUserProperty: sinon.stub(), } this.SubscriptionUpdater = SandboxedModule.require(modulePath, { requires: { @@ -527,7 +527,7 @@ describe('SubscriptionUpdater', function () { this.otherUserId, () => { sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.otherUserId, 'group-subscription-plan-code', 'group_subscription' @@ -547,7 +547,7 @@ describe('SubscriptionUpdater', function () { this.otherUserId ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.otherUserId, 'group-subscription-plan-code', 'group_subscription' @@ -564,7 +564,7 @@ describe('SubscriptionUpdater', function () { this.otherUserId ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.otherUserId, 'group-subscription-plan-code', 'better_group_subscription' @@ -581,7 +581,7 @@ describe('SubscriptionUpdater', function () { this.otherUserId ) sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.otherUserId, 'group-subscription-plan-code', 'better_group_subscription' @@ -622,7 +622,7 @@ describe('SubscriptionUpdater', function () { this.otherUserId, () => { sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.otherUserId, 'group-subscription-plan-code', null @@ -635,7 +635,7 @@ describe('SubscriptionUpdater', function () { it('should set the group plan code user property when removing user from all groups', function (done) { this.SubscriptionUpdater.removeUserFromAllGroups(this.otherUserId, () => { sinon.assert.calledWith( - this.AnalyticsManager.setUserPropertyForUser, + this.AnalyticsManager.setUserProperty, this.otherUserId, 'group-subscription-plan-code', null diff --git a/services/web/test/unit/src/TokenAccess/TokenAccessHandlerTests.js b/services/web/test/unit/src/TokenAccess/TokenAccessHandlerTests.js index 15b88bc127..6565d69bbb 100644 --- a/services/web/test/unit/src/TokenAccess/TokenAccessHandlerTests.js +++ b/services/web/test/unit/src/TokenAccess/TokenAccessHandlerTests.js @@ -12,6 +12,7 @@ * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ const SandboxedModule = require('sandboxed-module') +const assert = require('assert') const path = require('path') const sinon = require('sinon') const modulePath = path.join( @@ -42,7 +43,7 @@ describe('TokenAccessHandler', function () { }), crypto: (this.Crypto = require('crypto')), '../Analytics/AnalyticsManager': (this.Analytics = { - recordEventForUser: sinon.stub(), + recordEvent: sinon.stub(), }), }, })) @@ -137,7 +138,7 @@ describe('TokenAccessHandler', function () { this.Project.updateOne.lastCall.args[1].$addToSet ).to.have.keys('tokenAccessReadOnly_refs') sinon.assert.calledWith( - this.Analytics.recordEventForUser, + this.Analytics.recordEvent, this.userId, 'project-joined', { mode: 'read-only' } @@ -198,7 +199,7 @@ describe('TokenAccessHandler', function () { this.Project.updateOne.lastCall.args[1].$addToSet ).to.have.keys('tokenAccessReadAndWrite_refs') sinon.assert.calledWith( - this.Analytics.recordEventForUser, + this.Analytics.recordEvent, this.userId, 'project-joined', { mode: 'read-write' } diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index 76eb385cff..a3f7d6b51c 100644 --- a/services/web/test/unit/src/User/UserControllerTests.js +++ b/services/web/test/unit/src/User/UserControllerTests.js @@ -24,7 +24,6 @@ describe('UserController', function () { _id: this.user_id, email: 'old@something.com', }, - analyticsId: this.user_id, }, sessionID: '123', body: {}, @@ -545,10 +544,9 @@ describe('UserController', function () { }) it('should register the user and send them an email', function () { - sinon.assert.calledWith( - this.UserRegistrationHandler.registerNewUserAndSendActivationEmail, - this.email - ) + this.UserRegistrationHandler.registerNewUserAndSendActivationEmail + .calledWith(this.email) + .should.equal(true) }) it('should return the user and activation url', function () { diff --git a/services/web/test/unit/src/User/UserCreatorTests.js b/services/web/test/unit/src/User/UserCreatorTests.js index b1c0b8c874..82cbffd64c 100644 --- a/services/web/test/unit/src/User/UserCreatorTests.js +++ b/services/web/test/unit/src/User/UserCreatorTests.js @@ -42,8 +42,8 @@ describe('UserCreator', function () { }, }), '../Analytics/AnalyticsManager': (this.Analytics = { - recordEventForUser: sinon.stub(), - setUserPropertyForUser: sinon.stub(), + recordEvent: sinon.stub(), + setUserProperty: sinon.stub(), }), './UserOnboardingEmailManager': (this.UserOnboardingEmailManager = { scheduleOnboardingEmail: sinon.stub(), @@ -271,12 +271,12 @@ describe('UserCreator', function () { }) assert.equal(user.email, this.email) sinon.assert.calledWith( - this.Analytics.recordEventForUser, + this.Analytics.recordEvent, user._id, 'user-registered' ) sinon.assert.calledWith( - this.Analytics.setUserPropertyForUser, + this.Analytics.setUserProperty, user._id, 'created-at' ) diff --git a/services/web/test/unit/src/User/UserPostRegistrationAnalyticsManagerTests.js b/services/web/test/unit/src/User/UserPostRegistrationAnalyticsManagerTests.js index d14c9abf32..1f95a75825 100644 --- a/services/web/test/unit/src/User/UserPostRegistrationAnalyticsManagerTests.js +++ b/services/web/test/unit/src/User/UserPostRegistrationAnalyticsManagerTests.js @@ -36,7 +36,7 @@ describe('UserPostRegistrationAnalyticsManager', function () { }, } this.AnalyticsManager = { - setUserPropertyForUser: sinon.stub().resolves(), + setUserProperty: sinon.stub().resolves(), } this.UserPostRegistrationAnalyticsManager = SandboxedModule.require( MODULE_PATH, @@ -72,8 +72,7 @@ describe('UserPostRegistrationAnalyticsManager', function () { ) expect(this.InstitutionsAPI.promises.getUserAffiliations).not.to.have.been .called - expect(this.AnalyticsManager.setUserPropertyForUser).not.to.have.been - .called + expect(this.AnalyticsManager.setUserProperty).not.to.have.been.called }) it('sets user property if user has commons account affiliationd', async function () { @@ -93,9 +92,7 @@ describe('UserPostRegistrationAnalyticsManager', function () { await this.UserPostRegistrationAnalyticsManager.postRegistrationAnalytics( this.fakeUserId ) - expect( - this.AnalyticsManager.setUserPropertyForUser - ).to.have.been.calledWith( + expect(this.AnalyticsManager.setUserProperty).to.have.been.calledWith( this.fakeUserId, 'registered-from-commons-account', true @@ -113,8 +110,7 @@ describe('UserPostRegistrationAnalyticsManager', function () { await this.UserPostRegistrationAnalyticsManager.postRegistrationAnalytics( this.fakeUserId ) - expect(this.AnalyticsManager.setUserPropertyForUser).not.to.have.been - .called + expect(this.AnalyticsManager.setUserProperty).not.to.have.been.called }) }) }) diff --git a/services/web/test/unit/src/User/UserRegistrationHandlerTests.js b/services/web/test/unit/src/User/UserRegistrationHandlerTests.js index adcd01f14c..15748e7b17 100644 --- a/services/web/test/unit/src/User/UserRegistrationHandlerTests.js +++ b/services/web/test/unit/src/User/UserRegistrationHandlerTests.js @@ -24,11 +24,7 @@ const EmailHelper = require('../../../../app/src/Features/Helpers/EmailHelper') describe('UserRegistrationHandler', function () { beforeEach(function () { - this.analyticsId = '123456' - this.user = { - _id: (this.user_id = '31j2lk21kjl'), - analyticsId: this.analyticsId, - } + this.user = { _id: (this.user_id = '31j2lk21kjl') } this.User = { updateOne: sinon.stub().callsArgWith(2) } this.UserGetter = { getUserByAnyEmail: sinon.stub() } this.UserCreator = { @@ -53,9 +49,7 @@ describe('UserRegistrationHandler', function () { '../Email/EmailHandler': this.EmailHandler, '../Security/OneTimeTokenHandler': this.OneTimeTokenHandler, '../Analytics/AnalyticsManager': (this.AnalyticsManager = { - recordEventForUser: sinon.stub(), - setUserPropertyForUser: sinon.stub(), - identifyUser: sinon.stub(), + recordEvent: sinon.stub(), }), '@overleaf/settings': (this.settings = { siteUrl: 'http://sl.example.com', @@ -64,11 +58,10 @@ describe('UserRegistrationHandler', function () { }, }) - this.passingRequest = { + return (this.passingRequest = { email: 'something@email.com', password: '123', - analyticsId: this.analyticsId, - } + }) }) describe('validate Register Request', function () { @@ -174,15 +167,16 @@ describe('UserRegistrationHandler', function () { }) it('should create a new user', function (done) { - this.handler.registerNewUser(this.passingRequest, err => { - sinon.assert.calledWith(this.UserCreator.createNewUser, { - email: this.passingRequest.email, - holdingAccount: false, - first_name: this.passingRequest.first_name, - last_name: this.passingRequest.last_name, - analyticsId: this.user.analyticsId, - }) - done() + return this.handler.registerNewUser(this.passingRequest, err => { + this.UserCreator.createNewUser + .calledWith({ + email: this.passingRequest.email, + holdingAccount: false, + first_name: this.passingRequest.first_name, + last_name: this.passingRequest.last_name, + }) + .should.equal(true) + return done() }) }) @@ -233,6 +227,15 @@ describe('UserRegistrationHandler', function () { return done() }) }) + + it('should track the registration event', function (done) { + return this.handler.registerNewUser(this.passingRequest, err => { + this.AnalyticsManager.recordEvent + .calledWith(this.user._id, 'user-registered') + .should.equal(true) + return done() + }) + }) }) it('should call the ReferalAllocator', function (done) { @@ -267,10 +270,12 @@ describe('UserRegistrationHandler', function () { }) it('should ask the UserRegistrationHandler to register user', function () { - sinon.assert.calledWith(this.handler.registerNewUser, { - email: this.email, - password: this.password, - }) + return this.handler.registerNewUser + .calledWith({ + email: this.email, + password: this.password, + }) + .should.equal(true) }) it('should generate a new password reset token', function () {