From e741eb0cb8abdb7bd59d631514e6ae736637b298 Mon Sep 17 00:00:00 2001 From: Alf Eaton Date: Thu, 18 Aug 2022 09:14:44 +0100 Subject: [PATCH] Remove PDF detach split test (#9275) GitOrigin-RevId: 9e227fe45d874e01c4282ebbe489d7bd6d2250ac --- .../src/Features/Project/ProjectController.js | 31 +--------- .../web/app/views/project/editor/meta.pug | 1 - .../editor-navigation-toolbar-root.js | 8 --- .../components/pdf-toggle-button.tsx | 34 ----------- .../components/toolbar-header.js | 15 +---- .../pdf-preview/hooks/use-compile-triggers.js | 13 ++-- .../editor-navigation-toolbar.stories.js | 3 +- .../components/toolbar-header.test.js | 42 ++++--------- .../src/Project/ProjectControllerTests.js | 61 ------------------- 9 files changed, 21 insertions(+), 187 deletions(-) delete mode 100644 services/web/frontend/js/features/editor-navigation-toolbar/components/pdf-toggle-button.tsx diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index bcc5618239..65a1b80356 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -859,22 +859,6 @@ const ProjectController = { } ) }, - pdfDetachAssignment(cb) { - SplitTestHandler.getAssignment( - req, - res, - 'pdf-detach', - {}, - (error, assignment) => { - // do not fail editor load if assignment fails - if (error) { - cb(null) - } else { - cb(null, assignment) - } - } - ) - }, pdfjsAssignment(cb) { SplitTestHandler.getAssignment( req, @@ -1044,7 +1028,6 @@ const ProjectController = { isTokenMember, brandVariation, newSourceEditorAssignment, - pdfDetachAssignment, pdfjsAssignment, dictionaryEditorAssignment, stopOnFirstErrorAssignment, @@ -1120,20 +1103,9 @@ const ProjectController = { } } - const showPdfDetach = - !Features.hasFeature('saas') || - shouldDisplayFeature( - 'pdf_detach', - pdfDetachAssignment.variant === 'enabled' - ) - const debugPdfDetach = shouldDisplayFeature('debug_pdf_detach') - let detachRole = null - - if (showPdfDetach) { - detachRole = req.params.detachRole - } + const detachRole = req.params.detachRole const showNewSourceEditorOption = newSourceEditorAssignment?.variant === 'codemirror' || @@ -1222,7 +1194,6 @@ const ProjectController = { showSupport: Features.hasFeature('support'), pdfjsVariant: pdfjsAssignment.variant, dictionaryEditorEnabled, - showPdfDetach, debugPdfDetach, showNewSourceEditorOption, showSymbolPalette, diff --git a/services/web/app/views/project/editor/meta.pug b/services/web/app/views/project/editor/meta.pug index af090b1b15..7d8c7c9b69 100644 --- a/services/web/app/views/project/editor/meta.pug +++ b/services/web/app/views/project/editor/meta.pug @@ -19,7 +19,6 @@ meta(name="ol-aceBasePath" content="/js/" + lib('ace')) meta(name="ol-useShareJsHash" data-type="boolean" content=true) meta(name="ol-wsRetryHandshake" data-type="json" content=settings.wsRetryHandshake) meta(name="ol-pdfjsVariant" content=pdfjsVariant) -meta(name="ol-showPdfDetach" data-type="boolean" content=showPdfDetach) meta(name="ol-debugPdfDetach" data-type="boolean" content=debugPdfDetach) meta(name="ol-showNewSourceEditorOption" data-type="boolean" content=showNewSourceEditorOption) meta(name="ol-showSymbolPalette" data-type="boolean" content=showSymbolPalette) diff --git a/services/web/frontend/js/features/editor-navigation-toolbar/components/editor-navigation-toolbar-root.js b/services/web/frontend/js/features/editor-navigation-toolbar/components/editor-navigation-toolbar-root.js index 400b3666be..d3572502ca 100644 --- a/services/web/frontend/js/features/editor-navigation-toolbar/components/editor-navigation-toolbar-root.js +++ b/services/web/frontend/js/features/editor-navigation-toolbar/components/editor-navigation-toolbar-root.js @@ -65,7 +65,6 @@ const EditorNavigationToolbarRoot = React.memo( view, setView, setLeftMenuShown, - pdfLayout, } = useLayoutContext(layoutContextPropTypes) const { markMessagesAsRead, unreadMessageCount } = @@ -87,10 +86,6 @@ const EditorNavigationToolbarRoot = React.memo( setView(view === 'history' ? 'editor' : 'history') }, [view, setView]) - const togglePdfView = useCallback(() => { - setView(view === 'pdf' ? 'editor' : 'pdf') - }, [view, setView]) - const openShareModal = useCallback(() => { openShareProjectModal() }, [openShareProjectModal]) @@ -133,9 +128,6 @@ const EditorNavigationToolbarRoot = React.memo( renameProject={renameProject} hasRenamePermissions={permissionsLevel === 'owner'} openShareModal={openShareModal} - pdfViewIsOpen={view === 'pdf'} - pdfButtonIsVisible={pdfLayout === 'flat'} - togglePdfView={togglePdfView} trackChangesVisible={trackChangesVisible} /> ) diff --git a/services/web/frontend/js/features/editor-navigation-toolbar/components/pdf-toggle-button.tsx b/services/web/frontend/js/features/editor-navigation-toolbar/components/pdf-toggle-button.tsx deleted file mode 100644 index 6d20a39ecd..0000000000 --- a/services/web/frontend/js/features/editor-navigation-toolbar/components/pdf-toggle-button.tsx +++ /dev/null @@ -1,34 +0,0 @@ -import classNames from 'classnames' -import Tooltip from '../../../shared/components/tooltip' -import Icon from '../../../shared/components/icon' - -type PdfToggleButtonProps = { - onClick: () => void - pdfViewIsOpen?: boolean -} - -function PdfToggleButton({ onClick, pdfViewIsOpen }: PdfToggleButtonProps) { - const classes = classNames( - 'btn', - 'btn-full-height', - 'btn-full-height-no-border', - { - active: pdfViewIsOpen, - } - ) - - return ( - - {/* eslint-disable-next-line jsx-a11y/anchor-is-valid,jsx-a11y/click-events-have-key-events,jsx-a11y/interactive-supports-focus */} - - - - - ) -} - -export default PdfToggleButton diff --git a/services/web/frontend/js/features/editor-navigation-toolbar/components/toolbar-header.js b/services/web/frontend/js/features/editor-navigation-toolbar/components/toolbar-header.js index 2ed6990433..a3b5cc7e02 100644 --- a/services/web/frontend/js/features/editor-navigation-toolbar/components/toolbar-header.js +++ b/services/web/frontend/js/features/editor-navigation-toolbar/components/toolbar-header.js @@ -12,7 +12,6 @@ import ProjectNameEditableLabel from './project-name-editable-label' import TrackChangesToggleButton from './track-changes-toggle-button' import HistoryToggleButton from './history-toggle-button' import ShareProjectButton from './share-project-button' -import PdfToggleButton from './pdf-toggle-button' import importOverleafModules from '../../../../macros/import-overleaf-module.macro' const [publishModalModules] = importOverleafModules('publishModal') @@ -36,9 +35,6 @@ const ToolbarHeader = React.memo(function ToolbarHeader({ renameProject, hasRenamePermissions, openShareModal, - pdfViewIsOpen, - pdfButtonIsVisible, - togglePdfView, trackChangesVisible, }) { const { t } = useTranslation() @@ -59,12 +55,6 @@ const ToolbarHeader = React.memo(function ToolbarHeader({ )} - {!window.showPdfDetach && pdfButtonIsVisible && ( - - )} {window.showUpgradePrompt && } )} - {window.showPdfDetach && } + {!isRestrictedTokenMember && ( { - showPdfDetach - ? startOrTriggerCompile(event.detail) - : startCompile(event.detail) + startOrTriggerCompile(event.detail) }, - [startOrTriggerCompile, startCompile] + [startOrTriggerCompile] ) useEventListener('pdf:recompile', compileHandler) @@ -34,8 +29,8 @@ export default function useCompileTriggers() { 'detached' ) const setChangedAtHandler = useCallback(() => { - showPdfDetach ? setOrTriggerChangedAt(Date.now()) : setChangedAt(Date.now()) - }, [setOrTriggerChangedAt, setChangedAt]) + setOrTriggerChangedAt(Date.now()) + }, [setOrTriggerChangedAt]) useEventListener('doc:changed', setChangedAtHandler) useEventListener('doc:saved', setChangedAtHandler) // TODO: store this separately? } diff --git a/services/web/frontend/stories/editor-navigation-toolbar.stories.js b/services/web/frontend/stories/editor-navigation-toolbar.stories.js index 1c36fdb264..8e3b619440 100644 --- a/services/web/frontend/stories/editor-navigation-toolbar.stories.js +++ b/services/web/frontend/stories/editor-navigation-toolbar.stories.js @@ -1,4 +1,5 @@ import ToolbarHeader from '../js/features/editor-navigation-toolbar/components/toolbar-header' +import { ScopeDecorator } from './decorators/scope' export const UpToThreeConnectedUsers = args => { return @@ -29,7 +30,6 @@ export default { toggleHistoryOpen: { action: 'toggleHistoryOpen' }, toggleReviewPanelOpen: { action: 'toggleReviewPanelOpen' }, toggleChatOpen: { action: 'toggleChatOpen' }, - togglePdfView: { action: 'togglePdfView' }, openShareModal: { action: 'openShareModal' }, onShowLeftMenuClick: { action: 'onShowLeftMenuClick' }, }, @@ -38,4 +38,5 @@ export default { onlineUsers: [{ user_id: 'abc', name: 'overleaf' }], unreadMessageCount: 0, }, + decorators: [ScopeDecorator], } diff --git a/services/web/test/frontend/features/editor-navigation-toolbar/components/toolbar-header.test.js b/services/web/test/frontend/features/editor-navigation-toolbar/components/toolbar-header.test.js index b7f0b78a64..5d73efd895 100644 --- a/services/web/test/frontend/features/editor-navigation-toolbar/components/toolbar-header.test.js +++ b/services/web/test/frontend/features/editor-navigation-toolbar/components/toolbar-header.test.js @@ -1,7 +1,8 @@ import { expect } from 'chai' -import { render, screen } from '@testing-library/react' +import { screen } from '@testing-library/react' import ToolbarHeader from '../../../../../frontend/js/features/editor-navigation-toolbar/components/toolbar-header' +import { renderWithEditorContext } from '../../../helpers/render-with-context' describe('', function () { const defaultProps = { @@ -15,7 +16,6 @@ describe('', function () { projectName: 'test project', renameProject: () => {}, openShareModal: () => {}, - togglePdfView: () => {}, hasPublishPermissions: true, trackChangesVisible: true, handleChangeLayout: () => {}, @@ -27,7 +27,7 @@ describe('', function () { describe('cobranding logo', function () { it('is not displayed by default', function () { - render() + renderWithEditorContext() expect(screen.queryByRole('link', { name: 'variation' })).to.not.exist }) @@ -40,30 +40,14 @@ describe('', function () { logoImgUrl: 'http://cobranding/logo', }, } - render() + renderWithEditorContext() screen.getByRole('link', { name: 'variation' }) }) }) - describe('pdf toggle button', function () { - it('is not displayed by default', function () { - render() - expect(screen.queryByText('PDF')).to.not.exist - }) - - it('is displayed when "pdfButtonIsVisible" prop is set to true', function () { - const props = { - ...defaultProps, - pdfButtonIsVisible: true, - } - render() - screen.getByText('PDF') - }) - }) - describe('track changes toggle button', function () { it('is displayed by default', function () { - render() + renderWithEditorContext() screen.getByText('Review') }) @@ -72,7 +56,7 @@ describe('', function () { ...defaultProps, isRestrictedTokenMember: true, } - render() + renderWithEditorContext() expect(screen.queryByText('Review')).to.not.exist }) @@ -81,14 +65,14 @@ describe('', function () { ...defaultProps, trackChangesVisible: false, } - render() + renderWithEditorContext() expect(screen.queryByText('Review')).to.not.exist }) }) describe('History toggle button', function () { it('is displayed by default', function () { - render() + renderWithEditorContext() screen.getByText('History') }) @@ -97,14 +81,14 @@ describe('', function () { ...defaultProps, isRestrictedTokenMember: true, } - render() + renderWithEditorContext() expect(screen.queryByText('History')).to.not.exist }) }) describe('Chat toggle button', function () { it('is displayed by default', function () { - render() + renderWithEditorContext() screen.getByText('Chat') }) @@ -113,14 +97,14 @@ describe('', function () { ...defaultProps, isRestrictedTokenMember: true, } - render() + renderWithEditorContext() expect(screen.queryByText('Chat')).to.not.exist }) }) describe('Publish button', function () { it('is displayed by default', function () { - render() + renderWithEditorContext() screen.getByText('Submit') }) @@ -129,7 +113,7 @@ describe('', function () { ...defaultProps, hasPublishPermissions: false, } - render() + renderWithEditorContext() expect(screen.queryByText('Submit')).to.not.exist }) }) diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index 60e73c527e..cb29b8f8d6 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -1364,67 +1364,6 @@ describe('ProjectController', function () { }) }) - describe('feature flags', function () { - describe('showPdfDetach', function () { - describe('showPdfDetach=false', function () { - beforeEach(function () { - this.Features.hasFeature.withArgs('saas').returns(true) - }) - - it('should be false by default in SaaS', function (done) { - this.res.render = (pageName, opts) => { - expect(opts.showPdfDetach).to.be.false - done() - } - this.ProjectController.loadEditor(this.req, this.res) - }) - - it('should be true by default in Server Pro', function (done) { - this.Features.hasFeature.withArgs('saas').returns(false) - this.res.render = (pageName, opts) => { - expect(opts.showPdfDetach).to.be.true - done() - } - this.ProjectController.loadEditor(this.req, this.res) - }) - - 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.SplitTestHandler.getAssignment - .withArgs(this.req, this.res, 'pdf-detach') - .yields(null, { variant: 'enabled' }) - this.req.query.pdf_detach = 'false' - this.ProjectController.loadEditor(this.req, this.res) - }) - }) - - describe('showPdfDetach=true', function () { - it('should be true when ?pdf_detach=true', function (done) { - this.res.render = (pageName, opts) => { - expect(opts.showPdfDetach).to.be.true - done() - } - this.req.query.pdf_detach = 'true' - this.ProjectController.loadEditor(this.req, this.res) - }) - - it('should be true for alpha group', function (done) { - this.res.render = (pageName, opts) => { - expect(opts.showPdfDetach).to.be.true - done() - } - this.SplitTestHandler.getAssignment - .withArgs(this.req, this.res, 'pdf-detach') - .yields(null, { variant: 'enabled' }) - this.ProjectController.loadEditor(this.req, this.res) - }) - }) - }) - }) - describe('upgrade prompt (on header and share project modal)', function () { beforeEach(function () { // default to saas enabled