From 46d2b84103b74cb025725fccff58485db1e4d4ea Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Fri, 11 Feb 2022 09:39:05 -0500 Subject: [PATCH] Merge pull request #6727 from overleaf/em-revert-logs-ui-split-test Revert "Move new logs UI toggle to a split test" GitOrigin-RevId: 5f2aecb53d2df21c89da594b0a64c8cf8ca04bc6 --- .../web/app/src/Features/Helpers/NewLogsUI.js | 63 ++++++++ .../src/Features/Project/ProjectController.js | 25 +-- services/web/app/views/project/editor.pug | 3 +- services/web/app/views/project/editor/pdf.pug | 6 +- .../js/ide/pdf/controllers/PdfController.js | 7 +- .../unit/src/HelperFiles/NewLogsUITests.js | 144 ++++++++++++++++++ .../src/Project/ProjectControllerTests.js | 6 + 7 files changed, 230 insertions(+), 24 deletions(-) create mode 100644 services/web/app/src/Features/Helpers/NewLogsUI.js create mode 100644 services/web/test/unit/src/HelperFiles/NewLogsUITests.js diff --git a/services/web/app/src/Features/Helpers/NewLogsUI.js b/services/web/app/src/Features/Helpers/NewLogsUI.js new file mode 100644 index 0000000000..c7736c179a --- /dev/null +++ b/services/web/app/src/Features/Helpers/NewLogsUI.js @@ -0,0 +1,63 @@ +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, +} diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index 645f7fb81e..a61a257d02 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -36,6 +36,7 @@ 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') @@ -828,21 +829,6 @@ const ProjectController = { } ) }, - 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, @@ -856,7 +842,6 @@ const ProjectController = { newPdfPreviewAssignment, newSourceEditorAssignment, pdfDetachAssignment, - logsUIAssignment, } ) => { if (err != null) { @@ -921,6 +906,8 @@ const ProjectController = { }) } + const logsUIVariant = getNewLogsUIVariantForUser(user) + function shouldDisplayFeature(name, variantFlag) { if (req.query && req.query[name]) { return req.query[name] === 'true' @@ -1017,7 +1004,11 @@ const ProjectController = { gitBridgePublicBaseUrl: Settings.gitBridgePublicBaseUrl, wsUrl, showSupport: Features.hasFeature('support'), - logsUIVariant: logsUIAssignment.variant, + showNewLogsUI: shouldDisplayFeature( + 'new_logs_ui', + logsUIVariant.newLogsUI + ), + logsUISubvariant: logsUIVariant.subvariant, showPdfDetach, debugPdfDetach, showNewPdfPreview, diff --git a/services/web/app/views/project/editor.pug b/services/web/app/views/project/editor.pug index 641ed58c38..7caede1fbe 100644 --- a/services/web/app/views/project/editor.pug +++ b/services/web/app/views/project/editor.pug @@ -141,7 +141,8 @@ 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-logsUIVariant" data-type="boolean" content=logsUIVariant) + meta(name="ol-showNewLogsUI" data-type="boolean" content=showNewLogsUI) + meta(name="ol-logsUISubvariant" content=logsUISubvariant) 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) diff --git a/services/web/app/views/project/editor/pdf.pug b/services/web/app/views/project/editor/pdf.pug index 57e4eafe33..c6cc780c13 100644 --- a/services/web/app/views/project/editor/pdf.pug +++ b/services/web/app/views/project/editor/pdf.pug @@ -1,5 +1,5 @@ div.full-size.pdf(ng-controller="PdfController") - if logsUIVariant !== 'default' + if showNewLogsUI 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="logsUIVariant === 'new-logs-ui-with-popup'" + variant-with-first-error-popup="logsUISubvariant === '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 logsUIVariant === 'default' + if !showNewLogsUI .pdf-validation-problems(ng-switch-when="validation-problems") .alert.alert-danger(ng-show="pdf.validation.sizeCheck") diff --git a/services/web/frontend/js/ide/pdf/controllers/PdfController.js b/services/web/frontend/js/ide/pdf/controllers/PdfController.js index a7af09d394..2e9a1f47f7 100644 --- a/services/web/frontend/js/ide/pdf/controllers/PdfController.js +++ b/services/web/frontend/js/ide/pdf/controllers/PdfController.js @@ -36,7 +36,7 @@ App.controller( $scope.pdf.clearingCache = false $scope.shouldShowLogs = false - $scope.logsUIVariant = window.logsUIVariant + $scope.logsUISubvariant = window.logsUISubvariant // 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.logsUIVariant !== 'default') { + if (window.showNewLogsUI) { $scope.$watch( () => $scope.pdf.view === 'errors' || @@ -732,7 +732,8 @@ App.controller( errors: $scope.pdf.logEntries.errors.length, warnings: $scope.pdf.logEntries.warnings.length, typesetting: $scope.pdf.logEntries.typesetting.length, - logsUIVariant: window.logsUIVariant, + newLogsUI: window.showNewLogsUI, + subvariant: window.showNewLogsUI ? window.logsUISubvariant : null, } eventTracking.sendMBSampled('compile-result', metadata, 0.01) } diff --git a/services/web/test/unit/src/HelperFiles/NewLogsUITests.js b/services/web/test/unit/src/HelperFiles/NewLogsUITests.js new file mode 100644 index 0000000000..8e2d327bed --- /dev/null +++ b/services/web/test/unit/src/HelperFiles/NewLogsUITests.js @@ -0,0 +1,144 @@ +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 + }) + }) +}) diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index e841e60d60..be34fbe618 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -123,6 +123,11 @@ 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' }), @@ -170,6 +175,7 @@ describe('ProjectController', function () { '../../infrastructure/Modules': { hooks: { fire: sinon.stub().yields(null, []) }, }, + '../Helpers/NewLogsUI': this.NewLogsUIHelper, '../Spelling/SpellingHandler': { getUserDictionary: sinon.stub().yields(null, []), },