From 17d5a73d99dcaa042c47d5a3ede30021116ec837 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 13 Sep 2023 13:07:00 +0200 Subject: [PATCH] Merge pull request #14803 from overleaf/jpa-split-test-cache-alpha-beta [web] invalidate split test cache when alpha/beta program status changes GitOrigin-RevId: 3023d2adf8466b48490c51497f5c80e7b0a1fe3d --- .../AuthenticationController.js | 2 ++ .../BetaProgram/BetaProgramController.js | 9 ++++-- .../src/Features/Project/ProjectController.js | 2 ++ .../Features/Project/ProjectListController.js | 4 ++- .../Features/SplitTests/SplitTestHandler.js | 31 +++++++++++++++++++ .../BetaProgram/BetaProgramControllerTests.js | 18 +++++++++++ .../src/Project/ProjectControllerTests.js | 23 ++++++++++++++ .../src/Project/ProjectListControllerTests.js | 13 ++++++++ 8 files changed, 99 insertions(+), 3 deletions(-) diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index 333f797d22..0e2495b351 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -66,6 +66,8 @@ const AuthenticationController = { must_reconfirm: user.must_reconfirm, v1_id: user.overleaf != null ? user.overleaf.id : undefined, analyticsId: user.analyticsId || user._id, + alphaProgram: user.alphaProgram || undefined, // only store if set + betaProgram: user.betaProgram || undefined, // only store if set } callback(null, lightUser) }, diff --git a/services/web/app/src/Features/BetaProgram/BetaProgramController.js b/services/web/app/src/Features/BetaProgram/BetaProgramController.js index 1b1e7e24d4..02501d535a 100644 --- a/services/web/app/src/Features/BetaProgram/BetaProgramController.js +++ b/services/web/app/src/Features/BetaProgram/BetaProgramController.js @@ -4,6 +4,7 @@ const UserGetter = require('../User/UserGetter') const Settings = require('@overleaf/settings') const logger = require('@overleaf/logger') const SessionManager = require('../Authentication/SessionManager') +const SplitTestHandler = require('../SplitTests/SplitTestHandler') const BetaProgramController = { optIn(req, res, next) { @@ -16,7 +17,9 @@ const BetaProgramController = { if (err) { return next(err) } - res.redirect('/beta/participate') + SplitTestHandler.sessionMaintenance(req, null, () => + res.redirect('/beta/participate') + ) }) }, @@ -30,7 +33,9 @@ const BetaProgramController = { if (err) { return next(err) } - res.redirect('/beta/participate') + SplitTestHandler.sessionMaintenance(req, null, () => + res.redirect('/beta/participate') + ) }) }, diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index 3f2e7ce064..1b4b59b0bf 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -427,6 +427,7 @@ const ProjectController = { }, user(cb) { if (userId == null) { + SplitTestHandler.sessionMaintenance(req, null, () => {}) cb(null, defaultSettingsForAnonymousUser(userId)) } else { User.updateOne( @@ -448,6 +449,7 @@ const ProjectController = { return cb(err) } logger.debug({ projectId, userId }, 'got user') + SplitTestHandler.sessionMaintenance(req, user, () => {}) if (FeaturesUpdater.featuresEpochIsCurrent(user)) { return cb(null, user) } diff --git a/services/web/app/src/Features/Project/ProjectListController.js b/services/web/app/src/Features/Project/ProjectListController.js index b1dff38fd1..287cb9d78d 100644 --- a/services/web/app/src/Features/Project/ProjectListController.js +++ b/services/web/app/src/Features/Project/ProjectListController.js @@ -101,7 +101,7 @@ async function projectListPage(req, res, next) { }) const user = await User.findById( userId, - `email emails features lastPrimaryEmailCheck signUpDate${ + `email emails features alphaProgram betaProgram lastPrimaryEmailCheck signUpDate${ isSaas ? ' enrollment' : '' }` ) @@ -113,6 +113,8 @@ async function projectListPage(req, res, next) { } if (isSaas) { + await SplitTestHandler.promises.sessionMaintenance(req, user) + try { usersBestSubscription = await SubscriptionViewModelBuilder.promises.getBestSubscription({ diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index a20895ebab..cb58acba15 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -13,6 +13,7 @@ const { getAnalyticsIdFromMongoUser } = require('../Analytics/AnalyticsHelper') const Features = require('../../infrastructure/Features') const SplitTestUtils = require('./SplitTestUtils') const Settings = require('@overleaf/settings') +const SessionManager = require('../Authentication/SessionManager') const DEFAULT_VARIANT = 'default' const ALPHA_PHASE = 'alpha' @@ -188,6 +189,34 @@ async function getActiveAssignmentsForUser(userId, removeArchived = false) { return assignments } +/** + * @param {import('express').Request} req + * @param {Object|null} user optional, prefetched user with alphaProgram and betaProgram field + * @return {Promise} + */ +async function sessionMaintenance(req, user) { + const session = req.session + const sessionUser = SessionManager.getSessionUser(session) + + Metrics.inc('split_test_session_maintenance', 1, { status: 'start' }) + if (sessionUser) { + user = user || (await _getUser(sessionUser._id)) + if ( + Boolean(sessionUser.alphaProgram) !== Boolean(user.alphaProgram) || + Boolean(sessionUser.betaProgram) !== Boolean(user.betaProgram) + ) { + Metrics.inc('split_test_session_maintenance', 1, { + status: 'program-change', + }) + sessionUser.alphaProgram = user.alphaProgram || undefined // only store if set + sessionUser.betaProgram = user.betaProgram || undefined // only store if set + session.cachedSplitTestAssignments = {} + } + } + + // TODO: After changing the split test config fetching: remove split test assignments for archived split tests +} + /** * Returns an array of valid variant names for the given split test, including default * @@ -507,10 +536,12 @@ module.exports = { getAssignmentForMongoUser: callbackify(getAssignmentForMongoUser), getAssignmentForUser: callbackify(getAssignmentForUser), getActiveAssignmentsForUser: callbackify(getActiveAssignmentsForUser), + sessionMaintenance: callbackify(sessionMaintenance), promises: { getAssignment, getAssignmentForMongoUser, getAssignmentForUser, getActiveAssignmentsForUser, + sessionMaintenance, }, } diff --git a/services/web/test/unit/src/BetaProgram/BetaProgramControllerTests.js b/services/web/test/unit/src/BetaProgram/BetaProgramControllerTests.js index a4332bdd36..8db25d1b64 100644 --- a/services/web/test/unit/src/BetaProgram/BetaProgramControllerTests.js +++ b/services/web/test/unit/src/BetaProgram/BetaProgramControllerTests.js @@ -21,8 +21,12 @@ describe('BetaProgramController', function () { user: this.user, }, } + this.SplitTestHandler = { + sessionMaintenance: sinon.stub().yields(), + } this.BetaProgramController = SandboxedModule.require(modulePath, { requires: { + '../SplitTests/SplitTestHandler': this.SplitTestHandler, './BetaProgramHandler': (this.BetaProgramHandler = { optIn: sinon.stub(), optOut: sinon.stub(), @@ -68,6 +72,13 @@ describe('BetaProgramController', function () { this.BetaProgramHandler.optIn.callCount.should.equal(1) }) + it('should invoke the session maintenance', function () { + this.BetaProgramController.optIn(this.req, this.res, this.next) + this.SplitTestHandler.sessionMaintenance.should.have.been.calledWith( + this.req + ) + }) + describe('when BetaProgramHandler.opIn produces an error', function () { beforeEach(function () { this.BetaProgramHandler.optIn.callsArgWith(1, new Error('woops')) @@ -107,6 +118,13 @@ describe('BetaProgramController', function () { this.BetaProgramHandler.optOut.callCount.should.equal(1) }) + it('should invoke the session maintenance', function () { + this.BetaProgramController.optOut(this.req, this.res, this.next) + this.SplitTestHandler.sessionMaintenance.should.have.been.calledWith( + this.req + ) + }) + describe('when BetaProgramHandler.optOut produces an error', function () { beforeEach(function () { this.BetaProgramHandler.optOut.callsArgWith(1, new Error('woops')) diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index 4d7372f306..c414eac189 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -128,6 +128,7 @@ describe('ProjectController', function () { getAssignment: sinon.stub().resolves({ variant: 'default' }), }, getAssignment: sinon.stub().yields(null, { variant: 'default' }), + sessionMaintenance: sinon.stub().yields(), } this.InstitutionsFeatures = { hasLicence: sinon.stub().callsArgWith(1, null, false), @@ -515,6 +516,28 @@ describe('ProjectController', function () { return this.ProjectController.loadEditor(this.req, this.res) }) + it('should invoke the session maintenance for logged in user', function (done) { + this.res.render = () => { + this.SplitTestHandler.sessionMaintenance.should.have.been.calledWith( + this.req, + this.user + ) + done() + } + this.ProjectController.loadEditor(this.req, this.res) + }) + + it('should invoke the session maintenance for anonymous user', function (done) { + this.SessionManager.getLoggedInUserId.returns(null) + this.res.render = () => { + this.SplitTestHandler.sessionMaintenance.should.have.been.calledWith( + this.req + ) + done() + } + this.ProjectController.loadEditor(this.req, this.res) + }) + it('should render the closed page if the editor is closed', function (done) { this.settings.editorIsOpen = false this.res.render = (pageName, opts) => { diff --git a/services/web/test/unit/src/Project/ProjectListControllerTests.js b/services/web/test/unit/src/Project/ProjectListControllerTests.js index 5c84f291ea..e4eb3522f3 100644 --- a/services/web/test/unit/src/Project/ProjectListControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectListControllerTests.js @@ -102,6 +102,7 @@ describe('ProjectListController', function () { } this.SplitTestHandler = { promises: { + sessionMaintenance: sinon.stub().resolves(), getAssignment: sinon.stub().resolves({ variant: 'default' }), }, } @@ -204,6 +205,18 @@ describe('ProjectListController', function () { this.ProjectListController.projectListPage(this.req, this.res) }) + it('should invoke the session maintenance', function (done) { + this.Features.hasFeature.withArgs('saas').returns(true) + this.res.render = () => { + this.SplitTestHandler.promises.sessionMaintenance.should.have.been.calledWith( + this.req, + this.user + ) + done() + } + this.ProjectListController.projectListPage(this.req, this.res) + }) + it('should send the tags', function (done) { this.res.render = (pageName, opts) => { opts.tags.length.should.equal(this.tags.length)