diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs b/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs index 93a041bb13..be32cc5620 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs @@ -9,6 +9,7 @@ import SplitTestCache from './SplitTestCache.mjs' import { SplitTest } from '../../models/SplitTest.mjs' import UserAnalyticsDataCache from '../Analytics/UserAnalyticsDataCache.mjs' import Features from '../../infrastructure/Features.mjs' +import Modules from '../../infrastructure/Modules.mjs' import SplitTestUtils from './SplitTestUtils.mjs' import Settings from '@overleaf/settings' import SessionManager from '../Authentication/SessionManager.mjs' @@ -923,7 +924,10 @@ async function _loadSplitTestInfoInLocals(locals, splitTestName, session) { badgeInfo: splitTest.badgeInfo?.[phase], } - if (phase === 'labs') { + if ( + phase === 'labs' && + (await _userMeetsLabsRequirements(splitTest, session)) + ) { const variant = currentVersion.variants?.[0] info.labsDetails = { title: splitTest.labsTitle || '', @@ -959,6 +963,38 @@ async function _loadSplitTestInfoInLocals(locals, splitTestName, session) { } } +/** + * Whether the user meets the requirements a labs experiment declares, e.g. + * having premium compiles available. + * + * The check lives in the labs module, which owns both the list of requirements + * and the user lookup they need, so this is a no-op when that module is not + * loaded. Experiments the user does not qualify for are left out of the labs + * details, and so never reach the frontend. + * + * @param {object} splitTest a labs-phase split test + * @param {object} session the request session + * @returns {Promise} + */ +async function _userMeetsLabsRequirements(splitTest, session) { + try { + const results = await Modules.promises.hooks.fire( + 'userMeetsLabsExperimentRequirements', + session, + splitTest + ) + return results.every(result => result !== false) + } catch (error) { + // leave the experiment out rather than advertise one the user may not be + // able to opt into. The opt-in endpoint enforces the same requirements. + logger.warn( + { err: error, splitTestName: splitTest.name }, + 'failed to check labs experiment requirements' + ) + return false + } +} + function _getNonSaasAssignment(splitTestName) { if (Settings.splitTestOverrides?.[splitTestName]) { return { diff --git a/services/web/app/src/Features/SplitTests/SplitTestManager.mjs b/services/web/app/src/Features/SplitTests/SplitTestManager.mjs index e4149bdb1c..fa2f17830a 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestManager.mjs +++ b/services/web/app/src/Features/SplitTests/SplitTestManager.mjs @@ -98,6 +98,9 @@ async function createSplitTest( labsDescription: labsInfo.description, labsIcon: labsInfo.icon, labsSuccessNotification: labsInfo.successNotification, + labsRequirePremiumCompiles: labsInfo.requirePremiumCompiles + ? true + : undefined, versions: [ { versionNumber: 1, @@ -177,6 +180,11 @@ async function updateSplitTestInfo(name, info, labsInfo) { splitTest.labsDescription = labsInfo.description splitTest.labsIcon = labsInfo.icon splitTest.labsSuccessNotification = labsInfo.successNotification + // unset rather than stored as false, so turning the requirement back off + // leaves the split test as it was before it was ever set + splitTest.labsRequirePremiumCompiles = labsInfo.requirePremiumCompiles + ? true + : undefined } return _saveSplitTest(splitTest) } diff --git a/services/web/app/src/Features/Subscription/FeaturesHelper.mjs b/services/web/app/src/Features/Subscription/FeaturesHelper.mjs index bd1fb7e8b2..006dbe296b 100644 --- a/services/web/app/src/Features/Subscription/FeaturesHelper.mjs +++ b/services/web/app/src/Features/Subscription/FeaturesHelper.mjs @@ -111,6 +111,18 @@ function compareFeatures(currentFeatures, expectedFeatures) { return mismatchReasons } +// Compile groups that count as having premium compiles available. +const PREMIUM_COMPILE_GROUPS = ['alpha', 'priority'] + +/** + * Whether a feature set (or a resolved set of compile limits) has premium + * compiles available. Takes anything with a `compileGroup`, so it works for + * both a user's features and the limits resolved for a compile. + */ +function hasPremiumCompiles(features) { + return PREMIUM_COMPILE_GROUPS.includes(features?.compileGroup) +} + function getMatchedFeatureSet(features) { for (const [name, featureSet] of Object.entries(Settings.features)) { if (_.isEqual(features, featureSet)) { @@ -125,5 +137,6 @@ export default { computeFeatureSet, isFeatureSetBetter, compareFeatures, + hasPremiumCompiles, getMatchedFeatureSet, } diff --git a/services/web/app/src/models/SplitTest.mjs b/services/web/app/src/models/SplitTest.mjs index c69d7d6368..fae73244b3 100644 --- a/services/web/app/src/models/SplitTest.mjs +++ b/services/web/app/src/models/SplitTest.mjs @@ -213,6 +213,14 @@ export const SplitTestSchema = new Schema( type: LabsSuccessNotificationSchema, required: false, }, + // When set, only users with a premium compile group can see this + // experiment on the Labs page or opt into it. Left unset rather than + // defaulted to false, so split tests that never use it don't carry a + // labs-only field. + labsRequirePremiumCompiles: { + type: Boolean, + required: false, + }, }, { minimize: false } ) diff --git a/services/web/test/unit/src/SplitTests/SplitTestHandler.test.mjs b/services/web/test/unit/src/SplitTests/SplitTestHandler.test.mjs index a91eb9bf6c..7ff7e6f3e7 100644 --- a/services/web/test/unit/src/SplitTests/SplitTestHandler.test.mjs +++ b/services/web/test/unit/src/SplitTests/SplitTestHandler.test.mjs @@ -68,6 +68,14 @@ describe('SplitTestHandler', function () { ctx.SessionManager = { isUserLoggedIn: sinon.stub().returns(false), } + // no module gates labs experiments unless a test says otherwise + ctx.Modules = { + promises: { + hooks: { + fire: sinon.stub().resolves([]), + }, + }, + } Features = { hasFeature: vi.fn().mockReturnValue(true), @@ -77,6 +85,10 @@ describe('SplitTestHandler', function () { default: Features, })) + vi.doMock('../../../../app/src/infrastructure/Modules', () => ({ + default: ctx.Modules, + })) + vi.doMock('../../../../app/src/Features/User/UserGetter', () => ({ default: ctx.UserGetter, })) @@ -1084,6 +1096,108 @@ describe('SplitTestHandler', function () { sinon.match(info => info.labsDetails === undefined) ) }) + + describe('experiment requirements', function () { + beforeEach(function (ctx) { + ctx.labsSplitTest = { + name: 'labs-requirements-test', + labsTitle: 'Needs premium compiles', + versions: [ + { + active: true, + analyticsEnabled: true, + phase: 'labs', + versionNumber: 1, + createdAt: new Date('2024-06-15T12:00:00.000Z'), + variants: [ + { + name: 'variant-1', + rolloutPercent: 100, + rolloutStripes: [{ start: 0, end: 100 }], + }, + ], + }, + ], + } + ctx.cachedSplitTests.set('labs-requirements-test', ctx.labsSplitTest) + }) + + it('asks the labs module whether the user meets the requirements', async function (ctx) { + await ctx.SplitTestHandler.promises.getAssignment( + ctx.req, + ctx.res, + 'labs-requirements-test' + ) + + expect(ctx.Modules.promises.hooks.fire).to.have.been.calledWith( + 'userMeetsLabsExperimentRequirements', + ctx.req.session, + ctx.labsSplitTest + ) + }) + + it('does not set labsDetails when the user does not meet them', async function (ctx) { + ctx.Modules.promises.hooks.fire.resolves([false]) + + await ctx.SplitTestHandler.promises.getAssignment( + ctx.req, + ctx.res, + 'labs-requirements-test' + ) + + expect(ctx.LocalsHelper.setSplitTestInfo).to.have.been.calledWith( + ctx.res.locals, + 'labs-requirements-test', + sinon.match(info => info.labsDetails === undefined) + ) + }) + + it('sets labsDetails when the user meets them', async function (ctx) { + ctx.Modules.promises.hooks.fire.resolves([true]) + + await ctx.SplitTestHandler.promises.getAssignment( + ctx.req, + ctx.res, + 'labs-requirements-test' + ) + + expect(ctx.LocalsHelper.setSplitTestInfo).to.have.been.calledWith( + ctx.res.locals, + 'labs-requirements-test', + sinon.match({ + labsDetails: sinon.match({ title: 'Needs premium compiles' }), + }) + ) + }) + + // fail closed rather than advertise an experiment the opt-in endpoint + // would then refuse + it('does not set labsDetails when the check fails', async function (ctx) { + ctx.Modules.promises.hooks.fire.rejects(new Error('mongo is down')) + + await ctx.SplitTestHandler.promises.getAssignment( + ctx.req, + ctx.res, + 'labs-requirements-test' + ) + + expect(ctx.LocalsHelper.setSplitTestInfo).to.have.been.calledWith( + ctx.res.locals, + 'labs-requirements-test', + sinon.match(info => info.labsDetails === undefined) + ) + }) + + it('does not ask about a release-phase split test', async function (ctx) { + await ctx.SplitTestHandler.promises.getAssignment( + ctx.req, + ctx.res, + 'active-test' + ) + + expect(ctx.Modules.promises.hooks.fire).to.not.have.been.called + }) + }) }) describe('labs phase assignment (gradual rollout)', function () { diff --git a/services/web/test/unit/src/Subscription/FeaturesHelper.test.mjs b/services/web/test/unit/src/Subscription/FeaturesHelper.test.mjs index 24e776f57a..d3087d04af 100644 --- a/services/web/test/unit/src/Subscription/FeaturesHelper.test.mjs +++ b/services/web/test/unit/src/Subscription/FeaturesHelper.test.mjs @@ -150,4 +150,26 @@ describe('FeaturesHelper', function () { expect(result3).to.be.true }) }) + + describe('hasPremiumCompiles', function () { + it('should be true for premium compile groups', function (ctx) { + expect( + ctx.FeaturesHelper.hasPremiumCompiles({ compileGroup: 'priority' }) + ).to.be.true + expect(ctx.FeaturesHelper.hasPremiumCompiles({ compileGroup: 'alpha' })) + .to.be.true + }) + + it('should be false for the standard compile group', function (ctx) { + expect( + ctx.FeaturesHelper.hasPremiumCompiles({ compileGroup: 'standard' }) + ).to.be.false + }) + + it('should be false when the compile group is missing', function (ctx) { + expect(ctx.FeaturesHelper.hasPremiumCompiles({})).to.be.false + expect(ctx.FeaturesHelper.hasPremiumCompiles(undefined)).to.be.false + expect(ctx.FeaturesHelper.hasPremiumCompiles(null)).to.be.false + }) + }) })