diff --git a/services/web/app/src/Features/Project/ProjectController.mjs b/services/web/app/src/Features/Project/ProjectController.mjs index 08481d9e4a..9afd8fffb5 100644 --- a/services/web/app/src/Features/Project/ProjectController.mjs +++ b/services/web/app/src/Features/Project/ProjectController.mjs @@ -395,6 +395,7 @@ const _ProjectController = { 'track-pdf-download', !anonymous && 'writefull-oauth-promotion', 'hotjar', + 'hotjar-editor-onboarding', 'editor-redesign', 'overleaf-assist-bundle', 'word-count-client', @@ -453,17 +454,10 @@ const _ProjectController = { ), }) ) - const splitTestAssignments = {} try { const responses = await pProps({ userValues: userId ? getUserValues(userId) : defaultUserValues(), - splitTestAssignments: Promise.all( - splitTests.map(async splitTest => { - splitTestAssignments[splitTest] = - await SplitTestHandler.promises.getAssignment(req, res, splitTest) - }) - ), project: ProjectGetter.promises.getProject(projectId, { name: 1, lastUpdated: 1, @@ -503,8 +497,57 @@ const _ProjectController = { subscription, isTokenMember, isInvitedMember, + affiliations, } = userValues + let inEnterpriseCommons = false + for (const affiliation of affiliations || []) { + inEnterpriseCommons = + inEnterpriseCommons || affiliation.institution?.enterpriseCommons + } + + const getSplitTestAssignment = async splitTest => { + if (splitTest === 'hotjar-editor-onboarding') { + const sevenDaysAgo = new Date(Date.now() - 7 * 24 * 60 * 60 * 1000) + const userRegisteredMoreThan7DaysAgo = + user.signUpDate && user.signUpDate < sevenDaysAgo + + const isExcluded = + user.betaProgram || + inEnterpriseCommons || + userIsMemberOfGroupSubscription || + userRegisteredMoreThan7DaysAgo + + if (!isExcluded) { + return await SplitTestHandler.promises.getAssignment( + req, + res, + splitTest + ) + } else { + return { + variant: 'default', + analytics: { + segmentation: {}, + }, + } + } + } else { + return await SplitTestHandler.promises.getAssignment( + req, + res, + splitTest + ) + } + } + const splitTestAssignments = {} + await Promise.all( + splitTests.map(async splitTest => { + splitTestAssignments[splitTest] = + await getSplitTestAssignment(splitTest) + }) + ) + const brandVariation = project?.brandVariationId ? await BrandVariationsHandler.promises.getBrandVariationById( project.brandVariationId @@ -853,7 +896,9 @@ const _ProjectController = { otMigrationStage: project.overleaf?.history?.otMigrationStage ?? 0, projectTags, isSaas: Features.hasFeature('saas'), - shouldLoadHotjar: splitTestAssignments.hotjar?.variant === 'enabled', + shouldLoadHotjar: + splitTestAssignments['hotjar-editor-onboarding']?.variant === + 'enabled', isOverleafAssistBundleEnabled, customerIoEnabled, addonPrices, @@ -1195,6 +1240,7 @@ const defaultUserValues = () => ({ learnedWords: [], projectTags: [], userHasInstitutionLicence: false, + affiliations: [], subscription: undefined, isTokenMember: false, isInvitedMember: false, diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index 4bd904fdd4..f475083ae0 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -15,6 +15,7 @@ const SessionManager = require('../Authentication/SessionManager') const logger = require('@overleaf/logger') const SplitTestSessionHandler = require('./SplitTestSessionHandler') const SplitTestUserGetter = require('./SplitTestUserGetter') +const SplitTestManager = require('./SplitTestManager') /** * @import { Assignment } from "./types" @@ -513,9 +514,37 @@ async function _getAssignmentMetadata(analyticsId, user, splitTest) { const userId = user?._id.toString() const percentile = getPercentile(analyticsId || userId, splitTest.name, phase) - const selectedVariantName = + let selectedVariantName = _getVariantFromPercentile(currentVersion.variants, percentile) || DEFAULT_VARIANT + + // Some variants may have a limit on the number of users that can be assigned + if (selectedVariantName !== DEFAULT_VARIANT) { + const selectedVariant = currentVersion.variants.find( + variant => variant.name === selectedVariantName + ) + const userLimit = selectedVariant?.userLimit + + if (userLimit && typeof userLimit === 'number') { + const userAssignments = user?.splitTests?.[splitTest.name] + const existingAssignment = Array.isArray(userAssignments) + ? userAssignments.find( + assignment => + assignment.phase === phase && + assignment.variantName === selectedVariantName + ) + : null + + if (!existingAssignment) { + const currentCount = + selectedVariant.userCount ?? Number.MAX_SAFE_INTEGER + + if (currentCount >= userLimit) { + selectedVariantName = DEFAULT_VARIANT + } + } + } + } return { activeForUser: true, selectedVariantName, @@ -587,15 +616,117 @@ async function _recordAssignment({ const assignmentLog = assignedSplitTests[splitTestName] || [] const existingAssignment = _.find(assignmentLog, { versionNumber }) if (!existingAssignment) { - await UserUpdater.promises.updateUser(userId, { - $addToSet: { - [`splitTests.${splitTestName}`]: persistedAssignment, - }, - }) + const shouldIncrementCounter = await _shouldIncrementVariantCounter( + splitTestName, + variantName, + phase, + user + ) + + const updatePromises = [ + UserUpdater.promises.updateUser(userId, { + $addToSet: { + [`splitTests.${splitTestName}`]: persistedAssignment, + }, + }), + ] + + if (shouldIncrementCounter) { + updatePromises.push( + _incrementVariantCounter(splitTestName, variantName, versionNumber) + ) + } + + await Promise.all(updatePromises) } } } +/** + * Check if the variant counter should be incremented for this assignment + * Only increment for tests with user limits and when user hasn't been assigned to this variant in this phase before + * @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 + * @returns {Promise} Whether the counter should be incremented + */ +async function _shouldIncrementVariantCounter( + splitTestName, + variantName, + phase, + user +) { + if (variantName === DEFAULT_VARIANT) { + return false + } + + const splitTest = await _getSplitTest(splitTestName) + if (!splitTest) { + return false + } + + const currentVersion = SplitTestUtils.getCurrentVersion(splitTest) + if (!currentVersion) { + return false + } + + const variant = currentVersion.variants.find(v => v.name === variantName) + const hasUserLimit = + variant?.userLimit && typeof variant.userLimit === 'number' + + if (!hasUserLimit) { + return false + } + + const userAssignments = user?.splitTests?.[splitTest.name] + const existingPhaseAssignment = Array.isArray(userAssignments) + ? userAssignments.find( + assignment => + assignment.phase === phase && assignment.variantName === variantName + ) + : null + + // Only increment if user hasn't been assigned to this variant in this phase before + return !existingPhaseAssignment +} + +/** + * Increment the user counter for a specific variant + * @param {string} splitTestName - The name of the split test + * @param {string} variantName - The name of the variant + * @param {number} versionNumber - The version to update + */ +async function _incrementVariantCounter( + splitTestName, + variantName, + versionNumber +) { + try { + await SplitTest.updateOne( + { + name: splitTestName, + 'versions.versionNumber': versionNumber, + 'versions.variants.name': variantName, + }, + { + $inc: { + 'versions.$.variants.$[variant].userCount': 1, + }, + }, + { + arrayFilters: [{ 'variant.name': variantName }], + } + ).exec() + await SplitTestManager.clearCache() + } catch (error) { + logger.error( + { err: error, splitTestName, variantName, versionNumber }, + 'Failed to increment variant counter' + ) + } +} + function _makeAssignment({ variant, currentVersion, diff --git a/services/web/app/src/Features/SplitTests/SplitTestManager.js b/services/web/app/src/Features/SplitTests/SplitTestManager.js index 16120f56b0..d224a34e2d 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestManager.js +++ b/services/web/app/src/Features/SplitTests/SplitTestManager.js @@ -90,9 +90,10 @@ async function createSplitTest( configuration.analyticsEnabled ) for (const variant of configuration.variants) { - stripedVariants.push({ + const variantData = { name: (variant.name || '').trim(), rolloutPercent: variant.rolloutPercent, + userLimit: variant.userLimit, rolloutStripes: variant.rolloutPercent > 0 ? [ @@ -102,7 +103,11 @@ async function createSplitTest( }, ] : [], - }) + } + if (variant.userLimit && typeof variant.userLimit === 'number') { + variantData.userCount = 0 + } + stripedVariants.push(variantData) stripeStart += variant.rolloutPercent } const splitTest = new SplitTest({ @@ -258,6 +263,9 @@ async function switchToNextPhase({ name, comment }, userId) { for (const variant of lastVersionCopy.variants) { variant.rolloutPercent = 0 variant.rolloutStripes = [] + if (variant.userCount) { + variant.userCount = 0 + } } lastVersionCopy.author = userId lastVersionCopy.comment = comment @@ -307,6 +315,29 @@ async function revertToPreviousVersion( previousVersionCopy.createdAt = new Date() previousVersionCopy.author = userId previousVersionCopy.comment = comment + + // restore user count from most recent version of this phase + const mostRecentVersionOfTargetPhase = splitTest.versions.findLast( + v => v.phase === previousVersion.phase + ) + + if (mostRecentVersionOfTargetPhase) { + for (const variant of previousVersionCopy.variants) { + const correspondingVariant = mostRecentVersionOfTargetPhase.variants.find( + v => v.name === variant.name + ) + if (correspondingVariant?.userCount) { + variant.userCount = correspondingVariant.userCount + } + } + } else { + for (const variant of previousVersionCopy.variants) { + if (variant.userCount) { + variant.userCount = 0 + } + } + } + splitTest.versions.push(previousVersionCopy) return _saveSplitTest(splitTest) } @@ -362,6 +393,24 @@ function _checkNewVariantsConfiguration( )} cannot be decreased: revert to a previous configuration instead` ) } + if (variant.userLimit !== undefined) { + // Existing variant has a user limit - can only increase it + if ( + newVariantConfiguration.userLimit !== undefined && + newVariantConfiguration.userLimit < variant.userLimit + ) { + throw new OError( + `User limit for variant '${variant.name}' cannot be decreased: revert to a previous configuration instead` + ) + } + } else { + // Existing variant has no user limit - cannot add one + if (newVariantConfiguration.userLimit !== undefined) { + throw new OError( + `User limit cannot be added to variant '${variant.name}' after creation: user limits can only be set when the split test is created` + ) + } + } } } @@ -377,7 +426,7 @@ function _updateVariantsWithNewConfiguration( } const variant = _.find(variantsCopy, { name: newVariantConfig.name }) if (!variant) { - variantsCopy.push({ + const newVariant = { name: newVariantConfig.name, rolloutPercent: newVariantConfig.rolloutPercent, rolloutStripes: [ @@ -386,7 +435,15 @@ function _updateVariantsWithNewConfiguration( end: totalRolloutPercentage + newVariantConfig.rolloutPercent, }, ], - }) + } + if ( + newVariantConfig.userLimit && + typeof newVariantConfig.userLimit === 'number' + ) { + newVariant.userLimit = newVariantConfig.userLimit + newVariant.userCount = 0 + } + variantsCopy.push(newVariant) totalRolloutPercentage += newVariantConfig.rolloutPercent } else if (variant.rolloutPercent < newVariantConfig.rolloutPercent) { const newStripeSize = @@ -398,6 +455,12 @@ function _updateVariantsWithNewConfiguration( }) totalRolloutPercentage += newStripeSize } + if (newVariantConfig.userLimit >= variant?.userLimit) { + variant.userLimit = newVariantConfig.userLimit + } + if (variant?.userLimit && !variant.userCount) { + variant.userCount = 0 + } } return variantsCopy } diff --git a/services/web/app/src/models/SplitTest.js b/services/web/app/src/models/SplitTest.js index 2168db4750..c605693bfa 100644 --- a/services/web/app/src/models/SplitTest.js +++ b/services/web/app/src/models/SplitTest.js @@ -60,6 +60,16 @@ const VariantSchema = new Schema( end: RolloutPercentType, }, ], + userLimit: { + type: Number, + min: [0, 'User limit must be 0 or greater, got {VALUE}'], + required: false, + }, + userCount: { + type: Number, + min: [0, 'User count must be 0 or greater, got {VALUE}'], + required: false, + }, }, { _id: false } ) diff --git a/services/web/frontend/js/infrastructure/hotjar.ts b/services/web/frontend/js/infrastructure/hotjar.ts index ea660e7faf..de8f69c981 100644 --- a/services/web/frontend/js/infrastructure/hotjar.ts +++ b/services/web/frontend/js/infrastructure/hotjar.ts @@ -2,17 +2,34 @@ import getMeta from '@/utils/meta' import { debugConsole } from '@/utils/debugging' import { initializeHotjar } from '@/infrastructure/hotjar-snippet' import { createTrackingLoader } from '@/infrastructure/tracking-loader' +import grammarlyExtensionPresent from '@/shared/utils/grammarly' const { hotjarId, hotjarVersion } = getMeta('ol-ExposedSettings') const shouldLoadHotjar = getMeta('ol-shouldLoadHotjar') -if (hotjarId && hotjarVersion && shouldLoadHotjar) { +function attemptHotjarLoad() { + if (!hotjarId || !hotjarVersion) { + return + } + if (!/^\d+$/.test(hotjarId) || !/^\d+$/.test(hotjarVersion)) { debugConsole.error('Invalid Hotjar id or version') - } else { - createTrackingLoader( - () => initializeHotjar(hotjarId, hotjarVersion), - 'Hotjar' - ) + return } + + if (grammarlyExtensionPresent()) { + return + } + + createTrackingLoader( + () => initializeHotjar(hotjarId, hotjarVersion), + 'Hotjar' + ) +} + +if (hotjarId && hotjarVersion && shouldLoadHotjar) { + document.addEventListener('DOMContentLoaded', () => { + // add delay to allow extensions to inject shadow DOM + setTimeout(attemptHotjarLoad, 1000) + }) } diff --git a/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js b/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js index a682f0c954..aafed5fdc6 100644 --- a/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js +++ b/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js @@ -372,6 +372,101 @@ describe('SplitTestHandler', function () { ) }) }) + + describe('variant user limits', function () { + beforeEach(function () { + this.AnalyticsManager.getIdsFromSession.returns({ + userId: 'abc123abc123', + }) + this.SplitTestUserGetter.promises.getUser.resolves({ + _id: new ObjectId('abc123abc123abc123abc123'), + splitTests: {}, + }) + }) + + it('should assign to variant when under limit', async function () { + this.cachedSplitTests.set( + 'active-test', + makeSplitTest('active-test', { userLimit: 100, userCount: 50 }) + ) + + const assignment = await this.SplitTestHandler.promises.getAssignment( + this.req, + this.res, + 'active-test' + ) + + expect(assignment.variant).to.equal('variant-1') + }) + + it('should assign to default when limit reached', async function () { + this.cachedSplitTests.set( + 'active-test', + makeSplitTest('active-test', { userLimit: 100, userCount: 100 }) + ) + + const assignment = await this.SplitTestHandler.promises.getAssignment( + this.req, + this.res, + 'active-test' + ) + + expect(assignment.variant).to.equal('default') + }) + + it('should not apply limits when no limit configured', async function () { + const assignment = await this.SplitTestHandler.promises.getAssignment( + this.req, + this.res, + 'active-test' + ) + + expect(assignment.variant).to.equal('variant-1') + }) + + it('should allow already assigned users even when limit reached', async function () { + this.cachedSplitTests.set( + 'active-test', + makeSplitTest('active-test', { userLimit: 100, userCount: 100 }) + ) + this.SplitTestUserGetter.promises.getUser.resolves({ + _id: new ObjectId('abc123abc123abc123abc123'), + splitTests: { + 'active-test': [ + { + variantName: 'variant-1', + versionNumber: 1, + assignedAt: new Date(), + phase: 'release', + }, + ], + }, + }) + + const assignment = await this.SplitTestHandler.promises.getAssignment( + this.req, + this.res, + 'active-test' + ) + + expect(assignment.variant).to.equal('variant-1') + }) + + it('should assign to default if userCount is undefined', async function () { + this.cachedSplitTests.set( + 'active-test', + makeSplitTest('active-test', { userLimit: 100, userCount: undefined }) + ) + + const assignment = await this.SplitTestHandler.promises.getAssignment( + this.req, + this.res, + 'active-test' + ) + + expect(assignment.variant).to.equal('default') + }) + }) }) function makeSplitTest( @@ -381,8 +476,24 @@ function makeSplitTest( analyticsEnabled = active, phase = 'release', versionNumber = 1, + userLimit = undefined, + userCount = undefined, } = {} ) { + const variant = { + name: 'variant-1', + rolloutPercent: 100, + rolloutStripes: [{ start: 0, end: 100 }], + } + + if (userLimit !== undefined) { + variant.userLimit = userLimit + } + + if (userCount !== undefined) { + variant.userCount = userCount + } + return { name, versions: [ @@ -391,13 +502,7 @@ function makeSplitTest( analyticsEnabled, phase, versionNumber, - variants: [ - { - name: 'variant-1', - rolloutPercent: 100, - rolloutStripes: [{ start: 0, end: 100 }], - }, - ], + variants: [variant], }, ], }