diff --git a/services/web/app/src/Features/Helpers/NewLogsUI.js b/services/web/app/src/Features/Helpers/NewLogsUI.js deleted file mode 100644 index c7736c179a..0000000000 --- a/services/web/app/src/Features/Helpers/NewLogsUI.js +++ /dev/null @@ -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, -} diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index da94e83001..645f7fb81e 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/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, diff --git a/services/web/app/views/project/editor.pug b/services/web/app/views/project/editor.pug index 7caede1fbe..641ed58c38 100644 --- a/services/web/app/views/project/editor.pug +++ b/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) diff --git a/services/web/app/views/project/editor/pdf.pug b/services/web/app/views/project/editor/pdf.pug index c6cc780c13..57e4eafe33 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 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") diff --git a/services/web/frontend/js/ide/pdf/controllers/PdfController.js b/services/web/frontend/js/ide/pdf/controllers/PdfController.js index 2e9a1f47f7..a7af09d394 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.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) } diff --git a/services/web/test/unit/src/HelperFiles/NewLogsUITests.js b/services/web/test/unit/src/HelperFiles/NewLogsUITests.js deleted file mode 100644 index 8e2d327bed..0000000000 --- a/services/web/test/unit/src/HelperFiles/NewLogsUITests.js +++ /dev/null @@ -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 - }) - }) -}) diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index bbc8b2243a..e841e60d60 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/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) }) })