From 3b9eb4e8aa08f8eaa4c7a5d9e072985ba97b8458 Mon Sep 17 00:00:00 2001 From: Alexandre Bourdin Date: Thu, 18 Jan 2024 15:25:01 +0100 Subject: [PATCH] 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 --- .../src/Features/Project/ProjectController.js | 114 ++--------------- .../Features/Project/ProjectListController.js | 109 +++++----------- .../Features/SplitTests/SplitTestHandler.js | 99 ++++++++------ .../Subscription/SubscriptionController.js | 121 +++++------------- .../src/Features/User/UserPagesController.js | 23 ++-- 5 files changed, 149 insertions(+), 317 deletions(-) diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index fd80b48f01..41e2a68324 100644 --- 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) { diff --git a/services/web/app/src/Features/Project/ProjectListController.js b/services/web/app/src/Features/Project/ProjectListController.js index 3e107fb49d..90dae541c0 100644 --- 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 } } diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index 2c575d7e13..f77f4cfbbe 100644 --- 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 + } } /** diff --git a/services/web/app/src/Features/Subscription/SubscriptionController.js b/services/web/app/src/Features/Subscription/SubscriptionController.js index ab14929706..c1ca2a94ec 100644 --- 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' diff --git a/services/web/app/src/Features/User/UserPagesController.js b/services/web/app/src/Features/User/UserPagesController.js index 0f67d632d8..77b3c21c99 100644 --- 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'