fix(download): show appropriate download error #1081
Conversation
|
Verified that @priyajeet has signed the CLA. Thanks for the pull request! |
src/lib/Preview.js
Outdated
| const code = getProp(error, 'response.data.code'); | ||
| let msg = downloadErrorMsg; | ||
| if (code === 'forbidden_by_policy') { | ||
| msg = __('notification_cannot_download_due_to_policy'); |
There was a problem hiding this comment.
Can you create constants for these strings?
src/lib/Preview.js
Outdated
| */ | ||
| download() { | ||
| const downloadErrorMsg = __('notification_cannot_download'); | ||
| if (!canDownload(this.file, this.options)) { | ||
| this.ui.showNotification(downloadErrorMsg); | ||
| return; | ||
| return Promise.reject(); |
There was a problem hiding this comment.
Should we reject with a reason?
src/lib/Preview.js
Outdated
| @@ -513,25 +513,27 @@ class Preview extends EventEmitter { | |||
| * Downloads the file being previewed. | |||
| * | |||
| * @public | |||
| * @return {void} | |||
| * @return {Promise} download promise | |||
There was a problem hiding this comment.
This is a public API for the preview instance, right? Is this considered a breaking change?
There was a problem hiding this comment.
I am actually gonna remove the return promise. Only downside is that this function is not really being tested properly, specially for errors.
| .get(getDownloadUrl, { headers: this.getRequestHeaders() }) | ||
| .then(data => { | ||
| const downloadUrl = appendQueryParams(data.download_url, queryParams); | ||
| this.api.reachability.downloadWithReachabilityCheck(downloadUrl); |
There was a problem hiding this comment.
Should this return the promise result from downloadWithReachabilityCheck?
| if (code === 'forbidden_by_policy') { | ||
| msg = __('notification_cannot_download_due_to_policy'); | ||
| } | ||
| this.ui.showNotification(msg); |
There was a problem hiding this comment.
Should we do a Promise.reject here?
src/lib/Preview.js
Outdated
| @@ -541,15 +543,25 @@ class Preview extends EventEmitter { | |||
| params, | |||
| ); | |||
|
|
|||
| this.api.reachability.downloadWithReachabilityCheck(downloadUrl); | |||
| downloadPromise = this.api.reachability.downloadWithReachabilityCheck(downloadUrl); | |||
There was a problem hiding this comment.
It doesn't look like downloadWithReachabilityCheck actually returns a promise?
| @@ -41,11 +41,11 @@ has_x_refs=This preview has references you cannot view. Open the file in its nat | |||
|
|
|||
| # Error messages | |||
| # Default preview error message | |||
| error_generic=We're sorry, the preview didn't load. | |||
| error_generic=We’re sorry, the preview didn't load. | |||
There was a problem hiding this comment.
What was wrong with the apostrophes?
There was a problem hiding this comment.
Normally for user facing strings one uses the accented versions. It also helps with not having the need to escape.
for when download might get disabled due to an access policy while the user is already previewing the file without browser refresh.
Also fixes the apostrophes