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.

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

more explicit error messages for file uploads #3347

Merged
merged 1 commit into from
Dec 3, 2020

Conversation

rigelk
Copy link
Collaborator

@rigelk rigelk commented Nov 23, 2020

Description

File uploads are often met with network errors. They can also be met with classic HTTP errors as defined in our spec. Both are dealt with (reset + notification), but notification messages only last 5 seconds, and their messages are not always describing the code correctly nor left in the console for later investigation. Since users uploading large files or with unleliable connections are not always monitoring their upload page, they might miss a temporary notification.

This PR brings:

  • sticky notification option
  • notifications log to the console
  • some upload errors now trigger sticky notifications
  • RP sets a header showing maximum file size for errors

@Chocobozzz are you interested by the maximum file size information being given by the reverse-proxy, or is it superfluous?

Related issues

This issue is related to #324 (comment), and
fixes #3050

Has this been tested?

  • 👍 yes, I added tests to the test suite
  • 👍 yes, light tests as follows are enough
  • 💭 no, because this PR is a draft and still needs work

@rigelk
Copy link
Collaborator Author

rigelk commented Nov 24, 2020

@Chocobozzz do you know how I could test that in the CI?

@Chocobozzz
Copy link
Owner

@Chocobozzz are you interested by the maximum file size information being given by the reverse-proxy, or is it superfluous?

This is a good idea

do you know how I could test that in the CI?

E2E CI?

@Chocobozzz
Copy link
Owner

What's the status of this PR? I would like to ship it in v3 since upload errors are very frustrating at the moment. But I'm not sure you should use the sticky attribute. Instead, I think we should display the error on top of the upload form, with a retry button.

@rigelk
Copy link
Collaborator Author

rigelk commented Dec 2, 2020

@Chocobozzz I’ll keep the notifier refactoring, as sticky messages might come in handy elsewhere.

@rigelk
Copy link
Collaborator Author

rigelk commented Dec 2, 2020

I changed the error display so that it both notifies and keep the message in the progressbar, which now has a retry and cancel button. Maybe some errors could disable the retry button (like error 413 which a retry will not help solve).

@rigelk rigelk force-pushed the more-errors branch 2 times, most recently from def0ac1 to 83c1a35 Compare December 2, 2020 21:50
@Chocobozzz
Copy link
Owner

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Obvious error message to replace "unknown error"
2 participants