Before, when a 401 was received by a ujs-enabled link (like `link_to …,
method: :delete, data: { remote: true }`, rails-ujs tried to insert the
response text as a Javascript script.
As the script was something like `Please sign-in`, which is not valid
Javascript, the browser would throw an "Unexpected token" error.
The typical use-case is:
1. The user open a form in a tab,
2. The user disconnects in another tab,
3. In the first tab, the user clicks on a remote "Delete" link_to
In that case the browser raised an error in the console (and in Sentry),
but the user would see nothing.
With this commit, all 401 ujs errors are turned into redirects to the
sign-in page.
Fix https://sentry.io/organizations/demarches-simplifiees/issues/2522512693/activity/
Sentry reports many cases of the xhr object being missing in the
error handler.
Ensure the error handling code doesn't crash because of the missing xhr.
There are two cases where the draft auto-save might fail because the
user is no longer authenticated:
- The user signed-out in another tab,
- The brower quit and re-opened, so the Session cookie expired.
In both cases, the auto-save will never succeed until the user
authenticates again, so displaying a "Retry" button is cruel.
Moreover, in plus of all auto-save requests failing with a small error,
the actual hard failure only occurs after filling all the form and
trying to submit it. Then the user is redirected to the sign-in page –
but all their changes are lost.
Instead, we now redirect to the sign-in page on the first 401 error
during the auto-save, let the user sign-in, and then redirect back to
the form.
Currently ProgressBar is used to monitor upload progress of attachments.
But there's two cases where the associated DOM element may be removed:
- In the champs editor, when the list scrolls, DOM elements are removed
and added dynamically by React;
- In the user form, the user might start an upload on a repetition, and
then remove the associated row during the download.
In both those cases, we don't want the missing DOM element to trigger
an error.
Before, when autosaving a draft, removing a repetition row would
send `_destroy` inputs to the controller – but not remove the row
from the DOM. This led to the `_destroy` inputs being sent again
on the next autosave request, which made the controller raise
(because the row fields were already deleted before).
To fix this, we let the controller response remove the deleted
row(s) from the DOM.
Doing it using a controller response avoids the need to keep track
of operations on the Javascript side: the controller can easily
know which row was just deleted, and emit the relevant changes for
the DOM. This keeps the autosave requests robust: even if a request
is skipped (e.g. because of a network interruption), the next request
will still contain the relevant informations to succeed, and not let the
form in an unstable state.
Fix#5470
When subclassing a JS error, most browsers include the constructor
stacktrace :/
This is an issue, because:
- The stacktrace is deeper than it should be
- The stacktrace reaches into a polyfill for which there is not source
map, which causes Sentry to infer the issue grouping from the JS file
name. And the fingerprinted name changes on each release. So for each
release, the stacktrace is different ; and Sentry can't group issues
properly.
We have quite a lot of `Error reading file` errors when uploading files.
These errors are generated by ActiveStorage `file_checksum.js` component
but it eats the actual reason of errors.
(See https://github.com/rails/rails/blob/5-2-stable/activestorage/app/javascript/activestorage/file_checksum.js#L38)
We can't really override the class to generate better errors, as they
are deeply nested in ActiveStorage class hierarchy, and not exported to
external code.
Instead, we hook into the FileReader event handler, to insert a logger
when this error occur. The original event handler will also still be
called as usual.
This is intended to be temporary. The debug code will be removed once
we get a better idea of what is going on.
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.
DirectUpload returns errors as strings, including an HTTP status and a
file name (and without a stack trace).
But Sentry groups issues according to the stack trace, and maybe the
error message in last resort.
So we have an issue: as all DirectUpload errors logged by Sentry are
generated on the same line, with random-looking messages, Sentry groups
them either too or too little aggressively.
Instead of creating all the errors on the same line:
- add some `if`s statements to create them on different lines (and so
with different stack traces),
- strip the file name from the error message.
This allows Sentry to group the errors properly, with meaningful error
messages.
When the authenticity token is invalid, the creation of the blob before
the direct upload returns a 422.
In that case, the token will never become valid again, and it is useless
to try again. Don’t show the "Retry" button in this case.
NB: of course the real fix is to understand why the authenticity token
is so often invalid – but this will be for later.
Pooling for attachment status is a background operation. Errors should
not be reported to the user, who didn't even ask for this operation to
take place.
This is why we ignore all errors, whether Javascript exceptions or
network errors.
This helper is:
- no longer used;
- buggy (not all requests increment it);
- discouraged (we should instead match an UI change that signals the end
of an ajax request).
Good riddance.
When navigating away from the page, the field receives the 'focusout'
event – but stops to be present in the DOM.
Thus we need to check that the DOM element is actually present.
When calling `redirect_to` in a Rails controller that emits Javascript,
Rails will emit `Turbolinks.replace(…)` commands. And for this,
Turbolinks needs to be globally available.
Otherwise some champs are created before the previous one has been
saved, and React complains that several champs have the same
`champ-undefined` key.
(Plus it made the tests flaky and unreliable.)
Before the editor attempted to create a default champ as soon as the
list became empty.
This created many race conditions, which made the tests flaky.
Remove this behavior, and add an empty label instead.
When a file is being uploaded asynchronosely, hidden input fields
are created by DirectUpload to contain the result of the upload.
However, before the upload finishes, the value of these inputs is not
meaningful. Moreover, it makes the ActiveRecord signature invalid – thus
preventing drafts from being saved.
Exclude these fields from the auto-save.
Older browsers implemented a different default value for `window.fetch`
credentials: it was omitted unless explicitely set.
So we force the value for these older browsers:
- Firefox 39-60
- Chrome 42-67
- Safari 10.1-11.1.2
See https://github.com/github/fetch#sending-cookies
Before, when the "Add new champ" button was clicked, the new champ
was inserted after the **first** fully visible champ.
That was most of the time unexpected. The correct behavior would be to
insert the new champ after the **last** fully visible champ.
That's what this commit does. Now the "Add new champ" behavior feels
much less confusing.
