diff --git a/services/web/app/src/Features/Project/ProjectController.mjs b/services/web/app/src/Features/Project/ProjectController.mjs index c8dae62acb..87c7e5bbd6 100644 --- a/services/web/app/src/Features/Project/ProjectController.mjs +++ b/services/web/app/src/Features/Project/ProjectController.mjs @@ -648,7 +648,11 @@ const _ProjectController = { req, projectId ) - const imageNames = await ProjectHelper.getAllowedImagesForUser(user) + const imageNames = await ProjectHelper.getAllowedImagesForUser( + req, + res, + user + ) const privilegeLevel = await AuthorizationManager.promises.getPrivilegeLevelForProject( diff --git a/services/web/app/src/Features/Project/ProjectCreationHandler.mjs b/services/web/app/src/Features/Project/ProjectCreationHandler.mjs index eb78a4582d..e6de6cd8c1 100644 --- a/services/web/app/src/Features/Project/ProjectCreationHandler.mjs +++ b/services/web/app/src/Features/Project/ProjectCreationHandler.mjs @@ -17,6 +17,7 @@ import _ from 'lodash' import AnalyticsManager from '../Analytics/AnalyticsManager.mjs' import TpdsUpdateSender from '../ThirdPartyDataStore/TpdsUpdateSender.mjs' import SplitTestHandler from '../SplitTests/SplitTestHandler.mjs' +import SplitTestUserGetter from '../SplitTests/SplitTestUserGetter.mjs' import ClsiCacheManager from '../Compile/ClsiCacheManager.mjs' import crypto from 'node:crypto' @@ -271,11 +272,12 @@ async function _createBlankProject( const user = await User.findById(ownerId, { 'ace.spellCheckLanguage': 1, _id: 1, + ...SplitTestUserGetter.getProjection('history-ranges-support'), }) project.spellCheckLanguage = user.ace.spellCheckLanguage const historyRangesSupportAssignment = - await SplitTestHandler.promises.getAssignmentForUser( - user._id, + await SplitTestHandler.promises.getAssignmentForMongoUser( + user, 'history-ranges-support' ) if (historyRangesSupportAssignment.variant === 'enabled') { diff --git a/services/web/app/src/Features/Project/ProjectHelper.mjs b/services/web/app/src/Features/Project/ProjectHelper.mjs index 25951d39bb..1e9b8d8f10 100644 --- a/services/web/app/src/Features/Project/ProjectHelper.mjs +++ b/services/web/app/src/Features/Project/ProjectHelper.mjs @@ -149,14 +149,12 @@ function _addNumericSuffixToProjectName(name, allProjectNames, maxLength) { return null } -async function _monthlyExperimentalImageAllowed(user) { - const userId = user?._id?.toString() - if (!userId) return false - const { variant } = await SplitTestHandler.promises.getAssignmentForUser( - userId, +async function _monthlyExperimentalImageAllowed(req, res) { + return await SplitTestHandler.promises.featureFlagEnabled( + req, + res, 'monthly-texlive' ) - return variant === 'enabled' } function _imageAllowed( @@ -173,12 +171,12 @@ function _imageAllowed( return true } -async function getAllowedImagesForUser(user) { +async function getAllowedImagesForUser(req, res, user) { let images = Settings.allowedImageNames || [] const alphaImagesAllowed = Boolean(user?.alphaProgram) const monthlyExperimentalImagesAllowed = - await _monthlyExperimentalImageAllowed(user) + await _monthlyExperimentalImageAllowed(req, res) images = images.map(image => { return { diff --git a/services/web/app/src/Features/Project/ProjectListController.mjs b/services/web/app/src/Features/Project/ProjectListController.mjs index e2a3daf542..16e5de8893 100644 --- a/services/web/app/src/Features/Project/ProjectListController.mjs +++ b/services/web/app/src/Features/Project/ProjectListController.mjs @@ -539,7 +539,7 @@ async function projectListPage(req, res, next) { const aiBlocked = Features.hasFeature('saas') && !(await _canUseAIAssist(user)) const hasAiAssist = - Features.hasFeature('saas') && (await _userHasAIAssist(user)) + Features.hasFeature('saas') && (await _userHasAIAssist(req, res, user)) const splitTests = [ // Split tests that will be made available to the frontend @@ -949,16 +949,18 @@ function _hasActiveFilter(filters) { } /** + * @param {any} req + * @param {any} res * @param {any} user */ // todo: quota clean-up: rename function and vars -async function _userHasAIAssist(user) { +async function _userHasAIAssist(req, res, user) { let hasPremiumAiFeatures - const inQuotaSplitTest = - await SplitTestHandler.promises.featureFlagEnabledForUser( - user._id, - 'plans-2026-phase-1' - ) + const inQuotaSplitTest = await SplitTestHandler.promises.featureFlagEnabled( + req, + res, + 'plans-2026-phase-1' + ) if (inQuotaSplitTest) { hasPremiumAiFeatures = user.features?.aiUsageQuota === Settings.aiFeatures.unlimitedQuota diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs b/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs index 094da06473..e9ecb612ec 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs @@ -18,6 +18,7 @@ import SplitTestUserGetter from './SplitTestUserGetter.mjs' /** * @import { Assignment } from "./types" + * @import { SplitTestUser } from "./SplitTestUserGetter" */ const DEFAULT_VARIANT = 'default' @@ -47,16 +48,17 @@ const DEFAULT_ASSIGNMENT = { * @param req the request * @param res the Express response object * @param splitTestName the unique name of the split test - * @param {Object} options - * @param {boolean} options.sync - for test purposes only, to force the synchronous update of the user's profile - * @param {boolean} options.includeReferer For ajax requests and downloads include the split test overrides of the page + * @param {Object} [options] + * @param {boolean} [options.sync] - for test purposes only, to force the synchronous update of the user's profile + * @param {boolean} [options.includeReferer] For ajax requests and downloads include the split test overrides of the page + * @param {boolean} [options.ignoreOverrides] Ignore query-string variant overrides (e.g. for backend gating where the user must not be able to force a variant) * @returns {Promise} */ async function getAssignment( req, res, splitTestName, - { sync = false, includeReferer = false } = {} + { sync = false, includeReferer = false, ignoreOverrides = false } = {} ) { let assignment @@ -66,28 +68,30 @@ async function getAssignment( } else { await _loadSplitTestInfoInLocals(res.locals, splitTestName, req.session) - let query = req.query || {} - if (includeReferer && req.headers.referer) { - // Pick up the query of the top-level page, i.e. what's in the browsers address bar, from ajax requests. - // E.g. /project/:id?split-test=foo -> ajax /project/:id/compile should see split-test=foo. - // E.g. /project/:id?split-test=foo -> redirect /project/:id/download/zip should see split-test=foo. - try { - const u = new URL(req.headers.referer, Settings.siteUrl) - query = { - ...Object.fromEntries(u.searchParams.entries()), - ...query, - } - } catch {} - } + if (!ignoreOverrides) { + let query = req.query || {} + if (includeReferer && req.headers.referer) { + // Pick up the query of the top-level page, i.e. what's in the browsers address bar, from ajax requests. + // E.g. /project/:id?split-test=foo -> ajax /project/:id/compile should see split-test=foo. + // E.g. /project/:id?split-test=foo -> redirect /project/:id/download/zip should see split-test=foo. + try { + const u = new URL(req.headers.referer, Settings.siteUrl) + query = { + ...Object.fromEntries(u.searchParams.entries()), + ...query, + } + } catch {} + } - // Check the query string for an override, ignoring an invalid value - const queryVariant = query[splitTestName] - if (queryVariant) { - const variants = await _getVariantNames(splitTestName) - if (variants.includes(queryVariant)) { - assignment = { - variant: queryVariant, - metadata: {}, + // Check the query string for an override, ignoring an invalid value + const queryVariant = query[splitTestName] + if (queryVariant) { + const variants = await _getVariantNames(splitTestName) + if (variants.includes(queryVariant)) { + assignment = { + variant: queryVariant, + metadata: {}, + } } } } @@ -151,6 +155,33 @@ async function getAssignmentForUser( } } +/** + * Get the assignment of a user to a split test from an already-fetched mongo user. + * + * The user must include all the relevant fields. Unless you fetch the full user record, add `SplitTestUserGetter.getProjection(splitTestName)` to the projection. + * + * @param {SplitTestUser} user an already-fetched mongo 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} + */ +async function getAssignmentForMongoUser( + user, + splitTestName, + { sync = false } = {} +) { + const { userId, analyticsId } = _getIdsFromMongoUser(user) // throw outside the try/catch. + try { + if (!Features.hasFeature('saas')) { + return _getNonSaasAssignment(splitTestName) + } + return _getAssignment(splitTestName, { analyticsId, userId, user, sync }) + } catch (error) { + logger.error({ err: error }, 'Failed to get split test assignment for user') + return DEFAULT_ASSIGNMENT + } +} + /** * Returns true if user has already been explicitly assigned to a variant. * This will be false if the user **would** be assigned when calling getAssignment but hasn't yet. @@ -259,13 +290,33 @@ async function getActiveAssignmentsForUser( return {} } + return getActiveAssignmentsForMongoUser(user, removeArchived, ignoreVersion) +} + +/** + * Get a mapping of the active split test assignments from an already-fetched mongo user, avoiding a re-fetch. This should be the full user record. + * @param {SplitTestUser} user + * @param {boolean} removeArchived + * @param {boolean} ignoreVersion + */ +async function getActiveAssignmentsForMongoUser( + user, + removeArchived = false, + ignoreVersion = false +) { + if (!Features.hasFeature('saas')) { + return {} + } + + const { analyticsId } = _getIdsFromMongoUser(user) // throw early. + const splitTests = (await SplitTestCache.get('')).values() const assignments = {} for (const splitTest of splitTests) { if (!splitTest.versions[splitTest.versions.length - 1].active) continue if (removeArchived && splitTest.archived) continue const { activeForUser, selectedVariantName, phase, versionNumber } = - await _getAssignmentMetadata(user.analyticsId, user, splitTest) + await _getAssignmentMetadata(analyticsId, user, splitTest) if (activeForUser) { const assignment = { variantName: selectedVariantName, @@ -371,6 +422,20 @@ async function featureFlagEnabledForUser(userId, splitTestName) { return variant === 'enabled' } +/** + * Checks if a feature flag is enabled from an already-fetched mongo user + * + * See getAssignmentForMongoUser for details on the user. + * + * @param {SplitTestUser} user an already-fetched mongo user + * @param {string} splitTestName - The unique name of the feature flag + * @returns {Promise} True if the user's assigned variant is 'enabled', false otherwise + */ +async function featureFlagEnabledForMongoUser(user, splitTestName) { + const { variant } = await getAssignmentForMongoUser(user, splitTestName) + return variant === 'enabled' +} + /** * Returns an array of valid variant names for the given split test, including default * @@ -388,6 +453,28 @@ async function _getVariantNames(splitTestName) { } } +/** + * Extract the ids needed for a split test assignment from an already-fetched + * mongo user, throwing if a required field is missing from the projection. + * + * Only the ids are validated: the program/`splitTests` fields are read with + * optional chaining and a missing value is a legitimate "not enrolled" state. + * + * @param {SplitTestUser} user + * @return {{userId: string, analyticsId: string}} + */ +function _getIdsFromMongoUser(user) { + const userId = user?._id?.toString() + if (!userId) { + throw new Error('bug: include db.users._id in projection') + } + const analyticsId = user?.analyticsId + if (!analyticsId) { + throw new Error('bug: include db.users.analyticsId in projection') + } + return { userId, analyticsId } +} + async function _getAssignment( splitTestName, { analyticsId, user, userId, session, sync } @@ -749,7 +836,7 @@ async function _recordAssignment({ * @param {string} splitTestName - The name of the split test * @param {string} variantName - The name of the variant * @param {string} phase - The phase of the split test - * @param {Object} user - The user object + * @param {SplitTestUser} user - The user object * @returns {Promise} Whether the counter should be incremented */ async function _shouldIncrementVariantCounter( @@ -1042,20 +1129,28 @@ export default { getPercentile, getAssignment: callbackify(getAssignment), getAssignmentForUser: callbackify(getAssignmentForUser), + getAssignmentForMongoUser: callbackify(getAssignmentForMongoUser), featureFlagEnabled: callbackify(featureFlagEnabled), featureFlagEnabledForUser: callbackify(featureFlagEnabledForUser), + featureFlagEnabledForMongoUser: callbackify(featureFlagEnabledForMongoUser), getOneTimeAssignment: callbackify(getOneTimeAssignment), getActiveAssignmentsForUser: callbackify(getActiveAssignmentsForUser), + getActiveAssignmentsForMongoUser: callbackify( + getActiveAssignmentsForMongoUser + ), hasUserBeenAssignedToVariant: callbackify(hasUserBeenAssignedToVariant), setOverrideInSession, clearOverridesInSession, promises: { getAssignment, getAssignmentForUser, + getAssignmentForMongoUser, featureFlagEnabled, featureFlagEnabledForUser, + featureFlagEnabledForMongoUser, getOneTimeAssignment, getActiveAssignmentsForUser, + getActiveAssignmentsForMongoUser, hasUserBeenAssignedToVariant, decrementLabsVariantCounter, incrementLabsVariantCounterIfBelowLimit, diff --git a/services/web/app/src/Features/SplitTests/SplitTestUserGetter.mjs b/services/web/app/src/Features/SplitTests/SplitTestUserGetter.mjs index b7efe1c529..39c3d43269 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestUserGetter.mjs +++ b/services/web/app/src/Features/SplitTests/SplitTestUserGetter.mjs @@ -2,8 +2,32 @@ import { callbackify } from 'node:util' import Metrics from '@overleaf/metrics' import UserGetter from '../User/UserGetter.mjs' -async function getUser(id, splitTestName, path) { - Metrics.inc('split_test_get_user', 1, { path }) +/** + * A mongo user fetched with the projection from `getProjection`, carrying the + * fields the split test assignment logic reads. This is the shape the + * `*ForMongoUser` SplitTestHandler methods expect. + * + * @typedef {object} SplitTestUser + * @property {import('mongodb').ObjectId} _id + * @property {string} analyticsId + * @property {boolean} [alphaProgram] + * @property {boolean} [betaProgram] + * @property {boolean} labsProgram + * @property {string[]} [labsExperiments] + * @property {Record} [splitTests] + */ + +/** + * Build the mongo projection needed to compute split test assignments for a user. + * + * Call-sites that already fetch a user and want to pass it to one of the + * `*ForMongoUser` SplitTestHandler methods should spread this into their own + * projection, so the user carries exactly the fields the assignment logic reads. + * + * @param {string} [splitTestName] restrict the `splitTests` sub-document to a + * single test (for feature-flag style lookups); omit to fetch all assignments. + */ +function getProjection(splitTestName) { const projection = { analyticsId: 1, alphaProgram: 1, @@ -16,6 +40,18 @@ async function getUser(id, splitTestName, path) { } else { projection.splitTests = 1 } + return projection +} + +/** + * @param id + * @param {string} splitTestName + * @param {string} path + * @return {Promise} + */ +async function getUser(id, splitTestName, path) { + Metrics.inc('split_test_get_user', 1, { path }) + const projection = getProjection(splitTestName) const user = await UserGetter.promises.getUser(id, projection) Metrics.histogram( 'split_test_get_user_from_mongo_size', @@ -26,6 +62,7 @@ async function getUser(id, splitTestName, path) { } export default { + getProjection, getUser: callbackify(getUser), promises: { getUser, diff --git a/services/web/app/src/Features/Subscription/FeaturesUpdater.mjs b/services/web/app/src/Features/Subscription/FeaturesUpdater.mjs index c5da72262e..3104fe518c 100644 --- a/services/web/app/src/Features/Subscription/FeaturesUpdater.mjs +++ b/services/web/app/src/Features/Subscription/FeaturesUpdater.mjs @@ -21,6 +21,7 @@ import { GroupPolicy } from '../../models/GroupPolicy.mjs' import { AI_ADD_ON_CODE } from './AiHelper.mjs' import { fetchNothing } from '@overleaf/fetch-utils' import SplitTestHandler from '../SplitTests/SplitTestHandler.mjs' +import SplitTestUserGetter from '../SplitTests/SplitTestUserGetter.mjs' /** * Enqueue a job for refreshing features for the given user @@ -45,10 +46,10 @@ function featuresEpochIsCurrent(user) { async function refreshFeatures(userId, reason) { const user = await UserGetter.promises.getUser(userId, { _id: 1, - analyticsId: 1, - labsProgram: 1, features: 1, email: 1, + // analyticsId + labsProgram (analytics) and the split test fields below + ...SplitTestUserGetter.getProjection('plans-2026-phase-1'), }) const oldFeatures = _.clone(user.features) const features = await computeFeatures(userId) @@ -96,8 +97,8 @@ async function refreshFeatures(userId, reason) { // todo: quota clean-up: simplify once split test isnt needed let hasPremiumAiFeatures const inQuotaSplitTest = - await SplitTestHandler.promises.featureFlagEnabledForUser( - userId, + await SplitTestHandler.promises.featureFlagEnabledForMongoUser( + user, 'plans-2026-phase-1' ) if (inQuotaSplitTest) { diff --git a/services/web/app/src/infrastructure/rate-limiters/AiFeatureUsageRateLimiter.mjs b/services/web/app/src/infrastructure/rate-limiters/AiFeatureUsageRateLimiter.mjs index 9b8c4e9133..a4ae6a176f 100644 --- a/services/web/app/src/infrastructure/rate-limiters/AiFeatureUsageRateLimiter.mjs +++ b/services/web/app/src/infrastructure/rate-limiters/AiFeatureUsageRateLimiter.mjs @@ -4,6 +4,7 @@ import UserGetter from '../../Features/User/UserGetter.mjs' import FeatureUsageRateLimiter from './FeatureUsageRateLimiter.mjs' import Settings from '@overleaf/settings' import SplitTestHandler from '../../Features/SplitTests/SplitTestHandler.mjs' +import SplitTestUserGetter from '../../Features/SplitTests/SplitTestUserGetter.mjs' import FeaturesHelper from '../../Features/Subscription/FeaturesHelper.mjs' class AiFeatureUsageRateLimiter extends FeatureUsageRateLimiter { @@ -19,11 +20,12 @@ class AiFeatureUsageRateLimiter extends FeatureUsageRateLimiter { const user = await UserGetter.promises.getUser(userId, { features: 1, writefull: 1, + ...SplitTestUserGetter.getProjection('plans-2026-phase-1'), }) // todo: quota clean-up: remove aiErrorAssistant checking, and split test const inQuotaSplitTest = - await SplitTestHandler.promises.featureFlagEnabledForUser( - userId, + await SplitTestHandler.promises.featureFlagEnabledForMongoUser( + user, 'plans-2026-phase-1' ) diff --git a/services/web/app/src/infrastructure/rate-limiters/WorkbenchRateLimiter.mjs b/services/web/app/src/infrastructure/rate-limiters/WorkbenchRateLimiter.mjs index 07f3c980dd..61e063ccb6 100644 --- a/services/web/app/src/infrastructure/rate-limiters/WorkbenchRateLimiter.mjs +++ b/services/web/app/src/infrastructure/rate-limiters/WorkbenchRateLimiter.mjs @@ -1,5 +1,6 @@ // @ts-check import SplitTestHandler from '../../Features/SplitTests/SplitTestHandler.mjs' +import SplitTestUserGetter from '../../Features/SplitTests/SplitTestUserGetter.mjs' import UserGetter from '../../Features/User/UserGetter.mjs' import TokenUsageRateLimiter from './TokenUsageRateLimiter.mjs' /** @typedef {{usage?: number | null, periodStart?: Date | null}} FeatureUsage */ @@ -29,7 +30,7 @@ class WorkbenchRateLimiter extends TokenUsageRateLimiter { const user = await UserGetter.promises.getUser(userId, { features: 1, writefull: 1, - alphaProgram: 1, + ...SplitTestUserGetter.getProjection('plans-2026-phase-1'), }) if (user?.alphaProgram) { @@ -39,8 +40,8 @@ class WorkbenchRateLimiter extends TokenUsageRateLimiter { // todo: quota clean-up: remove split test let hasAddOn const inQuotaSplitTest = - await SplitTestHandler.promises.featureFlagEnabledForUser( - userId, + await SplitTestHandler.promises.featureFlagEnabledForMongoUser( + user, 'plans-2026-phase-1' ) if (inQuotaSplitTest) { diff --git a/services/web/app/src/models/User.mjs b/services/web/app/src/models/User.mjs index 9989804b20..24ddeda55c 100644 --- a/services/web/app/src/models/User.mjs +++ b/services/web/app/src/models/User.mjs @@ -221,7 +221,7 @@ export const UserSchema = new Schema( }, alphaProgram: { type: Boolean, default: false }, // experimental features betaProgram: { type: Boolean, default: false }, - labsProgram: { type: Boolean, default: false }, + labsProgram: { type: Boolean, default: false, required: true }, labsExperiments: { type: Array, default: [] }, overleaf: { id: { type: Number }, @@ -239,7 +239,7 @@ export const UserSchema = new Schema( }, onboardingEmailSentAt: { type: Date }, splitTests: Schema.Types.Mixed, - analyticsId: { type: String }, + analyticsId: { type: String, required: true }, completedTutorials: Schema.Types.Mixed, suspended: { type: Boolean }, dsMobileApp: { diff --git a/services/web/test/acceptance/src/ModelTests.mjs b/services/web/test/acceptance/src/ModelTests.mjs index 651a55f320..771c703740 100644 --- a/services/web/test/acceptance/src/ModelTests.mjs +++ b/services/web/test/acceptance/src/ModelTests.mjs @@ -6,15 +6,16 @@ import Features from '../../../app/src/infrastructure/Features.mjs' describe('mongoose', function () { describe('User', function () { const email = 'wombat@potato.net' + const defaultArgs = { analyticsId: '8055c676-bcc7-4e64-a66f-8069f9a0bd92' } it('allows the creation of a user', async function () { - await expect(User.create({ email })).to.be.fulfilled + await expect(User.create({ email, ...defaultArgs })).to.be.fulfilled await expect(User.findOne({ email }, { _id: 1 })).to.eventually.exist }) it('does not allow the creation of multiple users with the same email', async function () { - await expect(User.create({ email })).to.be.fulfilled - await expect(User.create({ email })).to.be.rejected + await expect(User.create({ email, ...defaultArgs })).to.be.fulfilled + await expect(User.create({ email, ...defaultArgs })).to.be.rejected await expect(User.countDocuments({ email })).to.eventually.equal(1) }) @@ -38,6 +39,7 @@ describe('mongoose', function () { }, ], }, + ...defaultArgs, }) ).to.be.fulfilled @@ -55,7 +57,10 @@ describe('mongoose', function () { this.skip() } - user = await User.create({ email: 'wombat@potato.net' }) + user = await User.create({ + email: 'wombat@potato.net', + analyticsId: '8055c676-bcc7-4e64-a66f-8069f9a0bd92', + }) }) it('allows the creation of a subscription', async function () { diff --git a/services/web/test/unit/src/Project/ProjectHelper.test.mjs b/services/web/test/unit/src/Project/ProjectHelper.test.mjs index a4a7ebcdba..78903a3eaf 100644 --- a/services/web/test/unit/src/Project/ProjectHelper.test.mjs +++ b/services/web/test/unit/src/Project/ProjectHelper.test.mjs @@ -51,9 +51,11 @@ describe('ProjectHelper', function () { ctx.SplitTestHandler = { promises: { - getAssignmentForUser: vi.fn().mockResolvedValue({ variant: 'default' }), + featureFlagEnabled: vi.fn().mockResolvedValue(false), }, } + ctx.req = {} + ctx.res = {} vi.doMock('mongodb-legacy', () => ({ default: { ObjectId }, @@ -159,7 +161,11 @@ describe('ProjectHelper', function () { describe('getAllowedImagesForUser', function () { it('marks alpha only images as not allowed when the user is anonymous', async function (ctx) { - const images = await ctx.ProjectHelper.getAllowedImagesForUser(null) + const images = await ctx.ProjectHelper.getAllowedImagesForUser( + ctx.req, + ctx.res, + null + ) const imageNames = _mapToAllowed(images) expect(imageNames).to.deep.equal([ { imageName: 'texlive-full:2018.1', allowed: true }, @@ -170,7 +176,11 @@ describe('ProjectHelper', function () { }) it('marks monthly labs images as not allowed when the user is anonymous', async function (ctx) { - const images = await ctx.ProjectHelper.getAllowedImagesForUser(null) + const images = await ctx.ProjectHelper.getAllowedImagesForUser( + ctx.req, + ctx.res, + null + ) const imageNames = _mapToAllowed(images) expect(imageNames).to.deep.equal([ { imageName: 'texlive-full:2018.1', allowed: true }, @@ -181,10 +191,12 @@ describe('ProjectHelper', function () { }) it('marks monthly labs images as allowed when the user is enrolled', async function (ctx) { - ctx.SplitTestHandler.promises.getAssignmentForUser.mockResolvedValue({ - variant: 'enabled', - }) - const images = await ctx.ProjectHelper.getAllowedImagesForUser(ctx.user) + ctx.SplitTestHandler.promises.featureFlagEnabled.mockResolvedValue(true) + const images = await ctx.ProjectHelper.getAllowedImagesForUser( + ctx.req, + ctx.res, + ctx.user + ) const imageNames = _mapToAllowed(images) expect(imageNames).to.deep.equal([ { imageName: 'texlive-full:2018.1', allowed: true }, @@ -195,7 +207,11 @@ describe('ProjectHelper', function () { }) it('marks alpha only images as not allowed when when the user is not admin', async function (ctx) { - const images = await ctx.ProjectHelper.getAllowedImagesForUser(ctx.user) + const images = await ctx.ProjectHelper.getAllowedImagesForUser( + ctx.req, + ctx.res, + ctx.user + ) const imageNames = _mapToAllowed(images) expect(imageNames).to.deep.equal([ { imageName: 'texlive-full:2018.1', allowed: true }, @@ -207,6 +223,8 @@ describe('ProjectHelper', function () { it('returns all images when the user is admin', async function (ctx) { const images = await ctx.ProjectHelper.getAllowedImagesForUser( + ctx.req, + ctx.res, ctx.adminUser ) const imageNames = _mapToAllowed(images) diff --git a/services/web/test/unit/src/Project/ProjectListController.test.mjs b/services/web/test/unit/src/Project/ProjectListController.test.mjs index 62adf8b7b4..1b02183459 100644 --- a/services/web/test/unit/src/Project/ProjectListController.test.mjs +++ b/services/web/test/unit/src/Project/ProjectListController.test.mjs @@ -137,7 +137,7 @@ describe('ProjectListController', function () { ctx.SplitTestHandler = { promises: { getAssignment: sinon.stub().resolves({ variant: 'default' }), - featureFlagEnabledForUser: sinon.stub().resolves(false), + featureFlagEnabled: sinon.stub().resolves(false), hasUserBeenAssignedToVariant: sinon.stub().resolves(false), }, } diff --git a/services/web/test/unit/src/SplitTests/SplitTestHandler.test.mjs b/services/web/test/unit/src/SplitTests/SplitTestHandler.test.mjs index ccd208b579..f4c144c746 100644 --- a/services/web/test/unit/src/SplitTests/SplitTestHandler.test.mjs +++ b/services/web/test/unit/src/SplitTests/SplitTestHandler.test.mjs @@ -139,6 +139,7 @@ describe('SplitTestHandler', function () { beforeEach(async function (ctx) { ctx.user = { _id: new ObjectId(), + analyticsId: 'analytics-id', splitTests: { 'active-test': [ { @@ -256,9 +257,101 @@ describe('SplitTestHandler', function () { }) }) + describe('mongo user variants', function () { + beforeEach(function (ctx) { + ctx.mongoUser = { + _id: new ObjectId(), + analyticsId: 'analytics-id', + splitTests: {}, + } + // a feature flag whose enabled variant covers 100% of users + ctx.cachedSplitTests.set('my-flag', { + name: 'my-flag', + versions: [ + { + active: true, + analyticsEnabled: true, + phase: 'release', + versionNumber: 1, + variants: [ + { + name: 'enabled', + rolloutPercent: 100, + rolloutStripes: [{ start: 0, end: 100 }], + }, + ], + }, + ], + }) + }) + + describe('getActiveAssignmentsForMongoUser', function () { + it('returns the assignments without re-fetching the user', async function (ctx) { + const assignments = + await ctx.SplitTestHandler.promises.getActiveAssignmentsForMongoUser( + ctx.mongoUser + ) + expect(assignments['active-test']).to.deep.equal({ + variantName: 'variant-1', + phase: 'release', + versionNumber: 2, + }) + expect(ctx.SplitTestUserGetter.promises.getUser).to.not.have.been.called + }) + + it('throws when _id is missing from the projection', async function (ctx) { + await expect( + ctx.SplitTestHandler.promises.getActiveAssignmentsForMongoUser({ + analyticsId: 'analytics-id', + }) + ).to.be.rejectedWith('bug: include db.users._id in projection') + }) + + it('throws when analyticsId is missing from the projection', async function (ctx) { + await expect( + ctx.SplitTestHandler.promises.getActiveAssignmentsForMongoUser({ + _id: new ObjectId(), + }) + ).to.be.rejectedWith('bug: include db.users.analyticsId in projection') + }) + }) + + describe('getAssignmentForMongoUser', function () { + it('returns the assignment without re-fetching the user', async function (ctx) { + const assignment = + await ctx.SplitTestHandler.promises.getAssignmentForMongoUser( + ctx.mongoUser, + 'my-flag' + ) + expect(assignment.variant).to.equal('enabled') + expect(ctx.SplitTestUserGetter.promises.getUser).to.not.have.been.called + }) + }) + + describe('featureFlagEnabledForMongoUser', function () { + it('returns true when the assigned variant is enabled', async function (ctx) { + const enabled = + await ctx.SplitTestHandler.promises.featureFlagEnabledForMongoUser( + ctx.mongoUser, + 'my-flag' + ) + expect(enabled).to.be.true + }) + + it('returns false when the assigned variant is not enabled', async function (ctx) { + const enabled = + await ctx.SplitTestHandler.promises.featureFlagEnabledForMongoUser( + ctx.mongoUser, + 'active-test' + ) + expect(enabled).to.be.false + }) + }) + }) + describe('with a user without assignments', function () { beforeEach(async function (ctx) { - ctx.user = { _id: new ObjectId() } + ctx.user = { _id: new ObjectId(), analyticsId: 'analytics-id' } ctx.SplitTestUserGetter.promises.getUser.resolves(ctx.user) ctx.assignments = await ctx.SplitTestHandler.promises.getActiveAssignmentsForUser( @@ -421,6 +514,35 @@ describe('SplitTestHandler', function () { }) }) + describe('getAssignment query overrides', function () { + beforeEach(function (ctx) { + ctx.AnalyticsManager.getIdsFromSession.returns({ + userId: 'abc123abc123', + }) + // 'active-test' would compute to 'variant-1'; override it to 'default' + ctx.req.query = { 'active-test': 'default' } + }) + + it('applies a query-string override by default', async function (ctx) { + const { variant } = await ctx.SplitTestHandler.promises.getAssignment( + ctx.req, + ctx.res, + 'active-test' + ) + expect(variant).to.equal('default') + }) + + it('ignores query-string overrides when ignoreOverrides is set', async function (ctx) { + const { variant } = await ctx.SplitTestHandler.promises.getAssignment( + ctx.req, + ctx.res, + 'active-test', + { ignoreOverrides: true } + ) + expect(variant).to.equal('variant-1') + }) + }) + describe('variant user limits', function () { beforeEach(function (ctx) { ctx.AnalyticsManager.getIdsFromSession.returns({ diff --git a/services/web/test/unit/src/Subscription/FeaturesUpdater.test.mjs b/services/web/test/unit/src/Subscription/FeaturesUpdater.test.mjs index 596b40b186..a54e4290aa 100644 --- a/services/web/test/unit/src/Subscription/FeaturesUpdater.test.mjs +++ b/services/web/test/unit/src/Subscription/FeaturesUpdater.test.mjs @@ -163,7 +163,7 @@ describe('FeaturesUpdater', function () { ctx.SplitTestHandler = { promises: { - featureFlagEnabledForUser: sinon.stub().resolves(false), + featureFlagEnabledForMongoUser: sinon.stub().resolves(false), }, } diff --git a/services/web/test/unit/src/infrastructure/AiFeatureUsageRateLimiter.test.mjs b/services/web/test/unit/src/infrastructure/AiFeatureUsageRateLimiter.test.mjs index 108d381cc0..fc489eb62b 100644 --- a/services/web/test/unit/src/infrastructure/AiFeatureUsageRateLimiter.test.mjs +++ b/services/web/test/unit/src/infrastructure/AiFeatureUsageRateLimiter.test.mjs @@ -78,7 +78,7 @@ describe('AiFeatureUsageRateLimiter', function () { ctx.SplitTestHandler = { promises: { - featureFlagEnabledForUser: sinon.stub().resolves(true), + featureFlagEnabledForMongoUser: sinon.stub().resolves(true), }, } diff --git a/services/web/test/unit/src/infrastructure/WorkbenchRateLimiter.sequential.test.mjs b/services/web/test/unit/src/infrastructure/WorkbenchRateLimiter.sequential.test.mjs index a7569734da..5e9e7de273 100644 --- a/services/web/test/unit/src/infrastructure/WorkbenchRateLimiter.sequential.test.mjs +++ b/services/web/test/unit/src/infrastructure/WorkbenchRateLimiter.sequential.test.mjs @@ -62,7 +62,7 @@ describe('WorkbenchRateLimiter', function () { ctx.SplitTestHandler = { promises: { getAssignmentForUser: sinon.stub(), - featureFlagEnabledForUser: sinon.stub().resolves(true), + featureFlagEnabledForMongoUser: sinon.stub().resolves(true), }, } ctx.SplitTestHandler.promises.getAssignmentForUser