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.

Sign up for GitHub

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

Jump to bottom

add failing test about form submission responded with exceptional 422 #1006

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

add failing test about form submission responded with exceptional 422 #1006

wants to merge 3 commits into from

Conversation

gnattali
Copy link

@afcapel
Copy link
Collaborator

afcapel commented Sep 28, 2023 •
edited
Loading

@gnattali good catch! We changed how we handle form submissions that modify tracked assets in #759 That PR tries to avoid discarding the response to a form submission because there is no visit during a form submission.

However, the current behavior also feels quite unexpected. We reload the page if the response has any <head>, but not if it's a text message or empty body. And reloading the page may often be misleading. For example, if the form submission is a POST, the reload will be a GET instead and it's going to look like the page flashing, but not displaying any error messages.

I think 7dfbe14 should solve the problem. We use the ErrorRenderer to render the response from the server on a form submission error. ErrorRenderer merges the new and old <head> so any new styles should be available. JS state won't totally reset, but we can not do much about it.

//cc @kevinmcconnell

@gnattali
Copy link
Author

gnattali commented Oct 4, 2023

@afcapel
Thank you for your comment and commit!
I think your solution forces a full reload of assets on normal validation error response which is not necessary.
How about avoiding it this way?
9d3d6be
Any refactoring, correction, or even reverting is welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gnattali @afcapel