From 32dbee87fc893ab8716a6fd48feddf7dfa9f0434 Mon Sep 17 00:00:00 2001 From: Jeremy Press Date: Mon, 25 Mar 2019 16:13:57 -0700 Subject: [PATCH 1/4] Fix: Fail intentionally when preview image files with deleted reps --- src/lib/__tests__/api-test.js | 24 +++++++++++++++++++++++ src/lib/api.js | 4 ++-- src/lib/util.js | 21 ++++++++++++++++++++ src/lib/viewers/image/ImageBaseViewer.js | 8 +++++--- src/lib/viewers/image/ImageViewer.js | 19 ++++++++++++------ src/lib/viewers/image/MultiImageViewer.js | 15 +++++++++----- 6 files changed, 75 insertions(+), 16 deletions(-) diff --git a/src/lib/__tests__/api-test.js b/src/lib/__tests__/api-test.js index 4872c103c..b35f0e4eb 100644 --- a/src/lib/__tests__/api-test.js +++ b/src/lib/__tests__/api-test.js @@ -7,6 +7,30 @@ describe('API helper', () => { sandbox.verifyAndRestore(); }); + describe('parseResponse()', () => { + it('should return the full response when the status is 202 or 204', () => { + const response = { + status: 202, + data: 'foo' + }; + + expect(api.parseResponse(response)).to.equal(response); + + response.status = 204; + expect(api.parseResponse(response)).to.equal(response); + }); + + it('should only return the data', () => { + const data = 'foo'; + const response = { + status: 200, + data + }; + + expect(api.parseResponse(response)).to.equal(data); + }); + }); + describe('get()', () => { const url = '/foo/bar'; diff --git a/src/lib/api.js b/src/lib/api.js index eac7b9b73..9c1d44be3 100644 --- a/src/lib/api.js +++ b/src/lib/api.js @@ -41,10 +41,10 @@ const api = { * * @private * @param {Response} response - Response to parse - * @return {Promise|Response} Response if 204, otherwise promise that resolves with JSON + * @return {Promise|Response} Response if 204 or 202, otherwise promise that resolves with JSON */ parseResponse: (response) => { - if (response.status === 204) { + if (response.status === 204 || response.status === 202) { return response; } diff --git a/src/lib/util.js b/src/lib/util.js index c4859f3d3..87b148310 100644 --- a/src/lib/util.js +++ b/src/lib/util.js @@ -1,7 +1,10 @@ import Uri from 'jsuri'; import { decode } from 'box-react-ui/lib/utils/keys'; +import api from './api'; import DownloadReachability from './DownloadReachability'; import Location from './Location'; +import PreviewError from './PreviewError'; +import { ERROR_CODE } from './events'; const HEADER_CLIENT_NAME = 'X-Box-Client-Name'; const HEADER_CLIENT_VERSION = 'X-Box-Client-Version'; @@ -294,6 +297,24 @@ export function prefetchAssets(urls, preload = false) { }); } +/** + * Method to fetch a given representation as a blob via an authenticated content URL. + * Useful for determining the true state of a "successful" representation + * + * @param {string} contentUrl - a representation's authenticated content URL + * @return {Promise} - Resolves with the representation data, rejects with a deleted rep error + */ +export function fetchRepresentationAsBlob(contentUrl) { + return api.get(contentUrl, { type: 'blob' }).then((response) => { + if (response.status && response.status === 202) { + const error = new PreviewError(ERROR_CODE.DELETED_REPS, __('error_refresh'), { isRepDeleted: true }); + return Promise.reject(error); + } + + return Promise.resolve(response); + }); +} + /** * Loads external stylsheets by appending a element * diff --git a/src/lib/viewers/image/ImageBaseViewer.js b/src/lib/viewers/image/ImageBaseViewer.js index 98eadc577..a3ba4044d 100644 --- a/src/lib/viewers/image/ImageBaseViewer.js +++ b/src/lib/viewers/image/ImageBaseViewer.js @@ -13,6 +13,9 @@ const CSS_CLASS_ZOOMABLE = 'zoomable'; const CSS_CLASS_PANNABLE = 'pannable'; class ImageBaseViewer extends BaseViewer { + /** @property {string} - Url used to download an image representation */ + downloadUrl; + /** @inheritdoc */ constructor(options) { super(options); @@ -320,9 +323,8 @@ class ImageBaseViewer extends BaseViewer { // eslint-disable-next-line console.error(err); - // Display a generic error message but log the real one - const error = new PreviewError(ERROR_CODE.CONTENT_DOWNLOAD, __('error_refresh'), {}, err.message); - super.handleDownloadError(error, imgUrl); + const genericDownloadError = new PreviewError(ERROR_CODE.CONTENT_DOWNLOAD, __('error_refresh')); + super.handleDownloadError(err instanceof PreviewError ? err : genericDownloadError, imgUrl); } /** diff --git a/src/lib/viewers/image/ImageViewer.js b/src/lib/viewers/image/ImageViewer.js index 9da815cb1..95d741c7c 100644 --- a/src/lib/viewers/image/ImageViewer.js +++ b/src/lib/viewers/image/ImageViewer.js @@ -57,12 +57,12 @@ class ImageViewer extends ImageBaseViewer { const { representation, viewer } = this.options; const template = representation.content.url_template; - const downloadUrl = this.createContentUrlWithAuthParams(template, viewer.ASSET); + this.downloadUrl = this.createContentUrlWithAuthParams(template, viewer.ASSET); this.bindDOMListeners(); return this.getRepStatus() .getPromise() - .then(() => this.handleAssetAndRepLoad(downloadUrl)) + .then(() => this.handleAssetAndRepLoad()) .catch(this.handleAssetError); } @@ -72,11 +72,18 @@ class ImageViewer extends ImageBaseViewer { * @override * @return {void} */ - handleAssetAndRepLoad(downloadUrl) { + handleAssetAndRepLoad() { this.startLoadTimer(); - this.imageEl.src = downloadUrl; - super.handleAssetAndRepLoad(); + util + .fetchRepresentationAsBlob(this.downloadUrl) + .then((blob) => { + const srcUrl = URL.createObjectURL(blob); + this.imageEl.src = srcUrl; + + super.handleAssetAndRepLoad(); + }) + .catch(this.handleImageDownloadError); } /** @@ -374,7 +381,7 @@ class ImageViewer extends ImageBaseViewer { * @return {void} */ handleImageDownloadError(err) { - this.handleDownloadError(err, this.imageEl.src); + this.handleDownloadError(err, this.downloadUrl); } /** diff --git a/src/lib/viewers/image/MultiImageViewer.js b/src/lib/viewers/image/MultiImageViewer.js index 52ae0ee90..7950f11bc 100644 --- a/src/lib/viewers/image/MultiImageViewer.js +++ b/src/lib/viewers/image/MultiImageViewer.js @@ -3,7 +3,7 @@ import PageControls from '../../PageControls'; import './MultiImage.scss'; import { ICON_FULLSCREEN_IN, ICON_FULLSCREEN_OUT } from '../../icons/icons'; import { CLASS_INVISIBLE, CLASS_MULTI_IMAGE_PAGE } from '../../constants'; -import { pageNumberFromScroll } from '../../util'; +import { pageNumberFromScroll, fetchRepresentationAsBlob } from '../../util'; const PADDING_BUFFER = 100; const CSS_CLASS_IMAGE = 'bp-images'; @@ -148,7 +148,14 @@ class MultiImageViewer extends ImageBaseViewer { this.singleImageEls[index].setAttribute('data-page-number', index + 1); this.singleImageEls[index].classList.add(CLASS_MULTI_IMAGE_PAGE); - this.singleImageEls[index].src = imageUrl; + this.downloadUrl = imageUrl; + + fetchRepresentationAsBlob(imageUrl) + .then((blob) => { + const srcUrl = URL.createObjectURL(blob); + this.singleImageEls[index].src = srcUrl; + }) + .catch(this.handleMultiImageDownloadError); } /** @inheritdoc */ @@ -271,12 +278,10 @@ class MultiImageViewer extends ImageBaseViewer { 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); + this.handleDownloadError(err, this.downloadUrl); } /** From 88f6551221338469c6edc012948729607a4b159d Mon Sep 17 00:00:00 2001 From: Jeremy Press Date: Tue, 26 Mar 2019 11:19:52 -0700 Subject: [PATCH 2/4] Chore: Fix tests and PR feedback --- src/lib/util.js | 46 +++++++++++-------- src/lib/viewers/image/ImageBaseViewer.js | 2 +- src/lib/viewers/image/ImageViewer.js | 3 +- .../image/__tests__/ImageViewer-test.js | 21 ++++----- .../image/__tests__/MultiImageViewer-test.js | 16 +++++-- 5 files changed, 51 insertions(+), 37 deletions(-) diff --git a/src/lib/util.js b/src/lib/util.js index 87b148310..54acd6af8 100644 --- a/src/lib/util.js +++ b/src/lib/util.js @@ -297,24 +297,6 @@ export function prefetchAssets(urls, preload = false) { }); } -/** - * Method to fetch a given representation as a blob via an authenticated content URL. - * Useful for determining the true state of a "successful" representation - * - * @param {string} contentUrl - a representation's authenticated content URL - * @return {Promise} - Resolves with the representation data, rejects with a deleted rep error - */ -export function fetchRepresentationAsBlob(contentUrl) { - return api.get(contentUrl, { type: 'blob' }).then((response) => { - if (response.status && response.status === 202) { - const error = new PreviewError(ERROR_CODE.DELETED_REPS, __('error_refresh'), { isRepDeleted: true }); - return Promise.reject(error); - } - - return Promise.resolve(response); - }); -} - /** * Loads external stylsheets by appending a element * @@ -737,3 +719,31 @@ export function isLocalStorageAvailable() { return false; } } + +/** + * Handles a blob response from axios. + * Generates a deleted reps error on a 202 response, which has no body and is unusable. + * + * @param {Object} response - A response from axios + * @return {Promise} - Resolves with the representation data, rejects with a deleted rep error + */ +export function handleRepresentationBlobFetch(response) { + const status = getProp(response, 'status'); + if (status === 202) { + const error = new PreviewError(ERROR_CODE.DELETED_REPS, __('error_refresh'), { isRepDeleted: true }); + return Promise.reject(error); + } + + return Promise.resolve(response); +} + +/** + * Method to fetch a given representation as a blob via an authenticated content URL. + * Useful for determining the true state of a "successful" representation + * + * @param {string} contentUrl - a representation's authenticated content URL + * @return {Promise} - Response from axios + */ +export function fetchRepresentationAsBlob(contentUrl) { + return api.get(contentUrl, { type: 'blob' }).then(handleRepresentationBlobFetch); +} diff --git a/src/lib/viewers/image/ImageBaseViewer.js b/src/lib/viewers/image/ImageBaseViewer.js index a3ba4044d..311814a3b 100644 --- a/src/lib/viewers/image/ImageBaseViewer.js +++ b/src/lib/viewers/image/ImageBaseViewer.js @@ -323,7 +323,7 @@ class ImageBaseViewer extends BaseViewer { // eslint-disable-next-line console.error(err); - const genericDownloadError = new PreviewError(ERROR_CODE.CONTENT_DOWNLOAD, __('error_refresh')); + const genericDownloadError = new PreviewError(ERROR_CODE.CONTENT_DOWNLOAD, __('error_refresh'), err.message); super.handleDownloadError(err instanceof PreviewError ? err : genericDownloadError, imgUrl); } diff --git a/src/lib/viewers/image/ImageViewer.js b/src/lib/viewers/image/ImageViewer.js index 95d741c7c..177043bc1 100644 --- a/src/lib/viewers/image/ImageViewer.js +++ b/src/lib/viewers/image/ImageViewer.js @@ -62,7 +62,7 @@ class ImageViewer extends ImageBaseViewer { this.bindDOMListeners(); return this.getRepStatus() .getPromise() - .then(() => this.handleAssetAndRepLoad()) + .then(this.handleAssetAndRepLoad) .catch(this.handleAssetError); } @@ -80,7 +80,6 @@ class ImageViewer extends ImageBaseViewer { .then((blob) => { const srcUrl = URL.createObjectURL(blob); this.imageEl.src = srcUrl; - super.handleAssetAndRepLoad(); }) .catch(this.handleImageDownloadError); diff --git a/src/lib/viewers/image/__tests__/ImageViewer-test.js b/src/lib/viewers/image/__tests__/ImageViewer-test.js index 81217cf62..ebd839c49 100644 --- a/src/lib/viewers/image/__tests__/ImageViewer-test.js +++ b/src/lib/viewers/image/__tests__/ImageViewer-test.js @@ -236,6 +236,8 @@ describe('lib/viewers/image/ImageViewer', () => { image.wrapperEl.style.height = '50px'; sandbox.stub(image, 'getRepStatus').returns({ getPromise: () => Promise.resolve() }); + sandbox.stub(util, 'fetchRepresentationAsBlob').returns(Promise.resolve(imageUrl)); + sandbox.stub(URL, 'createObjectURL').returns(imageUrl); image.setup(); image.load(imageUrl).catch(() => {}); }); @@ -273,21 +275,18 @@ describe('lib/viewers/image/ImageViewer', () => { }); }); - it('should zoom the width & height when the image rotated', () => { + it('should increase scale when the image is rotated', () => { sandbox.stub(image, 'isRotated').returns(true); + sandbox.stub(image, 'getTransformWidthAndHeight').returns({ + width: 100, + height: 100 + }); + stubs.setScale = sandbox.stub(image, 'setScale'); - image.imageEl.style.transform = 'rotate(90deg)'; - image.imageEl.style.width = '200px'; - image.imageEl.setAttribute('originalWidth', '150'); - image.imageEl.setAttribute('originalHeight', '100'); - image.imageEl.src = imageUrl; - const origImageSize = image.imageEl.getBoundingClientRect(); image.zoomIn(); - const newImageSize = image.imageEl.getBoundingClientRect(); - expect(newImageSize.width).gt(origImageSize.width); - expect(newImageSize.height).gt(origImageSize.height); + + expect(stubs.setScale).to.be.calledWith(undefined, 120); expect(stubs.adjustZoom).to.be.called; - image.imageEl.style.transform = ''; }); it('should reset dimensions and adjust padding when called with reset', () => { diff --git a/src/lib/viewers/image/__tests__/MultiImageViewer-test.js b/src/lib/viewers/image/__tests__/MultiImageViewer-test.js index 39e1b35c9..517e31c5c 100644 --- a/src/lib/viewers/image/__tests__/MultiImageViewer-test.js +++ b/src/lib/viewers/image/__tests__/MultiImageViewer-test.js @@ -210,11 +210,17 @@ describe('lib/viewers/image/MultiImageViewer', () => { expect(stubs.bindImageListeners).to.be.called; }); - it('should set the image source', () => { + it('should set the download URL and fetch the page as a blob', () => { + stubs.fetchRepresentationAsBlob = sandbox + .stub(util, 'fetchRepresentationAsBlob') + .returns(Promise.resolve('foo')); multiImage.singleImageEls = [stubs.singleImageEl]; + const repUrl = 'file/100/content/{page}.png'; - multiImage.setupImageEls('file/100/content/{page}.png', 0); - expect(multiImage.singleImageEls[0].src).to.be.equal('file/100/content/{page}.png'); + multiImage.setupImageEls(repUrl, 0); + + expect(multiImage.downloadUrl).to.be.equal(repUrl); + expect(stubs.fetchRepresentationAsBlob).to.be.called; }); it('should set the page number for each image el', () => { @@ -405,12 +411,12 @@ describe('lib/viewers/image/MultiImageViewer', () => { }); it('unbind the image listeners, clear the image Els array, and handle the download error', () => { - const { src } = multiImage.singleImageEls[0]; + multiImage.downloadUrl = multiImage.singleImageEls[0]; multiImage.handleMultiImageDownloadError('err'); expect(multiImage.singleImageEls).to.deep.equal([]); - expect(multiImage.handleDownloadError).to.be.calledWith('err', src); + expect(multiImage.handleDownloadError).to.be.calledWith('err', multiImage.downloadUrl); expect(multiImage.unbindImageListeners).to.be.calledTwice; }); }); From b72530621aa69e5ba238c4bd5e0f806ba53260f5 Mon Sep 17 00:00:00 2001 From: Jeremy Press Date: Tue, 26 Mar 2019 14:35:08 -0700 Subject: [PATCH 3/4] Chore: Cypress tests --- test/integration/sanity/DeletedReps.e2e.test.js | 10 ++++++++++ test/support/constants.js | 2 ++ 2 files changed, 12 insertions(+) diff --git a/test/integration/sanity/DeletedReps.e2e.test.js b/test/integration/sanity/DeletedReps.e2e.test.js index 5fd88c3bb..6a17e691d 100644 --- a/test/integration/sanity/DeletedReps.e2e.test.js +++ b/test/integration/sanity/DeletedReps.e2e.test.js @@ -3,7 +3,9 @@ describe('Previewing a file with deleted representations', () => { const token = Cypress.env('ACCESS_TOKEN'); const fileIdDoc = Cypress.env('FILE_ID_DOC'); + const fileIdImage = Cypress.env('FILE_ID_IMAGE'); const fileIdPresentation = Cypress.env('FILE_ID_PRESENTATION'); + const fileIdTiff = Cypress.env('FILE_ID_TIFF'); const REPS_ERROR = 'error_deleted_reps'; @@ -53,6 +55,14 @@ describe('Previewing a file with deleted representations', () => { { viewer: 'Presentation', fileId: fileIdPresentation + }, + { + viewer: 'Image', + fileId: fileIdImage + }, + { + viewer: 'MultiImage', + fileId: fileIdTiff } ].forEach((test) => { it(test.viewer, () => { diff --git a/test/support/constants.js b/test/support/constants.js index 52e39eaf2..1c665f226 100644 --- a/test/support/constants.js +++ b/test/support/constants.js @@ -3,11 +3,13 @@ Cypress.env({ FILE_ID_DOC: '415542803939', FILE_ID_DOC_LARGE: '420985736453', FILE_ID_HTML: '420872247534', + FILE_ID_IMAGE: '428416513276', FILE_ID_JS: '416043839991', FILE_ID_JSON: '420868959262', FILE_ID_MP3: '415542687401', FILE_ID_PRESENTATION_WITH_LINKS: '416052594867', FILE_ID_PRESENTATION: '415537552367', + FILE_ID_TIFF: '428410193698', FILE_ID_TXT: '420870357452', FILE_ID_VIDEO_SMALL: '415542846356', FILE_ID_VIDEO_SUBTITLES_TRACKS: '415542245854', From 6cc8a2c6ee635a2d27a8499c6ba41b7f14550bed Mon Sep 17 00:00:00 2001 From: Jeremy Press Date: Tue, 26 Mar 2019 15:05:30 -0700 Subject: [PATCH 4/4] Fix: Prevent duplicate multi image errors and more tests --- src/lib/__tests__/util-test.js | 22 +++++++++++++++++++ src/lib/viewers/image/ImageViewer.js | 2 +- src/lib/viewers/image/MultiImageViewer.js | 4 ++++ .../image/__tests__/ImageViewer-test.js | 6 +++-- .../image/__tests__/MultiImageViewer-test.js | 9 ++++++++ 5 files changed, 40 insertions(+), 3 deletions(-) diff --git a/src/lib/__tests__/util-test.js b/src/lib/__tests__/util-test.js index aebe21506..f26b76b60 100644 --- a/src/lib/__tests__/util-test.js +++ b/src/lib/__tests__/util-test.js @@ -1,6 +1,7 @@ /* eslint-disable no-unused-expressions */ import Location from '../Location'; import * as util from '../util'; +import { ERROR_CODE } from '../events'; import DownloadReachability from '../DownloadReachability'; const sandbox = sinon.sandbox.create(); @@ -706,4 +707,25 @@ describe('lib/util', () => { } ); }); + + describe('handleRepresentationBlobFetch()', () => { + it('should reject if the response is a 202', () => { + const response = { + status: 202 + }; + + util.handleRepresentationBlobFetch(response).catch((e) => expect(e.code).to.equal(ERROR_CODE.DELETED_REPS)); + }); + + it('should pass the response through', () => { + const response = { + status: 200, + body: 'body' + }; + + util + .handleRepresentationBlobFetch(response) + .then((passedResponse) => expect(passedResponse).to.equal(response)); + }); + }); }); diff --git a/src/lib/viewers/image/ImageViewer.js b/src/lib/viewers/image/ImageViewer.js index 177043bc1..3745507e6 100644 --- a/src/lib/viewers/image/ImageViewer.js +++ b/src/lib/viewers/image/ImageViewer.js @@ -75,7 +75,7 @@ class ImageViewer extends ImageBaseViewer { handleAssetAndRepLoad() { this.startLoadTimer(); - util + return util .fetchRepresentationAsBlob(this.downloadUrl) .then((blob) => { const srcUrl = URL.createObjectURL(blob); diff --git a/src/lib/viewers/image/MultiImageViewer.js b/src/lib/viewers/image/MultiImageViewer.js index 7950f11bc..d890617c6 100644 --- a/src/lib/viewers/image/MultiImageViewer.js +++ b/src/lib/viewers/image/MultiImageViewer.js @@ -274,6 +274,10 @@ class MultiImageViewer extends ImageBaseViewer { * @return {void} */ handleMultiImageDownloadError(err) { + if (this.isDestroyed()) { + return; + } + this.singleImageEls.forEach((el, index) => { this.unbindImageListeners(index); }); diff --git a/src/lib/viewers/image/__tests__/ImageViewer-test.js b/src/lib/viewers/image/__tests__/ImageViewer-test.js index ebd839c49..8984d407e 100644 --- a/src/lib/viewers/image/__tests__/ImageViewer-test.js +++ b/src/lib/viewers/image/__tests__/ImageViewer-test.js @@ -598,11 +598,13 @@ describe('lib/viewers/image/ImageViewer', () => { done(); }) ); + sandbox.stub(URL, 'URL.createObjectURL(blob)').returns(url); - image.handleAssetAndRepLoad(url); + image.handleAssetAndRepLoad(url).then(() => { + expect(imageEl.src).to.be.equal(url); + }); expect(startLoadTimer).to.be.called; - expect(imageEl.url).to.be.equal(url); expect(loadBoxAnnotations).to.be.called; expect(createAnnotator).to.be.called; }); diff --git a/src/lib/viewers/image/__tests__/MultiImageViewer-test.js b/src/lib/viewers/image/__tests__/MultiImageViewer-test.js index 517e31c5c..fa89dc9eb 100644 --- a/src/lib/viewers/image/__tests__/MultiImageViewer-test.js +++ b/src/lib/viewers/image/__tests__/MultiImageViewer-test.js @@ -410,6 +410,15 @@ describe('lib/viewers/image/MultiImageViewer', () => { sandbox.stub(multiImage, 'unbindImageListeners'); }); + it('should do nothing if the viewer is already destroyed', () => { + sandbox.stub(multiImage, 'isDestroyed').returns(true); + multiImage.downloadUrl = multiImage.singleImageEls[0]; + multiImage.handleMultiImageDownloadError('err'); + + expect(multiImage.handleDownloadError).to.not.be.called; + expect(multiImage.unbindImageListeners).to.not.be.called; + }); + it('unbind the image listeners, clear the image Els array, and handle the download error', () => { multiImage.downloadUrl = multiImage.singleImageEls[0];