Merge pull request #16307 from overleaf/ab-split-tests-never-throw

[web] Guarantee split test assignments can never throw and cleanup boilterplate error handling GitOrigin-RevId: ab50abde6e0632c5a9625b5c3d3e98f3383cc56c
2024-11-21 20:47:08 -05:00 · 2024-01-18 15:25:01 +01:00 · 2024-01-18 15:25:01 +01:00 · 3b9eb4e8aa
commit 3b9eb4e8aa
parent 29d41497ef
5 changed files with 149 additions and 317 deletions
--- a/services/web/app/src/Features/Project/ProjectController.js
+++ b/services/web/app/src/Features/Project/ProjectController.js
@ -542,78 +542,31 @@ const ProjectController = {
            req,
            res,
            'project-share-modal-paywall',
-            {},
-            () => {
-              // do not fail editor load if assignment fails
-              cb()
-            }
+            cb
          )
        },
        sharingModalNullTest(cb) {
          // null test targeting logged in users, for front-end side
-          SplitTestHandler.getAssignment(
-            req,
-            res,
-            'null-test-share-modal',
-            {},
-            () => {
-              // do not fail editor load if assignment fails
-              cb()
-            }
-          )
+          SplitTestHandler.getAssignment(req, res, 'null-test-share-modal', cb)
        },
        pdfjsAssignment(cb) {
-          SplitTestHandler.getAssignment(
-            req,
-            res,
-            'pdfjs-40',
-            {},
-            (error, assignment) => {
-              // do not fail editor load if assignment fails
-              if (error) {
-                cb(null, { variant: 'default' })
-              } else {
-                cb(null, assignment)
-              }
-            }
-          )
+          SplitTestHandler.getAssignment(req, res, 'pdfjs-40', cb)
        },
        latexLogParserAssignment(cb) {
-          SplitTestHandler.getAssignment(
-            req,
-            res,
-            'latex-log-parser',
-            (error, assignment) => {
-              // do not fail editor load if assignment fails
-              if (error) {
-                cb(null, { variant: 'default' })
-              } else {
-                cb(null, assignment)
-              }
-            }
-          )
+          SplitTestHandler.getAssignment(req, res, 'latex-log-parser', cb)
        },
        trackPdfDownloadAssignment(cb) {
-          SplitTestHandler.getAssignment(req, res, 'track-pdf-download', () => {
-            // We'll pick up the assignment from the res.locals assignment.
-            cb()
-          })
+          SplitTestHandler.getAssignment(req, res, 'track-pdf-download', cb)
        },
        pdfCachingModeAssignment(cb) {
-          SplitTestHandler.getAssignment(req, res, 'pdf-caching-mode', () => {
-            // We'll pick up the assignment from the res.locals assignment.
-            cb()
-          })
+          SplitTestHandler.getAssignment(req, res, 'pdf-caching-mode', cb)
        },
        pdfCachingPrefetchingAssignment(cb) {
          SplitTestHandler.getAssignment(
            req,
            res,
            'pdf-caching-prefetching',
-            () => {
-              // We'll pick up the assignment from the res.locals assignment.
-              cb()
-            }
+            cb
          )
        },
        pdfCachingPrefetchLargeAssignment(cb) {
@ -621,10 +574,7 @@ const ProjectController = {
            req,
            res,
            'pdf-caching-prefetch-large',
-            () => {
-              // We'll pick up the assignment from the res.locals assignment.
-              cb()
-            }
+            cb
          )
        },
        pdfCachingCachedUrlLookupAssignment(cb) {
@ -632,10 +582,7 @@ const ProjectController = {
            req,
            res,
            'pdf-caching-cached-url-lookup',
-            () => {
-              // We'll pick up the assignment from the res.locals assignment.
-              cb()
-            }
+            cb
          )
        },
        tableGeneratorPromotionAssignment(cb) {
@ -643,52 +590,17 @@ const ProjectController = {
            req,
            res,
            'table-generator-promotion',
-            () => {
-              // We'll pick up the assignment from the res.locals assignment.
-              cb()
-            }
+            cb
          )
        },
        personalAccessTokenAssignment(cb) {
-          SplitTestHandler.getAssignment(
-            req,
-            res,
-            'personal-access-token',
-            (error, assignment) => {
-              // do not fail editor load if assignment fails
-              if (error) {
-                cb(null, { variant: 'default' })
-              } else {
-                cb(null, assignment)
-              }
-            }
-          )
+          SplitTestHandler.getAssignment(req, res, 'personal-access-token', cb)
        },
        idePageAssignment(cb) {
-          SplitTestHandler.getAssignment(
-            req,
-            res,
-            'ide-page',
-            (error, assignment) => {
-              // do not fail editor load if assignment fails
-              if (error) {
-                cb(null, { variant: 'default' })
-              } else {
-                cb(null, assignment)
-              }
-            }
-          )
+          SplitTestHandler.getAssignment(req, res, 'ide-page', cb)
        },
        writefullIntegrationAssignment(cb) {
-          SplitTestHandler.getAssignment(
-            req,
-            res,
-            'writefull-integration',
-            () => {
-              // We'll pick up the assignment from the res.locals assignment.
-              cb()
-            }
-          )
+          SplitTestHandler.getAssignment(req, res, 'writefull-integration', cb)
        },
        projectTags(cb) {
          if (!userId) {
--- a/services/web/app/src/Features/Project/ProjectListController.js
+++ b/services/web/app/src/Features/Project/ProjectListController.js
@ -326,19 +326,13 @@ async function projectListPage(req, res, next) {
      )
    }
  }
