From 006a140fb8a6b20f433732b0019d88ccb06f11e6 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 24 Aug 2021 11:52:38 +0200 Subject: [PATCH] Merge pull request #4645 from overleaf/jpa-pdf-caching-opt-in [misc] make PDF caching an opt-in feature GitOrigin-RevId: 85ea5739d7bbeea3ac2517ec99f90f2beec2a768 --- .../src/Features/Project/ProjectController.js | 43 +++---------------- .../src/Project/ProjectControllerTests.js | 26 +++++++++++ 2 files changed, 33 insertions(+), 36 deletions(-) diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index f9dd6358e8..1f1c4724ab 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -37,7 +37,6 @@ const BrandVariationsHandler = require('../BrandVariations/BrandVariationsHandle const UserController = require('../User/UserController') const AnalyticsManager = require('../Analytics/AnalyticsManager') const Modules = require('../../infrastructure/Modules') -const SplitTestHandler = require('../SplitTests/SplitTestHandler') const SplitTestV2Handler = require('../SplitTests/SplitTestV2Handler') const { getNewLogsUIVariantForUser } = require('../Helpers/NewLogsUI') @@ -711,21 +710,6 @@ const ProjectController = { flushToTpds: cb => { TpdsProjectFlusher.flushProjectToTpdsIfNeeded(projectId, cb) }, - pdfCachingFeatureFlag(cb) { - if (!Settings.enablePdfCaching) return cb(null, '') - if (!userId) return cb(null, 'enable-caching-only') - SplitTestHandler.getTestSegmentation( - userId, - 'pdf_caching_full', - (err, segmentation) => { - if (err) { - // Do not fail loading the editor. - return cb(null, '') - } - cb(null, (segmentation && segmentation.variant) || '') - } - ) - }, sharingModalSplitTest(cb) { SplitTestV2Handler.assignInLocalsContext( res, @@ -737,17 +721,7 @@ const ProjectController = { ) }, }, - ( - err, - { - project, - user, - subscription, - isTokenMember, - brandVariation, - pdfCachingFeatureFlag, - } - ) => { + (err, { project, user, subscription, isTokenMember, brandVariation }) => { if (err != null) { OError.tag(err, 'error getting details for project page') return next(err) @@ -826,15 +800,10 @@ const ProjectController = { // The feature is disabled globally. return false } - const canSeeFeaturePreview = pdfCachingFeatureFlag.includes(flag) - if (!canSeeFeaturePreview) { - // The user is not in the target group. - return false - } - // Optionally let the user opt-out. - // The will opt-out of both caching and metrics collection, + // Let the user opt-in only. + // The flag will opt-out of both caching and metrics collection, // as if this editing session never happened. - return shouldDisplayFeature('enable_pdf_caching', true) + return shouldDisplayFeature('enable_pdf_caching', false) } res.render('project/editor', { @@ -909,7 +878,9 @@ const ProjectController = { ), trackPdfDownload: partOfPdfCachingRollout('collect-metrics'), enablePdfCaching: partOfPdfCachingRollout('enable-caching'), - resetServiceWorker: Boolean(Settings.resetServiceWorker), + resetServiceWorker: + Boolean(Settings.resetServiceWorker) && + !shouldDisplayFeature('enable_pdf_caching', false), }) timer.done() } diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index c5f577401b..f111801131 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -1237,6 +1237,9 @@ describe('ProjectController', function () { }) describe('during regular roll-out', function () { + before(function () { + this.skip() + }) describe('disabled', function () { showNoVariant() @@ -1285,6 +1288,29 @@ describe('ProjectController', function () { }) }) }) + + describe('during opt-in only', function () { + describe('with no query', function () { + expectBandwidthTrackingDisabled() + expectPDFCachingDisabled() + }) + + describe('with enable_pdf_caching=false', function () { + beforeEach(function () { + this.req.query.enable_pdf_caching = 'false' + }) + expectBandwidthTrackingDisabled() + expectPDFCachingDisabled() + }) + + describe('with enable_pdf_caching=true', function () { + beforeEach(function () { + this.req.query.enable_pdf_caching = 'true' + }) + expectBandwidthTrackingEnabled() + expectPDFCachingEnabled() + }) + }) }) describe('wsUrl', function () {