From 6b03becff15f07756f483d5028557aa5d40f3770 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Tue, 10 Aug 2021 10:35:16 -0700 Subject: [PATCH] chore: pr comments --- src/lib/Browser.js | 8 ++ src/lib/viewers/doc/DocBaseViewer.js | 12 +-- src/lib/viewers/doc/DocumentViewer.js | 16 ++++ .../doc/__tests__/DocBaseViewer-test.js | 38 +++------- .../doc/__tests__/DocumentViewer-test.js | 74 +++++++++++++++++++ src/lib/viewers/office/OfficeLoader.js | 6 +- 6 files changed, 113 insertions(+), 41 deletions(-) diff --git a/src/lib/Browser.js b/src/lib/Browser.js index 10d834313..ed5f2b61e 100644 --- a/src/lib/Browser.js +++ b/src/lib/Browser.js @@ -352,6 +352,14 @@ class Browser { }, }; } + + /** + * Returns whether the browser is Internet Explorer + * @returns {boolean} - Whether the browser is Internet Explorer + */ + static isIE() { + return Browser.getName() === BROWSERS.INTERNET_EXPLORER; + } } export default Browser; diff --git a/src/lib/viewers/doc/DocBaseViewer.js b/src/lib/viewers/doc/DocBaseViewer.js index 288685e0a..976e1a9b3 100644 --- a/src/lib/viewers/doc/DocBaseViewer.js +++ b/src/lib/viewers/doc/DocBaseViewer.js @@ -9,11 +9,9 @@ import Popup from '../../Popup'; import PreviewError from '../../PreviewError'; import RepStatus from '../../RepStatus'; import ThumbnailsSidebar from '../../ThumbnailsSidebar'; -import Timer from '../../Timer'; import { AnnotationInput, AnnotationMode, AnnotationState } from '../../AnnotationControlsFSM'; import { ANNOTATOR_EVENT, - BROWSERS, CLASS_ANNOTATIONS_DOCUMENT_FTUX_CURSOR_SEEN, CLASS_BOX_PREVIEW_THUMBNAILS_CLOSE_ACTIVE, CLASS_BOX_PREVIEW_THUMBNAILS_CLOSE, @@ -44,7 +42,7 @@ import { USER_DOCUMENT_THUMBNAIL_EVENTS, VIEWER_EVENT, } from '../../events'; -import { EXCEL_EXTENSIONS } from '../../extensions'; +import Timer from '../../Timer'; export const DISCOVERABILITY_STATES = [ AnnotationState.HIGHLIGHT_TEMP, @@ -379,14 +377,6 @@ class DocBaseViewer extends BaseViewer { this.showPreload(); const template = this.options.representation.content.url_template; - const { extension } = this.options.file; - const isExcelExtension = EXCEL_EXTENSIONS.indexOf(extension) !== -1; - const isIE = Browser.getName() === BROWSERS.INTERNET_EXPLORER; - - if (isExcelExtension && isIE) { - this.options.ui.showNotification(__('error_internet_explorer_office_online'), null, true); - } - this.pdfUrl = this.createContentUrlWithAuthParams(template); return Promise.all([this.loadAssets(JS, CSS), this.getRepStatus().getPromise()]) diff --git a/src/lib/viewers/doc/DocumentViewer.js b/src/lib/viewers/doc/DocumentViewer.js index 33b90c2da..503fa9181 100644 --- a/src/lib/viewers/doc/DocumentViewer.js +++ b/src/lib/viewers/doc/DocumentViewer.js @@ -1,6 +1,8 @@ +import Browser from '../../Browser'; import DocBaseViewer from './DocBaseViewer'; import DocPreloader from './DocPreloader'; import fullscreen from '../../Fullscreen'; +import { EXCEL_EXTENSIONS } from '../../extensions'; import './Document.scss'; class DocumentViewer extends DocBaseViewer { @@ -33,6 +35,20 @@ class DocumentViewer extends DocBaseViewer { this.preloader.removeAllListeners('preload'); } + /** + * @inheritdoc + */ + load() { + super.load(); + + const { extension } = this.options.file; + const isExcelExtension = EXCEL_EXTENSIONS.includes(extension); + + if (isExcelExtension && Browser.isIE()) { + this.options.ui.showNotification(__('error_internet_explorer_office_online'), null, true); + } + } + /** * Handles keyboard events for document viewer. * diff --git a/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js b/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js index bcfe6dae0..5d2d65de1 100644 --- a/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js +++ b/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js @@ -16,20 +16,19 @@ import DocPreloader from '../DocPreloader'; import fullscreen from '../../../Fullscreen'; import { ANNOTATOR_EVENT, - BROWSERS, - CLASS_ANNOTATIONS_DOCUMENT_FTUX_CURSOR_SEEN, - CLASS_BOX_PREVIEW_THUMBNAILS_CONTAINER, - CLASS_BOX_PREVIEW_THUMBNAILS_OPEN, CLASS_HIDDEN, DOCUMENT_FTUX_CURSOR_SEEN_KEY, - ENCODING_TYPES, PERMISSION_DOWNLOAD, - QUERY_PARAM_ENCODING, - SELECTOR_BOX_PREVIEW_CONTENT, - SELECTOR_BOX_PREVIEW, STATUS_ERROR, STATUS_PENDING, STATUS_SUCCESS, + QUERY_PARAM_ENCODING, + ENCODING_TYPES, + SELECTOR_BOX_PREVIEW_CONTENT, + CLASS_ANNOTATIONS_DOCUMENT_FTUX_CURSOR_SEEN, + CLASS_BOX_PREVIEW_THUMBNAILS_CONTAINER, + CLASS_BOX_PREVIEW_THUMBNAILS_OPEN, + SELECTOR_BOX_PREVIEW, } from '../../../constants'; import { ICON_PRINT_CHECKMARK } from '../../../icons'; import { LOAD_METRIC, RENDER_EVENT, USER_DOCUMENT_THUMBNAIL_EVENTS, VIEWER_EVENT } from '../../../events'; @@ -599,7 +598,11 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => { describe('load()', () => { const loadFunc = BaseViewer.prototype.load; - beforeEach(() => { + afterEach(() => { + Object.defineProperty(BaseViewer.prototype, 'load', { value: loadFunc }); + }); + + test('should load a document', () => { jest.spyOn(stubs.api, 'get').mockImplementation(); jest.spyOn(docBase, 'setup').mockImplementation(); Object.defineProperty(BaseViewer.prototype, 'load', { value: sandbox.mock() }); @@ -608,13 +611,7 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => { jest.spyOn(docBase, 'getRepStatus').mockReturnValue({ getPromise: () => Promise.resolve() }); jest.spyOn(docBase, 'loadAssets').mockImplementation(); jest.spyOn(docBase, 'loadBoxAnnotations').mockImplementation(); - }); - afterEach(() => { - Object.defineProperty(BaseViewer.prototype, 'load', { value: loadFunc }); - }); - - test('should load a document', () => { return docBase.load().then(() => { expect(docBase.loadAssets).toBeCalled(); expect(docBase.setup).not.toBeCalled(); @@ -622,17 +619,6 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => { expect(docBase.handleAssetAndRepLoad).toBeCalled(); }); }); - - test('should show notification if file has excel extension and is internet explorer', () => { - const showNotification = jest.fn(); - docBase.options.ui = { showNotification }; - docBase.options.file.extension = 'xlsx'; - jest.spyOn(Browser, 'getName').mockImplementation(() => BROWSERS.INTERNET_EXPLORER); - - return docBase.load().then(() => { - expect(showNotification).toBeCalledWith(__('error_internet_explorer_office_online'), null, true); - }); - }); }); describe('handleAssetAndRepLoad', () => { diff --git a/src/lib/viewers/doc/__tests__/DocumentViewer-test.js b/src/lib/viewers/doc/__tests__/DocumentViewer-test.js index 238dd72c6..b82a31c0e 100644 --- a/src/lib/viewers/doc/__tests__/DocumentViewer-test.js +++ b/src/lib/viewers/doc/__tests__/DocumentViewer-test.js @@ -1,4 +1,6 @@ /* eslint-disable no-unused-expressions */ +import Api from '../../../api'; +import Browser from '../../../Browser'; import DocumentViewer from '../DocumentViewer'; import DocBaseViewer from '../DocBaseViewer'; import BaseViewer from '../../BaseViewer'; @@ -15,6 +17,8 @@ describe('lib/viewers/doc/DocumentViewer', () => { beforeEach(() => { fixture.load('viewers/doc/__tests__/DocumentViewer-test.html'); + stubs.api = new Api(); + containerEl = document.querySelector('.container'); doc = new DocumentViewer({ container: containerEl, @@ -83,6 +87,76 @@ describe('lib/viewers/doc/DocumentViewer', () => { }); }); + describe('load()', () => { + const docBaseLoadFunc = DocBaseViewer.prototype.load; + + beforeEach(() => { + jest.spyOn(stubs.api, 'get').mockImplementation(); + jest.spyOn(doc, 'setup').mockImplementation(); + Object.defineProperty(DocBaseViewer.prototype, 'load', { + value: jest.fn().mockImplementation(() => Promise.resolve()), + }); + jest.spyOn(doc, 'createContentUrlWithAuthParams').mockImplementation(); + jest.spyOn(doc, 'handleAssetAndRepLoad').mockImplementation(); + jest.spyOn(doc, 'getRepStatus').mockReturnValue({ getPromise: () => Promise.resolve() }); + jest.spyOn(doc, 'loadAssets').mockImplementation(); + jest.spyOn(doc, 'loadBoxAnnotations').mockImplementation(); + }); + + afterEach(() => { + Object.defineProperty(DocBaseViewer.prototype, 'load', { value: docBaseLoadFunc }); + }); + + test.each(['xls', 'xlsm', 'xlsx'])( + 'should show notification if file has excel extension %s and is internet explorer', + extension => { + const showNotification = jest.fn(); + doc.options.ui = { showNotification }; + doc.options.file.extension = extension; + jest.spyOn(Browser, 'isIE').mockImplementation(() => true); + + doc.load(); + + expect(showNotification).toBeCalledWith(__('error_internet_explorer_office_online'), null, true); + }, + ); + + test.each(['xls', 'xlsm', 'xlsx'])( + 'should not show notification if file has excel extension %s but is not internet explorer', + extension => { + const showNotification = jest.fn(); + doc.options.ui = { showNotification }; + doc.options.file.extension = extension; + jest.spyOn(Browser, 'isIE').mockImplementation(() => false); + + doc.load(); + + expect(showNotification).not.toBeCalledWith(__('error_internet_explorer_office_online'), null, true); + }, + ); + + test('should not show notification if file is not excel extension', () => { + const showNotification = jest.fn(); + doc.options.ui = { showNotification }; + doc.options.file.extension = 'pdf'; + + doc.load(); + + expect(showNotification).not.toBeCalledWith(__('error_internet_explorer_office_online'), null, true); + }); + + test('should not show notification if file is not excel extension and is internet explorer', () => { + const showNotification = jest.fn(); + doc.options.ui = { showNotification }; + doc.options.file.extension = 'pdf'; + jest.spyOn(Browser, 'isIE').mockImplementation(() => true); + + doc.load(); + + expect(showNotification).not.toBeCalledWith(__('error_internet_explorer_office_online'), null, true); + }); + }); + describe('onKeydown()', () => { beforeEach(() => { stubs.zoomIn = jest.spyOn(doc, 'zoomIn').mockImplementation(); diff --git a/src/lib/viewers/office/OfficeLoader.js b/src/lib/viewers/office/OfficeLoader.js index 9afba5771..93dd3dc83 100644 --- a/src/lib/viewers/office/OfficeLoader.js +++ b/src/lib/viewers/office/OfficeLoader.js @@ -3,7 +3,7 @@ import AssetLoader from '../AssetLoader'; import Browser from '../../Browser'; import OfficeViewer from './OfficeViewer'; import { checkPermission } from '../../file'; -import { BROWSERS, ORIGINAL_REP_NAME, PERMISSION_DOWNLOAD } from '../../constants'; +import { ORIGINAL_REP_NAME, PERMISSION_DOWNLOAD } from '../../constants'; const FIVE_MB = 5242880; const OFFICE_VIEWER_NAME = 'Office'; @@ -35,15 +35,13 @@ class OfficeLoader extends AssetLoader { // The Office viewer is disabled when this is a password protected shared link const isDisabledDueToPasswordProtectedSharedLink = file.shared_link && file.shared_link.is_password_enabled; const maxFileSize = getProp(viewerOptions, 'Office.maxFileSize', FIVE_MB); - // Disable Office viewer if browser is Internet Explorer due to EOL - const isIE = Browser.getName() === BROWSERS.INTERNET_EXPLORER; // If the user does not have permission to download the file, the file is larger than max file size, // or isDisabledDueToSharedLink is true, then disable the Office viewer if ( !checkPermission(file, PERMISSION_DOWNLOAD) || file.size > maxFileSize || isDisabledDueToPasswordProtectedSharedLink || - isIE + Browser.isIE() // Disable Office viewer if browser is Internet Explorer due to EOL ) { disabledViewers.push(OFFICE_VIEWER_NAME); }