From 04432478e1d2b13a3a50ba3b303e24d0f660930d Mon Sep 17 00:00:00 2001 From: Jimmy Domagala-Tang Date: Mon, 24 Jun 2024 09:27:13 -0400 Subject: [PATCH] Merge pull request #19053 from overleaf/ab-split-tests-first-time-assignments [web] Return isFirstTimeAssignment flag with split test assignments GitOrigin-RevId: 70954470fbd9430749d83d8d1e08a3969d4a09e6 --- .../src/Features/Project/ProjectController.js | 25 +++++ .../Features/SplitTests/SplitTestHandler.js | 101 +++++++++++------- .../app/src/Features/SplitTests/types.d.ts | 12 +++ .../src/Project/ProjectControllerTests.js | 5 + 4 files changed, 104 insertions(+), 39 deletions(-) create mode 100644 services/web/app/src/Features/SplitTests/types.d.ts diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index 57b093964c..5ede7b89d0 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -41,6 +41,7 @@ const ProjectAuditLogHandler = require('./ProjectAuditLogHandler') const PublicAccessLevels = require('../Authorization/PublicAccessLevels') const TagsHandler = require('../Tags/TagsHandler') const TutorialHandler = require('../Tutorial/TutorialHandler') +const UserUpdater = require('../User/UserUpdater') /** * @typedef {import("./types").GetProjectsRequest} GetProjectsRequest @@ -423,6 +424,30 @@ const _ProjectController = { isInvitedMember, } = userValues + // check if a user is not in the writefull-oauth-promotion, in which case they may be part of the auto trial group + if ( + !anonymous && + splitTestAssignments['writefull-oauth-promotion']?.variant === 'default' + ) { + // since we are auto-enrolling users into writefull if they are part of the group, we only want to + // auto enroll (set writefull to true) if its the first time they have entered the test + // this ensures that they can still turn writefull off (otherwise, we would be setting writefull on every time they access their projects) + const { variant, metadata } = + await SplitTestHandler.promises.getAssignment( + req, + res, + 'writefull-auto-load' + ) + if (variant === 'enabled' && metadata?.isFirstNonDefaultAssignment) { + await UserUpdater.promises.updateUser(userId, { + $set: { + writefull: { enabled: true }, + }, + }) + user.writefull.enabled = true + } + } + const brandVariation = project?.brandVariationId ? await BrandVariationsHandler.promises.getBrandVariationById( project.brandVariationId diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index 8e1d2a3d51..6ba9074d84 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -16,15 +16,17 @@ const logger = require('@overleaf/logger') const SplitTestSessionHandler = require('./SplitTestSessionHandler') const SplitTestUserGetter = require('./SplitTestUserGetter') +/** + * @typedef {import("./types").Assignment} Assignment + */ + const DEFAULT_VARIANT = 'default' const ALPHA_PHASE = 'alpha' const BETA_PHASE = 'beta' const RELEASE_PHASE = 'release' const DEFAULT_ASSIGNMENT = { variant: DEFAULT_VARIANT, - analytics: { - segmentation: {}, - }, + metadata: {}, } /** @@ -40,17 +42,12 @@ const DEFAULT_ASSIGNMENT = { * 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 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<{variant: string, analytics: {segmentation: {splitTest: string, variant: string, phase: string, versionNumber: number}|{}}}>} + * @returns {Promise} */ async function getAssignment(req, res, splitTestName, { sync = false } = {}) { const query = req.query || {} @@ -69,9 +66,7 @@ async function getAssignment(req, res, splitTestName, { sync = false } = {}) { if (variants.includes(queryVariant)) { assignment = { variant: queryVariant, - analytics: { - segmentation: {}, - }, + metadata: {}, } } } @@ -112,7 +107,7 @@ async function getAssignment(req, res, splitTestName, { sync = false } = {}) { * @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<{variant: string, analytics: {segmentation: {splitTest: string, variant: string, phase: string, versionNumber: number}|{}}}>} + * @returns {Promise} */ async function getAssignmentForUser( userId, @@ -141,7 +136,7 @@ async function getAssignmentForUser( * @param user the user * @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}|{}}}>} + * @returns {Promise} */ async function getAssignmentForMongoUser( user, @@ -215,7 +210,7 @@ async function getActiveAssignmentsForUser(userId, removeArchived = false) { * To be used only in cases where we need random assignments that are independent of a user or session. * If the test is in alpha or beta phase, always returns the default variant. * @param splitTestName - * @returns {Promise<{variant: string, analytics: {segmentation: {splitTest: string, variant: string, phase: string, versionNumber: number}|{}}}>} + * @returns {Promise} */ async function getOneTimeAssignment(splitTestName) { try { @@ -239,7 +234,13 @@ async function getOneTimeAssignment(splitTestName) { undefined, splitTest ) - return _makeAssignment(splitTest, selectedVariantName, currentVersion) + return _makeAssignment({ + variant: selectedVariantName, + currentVersion, + isFirstNonDefaultAssignment: + selectedVariantName !== DEFAULT_VARIANT && + currentVersion.analyticsEnabled, + }) } catch (error) { logger.error({ err: error }, 'Failed to get one time split test assignment') return DEFAULT_ASSIGNMENT @@ -277,7 +278,7 @@ async function _getAssignment( if (Settings.devToolbar.enabled) { const override = session?.splitTestOverrides?.[splitTestName] if (override) { - return _makeAssignment(splitTest, override, currentVersion) + return _makeAssignment({ variant: override, currentVersion }) } } @@ -308,7 +309,11 @@ async function _getAssignment( ) { return DEFAULT_ASSIGNMENT } else { - return _makeAssignment(splitTest, cachedVariant, currentVersion) + return _makeAssignment({ + variant: cachedVariant, + currentVersion, + isFirstNonDefaultAssignment: false, + }) } } } @@ -325,8 +330,9 @@ async function _getAssignment( user || (userId && (await SplitTestUserGetter.promises.getUser(userId, splitTestName))) - const { activeForUser, selectedVariantName, phase, versionNumber } = - await _getAssignmentMetadata(analyticsId, user, splitTest) + const metadata = await _getAssignmentMetadata(analyticsId, user, splitTest) + const { activeForUser, selectedVariantName, phase, versionNumber } = metadata + if (canUseSessionCache) { SplitTestSessionHandler.setVariantInCache({ session, @@ -336,6 +342,7 @@ async function _getAssignment( activeForUser, }) } + if (activeForUser) { if (currentVersion.analyticsEnabled) { // if the user is logged in, persist the assignment @@ -396,7 +403,20 @@ async function _getAssignment( ) }) } - return _makeAssignment(splitTest, selectedVariantName, currentVersion) + let isFirstNonDefaultAssignment + if (userId) { + isFirstNonDefaultAssignment = metadata.isFirstNonDefaultAssignment + } else { + const assignments = + await SplitTestSessionHandler.promises.getAssignments(session) + isFirstNonDefaultAssignment = !assignments?.[splitTestName] + } + + return _makeAssignment({ + variant: selectedVariantName, + currentVersion, + isFirstNonDefaultAssignment, + }) } return DEFAULT_ASSIGNMENT @@ -415,15 +435,21 @@ async function _getAssignmentMetadata(analyticsId, user, splitTest) { } const userId = user?._id.toString() const percentile = getPercentile(analyticsId || userId, splitTest.name, phase) - const selectedVariantName = _getVariantFromPercentile( - currentVersion.variants, - percentile - ) + const selectedVariantName = + _getVariantFromPercentile(currentVersion.variants, percentile) || + DEFAULT_VARIANT return { activeForUser: true, - selectedVariantName: selectedVariantName || DEFAULT_VARIANT, + selectedVariantName, phase, versionNumber: currentVersion.versionNumber, + isFirstNonDefaultAssignment: + selectedVariantName !== DEFAULT_VARIANT && + currentVersion.analyticsEnabled && + (!Array.isArray(user?.splitTests?.[splitTest.name]) || + !user?.splitTests?.[splitTest.name]?.some( + assignment => assignment.variantName !== DEFAULT_VARIANT + )), } } @@ -492,18 +518,17 @@ async function _recordAssignment({ } } -function _makeAssignment(splitTest, variant, currentVersion) { +function _makeAssignment({ + variant, + currentVersion, + isFirstNonDefaultAssignment, +}) { return { variant, - analytics: { - segmentation: splitTest - ? { - splitTest: splitTest.name, - variant, - phase: currentVersion.phase, - versionNumber: currentVersion.versionNumber, - } - : {}, + metadata: { + phase: currentVersion.phase, + versionNumber: currentVersion.versionNumber, + isFirstNonDefaultAssignment, }, } } @@ -543,9 +568,7 @@ function _getNonSaasAssignment(splitTestName) { if (Settings.splitTestOverrides?.[splitTestName]) { return { variant: Settings.splitTestOverrides?.[splitTestName], - analytics: { - segmentation: {}, - }, + metadata: {}, } } return DEFAULT_ASSIGNMENT diff --git a/services/web/app/src/Features/SplitTests/types.d.ts b/services/web/app/src/Features/SplitTests/types.d.ts new file mode 100644 index 0000000000..47e04def57 --- /dev/null +++ b/services/web/app/src/Features/SplitTests/types.d.ts @@ -0,0 +1,12 @@ +type AssignmentMetadata = { + phase?: 'alpha' | 'beta' | 'release' + versionNumber?: boolean + // only returned when `analyticsEnabled` is set to `true` on the current version + // of the split test, and an assignment is queried for the user for the first time + isFirstNonDefaultAssignment?: boolean +} + +export type Assignment = { + variant: string + metadata: AssignmentMetadata +} diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index 66531ad6d9..2389c4a151 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -239,6 +239,11 @@ describe('ProjectController', function () { '../Survey/SurveyHandler': this.SurveyHandler, './ProjectAuditLogHandler': this.ProjectAuditLogHandler, '../Tutorial/TutorialHandler': this.TutorialHandler, + '../User/UserUpdater': { + promises: { + updateUser: sinon.stub().resolves(), + }, + }, }, })