From a3a26e7d4dd1856f431c42ae2cdbdf46b09dba3e Mon Sep 17 00:00:00 2001 From: Jeremy Press Date: Mon, 12 Feb 2018 15:12:54 -0800 Subject: [PATCH 1/5] Chore: Adding download reachability checks to Document and Image Viewers --- src/i18n/en-US.properties | 2 + src/lib/Preview.js | 40 ++++-- src/lib/__tests__/Preview-test.js | 43 +++++- .../__tests__/downloadReachability-test.js | 124 ++++++++++++++++++ src/lib/__tests__/util-test.js | 29 +++- src/lib/downloadReachability.js | 93 +++++++++++++ src/lib/util.js | 8 ++ src/lib/viewers/BaseViewer.js | 64 ++++++++- src/lib/viewers/__tests__/BaseViewer-test.js | 68 ++++++++++ src/lib/viewers/doc/DocBaseViewer.js | 6 +- .../doc/__tests__/DocBaseViewer-test.js | 18 +++ src/lib/viewers/image/ImageBaseViewer.js | 18 +-- src/lib/viewers/image/ImageViewer.js | 18 ++- src/lib/viewers/image/MultiImageViewer.js | 24 +++- .../image/__tests__/ImageBaseViewer-test.js | 40 ++++-- .../image/__tests__/ImageViewer-test.js | 9 +- .../image/__tests__/MultiImageViewer-test.js | 33 ++++- .../text/__tests__/PlainTextViewer-test.js | 7 +- 18 files changed, 588 insertions(+), 56 deletions(-) create mode 100644 src/lib/__tests__/downloadReachability-test.js create mode 100644 src/lib/downloadReachability.js diff --git a/src/i18n/en-US.properties b/src/i18n/en-US.properties index e8e65f52c..03714ecd7 100644 --- a/src/i18n/en-US.properties +++ b/src/i18n/en-US.properties @@ -176,6 +176,8 @@ notification_button_default_text=Okay notification_annotation_point_mode=Click anywhere to add a comment to the document # Notification message shown when user enters drawing annotation mode notification_annotation_draw_mode=Press down and drag the pointer to draw on the document +# Notification message shown when the user has a degraded preview experience due to blocked download hosts +notification_degraded_preview=It looks like your connection to {1} is being blocked. We think we can make file previews faster for you. To do that, please ask your network admin to configure firewall settings so that {1} is reachable. # Link Text link_contact_us=Contact Us diff --git a/src/lib/Preview.js b/src/lib/Preview.js index bf1eaa650..c0760857a 100644 --- a/src/lib/Preview.js +++ b/src/lib/Preview.js @@ -25,6 +25,12 @@ import { replacePlaceholders, stripAuthFromString } from './util'; +import { + isDownloadHostBlocked, + setDownloadReachability, + isCustomDownloadHost, + replaceDownloadHostWithDefault +} from './downloadReachability'; import { getURL, getDownloadURL, @@ -478,13 +484,29 @@ class Preview extends EventEmitter { download() { const { apiHost, queryParams } = this.options; - if (checkPermission(this.file, PERMISSION_DOWNLOAD)) { - // Append optional query params - const downloadUrl = appendQueryParams(getDownloadURL(this.file.id, apiHost), queryParams); - get(downloadUrl, this.getRequestHeaders()).then((data) => { - openUrlInsideIframe(data.download_url); - }); + if (!checkPermission(this.file, PERMISSION_DOWNLOAD)) { + return; } + + // Append optional query params + const downloadUrl = appendQueryParams(getDownloadURL(this.file.id, apiHost), queryParams); + get(downloadUrl, this.getRequestHeaders()).then((data) => { + const defaultDownloadUrl = replaceDownloadHostWithDefault(data.download_url); + if (isDownloadHostBlocked() || !isCustomDownloadHost(data.download_url)) { + // If we know the host is blocked, or we are already using the default, + // use the default. + openUrlInsideIframe(defaultDownloadUrl); + } else { + // Try the custom host, then check reachability + openUrlInsideIframe(data.download_url); + setDownloadReachability(data.download_url).then(() => { + if (isDownloadHostBlocked()) { + // If download is unreachable, try again with default + openUrlInsideIframe(defaultDownloadUrl); + } + }); + } + }); } /** @@ -747,6 +769,9 @@ class Preview extends EventEmitter { this.throttledMousemoveHandler ); + // Set up the notification + this.ui.setupNotification(); + // Update navigation this.ui.showNavigation(this.file.id, this.collection); @@ -1200,9 +1225,6 @@ class Preview extends EventEmitter { // Hide the loading indicator this.ui.hideLoadingIndicator(); - // Set up the notification - this.ui.setupNotification(); - // Prefetch next few files this.prefetchNextFiles(); } diff --git a/src/lib/__tests__/Preview-test.js b/src/lib/__tests__/Preview-test.js index 325d28eac..f89e85792 100644 --- a/src/lib/__tests__/Preview-test.js +++ b/src/lib/__tests__/Preview-test.js @@ -8,6 +8,7 @@ import Browser from '../Browser'; import PreviewError from '../PreviewError'; import * as file from '../file'; import * as util from '../util'; +import * as dr from '../downloadReachability'; import { API_HOST, CLASS_NAVIGATION_VISIBILITY } from '../constants'; import { VIEWER_EVENT, ERROR_CODE, LOAD_METRIC, PREVIEW_METRIC } from '../events'; import Timer from '../Timer'; @@ -741,15 +742,21 @@ describe('lib/Preview', () => { beforeEach(() => { stubs.promise = Promise.resolve({ data: { - download_url: 'dl.box' + download_url: 'dl.boxcloud.com' } }); + stubs.reachabilityPromise = Promise.resolve({}); + stubs.checkPermission = sandbox.stub(file, 'checkPermission'); stubs.get = sandbox.stub(util, 'get').returns(stubs.promise); + stubs.get = sandbox.stub(dr, 'setDownloadReachability').returns(stubs.reachabilityPromise); stubs.openUrlInsideIframe = sandbox.stub(util, 'openUrlInsideIframe'); stubs.getRequestHeaders = sandbox.stub(preview, 'getRequestHeaders'); stubs.getDownloadURL = sandbox.stub(file, 'getDownloadURL'); + stubs.isDownloadHostBlocked = sandbox.stub(dr, 'isDownloadHostBlocked'); + stubs.isCustomDownloadHost = sandbox.stub(dr, 'isCustomDownloadHost'); + stubs.replaceDownloadHostWithDefault = sandbox.stub(dr, 'replaceDownloadHostWithDefault').returns('default'); }); it('should not do anything if there is no download permission', () => { @@ -759,12 +766,38 @@ describe('lib/Preview', () => { expect(stubs.openUrlInsideIframe).to.not.be.called; }); - it('get the file and then open in an iframe', () => { + it('open the default download URL in an iframe if the custom host is blocked or if we were given the default', () => { + stubs.checkPermission.returns(true); + stubs.isDownloadHostBlocked.returns(true); + stubs.isCustomDownloadHost.returns(true); + + preview.download(); + return stubs.promise.then((data) => { + expect(stubs.openUrlInsideIframe).to.be.calledWith('default'); + }); + + stubs.isDownloadHostBlocked.returns(false); + stubs.isCustomDownloadHost.returns(false); + + preview.download(); + return stubs.promise.then((data) => { + expect(stubs.openUrlInsideIframe).to.be.calledWith('default'); + }); + }); + + + it('should check download reachability and fallback if we do not know the status of our custom host', () => { stubs.checkPermission.returns(true); + stubs.isDownloadHostBlocked.returns(false); + stubs.isCustomDownloadHost.returns(true); preview.download(); return stubs.promise.then((data) => { expect(stubs.openUrlInsideIframe).to.be.calledWith(data.download_url); + stubs.isDownloadHostBlocked.returns(true); + return stubs.reachabilityPromise.then(() => { + expect(stubs.openUrlInsideIframe).to.be.calledWith('default'); + }); }); }); }); @@ -994,6 +1027,7 @@ describe('lib/Preview', () => { previewUIMock.expects('showLoadingIndicator'); previewUIMock.expects('startProgressBar'); previewUIMock.expects('showNavigation'); + previewUIMock.expects('setupNotification'); preview.setupUI(); }); @@ -1810,11 +1844,6 @@ describe('lib/Preview', () => { expect(stubs.hideLoadingIndicator).to.be.called; }); - it('should set up the notification', () => { - preview.finishLoading(); - expect(stubs.setupNotification).to.be.called; - }); - it('should prefetch next files', () => { preview.finishLoading(); expect(stubs.prefetchNextFiles).to.be.called; diff --git a/src/lib/__tests__/downloadReachability-test.js b/src/lib/__tests__/downloadReachability-test.js new file mode 100644 index 000000000..a0f389386 --- /dev/null +++ b/src/lib/__tests__/downloadReachability-test.js @@ -0,0 +1,124 @@ +/* eslint-disable no-unused-expressions */ +import 'whatwg-fetch'; +import fetchMock from 'fetch-mock'; +import * as dr from '../downloadReachability'; + +const sandbox = sinon.sandbox.create(); + +const DEFAULT_DOWNLOAD_HOST_PREFIX = 'https://dl.'; +const DOWNLOAD_NOTIFICATION_SHOWN_KEY = 'download_host_notification_shown'; +const DOWNLOAD_HOST_FALLBACK_KEY = 'download_host_fallback'; + +describe('lib/downloadReachability', () => { + beforeEach(() => { + sessionStorage.clear(); + localStorage.clear(); + + }); + + afterEach(() => { + sessionStorage.clear(); + localStorage.clear(); + sandbox.verifyAndRestore(); + + }); + + describe('isCustomDownloadHost()', () => { + it('should be true if the url does not start with the default host prefix but is a dl host', () => { + let url = 'https://dl3.boxcloud.com/foo'; + let result = dr.isCustomDownloadHost(url) + expect(result).to.be.true; + + url = 'https://dl.boxcloud.com/foo'; + expect(dr.isCustomDownloadHost(url)).to.be.false; + + url = 'https://www.google.com'; + expect(dr.isCustomDownloadHost(url)).to.be.false; + }); + }); + + describe('replaceDownloadHostWithDefault()', () => { + it('should add the given host to the array of shown hosts', () => { + const blockedHost = 'https://dl3.boxcloud.com'; + + const result = dr.setDownloadHostNotificationShown(blockedHost); + + const shownHostsArr = JSON.parse(localStorage.getItem(DOWNLOAD_NOTIFICATION_SHOWN_KEY)) || []; + expect(shownHostsArr).to.contain('https://dl3.boxcloud.com'); + }); + }); + + describe('setDownloadHostFallback()', () => { + it('should set the download host fallback key to be true', () => { + expect(sessionStorage.getItem(DOWNLOAD_HOST_FALLBACK_KEY) === 'true').to.be.false; + + dr.setDownloadHostFallback(); + + expect(sessionStorage.getItem(DOWNLOAD_HOST_FALLBACK_KEY) === 'true').to.be.true; + + }); + }); + + describe('isDownloadHostBlocked()', () => { + it('should set the download host fallback key to be true', () => { + expect(dr.isDownloadHostBlocked()).to.be.false; + + dr.setDownloadHostFallback(); + + expect(dr.isDownloadHostBlocked()).to.be.true; + }); + }); + + describe('setDownloadHostNotificationShown()', () => { + it('should set the download host fallback key to be true', () => { + expect(dr.isDownloadHostBlocked()).to.be.false; + + dr.setDownloadHostFallback(); + + expect(dr.isDownloadHostBlocked()).to.be.true; + }); + }); + + describe('downloadNotificationToShow()', () => { + beforeEach(() => { + sessionStorage.setItem('download_host_fallback', 'false'); + }); + + it('should return true if we do not have an entry for the given host and our session indicates we are falling back to the default host', () => { + let result = dr.downloadNotificationToShow(); + expect(result).to.be.undefined;; + + sessionStorage.setItem('download_host_fallback', 'true'); + result = dr.downloadNotificationToShow('https://dl5.boxcloud.com'); + expect(result).to.equal('dl5.boxcloud.com'); + + const shownHostsArr = ['dl5.boxcloud.com']; + localStorage.setItem('download_host_notification_shown', JSON.stringify(shownHostsArr)); + result = dr.downloadNotificationToShow(shownHostsArr[0]); + expect(result).to.be.undefined; + + }); + }); + + + describe('setDownloadReachability()', () => { + afterEach(() => { + fetchMock.restore(); + }) + it('should catch an errored response', () => { + const setDownloadHostFallbackStub = sandbox.stub(dr, 'setDownloadHostFallback'); + fetchMock.head('https://dl3.boxcloud.com', {throws: new Error('woohoo')}) + + return dr.setDownloadReachability('https://dl3.boxcloud.com').catch(() => { + expect(setDownloadHostFallbackStub).to.be.called; + }) + + + + }); + }); + + + + +}); \ No newline at end of file diff --git a/src/lib/__tests__/util-test.js b/src/lib/__tests__/util-test.js index 915e9a11c..bc56103e6 100644 --- a/src/lib/__tests__/util-test.js +++ b/src/lib/__tests__/util-test.js @@ -10,9 +10,12 @@ describe('lib/util', () => { afterEach(() => { sandbox.verifyAndRestore(); }); - + describe('get()', () => { - const url = 'foo?bar=bum'; + let url; + beforeEach(() => { + url = 'foo?bar=bum'; + }); afterEach(() => { fetchMock.restore(); @@ -96,6 +99,23 @@ describe('lib/util', () => { expect(typeof response === 'object').to.be.true; }); }); + + it('set the download host fallback and try again if we\'re fetching from a non default host', () => { + url = 'dl7.boxcloud.com' + fetchMock.get(url, { + status: 500 + }); + + return util.get(url, 'any') + .then(() => { + expect(response.status).to.equal(200); + }) + .catch(() => { + fetchMock.get(url, { + status: 200 + }); + }) + }); }); describe('post()', () => { @@ -327,6 +347,11 @@ describe('lib/util', () => { it('should return correct content url when no asset_path', () => { expect(util.createContentUrl('foo', 'bar')).to.equal('foo'); }); + + it('should replace the download host with the default if we are falling back', () => { + sessionStorage.setItem('download_host_fallback', 'true'); + expect(util.createContentUrl('https://dl6.boxcloud.com', 'bar')).to.equal('https://dl.boxcloud.com'); + }); }); describe('createAssetUrlCreator()', () => { diff --git a/src/lib/downloadReachability.js b/src/lib/downloadReachability.js new file mode 100644 index 000000000..87e4517fb --- /dev/null +++ b/src/lib/downloadReachability.js @@ -0,0 +1,93 @@ +const DEFAULT_DOWNLOAD_HOST_PREFIX = 'https://dl.'; +const DOWNLOAD_NOTIFICATION_SHOWN_KEY = 'download_host_notification_shown'; +const DOWNLOAD_HOST_FALLBACK_KEY = 'download_host_fallback'; +const DEFAULT_HOST_REGEX = /^https:\/\/dl\d+\./; + +/** + * Checks if the url is a download host, but not the default download host. + * + * @public + * @param {string} downloadUrl - Content download URL, may either be a template or an actual URL + * @return {boolean} - HTTP response + */ +export function isCustomDownloadHost(downloadUrl) { + return ( + downloadUrl && !downloadUrl.startsWith(DEFAULT_DOWNLOAD_HOST_PREFIX) && !!downloadUrl.match(DEFAULT_HOST_REGEX) + ); +} + +/** + * Replaces the hostname of a download URL with the default hostname, https://dl. + * + * @private + * @param {string} downloadUrl - Content download URL, may either be a template or an actual URL + * @return {string} - The updated download URL + */ +export function replaceDownloadHostWithDefault(downloadUrl) { + return downloadUrl.replace(DEFAULT_HOST_REGEX, DEFAULT_DOWNLOAD_HOST_PREFIX); +} + +/** + * Sets session storage to use the default download host. + * + * @public + * @return {void} + */ +export function setDownloadHostFallback() { + sessionStorage.setItem(DOWNLOAD_HOST_FALLBACK_KEY, 'true'); +} + +/** + * Checks if we have detected a blocked download host and have decided to fall back. + * + * @public + * @return {boolean} Whether the sessionStorage indicates that a download host has been blocked + */ +export function isDownloadHostBlocked() { + return sessionStorage.getItem(DOWNLOAD_HOST_FALLBACK_KEY) === 'true'; +} + +/** + * Stores the host in an array via localstorage so that we don't show a notification for it again + * + * @public + * @param {string} downloadHost - Download URL host name + * @return {void} + */ +export function setDownloadHostNotificationShown(downloadHost) { + const shownHostsArr = JSON.parse(localStorage.getItem(DOWNLOAD_NOTIFICATION_SHOWN_KEY)) || []; + shownHostsArr.push(downloadHost); + localStorage.setItem(DOWNLOAD_NOTIFICATION_SHOWN_KEY, JSON.stringify(shownHostsArr)); +} + +/** + * Determines what notification should be shown if needed. + * + * @public + * @param {string} contentTemplate - Content download URL template + * @return {string|undefined} Should the notification be shown + */ +export function downloadNotificationToShow(contentTemplate) { + const contentHost = document.createElement('a'); + contentHost.href = contentTemplate; + const contentHostname = contentHost.hostname; + const shownHostsArr = JSON.parse(localStorage.getItem(DOWNLOAD_NOTIFICATION_SHOWN_KEY)) || []; + + return sessionStorage.getItem(DOWNLOAD_HOST_FALLBACK_KEY) === 'true' && + !shownHostsArr.includes(contentHostname) && + isCustomDownloadHost(contentTemplate) + ? contentHostname + : undefined; +} + +/** + * Checks if the provided host is reachable. If not set the session storage to reflect this. + * + * @param {string} downloadUrl - Content download URL, may either be a template or an actual URL + * @return {void} + */ +export function setDownloadReachability(downloadUrl) { + return fetch(downloadUrl, { method: 'HEAD' }).catch(() => { + setDownloadHostFallback(); + }); +} diff --git a/src/lib/util.js b/src/lib/util.js index 0d8c4728b..48bf9028b 100644 --- a/src/lib/util.js +++ b/src/lib/util.js @@ -1,10 +1,13 @@ import Uri from 'jsuri'; import 'whatwg-fetch'; +import { isDownloadHostBlocked, replaceDownloadHostWithDefault } from './downloadReachability'; + const HEADER_CLIENT_NAME = 'X-Box-Client-Name'; const HEADER_CLIENT_VERSION = 'X-Box-Client-Version'; const CLIENT_NAME_KEY = 'box_client_name'; const CLIENT_VERSION_KEY = 'box_client_version'; + /* eslint-disable no-undef */ const CLIENT_NAME = __NAME__; export const CLIENT_VERSION = __VERSION__; @@ -403,6 +406,11 @@ export function appendAuthParams(url, token = '', sharedLink = '', password = '' * @return {string} Content url */ export function createContentUrl(template, asset) { + if (isDownloadHostBlocked()) { + /* eslint-disable no-param-reassign */ + template = replaceDownloadHostWithDefault(template); + /* eslint-enable no-param-reassign */ + } return template.replace('{+asset_path}', asset || ''); } diff --git a/src/lib/viewers/BaseViewer.js b/src/lib/viewers/BaseViewer.js index b7d0a0984..aeb094e14 100644 --- a/src/lib/viewers/BaseViewer.js +++ b/src/lib/viewers/BaseViewer.js @@ -7,13 +7,23 @@ import { getProp, appendQueryParams, appendAuthParams, - getHeaders, createContentUrl, + getHeaders, loadStylesheets, loadScripts, prefetchAssets, - createAssetUrlCreator + createAssetUrlCreator, + replacePlaceholders } from '../util'; + +import { + setDownloadReachability, + isCustomDownloadHost, + replaceDownloadHostWithDefault, + setDownloadHostNotificationShown, + downloadNotificationToShow +} from '../downloadReachability'; + import Browser from '../Browser'; import { CLASS_FULLSCREEN, @@ -100,6 +110,9 @@ class BaseViewer extends EventEmitter { /** @property {Object} - Viewer startAt options */ startAt; + + /** @property {boolean} - Has the viewer retried downloading the content */ + haveRetriedContentDownload = false; /** * [constructor] @@ -291,6 +304,31 @@ class BaseViewer extends EventEmitter { this.destroyed = true; } + /** + * Handles a download error when using a non default host. + * + * @param {Error} err - Load error + * @param {string} downloadURL - download URL + * @return {void} + */ + handleDownloadError(err, downloadURL) { + if (this.haveRetriedContentDownload) { + /* eslint-disable no-console */ + console.error(err); + /* eslint-enable no-console */ + + this.triggerError(err); + return; + } + + this.haveRetriedContentDownload = true; + this.load(); + + if (isCustomDownloadHost(downloadURL)) { + setDownloadReachability(downloadURL); + } + } + /** * Emits error event with refresh message. * @@ -350,6 +388,12 @@ class BaseViewer extends EventEmitter { * @return {string} content url */ createContentUrl(template, asset) { + if (this.haveRetriedContentDownload) { + /* eslint-disable no-param-reassign */ + template = replaceDownloadHostWithDefault(template); + /* eslint-enable no-param-reassign */ + } + // Append optional query params const { queryParams } = this.options; return appendQueryParams(createContentUrl(template, asset), queryParams); @@ -366,7 +410,7 @@ class BaseViewer extends EventEmitter { * @return {string} content url */ createContentUrlWithAuthParams(template, asset) { - const urlWithAuthParams = this.appendAuthParams(createContentUrl(template, asset)); + const urlWithAuthParams = this.appendAuthParams(this.createContentUrl(template, asset)); // Append optional query params const { queryParams } = this.options; @@ -408,13 +452,25 @@ class BaseViewer extends EventEmitter { } /** - * Handles the viewer load to potentially set up Box Annotations. + * Handles the viewer load to finish viewer setup after loading. * * @private * @param {Object} event - load event data * @return {void} */ viewerLoadHandler(event) { + const contentTemplate = getProp(this.options, 'representation.content.url_template', null); + const downloadHostToNotify = downloadNotificationToShow(contentTemplate); + if (downloadHostToNotify) { + this.previewUI.notification.show( + replacePlaceholders(__('notification_degraded_preview'), [downloadHostToNotify]), + __('notification_button_default_text'), + true + ); + + setDownloadHostNotificationShown(downloadHostToNotify); + } + if (event && event.scale) { this.scale = event.scale; } diff --git a/src/lib/viewers/__tests__/BaseViewer-test.js b/src/lib/viewers/__tests__/BaseViewer-test.js index 3cd829cca..40588b6fb 100644 --- a/src/lib/viewers/__tests__/BaseViewer-test.js +++ b/src/lib/viewers/__tests__/BaseViewer-test.js @@ -6,6 +6,8 @@ import RepStatus from '../../RepStatus'; import PreviewError from '../../PreviewError'; import fullscreen from '../../Fullscreen'; import * as util from '../../util'; +import * as dr from '../../downloadReachability'; + import * as file from '../../file'; import * as icons from '../../icons/icons'; import * as constants from '../../constants'; @@ -200,6 +202,38 @@ describe('lib/viewers/BaseViewer', () => { }); }); + describe('handleDownloadError()', () => { + beforeEach(() => { + sandbox.stub(base, 'triggerError'); + sandbox.stub(dr, 'isCustomDownloadHost'); + sandbox.stub(dr, 'setDownloadReachability'); + sandbox.stub(base, 'load'); + }); + + it('should trigger an error if we have already retried', () => { + base.haveRetriedContentDownload = true; + base.handleDownloadError('error', 'https://dl.boxcloud.com'); + expect(base.triggerError).to.be.called; + expect(base.load).to.not.be.called; + }); + + it('should retry load, and check download reachability if we are on a custom host', () => { + base.haveRetriedContentDownload = false; + dr.isCustomDownloadHost.returns(false); + + base.handleDownloadError('error', 'https://dl.boxcloud.com'); + expect(base.load).to.be.called; + expect(dr.setDownloadReachability).to.be.not.called; + + base.haveRetriedContentDownload = false; + // Now try on a custom host + dr.isCustomDownloadHost.returns(true); + base.handleDownloadError('error', 'https://dl3.boxcloud.com'); + expect(dr.setDownloadReachability).to.be.called; + + }); + }); + describe('triggerError()', () => { it('should emit PreviewError event', () => { const stub = sandbox.stub(base, 'emit'); @@ -278,6 +312,16 @@ describe('lib/viewers/BaseViewer', () => { expect(result).to.equal('urlbar'); expect(util.createContentUrl).to.be.calledWith(url, 'bar'); }); + + it('should fallback to the default host if we have retried', () => { + base.haveRetriedContentDownload = true; + sandbox.stub(dr, 'replaceDownloadHostWithDefault'); + sandbox.stub(util, 'createContentUrl'); + + + base.createContentUrl('https://dl3.boxcloud.com', ''); + expect(dr.replaceDownloadHostWithDefault).to.be.called; + }); }); describe('createContentUrlWithAuthParams()', () => { @@ -357,6 +401,30 @@ describe('lib/viewers/BaseViewer', () => { beforeEach(() => { base.annotationsLoadPromise = Promise.resolve(); stubs.annotationsLoadHandler = sandbox.stub(base, 'annotationsLoadHandler'); + base.options.representation = { + content: { + url_template: 'dl.boxcloud.com' + } + }; + stubs.downloadNotificationToShow = sandbox.stub(dr, 'downloadNotificationToShow').returns(undefined); + + }); + + it('should show the notification if downloads are degraded and we have not shown the notification yet', () => { + const result = stubs.downloadNotificationToShow.returns('dl3.boxcloud.com'); + base.previewUI = + { + notification: { + show: sandbox.stub() + + } + } + + sandbox.stub(dr, 'setDownloadHostNotificationShown'); + + base.viewerLoadHandler({ scale: 1.5 }); + expect(base.previewUI.notification.show).to.be.called; + expect(dr.setDownloadHostNotificationShown).to.be.called; }); it('should set the scale if it exists', () => { diff --git a/src/lib/viewers/doc/DocBaseViewer.js b/src/lib/viewers/doc/DocBaseViewer.js index e00871d29..9918a2016 100644 --- a/src/lib/viewers/doc/DocBaseViewer.js +++ b/src/lib/viewers/doc/DocBaseViewer.js @@ -582,7 +582,11 @@ class DocBaseViewer extends BaseViewer { // Display a generic error message but log the real one const error = new PreviewError(ERROR_CODE.LOAD_DOCUMENT, __('error_document'), {}, err.message); - this.triggerError(error); + if (err instanceof Error) { + error.displayMessage = __('error_document'); + } + + this.handleDownloadError(err, pdfUrl); }); } diff --git a/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js b/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js index 22f256381..e91c48240 100644 --- a/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js +++ b/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js @@ -868,6 +868,24 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => { docBase.initViewer('url'); expect(docBase.startLoadTimer).to.be.called; + + }); + + it('should handle any download error', () => { + stubs.handleDownloadError = sandbox.stub(docBase, 'handleDownloadError'); + const doc = { + url: 'url' + }; + + docBase.options.location = { + locale: 'en-US' + }; + + const getDocumentStub = sandbox.stub(PDFJS, 'getDocument').returns(Promise.reject(doc)); + + return docBase.initViewer('url').catch(() => { + expect(stubs.handleDownloadError).to.be.called; + }); }); }); diff --git a/src/lib/viewers/image/ImageBaseViewer.js b/src/lib/viewers/image/ImageBaseViewer.js index ca665ecdb..dcab00530 100644 --- a/src/lib/viewers/image/ImageBaseViewer.js +++ b/src/lib/viewers/image/ImageBaseViewer.js @@ -27,7 +27,6 @@ class ImageBaseViewer extends BaseViewer { this.handleMouseUp = this.handleMouseUp.bind(this); this.cancelDragEvent = this.cancelDragEvent.bind(this); this.finishLoading = this.finishLoading.bind(this); - this.errorHandler = this.errorHandler.bind(this); if (this.isMobile) { if (Browser.isIOS()) { @@ -312,19 +311,20 @@ class ImageBaseViewer extends BaseViewer { } /** - * Handles image element loading errors. + * Handles a content download error * - * @private - * @param {Error} err - Error to handle + * @param {Error} err - Load error + * @param {string} imgUrl - URL we are using as the image src * @return {void} */ - errorHandler(err) { - // eslint-disable-next-line - console.error(err); - + handleDownloadError(err, imgUrl) { // Display a generic error message but log the real one const error = new PreviewError(ERROR_CODE.IMAGE_SIZING, __('error_refresh'), {}, err.message); - this.emit('error', error); + if (err instanceof Error) { + error.displayMessage = __('error_refresh'); + } + + super.handleDownloadError(err, imgUrl); } /** diff --git a/src/lib/viewers/image/ImageViewer.js b/src/lib/viewers/image/ImageViewer.js index d29997e21..f15f6184a 100644 --- a/src/lib/viewers/image/ImageViewer.js +++ b/src/lib/viewers/image/ImageViewer.js @@ -16,6 +16,7 @@ class ImageViewer extends ImageBaseViewer { this.rotateLeft = this.rotateLeft.bind(this); this.updatePannability = this.updatePannability.bind(this); + this.handleImageDownloadError = this.handleImageDownloadError.bind(this); if (this.isMobile) { this.handleOrientationChange = this.handleOrientationChange.bind(this); @@ -52,13 +53,14 @@ class ImageViewer extends ImageBaseViewer { const { representation, viewer } = this.options; const template = representation.content.url_template; + const downloadURL = this.createContentUrlWithAuthParams(template, viewer.ASSET); this.bindDOMListeners(); return this.getRepStatus() .getPromise() .then(() => { this.startLoadTimer(); - this.imageEl.src = this.createContentUrlWithAuthParams(template, viewer.ASSET); + this.imageEl.src = downloadURL; }) .catch(this.handleAssetError); } @@ -351,6 +353,16 @@ class ImageViewer extends ImageBaseViewer { // Event Listeners //-------------------------------------------------------------------------- + /** + * Passes the error and download URL to the download error handler. + * + * @param {Error} err - Download error + * @return {void} + */ + handleImageDownloadError(err) { + this.handleDownloadError(err, this.imageEl.src); + } + /** * Binds DOM listeners for image viewer. * @@ -361,7 +373,7 @@ class ImageViewer extends ImageBaseViewer { super.bindDOMListeners(); this.imageEl.addEventListener('load', this.finishLoading); - this.imageEl.addEventListener('error', this.errorHandler); + this.imageEl.addEventListener('error', this.handleImageDownloadError); if (this.isMobile) { this.imageEl.addEventListener('orientationchange', this.handleOrientationChange); @@ -379,7 +391,7 @@ class ImageViewer extends ImageBaseViewer { if (this.imageEl) { this.imageEl.removeEventListener('load', this.finishLoading); - this.imageEl.removeEventListener('error', this.errorHandler); + this.imageEl.removeEventListener('error', this.handleImageDownloadError); } if (this.isMobile) { diff --git a/src/lib/viewers/image/MultiImageViewer.js b/src/lib/viewers/image/MultiImageViewer.js index d4c3654f3..f3b26050e 100644 --- a/src/lib/viewers/image/MultiImageViewer.js +++ b/src/lib/viewers/image/MultiImageViewer.js @@ -21,6 +21,7 @@ class MultiImageViewer extends ImageBaseViewer { this.setPage = this.setPage.bind(this); this.scrollHandler = this.scrollHandler.bind(this); this.handlePageChangeFromScroll = this.handlePageChangeFromScroll.bind(this); + this.handleMultiImageDownloadError = this.handleMultiImageDownloadError.bind(this); } /** @@ -245,6 +246,25 @@ class MultiImageViewer extends ImageBaseViewer { this.controls.add(__('exit_fullscreen'), this.toggleFullscreen, 'bp-exit-fullscreen-icon', ICON_FULLSCREEN_OUT); } + /** + * Passes the error and download URL to the download error handler. + * + * @param {Error} err - Download error + * @return {void} + */ + handleMultiImageDownloadError(err) { + this.singleImageEls.forEach((el, index) => { + this.unbindImageListeners(index); + }); + + // Since we're using the src to get the hostname, we can always use the src of the first page + const { src } = this.singleImageEls[0]; + // Clear any images we may have started to load. + this.singleImageEls = []; + + this.handleDownloadError(err, src); + } + /** * Binds error and load event listeners for an image element. * @@ -256,7 +276,7 @@ class MultiImageViewer extends ImageBaseViewer { this.singleImageEls[index].addEventListener('load', this.finishLoading); } - this.singleImageEls[index].addEventListener('error', this.errorHandler); + this.singleImageEls[index].addEventListener('error', this.handleMultiImageDownloadError); } /** @@ -270,7 +290,7 @@ class MultiImageViewer extends ImageBaseViewer { this.singleImageEls[index].removeEventListener('load', this.finishLoading); } - this.singleImageEls[index].removeEventListener('error', this.errorHandler); + this.singleImageEls[index].removeEventListener('error', this.handleMultiImageDownloadError); } /** diff --git a/src/lib/viewers/image/__tests__/ImageBaseViewer-test.js b/src/lib/viewers/image/__tests__/ImageBaseViewer-test.js index 6b81dda0e..6f8c1e565 100644 --- a/src/lib/viewers/image/__tests__/ImageBaseViewer-test.js +++ b/src/lib/viewers/image/__tests__/ImageBaseViewer-test.js @@ -544,24 +544,35 @@ describe('lib/viewers/image/ImageBaseViewer', () => { }); }); - describe('errorHandler()', () => { + describe('handleDownloadError()', () => { + const handleDownloadErrorFunc = BaseViewer.prototype.handleDownloadError; + beforeEach(() => { - stubs.emit = sandbox.stub(imageBase, 'emit'); + Object.defineProperty(Object.getPrototypeOf(ImageBaseViewer.prototype), 'handleDownloadError', { + value: sandbox.stub() + }); }); - it('should console log error and emit preview error', () => { - const err = new Error('blah'); - sandbox - .mock(window.console) - .expects('error') - .withArgs(err); + afterEach(() => { + Object.defineProperty(Object.getPrototypeOf(ImageBaseViewer.prototype), 'handleDownloadError', { + value: handleDownloadErrorFunc + }); + }); - imageBase.errorHandler(err); + it('should call the parent method with an error display message and the image URL', () => { + const err = new Error('downloadError') + + try { + imageBase.handleDownloadError(err, 'foo'); + } catch (e) { + // no-op + } const [ event, error ] = stubs.emit.getCall(0).args; expect(event).to.equal('error'); expect(error).to.be.instanceof(PreviewError); expect(error.code).to.equal('error_image_sizing'); + expect(BaseViewer.prototype.handleDownloadError).to.be.calledWith(err, 'foo') }); }); @@ -571,7 +582,14 @@ describe('lib/viewers/image/ImageBaseViewer', () => { stubs.zoom = sandbox.stub(imageBase, 'zoom'); stubs.loadUI = sandbox.stub(imageBase, 'loadUI'); stubs.setOriginalImageSize = sandbox.stub(imageBase, 'setOriginalImageSize'); - stubs.errorHandler = sandbox.stub(imageBase, 'errorHandler'); + imageBase.options = { + file: { + id: 1 + }, + viewer: { + viewerName: "Image" + } + } }); it('should do nothing if already destroyed', () => { @@ -584,12 +602,10 @@ describe('lib/viewers/image/ImageBaseViewer', () => { expect(stubs.zoom).to.not.have.been.called; expect(stubs.setOriginalImageSize).to.not.have.been.called; expect(stubs.loadUI).to.not.have.been.called; - expect(stubs.errorHandler).to.not.have.been.called; }); it('should load UI if not destroyed', (done) => { imageBase.on(VIEWER_EVENT.load, () => { - expect(stubs.errorHandler).to.not.have.been.called; expect(imageBase.loaded).to.be.true; expect(stubs.zoom).to.have.been.called; expect(stubs.loadUI).to.have.been.called; diff --git a/src/lib/viewers/image/__tests__/ImageViewer-test.js b/src/lib/viewers/image/__tests__/ImageViewer-test.js index cd83f26f7..fe5b97a43 100644 --- a/src/lib/viewers/image/__tests__/ImageViewer-test.js +++ b/src/lib/viewers/image/__tests__/ImageViewer-test.js @@ -77,7 +77,6 @@ describe('lib/viewers/image/ImageViewer', () => { sandbox.stub(image, 'getRepStatus').returns({ getPromise: () => Promise.resolve() }); stubs.event = sandbox.stub(image.imageEl, 'addEventListener'); stubs.load = sandbox.stub(image, 'finishLoading'); - stubs.error = sandbox.stub(image, 'errorHandler'); stubs.bind = sandbox.stub(image, 'bindDOMListeners'); // load the image @@ -459,6 +458,12 @@ describe('lib/viewers/image/ImageViewer', () => { stubs.listeners = image.imageEl.addEventListener; }); + it('should bind error and load listeners', () => { + image.bindDOMListeners(); + expect(stubs.listeners).to.have.been.calledWith('load', image.finishLoading); + expect(stubs.listeners).to.have.been.calledWith('error', image.handleImageDownloadError); + }); + it('should bind all mobile listeners', () => { sandbox.stub(Browser, 'isIOS').returns(true); image.bindDOMListeners(); @@ -477,7 +482,7 @@ describe('lib/viewers/image/ImageViewer', () => { it('should unbind all default image listeners', () => { image.unbindDOMListeners(); expect(stubs.listeners).to.have.been.calledWith('load', image.finishLoading); - expect(stubs.listeners).to.have.been.calledWith('error', image.errorHandler); + expect(stubs.listeners).to.have.been.calledWith('error', image.handleImageDownloadError); }); it('should unbind all mobile listeners', () => { diff --git a/src/lib/viewers/image/__tests__/MultiImageViewer-test.js b/src/lib/viewers/image/__tests__/MultiImageViewer-test.js index 5a8aceea6..c07c8ba00 100644 --- a/src/lib/viewers/image/__tests__/MultiImageViewer-test.js +++ b/src/lib/viewers/image/__tests__/MultiImageViewer-test.js @@ -18,7 +18,6 @@ let clock; let containerEl; describe('lib/viewers/image/MultiImageViewer', () => { - stubs.errorHandler = MultiImageViewer.prototype.errorHandler; const setupFunc = BaseViewer.prototype.setup; const sizeFunc = ImageBaseViewer.prototype.setOriginalImageSize; @@ -354,7 +353,7 @@ describe('lib/viewers/image/MultiImageViewer', () => { }); }); - describe('bindPageControlListeners', () => { + describe('bindPageControlListeners()', () => { beforeEach(() => { multiImage.currentPageNumber = 1; multiImage.pagesCount = 10; @@ -384,8 +383,34 @@ describe('lib/viewers/image/MultiImageViewer', () => { }); + describe('handleMultiImageDownloadError()', () => { + beforeEach(() => { + multiImage.singleImageEls = [ + { + src: 'foo' + }, + { + src: 'baz' + } + ]; + + sandbox.stub(multiImage, 'handleDownloadError'); + sandbox.stub(multiImage, 'unbindImageListeners') + }); + + it('unbind the image listeners, clear the image Els array, and handle the download error', () => { + const src = multiImage.singleImageEls[0].src; + + multiImage.handleMultiImageDownloadError('err'); + + expect(multiImage.singleImageEls).to.deep.equal([]); + expect(multiImage.handleDownloadError).to.be.calledWith('err', src); + expect(multiImage.unbindImageListeners).to.be.calledTwice; + + }); + }); - describe('bindImageListeners', () => { + describe('bindImageListeners()', () => { beforeEach(() => { multiImage.singleImageEls = [ { @@ -408,7 +433,7 @@ describe('lib/viewers/image/MultiImageViewer', () => { }); }); - describe('unbindImageListeners', () => { + describe('unbindImageListeners()', () => { beforeEach(() => { multiImage.singleImageEls = [ { diff --git a/src/lib/viewers/text/__tests__/PlainTextViewer-test.js b/src/lib/viewers/text/__tests__/PlainTextViewer-test.js index 504005e72..4401780e5 100644 --- a/src/lib/viewers/text/__tests__/PlainTextViewer-test.js +++ b/src/lib/viewers/text/__tests__/PlainTextViewer-test.js @@ -237,9 +237,14 @@ describe('lib/viewers/text/PlainTextViewer', () => { sandbox.stub(text, 'startLoadTimer'); sandbox.stub(util, 'get').returns(Promise.resolve('')); + const someText = 'blah'; + const getPromise = Promise.resolve(someText); + text.postLoad(); - expect(text.startLoadTimer).to.be.called; + return getPromise.then(() => { + expect(text.startLoadTimer).to.be.called; + }); }); }); From 85f16c139ebafc2c1578cd009d7cfc636fc66ea5 Mon Sep 17 00:00:00 2001 From: Jeremy Press Date: Tue, 20 Feb 2018 17:44:48 -0800 Subject: [PATCH 2/5] Chore: Responding to comments --- .../__tests__/downloadReachability-test.js | 26 +++++++++++++------ src/lib/downloadReachability.js | 22 ++++++++++++---- src/lib/events.js | 4 ++- src/lib/viewers/BaseViewer.js | 8 +++--- src/lib/viewers/__tests__/BaseViewer-test.js | 8 +++--- src/lib/viewers/doc/DocBaseViewer.js | 2 +- src/lib/viewers/image/ImageBaseViewer.js | 13 +++++++--- src/lib/viewers/image/ImageViewer.js | 4 +-- .../image/__tests__/ImageBaseViewer-test.js | 13 +++------- 9 files changed, 62 insertions(+), 38 deletions(-) diff --git a/src/lib/__tests__/downloadReachability-test.js b/src/lib/__tests__/downloadReachability-test.js index a0f389386..2516c9cc6 100644 --- a/src/lib/__tests__/downloadReachability-test.js +++ b/src/lib/__tests__/downloadReachability-test.js @@ -34,17 +34,28 @@ describe('lib/downloadReachability', () => { url = 'https://www.google.com'; expect(dr.isCustomDownloadHost(url)).to.be.false; + + + url = 'https://kld3lk.boxcloud.com'; + expect(dr.isCustomDownloadHost(url)).to.be.true; + + url = 'https://dl3.user.inside-box.net'; + expect(dr.isCustomDownloadHost(url)).to.be.true; + + + url = 'https://dl.user.inside-box.net'; + expect(dr.isCustomDownloadHost(url)).to.be.false; }); }); describe('replaceDownloadHostWithDefault()', () => { it('should add the given host to the array of shown hosts', () => { - const blockedHost = 'https://dl3.boxcloud.com'; + // const blockedHost = 'https://dl3.boxcloud.com'; - const result = dr.setDownloadHostNotificationShown(blockedHost); + // const result = dr.setDownloadHostNotificationShown(blockedHost); - const shownHostsArr = JSON.parse(localStorage.getItem(DOWNLOAD_NOTIFICATION_SHOWN_KEY)) || []; - expect(shownHostsArr).to.contain('https://dl3.boxcloud.com'); + // const shownHostsArr = JSON.parse(localStorage.getItem(DOWNLOAD_NOTIFICATION_SHOWN_KEY)) || []; + // expect(shownHostsArr).to.contain('https://dl3.boxcloud.com'); }); }); @@ -85,7 +96,7 @@ describe('lib/downloadReachability', () => { }); it('should return true if we do not have an entry for the given host and our session indicates we are falling back to the default host', () => { - let result = dr.downloadNotificationToShow(); + let result = dr.downloadNotificationToShow('https://foo.com'); expect(result).to.be.undefined;; sessionStorage.setItem('download_host_fallback', 'true'); @@ -94,20 +105,19 @@ describe('lib/downloadReachability', () => { const shownHostsArr = ['dl5.boxcloud.com']; localStorage.setItem('download_host_notification_shown', JSON.stringify(shownHostsArr)); - result = dr.downloadNotificationToShow(shownHostsArr[0]); + result = dr.downloadNotificationToShow('https://dl5.boxcloud.com'); expect(result).to.be.undefined; }); }); - describe('setDownloadReachability()', () => { afterEach(() => { fetchMock.restore(); }) it('should catch an errored response', () => { const setDownloadHostFallbackStub = sandbox.stub(dr, 'setDownloadHostFallback'); - fetchMock.head('https://dl3.boxcloud.com', {throws: new Error('woohoo')}) + fetchMock.head('https://dl3.boxcloud.com', {throws: new Error()}) return dr.setDownloadReachability('https://dl3.boxcloud.com').catch(() => { expect(setDownloadHostFallbackStub).to.be.called; diff --git a/src/lib/downloadReachability.js b/src/lib/downloadReachability.js index 87e4517fb..bc032712b 100644 --- a/src/lib/downloadReachability.js +++ b/src/lib/downloadReachability.js @@ -1,7 +1,9 @@ const DEFAULT_DOWNLOAD_HOST_PREFIX = 'https://dl.'; +const PROD_CUSTOM_HOST_SUFFIX = 'boxcloud.com'; const DOWNLOAD_NOTIFICATION_SHOWN_KEY = 'download_host_notification_shown'; const DOWNLOAD_HOST_FALLBACK_KEY = 'download_host_fallback'; -const DEFAULT_HOST_REGEX = /^https:\/\/dl\d+\./; +const NUMBERED_HOST_PREFIX_REGEX = /^https:\/\/dl\d+\./; +const CUSTOM_HOST_PREFIX_REGEX = /^https:\/\/[A-Za-z0-9]+./; /** * Checks if the url is a download host, but not the default download host. @@ -11,20 +13,30 @@ const DEFAULT_HOST_REGEX = /^https:\/\/dl\d+\./; * @return {boolean} - HTTP response */ export function isCustomDownloadHost(downloadUrl) { + // A custom download host either + // 1. begins with a numbered dl hostname + // 2. or starts with a custom prefix and ends with boxcloud.com return ( - downloadUrl && !downloadUrl.startsWith(DEFAULT_DOWNLOAD_HOST_PREFIX) && !!downloadUrl.match(DEFAULT_HOST_REGEX) + !downloadUrl.startsWith(DEFAULT_DOWNLOAD_HOST_PREFIX) && + (!!downloadUrl.match(NUMBERED_HOST_PREFIX_REGEX) || downloadUrl.includes(PROD_CUSTOM_HOST_SUFFIX)) ); } /** * Replaces the hostname of a download URL with the default hostname, https://dl. * - * @private + * @public * @param {string} downloadUrl - Content download URL, may either be a template or an actual URL * @return {string} - The updated download URL */ export function replaceDownloadHostWithDefault(downloadUrl) { - return downloadUrl.replace(DEFAULT_HOST_REGEX, DEFAULT_DOWNLOAD_HOST_PREFIX); + if (downloadUrl.match(NUMBERED_HOST_PREFIX_REGEX)) { + // First check to see if we can swap a numbered dl prefix for the default + return downloadUrl.replace(NUMBERED_HOST_PREFIX_REGEX, DEFAULT_DOWNLOAD_HOST_PREFIX); + } + + // Otherwise replace the custom prefix with the default + return downloadUrl.replace(CUSTOM_HOST_PREFIX_REGEX, DEFAULT_DOWNLOAD_HOST_PREFIX); } /** @@ -65,7 +77,7 @@ export function setDownloadHostNotificationShown(downloadHost) { * * @public * @param {string} contentTemplate - Content download URL template - * @return {string|undefined} Should the notification be shown + * @return {string|undefined} Which host should we show a notification for, if any */ export function downloadNotificationToShow(contentTemplate) { const contentHost = document.createElement('a'); diff --git a/src/lib/events.js b/src/lib/events.js index 695e156ca..f555bf556 100644 --- a/src/lib/events.js +++ b/src/lib/events.js @@ -1,6 +1,7 @@ // Events emitted by Viewers export const VIEWER_EVENT = { download: 'download', // Begin downloading the file. + unreachableDownloadNotification: 'unreachabledownloadnotification', // Notification that download host is not reachable is displayed to the user. reload: 'reload', // Reload preview. load: 'load', // Preview is finished loading. progressStart: 'progressstart', // Begin using loading indicator. @@ -37,7 +38,8 @@ export const ERROR_CODE = { CONVERSION_GENERIC: 'error_conversion_generic', CONVERSION_PASSWORD_PROTECTED: 'error_password_protected', CONVERSION_TRY_AGAIN_LATER: 'error_try_again_later', - CONVERSION_UNSUPPORTED_FORMAT: 'error_unsupported_format' + CONVERSION_UNSUPPORTED_FORMAT: 'error_unsupported_format', + CONTENT_DOWNLOAD: 'error_content_download' }; export const PREVIEW_LOAD_EVENT = ''; diff --git a/src/lib/viewers/BaseViewer.js b/src/lib/viewers/BaseViewer.js index aeb094e14..d59092b98 100644 --- a/src/lib/viewers/BaseViewer.js +++ b/src/lib/viewers/BaseViewer.js @@ -112,7 +112,7 @@ class BaseViewer extends EventEmitter { startAt; /** @property {boolean} - Has the viewer retried downloading the content */ - haveRetriedContentDownload = false; + hasRetriedContentDownload = false; /** * [constructor] @@ -312,7 +312,7 @@ class BaseViewer extends EventEmitter { * @return {void} */ handleDownloadError(err, downloadURL) { - if (this.haveRetriedContentDownload) { + if (this.hasRetriedContentDownload) { /* eslint-disable no-console */ console.error(err); /* eslint-enable no-console */ @@ -321,7 +321,7 @@ class BaseViewer extends EventEmitter { return; } - this.haveRetriedContentDownload = true; + this.hasRetriedContentDownload = true; this.load(); if (isCustomDownloadHost(downloadURL)) { @@ -388,7 +388,7 @@ class BaseViewer extends EventEmitter { * @return {string} content url */ createContentUrl(template, asset) { - if (this.haveRetriedContentDownload) { + if (this.hasRetriedContentDownload) { /* eslint-disable no-param-reassign */ template = replaceDownloadHostWithDefault(template); /* eslint-enable no-param-reassign */ diff --git a/src/lib/viewers/__tests__/BaseViewer-test.js b/src/lib/viewers/__tests__/BaseViewer-test.js index 40588b6fb..b205559da 100644 --- a/src/lib/viewers/__tests__/BaseViewer-test.js +++ b/src/lib/viewers/__tests__/BaseViewer-test.js @@ -211,21 +211,21 @@ describe('lib/viewers/BaseViewer', () => { }); it('should trigger an error if we have already retried', () => { - base.haveRetriedContentDownload = true; + base.hasRetriedContentDownload = true; base.handleDownloadError('error', 'https://dl.boxcloud.com'); expect(base.triggerError).to.be.called; expect(base.load).to.not.be.called; }); it('should retry load, and check download reachability if we are on a custom host', () => { - base.haveRetriedContentDownload = false; + base.hasRetriedContentDownload = false; dr.isCustomDownloadHost.returns(false); base.handleDownloadError('error', 'https://dl.boxcloud.com'); expect(base.load).to.be.called; expect(dr.setDownloadReachability).to.be.not.called; - base.haveRetriedContentDownload = false; + base.hasRetriedContentDownload = false; // Now try on a custom host dr.isCustomDownloadHost.returns(true); base.handleDownloadError('error', 'https://dl3.boxcloud.com'); @@ -314,7 +314,7 @@ describe('lib/viewers/BaseViewer', () => { }); it('should fallback to the default host if we have retried', () => { - base.haveRetriedContentDownload = true; + base.hasRetriedContentDownload = true; sandbox.stub(dr, 'replaceDownloadHostWithDefault'); sandbox.stub(util, 'createContentUrl'); diff --git a/src/lib/viewers/doc/DocBaseViewer.js b/src/lib/viewers/doc/DocBaseViewer.js index 9918a2016..a8104e012 100644 --- a/src/lib/viewers/doc/DocBaseViewer.js +++ b/src/lib/viewers/doc/DocBaseViewer.js @@ -581,7 +581,7 @@ class DocBaseViewer extends BaseViewer { console.error(err); // Display a generic error message but log the real one - const error = new PreviewError(ERROR_CODE.LOAD_DOCUMENT, __('error_document'), {}, err.message); + const error = new PreviewError(ERROR_CODE.CONTENT_DOWNLOAD, __('error_document'), {}, err.message); if (err instanceof Error) { error.displayMessage = __('error_document'); } diff --git a/src/lib/viewers/image/ImageBaseViewer.js b/src/lib/viewers/image/ImageBaseViewer.js index dcab00530..cf86a427a 100644 --- a/src/lib/viewers/image/ImageBaseViewer.js +++ b/src/lib/viewers/image/ImageBaseViewer.js @@ -81,7 +81,9 @@ class ImageBaseViewer extends BaseViewer { this.loaded = true; this.emit(VIEWER_EVENT.load); }) - .catch(this.errorHandler); + .catch(() => { + // No-op, this prmise should always resolve + }); } /** @@ -314,17 +316,20 @@ class ImageBaseViewer extends BaseViewer { * Handles a content download error * * @param {Error} err - Load error - * @param {string} imgUrl - URL we are using as the image src + * @param {string} imgUrl - Image src URL * @return {void} */ handleDownloadError(err, imgUrl) { + // eslint-disable-next-line + console.error(err); + // Display a generic error message but log the real one - const error = new PreviewError(ERROR_CODE.IMAGE_SIZING, __('error_refresh'), {}, err.message); + const error = new PreviewError(ERROR_CODE.CONTENT_DOWNLOAD, __('error_refresh'), {}, err.message); if (err instanceof Error) { error.displayMessage = __('error_refresh'); } - super.handleDownloadError(err, imgUrl); + super.handleDownloadError(error, imgUrl); } /** diff --git a/src/lib/viewers/image/ImageViewer.js b/src/lib/viewers/image/ImageViewer.js index f15f6184a..be5b6fd4b 100644 --- a/src/lib/viewers/image/ImageViewer.js +++ b/src/lib/viewers/image/ImageViewer.js @@ -53,14 +53,14 @@ class ImageViewer extends ImageBaseViewer { const { representation, viewer } = this.options; const template = representation.content.url_template; - const downloadURL = this.createContentUrlWithAuthParams(template, viewer.ASSET); + const downloadUrl = this.createContentUrlWithAuthParams(template, viewer.ASSET); this.bindDOMListeners(); return this.getRepStatus() .getPromise() .then(() => { this.startLoadTimer(); - this.imageEl.src = downloadURL; + this.imageEl.src = downloadUrl; }) .catch(this.handleAssetError); } diff --git a/src/lib/viewers/image/__tests__/ImageBaseViewer-test.js b/src/lib/viewers/image/__tests__/ImageBaseViewer-test.js index 6f8c1e565..10aa83232 100644 --- a/src/lib/viewers/image/__tests__/ImageBaseViewer-test.js +++ b/src/lib/viewers/image/__tests__/ImageBaseViewer-test.js @@ -562,17 +562,12 @@ describe('lib/viewers/image/ImageBaseViewer', () => { it('should call the parent method with an error display message and the image URL', () => { const err = new Error('downloadError') - try { - imageBase.handleDownloadError(err, 'foo'); - } catch (e) { - // no-op - } + imageBase.handleDownloadError(err, 'foo'); - const [ event, error ] = stubs.emit.getCall(0).args; - expect(event).to.equal('error'); + const [ error, URL ] = BaseViewer.prototype.handleDownloadError.getCall(0).args; + expect(URL).to.equal('foo'); expect(error).to.be.instanceof(PreviewError); - expect(error.code).to.equal('error_image_sizing'); - expect(BaseViewer.prototype.handleDownloadError).to.be.calledWith(err, 'foo') + expect(error.code).to.equal('error_content_download'); }); }); From fa1f2cbdd0c82fcfc13c83b52c7903c6161e534b Mon Sep 17 00:00:00 2001 From: Jeremy Press Date: Thu, 22 Feb 2018 14:47:31 -0800 Subject: [PATCH 3/5] Chore: Refactor and adding logging --- src/lib/Preview.js | 5 +-- src/lib/__tests__/Preview-test.js | 4 +-- .../__tests__/downloadReachability-test.js | 12 +++---- src/lib/__tests__/util-test.js | 2 +- src/lib/downloadReachability.js | 33 ++++++++++++++----- src/lib/events.js | 7 +++- src/lib/util.js | 4 +-- src/lib/viewers/BaseViewer.js | 21 +++++++----- src/lib/viewers/__tests__/BaseViewer-test.js | 2 ++ src/lib/viewers/doc/DocBaseViewer.js | 6 +--- src/lib/viewers/image/ImageBaseViewer.js | 4 --- 11 files changed, 55 insertions(+), 45 deletions(-) diff --git a/src/lib/Preview.js b/src/lib/Preview.js index c0760857a..cd93beedc 100644 --- a/src/lib/Preview.js +++ b/src/lib/Preview.js @@ -169,6 +169,7 @@ class Preview extends EventEmitter { this.handleFileInfoResponse = this.handleFileInfoResponse.bind(this); this.handleFetchError = this.handleFetchError.bind(this); this.handleViewerEvents = this.handleViewerEvents.bind(this); + this.handleViewerMetrics = this.handleViewerMetrics.bind(this); this.triggerError = this.triggerError.bind(this); this.throttledMousemoveHandler = this.getGlobalMousemoveHandler().bind(this); this.navigateLeft = this.navigateLeft.bind(this); @@ -499,8 +500,8 @@ class Preview extends EventEmitter { } else { // Try the custom host, then check reachability openUrlInsideIframe(data.download_url); - setDownloadReachability(data.download_url).then(() => { - if (isDownloadHostBlocked()) { + setDownloadReachability(data.download_url).then((isBlocked) => { + if (isBlocked) { // If download is unreachable, try again with default openUrlInsideIframe(defaultDownloadUrl); } diff --git a/src/lib/__tests__/Preview-test.js b/src/lib/__tests__/Preview-test.js index f89e85792..ac726cf18 100644 --- a/src/lib/__tests__/Preview-test.js +++ b/src/lib/__tests__/Preview-test.js @@ -746,7 +746,7 @@ describe('lib/Preview', () => { } }); - stubs.reachabilityPromise = Promise.resolve({}); + stubs.reachabilityPromise = Promise.resolve(true); stubs.checkPermission = sandbox.stub(file, 'checkPermission'); stubs.get = sandbox.stub(util, 'get').returns(stubs.promise); @@ -788,13 +788,11 @@ describe('lib/Preview', () => { it('should check download reachability and fallback if we do not know the status of our custom host', () => { stubs.checkPermission.returns(true); - stubs.isDownloadHostBlocked.returns(false); stubs.isCustomDownloadHost.returns(true); preview.download(); return stubs.promise.then((data) => { expect(stubs.openUrlInsideIframe).to.be.calledWith(data.download_url); - stubs.isDownloadHostBlocked.returns(true); return stubs.reachabilityPromise.then(() => { expect(stubs.openUrlInsideIframe).to.be.calledWith('default'); }); diff --git a/src/lib/__tests__/downloadReachability-test.js b/src/lib/__tests__/downloadReachability-test.js index 2516c9cc6..20d37a620 100644 --- a/src/lib/__tests__/downloadReachability-test.js +++ b/src/lib/__tests__/downloadReachability-test.js @@ -50,12 +50,12 @@ describe('lib/downloadReachability', () => { describe('replaceDownloadHostWithDefault()', () => { it('should add the given host to the array of shown hosts', () => { - // const blockedHost = 'https://dl3.boxcloud.com'; + const blockedHost = 'https://dl3.boxcloud.com'; - // const result = dr.setDownloadHostNotificationShown(blockedHost); + const result = dr.setDownloadHostNotificationShown(blockedHost); - // const shownHostsArr = JSON.parse(localStorage.getItem(DOWNLOAD_NOTIFICATION_SHOWN_KEY)) || []; - // expect(shownHostsArr).to.contain('https://dl3.boxcloud.com'); + const shownHostsArr = JSON.parse(localStorage.getItem(DOWNLOAD_NOTIFICATION_SHOWN_KEY)) || []; + expect(shownHostsArr).to.contain('https://dl3.boxcloud.com'); }); }); @@ -127,8 +127,4 @@ describe('lib/downloadReachability', () => { }); }); - - - - }); \ No newline at end of file diff --git a/src/lib/__tests__/util-test.js b/src/lib/__tests__/util-test.js index bc56103e6..c50fdf324 100644 --- a/src/lib/__tests__/util-test.js +++ b/src/lib/__tests__/util-test.js @@ -10,7 +10,7 @@ describe('lib/util', () => { afterEach(() => { sandbox.verifyAndRestore(); }); - + describe('get()', () => { let url; beforeEach(() => { diff --git a/src/lib/downloadReachability.js b/src/lib/downloadReachability.js index bc032712b..b81b46ca1 100644 --- a/src/lib/downloadReachability.js +++ b/src/lib/downloadReachability.js @@ -5,6 +5,18 @@ const DOWNLOAD_HOST_FALLBACK_KEY = 'download_host_fallback'; const NUMBERED_HOST_PREFIX_REGEX = /^https:\/\/dl\d+\./; const CUSTOM_HOST_PREFIX_REGEX = /^https:\/\/[A-Za-z0-9]+./; +/** + * Extracts the hostname from a URL + * + * @param {string} downloadUrl - Content download URL, may either be a template or an actual URL + * @return {string} The hoostname of the given URL + */ +export function getHostnameFromUrl(downloadUrl) { + const contentHost = document.createElement('a'); + contentHost.href = downloadUrl; + return contentHost.hostname; +} + /** * Checks if the url is a download host, but not the default download host. * @@ -76,18 +88,16 @@ export function setDownloadHostNotificationShown(downloadHost) { * Determines what notification should be shown if needed. * * @public - * @param {string} contentTemplate - Content download URL template + * @param {string} downloadUrl - Content download URL * @return {string|undefined} Which host should we show a notification for, if any */ -export function downloadNotificationToShow(contentTemplate) { - const contentHost = document.createElement('a'); - contentHost.href = contentTemplate; - const contentHostname = contentHost.hostname; +export function downloadNotificationToShow(downloadUrl) { + const contentHostname = getHostnameFromUrl(downloadUrl); const shownHostsArr = JSON.parse(localStorage.getItem(DOWNLOAD_NOTIFICATION_SHOWN_KEY)) || []; return sessionStorage.getItem(DOWNLOAD_HOST_FALLBACK_KEY) === 'true' && !shownHostsArr.includes(contentHostname) && - isCustomDownloadHost(contentTemplate) + isCustomDownloadHost(downloadUrl) ? contentHostname : undefined; } @@ -99,7 +109,12 @@ export function downloadNotificationToShow(contentTemplate) { * @return {void} */ export function setDownloadReachability(downloadUrl) { - return fetch(downloadUrl, { method: 'HEAD' }).catch(() => { - setDownloadHostFallback(); - }); + return fetch(downloadUrl, { method: 'HEAD' }) + .then(() => { + return Promise.resolve(false); + }) + .catch(() => { + setDownloadHostFallback(); + return Promise.resolve(true); + }); } diff --git a/src/lib/events.js b/src/lib/events.js index f555bf556..7f07e7277 100644 --- a/src/lib/events.js +++ b/src/lib/events.js @@ -1,7 +1,6 @@ // Events emitted by Viewers export const VIEWER_EVENT = { download: 'download', // Begin downloading the file. - unreachableDownloadNotification: 'unreachabledownloadnotification', // Notification that download host is not reachable is displayed to the user. reload: 'reload', // Reload preview. load: 'load', // Preview is finished loading. progressStart: 'progressstart', // Begin using loading indicator. @@ -55,3 +54,9 @@ export const LOAD_METRIC = { downloadResponseTime: 'download_response_time', // Time it took for TTFB when requesting a rep. fullDocumentLoadTime: 'full_document_load_time' // How long it took to load the document so it could be previewed. }; + +// Events around download reachability +export const DOWNLOAD_REACHABILITY_METRICS = { + NOTIFICATION_SHOWN: 'dl_reachability_notification_shown', + DOWNLOAD_BLOCKED: 'dl_reachability_host_blocked' +}; diff --git a/src/lib/util.js b/src/lib/util.js index 48bf9028b..25e33575d 100644 --- a/src/lib/util.js +++ b/src/lib/util.js @@ -7,7 +7,6 @@ const HEADER_CLIENT_NAME = 'X-Box-Client-Name'; const HEADER_CLIENT_VERSION = 'X-Box-Client-Version'; const CLIENT_NAME_KEY = 'box_client_name'; const CLIENT_VERSION_KEY = 'box_client_version'; - /* eslint-disable no-undef */ const CLIENT_NAME = __NAME__; export const CLIENT_VERSION = __VERSION__; @@ -407,9 +406,8 @@ export function appendAuthParams(url, token = '', sharedLink = '', password = '' */ export function createContentUrl(template, asset) { if (isDownloadHostBlocked()) { - /* eslint-disable no-param-reassign */ + // eslint-disable-next-line template = replaceDownloadHostWithDefault(template); - /* eslint-enable no-param-reassign */ } return template.replace('{+asset_path}', asset || ''); } diff --git a/src/lib/viewers/BaseViewer.js b/src/lib/viewers/BaseViewer.js index d59092b98..617f63ddf 100644 --- a/src/lib/viewers/BaseViewer.js +++ b/src/lib/viewers/BaseViewer.js @@ -21,7 +21,8 @@ import { isCustomDownloadHost, replaceDownloadHostWithDefault, setDownloadHostNotificationShown, - downloadNotificationToShow + downloadNotificationToShow, + getHostnameFromUrl } from '../downloadReachability'; import Browser from '../Browser'; @@ -40,7 +41,7 @@ import { STATUS_VIEWABLE } from '../constants'; import { getIconFromExtension, getIconFromName } from '../icons/icons'; -import { VIEWER_EVENT, ERROR_CODE, LOAD_METRIC } from '../events'; +import { VIEWER_EVENT, ERROR_CODE, LOAD_METRIC, DOWNLOAD_REACHABILITY_METRICS } from '../events'; import PreviewError from '../PreviewError'; import Timer from '../Timer'; @@ -313,10 +314,6 @@ class BaseViewer extends EventEmitter { */ handleDownloadError(err, downloadURL) { if (this.hasRetriedContentDownload) { - /* eslint-disable no-console */ - console.error(err); - /* eslint-enable no-console */ - this.triggerError(err); return; } @@ -325,7 +322,11 @@ class BaseViewer extends EventEmitter { this.load(); if (isCustomDownloadHost(downloadURL)) { - setDownloadReachability(downloadURL); + setDownloadReachability(downloadURL).then((isBlocked) => { + if (isBlocked) { + this.emitMetric(DOWNLOAD_REACHABILITY_METRICS.DOWNLOAD_BLOCKED, getHostnameFromUrl(downloadURL)); + } + }); } } @@ -389,9 +390,8 @@ class BaseViewer extends EventEmitter { */ createContentUrl(template, asset) { if (this.hasRetriedContentDownload) { - /* eslint-disable no-param-reassign */ + // eslint-disable-next-line template = replaceDownloadHostWithDefault(template); - /* eslint-enable no-param-reassign */ } // Append optional query params @@ -469,6 +469,9 @@ class BaseViewer extends EventEmitter { ); setDownloadHostNotificationShown(downloadHostToNotify); + this.emitMetric(DOWNLOAD_REACHABILITY_METRICS.NOTIFICATION_SHOWN, { + host: downloadHostToNotify + }); } if (event && event.scale) { diff --git a/src/lib/viewers/__tests__/BaseViewer-test.js b/src/lib/viewers/__tests__/BaseViewer-test.js index b205559da..1b05ce2be 100644 --- a/src/lib/viewers/__tests__/BaseViewer-test.js +++ b/src/lib/viewers/__tests__/BaseViewer-test.js @@ -208,6 +208,7 @@ describe('lib/viewers/BaseViewer', () => { sandbox.stub(dr, 'isCustomDownloadHost'); sandbox.stub(dr, 'setDownloadReachability'); sandbox.stub(base, 'load'); + sandbox.stub(base, 'emitMetric'); }); it('should trigger an error if we have already retried', () => { @@ -228,6 +229,7 @@ describe('lib/viewers/BaseViewer', () => { base.hasRetriedContentDownload = false; // Now try on a custom host dr.isCustomDownloadHost.returns(true); + dr.setDownloadReachability.returns(Promise.resolve(true)) base.handleDownloadError('error', 'https://dl3.boxcloud.com'); expect(dr.setDownloadReachability).to.be.called; diff --git a/src/lib/viewers/doc/DocBaseViewer.js b/src/lib/viewers/doc/DocBaseViewer.js index a8104e012..52eb0957a 100644 --- a/src/lib/viewers/doc/DocBaseViewer.js +++ b/src/lib/viewers/doc/DocBaseViewer.js @@ -582,11 +582,7 @@ class DocBaseViewer extends BaseViewer { // Display a generic error message but log the real one const error = new PreviewError(ERROR_CODE.CONTENT_DOWNLOAD, __('error_document'), {}, err.message); - if (err instanceof Error) { - error.displayMessage = __('error_document'); - } - - this.handleDownloadError(err, pdfUrl); + this.handleDownloadError(error, pdfUrl); }); } diff --git a/src/lib/viewers/image/ImageBaseViewer.js b/src/lib/viewers/image/ImageBaseViewer.js index cf86a427a..f27a49ba6 100644 --- a/src/lib/viewers/image/ImageBaseViewer.js +++ b/src/lib/viewers/image/ImageBaseViewer.js @@ -325,10 +325,6 @@ class ImageBaseViewer extends BaseViewer { // Display a generic error message but log the real one const error = new PreviewError(ERROR_CODE.CONTENT_DOWNLOAD, __('error_refresh'), {}, err.message); - if (err instanceof Error) { - error.displayMessage = __('error_refresh'); - } - super.handleDownloadError(error, imgUrl); } From 02ad6d66e4904957f090164e81e252f0ce195dcc Mon Sep 17 00:00:00 2001 From: Jeremy Press Date: Thu, 1 Mar 2018 10:32:13 -0800 Subject: [PATCH 4/5] Chore: Responding to comments --- .../__tests__/downloadReachability-test.js | 4 ++-- src/lib/viewers/image/ImageBaseViewer.js | 20 ++++++++----------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/lib/__tests__/downloadReachability-test.js b/src/lib/__tests__/downloadReachability-test.js index 20d37a620..ace9a24d4 100644 --- a/src/lib/__tests__/downloadReachability-test.js +++ b/src/lib/__tests__/downloadReachability-test.js @@ -61,11 +61,11 @@ describe('lib/downloadReachability', () => { describe('setDownloadHostFallback()', () => { it('should set the download host fallback key to be true', () => { - expect(sessionStorage.getItem(DOWNLOAD_HOST_FALLBACK_KEY) === 'true').to.be.false; + expect(sessionStorage.getItem(DOWNLOAD_HOST_FALLBACK_KEY)).to.not.equal('true') dr.setDownloadHostFallback(); - expect(sessionStorage.getItem(DOWNLOAD_HOST_FALLBACK_KEY) === 'true').to.be.true; + expect(sessionStorage.getItem(DOWNLOAD_HOST_FALLBACK_KEY)).to.equal('true') }); }); diff --git a/src/lib/viewers/image/ImageBaseViewer.js b/src/lib/viewers/image/ImageBaseViewer.js index f27a49ba6..df1b9543f 100644 --- a/src/lib/viewers/image/ImageBaseViewer.js +++ b/src/lib/viewers/image/ImageBaseViewer.js @@ -72,18 +72,14 @@ class ImageBaseViewer extends BaseViewer { } const loadOriginalDimensions = this.setOriginalImageSize(this.imageEl); - loadOriginalDimensions - .then(() => { - this.loadUI(); - this.zoom(); - - this.imageEl.classList.remove(CLASS_INVISIBLE); - this.loaded = true; - this.emit(VIEWER_EVENT.load); - }) - .catch(() => { - // No-op, this prmise should always resolve - }); + loadOriginalDimensions.then(() => { + this.loadUI(); + this.zoom(); + + this.imageEl.classList.remove(CLASS_INVISIBLE); + this.loaded = true; + this.emit(VIEWER_EVENT.load); + }); } /** From f46a1f9ea0363cbcb9a3aa9dcdf576d7ed5fcfd8 Mon Sep 17 00:00:00 2001 From: Jeremy Press Date: Thu, 1 Mar 2018 15:45:55 -0800 Subject: [PATCH 5/5] Chore: Adding startsWith polyfill --- src/lib/polyfill.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/lib/polyfill.js b/src/lib/polyfill.js index 979c837c8..085d39555 100644 --- a/src/lib/polyfill.js +++ b/src/lib/polyfill.js @@ -384,4 +384,12 @@ if (!Array.from) { }; })(); } -/* eslint-enable */ + +// startsWith polyfill for IE 11 +// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/startsWith +if (!String.prototype.startsWith) { + String.prototype.startsWith = function(search, pos) { + return this.substr(!pos || pos < 0 ? 0 : +pos, search.length) === search; + }; +} +/* eslint-enable */ \ No newline at end of file