From b9dd4f354ea5baaa5cc52222add748e0a5592576 Mon Sep 17 00:00:00 2001 From: Miguel Serrano Date: Thu, 2 Dec 2021 11:13:50 +0100 Subject: [PATCH] Merge pull request #5881 from overleaf/ab-split-test-middleware New global split test middleware for locals GitOrigin-RevId: b530572f709572663fc3d051f544064bd8804f76 --- .../src/Features/SplitTests/LocalsHelper.js | 10 + .../SplitTests/SplitTestMiddleware.js | 54 ++++++ .../Features/SplitTests/SplitTestV2Handler.js | 19 +- services/web/app/src/router.js | 6 + .../SplitTests/SplitTestMiddlewareTests.js | 178 ++++++++++++++++++ .../web/test/unit/src/helpers/MockResponse.js | 1 + 6 files changed, 260 insertions(+), 8 deletions(-) create mode 100644 services/web/app/src/Features/SplitTests/LocalsHelper.js create mode 100644 services/web/app/src/Features/SplitTests/SplitTestMiddleware.js create mode 100644 services/web/test/unit/src/SplitTests/SplitTestMiddlewareTests.js diff --git a/services/web/app/src/Features/SplitTests/LocalsHelper.js b/services/web/app/src/Features/SplitTests/LocalsHelper.js new file mode 100644 index 0000000000..51de4bed01 --- /dev/null +++ b/services/web/app/src/Features/SplitTests/LocalsHelper.js @@ -0,0 +1,10 @@ +function setSplitTestVariant(locals, splitTestName, variant) { + if (!locals.splitTestVariants) { + locals.splitTestVariants = {} + } + locals.splitTestVariants[splitTestName] = variant +} + +module.exports = { + setSplitTestVariant, +} diff --git a/services/web/app/src/Features/SplitTests/SplitTestMiddleware.js b/services/web/app/src/Features/SplitTests/SplitTestMiddleware.js new file mode 100644 index 0000000000..cff0ca3214 --- /dev/null +++ b/services/web/app/src/Features/SplitTests/SplitTestMiddleware.js @@ -0,0 +1,54 @@ +const SplitTestV2Handler = require('./SplitTestV2Handler') +const SplitTestCache = require('./SplitTestCache') +const LocalsHelper = require('./LocalsHelper') +const logger = require('@overleaf/logger') + +function loadAssignmentsInLocals(splitTestNames) { + return async function (req, res, next) { + try { + if (!req.session.cachedSplitTestAssignments) { + req.session.cachedSplitTestAssignments = {} + } + for (const splitTestName of splitTestNames) { + const splitTest = await SplitTestCache.get(splitTestName) + if (splitTest) { + await _loadAssignmentInLocals(splitTest, req.session, res.locals) + } + } + } catch (error) { + logger.error( + { err: error, splitTestNames }, + 'Failed to load split test assignments in express locals in middleware' + ) + } + next() + } +} + +async function _loadAssignmentInLocals(splitTest, session, locals) { + const currentVersion = splitTest.getCurrentVersion() + const cacheKey = `${splitTest.name}-${currentVersion.versionNumber}` + if (currentVersion.active) { + const cachedVariant = session.cachedSplitTestAssignments[cacheKey] + if (cachedVariant) { + LocalsHelper.setSplitTestVariant(locals, splitTest.name, cachedVariant) + } else { + const assignment = await SplitTestV2Handler.promises.getAssignmentForSession( + session, + splitTest.name + ) + session.cachedSplitTestAssignments[cacheKey] = assignment.variant + LocalsHelper.setSplitTestVariant( + locals, + splitTest.name, + assignment.variant + ) + } + } else { + delete session.cachedSplitTestAssignments[cacheKey] + } +} + +module.exports = { + loadAssignmentsInLocals, +} diff --git a/services/web/app/src/Features/SplitTests/SplitTestV2Handler.js b/services/web/app/src/Features/SplitTests/SplitTestV2Handler.js index 095871753f..4f3dd1bb42 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestV2Handler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestV2Handler.js @@ -2,6 +2,7 @@ const UserGetter = require('../User/UserGetter') const UserUpdater = require('../User/UserUpdater') const AnalyticsManager = require('../Analytics/AnalyticsManager') const UserAnalyticsIdCache = require('../Analytics/UserAnalyticsIdCache') +const LocalsHelper = require('./LocalsHelper') const crypto = require('crypto') const _ = require('lodash') const { callbackify } = require('util') @@ -89,10 +90,11 @@ async function getAssignmentForSession(session, splitTestName, options) { */ async function assignInLocalsContext(res, userId, splitTestName, options) { const assignment = await getAssignment(userId, splitTestName, options) - if (!res.locals.splitTestVariants) { - res.locals.splitTestVariants = {} - } - res.locals.splitTestVariants[splitTestName] = assignment.variant + LocalsHelper.setSplitTestVariant( + res.locals, + splitTestName, + assignment.variant + ) } /** @@ -115,10 +117,11 @@ async function assignInLocalsContextForSession( splitTestName, options ) - if (!res.locals.splitTestVariants) { - res.locals.splitTestVariants = {} - } - res.locals.splitTestVariants[splitTestName] = assignment.variant + LocalsHelper.setSplitTestVariant( + res.locals, + splitTestName, + assignment.variant + ) } async function _getAssignment( diff --git a/services/web/app/src/router.js b/services/web/app/src/router.js index 8b11bffff6..4f2f8af87f 100644 --- a/services/web/app/src/router.js +++ b/services/web/app/src/router.js @@ -51,6 +51,7 @@ const UserMembershipRouter = require('./Features/UserMembership/UserMembershipRo const SystemMessageController = require('./Features/SystemMessages/SystemMessageController') const AnalyticsRegistrationSourceMiddleware = require('./Features/Analytics/AnalyticsRegistrationSourceMiddleware') const AnalyticsUTMTrackingMiddleware = require('./Features/Analytics/AnalyticsUTMTrackingMiddleware') +const SplitTestMiddleware = require('./Features/SplitTests/SplitTestMiddleware') const { Joi, validate } = require('./infrastructure/Validation') const { renderUnsupportedBrowserPage, @@ -59,6 +60,7 @@ const { const logger = require('@overleaf/logger') const _ = require('underscore') +const { expressify } = require('./util/promises') module.exports = { initialize } @@ -71,6 +73,10 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.get('*', AnalyticsRegistrationSourceMiddleware.setInbound()) webRouter.get('*', AnalyticsUTMTrackingMiddleware.recordUTMTags()) + webRouter.get( + '*', + expressify(SplitTestMiddleware.loadAssignmentsInLocals([])) + ) webRouter.get('/login', UserPagesController.loginPage) AuthenticationController.addEndpointToLoginWhitelist('/login') diff --git a/services/web/test/unit/src/SplitTests/SplitTestMiddlewareTests.js b/services/web/test/unit/src/SplitTests/SplitTestMiddlewareTests.js new file mode 100644 index 0000000000..b319c29684 --- /dev/null +++ b/services/web/test/unit/src/SplitTests/SplitTestMiddlewareTests.js @@ -0,0 +1,178 @@ +const SandboxedModule = require('sandboxed-module') +const path = require('path') +const modulePath = path.join( + __dirname, + '../../../../app/src/Features/SplitTests/SplitTestMiddleware' +) +const sinon = require('sinon') +const { assert } = require('chai') +const MockResponse = require('../helpers/MockResponse') +const MockRequest = require('../helpers/MockRequest') + +describe('SplitTestMiddleware', function () { + beforeEach(function () { + this.SplitTestMiddleware = SandboxedModule.require(modulePath, { + requires: { + './SplitTestV2Handler': (this.SplitTestV2Handler = { + promises: { + getAssignmentForSession: sinon.stub().resolves(), + }, + }), + './SplitTestCache': (this.SplitTestCache = { + get: sinon.stub().resolves(), + }), + }, + }) + + this.req = new MockRequest() + this.req.session = {} + this.res = new MockResponse() + this.next = sinon.stub() + }) + + it('assign split test variant in locals', async function () { + this.SplitTestCache.get.withArgs('ui-overhaul').resolves({ + name: 'ui-overhaul', + getCurrentVersion: () => ({ + versionNumber: 1, + active: true, + }), + }) + this.SplitTestV2Handler.promises.getAssignmentForSession + .withArgs(this.req.session, 'ui-overhaul') + .resolves({ + variant: 'new', + }) + + const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([ + 'ui-overhaul', + ]) + await middleware(this.req, this.res, this.next) + + assert.equal(this.res.locals.splitTestVariants['ui-overhaul'], 'new') + assert.deepEqual(this.req.session.cachedSplitTestAssignments, { + 'ui-overhaul-1': 'new', + }) + sinon.assert.calledOnce(this.next) + }) + + it('assign multiple split test variant in locals', async function () { + this.SplitTestCache.get + .withArgs('ui-overhaul') + .resolves({ + name: 'ui-overhaul', + getCurrentVersion: () => ({ + versionNumber: 1, + active: true, + }), + }) + .withArgs('other-test') + .resolves({ + name: 'other-test', + getCurrentVersion: () => ({ + versionNumber: 1, + active: true, + }), + }) + + this.SplitTestV2Handler.promises.getAssignmentForSession + .withArgs(this.req.session, 'ui-overhaul') + .resolves({ + variant: 'default', + }) + this.SplitTestV2Handler.promises.getAssignmentForSession + .withArgs(this.req.session, 'other-test') + .resolves({ + variant: 'foobar', + }) + + const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([ + 'ui-overhaul', + 'other-test', + ]) + await middleware(this.req, this.res, this.next) + + assert.equal(this.res.locals.splitTestVariants['ui-overhaul'], 'default') + assert.equal(this.res.locals.splitTestVariants['other-test'], 'foobar') + assert.deepEqual(this.req.session.cachedSplitTestAssignments, { + 'ui-overhaul-1': 'default', + 'other-test-1': 'foobar', + }) + sinon.assert.calledOnce(this.next) + }) + + it('cached assignment in session is used', async function () { + this.req.session.cachedSplitTestAssignments = { + 'ui-overhaul-1': 'cached-variant', + } + this.SplitTestCache.get.withArgs('ui-overhaul').resolves({ + name: 'ui-overhaul', + getCurrentVersion: () => ({ + versionNumber: 1, + active: true, + }), + }) + + const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([ + 'ui-overhaul', + ]) + await middleware(this.req, this.res, this.next) + + sinon.assert.notCalled( + this.SplitTestV2Handler.promises.getAssignmentForSession + ) + assert.equal( + this.res.locals.splitTestVariants['ui-overhaul'], + 'cached-variant' + ) + assert.deepEqual(this.req.session.cachedSplitTestAssignments, { + 'ui-overhaul-1': 'cached-variant', + }) + sinon.assert.calledOnce(this.next) + }) + + it('inactive split test is not assigned in locals', async function () { + this.SplitTestCache.get.withArgs('ui-overhaul').resolves({ + name: 'ui-overhaul', + getCurrentVersion: () => ({ + versionNumber: 1, + active: false, + }), + }) + + const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([ + 'ui-overhaul', + ]) + await middleware(this.req, this.res, this.next) + + assert.equal(this.res.locals.splitTestVariants, undefined) + assert.deepEqual(this.req.session.cachedSplitTestAssignments, {}) + sinon.assert.calledOnce(this.next) + }) + + it('not existing split test is not assigned in locals', async function () { + this.SplitTestCache.get.withArgs('not-found').resolves(undefined) + + const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([ + 'not-found', + ]) + await middleware(this.req, this.res, this.next) + + assert.equal(this.res.locals.splitTestVariants, undefined) + assert.deepEqual(this.req.session.cachedSplitTestAssignments, {}) + sinon.assert.calledOnce(this.next) + }) + + it('next middleware is called even if there is an error', async function () { + this.SplitTestCache.get.throws('some error') + + const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([ + 'some-test', + ]) + await middleware(this.req, this.res, this.next) + + assert.equal(this.res.locals.splitTestVariants, undefined) + assert.deepEqual(this.req.session.cachedSplitTestAssignments, {}) + sinon.assert.calledOnce(this.next) + }) +}) diff --git a/services/web/test/unit/src/helpers/MockResponse.js b/services/web/test/unit/src/helpers/MockResponse.js index e018519a0d..6a0777d06e 100644 --- a/services/web/test/unit/src/helpers/MockResponse.js +++ b/services/web/test/unit/src/helpers/MockResponse.js @@ -27,6 +27,7 @@ class MockResponse { this.redirected = false this.returned = false this.headers = {} + this.locals = {} } render(template, variables) {