[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

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,

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<sync: boolean>} - 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<Assignment>}
  */
-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) {

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 => {

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