-  try {
-    // The assignment will be picked up via 'ol-splitTestVariants' in react.
-    await SplitTestHandler.promises.getAssignment(
-      req,
-      res,
-      'download-pdf-dashboard'
-    )
-  } catch (err) {
-    logger.error(
-      { err },
-      'failed to get "download-pdf-dashboard" split test assignment'
-    )
-  }
+
+  // The assignment will be picked up via 'ol-splitTestVariants' in react.
+  await SplitTestHandler.promises.getAssignment(
+    req,
+    res,
+    'download-pdf-dashboard'
+  )

  const hasPaidAffiliation = userAffiliations.some(
    affiliation => affiliation.licence && affiliation.licence !== 'free'
@ -370,18 +364,11 @@ async function projectListPage(req, res, next) {
    }
  }

-  try {
-    await SplitTestHandler.promises.getAssignment(
-      req,
-      res,
-      'writefull-integration'
-    )
-  } catch (err) {
-    logger.warn(
-      { err },
-      'failed to get "writefull-integration" split test assignment'
-    )
-  }
+  await SplitTestHandler.promises.getAssignment(
+    req,
+    res,
+    'writefull-integration'
+  )

  let showInrGeoBanner, inrGeoBannerSplitTestName
  let inrGeoBannerVariant = 'default'
