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.

Sign up for GitHub

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

Jump to bottom

Update: Add un-downloadable error case #667

Merged
merged 1 commit into from
Feb 20, 2018
Merged

Update: Add un-downloadable error case #667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 6 additions & 3 deletions src/i18n/en-US.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ error_password_protected=We're sorry the preview didn't load. This document is p
# Preview error message shown when conversion was unable to process the file at the given time.
error_try_again_later=We're sorry the preview didn't load. Please try again later.
# Preview error message shown when conversion failed due to file contents
error_bad_file=We're sorry the preivew didn't load. This file may be corrupted.
error_bad_file=We're sorry the preview didn't load. This file could not be converted.
# Preview error message shown when the file cannot be downloaded
error_not_downloadable=Oops! It looks like something is wrong with this file. We recommend that you upload a new version of this file or roll back to a previous version. Please contact us for more details. We apologize for the inconvenience.

# Media Preview
# Label for autoplay in media player
Expand Down Expand Up @@ -105,7 +107,6 @@ media_audio=Audio
# Label for alternate audio tracks in media player
track=Track


# 3D Preview
# Button tooltip for showing/hiding the list of animation clips
box3d_animation_clips=Animation clips
Expand Down Expand Up @@ -168,7 +169,6 @@ annotations_delete_error=We're sorry, the annotation could not be deleted.
# Text for when the authorization token is invalid
annotations_authorization_error=Your session has expired. Please refresh the page.


# Notifications
# Default text for notification button that dismisses notification
notification_button_default_text=Okay
Expand All @@ -177,6 +177,9 @@ notification_annotation_point_mode=Click anywhere to add a comment to the docume
# Notification message shown when user enters drawing annotation mode
notification_annotation_draw_mode=Press down and drag the pointer to draw on the document

# Link Text
link_contact_us=Contact Us

# File Types
# 360 degree video file type
360_videos=360-degree videos
Expand Down
13 changes: 12 additions & 1 deletion src/lib/Preview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ const KEYDOWN_EXCEPTIONS = ['INPUT', 'SELECT', 'TEXTAREA']; // Ignore keydown ev
const LOG_RETRY_TIMEOUT_MS = 500; // retry interval for logging preview event
const LOG_RETRY_COUNT = 3; // number of times to retry logging preview event
const MS_IN_S = 1000; // ms in a sec
const SUPPORT_URL = 'https://support.box.com';

// All preview assets are relative to preview.js. Here we create a location
// object that mimics the window location object and points to where
Expand Down Expand Up @@ -923,6 +924,16 @@ class Preview extends EventEmitter {
this.file = file;
this.logger.setFile(file);

// If file is not downloadable, trigger an error
if (file.is_download_available === false) {
const error = createPreviewError(ERROR_CODE.notDownloadable, __('error_not_downloadable'), null, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JustinHoldstock am I using createPreviewError() right here? I'm not clear on what the third param will be used for - I see it can be any type, string, obj, Error. Are we parsing that at the WebApp level?

Also const error = createPreviewError(ERROR_CODE.prefetchFile, null, err); on https://github.com/box/box-content-preview/blob/master/src/lib/Preview.js#L550 might not be doing what you expect since null will be explicitly assigned to displayMessage. Default function parameters are only used when you pass no value or undefined: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters

Copy link
Contributor

Choose a reason for hiding this comment

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

The third param should be the details of the error. You can remove the third param, null.

The second param of the function is set to the displayMessage property of the error created. The third is set to the message property of the error created.

Aww hell, I forgot that null wouldn't work with default params. I will make a ticket to update anywhere using null

Copy link
Contributor

Choose a reason for hiding this comment

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

Just saw what you did there, with the fourth param. See comment below

linkText: __('link_contact_us'),
linkUrl: SUPPORT_URL
});
this.triggerError(error);
return;
}

// Keep reference to previously cached file version
const cachedFile = getCachedFile(this.cache, { fileVersionId: responseFileVersionId });

