From 1df98c028de729782c64b9aeaf9d0e7e7e833cce Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Mon, 11 May 2026 14:09:47 +0200 Subject: [PATCH] [web] add includeReferer flag to SplitTestHandler.getAssignment (#33235) * [web] add includeReferer flag to SplitTestHandler.getAssignment * [web] tests: migrate User.getSplitTestAssignment to async/await I don't want to fight with callbacks and optional arguments. Just move it to async/await. New tests should use async/await, so there is no point in making this work in callback-hell. * [web] remove unused URL import GitOrigin-RevId: 6251001e6ba7354f704fa663be8ef365ca0b9d23 --- .../Features/Compile/CompileController.mjs | 20 ++----- .../Features/SplitTests/SplitTestHandler.mjs | 26 ++++++++- .../test/acceptance/src/helpers/InitApp.mjs | 3 +- .../web/test/acceptance/src/helpers/User.mjs | 56 +++++++++---------- 4 files changed, 54 insertions(+), 51 deletions(-) diff --git a/services/web/app/src/Features/Compile/CompileController.mjs b/services/web/app/src/Features/Compile/CompileController.mjs index a40c2bc3a7..ef176598cf 100644 --- a/services/web/app/src/Features/Compile/CompileController.mjs +++ b/services/web/app/src/Features/Compile/CompileController.mjs @@ -1,4 +1,3 @@ -import { URL } from 'node:url' import { pipeline } from 'node:stream/promises' import Metrics from '@overleaf/metrics' import ProjectGetter from '../Project/ProjectGetter.mjs' @@ -46,23 +45,12 @@ function getOutputFilesArchiveSpecification(projectId, userId, buildId) { } } -function getPdfCachingMinChunkSize(req, res) { - return Settings.pdfCachingMinChunkSize -} - async function _getSplitTestOptions(req, res) { - // Use the query flags from the editor request for overriding the split test. - let query = {} - try { - const u = new URL(req.headers.referer || req.url, Settings.siteUrl) - query = Object.fromEntries(u.searchParams.entries()) - } catch (e) {} - const editorReq = { ...req, query } - const { variant } = await SplitTestHandler.promises.getAssignment( - editorReq, + req, res, - 'compile-from-history' + 'compile-from-history', + { includeReferer: true } ) const compileFromHistory = variant === 'enabled' @@ -78,7 +66,7 @@ async function _getSplitTestOptions(req, res) { } } - const pdfCachingMinChunkSize = getPdfCachingMinChunkSize(editorReq, res) + const pdfCachingMinChunkSize = Settings.pdfCachingMinChunkSize return { compileFromHistory, pdfDownloadDomain, diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs b/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs index fd70276de1..5dd03a39af 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs @@ -47,11 +47,17 @@ const DEFAULT_ASSIGNMENT = { * @param req the request * @param res the Express response object * @param splitTestName the unique name of the split test - * @param options {Object} - for test purposes only, to force the synchronous update of the user's profile + * @param {Object} options + * @param {boolean} options.sync - for test purposes only, to force the synchronous update of the user's profile + * @param {boolean} options.includeReferer For ajax requests and downloads include the split test overrides of the page * @returns {Promise} */ -async function getAssignment(req, res, splitTestName, { sync = false } = {}) { - const query = req.query || {} +async function getAssignment( + req, + res, + splitTestName, + { sync = false, includeReferer = false } = {} +) { let assignment try { @@ -60,6 +66,20 @@ async function getAssignment(req, res, splitTestName, { sync = false } = {}) { } else { await _loadSplitTestInfoInLocals(res.locals, splitTestName, req.session) + let query = req.query || {} + if (includeReferer && req.headers.referer) { + // Pick up the query of the top-level page, i.e. what's in the browsers address bar, from ajax requests. + // E.g. /project/:id?split-test=foo -> ajax /project/:id/compile should see split-test=foo. + // E.g. /project/:id?split-test=foo -> redirect /project/:id/download/zip should see split-test=foo. + try { + const u = new URL(req.headers.referer, Settings.siteUrl) + query = { + ...Object.fromEntries(u.searchParams.entries()), + ...query, + } + } catch {} + } + // Check the query string for an override, ignoring an invalid value const queryVariant = query[splitTestName] if (queryVariant) { diff --git a/services/web/test/acceptance/src/helpers/InitApp.mjs b/services/web/test/acceptance/src/helpers/InitApp.mjs index 6bd954a4bb..259e844f67 100644 --- a/services/web/test/acceptance/src/helpers/InitApp.mjs +++ b/services/web/test/acceptance/src/helpers/InitApp.mjs @@ -53,10 +53,11 @@ before('start main app', function (done) { route => route.path && route.path === '/dev/csrf', router => { router.get('/dev/split_test/get_assignment', (req, res) => { - const { splitTestName } = req.query + const { splitTestName, includeReferer } = req.query SplitTestHandler.promises .getAssignment(req, res, splitTestName, { sync: true, + includeReferer: includeReferer === 'true', }) .then(assignment => res.json(assignment)) .catch(error => { diff --git a/services/web/test/acceptance/src/helpers/User.mjs b/services/web/test/acceptance/src/helpers/User.mjs index 09e6e01936..464a942f4f 100644 --- a/services/web/test/acceptance/src/helpers/User.mjs +++ b/services/web/test/acceptance/src/helpers/User.mjs @@ -100,37 +100,6 @@ class User { }) } - getSplitTestAssignment(splitTestName, query, callback) { - if (!callback) { - callback = query - } - const params = new URLSearchParams({ - splitTestName, - ...query, - }).toString() - this.request.get( - { - url: `/dev/split_test/get_assignment?${params}`, - }, - (err, response, body) => { - if (err != null) { - return callback(err) - } - if (response.statusCode !== 200) { - return callback( - new Error( - `get split test assignment failed: status=${ - response.statusCode - } body=${JSON.stringify(body)}` - ) - ) - } - const assignment = JSON.parse(response.body) - callback(null, assignment) - } - ) - } - doSessionMaintenance(callback) { this.request.post( { @@ -1350,4 +1319,29 @@ User.promises.prototype.doRequest = async function (method, params) { }) } +User.promises.prototype.getSplitTestAssignment = async function ( + splitTestName, + query, + referer, + includeReferer +) { + const params = new URLSearchParams({ + splitTestName, + includeReferer, + ...query, + }).toString() + const { response, body } = await this.doRequest('GET', { + url: `/dev/split_test/get_assignment?${params}`, + headers: { referer }, + }) + if (response.statusCode !== 200) { + throw new Error( + `get split test assignment failed: status=${ + response.statusCode + } body=${JSON.stringify(body)}` + ) + } + return JSON.parse(response.body) +} + export default User