Fix: Fail intentionally when preview image files with deleted reps #964
Conversation
|
Verified that @jeremypress has signed the CLA. Thanks for the pull request! |
src/lib/util.js
Outdated
| * @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) { |
There was a problem hiding this comment.
I added this to util because the DocPreloader will need it when fetching first page images. That class lives outside of the normal viewer inheritance hierarchy.
There was a problem hiding this comment.
Is it possible for us to share the response handler function only, rather than the api call itself? That seems to fit more as a util, to me.
There was a problem hiding this comment.
It's possible, but I like the idea of keeping the response type in one place so that it's not repeated every time we use it.
| fetchRepresentationAsBlob(imageUrl) | ||
| .then((blob) => { | ||
| const srcUrl = URL.createObjectURL(blob); | ||
| this.singleImageEls[index].src = srcUrl; |
There was a problem hiding this comment.
Now that we're using axios, we can optimize here in the future by cancelling subsequent requests after we detect an initial failure.
src/lib/viewers/image/ImageViewer.js
Outdated
|
|
||
| this.bindDOMListeners(); | ||
| return this.getRepStatus() | ||
| .getPromise() | ||
| .then(() => this.handleAssetAndRepLoad(downloadUrl)) | ||
| .then(() => this.handleAssetAndRepLoad()) |
There was a problem hiding this comment.
You should be able to remove the wrapping arrow function.
src/lib/util.js
Outdated
| * @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) { |
There was a problem hiding this comment.
Is it possible for us to share the response handler function only, rather than the api call itself? That seems to fit more as a util, to me.
| // 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')); |
There was a problem hiding this comment.
do we not need the extra params passed into the PreviewError constructor?
There was a problem hiding this comment.
Nope, they're optional past the first param (error code)
There was a problem hiding this comment.
true, but this one should stay since it was there previously. I had modified this function before but it's not necessary anymore.
src/lib/util.js
Outdated
| * @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) => { |
There was a problem hiding this comment.
Can you pull out the callback into a method? It'll allow for easy testing
| // 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')); |
There was a problem hiding this comment.
Nope, they're optional past the first param (error code)
This download strategy can be applied to other "simple" viewers. We're now able to determine the http status in a 202 case, and caching still works as expected.
todo: