Resolve 500 error with dissemination search/advanced #4497
Conversation
Handled search AND advanced search. - Add form error labels for start and end dates in search. - Re-render search template instead of raising validation errors.
|
Terraform plan for meta No changes. Your infrastructure matches the configuration.✅ Plan applied in Deploy to Development and Management Environment #883 |
|
Terraform plan for dev Plan: 1 to add, 0 to change, 1 to destroy.Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
-/+ destroy and then create replacement
Terraform will perform the following actions:
# module.dev.module.cors.null_resource.cors_header must be replaced
-/+ resource "null_resource" "cors_header" {
!~ id = "*******************" -> (known after apply)
!~ triggers = { # forces replacement
!~ "always_run" = "2024-12-10T17:35:56Z" -> (known after apply)
}
}
Plan: 1 to add, 0 to change, 1 to destroy.✅ Plan applied in Deploy to Development and Management Environment #883 |
|
This does work well, just a couple questions/suggestions! No luck getting this to show up, like in the step 2 form? If it's not doable, I think we'd need to better indicate that the search failed due to an error. Right now you can't really tell until you expand the date section again. Maybe put a message where the search results show up, something like "Search submission failed" with a list of all the errors? Ideally the form should retain all the info the user submitted as well, if possible. I could see it being pretty frustrating if everything got wiped because you entered a bad date 😬 |
- Removed "duplicate" logic for rendering form when it is not valid. - Instead, re-uses the logic for success and only applies a search on the tables when `form.is_valid()`.
Hey Phil, Thanks for your feedback. That tooltip for the form field is generated by the browser, and it does not seem like we can easily guarantee a way to get it to generate without changing up some of the front-end logic quite a bit. I updated the PR with the following changes for both search AND advanced search. Please double check this on your end when you are ready! New USWDS error component renders on invalid form field. It looks like this: Your inputted fields will keep the values they had before submission (and the appropriate expanders are open/closed). The error will appear like this: Give it a test on both basic and advanced search. |
|
^ Relatedly, my browser won't give me an error the first time I put in a bad date. But if I get the bad date error from submitting and then go in with a bad date, my browser will let me know. Not the first time, when I would have needed it. Weird. Oh well.
|
| <div class="usa-alert usa-alert--error"> | ||
| <div class="usa-alert__body"> | ||
| <h2 class="usa-alert__heading">Error</h2> | ||
| <p>Oops! It looks like some of the information you entered is invalid. Please double-check your information for errors.</p> |
There was a problem hiding this comment.
Not sure about "Oops." Might be too friendly for the FAC's voice? @James-Paul-Mason, how do you feel?
We have a similar sort of message here:
There was a problem hiding this comment.
Once we get guidance from James on this I'm good with approving. Works great!
|
In terms of the error message, I would agree with @jperson1 that we should drop the "Oops!" for tonal consistency. Also, based on our voice/tone guide, we want our messaging to be authoritative, so I would delete "looks like"
From my reading of the ticket, it sounds like this error pops up when a user inputs an invalid End date. If that is the case, can we get more specific about "the information" in the error message, and replace it with something like this: "The date you entered is invalid. Please double-check the End date field for errors." Open to any suggestions if that sounds too specific! |
Now displaying list of erroneous fields in error message when the form is invalid.
Thanks @James-Paul-Mason, I updated the language to be authoritative. Regarding your other point, the changes in this PR will detect form errors beyond just the start/end dates in this form. Because the errors provided by Django are not always clear to the end user, I added some custom context to handle when the start/end dates are invalid. Like so:
I do not think it is possible for the other fields to have invalid input. Either way, moving forward we can provide custom information for them if needed. @phildominguez-gsa I think we may be good to go now. |
Fixes padding issue if there are no errors.
|
Addresses #4437.
How to test
PR Checklist: Submitter
maininto your branch shortly before creating the PR. (You should also be mergingmaininto your branch regularly during development.)git status | grep migrations. If there are any results, you probably need to add them to the branch for the PR. Your PR should have only one new migration file for each of the component apps, except in rare circumstances; you may need to delete some and re-runpython manage.py makemigrationsto reduce the number to one. (Also, unless in exceptional circumstances, your PR should not delete any migration files.)PR Checklist: Reviewer
make docker-clean; make docker-first-run && docker compose up; then rundocker compose exec web /bin/bash -c "python manage.py test"The larger the PR, the stricter we should be about these points.
Pre Merge Checklist: Merger
-/+ resource "null_resource" "cors_header"should be destroying and recreating its self and~ resource "cloudfoundry_app" "clamav_api"might be updating itssha256for thefac-file-scannerandfac-av-${ENV}by default.main.