Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(office): Preview PDF rep if using internet explorer #1416

Merged
merged 5 commits into from
Aug 10, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/i18n/en-US.properties
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ error_bad_file=We’re sorry the preview didn’t load. This file could not be c
error_not_downloadable=Oops! It looks like something is wrong with this file. We apologize for the inconvenience and recommend that you upload a new version of this file or roll back to a previous version. Please contact us for more details.
# Preview error message shown when flash is not enabled on their browser
error_flash_not_enabled=We’re sorry, the preview didn’t load because Flash is not enabled on your browser. If possible, please enable Flash and refresh the page.
# Preview error message shown when loading what would be an Office Excel Online file extension in Internet Explorer
error_internet_explorer_office_online=Microsoft 365 apps and services no longer support Internet Explorer 11, therefore, Box users may have a degraded experience.

# Archive Preview
# Label for filename column name
Expand Down
12 changes: 11 additions & 1 deletion src/lib/viewers/doc/DocBaseViewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ 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,
Expand Down Expand Up @@ -42,7 +44,7 @@ import {
USER_DOCUMENT_THUMBNAIL_EVENTS,
VIEWER_EVENT,
} from '../../events';
import Timer from '../../Timer';
import { EXCEL_EXTENSIONS } from '../../extensions';

export const DISCOVERABILITY_STATES = [
AnnotationState.HIGHLIGHT_TEMP,
Expand Down Expand Up @@ -377,6 +379,14 @@ class DocBaseViewer extends BaseViewer {
this.showPreload();

const template = this.options.representation.content.url_template;
const { extension } = this.options.file;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this.options.file always defined as an object? Should we move this logic to DocumentViewer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it is always passed into the viewer options:

file: this.file,

const isExcelExtension = EXCEL_EXTENSIONS.indexOf(extension) !== -1;
ConradJChan marked this conversation as resolved.
Show resolved Hide resolved
const isIE = Browser.getName() === BROWSERS.INTERNET_EXPLORER;
ConradJChan marked this conversation as resolved.
Show resolved Hide resolved

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()])
Expand Down
38 changes: 26 additions & 12 deletions src/lib/viewers/doc/__tests__/DocBaseViewer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,20 @@ 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,
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,
STATUS_ERROR,
STATUS_PENDING,
STATUS_SUCCESS,
} from '../../../constants';
import { ICON_PRINT_CHECKMARK } from '../../../icons';
import { LOAD_METRIC, RENDER_EVENT, USER_DOCUMENT_THUMBNAIL_EVENTS, VIEWER_EVENT } from '../../../events';
Expand Down Expand Up @@ -598,11 +599,7 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => {
describe('load()', () => {
const loadFunc = BaseViewer.prototype.load;

afterEach(() => {
Object.defineProperty(BaseViewer.prototype, 'load', { value: loadFunc });
});

test('should load a document', () => {
beforeEach(() => {
jest.spyOn(stubs.api, 'get').mockImplementation();
jest.spyOn(docBase, 'setup').mockImplementation();
Object.defineProperty(BaseViewer.prototype, 'load', { value: sandbox.mock() });
Expand All @@ -611,14 +608,31 @@ 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();
expect(docBase.createContentUrlWithAuthParams).toBeCalledWith('foo');
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', () => {
Expand Down
8 changes: 6 additions & 2 deletions src/lib/viewers/office/OfficeLoader.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import getProp from 'lodash/get';
import AssetLoader from '../AssetLoader';
import Browser from '../../Browser';
import OfficeViewer from './OfficeViewer';
import { checkPermission } from '../../file';
import { ORIGINAL_REP_NAME, PERMISSION_DOWNLOAD } from '../../constants';
import { BROWSERS, ORIGINAL_REP_NAME, PERMISSION_DOWNLOAD } from '../../constants';

const FIVE_MB = 5242880;
const OFFICE_VIEWER_NAME = 'Office';
Expand Down Expand Up @@ -34,12 +35,15 @@ 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
isDisabledDueToPasswordProtectedSharedLink ||
isIE
) {
disabledViewers.push(OFFICE_VIEWER_NAME);
}
Expand Down
10 changes: 9 additions & 1 deletion src/lib/viewers/office/__tests__/OfficeLoader-test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import * as file from '../../../file';
import Browser from '../../../Browser';
import OfficeLoader from '../OfficeLoader';
import OfficeViewer from '../OfficeViewer';
import * as file from '../../../file';
import { BROWSERS } from '../../../constants';

const FIVE_MB = 5242880;

Expand Down Expand Up @@ -86,6 +88,12 @@ describe('lib/viewers/office/OfficeLoader', () => {
const viewer = OfficeLoader.determineViewer(editedFakeFile, [], viewerOptions);
expect(viewer).toBeDefined();
});

test('should not return a viewer if the browser is internet explorer', () => {
jest.spyOn(Browser, 'getName').mockImplementation(() => BROWSERS.INTERNET_EXPLORER);
const viewer = OfficeLoader.determineViewer(fakeFile, []);
expect(viewer).toBeUndefined();
});
});
});
});