From 5ded04eaea8df0cac8ca89758921d6f5ca8fcd23 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 28 Feb 2022 07:57:41 -0500 Subject: [PATCH] Merge pull request #6785 from overleaf/em-split-tests-analytics-enabled Add "analytics enabled" setting to split tests GitOrigin-RevId: 9ddfda9e246cac7a13361b2d3df6884212583000 --- .../Features/SplitTests/SplitTestHandler.js | 75 +++++----- .../Features/SplitTests/SplitTestManager.js | 3 + services/web/app/src/models/SplitTest.js | 5 + .../frontend/stylesheets/app/admin-hub.less | 4 + ...222095146_split_tests_analytics_enabled.js | 51 +++++++ services/web/test/acceptance/bootstrap.js | 1 + .../src/SplitTests/SplitTestHandlerTests.js | 137 ++++++++++++++---- .../src/Subscription/RecurlyClientTests.js | 7 +- 8 files changed, 212 insertions(+), 71 deletions(-) create mode 100644 services/web/migrations/20220222095146_split_tests_analytics_enabled.js diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index 46c1f7f3a8..7593e63989 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -6,6 +6,7 @@ const crypto = require('crypto') const _ = require('lodash') const { callbackify } = require('util') const SplitTestCache = require('./SplitTestCache') +const { SplitTest } = require('../../models/SplitTest') const DEFAULT_VARIANT = 'default' const ALPHA_PHASE = 'alpha' @@ -107,31 +108,37 @@ async function assignInLocalsContext( * Get a mapping of the active split test assignments for the given user */ async function getActiveAssignmentsForUser(userId) { - const user = await UserGetter.promises.getUser(userId, { splitTests: 1 }) - if (user == null || user.splitTests == null) { + const user = await _getUser(userId) + if (user == null) { return {} } - const activeAssignments = {} - for (const [splitTestName, assignments] of Object.entries(user.splitTests)) { - const splitTest = await SplitTestCache.get(splitTestName) - if (splitTest == null) { - continue - } - const currentVersion = splitTest.getCurrentVersion() - if (!currentVersion || !currentVersion.active) { - continue - } - let assignment - if (Array.isArray(assignments)) { - assignment = _.maxBy(assignments, 'versionNumber') - } else { - // Older format is a single string rather than an array of objects - assignment = { variantName: assignments } + const splitTests = await SplitTest.find({ + $where: 'this.versions[this.versions.length - 1].active', + }).exec() + const assignments = {} + for (const splitTest of splitTests) { + const { activeForUser, selectedVariantName, phase, versionNumber } = + await _getAssignmentMetadata(user.analyticsId, user, splitTest) + if (activeForUser) { + const assignment = { + variantName: selectedVariantName, + versionNumber, + phase, + } + const userAssignments = user.splitTests?.[splitTest.name] + if (Array.isArray(userAssignments)) { + const userAssignment = userAssignments.find( + x => x.versionNumber === versionNumber + ) + if (userAssignment) { + assignment.assignedAt = userAssignment.assignedAt + } + } + assignments[splitTest.name] = assignment } - activeAssignments[splitTestName] = assignment } - return activeAssignments + return assignments } async function _getAssignment( @@ -158,8 +165,9 @@ async function _getAssignment( return _makeAssignment(splitTest, cachedVariant, currentVersion) } } + const user = userId && (await _getUser(userId)) const { activeForUser, selectedVariantName, phase, versionNumber } = - await _getAssignmentMetadata(analyticsId, userId, splitTest) + await _getAssignmentMetadata(analyticsId, user, splitTest) if (activeForUser) { const assignmentConfig = { userId, @@ -181,26 +189,19 @@ async function _getAssignment( return DEFAULT_ASSIGNMENT } -async function _getAssignmentMetadata(analyticsId, userId, splitTest) { +async function _getAssignmentMetadata(analyticsId, user, splitTest) { const currentVersion = splitTest.getCurrentVersion() const phase = currentVersion.phase - if ([ALPHA_PHASE, BETA_PHASE].includes(phase)) { - if (userId) { - const user = await _getUser(userId) - if ( - (phase === ALPHA_PHASE && !(user && user.alphaProgram)) || - (phase === BETA_PHASE && !(user && user.betaProgram)) - ) { - return { - activeForUser: false, - } - } - } else { - return { - activeForUser: false, - } + if ( + !user || + (phase === ALPHA_PHASE && !user.alphaProgram) || + (phase === BETA_PHASE && !user.betaProgram) + ) { + return { + activeForUser: false, } } + const userId = user?._id.toString() const percentile = _getPercentile( analyticsId || userId, splitTest.name, diff --git a/services/web/app/src/Features/SplitTests/SplitTestManager.js b/services/web/app/src/Features/SplitTests/SplitTestManager.js index 2c075b45e2..eeef6b2964 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestManager.js +++ b/services/web/app/src/Features/SplitTests/SplitTestManager.js @@ -58,6 +58,8 @@ async function createSplitTest(name, configuration, info = {}) { versionNumber: 1, phase: configuration.phase, active: configuration.active, + analyticsEnabled: + configuration.active && configuration.analyticsEnabled, variants: stripedVariants, }, ], @@ -84,6 +86,7 @@ async function updateSplitTestConfig(name, configuration) { versionNumber: lastVersion.versionNumber + 1, phase: configuration.phase, active: configuration.active, + analyticsEnabled: configuration.active && configuration.analyticsEnabled, variants: updatedVariants, }) return _saveSplitTest(splitTest) diff --git a/services/web/app/src/models/SplitTest.js b/services/web/app/src/models/SplitTest.js index a84fcc0d50..85b3e9b071 100644 --- a/services/web/app/src/models/SplitTest.js +++ b/services/web/app/src/models/SplitTest.js @@ -60,6 +60,11 @@ const VersionSchema = new Schema( default: true, required: true, }, + analyticsEnabled: { + type: Boolean, + default: true, + required: true, + }, variants: [VariantSchema], createdAt: { type: Date, diff --git a/services/web/frontend/stylesheets/app/admin-hub.less b/services/web/frontend/stylesheets/app/admin-hub.less index a4f199d623..d0cf2c8086 100644 --- a/services/web/frontend/stylesheets/app/admin-hub.less +++ b/services/web/frontend/stylesheets/app/admin-hub.less @@ -118,6 +118,10 @@ background: inherit; left: 20px; } + &:disabled + label { + opacity: 0.5; + cursor: not-allowed; + } } label { diff --git a/services/web/migrations/20220222095146_split_tests_analytics_enabled.js b/services/web/migrations/20220222095146_split_tests_analytics_enabled.js new file mode 100644 index 0000000000..97aa614cb1 --- /dev/null +++ b/services/web/migrations/20220222095146_split_tests_analytics_enabled.js @@ -0,0 +1,51 @@ +exports.tags = ['saas'] + +exports.migrate = async client => { + const { db } = client + await db.splittests.updateMany( + {}, + { $set: { 'versions.$[version].analyticsEnabled': true } }, + { + arrayFilters: [ + { + 'version.active': true, + 'version.analyticsEnabled': { $exists: false }, + }, + ], + } + ) + await db.splittests.updateMany( + {}, + { $set: { 'versions.$[version].analyticsEnabled': false } }, + { + arrayFilters: [ + { + 'version.active': false, + 'version.analyticsEnabled': { $exists: false }, + }, + ], + } + ) +} + +exports.rollback = async client => { + const { db } = client + await db.splittests.updateMany( + {}, + { $unset: { 'versions.$[version].analyticsEnabled': 1 } }, + { + arrayFilters: [ + { 'version.active': true, 'version.analyticsEnabled': true }, + ], + } + ) + await db.splittests.updateMany( + {}, + { $unset: { 'versions.$[version].analyticsEnabled': 1 } }, + { + arrayFilters: [ + { 'version.active': false, 'version.analyticsEnabled': false }, + ], + } + ) +} diff --git a/services/web/test/acceptance/bootstrap.js b/services/web/test/acceptance/bootstrap.js index 7372c5cf70..f960562943 100644 --- a/services/web/test/acceptance/bootstrap.js +++ b/services/web/test/acceptance/bootstrap.js @@ -3,6 +3,7 @@ chai.should() chai.use(require('chai-as-promised')) chai.use(require('chaid')) chai.use(require('sinon-chai')) +chai.use(require('chai-exclude')) // Do not truncate assertion errors chai.config.truncateThreshold = 0 diff --git a/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js b/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js index 6cc878d917..1ed216da28 100644 --- a/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js +++ b/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js @@ -11,12 +11,15 @@ const MODULE_PATH = Path.join( describe('SplitTestHandler', function () { beforeEach(function () { - this.splitTest = { - getCurrentVersion: sinon.stub().returns({ active: true }), - } - this.inactiveSplitTest = { - getCurrentVersion: sinon.stub().returns({ active: false }), - } + this.splitTests = [ + makeSplitTest('active-test'), + makeSplitTest('legacy-test'), + makeSplitTest('no-analytics-test-1', { analyticsEnabled: false }), + makeSplitTest('no-analytics-test-2', { + analyticsEnabled: false, + versionNumber: 2, + }), + ] this.UserGetter = { promises: { @@ -24,19 +27,24 @@ describe('SplitTestHandler', function () { }, } + this.SplitTest = { + find: sinon.stub().returns({ + exec: sinon.stub().resolves(this.splitTests), + }), + } + this.SplitTestCache = { get: sinon.stub().resolves(null), } - this.SplitTestCache.get.withArgs('legacy-test').resolves(this.splitTest) - this.SplitTestCache.get.withArgs('other-test').resolves(this.splitTest) - this.SplitTestCache.get - .withArgs('inactive-test') - .resolves(this.inactiveSplitTest) + for (const splitTest of this.splitTests) { + this.SplitTestCache.get.withArgs(splitTest.name).resolves(splitTest) + } this.SplitTestHandler = SandboxedModule.require(MODULE_PATH, { requires: { '../User/UserGetter': this.UserGetter, './SplitTestCache': this.SplitTestCache, + '../../models/SplitTest': { SplitTest: this.SplitTest }, '../User/UserUpdater': {}, '../Analytics/AnalyticsManager': {}, './LocalsHelper': {}, @@ -49,14 +57,23 @@ describe('SplitTestHandler', function () { this.user = { _id: ObjectId(), splitTests: { - 'legacy-test': 'legacy-variant', - 'other-test': [ - { variantName: 'default', versionNumber: 1 }, - { variantName: 'latest', versionNumber: 3 }, - { variantName: 'experiment', versionNumber: 2 }, + 'active-test': [ + { + variantName: 'default', + versionNumber: 1, + assignedAt: 'active-test-assigned-at', + }, ], + 'legacy-test': 'legacy-variant', 'inactive-test': [{ variantName: 'trythis' }], 'unknown-test': [{ variantName: 'trythis' }], + 'no-analytics-test-2': [ + { + variantName: 'some-variant', + versionNumber: 1, + assignedAt: 'no-analytics-assigned-at', + }, + ], }, } this.UserGetter.promises.getUser @@ -69,19 +86,36 @@ describe('SplitTestHandler', function () { }) it('handles the legacy assignment format', function () { - expect(this.assignments).to.have.property('legacy-test') - expect(this.assignments['legacy-test'].variantName).to.equal( - 'legacy-variant' - ) + expect(this.assignments['legacy-test']).to.deep.equal({ + variantName: 'variant-1', + phase: 'release', + versionNumber: 1, + }) }) - it('returns the last assignment for each active test', function () { - expect(this.assignments).to.have.property('other-test') - expect(this.assignments['other-test'].variantName).to.equal('latest') + it('returns the current assignment for each active test', function () { + expect(this.assignments['active-test']).to.deep.equal({ + variantName: 'variant-1', + phase: 'release', + versionNumber: 1, + assignedAt: 'active-test-assigned-at', + }) }) - it('does not return assignments for inactive tests', function () { - expect(this.assignments).not.to.have.property('inactive-test') + it('returns the current assignment for tests with analytics disabled', function () { + expect(this.assignments['no-analytics-test-1']).to.deep.equal({ + variantName: 'variant-1', + phase: 'release', + versionNumber: 1, + }) + }) + + it('returns the current assignment for tests with analytics disabled that had previous assignments', function () { + expect(this.assignments['no-analytics-test-2']).to.deep.equal({ + variantName: 'variant-1', + phase: 'release', + versionNumber: 2, + }) }) it('does not return assignments for unknown tests', function () { @@ -89,7 +123,7 @@ describe('SplitTestHandler', function () { }) }) - describe('with an inexistent user', function () { + describe('with an non-existent user', function () { beforeEach(async function () { const unknownUserId = ObjectId() this.assignments = @@ -115,8 +149,55 @@ describe('SplitTestHandler', function () { ) }) - it('returns empty assignments', function () { - expect(this.assignments).to.deep.equal({}) + it('returns current assignments', function () { + expect(this.assignments).to.deep.equal({ + 'active-test': { + phase: 'release', + variantName: 'variant-1', + versionNumber: 1, + }, + 'legacy-test': { + phase: 'release', + variantName: 'variant-1', + versionNumber: 1, + }, + 'no-analytics-test-1': { + phase: 'release', + variantName: 'variant-1', + versionNumber: 1, + }, + 'no-analytics-test-2': { + phase: 'release', + variantName: 'variant-1', + versionNumber: 2, + }, + }) }) }) }) + +function makeSplitTest(name, opts = {}) { + const { + 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 }], + }, + ], + }), + } +} diff --git a/services/web/test/unit/src/Subscription/RecurlyClientTests.js b/services/web/test/unit/src/Subscription/RecurlyClientTests.js index 726c545892..80a0b68159 100644 --- a/services/web/test/unit/src/Subscription/RecurlyClientTests.js +++ b/services/web/test/unit/src/Subscription/RecurlyClientTests.js @@ -1,10 +1,5 @@ const sinon = require('sinon') -const sinonChai = require('sinon-chai') -const chai = require('chai') -const chaiAsPromised = require('chai-as-promised') -chai.use(sinonChai) -chai.use(chaiAsPromised) -const { expect } = chai +const { expect } = require('chai') const recurly = require('recurly') const modulePath = '../../../../app/src/Features/Subscription/RecurlyClient' const SandboxedModule = require('sandboxed-module')