diff --git a/services/web/app/src/Features/SplitTests/SplitTestCache.js b/services/web/app/src/Features/SplitTests/SplitTestCache.js index 5733bfe66a..fcf720504e 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestCache.js +++ b/services/web/app/src/Features/SplitTests/SplitTestCache.js @@ -1,5 +1,4 @@ const SplitTestManager = require('./SplitTestManager') -const { SplitTest } = require('../../models/SplitTest') const { CacheLoader } = require('cache-flow') class SplitTestCache extends CacheLoader { @@ -10,18 +9,19 @@ class SplitTestCache extends CacheLoader { } async load(name) { - return await SplitTestManager.getSplitTest({ + const splitTest = await SplitTestManager.getSplitTest({ name, archived: { $ne: true }, }) + return splitTest?.toObject() } serialize(value) { - return value ? value.toObject() : undefined + return value } deserialize(value) { - return new SplitTest(value) + return value } } diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index ef226b161d..ac2f3a8204 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -10,6 +10,7 @@ const { SplitTest } = require('../../models/SplitTest') const UserAnalyticsIdCache = require('../Analytics/UserAnalyticsIdCache') const { getAnalyticsIdFromMongoUser } = require('../Analytics/AnalyticsHelper') const Features = require('../../infrastructure/Features') +const SplitTestUtils = require('./SplitTestUtils') const DEFAULT_VARIANT = 'default' const ALPHA_PHASE = 'alpha' @@ -191,7 +192,7 @@ async function getActiveAssignmentsForUser(userId) { */ async function _getVariantNames(splitTestName) { const splitTest = await SplitTestCache.get(splitTestName) - const currentVersion = splitTest?.getCurrentVersion() + const currentVersion = SplitTestUtils.getCurrentVersion(splitTest) if (currentVersion?.active) { return currentVersion.variants.map(v => v.name).concat([DEFAULT_VARIANT]) } else { @@ -208,7 +209,7 @@ async function _getAssignment( } const splitTest = await SplitTestCache.get(splitTestName) - const currentVersion = splitTest?.getCurrentVersion() + const currentVersion = SplitTestUtils.getCurrentVersion(splitTest) if (!currentVersion?.active) { return DEFAULT_ASSIGNMENT } @@ -251,7 +252,7 @@ async function _getAssignment( } async function _getAssignmentMetadata(analyticsId, user, splitTest) { - const currentVersion = splitTest.getCurrentVersion() + const currentVersion = SplitTestUtils.getCurrentVersion(splitTest) const phase = currentVersion.phase if ( (phase === ALPHA_PHASE && !user?.alphaProgram) || @@ -398,10 +399,10 @@ async function _getUser(id) { async function _loadSplitTestInfoInLocals(locals, splitTestName) { const splitTest = await SplitTestCache.get(splitTestName) if (splitTest) { - const phase = splitTest.getCurrentVersion().phase + const phase = SplitTestUtils.getCurrentVersion(splitTest).phase LocalsHelper.setSplitTestInfo(locals, splitTestName, { phase, - badgeInfo: splitTest.toObject().badgeInfo?.[phase], + badgeInfo: splitTest.badgeInfo?.[phase], }) } } diff --git a/services/web/app/src/Features/SplitTests/SplitTestManager.js b/services/web/app/src/Features/SplitTests/SplitTestManager.js index 29d7e80607..e963dcf673 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestManager.js +++ b/services/web/app/src/Features/SplitTests/SplitTestManager.js @@ -1,4 +1,5 @@ const { SplitTest } = require('../../models/SplitTest') +const SplitTestUtils = require('./SplitTestUtils') const OError = require('@overleaf/o-error') const _ = require('lodash') @@ -89,7 +90,7 @@ async function updateSplitTestConfig(name, configuration) { if (splitTest.archived) { throw new OError('Cannot update an archived split test', { name }) } - const lastVersion = splitTest.getCurrentVersion().toObject() + const lastVersion = SplitTestUtils.getCurrentVersion(splitTest).toObject() if (configuration.phase !== lastVersion.phase) { throw new OError( `Cannot update with different phase - use /switch-to-next-phase endpoint instead` @@ -145,7 +146,7 @@ async function switchToNextPhase(name) { name, }) } - const lastVersionCopy = splitTest.getCurrentVersion().toObject() + const lastVersionCopy = SplitTestUtils.getCurrentVersion(splitTest).toObject() lastVersionCopy.versionNumber++ if (lastVersionCopy.phase === ALPHA_PHASE) { lastVersionCopy.phase = BETA_PHASE @@ -189,13 +190,13 @@ async function revertToPreviousVersion(name, versionNumber) { `Cannot revert split test with ID '${name}' to previous version: split test must have at least 2 versions` ) } - const previousVersion = splitTest.getVersion(versionNumber) + const previousVersion = SplitTestUtils.getVersion(splitTest, versionNumber) if (!previousVersion) { throw new OError( `Cannot revert split test with ID '${name}' to version number ${versionNumber}: version not found` ) } - const lastVersion = splitTest.getCurrentVersion() + const lastVersion = SplitTestUtils.getCurrentVersion(splitTest) if ( lastVersion.phase === RELEASE_PHASE && previousVersion.phase !== RELEASE_PHASE @@ -217,7 +218,8 @@ async function archive(name) { throw new OError(`Split test with ID '${name}' is already archived`) } splitTest.archived = true - const previousVersionCopy = splitTest.getCurrentVersion().toObject() + const previousVersionCopy = + SplitTestUtils.getCurrentVersion(splitTest).toObject() previousVersionCopy.versionNumber += 1 previousVersionCopy.active = false splitTest.versions.push(previousVersionCopy) diff --git a/services/web/app/src/Features/SplitTests/SplitTestUtils.js b/services/web/app/src/Features/SplitTests/SplitTestUtils.js new file mode 100644 index 0000000000..ef26dbec33 --- /dev/null +++ b/services/web/app/src/Features/SplitTests/SplitTestUtils.js @@ -0,0 +1,20 @@ +const _ = require('lodash') + +function getCurrentVersion(splitTest) { + if (splitTest?.versions?.length > 0) { + return _.maxBy(splitTest.versions, 'versionNumber') + } else { + return undefined + } +} + +function getVersion(splitTest, versionNumber) { + return _.find(splitTest.versions || [], { + versionNumber, + }) +} + +module.exports = { + getCurrentVersion, + getVersion, +} diff --git a/services/web/app/src/models/SplitTest.js b/services/web/app/src/models/SplitTest.js index 720509368a..03c82d2e86 100644 --- a/services/web/app/src/models/SplitTest.js +++ b/services/web/app/src/models/SplitTest.js @@ -1,6 +1,5 @@ const mongoose = require('../infrastructure/Mongoose') const { Schema } = mongoose -const _ = require('lodash') const MIN_NAME_LENGTH = 3 const MAX_NAME_LENGTH = 200 @@ -147,20 +146,6 @@ const SplitTestSchema = new Schema({ }, }) -SplitTestSchema.methods.getCurrentVersion = function () { - if (this.versions && this.versions.length > 0) { - return _.maxBy(this.versions, 'versionNumber') - } else { - return undefined - } -} - -SplitTestSchema.methods.getVersion = function (versionNumber) { - return _.find(this.versions || [], { - versionNumber, - }) -} - module.exports = { SplitTest: mongoose.model('SplitTest', SplitTestSchema), SplitTestSchema, diff --git a/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js b/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js index 1ed216da28..829b9f18f4 100644 --- a/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js +++ b/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js @@ -176,28 +176,31 @@ describe('SplitTestHandler', function () { }) }) -function makeSplitTest(name, opts = {}) { - const { +function makeSplitTest( + name, + { active = true, analyticsEnabled = active, phase = 'release', versionNumber = 1, - } = opts - + } = {} +) { return { name, - getCurrentVersion: sinon.stub().returns({ - active, - analyticsEnabled, - phase, - versionNumber, - variants: [ - { - name: 'variant-1', - rolloutPercent: 100, - rolloutStripes: [{ start: 0, end: 100 }], - }, - ], - }), + versions: [ + { + active, + analyticsEnabled, + phase, + versionNumber, + variants: [ + { + name: 'variant-1', + rolloutPercent: 100, + rolloutStripes: [{ start: 0, end: 100 }], + }, + ], + }, + ], } }