diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index ac0407a008..4ac649e958 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -37,7 +37,7 @@ const BrandVariationsHandler = require('../BrandVariations/BrandVariationsHandle const UserController = require('../User/UserController') const AnalyticsManager = require('../Analytics/AnalyticsManager') const Modules = require('../../infrastructure/Modules') -const SplitTestV2Handler = require('../SplitTests/SplitTestV2Handler') +const SplitTestHandler = require('../SplitTests/SplitTestHandler') const { getNewLogsUIVariantForUser } = require('../Helpers/NewLogsUI') const FeaturesUpdater = require('../Subscription/FeaturesUpdater') const SpellingHandler = require('../Spelling/SpellingHandler') @@ -607,10 +607,7 @@ const ProjectController = { } // null test targeting logged in users - SplitTestV2Handler.promises.getAssignmentForSession( - req.session, - 'null-test-dashboard' - ) + SplitTestHandler.promises.getAssignment(req, 'null-test-dashboard') res.render('project/list', viewModel) timer.done() @@ -737,9 +734,9 @@ const ProjectController = { TpdsProjectFlusher.flushProjectToTpdsIfNeeded(projectId, cb) }, sharingModalSplitTest(cb) { - SplitTestV2Handler.assignInLocalsContextForSession( + SplitTestHandler.assignInLocalsContext( + req, res, - req.session, 'project-share-modal-paywall', {}, () => { @@ -750,9 +747,9 @@ const ProjectController = { }, sharingModalNullTest(cb) { // null test targeting logged in users, for front-end side - SplitTestV2Handler.assignInLocalsContextForSession( + SplitTestHandler.assignInLocalsContext( + req, res, - req.session, 'null-test-share-modal', {}, () => { @@ -762,8 +759,8 @@ const ProjectController = { ) }, newPdfPreviewAssignment(cb) { - SplitTestV2Handler.getAssignmentForSession( - req.session, + SplitTestHandler.getAssignment( + req, 'react-pdf-preview-rollout', {}, (error, assignment) => { diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index 989e21b14f..3bc71ebd6f 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -1,127 +1,284 @@ const UserGetter = require('../User/UserGetter') const UserUpdater = require('../User/UserUpdater') const AnalyticsManager = require('../Analytics/AnalyticsManager') -const Settings = require('@overleaf/settings') -const _ = require('lodash') +const LocalsHelper = require('./LocalsHelper') const crypto = require('crypto') -const OError = require('@overleaf/o-error') +const _ = require('lodash') const { callbackify } = require('util') +const splitTestCache = require('./SplitTestCache') -const duplicateSplitTest = _.findKey( - _.groupBy(Settings.splitTests, 'id'), - group => { - return group.length > 1 +const DEFAULT_VARIANT = 'default' +const ALPHA_PHASE = 'alpha' +const BETA_PHASE = 'beta' +const DEFAULT_ASSIGNMENT = { + variant: DEFAULT_VARIANT, + analytics: { + segmentation: {}, + }, +} + +/** + * Get the assignment of a user to a split test by their session. + * + * @example + * // Assign user and record an event + * + * const assignment = await SplitTestHandler.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 req the request + * @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 getAssignment(req, splitTestName, options) { + const query = req.query || {} + if (query[splitTestName]) { + return { + variant: query[splitTestName], + analytics: { + segmentation: {}, + }, + } } -) -if (duplicateSplitTest) { - throw new OError( - `Split test IDs must be unique: ${duplicateSplitTest} is defined at least twice` + const { userId, analyticsId } = AnalyticsManager.getIdsFromSession( + req.session + ) + return _getAssignment( + analyticsId, + userId, + req.session, + splitTestName, + options ) } -const ACTIVE_SPLIT_TESTS = [] -for (const splitTest of Settings.splitTests) { - for (const variant of splitTest.variants) { - if (variant.id === 'default') { - throw new OError( - `Split test variant ID cannot be 'default' (reserved value), defined in split test ${JSON.stringify( - splitTest - )}` +/** + * Get the assignment of a user to a split test by their session and stores it in the locals context. + * + * @param req the request + * @param res the Express response object + * @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} + */ +async function assignInLocalsContext(req, res, splitTestName, options) { + const assignment = await getAssignment(req, splitTestName, options) + LocalsHelper.setSplitTestVariant( + res.locals, + splitTestName, + assignment.variant + ) +} + +async function _getAssignment( + analyticsId, + userId, + session, + splitTestName, + options +) { + if (!analyticsId && !userId) { + return DEFAULT_ASSIGNMENT + } + const splitTest = await splitTestCache.get(splitTestName) + if (splitTest) { + const currentVersion = splitTest.getCurrentVersion() + const cachedVariant = _getCachedVariantFromSession( + session, + splitTest.name, + currentVersion + ) + if (currentVersion.active) { + if (cachedVariant) { + return _makeAssignment(splitTest, cachedVariant, currentVersion) + } + const { activeForUser, selectedVariantName, phase, versionNumber } = + await _getAssignmentMetadata(analyticsId, userId, splitTest) + if (activeForUser) { + const assignmentConfig = { + userId, + analyticsId, + session, + splitTestName, + variantName: selectedVariantName, + phase, + versionNumber, + } + if (options && options.sync === true) { + await _updateVariantAssignment(assignmentConfig) + } else { + _updateVariantAssignment(assignmentConfig) + } + return _makeAssignment(splitTest, selectedVariantName, currentVersion) + } + } + } + return DEFAULT_ASSIGNMENT +} + +async function _getAssignmentMetadata(analyticsId, 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 { + return { + activeForUser: false, + } + } + } + const percentile = _getPercentile(analyticsId, splitTest.name, phase) + const selectedVariantName = _getVariantFromPercentile( + currentVersion.variants, + percentile + ) + return { + activeForUser: true, + selectedVariantName: selectedVariantName || DEFAULT_VARIANT, + phase, + versionNumber: currentVersion.versionNumber, + } +} + +function _getPercentile(analyticsId, splitTestName, splitTestPhase) { + const hash = crypto + .createHash('md5') + .update(analyticsId + splitTestName + splitTestPhase) + .digest('hex') + const hashPrefix = hash.substr(0, 8) + return Math.floor( + ((parseInt(hashPrefix, 16) % 0xffffffff) / 0xffffffff) * 100 + ) +} + +function _getVariantFromPercentile(variants, percentile) { + for (const variant of variants) { + for (const stripe of variant.rolloutStripes) { + if (percentile >= stripe.start && percentile < stripe.end) { + return variant.name + } + } + } +} + +async function _updateVariantAssignment({ + userId, + analyticsId, + session, + splitTestName, + phase, + versionNumber, + variantName, +}) { + const persistedAssignment = { + variantName, + versionNumber, + phase, + assignedAt: new Date(), + } + // if the user is logged in + 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 + ) + } + } + } + // otherwise this is an anonymous user, we store assignments in session to persist them on registration + else if (session) { + if (!session.splitTests) { + session.splitTests = {} + } + if (!session.splitTests[splitTestName]) { + session.splitTests[splitTestName] = [] + } + const existingAssignment = _.find(session.splitTests[splitTestName], { + versionNumber, + }) + if (!existingAssignment) { + session.splitTests[splitTestName].push(persistedAssignment) + AnalyticsManager.setUserPropertyForAnalyticsId( + analyticsId, + `split-test-${splitTestName}-${versionNumber}`, + variantName ) } } - const totalVariantsRolloutPercent = _.sumBy( - splitTest.variants, - 'rolloutPercent' - ) - if (splitTest.active) { - if (totalVariantsRolloutPercent > 100) { - for (const variant of splitTest.variants) { - variant.rolloutPercent = - (variant.rolloutPercent * 100) / totalVariantsRolloutPercent - } - } - if (totalVariantsRolloutPercent > 0) { - ACTIVE_SPLIT_TESTS.push(splitTest) - } - } } -async function getTestSegmentation(userId, splitTestId) { - const splitTest = _.find(ACTIVE_SPLIT_TESTS, ['id', splitTestId]) - if (splitTest) { - const alreadyAssignedVariant = await getAlreadyAssignedVariant( - userId, - splitTestId - ) - if (alreadyAssignedVariant) { - return { - enabled: true, - variant: alreadyAssignedVariant, - } - } else { - const variant = await assignUserToVariant(userId, splitTest) - return { - enabled: true, - variant, - } - } - } +function _makeAssignment(splitTest, variant, currentVersion) { return { - enabled: false, - } -} - -async function getAlreadyAssignedVariant(userId, splitTestId) { - const user = await UserGetter.promises.getUser(userId, { splitTests: 1 }) - if (user && user.splitTests) { - return user.splitTests[splitTestId] - } - return undefined -} - -async function assignUserToVariant(userId, splitTest) { - let userIdAsPercentile = await _getPercentile(userId, splitTest.id) - let selectedVariant = 'default' - for (const variant of splitTest.variants) { - if (userIdAsPercentile < variant.rolloutPercent) { - selectedVariant = variant.id - break - } else { - userIdAsPercentile -= variant.rolloutPercent - } - } - await UserUpdater.promises.updateUser(userId, { - $set: { - [`splitTests.${splitTest.id}`]: selectedVariant, + variant, + analytics: { + segmentation: { + splitTest: splitTest.name, + variant, + phase: currentVersion.phase, + versionNumber: currentVersion.versionNumber, + }, }, - }) - AnalyticsManager.setUserPropertyForUser( - userId, - `split-test-${splitTest.id}`, - selectedVariant - ) - return selectedVariant + } } -function _getPercentile(userId, splitTestId) { - const hash = crypto - .createHash('md5') - .update(userId + splitTestId) - .digest('hex') - const hashPrefix = hash.substr(0, 8) - return Math.floor((parseInt(hashPrefix, 16) / 0xffffffff) * 100) +function _getCachedVariantFromSession(session, splitTestName, currentVersion) { + if (!session.cachedSplitTestAssignments) { + session.cachedSplitTestAssignments = {} + return + } + const cacheKey = `${splitTestName}-${currentVersion.versionNumber}` + if (currentVersion.active) { + return session.cachedSplitTestAssignments[cacheKey] + } else { + delete session.cachedSplitTestAssignments[cacheKey] + } +} + +async function _getUser(id) { + return UserGetter.promises.getUser(id, { + splitTests: 1, + alphaProgram: 1, + betaProgram: 1, + }) } module.exports = { - /** - * @deprecated: use SplitTestV2Handler.getAssignment instead - */ - getTestSegmentation: callbackify(getTestSegmentation), + getAssignment: callbackify(getAssignment), + assignInLocalsContext: callbackify(assignInLocalsContext), promises: { - /** - * @deprecated: use SplitTestV2Handler.promises.getAssignment instead - */ - getTestSegmentation, + getAssignment, + assignInLocalsContext, }, } diff --git a/services/web/app/src/Features/SplitTests/SplitTestMiddleware.js b/services/web/app/src/Features/SplitTests/SplitTestMiddleware.js index 8355dedd97..d08e195fb1 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestMiddleware.js +++ b/services/web/app/src/Features/SplitTests/SplitTestMiddleware.js @@ -1,27 +1,15 @@ -const SplitTestV2Handler = require('./SplitTestV2Handler') -const SplitTestCache = require('./SplitTestCache') -const LocalsHelper = require('./LocalsHelper') +const SplitTestHandler = require('./SplitTestHandler') const logger = require('@overleaf/logger') function loadAssignmentsInLocals(splitTestNames) { return async function (req, res, next) { try { - if (!req.session.cachedSplitTestAssignments) { - req.session.cachedSplitTestAssignments = {} - } for (const splitTestName of splitTestNames) { - if (req.query[splitTestName]) { - LocalsHelper.setSplitTestVariant( - res.locals, - splitTestName, - req.query[splitTestName] - ) - } else { - const splitTest = await SplitTestCache.get(splitTestName) - if (splitTest) { - await _loadAssignmentInLocals(splitTest, req.session, res.locals) - } - } + await SplitTestHandler.promises.assignInLocalsContext( + req, + res, + splitTestName + ) } } catch (error) { logger.error( @@ -33,31 +21,6 @@ function loadAssignmentsInLocals(splitTestNames) { } } -async function _loadAssignmentInLocals(splitTest, session, locals) { - const currentVersion = splitTest.getCurrentVersion() - const cacheKey = `${splitTest.name}-${currentVersion.versionNumber}` - if (currentVersion.active) { - const cachedVariant = session.cachedSplitTestAssignments[cacheKey] - if (cachedVariant) { - LocalsHelper.setSplitTestVariant(locals, splitTest.name, cachedVariant) - } else { - const assignment = - await SplitTestV2Handler.promises.getAssignmentForSession( - session, - splitTest.name - ) - session.cachedSplitTestAssignments[cacheKey] = assignment.variant - LocalsHelper.setSplitTestVariant( - locals, - splitTest.name, - assignment.variant - ) - } - } else { - delete session.cachedSplitTestAssignments[cacheKey] - } -} - module.exports = { loadAssignmentsInLocals, } diff --git a/services/web/app/src/Features/SplitTests/SplitTestV2Handler.js b/services/web/app/src/Features/SplitTests/SplitTestV2Handler.js deleted file mode 100644 index d01fa38073..0000000000 --- a/services/web/app/src/Features/SplitTests/SplitTestV2Handler.js +++ /dev/null @@ -1,303 +0,0 @@ -const UserGetter = require('../User/UserGetter') -const UserUpdater = require('../User/UserUpdater') -const AnalyticsManager = require('../Analytics/AnalyticsManager') -const UserAnalyticsIdCache = require('../Analytics/UserAnalyticsIdCache') -const LocalsHelper = require('./LocalsHelper') -const crypto = require('crypto') -const _ = require('lodash') -const { callbackify } = require('util') -const splitTestCache = require('./SplitTestCache') - -const DEFAULT_VARIANT = 'default' -const ALPHA_PHASE = 'alpha' -const BETA_PHASE = 'beta' -const DEFAULT_ASSIGNMENT = { - variant: DEFAULT_VARIANT, - analytics: { - segmentation: {}, - }, -} - -/** - * Get the assignment of a user to a split test. - * - * @example - * // Assign user and record an event - * - * const assignment = await SplitTestV2Handler.getAssignment(userId, 'example-project') - * if (assignment.variant === 'awesome-new-version') { - * // execute my awesome change - * } - * else { - * // execute the default behaviour (control group) - * } - * // then record an event - * AnalyticsManager.recordEventForUser(userId, 'example-project-created', { - * projectId: project._id, - * ...assignment.analytics.segmentation - * }) - * - * @param userId the user's ID - * @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 getAssignment(userId, splitTestName, options) { - if (!userId) { - return DEFAULT_ASSIGNMENT - } - 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) -} - -/** - * Get the assignment of a user to a split test by their ID and stores it in the locals context. - * - * @param res the Express response object - * @param userId the user ID - * @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} - */ -async function assignInLocalsContext(res, userId, splitTestName, options) { - const assignment = await getAssignment(userId, splitTestName, options) - LocalsHelper.setSplitTestVariant( - res.locals, - splitTestName, - assignment.variant - ) -} - -/** - * Get the assignment of a user to a split test by their session and stores it in the locals context. - * - * @param res the Express response object - * @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} - */ -async function assignInLocalsContextForSession( - res, - session, - splitTestName, - options -) { - const assignment = await getAssignmentForSession( - session, - splitTestName, - options - ) - LocalsHelper.setSplitTestVariant( - res.locals, - splitTestName, - assignment.variant - ) -} - -async function _getAssignment( - analyticsId, - userId, - session, - splitTestName, - options -) { - if (!analyticsId && !userId) { - return DEFAULT_ASSIGNMENT - } - const splitTest = await splitTestCache.get(splitTestName) - if (splitTest) { - const currentVersion = splitTest.getCurrentVersion() - if (currentVersion.active) { - const { activeForUser, selectedVariantName, phase, versionNumber } = - await _getAssignmentMetadata(analyticsId, userId, splitTest) - if (activeForUser) { - const assignmentConfig = { - userId, - analyticsId, - session, - splitTestName, - variantName: selectedVariantName, - phase, - versionNumber, - } - if (options && options.sync === true) { - await _updateVariantAssignment(assignmentConfig) - } else { - _updateVariantAssignment(assignmentConfig) - } - return { - variant: selectedVariantName, - analytics: { - segmentation: { - splitTest: splitTestName, - variant: selectedVariantName, - phase, - versionNumber, - }, - }, - } - } - } - } - return DEFAULT_ASSIGNMENT -} - -async function _getAssignmentMetadata(analyticsId, 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 { - return { - activeForUser: false, - } - } - } - const percentile = _getPercentile(analyticsId, splitTest.name, phase) - const selectedVariantName = _getVariantFromPercentile( - currentVersion.variants, - percentile - ) - return { - activeForUser: true, - selectedVariantName: selectedVariantName || DEFAULT_VARIANT, - phase, - versionNumber: currentVersion.versionNumber, - } -} - -function _getPercentile(analyticsId, splitTestName, splitTestPhase) { - const hash = crypto - .createHash('md5') - .update(analyticsId + splitTestName + splitTestPhase) - .digest('hex') - const hashPrefix = hash.substr(0, 8) - return Math.floor( - ((parseInt(hashPrefix, 16) % 0xffffffff) / 0xffffffff) * 100 - ) -} - -function _getVariantFromPercentile(variants, percentile) { - for (const variant of variants) { - for (const stripe of variant.rolloutStripes) { - if (percentile >= stripe.start && percentile < stripe.end) { - return variant.name - } - } - } -} - -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, - }) - if (!existingAssignment) { - session.splitTests[splitTestName].push(persistedAssignment) - AnalyticsManager.setUserPropertyForAnalyticsId( - analyticsId, - `split-test-${splitTestName}-${versionNumber}`, - variantName - ) - } - } -} - -async function _getUser(id) { - return UserGetter.promises.getUser(id, { - splitTests: 1, - alphaProgram: 1, - betaProgram: 1, - }) -} - -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/RecurlyEventHandler.js b/services/web/app/src/Features/Subscription/RecurlyEventHandler.js index c1e182012b..44418a6c12 100644 --- a/services/web/app/src/Features/Subscription/RecurlyEventHandler.js +++ b/services/web/app/src/Features/Subscription/RecurlyEventHandler.js @@ -1,5 +1,5 @@ const AnalyticsManager = require('../Analytics/AnalyticsManager') -const SplitTestV2Handler = require('../SplitTests/SplitTestV2Handler') +const SplitTestHandler = require('../SplitTests/SplitTestHandler') const SubscriptionEmailHandler = require('./SubscriptionEmailHandler') function sendRecurlyAnalyticsEvent(event, eventData) { @@ -63,7 +63,7 @@ async function sendSubscriptionStartedEvent(eventData) { // send the trial onboarding email if (isTrial) { - const assignment = await SplitTestV2Handler.promises.getAssignment( + const assignment = await SplitTestHandler.promises.getAssignment( userId, 'trial-onboarding-email' ) diff --git a/services/web/app/src/infrastructure/mongodb.js b/services/web/app/src/infrastructure/mongodb.js index f7ce25759d..de3e9a9a10 100644 --- a/services/web/app/src/infrastructure/mongodb.js +++ b/services/web/app/src/infrastructure/mongodb.js @@ -62,6 +62,7 @@ async function setupDb() { db.samlCache = internalDb.collection('samlCache') db.samlLogs = internalDb.collection('samlLogs') db.spellingPreferences = internalDb.collection('spellingPreferences') + db.splittests = internalDb.collection('splittests') db.subscriptions = internalDb.collection('subscriptions') db.systemmessages = internalDb.collection('systemmessages') db.tags = internalDb.collection('tags') diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index a98942d22f..bbc8b2243a 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -129,28 +129,12 @@ describe('ProjectController', function () { .returns({ newLogsUI: false, subvariant: null }), } this.SplitTestHandler = { - promises: { - getTestSegmentation: sinon.stub().resolves({ enabled: false }), - }, - getTestSegmentation: sinon.stub().yields(null, { enabled: false }), - } - this.SplitTestV2Handler = { promises: { getAssignment: sinon.stub().resolves({ variant: 'default' }), - getAssignmentForSession: sinon.stub().resolves({ variant: 'default' }), assignInLocalsContext: sinon.stub().resolves({ variant: 'default' }), - assignInLocalsContextForSession: sinon - .stub() - .resolves({ variant: 'default' }), }, getAssignment: sinon.stub().yields(null, { variant: 'default' }), - getAssignmentForSession: sinon - .stub() - .yields(null, { variant: 'default' }), assignInLocalsContext: sinon.stub().yields(null, { variant: 'default' }), - assignInLocalsContextForSession: sinon - .stub() - .yields(null, { variant: 'default' }), } this.ProjectController = SandboxedModule.require(MODULE_PATH, { @@ -159,7 +143,6 @@ describe('ProjectController', function () { '@overleaf/settings': this.settings, '@overleaf/metrics': this.Metrics, '../SplitTests/SplitTestHandler': this.SplitTestHandler, - '../SplitTests/SplitTestV2Handler': this.SplitTestV2Handler, './ProjectDeleter': this.ProjectDeleter, './ProjectDuplicator': this.ProjectDuplicator, './ProjectCreationHandler': this.ProjectCreationHandler, @@ -1126,21 +1109,6 @@ describe('ProjectController', function () { }) describe('pdf caching feature flags', function () { - /* eslint-disable mocha/no-identical-title */ - function showNoVariant() { - beforeEach(function () { - this.SplitTestHandler.getTestSegmentation = sinon - .stub() - .yields(null, { enabled: false }) - }) - } - function showVariant(variant) { - beforeEach(function () { - this.SplitTestHandler.getTestSegmentation = sinon - .stub() - .yields(null, { enabled: true, variant }) - }) - } function expectBandwidthTrackingEnabled() { it('should track pdf bandwidth', function (done) { this.res.render = (pageName, opts) => { @@ -1177,160 +1145,11 @@ describe('ProjectController', function () { this.ProjectController.loadEditor(this.req, this.res) }) } - function expectToCollectMetricsAndCachePDF() { - describe('with no query', function () { - expectBandwidthTrackingEnabled() - expectPDFCachingEnabled() - }) - - describe('with enable_pdf_caching=false', function () { - beforeEach(function () { - this.req.query.enable_pdf_caching = 'false' - }) - expectBandwidthTrackingDisabled() - expectPDFCachingDisabled() - }) - - describe('with enable_pdf_caching=true', function () { - beforeEach(function () { - this.req.query.enable_pdf_caching = 'true' - }) - expectBandwidthTrackingEnabled() - expectPDFCachingEnabled() - }) - } - function expectToCollectMetricsOnly() { - describe('with no query', function () { - expectBandwidthTrackingEnabled() - expectPDFCachingDisabled() - }) - - describe('with enable_pdf_caching=false', function () { - beforeEach(function () { - this.req.query.enable_pdf_caching = 'false' - }) - expectBandwidthTrackingDisabled() - expectPDFCachingDisabled() - }) - - describe('with enable_pdf_caching=true', function () { - beforeEach(function () { - this.req.query.enable_pdf_caching = 'true' - }) - expectBandwidthTrackingEnabled() - expectPDFCachingDisabled() - }) - } - - function expectToCachePDFOnly() { - describe('with no query', function () { - expectBandwidthTrackingDisabled() - expectPDFCachingEnabled() - }) - - describe('with enable_pdf_caching=false', function () { - beforeEach(function () { - this.req.query.enable_pdf_caching = 'false' - }) - expectBandwidthTrackingDisabled() - expectPDFCachingDisabled() - }) - - describe('with enable_pdf_caching=true', function () { - beforeEach(function () { - this.req.query.enable_pdf_caching = 'true' - }) - expectBandwidthTrackingDisabled() - expectPDFCachingEnabled() - }) - } - - function expectToNotBeEnrolledAtAll() { - describe('with no query', function () { - expectBandwidthTrackingDisabled() - expectPDFCachingDisabled() - }) - - describe('with enable_pdf_caching=false', function () { - beforeEach(function () { - this.req.query.enable_pdf_caching = 'false' - }) - expectBandwidthTrackingDisabled() - expectPDFCachingDisabled() - }) - - describe('with enable_pdf_caching=true', function () { - beforeEach(function () { - this.req.query.enable_pdf_caching = 'true' - }) - expectBandwidthTrackingDisabled() - expectPDFCachingDisabled() - }) - } - - function tagAnonymous() { - beforeEach(function () { - this.SessionManager.isUserLoggedIn = sinon.stub().returns(false) - }) - } beforeEach(function () { this.settings.enablePdfCaching = true }) - describe('during regular roll-out', function () { - before(function () { - this.skip() - }) - describe('disabled', function () { - showNoVariant() - - describe('regular user', function () { - expectToNotBeEnrolledAtAll() - }) - describe('anonymous user', function () { - tagAnonymous() - expectToCachePDFOnly() - }) - }) - - describe('variant=collect-metrics', function () { - showVariant('collect-metrics') - - describe('regular user', function () { - expectToCollectMetricsOnly() - }) - describe('anonymous user', function () { - tagAnonymous() - expectToCachePDFOnly() - }) - }) - - describe('variant=collect-metrics-and-enable-caching', function () { - showVariant('collect-metrics-and-enable-caching') - - describe('regular user', function () { - expectToCollectMetricsAndCachePDF() - }) - describe('anonymous user', function () { - tagAnonymous() - expectToCachePDFOnly() - }) - }) - - describe('variant=enable-caching-only', function () { - showVariant('enable-caching-only') - - describe('regular user', function () { - expectToCachePDFOnly() - }) - describe('anonymous user', function () { - tagAnonymous() - expectToCachePDFOnly() - }) - }) - }) - describe('during opt-in only', function () { describe('with no query', function () { expectBandwidthTrackingDisabled() diff --git a/services/web/test/unit/src/SplitTests/SplitTestMiddlewareTests.js b/services/web/test/unit/src/SplitTests/SplitTestMiddlewareTests.js index 5e5164fd5a..fbbbb21714 100644 --- a/services/web/test/unit/src/SplitTests/SplitTestMiddlewareTests.js +++ b/services/web/test/unit/src/SplitTests/SplitTestMiddlewareTests.js @@ -5,7 +5,6 @@ const modulePath = path.join( '../../../../app/src/Features/SplitTests/SplitTestMiddleware' ) const sinon = require('sinon') -const { assert } = require('chai') const MockResponse = require('../helpers/MockResponse') const MockRequest = require('../helpers/MockRequest') @@ -13,75 +12,27 @@ describe('SplitTestMiddleware', function () { beforeEach(function () { this.SplitTestMiddleware = SandboxedModule.require(modulePath, { requires: { - './SplitTestV2Handler': (this.SplitTestV2Handler = { + './SplitTestHandler': (this.SplitTestHandler = { promises: { - getAssignmentForSession: sinon.stub().resolves(), + assignInLocalsContext: sinon.stub().resolves(), }, }), - './SplitTestCache': (this.SplitTestCache = { - get: sinon.stub().resolves(), - }), }, }) this.req = new MockRequest() - this.req.session = {} this.res = new MockResponse() this.next = sinon.stub() }) - it('assign split test variant in locals', async function () { - this.SplitTestCache.get.withArgs('ui-overhaul').resolves({ - name: 'ui-overhaul', - getCurrentVersion: () => ({ - versionNumber: 1, - active: true, - }), - }) - this.SplitTestV2Handler.promises.getAssignmentForSession - .withArgs(this.req.session, 'ui-overhaul') - .resolves({ - variant: 'new', - }) - - const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([ - 'ui-overhaul', - ]) - await middleware(this.req, this.res, this.next) - - assert.equal(this.res.locals.splitTestVariants['ui-overhaul'], 'new') - assert.deepEqual(this.req.session.cachedSplitTestAssignments, { - 'ui-overhaul-1': 'new', - }) - sinon.assert.calledOnce(this.next) - }) - - it('assign multiple split test variant in locals', async function () { - this.SplitTestCache.get - .withArgs('ui-overhaul') - .resolves({ - name: 'ui-overhaul', - getCurrentVersion: () => ({ - versionNumber: 1, - active: true, - }), - }) - .withArgs('other-test') - .resolves({ - name: 'other-test', - getCurrentVersion: () => ({ - versionNumber: 1, - active: true, - }), - }) - - this.SplitTestV2Handler.promises.getAssignmentForSession - .withArgs(this.req.session, 'ui-overhaul') + it('assign multiple split test variants in locals', async function () { + this.SplitTestHandler.promises.assignInLocalsContext + .withArgs(this.req, 'ui-overhaul') .resolves({ variant: 'default', }) - this.SplitTestV2Handler.promises.getAssignmentForSession - .withArgs(this.req.session, 'other-test') + this.SplitTestHandler.promises.assignInLocalsContext + .withArgs(this.req, 'other-test') .resolves({ variant: 'foobar', }) @@ -92,135 +43,47 @@ describe('SplitTestMiddleware', function () { ]) await middleware(this.req, this.res, this.next) - assert.equal(this.res.locals.splitTestVariants['ui-overhaul'], 'default') - assert.equal(this.res.locals.splitTestVariants['other-test'], 'foobar') - assert.deepEqual(this.req.session.cachedSplitTestAssignments, { - 'ui-overhaul-1': 'default', - 'other-test-1': 'foobar', - }) - sinon.assert.calledOnce(this.next) - }) - - it('variants are overridden in locals with query parameters', async function () { - this.SplitTestCache.get.withArgs('active-split-test').resolves({ - name: 'active-split-test', - getCurrentVersion: () => ({ - versionNumber: 1, - active: true, - }), - }) - - this.SplitTestV2Handler.promises.getAssignmentForSession - .withArgs(this.req.session, 'active-split-test') - .resolves({ - variant: 'default', - }) - - const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([ - 'active-split-test', - ]) - - this.req.query['active-split-test'] = 'variant' - - await middleware(this.req, this.res, this.next) - - assert.equal( - this.res.locals.splitTestVariants['active-split-test'], - 'variant' + sinon.assert.calledWith( + this.SplitTestHandler.promises.assignInLocalsContext, + this.req, + this.res, + 'ui-overhaul' ) - assert.deepEqual(this.req.session.cachedSplitTestAssignments, {}) // variants overriden using req.query are not cached - sinon.assert.calledOnce(this.next) - }) - - it('non-active split tests can be set in locals with query parameters', async function () { - const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([ - 'non-active-split-test', - ]) - - this.req.query['non-active-split-test'] = 'variant' - - await middleware(this.req, this.res, this.next) - - assert.equal( - this.res.locals.splitTestVariants['non-active-split-test'], - 'variant' + sinon.assert.calledWith( + this.SplitTestHandler.promises.assignInLocalsContext, + this.req, + this.res, + 'other-test' ) - assert.deepEqual(this.req.session.cachedSplitTestAssignments, {}) // variants overriden using req.query are not cached sinon.assert.calledOnce(this.next) }) - it('cached assignment in session is used', async function () { - this.req.session.cachedSplitTestAssignments = { - 'ui-overhaul-1': 'cached-variant', - } - this.SplitTestCache.get.withArgs('ui-overhaul').resolves({ - name: 'ui-overhaul', - getCurrentVersion: () => ({ - versionNumber: 1, - active: true, - }), - }) + it('assign no split test variant in locals', async function () { + const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([]) - const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([ - 'ui-overhaul', - ]) await middleware(this.req, this.res, this.next) - sinon.assert.notCalled( - this.SplitTestV2Handler.promises.getAssignmentForSession - ) - assert.equal( - this.res.locals.splitTestVariants['ui-overhaul'], - 'cached-variant' - ) - assert.deepEqual(this.req.session.cachedSplitTestAssignments, { - 'ui-overhaul-1': 'cached-variant', - }) + sinon.assert.notCalled(this.SplitTestHandler.promises.assignInLocalsContext) sinon.assert.calledOnce(this.next) }) - it('inactive split test is not assigned in locals', async function () { - this.SplitTestCache.get.withArgs('ui-overhaul').resolves({ - name: 'ui-overhaul', - getCurrentVersion: () => ({ - versionNumber: 1, - active: false, - }), - }) - - const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([ - 'ui-overhaul', - ]) - await middleware(this.req, this.res, this.next) - - assert.equal(this.res.locals.splitTestVariants, undefined) - assert.deepEqual(this.req.session.cachedSplitTestAssignments, {}) - sinon.assert.calledOnce(this.next) - }) - - it('not existing split test is not assigned in locals', async function () { - this.SplitTestCache.get.withArgs('not-found').resolves(undefined) - - const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([ - 'not-found', - ]) - await middleware(this.req, this.res, this.next) - - assert.equal(this.res.locals.splitTestVariants, undefined) - assert.deepEqual(this.req.session.cachedSplitTestAssignments, {}) - sinon.assert.calledOnce(this.next) - }) - - it('next middleware is called even if there is an error', async function () { - this.SplitTestCache.get.throws('some error') + it('exception thrown by assignment does not fail the request', async function () { + this.SplitTestHandler.promises.assignInLocalsContext + .withArgs(this.req, this.res, 'some-test') + .throws(new Error('failure')) const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([ 'some-test', ]) + await middleware(this.req, this.res, this.next) - assert.equal(this.res.locals.splitTestVariants, undefined) - assert.deepEqual(this.req.session.cachedSplitTestAssignments, {}) + sinon.assert.calledWith( + this.SplitTestHandler.promises.assignInLocalsContext, + this.req, + this.res, + 'some-test' + ) sinon.assert.calledOnce(this.next) }) }) diff --git a/services/web/test/unit/src/Subscription/RecurlyEventHandlerTests.js b/services/web/test/unit/src/Subscription/RecurlyEventHandlerTests.js index 9c6d5e4cf4..6506e6e02a 100644 --- a/services/web/test/unit/src/Subscription/RecurlyEventHandlerTests.js +++ b/services/web/test/unit/src/Subscription/RecurlyEventHandlerTests.js @@ -29,7 +29,7 @@ describe('RecurlyEventHandler', function () { './SubscriptionEmailHandler': (this.SubscriptionEmailHandler = { sendTrialOnboardingEmail: sinon.stub(), }), - '../SplitTests/SplitTestV2Handler': (this.SplitTestV2Handler = { + '../SplitTests/SplitTestHandler': (this.SplitTestHandler = { promises: { getAssignment: sinon.stub().resolves({ active: false }), }, @@ -76,14 +76,14 @@ describe('RecurlyEventHandler', function () { true ) sinon.assert.calledWith( - this.SplitTestV2Handler.promises.getAssignment, + this.SplitTestHandler.promises.getAssignment, this.userId, 'trial-onboarding-email' ) }) it('sends free trial onboarding email if user in ab group', async function () { - this.SplitTestV2Handler.promises.getAssignment = sinon + this.SplitTestHandler.promises.getAssignment = sinon .stub() .resolves({ active: true, variant: 'send-email' }) this.userId = '123456789trial' @@ -94,7 +94,7 @@ describe('RecurlyEventHandler', function () { await this.RecurlyEventHandler.sendSubscriptionStartedEvent(this.eventData) sinon.assert.calledWith( - this.SplitTestV2Handler.promises.getAssignment, + this.SplitTestHandler.promises.getAssignment, this.userId, 'trial-onboarding-email' ) diff --git a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js b/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js index 170afae8de..3f98cc2f92 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js @@ -133,12 +133,12 @@ describe('SubscriptionController', function () { } this.SplitTestV2Hander = { promises: { - getAssignmentForSession: sinon.stub().resolves({ variant: 'default' }), + getAssignment: sinon.stub().resolves({ variant: 'default' }), }, } this.SubscriptionController = SandboxedModule.require(modulePath, { requires: { - '../SplitTests/SplitTestV2Handler': this.SplitTestV2Hander, + '../SplitTests/SplitTestHandler': this.SplitTestV2Hander, '../Authentication/SessionManager': this.SessionManager, './SubscriptionHandler': this.SubscriptionHandler, './PlansLocator': this.PlansLocator, @@ -163,9 +163,6 @@ describe('SubscriptionController', function () { recordEventForSession: sinon.stub(), setUserPropertyForUser: sinon.stub(), }), - '../SplitTests/SplitTestHandler': (this.SplitTestHandler = { - getTestSegmentation: () => {}, - }), }, })