Merge pull request #6640 from overleaf/em-alpha-beta-split-tests

Move alpha/beta feature toggles to split tests

GitOrigin-RevId: e64387d9bde2db9c4e96ea7f5992c877ad16b9a9

services/web/app/src/Features/Helpers/NewLogsUI.js

@@ -1,63 +0,0 @@
-const { ObjectId } = require('mongodb')
-const Settings = require('@overleaf/settings')
-
-const EXISTING_UI = { newLogsUI: false, subvariant: null }
-const NEW_UI_WITH_POPUP = {
-  newLogsUI: true,
-  subvariant: 'new-logs-ui-with-popup',
-}
-const NEW_UI_WITHOUT_POPUP = {
-  newLogsUI: true,
-  subvariant: 'new-logs-ui-without-popup',
-}
-
-function _getVariantForPercentile(percentile) {
-  // The current percentages are:
-  // - 33% New UI with pop-up (originally, 5%)
-  // - 33% New UI without pop-up (originally, 5%)
-  // - 34% Existing UI
-  // To ensure group stability, the implementation below respects the original partitions
-  // for the new UI variants: [0, 5[ and [5,10[.
-  // Two new partitions are added: [10, 38[ and [38, 66[. These represent an extra 28p.p.
-  // which, with to the original 5%, add up to 33%.
-
-  if (percentile < 5) {
-    // This partition represents the "New UI with pop-up" group in the original roll-out (5%)
-    return NEW_UI_WITH_POPUP
-  } else if (percentile >= 5 && percentile < 10) {
-    // This partition represents the "New UI without pop-up" group in the original roll-out (5%)
-    return NEW_UI_WITHOUT_POPUP
-  } else if (percentile >= 10 && percentile < 38) {
-    // This partition represents an extra 28% of users getting the "New UI with pop-up"
-    return NEW_UI_WITH_POPUP
-  } else if (percentile >= 38 && percentile < 66) {
-    // This partition represents an extra 28% of users getting the "New UI without pop-up"
-    return NEW_UI_WITHOUT_POPUP
-  } else {
-    return EXISTING_UI
-  }
-}
-
-// eslint-disable-next-line no-unused-vars
-function getNewLogsUIVariantForUser(user) {
-  const { _id: userId, alphaProgram: isAlphaUser } = user
-  const isSaaS = Boolean(Settings.overleaf)
-
-  if (!userId || !isSaaS) {
-    return EXISTING_UI
-  }
-
-  const userIdAsPercentile = (ObjectId(userId).getTimestamp() / 1000) % 100
-
-  if (isAlphaUser) {
-    return NEW_UI_WITH_POPUP
-  } else {
-    return _getVariantForPercentile(userIdAsPercentile)
-  }
-}
-
-module.exports = {
-  // We're disabling the split tests while rolling out the PDF Preview
-  // https://github.com/overleaf/internal/issues/5553
-  getNewLogsUIVariantForUser: () => EXISTING_UI,
-}

services/web/app/src/Features/Project/ProjectController.js

@@ -36,7 +36,6 @@ const UserController = require('../User/UserController')
 const AnalyticsManager = require('../Analytics/AnalyticsManager')
 const Modules = require('../../infrastructure/Modules')
 const SplitTestHandler = require('../SplitTests/SplitTestHandler')
-const { getNewLogsUIVariantForUser } = require('../Helpers/NewLogsUI')
 const FeaturesUpdater = require('../Subscription/FeaturesUpdater')
 const SpellingHandler = require('../Spelling/SpellingHandler')
 const UserPrimaryEmailCheckHandler = require('../User/UserPrimaryEmailCheckHandler')
@@ -814,6 +813,36 @@ const ProjectController = {
             }
           )
         },