@ -390,63 +377,35 @@ async function projectListPage(req, res, next) {
  if (usersBestSubscription?.type === 'free') {
    const { currencyCode, countryCode } =
      await GeoIpLookup.promises.getCurrencyCode(req.ip)
-    let inrGeoPricingVariant = 'default'
-    try {
-      // Split test is kept active, but all users geolocated in India can
-      // now use the INR currency (See #13507)
-      const inrGeoPricingAssignment =
-        await SplitTestHandler.promises.getAssignment(
-          req,
-          res,
-          'geo-pricing-inr'
-        )
-      inrGeoPricingVariant = inrGeoPricingAssignment.variant
-    } catch (error) {
-      logger.error(
-        { err: error },
-        'Failed to get geo-pricing-inr split test assignment'
-      )
-    }
-    try {
-      const latamGeoPricingAssignment =
-        await SplitTestHandler.promises.getAssignment(
-          req,
-          res,
-          'geo-pricing-latam'
-        )
-      showLATAMBanner =
-        latamGeoPricingAssignment.variant === 'latam' &&
-        ['BR', 'MX', 'CO', 'CL', 'PE'].includes(countryCode)
-      // LATAM Banner needs to know which currency to display
-      if (showLATAMBanner) {
-        recommendedCurrency = currencyCode
-      }
-    } catch (error) {
-      logger.error(
-        { err: error },
-        'Failed to get geo-pricing-latam split test assignment'
+    // Split test is kept active, but all users geolocated in India can
+    // now use the INR currency (See #13507)
+    const { variant: inrGeoPricingVariant } =
+      await SplitTestHandler.promises.getAssignment(req, res, 'geo-pricing-inr')
+    const latamGeoPricingAssignment =
+      await SplitTestHandler.promises.getAssignment(
+        req,
+        res,
+        'geo-pricing-latam'
      )
+    showLATAMBanner =
+      latamGeoPricingAssignment.variant === 'latam' &&
+      ['BR', 'MX', 'CO', 'CL', 'PE'].includes(countryCode)
+    // LATAM Banner needs to know which currency to display
+    if (showLATAMBanner) {
+      recommendedCurrency = currencyCode
    }
    if (countryCode === 'IN') {
      inrGeoBannerSplitTestName =
        inrGeoPricingVariant === 'inr'
          ? 'geo-banners-inr-2'
          : 'geo-banners-inr-1'
-      try {
-        const geoBannerAssignment =
-          await SplitTestHandler.promises.getAssignment(
-            req,
-            res,
-            inrGeoBannerSplitTestName
-          )
-        showInrGeoBanner = true
-        inrGeoBannerVariant = geoBannerAssignment.variant
-      } catch (error) {
-        logger.error(
-          { err: error },
-          `Failed to get INR geo banner lookup or assignment (${inrGeoBannerSplitTestName})`
-        )
-      }
+      const geoBannerAssignment = await SplitTestHandler.promises.getAssignment(
+        req,
+        res,
+        inrGeoBannerSplitTestName
+      )
+      showInrGeoBanner = true
+      inrGeoBannerVariant = geoBannerAssignment.variant
    }
  }

--- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js
+++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js
@ -13,6 +13,7 @@ const Features = require('../../infrastructure/Features')
 const SplitTestUtils = require('./SplitTestUtils')
 const Settings = require('@overleaf/settings')
 const SessionManager = require('../Authentication/SessionManager')
+const logger = require('@overleaf/logger')

 const DEFAULT_VARIANT = 'default'
 const ALPHA_PHASE = 'alpha'
@ -54,37 +55,42 @@ async function getAssignment(req, res, splitTestName, { sync = false } = {}) {
  const query = req.query || {}
  let assignment

-  if (!Features.hasFeature('saas')) {
-    assignment = _getNonSaasAssignment(splitTestName)
-  } else {
-    await _loadSplitTestInfoInLocals(res.locals, splitTestName, req.session)
+  try {
+    if (!Features.hasFeature('saas')) {
+      assignment = _getNonSaasAssignment(splitTestName)
+    } else {
+      await _loadSplitTestInfoInLocals(res.locals, splitTestName, req.session)

-    // Check the query string for an override, ignoring an invalid value
-    const queryVariant = query[splitTestName]
-    if (queryVariant) {
-      const variants = await _getVariantNames(splitTestName)
-      if (variants.includes(queryVariant)) {
-        assignment = {
-          variant: queryVariant,
-          analytics: {
-            segmentation: {},
-          },
+      // Check the query string for an override, ignoring an invalid value
+      const queryVariant = query[splitTestName]
+      if (queryVariant) {
+        const variants = await _getVariantNames(splitTestName)
+        if (variants.includes(queryVariant)) {
+          assignment = {
+            variant: queryVariant,
+            analytics: {
+              segmentation: {},
+            },
+          }
        }
      }
-    }

-    if (!assignment) {
-      const { userId, analyticsId } = AnalyticsManager.getIdsFromSession(
-        req.session
-      )
-      assignment = await _getAssignment(splitTestName, {
-        analyticsId,
-        userId,
-        session: req.session,
-        sync,
-      })
-      _collectSessionStats(req.session)
+      if (!assignment) {
+        const { userId, analyticsId } = AnalyticsManager.getIdsFromSession(
+          req.session
+        )
+        assignment = await _getAssignment(splitTestName, {
+          analyticsId,
+          userId,
+          session: req.session,
+          sync,
+        })
+        _collectSessionStats(req.session)
+      }
    }
+  } catch (error) {
+    logger.error({ err: error }, 'Failed to get split test assignment')
+    assignment = DEFAULT_ASSIGNMENT
  }

  LocalsHelper.setSplitTestVariant(
@ -112,12 +118,17 @@ async function getAssignmentForUser(
  splitTestName,
  { sync = false } = {}
 ) {
-  if (!Features.hasFeature('saas')) {
-    return _getNonSaasAssignment(splitTestName)
-  }
+  try {
+    if (!Features.hasFeature('saas')) {
+      return _getNonSaasAssignment(splitTestName)
+    }

-  const analyticsId = await UserAnalyticsIdCache.get(userId)
-  return _getAssignment(splitTestName, { analyticsId, userId, sync })
+    const analyticsId = await UserAnalyticsIdCache.get(userId)
+    return _getAssignment(splitTestName, { analyticsId, userId, sync })
+  } catch (error) {
+    logger.error({ err: error }, 'Failed to get split test assignment for user')
+    return DEFAULT_ASSIGNMENT
+  }
 }

 /**
@ -136,16 +147,24 @@ async function getAssignmentForMongoUser(
  splitTestName,
  { sync = false } = {}
 ) {
-  if (!Features.hasFeature('saas')) {
-    return _getNonSaasAssignment(splitTestName)
-  }
+  try {
+    if (!Features.hasFeature('saas')) {
+      return _getNonSaasAssignment(splitTestName)
+    }

-  return _getAssignment(splitTestName, {
-    analyticsId: await UserAnalyticsIdCache.get(user._id),
-    sync,
-    user,
-    userId: user._id.toString(),
-  })
+    return _getAssignment(splitTestName, {
+      analyticsId: await UserAnalyticsIdCache.get(user._id),
+      sync,
+      user,
+      userId: user._id.toString(),
+    })
+  } catch (error) {
+    logger.error(
+      { err: error },
+      'Failed to get split test assignment for mongo user'
+    )
+    return DEFAULT_ASSIGNMENT
+  }
 }

 /**
--- a/services/web/app/src/Features/Subscription/SubscriptionController.js
+++ b/services/web/app/src/Features/Subscription/SubscriptionController.js
@ -76,39 +76,23 @@ async function plansPage(req, res) {
      geoPricingINRTestVariant === 'inr'
        ? 'geo-banners-inr-2'
        : 'geo-banners-inr-1'
-    try {
-      const geoBannerAssignment = await SplitTestHandler.promises.getAssignment(
-        req,
-        res,
-        inrGeoBannerSplitTestName
-      )
-      inrGeoBannerVariant = geoBannerAssignment.variant
-      if (inrGeoBannerVariant !== 'default') {
-        showInrGeoBanner = true
-      }
-    } catch (error) {
-      logger.error(
-        { err: error },
-        `Failed to get INR geo banner lookup or assignment (${inrGeoBannerSplitTestName})`
-      )
+    const geoBannerAssignment = await SplitTestHandler.promises.getAssignment(
+      req,
+      res,
+      inrGeoBannerSplitTestName
+    )
+    inrGeoBannerVariant = geoBannerAssignment.variant
+    if (inrGeoBannerVariant !== 'default') {
+      showInrGeoBanner = true
    }
  }

  // annual plans with the free trial option split test - nudge variant
-  let annualTrialsAssignment = { variant: 'default' }
-
-  try {
-    annualTrialsAssignment = await SplitTestHandler.promises.getAssignment(
-      req,
-      res,
-      'annual-trials'
-    )
-  } catch (error) {
-    logger.error(
-      { err: error },
-      'failed to get "annualTrialsAssignment" split test assignment'
-    )
-  }
+  const annualTrialsAssignment = await SplitTestHandler.promises.getAssignment(
+    req,
+    res,
+    'annual-trials'
+  )

  const websiteRedesignVariant =
    res.locals.splitTestVariants?.['website-redesign']
@ -285,40 +269,20 @@ async function interstitialPaymentPage(req, res) {
        geoPricingINRTestVariant === 'inr'
          ? 'geo-banners-inr-2'
          : 'geo-banners-inr-1'
-      try {
-        const geoBannerAssignment =
-          await SplitTestHandler.promises.getAssignment(
-            req,
-            res,
-            inrGeoBannerSplitTestName
-          )
-        inrGeoBannerVariant = geoBannerAssignment.variant
-        if (inrGeoBannerVariant !== 'default') {
-          showInrGeoBanner = true
-        }
-      } catch (error) {
-        logger.error(
-          { err: error },
-          `Failed to get INR geo banner lookup or assignment (${inrGeoBannerSplitTestName})`
-        )
+      const geoBannerAssignment = await SplitTestHandler.promises.getAssignment(
+        req,
+        res,
+        inrGeoBannerSplitTestName
+      )
+      inrGeoBannerVariant = geoBannerAssignment.variant
+      if (inrGeoBannerVariant !== 'default') {
+        showInrGeoBanner = true
      }
    }

    // annual plans with the free trial option split test - nudge variant
-    let annualTrialsAssignment = { variant: 'default' }
-
-    try {
-      annualTrialsAssignment = await SplitTestHandler.promises.getAssignment(
-        req,
-        res,
-        'annual-trials'
-      )
-    } catch (error) {
-      logger.error(
-        { err: error },
-        'failed to get "annualTrialsAssignment" split test assignment'
-      )
-    }
+    const annualTrialsAssignment =
+      await SplitTestHandler.promises.getAssignment(req, res, 'annual-trials')

    const paywallPlansPageViewSegmentation = {
      currency: recommendedCurrency,
@ -680,36 +644,21 @@ async function _getRecommendedCurrency(req, res) {
  }
  const currencyLookup = await GeoIpLookup.promises.getCurrencyCode(ip)
  const countryCode = currencyLookup.countryCode
-  let assignmentINR, assignmentLATAM
  let recommendedCurrency = currencyLookup.currencyCode
  // for #12703
-  try {
-    // Split test is kept active, but all users geolocated in India can
-    // now use the INR currency (See #13507)
-    assignmentINR = await SplitTestHandler.promises.getAssignment(
-      req,
-      res,
-      'geo-pricing-inr'
-    )
-  } catch (error) {
-    logger.error(
-      { err: error },
-      'Failed to get assignment for geo-pricing-inr test'
-    )
-  }
+  // Split test is kept active, but all users geolocated in India can
+  // now use the INR currency (See #13507)
+  const assignmentINR = await SplitTestHandler.promises.getAssignment(
+    req,
+    res,
+    'geo-pricing-inr'
+  )
  // for #13559
-  try {
-    assignmentLATAM = await SplitTestHandler.promises.getAssignment(
-      req,
-      res,
-      'geo-pricing-latam'
-    )
-  } catch (error) {
-    logger.error(
-      { err: error },
-      'Failed to get assignment for geo-pricing-latam test'
-    )
-  }
+  const assignmentLATAM = await SplitTestHandler.promises.getAssignment(
+    req,
+    res,
+    'geo-pricing-latam'
+  )
  if (
    ['BRL', 'MXN', 'COP', 'CLP', 'PEN'].includes(recommendedCurrency) &&
    assignmentLATAM?.variant !== 'latam'
--- a/services/web/app/src/Features/User/UserPagesController.js
+++ b/services/web/app/src/Features/User/UserPagesController.js
@ -74,23 +74,16 @@ async function settingsPage(req, res) {
  // if not already enabled, use a split test to determine whether to offer personal access tokens
  let optionalPersonalAccessToken = false
  if (!showPersonalAccessToken) {
-    try {
-      const { variant } = await SplitTestHandler.promises.getAssignment(
-        req,
-        res,
-        'personal-access-token'
-      )
-      optionalPersonalAccessToken = variant === 'enabled' // `?personal-access-token=enabled`
-    } catch (error) {
-      logger.error(
-        { err: error },
-        'Failed to get personal-access-token split test assignment'
-      )
-    }
+    const { variant } = await SplitTestHandler.promises.getAssignment(
+      req,
+      res,
+      'personal-access-token'
+    )
+    optionalPersonalAccessToken = variant === 'enabled' // `?personal-access-token=enabled`
  }

-  // eslint-disable-next-line no-unused-vars -- getAssignment sets res.locals, which will pass to the splitTest context
-  const writefullSplitTest = await SplitTestHandler.promises.getAssignment(
+  // getAssignment sets res.locals, which will pass to the splitTest context
+  await SplitTestHandler.promises.getAssignment(
    req,
    res,
    'writefull-integration'