Update: Add un-downloadable error case #667
Conversation
|
Verified that @tonyjin has signed the CLA. Thanks for the pull request! |
| // Add optional download button | ||
| if (checkPermission(file, PERMISSION_DOWNLOAD) && showDownload && Browser.canDownload()) { | ||
| // Add optional link or download button | ||
| const { linkMsg, linkUrl } = err; |
There was a problem hiding this comment.
Where are we adding this linkUrl to the error object?
There was a problem hiding this comment.
In a separate patch when the field is ready.
There was a problem hiding this comment.
Ignore what I said, I've added the check for is_download_available into this patch now.
tonyjin
force-pushed
the
add-undownloadable-error-case
branch
from
a4f4564
to
da8bbc0
Compare
tonyjin
force-pushed
the
add-undownloadable-error-case
branch
from
da8bbc0
to
738bb4b
Compare
| @@ -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, { | |||
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Just saw what you did there, with the fourth param. See comment below
| @@ -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); | |||
There was a problem hiding this comment.
@JustinHoldstock we also want a specific code here right?
There was a problem hiding this comment.
Yes. I apologize for missing that one. Thanks for catching it
| Object.keys(properties).forEach((key) => { | ||
| error[key] = properties[key]; | ||
| }); | ||
| } |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
| const linkBtnEl = this.infoEl.appendChild(document.createElement('a')); | ||
| linkBtnEl.className = 'bp-btn bp-btn-primary'; | ||
| linkBtnEl.target = '_blank'; | ||
| linkBtnEl.textContent = linkText; |
There was a problem hiding this comment.
Should this be localized too?
There was a problem hiding this comment.
I'll add documentation to the JSDoc that this should be localized before passed in.
Show an error message when a file is undownloadable.
tonyjin
force-pushed
the
add-undownloadable-error-case
branch
from
98a35b1
to
6edd67e
Compare
Add error message for un-downloadable file and support for a link button instead of a download button in the error viewer.