+        pdfDetachAssignment(cb) {
+          SplitTestHandler.getAssignment(
+            req,
+            'pdf-detach',
+            {},
+            (error, assignment) => {
+              // do not fail editor load if assignment fails
+              if (error) {
+                cb(null)
+              } else {
+                cb(null, assignment)
+              }
+            }
+          )
+        },
+        logsUIAssignment(cb) {
+          SplitTestHandler.getAssignment(
+            req,
+            'logs-ui',
+            {},
+            (error, assignment) => {
+              // do not fail editor load if assignment fails
+              if (error) {
+                cb(null, { variant: 'default' })
+              } else {
+                cb(null, assignment)
+              }
+            }
+          )
+        },
       },
       (
         err,
@@ -826,6 +855,8 @@ const ProjectController = {
           brandVariation,
           newPdfPreviewAssignment,
           newSourceEditorAssignment,
+          pdfDetachAssignment,
+          logsUIAssignment,
         }
       ) => {
         if (err != null) {
@@ -890,8 +921,6 @@ const ProjectController = {
               })
             }
 
-            const logsUIVariant = getNewLogsUIVariantForUser(user)
-
             function shouldDisplayFeature(name, variantFlag) {
               if (req.query && req.query[name]) {
                 return req.query[name] === 'true'
@@ -918,7 +947,7 @@ const ProjectController = {
 
             const showPdfDetach = shouldDisplayFeature(
               'pdf_detach',
-              user.alphaProgram
+              pdfDetachAssignment.variant === 'enabled'
             )
 
             const debugPdfDetach = shouldDisplayFeature('debug_pdf_detach')
@@ -988,11 +1017,7 @@ const ProjectController = {
               gitBridgePublicBaseUrl: Settings.gitBridgePublicBaseUrl,
               wsUrl,
               showSupport: Features.hasFeature('support'),
-              showNewLogsUI: shouldDisplayFeature(
-                'new_logs_ui',
-                logsUIVariant.newLogsUI
-              ),
-              logsUISubvariant: logsUIVariant.subvariant,
+              logsUIVariant: logsUIAssignment.variant,
               showPdfDetach,
               debugPdfDetach,
               showNewPdfPreview,

services/web/app/views/project/editor.pug

@@ -141,8 +141,7 @@ block append meta
 	//- used in public/js/libs/sharejs.js
 	meta(name="ol-useShareJsHash" data-type="boolean" content=true)
 	meta(name="ol-wsRetryHandshake" data-type="json" content=settings.wsRetryHandshake)
-	meta(name="ol-showNewLogsUI" data-type="boolean" content=showNewLogsUI)
-	meta(name="ol-logsUISubvariant" content=logsUISubvariant)
+	meta(name="ol-logsUIVariant" data-type="boolean" content=logsUIVariant)
 	meta(name="ol-showPdfDetach" data-type="boolean" content=showPdfDetach)
 	meta(name="ol-debugPdfDetach" data-type="boolean" content=debugPdfDetach)
 	meta(name="ol-showNewPdfPreview" data-type="boolean" content=showNewPdfPreview)

services/web/app/views/project/editor/pdf.pug

@@ -1,5 +1,5 @@
 div.full-size.pdf(ng-controller="PdfController")
-	if showNewLogsUI
+	if logsUIVariant !== 'default'
 		preview-pane(
 			compiler-state=`{
 				autoCompileHasChanges: changesToAutoCompile,
@@ -43,7 +43,7 @@ div.full-size.pdf(ng-controller="PdfController")
 			on-set-split-layout="setPdfSplitLayout"
 			on-set-full-layout="setPdfFullLayout"
 			on-stop-compilation="stop"
-			variant-with-first-error-popup="logsUISubvariant === 'new-logs-ui-with-popup'"
+			variant-with-first-error-popup="logsUIVariant === 'new-logs-ui-with-popup'"
 			show-logs="shouldShowLogs"
 			on-log-entry-location-click="openInEditor"
 		)
@@ -303,7 +303,7 @@ div.full-size.pdf(ng-controller="PdfController")
 				ng-if="settings.pdfViewer == 'native'"
 			)
 
-		if !showNewLogsUI
+		if logsUIVariant === 'default'
 			.pdf-validation-problems(ng-switch-when="validation-problems")
 
 				.alert.alert-danger(ng-show="pdf.validation.sizeCheck")

services/web/frontend/js/ide/pdf/controllers/PdfController.js

@@ -36,7 +36,7 @@ App.controller(
     $scope.pdf.clearingCache = false
     $scope.shouldShowLogs = false
 
-    $scope.logsUISubvariant = window.logsUISubvariant
+    $scope.logsUIVariant = window.logsUIVariant
 
     // view logic to check whether the files dropdown should "drop up" or "drop down"
     $scope.shouldDropUp = false
@@ -538,7 +538,7 @@ App.controller(
     // In the existing compile UI, errors and validation problems are shown in the PDF pane, whereas in the new
     // one they're shown in the logs pane. This `$watch`er makes sure we change the view in the new logs UI.
     // This should be removed once we stop supporting the two different log UIs.
-    if (window.showNewLogsUI) {
+    if (window.logsUIVariant !== 'default') {
       $scope.$watch(
         () =>
           $scope.pdf.view === 'errors' ||
@@ -732,8 +732,7 @@ App.controller(
           errors: $scope.pdf.logEntries.errors.length,
           warnings: $scope.pdf.logEntries.warnings.length,
           typesetting: $scope.pdf.logEntries.typesetting.length,
-          newLogsUI: window.showNewLogsUI,
-          subvariant: window.showNewLogsUI ? window.logsUISubvariant : null,
+          logsUIVariant: window.logsUIVariant,
         }
         eventTracking.sendMBSampled('compile-result', metadata, 0.01)
       }

services/web/test/unit/src/HelperFiles/NewLogsUITests.js

@@ -1,144 +0,0 @@
-const SandboxedModule = require('sandboxed-module')
-const { expect } = require('chai')
-const { ObjectId } = require('mongodb')
-const MODULE_PATH = require('path').join(
-  __dirname,
-  '../../../../app/src/Features/Helpers/NewLogsUI.js'
-)
-
-describe('NewLogsUI helper', function () {
-  let NewLogsUI
-
-  before(function () {
-    // We're disabling the Logs UI split test while rolling out the PDF Preview
-    this.skip()
-  })
-
-  function userIdFromTime(time) {
-    return ObjectId.createFromTime(time).toString()
-  }
-
-  function isExistingUI(variant) {
-    return !variant.newLogsUI && !variant.subvariant
-  }
-
-  function isNewUIWithPopup(variant) {
-    return variant.newLogsUI && variant.subvariant === 'new-logs-ui-with-popup'
-  }
-
-  function isNewUIWithoutPopup(variant) {
-    return (
-      variant.newLogsUI && variant.subvariant === 'new-logs-ui-without-popup'
-    )
-  }
-
-  function getTestInterval(lowerBoundary, upperBoundary) {
-    const midpoint = Math.floor(
-      lowerBoundary + (upperBoundary - lowerBoundary) / 2
-    )
-    return [lowerBoundary, midpoint, upperBoundary]
-  }
-
-  beforeEach(function () {
-    this.user = {
-      alphaProgram: false,
-      _id: ObjectId('60085414b76eeb00737d93aa'),
-    }
-    this.settings = {
-      overleaf: {
-        foo: 'bar',
-      },
-    }
-
-    NewLogsUI = SandboxedModule.require(MODULE_PATH, {
-      requires: {
-        mongodb: { ObjectId },
-        '@overleaf/settings': this.settings,
-      },
-    })
-  })
-
-  describe('In a non-SaaS context', function () {
-    beforeEach(function () {
-      delete this.settings.overleaf
-    })
-    it('should always show the existing UI', function () {
-      for (const percentile of [0, 20, 40, 60, 80]) {
-        this.user._id = userIdFromTime(percentile)
-        const variant = NewLogsUI.getNewLogsUIVariantForUser(this.user)
-        expect(isExistingUI(variant)).to.be.true
-      }
-    })
-  })
-
-  describe('For alpha users', function () {
-    beforeEach(function () {
-      this.user.alphaProgram = true
-    })
-    it('should always show the new UI with popup', function () {
-      for (const percentile of [0, 20, 40, 60, 80]) {
-        this.user._id = userIdFromTime(percentile)
-        const variant = NewLogsUI.getNewLogsUIVariantForUser(this.user)
-        expect(isNewUIWithPopup(variant)).to.be.true
-      }
-    })
-  })
-
-  describe('For regular users', function () {
-    it('should show the new UI with popup when the id is in the [0, 5[ interval', function () {
-      const testInterval = getTestInterval(0, 4)
-      for (const percentile of testInterval) {
-        this.user._id = userIdFromTime(percentile)
-        const variant = NewLogsUI.getNewLogsUIVariantForUser(this.user)
-        expect(isNewUIWithPopup(variant)).to.be.true
-      }
-      this.user._id = userIdFromTime(5)
-      const variant = NewLogsUI.getNewLogsUIVariantForUser(this.user)
-      expect(isNewUIWithPopup(variant)).to.be.false
-    })
-    it('should show the new UI without popup when the id is in the [5, 10[ interval', function () {
-      const testInterval = getTestInterval(5, 9)
-      for (const percentile of testInterval) {
-        this.user._id = userIdFromTime(percentile)
-        const variant = NewLogsUI.getNewLogsUIVariantForUser(this.user)
-        expect(isNewUIWithoutPopup(variant)).to.be.true
-      }
-      this.user._id = userIdFromTime(10)
-      const variant = NewLogsUI.getNewLogsUIVariantForUser(this.user)
-      expect(isNewUIWithoutPopup(variant)).to.be.false
-    })
-    it('should show the new UI with popup when the id is in the [10, 38[ interval', function () {
-      const testInterval = getTestInterval(10, 37)
-      for (const percentile of testInterval) {
-        this.user._id = userIdFromTime(percentile)
-        const variant = NewLogsUI.getNewLogsUIVariantForUser(this.user)
-        expect(isNewUIWithPopup(variant)).to.be.true
-      }
-      this.user._id = userIdFromTime(38)
-      const variant = NewLogsUI.getNewLogsUIVariantForUser(this.user)
-      expect(isNewUIWithPopup(variant)).to.be.false
-    })
-    it('should show the new UI without popup when the id is in the [38, 66[ interval', function () {
-      const testInterval = getTestInterval(38, 65)
-      for (const percentile of testInterval) {
-        this.user._id = userIdFromTime(percentile)
-        const variant = NewLogsUI.getNewLogsUIVariantForUser(this.user)
-        expect(isNewUIWithoutPopup(variant)).to.be.true
-      }
-      this.user._id = userIdFromTime(66)
-      const variant = NewLogsUI.getNewLogsUIVariantForUser(this.user)
-      expect(isNewUIWithoutPopup(variant)).to.be.false
-    })
-    it('should show the existing UI when the id is in the [66, 99] interval', function () {
-      const testInterval = getTestInterval(66, 99)
-      for (const percentile of testInterval) {
-        this.user._id = userIdFromTime(percentile)
-        const variant = NewLogsUI.getNewLogsUIVariantForUser(this.user)
-        expect(isExistingUI(variant)).to.be.true
-      }
-      this.user._id = userIdFromTime(100)
-      const variant = NewLogsUI.getNewLogsUIVariantForUser(this.user)
-      expect(isExistingUI(variant)).to.be.false
-    })
-  })
-})

services/web/test/unit/src/Project/ProjectControllerTests.js

@@ -123,11 +123,6 @@ describe('ProjectController', function () {
       },
       inc: sinon.stub(),
     }
-    this.NewLogsUIHelper = {
-      getNewLogsUIVariantForUser: sinon
-        .stub()
-        .returns({ newLogsUI: false, subvariant: null }),
-    }
     this.SplitTestHandler = {
       promises: {
         getAssignment: sinon.stub().resolves({ variant: 'default' }),
@@ -175,7 +170,6 @@ describe('ProjectController', function () {
         '../../infrastructure/Modules': {
           hooks: { fire: sinon.stub().yields(null, []) },
         },
-        '../Helpers/NewLogsUI': this.NewLogsUIHelper,
         '../Spelling/SpellingHandler': {
           getUserDictionary: sinon.stub().yields(null, []),
         },
@@ -1411,6 +1405,7 @@ describe('ProjectController', function () {
           }
           this.ProjectController.loadEditor(this.req, this.res)
         })
+
         it('should be true when ?new_pdf_preview=true ', function (done) {
           this.res.render = (pageName, opts) => {
             expect(opts.showNewPdfPreview).to.be.true
@@ -1419,24 +1414,31 @@ describe('ProjectController', function () {
           this.req.query.new_pdf_preview = 'true'
           this.ProjectController.loadEditor(this.req, this.res)
         })
-        it('should be true for alpha group', function (done) {
+
+        it('should be true when the react-pdf-preview-rollout split test is enabled', function (done) {
           this.res.render = (pageName, opts) => {
             expect(opts.showNewPdfPreview).to.be.true
             done()
           }
-          this.user.alphaProgram = true
+          this.SplitTestHandler.getAssignment
+            .withArgs(this.req, 'react-pdf-preview-rollout')
+            .yields(null, { variant: 'react-pdf-preview' })
           this.ProjectController.loadEditor(this.req, this.res)
         })
-        it('should be false when when ?new_pdf_preview=true and alpha group', function (done) {
+
+        it('should be false when when ?new_pdf_preview=false and the split test is enabled', function (done) {
           this.res.render = (pageName, opts) => {
-            expect(opts.showNewPdfPreview).to.be.true
+            expect(opts.showNewPdfPreview).to.be.false
             done()
           }
-          this.user.alphaProgram = true
+          this.SplitTestHandler.getAssignment
+            .withArgs(this.req, 'react-pdf-preview-rollout')
+            .yields(null, { variant: 'react-pdf-preview' })
           this.req.query.new_pdf_preview = 'false'
           this.ProjectController.loadEditor(this.req, this.res)
         })
       })
+
       describe('showPdfDetach', function () {
         describe('showPdfDetach=false', function () {
           it('should be false by default', function (done) {
@@ -1446,6 +1448,7 @@ describe('ProjectController', function () {
             }
             this.ProjectController.loadEditor(this.req, this.res)
           })
+
           it('should be false by default, even when ?new_pdf_preview=true', function (done) {
             this.res.render = (pageName, opts) => {
               expect(opts.showPdfDetach).to.be.false
@@ -1455,11 +1458,15 @@ describe('ProjectController', function () {
             this.req.query.new_pdf_preview = 'true'
             this.ProjectController.loadEditor(this.req, this.res)
           })
-          it('should be false when when ?pdf_detach=true and alpha group', function (done) {
+
+          it('should be false when the split test is enabled and ?pdf_detach=false', function (done) {
             this.res.render = (pageName, opts) => {
+              expect(opts.showPdfDetach).to.be.false
               done()
             }
-            this.user.alphaProgram = true
+            this.SplitTestHandler.getAssignment
+              .withArgs(this.req, 'pdf-detach')
+              .yields(null, { variant: 'enabled' })
             this.req.query.pdf_detach = 'false'
             this.ProjectController.loadEditor(this.req, this.res)
           })
@@ -1475,13 +1482,16 @@ describe('ProjectController', function () {
             this.req.query.pdf_detach = 'true'
             this.ProjectController.loadEditor(this.req, this.res)
           })
+
           it('should be true for alpha group, and set showNewPdfPreview as true', function (done) {
             this.res.render = (pageName, opts) => {
               expect(opts.showPdfDetach).to.be.true
               expect(opts.showNewPdfPreview).to.be.true
               done()
             }
-            this.user.alphaProgram = true
+            this.SplitTestHandler.getAssignment
+              .withArgs(this.req, 'pdf-detach')
+              .yields(null, { variant: 'enabled' })
             this.ProjectController.loadEditor(this.req, this.res)
           })
         })