Expand Down Expand Up @@ -1497,7 +1508,7 @@ class Preview extends EventEmitter {
console.error(message);
/* eslint-enable no-console */

const error = createPreviewError(ERROR_CODE, message, filesToPrefetch);
const error = createPreviewError(ERROR_CODE.prefetchFile, message, filesToPrefetch);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JustinHoldstock we also want a specific code here right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. I apologize for missing that one. Thanks for catching it

this.emitPreviewError(error);
});
}
Expand Down
61 changes: 17 additions & 44 deletions src/lib/__tests__/Preview-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,8 @@ describe('lib/Preview', () => {
extension: 'pdf',
representations: {},
watermark_info: {},
authenticated_download_url: 'url'
authenticated_download_url: 'url',
is_download_available: true
}

preview.show(file, 'foken');
Expand Down Expand Up @@ -238,7 +239,8 @@ describe('lib/Preview', () => {
extension: 'docx',
representations: {},
watermark_info: {},
authenticated_download_url: 'url'
authenticated_download_url: 'url',
is_download_available: true
};
});

Expand Down Expand Up @@ -772,7 +774,8 @@ describe('lib/Preview', () => {
extension: 'pdf',
representations: {},
watermark_info: {},
authenticated_download_url: 'url'
authenticated_download_url: 'url',
is_download_available: true
};

stubs.promise = Promise.resolve({
Expand Down Expand Up @@ -1165,6 +1168,10 @@ describe('lib/Preview', () => {
setFile: sandbox.stub(),
setCacheStale: sandbox.stub()
};
preview.open = true;
preview.file = {
id: 0
};

stubs.getCachedFile = sandbox.stub(file, 'getCachedFile');
stubs.set = sandbox.stub(preview.cache, 'set');
Expand Down Expand Up @@ -1216,7 +1223,6 @@ describe('lib/Preview', () => {
});

it('should do nothing if response comes back for an incorrect file', () => {
preview.open = true;
preview.file = {
id: '123',
file_version: {
Expand All @@ -1230,22 +1236,19 @@ describe('lib/Preview', () => {
});

it('should save a reference to the file and update the logger', () => {
preview.open = true;
preview.file = {
id: 0
};

preview.handleFileInfoResponse(stubs.file);
expect(preview.file).to.equal(stubs.file);
expect(preview.logger.setFile).to.be.called;
});

it('should get the latest cache, then update it with the new file', () => {
preview.open = true;
preview.file = {
id: 0
};
it('should trigger an error if file is not downloadable', () => {
stubs.file.is_download_available = false;
preview.handleFileInfoResponse(stubs.file);
expect(stubs.triggerError).to.be.called;
expect(stubs.loadViewer).to.not.be.called;
});

it('should get the latest cache, then update it with the new file', () => {
stubs.getCachedFile.returns({
file_version: {
sha1: 0
Expand All @@ -1262,11 +1265,6 @@ describe('lib/Preview', () => {

it('should uncache the file if the file is watermarked', () => {
stubs.isWatermarked.returns(true);
preview.open = true;
preview.file = {
id: 0
};

stubs.getCachedFile.returns({
file_version: {
sha1: 0
Expand All @@ -1280,35 +1278,20 @@ describe('lib/Preview', () => {
});

it('should load the viewer if the file is not in the cache', () => {
preview.open = true;
preview.file = {
id: 0
};

stubs.getCachedFile.returns(null);

preview.handleFileInfoResponse(stubs.file);
expect(stubs.loadViewer).to.be.called;
});

it('should load the viewer if the cached file is not valid', () => {
preview.open = true;
preview.file = {
id: 0
};

stubs.checkFileValid.returns(false);

preview.handleFileInfoResponse(stubs.file);
expect(stubs.loadViewer).to.be.called;
});

it('should set the cache stale and re-load the viewer if the cached sha1 does not match the files sha1', () => {
preview.open = true;
preview.file = {
id: 0
};

stubs.getCachedFile.returns({
file_version: {
sha1: 0
Expand All @@ -1323,11 +1306,6 @@ describe('lib/Preview', () => {
});

it('should set the cache stale and re-load the viewer if the file is watermarked', () => {
preview.open = true;
preview.file = {
id: 0
};

stubs.isWatermarked.returns(true);
stubs.getCachedFile.returns({
file_version: {
Expand All @@ -1343,11 +1321,6 @@ describe('lib/Preview', () => {
});

it('should trigger an error if any cache or load operations fail', () => {
preview.open = true;
preview.file = {
id: 0
};

stubs.getCachedFile.throws(new Error());

preview.handleFileInfoResponse(stubs.file);
Expand Down
10 changes: 6 additions & 4 deletions src/lib/__tests__/file-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ describe('lib/file', () => {
it('should return the correct api url', () => {
assert.equal(
getURL('id', '', 'api'),
'api/2.0/files/id?fields=id,permissions,shared_link,sha1,file_version,name,size,extension,representations,watermark_info,authenticated_download_url'
'api/2.0/files/id?fields=id,permissions,shared_link,sha1,file_version,name,size,extension,representations,watermark_info,authenticated_download_url,is_download_available'
);
});

it('should return the correct API url for file version', () => {
assert.equal(
getURL('id', 'versionId', 'api'),
'api/2.0/files/id/versions/versionId?fields=id,permissions,shared_link,sha1,file_version,name,size,extension,representations,watermark_info,authenticated_download_url'
'api/2.0/files/id/versions/versionId?fields=id,permissions,shared_link,sha1,file_version,name,size,extension,representations,watermark_info,authenticated_download_url,is_download_available'
);
});
});
Expand Down Expand Up @@ -121,7 +121,8 @@ describe('lib/file', () => {
extension: 'blah',
representations: {},
watermark_info: {},
authenticated_download_url: 'blah'
authenticated_download_url: 'blah',
is_download_available: true
};
assert.ok(checkFileValid(file));
});
Expand All @@ -139,7 +140,8 @@ describe('lib/file', () => {
extension: 'exe',
representations: {},
watermark_info: {},
authenticated_download_url: 'blah?version=file_version_123'
authenticated_download_url: 'blah?version=file_version_123',
is_download_available: true
};

const file = normalizeFileVersion(fileVersion, fileId);
Expand Down
10 changes: 10 additions & 0 deletions src/lib/__tests__/logUtils-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,5 +80,15 @@ describe('lib/logUtils', () => {

expect(err.displayMessage).to.equal(errRefresh);
});

it('should append optional properties to error if provided', () => {
const err = createPreviewError('', '', null, {
foo: 'bar',
some: 'value'
});

expect(err.foo).to.equal('bar');
expect(err.some).to.equal('value');
});
});
});
3 changes: 2 additions & 1 deletion src/lib/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ export const ERROR_CODE = {
prefetchFile: 'error_prefetch_file',
rateLimit: 'error_rate_limit',
retriesExceeded: 'error_retries_exceeded',
browserError: 'error_browser_thrown'
browserError: 'error_browser_thrown',
notDownloadable: 'error_file_not_downloadable'
};

export const PREVIEW_LOAD_EVENT = '';
Expand Down
3 changes: 2 additions & 1 deletion src/lib/file.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ const FILE_FIELDS = [
'extension',
'representations',
'watermark_info',
'authenticated_download_url'
'authenticated_download_url',
'is_download_available'
];

/**
Expand Down
9 changes: 8 additions & 1 deletion src/lib/logUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,20 @@ export function getClientLogDetails() {
* @param {string} code - Code associated with the error that occurred.
* @param {string} [displayMessage] - Optional string to display, if applicable.
* @param {*} [details] - Additional details about the error. IE) File id, reason, etc.
* @param {*} [properties] - Additional optional properties on the error
* @return {Error} An error object with an associated error code.
*/
export function createPreviewError(code, displayMessage = __('error_refresh'), details = null) {
export function createPreviewError(code, displayMessage = __('error_refresh'), details = null, properties) {
const error = new Error(code);
error.code = code;
error.message = details || displayMessage;
error.displayMessage = displayMessage;

if (properties) {
Object.keys(properties).forEach((key) => {
error[key] = properties[key];
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JustinHoldstock I wonder if there's an opportunity to combine details and properties here. Maybe I can put linkText and linkUrl inside details and check that? It just feels a bit odd to be checking error.message.linkText and error.message.linkUrl in my logic in PreviewErrorViewer.

Open to other thoughts here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say anything you want logged to the backend needs to be stored in message. If you want linkUrl and linkText to be sent with error details for debugging purposes, I'd agree that it makes sense to fold them into details. My only issue with that is we end up sending a translated string.

Copy link
Contributor

Choose a reason for hiding this comment

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

One other note, I'm quite opposed to us dynamically creating properties on an already defined object type (Error). Now may be a good time for us to consider having our own class that extends Error, with our additional props on it, and only using that when creating errors in Preview.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JustinHoldstock are you okay with my merging this as-is and then following up with a refactor? There are a lot of places I'd have to touch for a Previewer for refactor and 1) I'd like to get this change in before the release cut tomorrow and 2) I probably want that larger change in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely! :D


return error;
}
8 changes: 6 additions & 2 deletions src/lib/viewers/error/PreviewError.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@
height: 247px; // Error icon + text + optional download button
text-align: center;

.bp-error-download {
padding-top: 20px;
.bp-error-text {
padding: 0 80px;
}

.bp-btn {
margin-top: 20px;
}
}
36 changes: 27 additions & 9 deletions src/lib/viewers/error/PreviewErrorViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,13 @@ class PreviewErrorViewer extends BaseViewer {
super.setup();

this.infoEl = this.containerEl.appendChild(document.createElement('div'));
this.infoEl.className = 'bp-error';

this.iconEl = this.infoEl.appendChild(document.createElement('div'));
this.iconEl.className = 'bp-icon bp-icon-file';

this.messageEl = this.infoEl.appendChild(document.createElement('div'));
this.infoEl.className = 'bp-error';
this.messageEl.className = 'bp-error-text';
}

/**
Expand Down Expand Up @@ -91,8 +94,11 @@ class PreviewErrorViewer extends BaseViewer {
this.iconEl.innerHTML = this.icon;
this.messageEl.textContent = displayMessage;

// Add optional download button
if (checkPermission(file, PERMISSION_DOWNLOAD) && showDownload && Browser.canDownload()) {
// Add optional link or download button
const { linkText, linkUrl } = err;
if (linkText && linkUrl) {
this.addLinkButton(linkText, linkUrl);
} else if (checkPermission(file, PERMISSION_DOWNLOAD) && showDownload && Browser.canDownload()) {
this.addDownloadButton();
}

Expand All @@ -111,17 +117,29 @@ class PreviewErrorViewer extends BaseViewer {
}

/**
* Adds optional download button
* Adds a link button underneath error message.
*
* @param {string} linkText - Translated button message
* @param {string} linkUrl - URL for link
* @return {void}
*/
addLinkButton(linkText, linkUrl) {
const linkBtnEl = this.infoEl.appendChild(document.createElement('a'));
linkBtnEl.className = 'bp-btn bp-btn-primary';
linkBtnEl.target = '_blank';
linkBtnEl.textContent = linkText;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be localized too?

Copy link
Contributor Author

@tonyjin tonyjin Feb 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'll add documentation to the JSDoc that this should be localized before passed in.

linkBtnEl.href = linkUrl;
}

/**
* Adds a download file button underneath error message.
*
* @private
* @return {void}
*/
addDownloadButton() {
this.downloadEl = this.infoEl.appendChild(document.createElement('div'));
this.downloadEl.classList.add('bp-error-download');
this.downloadBtnEl = this.downloadEl.appendChild(document.createElement('button'));
this.downloadBtnEl.classList.add('bp-btn');
this.downloadBtnEl.classList.add('bp-btn-primary');
this.downloadBtnEl = this.infoEl.appendChild(document.createElement('button'));
this.downloadBtnEl.className = 'bp-btn bp-btn-primary';
this.downloadBtnEl.textContent = __('download');
this.downloadBtnEl.addEventListener('click', this.download);
}
Expand Down
Loading