From 57228c5589e1324b7d06f05c718965884f5db790 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Wed, 26 Jan 2022 09:02:10 -0500 Subject: [PATCH] Merge pull request #6442 from overleaf/em-split-tests-user-admin Show split test assignments in user admin GitOrigin-RevId: 4563a4899d5278a0ef84188ae25cb5dfd3d5cb57 --- .../Features/SplitTests/SplitTestHandler.js | 37 +++++- .../src/SplitTests/SplitTestHandlerTests.js | 122 ++++++++++++++++++ 2 files changed, 157 insertions(+), 2 deletions(-) create mode 100644 services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index 9ac264ad4d..46c1f7f3a8 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -5,7 +5,7 @@ const LocalsHelper = require('./LocalsHelper') const crypto = require('crypto') const _ = require('lodash') const { callbackify } = require('util') -const splitTestCache = require('./SplitTestCache') +const SplitTestCache = require('./SplitTestCache') const DEFAULT_VARIANT = 'default' const ALPHA_PHASE = 'alpha' @@ -103,6 +103,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) { + 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 } + } + activeAssignments[splitTestName] = assignment + } + return activeAssignments +} + async function _getAssignment( splitTestName, { analyticsId, userId, session, sync } @@ -111,7 +142,7 @@ async function _getAssignment( return DEFAULT_ASSIGNMENT } - const splitTest = await splitTestCache.get(splitTestName) + const splitTest = await SplitTestCache.get(splitTestName) const currentVersion = splitTest?.getCurrentVersion() if (!splitTest || !currentVersion?.active) { return DEFAULT_ASSIGNMENT @@ -304,10 +335,12 @@ async function _getUser(id) { module.exports = { getAssignment: callbackify(getAssignment), getAssignmentForUser: callbackify(getAssignmentForUser), + getActiveAssignmentsForUser: callbackify(getActiveAssignmentsForUser), assignInLocalsContext: callbackify(assignInLocalsContext), promises: { getAssignment, getAssignmentForUser, + getActiveAssignmentsForUser, assignInLocalsContext, }, } diff --git a/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js b/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js new file mode 100644 index 0000000000..6cc878d917 --- /dev/null +++ b/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js @@ -0,0 +1,122 @@ +const Path = require('path') +const SandboxedModule = require('sandboxed-module') +const sinon = require('sinon') +const { ObjectId } = require('mongodb') +const { expect } = require('chai') + +const MODULE_PATH = Path.join( + __dirname, + '../../../../app/src/Features/SplitTests/SplitTestHandler' +) + +describe('SplitTestHandler', function () { + beforeEach(function () { + this.splitTest = { + getCurrentVersion: sinon.stub().returns({ active: true }), + } + this.inactiveSplitTest = { + getCurrentVersion: sinon.stub().returns({ active: false }), + } + + this.UserGetter = { + promises: { + getUser: sinon.stub().resolves(null), + }, + } + + 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) + + this.SplitTestHandler = SandboxedModule.require(MODULE_PATH, { + requires: { + '../User/UserGetter': this.UserGetter, + './SplitTestCache': this.SplitTestCache, + '../User/UserUpdater': {}, + '../Analytics/AnalyticsManager': {}, + './LocalsHelper': {}, + }, + }) + }) + + describe('with an existing user', function () { + beforeEach(async function () { + this.user = { + _id: ObjectId(), + splitTests: { + 'legacy-test': 'legacy-variant', + 'other-test': [ + { variantName: 'default', versionNumber: 1 }, + { variantName: 'latest', versionNumber: 3 }, + { variantName: 'experiment', versionNumber: 2 }, + ], + 'inactive-test': [{ variantName: 'trythis' }], + 'unknown-test': [{ variantName: 'trythis' }], + }, + } + this.UserGetter.promises.getUser + .withArgs(this.user._id) + .resolves(this.user) + this.assignments = + await this.SplitTestHandler.promises.getActiveAssignmentsForUser( + this.user._id + ) + }) + + 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' + ) + }) + + 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('does not return assignments for inactive tests', function () { + expect(this.assignments).not.to.have.property('inactive-test') + }) + + it('does not return assignments for unknown tests', function () { + expect(this.assignments).not.to.have.property('unknown-test') + }) + }) + + describe('with an inexistent user', function () { + beforeEach(async function () { + const unknownUserId = ObjectId() + this.assignments = + await this.SplitTestHandler.promises.getActiveAssignmentsForUser( + unknownUserId + ) + }) + + it('returns empty assignments', function () { + expect(this.assignments).to.deep.equal({}) + }) + }) + + describe('with a user without assignments', function () { + beforeEach(async function () { + this.user = { _id: ObjectId() } + this.UserGetter.promises.getUser + .withArgs(this.user._id) + .resolves(this.user) + this.assignments = + await this.SplitTestHandler.promises.getActiveAssignmentsForUser( + this.user._id + ) + }) + + it('returns empty assignments', function () { + expect(this.assignments).to.deep.equal({}) + }) + }) +})