From d8f3b86b0e79691a836ff3f1beac001847b9f272 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Wed, 15 Apr 2020 14:20:31 +0000 Subject: [PATCH 1/4] javascript: move auto-upload attachment to the Uploader class Rationale: - It makes more sense to handle the progress bar updates in a single class; - This will allow us to unify the error handling. --- .../dossiers/auto-upload-controller.js | 69 ++++--------------- .../shared/activestorage/uploader.js | 47 +++++++++++-- 2 files changed, 57 insertions(+), 59 deletions(-) diff --git a/app/javascript/new_design/dossiers/auto-upload-controller.js b/app/javascript/new_design/dossiers/auto-upload-controller.js index ad4eec39c..c33ff4e8e 100644 --- a/app/javascript/new_design/dossiers/auto-upload-controller.js +++ b/app/javascript/new_design/dossiers/auto-upload-controller.js @@ -1,6 +1,5 @@ import Uploader from '../../shared/activestorage/uploader'; -import ProgressBar from '../../shared/activestorage/progress-bar'; -import { ajax, show, hide, toggle } from '@utils'; +import { show, hide, toggle } from '@utils'; // Given a file input in a champ with a selected file, upload a file, // then attach it to the dossier. @@ -11,27 +10,19 @@ export default class AutoUploadController { constructor(input, file) { this.input = input; this.file = file; + this.uploader = new Uploader( + input, + file, + input.dataset.directUploadUrl, + input.dataset.autoAttachUrl + ); } async start() { try { this._begin(); - - // Sanity checks - const autoAttachUrl = this.input.dataset.autoAttachUrl; - if (!autoAttachUrl) { - throw new Error('L’attribut "data-auto-attach-url" est manquant'); - } - - // Upload the file (using Direct Upload) - let blobSignedId = await this._upload(); - - // Attach the blob to the champ - // (The request responds with Javascript, which displays the attachment HTML fragment). - await this._attach(blobSignedId, autoAttachUrl); - - // Everything good: clear the original file input value - this.input.value = null; + await this.uploader.start(); + this._succeeded(); } catch (error) { this._failed(error); throw error; @@ -45,35 +36,8 @@ export default class AutoUploadController { this._hideErrorMessage(); } - async _upload() { - const uploader = new Uploader( - this.input, - this.file, - this.input.dataset.directUploadUrl - ); - return await uploader.start(); - } - - async _attach(blobSignedId, autoAttachUrl) { - // Now that the upload is done, display a new progress bar - // to show that the attachment request is still pending. - const progressBar = new ProgressBar( - this.input, - `${this.input.id}-progress-bar`, - this.file - ); - progressBar.progress(100); - progressBar.end(); - - const attachmentRequest = { - url: autoAttachUrl, - type: 'PUT', - data: `blob_signed_id=${blobSignedId}` - }; - await ajax(attachmentRequest); - - // The progress bar has been destroyed by the attachment HTML fragment that replaced the input, - // so no further cleanup is needed. + _succeeded() { + this.input.value = null; } _failed(error) { @@ -81,12 +45,10 @@ export default class AutoUploadController { return; } - let progressBar = this.input.parentElement.querySelector('.direct-upload'); - if (progressBar) { - progressBar.remove(); - } + this.uploader.progressBar.destroy(); - this._displayErrorMessage(error); + let message = this._messageFromError(error); + this._displayErrorMessage(message); } _done() { @@ -126,11 +88,10 @@ export default class AutoUploadController { } } - _displayErrorMessage(error) { + _displayErrorMessage(message) { let errorNode = this.input.parentElement.querySelector('.attachment-error'); if (errorNode) { show(errorNode); - let message = this._messageFromError(error); errorNode.querySelector('.attachment-error-title').textContent = message.title || ''; errorNode.querySelector('.attachment-error-description').textContent = diff --git a/app/javascript/shared/activestorage/uploader.js b/app/javascript/shared/activestorage/uploader.js index b303a441e..04dd21be5 100644 --- a/app/javascript/shared/activestorage/uploader.js +++ b/app/javascript/shared/activestorage/uploader.js @@ -1,4 +1,5 @@ import { DirectUpload } from '@rails/activestorage'; +import { ajax } from '@utils'; import ProgressBar from './progress-bar'; import errorFromDirectUploadMessage from './errors'; @@ -7,29 +8,65 @@ import errorFromDirectUploadMessage from './errors'; used to track lifecycle and progress of an upload. */ export default class Uploader { - constructor(input, file, directUploadUrl) { + constructor(input, file, directUploadUrl, autoAttachUrl) { this.directUpload = new DirectUpload(file, directUploadUrl, this); this.progressBar = new ProgressBar(input, this.directUpload.id, file); + this.autoAttachUrl = autoAttachUrl; } - start() { + /** + Upload (and optionally attach) the file. + Returns the blob signed id on success. + */ + async start() { this.progressBar.start(); + try { + let blobSignedId = await this._upload(); + + if (this.autoAttachUrl) { + await this._attach(blobSignedId); + } + + this.progressBar.end(); + this.progressBar.destroy(); + + return blobSignedId; + } catch (error) { + this.progressBar.error(error.message); + throw error; + } + } + + /** + Upload the file using the DirectUpload instance, and return the blob signed_id. + */ + async _upload() { return new Promise((resolve, reject) => { this.directUpload.create((errorMsg, attributes) => { if (errorMsg) { - this.progressBar.error(errorMsg); let error = errorFromDirectUploadMessage(errorMsg); reject(error); } else { resolve(attributes.signed_id); } - this.progressBar.end(); - this.progressBar.destroy(); }); }); } + /** + Attach the file by sending a POST request to the autoAttachUrl. + */ + async _attach(blobSignedId) { + const attachmentRequest = { + url: this.autoAttachUrl, + type: 'PUT', + data: `blob_signed_id=${blobSignedId}` + }; + + await ajax(attachmentRequest); + } + uploadRequestDidProgress(event) { const progress = (event.loaded / event.total) * 100; if (progress) { From 432967bd769f36a1d8fbdfa2cebaeb839bbbdb9a Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Wed, 15 Apr 2020 16:29:15 +0200 Subject: [PATCH 2/4] javascript: make Uploader always throw the same kind of errors A DirectUpload may fail for several reasons, and return many types of errors (string, xhr response, Error objects, etc). For convenience, wrap all these errors in a FileUploadError object. - It makes easier for clients of the Uploader class to handle errors; - It allows to propagate the error code and failure responsability. --- .../dossiers/auto-upload-controller.js | 37 +++------- app/javascript/shared/activestorage/errors.js | 22 ------ .../shared/activestorage/file-upload-error.js | 67 +++++++++++++++++++ app/javascript/shared/activestorage/ujs.js | 2 +- .../shared/activestorage/uploader.js | 17 ++++- 5 files changed, 92 insertions(+), 53 deletions(-) delete mode 100644 app/javascript/shared/activestorage/errors.js create mode 100644 app/javascript/shared/activestorage/file-upload-error.js diff --git a/app/javascript/new_design/dossiers/auto-upload-controller.js b/app/javascript/new_design/dossiers/auto-upload-controller.js index c33ff4e8e..5ac04e7ad 100644 --- a/app/javascript/new_design/dossiers/auto-upload-controller.js +++ b/app/javascript/new_design/dossiers/auto-upload-controller.js @@ -18,6 +18,8 @@ export default class AutoUploadController { ); } + // Create, upload and attach the file. + // On failure, display an error message and throw a FileUploadError. async start() { try { this._begin(); @@ -55,36 +57,15 @@ export default class AutoUploadController { this.input.disabled = false; } - _isError422(error) { - // Ajax errors have an xhr attribute - if (error && error.xhr && error.xhr.status == 422) return true; - // Rails DirectUpload errors are returned as a String, e.g. 'Error creating Blob for "Demain.txt". Status: 422' - if (error && error.toString().includes('422')) return true; - - return false; - } - _messageFromError(error) { - let allowRetry = !this._isError422(error); + let message = error.message || error.toString(); + let canRetry = error.status && error.status != 422; - if ( - error.xhr && - error.xhr.status == 422 && - error.response && - error.response.errors && - error.response.errors[0] - ) { - return { - title: error.response.errors[0], - description: '', - retry: allowRetry - }; - } else { - return { - title: 'Une erreur s’est produite pendant l’envoi du fichier.', - description: error.message || error.toString(), - retry: allowRetry - }; + + return { + title: 'Le fichier n’a pas pu être envoyé.', + description: message, + retry: canRetry } } diff --git a/app/javascript/shared/activestorage/errors.js b/app/javascript/shared/activestorage/errors.js deleted file mode 100644 index 072e99b5c..000000000 --- a/app/javascript/shared/activestorage/errors.js +++ /dev/null @@ -1,22 +0,0 @@ -// Convert an error message returned by DirectUpload to a proper error object. -// -// This function has two goals: -// 1. Remove the file name from the DirectUpload error message -// (because the filename confuses Sentry error grouping) -// 2. Create each kind of error on a different line -// (so that Sentry knows they are different kind of errors, from -// the line they were created.) -export default function errorFromDirectUploadMessage(message) { - let matches = message.match(/ Status: [0-9]{1,3}/); - let status = (matches && matches[0]) || ''; - - if (message.includes('Error creating')) { - return new Error('Error creating file.' + status); - } else if (message.includes('Error storing')) { - return new Error('Error storing file.' + status); - } else if (message.includes('Error reading')) { - return new Error('Error reading file.' + status); - } else { - return new Error(message); - } -} diff --git a/app/javascript/shared/activestorage/file-upload-error.js b/app/javascript/shared/activestorage/file-upload-error.js new file mode 100644 index 000000000..360557956 --- /dev/null +++ b/app/javascript/shared/activestorage/file-upload-error.js @@ -0,0 +1,67 @@ +// Error while reading the file client-side +export const ERROR_CODE_READ = 'file-upload-read-error'; +// Error while creating the empty blob on the server +export const ERROR_CODE_CREATE = 'file-upload-create-error'; +// Error while uploading the blob content +export const ERROR_CODE_STORE = 'file-upload-store-error'; +// Error while attaching the blob to the record +export const ERROR_CODE_ATTACH = 'file-upload-attach-error'; + +// Failure on the client side (syntax error, error reading file, etc.) +export const FAILURE_CLIENT = 'file-upload-failure-client'; +// Failure on the server side (typically non-200 response) +export const FAILURE_SERVER = 'file-upload-failure-server'; +// Failure during the transfert (request aborted, connection lost, etc) +export const FAILURE_CONNECTIVITY = 'file-upload-failure-connectivity'; + +/** + Represent an error during a file upload. + */ +export default class FileUploadError extends Error { + constructor(message, status, code) { + super(message); + this.name = 'FileUploadError'; + this.status = status; + this.code = code; + } + + /** + Return the component responsible of the error (client, server or connectivity). + See FAILURE_* constants for values. + */ + get failureReason() { + let isNetworkError = this.code != ERROR_CODE_READ; + + if (isNetworkError && this.status != 0) { + return FAILURE_SERVER; + } else if (isNetworkError && this.status == 0) { + return FAILURE_CONNECTIVITY; + } else { + return FAILURE_CLIENT; + } + } +} + +// Convert an error message returned by DirectUpload to a proper error object. +// +// This function has two goals: +// 1. Remove the file name from the DirectUpload error message +// (because the filename confuses Sentry error grouping) +// 2. Create each kind of error on a different line +// (so that Sentry knows they are different kind of errors, from +// the line they were created.) +export function errorFromDirectUploadMessage(message) { + let matches = message.match(/ Status: [0-9]{1,3}/); + let status = (matches && parseInt(matches[0], 10)) || undefined; + + // prettier-ignore + if (message.includes('Error reading')) { + return new FileUploadError('Error reading file.', status, ERROR_CODE_READ); + } else if (message.includes('Error creating')) { + return new FileUploadError('Error creating file.', status, ERROR_CODE_CREATE); + } else if (message.includes('Error storing')) { + return new FileUploadError('Error storing file.', status, ERROR_CODE_STORE); + } else { + return new FileUploadError(message, status, undefined); + } +} diff --git a/app/javascript/shared/activestorage/ujs.js b/app/javascript/shared/activestorage/ujs.js index aa3530ec2..d990d2d87 100644 --- a/app/javascript/shared/activestorage/ujs.js +++ b/app/javascript/shared/activestorage/ujs.js @@ -1,5 +1,5 @@ import ProgressBar from './progress-bar'; -import errorFromDirectUploadMessage from './errors'; +import { errorFromDirectUploadMessage } from './file-upload-error'; import { fire } from '@utils'; const INITIALIZE_EVENT = 'direct-upload:initialize'; diff --git a/app/javascript/shared/activestorage/uploader.js b/app/javascript/shared/activestorage/uploader.js index 04dd21be5..ad9a2ffc1 100644 --- a/app/javascript/shared/activestorage/uploader.js +++ b/app/javascript/shared/activestorage/uploader.js @@ -1,7 +1,7 @@ import { DirectUpload } from '@rails/activestorage'; import { ajax } from '@utils'; import ProgressBar from './progress-bar'; -import errorFromDirectUploadMessage from './errors'; +import FileUploadError, { errorFromDirectUploadMessage, ERROR_CODE_ATTACH } from './file-upload-error'; /** Uploader class is a delegate for DirectUpload instance @@ -17,6 +17,7 @@ export default class Uploader { /** Upload (and optionally attach) the file. Returns the blob signed id on success. + Throws a FileUploadError on failure. */ async start() { this.progressBar.start(); @@ -40,6 +41,7 @@ export default class Uploader { /** Upload the file using the DirectUpload instance, and return the blob signed_id. + Throws a FileUploadError on failure. */ async _upload() { return new Promise((resolve, reject) => { @@ -56,6 +58,8 @@ export default class Uploader { /** Attach the file by sending a POST request to the autoAttachUrl. + Throws a FileUploadError on failure (containing the first validation + error message, if any). */ async _attach(blobSignedId) { const attachmentRequest = { @@ -64,7 +68,16 @@ export default class Uploader { data: `blob_signed_id=${blobSignedId}` }; - await ajax(attachmentRequest); + try { + await ajax(attachmentRequest); + } catch (e) { + let message = e.response && e.response.errors && e.response.errors[0]; + throw new FileUploadError( + message || 'Error attaching file.', + e.xhr.status, + ERROR_CODE_ATTACH + ); + } } uploadRequestDidProgress(event) { From 55788990da1c3be5390c2f2d4bc1d1cab2d9eb5e Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Wed, 15 Apr 2020 16:34:25 +0200 Subject: [PATCH 3/4] javascript: add a helpful message on connectivity error --- .../dossiers/auto-upload-controller.js | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/app/javascript/new_design/dossiers/auto-upload-controller.js b/app/javascript/new_design/dossiers/auto-upload-controller.js index 5ac04e7ad..a2780a65e 100644 --- a/app/javascript/new_design/dossiers/auto-upload-controller.js +++ b/app/javascript/new_design/dossiers/auto-upload-controller.js @@ -1,5 +1,6 @@ import Uploader from '../../shared/activestorage/uploader'; import { show, hide, toggle } from '@utils'; +import { FAILURE_CONNECTIVITY } from '../../shared/activestorage/file-upload-error'; // Given a file input in a champ with a selected file, upload a file, // then attach it to the dossier. @@ -61,11 +62,18 @@ export default class AutoUploadController { let message = error.message || error.toString(); let canRetry = error.status && error.status != 422; - - return { - title: 'Le fichier n’a pas pu être envoyé.', - description: message, - retry: canRetry + if (error.failureReason == FAILURE_CONNECTIVITY) { + return { + title: 'Le fichier n’a pas pu être envoyé.', + description: 'Vérifiez votre connexion à Internet, puis ré-essayez.', + retry: true + }; + } else { + return { + title: 'Le fichier n’a pas pu être envoyé.', + description: message, + retry: canRetry + }; } } From b2231e98d55381168eb978d98b9c785fa3f1c145 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Wed, 15 Apr 2020 14:35:39 +0000 Subject: [PATCH 4/4] javascript: don't report connectivity issues to Sentry --- .../new_design/dossiers/auto-uploads-controllers.js | 6 ++++++ app/javascript/shared/activestorage/ujs.js | 9 +++++++-- app/javascript/shared/activestorage/uploader.js | 5 ++++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/app/javascript/new_design/dossiers/auto-uploads-controllers.js b/app/javascript/new_design/dossiers/auto-uploads-controllers.js index a50b083ac..ad68b081d 100644 --- a/app/javascript/new_design/dossiers/auto-uploads-controllers.js +++ b/app/javascript/new_design/dossiers/auto-uploads-controllers.js @@ -1,5 +1,6 @@ import Rails from '@rails/ujs'; import AutoUploadController from './auto-upload-controller.js'; +import { FAILURE_CONNECTIVITY } from '../../shared/activestorage/file-upload-error'; // Manage multiple concurrent uploads. // @@ -17,6 +18,11 @@ export default class AutoUploadsControllers { try { let controller = new AutoUploadController(input, file); await controller.start(); + } catch (error) { + // Report errors to Sentry (except connectivity issues) + if (error.failureReason != FAILURE_CONNECTIVITY) { + throw error; + } } finally { this._decrementInFlightUploads(form); } diff --git a/app/javascript/shared/activestorage/ujs.js b/app/javascript/shared/activestorage/ujs.js index d990d2d87..bdc3b5031 100644 --- a/app/javascript/shared/activestorage/ujs.js +++ b/app/javascript/shared/activestorage/ujs.js @@ -1,5 +1,8 @@ import ProgressBar from './progress-bar'; -import { errorFromDirectUploadMessage } from './file-upload-error'; +import { + errorFromDirectUploadMessage, + FAILURE_CONNECTIVITY +} from './file-upload-error'; import { fire } from '@utils'; const INITIALIZE_EVENT = 'direct-upload:initialize'; @@ -56,7 +59,9 @@ addUploadEventListener(ERROR_EVENT, event => { ProgressBar.error(id, errorMsg); let error = errorFromDirectUploadMessage(errorMsg); - fire(document, 'sentry:capture-exception', error); + if (error.failureReason != FAILURE_CONNECTIVITY) { + fire(document, 'sentry:capture-exception', error); + } }); addUploadEventListener(END_EVENT, ({ detail: { id } }) => { diff --git a/app/javascript/shared/activestorage/uploader.js b/app/javascript/shared/activestorage/uploader.js index ad9a2ffc1..1d1dcce8b 100644 --- a/app/javascript/shared/activestorage/uploader.js +++ b/app/javascript/shared/activestorage/uploader.js @@ -1,7 +1,10 @@ import { DirectUpload } from '@rails/activestorage'; import { ajax } from '@utils'; import ProgressBar from './progress-bar'; -import FileUploadError, { errorFromDirectUploadMessage, ERROR_CODE_ATTACH } from './file-upload-error'; +import FileUploadError, { + errorFromDirectUploadMessage, + ERROR_CODE_ATTACH +} from './file-upload-error'; /** Uploader class is a delegate for DirectUpload instance