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

Fix: Fail gracefully when previewing a doc with deleted reps #962

Merged
merged 6 commits into from
Mar 25, 2019
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
1 change: 1 addition & 0 deletions src/lib/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export const ERROR_CODE = {
UNSUPPORTED_FILE_TYPE: 'error_unsupported_file_type',
PERMISSIONS_PREVIEW: 'error_permissions_preview',
BAD_INPUT: 'error_bad_input',
DELETED_REPS: 'error_deleted_reps',
LOAD_ANNOTATIONS: 'error_load_annotations',
LOAD_ASSET: 'error_load_asset',
LOAD_CSV: 'error_load_csv',
Expand Down
4 changes: 3 additions & 1 deletion src/lib/viewers/BaseViewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,9 @@ class BaseViewer extends EventEmitter {
* @return {void}
*/
handleDownloadError(err, downloadURL) {
if (this.hasRetriedContentDownload) {
const isRepDeleted = getProp(err, 'details.isRepDeleted', false);

if (this.hasRetriedContentDownload || isRepDeleted) {
this.triggerError(err);
return;
}
Expand Down
17 changes: 16 additions & 1 deletion src/lib/viewers/__tests__/BaseViewer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -241,13 +241,28 @@ describe('lib/viewers/BaseViewer', () => {
sandbox.stub(base, 'emitMetric');
});

it('should trigger an error if we have already retried', () => {
it('should trigger an error if we have already retried', () => {
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 trigger an error if the rep was deleted', () => {
base.hasRetriedContentDownload = false;
base.handleDownloadError(
{
details: {
isRepDeleted: true
}
},
'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.hasRetriedContentDownload = false;
DownloadReachability.isCustomDownloadHost.returns(false);
Expand Down
13 changes: 12 additions & 1 deletion src/lib/viewers/doc/DocBaseViewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -662,8 +662,19 @@ class DocBaseViewer extends BaseViewer {
// eslint-disable-next-line
console.error(err);

// pdf.js gives us the status code in their error message
const { status } = err;

// Display a generic error message but log the real one
const error = new PreviewError(ERROR_CODE.CONTENT_DOWNLOAD, __('error_document'), {}, err.message);
const error =
status === 202
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does pdf.js recognize 202 as an error code or is this placeholder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's recognized as a success, but then PDF.js will throw a MissingPDFException, which includes the HTTP status code that led to the error.

? new PreviewError(
ERROR_CODE.DELETED_REPS,
__('error_refresh'),
{ isRepDeleted: true },
err.message
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we destructure message, as well?

)
: new PreviewError(ERROR_CODE.CONTENT_DOWNLOAD, __('error_document'), err.message);
this.handleDownloadError(error, pdfUrl);
});
}
Expand Down