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

LG-15171 Fixes redirect for PUT #11545

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

jennyverdeyen
Copy link
Contributor

🎫 Ticket

Link to the relevant ticket:
LG-15171

🛠 Summary of changes

Fixes redirect for put for state id routes renaming. This is so that the transition to the new routes can be made without users still being directed to the old routes.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Enter the application through Sinatra, choosing "Identity-verified"
  • Go through the IPP flow until the state id page
  • Enter an address with a comma and hit submit
  • Observe in the application logs that "PUT /in_person/state_id" occurs on submit, and no calls to "/in_person_proofing/state_id" are made.

changelog: Bug Fixes, In-person proofing, fixes redirect for put for state id routes renaming
@aduth
Copy link
Contributor

aduth commented Nov 25, 2024

While this would successfully redirect requests to /verify/in_person_proofing/state_id, if the idea is to get rid of those routes altogether, we'll need to address the fact that requests are going to that path in the first place, won't we?

@jennyverdeyen
Copy link
Contributor Author

jennyverdeyen commented Nov 25, 2024

@aduth This is meant to address the fact that requests are going to that path. This change also affects the behavior we were seeing where the old route was being called in the first place, upon an unsuccessful form submission. We are still trying to understand exactly why this would be the case, but if you run through the test steps manually you can see the route isn't being hit anymore at all (not even as a redirect to the correct one).

Edited to add more detail: what it seems like was happening is that having two routes mapping to the same controller action was causing weird behavior where the old route was being generated (I think it has to do with the url_for helper in the form). Before just fixing it to make it a redirect, I noticed that moving the "old" update route below the "new" one makes it so the url_for only picks the "new" one because it's defined first. But, just fixing it to be a redirect means that we no longer have two routes competing for the update action.

@jennyverdeyen jennyverdeyen requested review from a team and KeithNava November 25, 2024 15:04
@gina-yamada
Copy link
Contributor

gina-yamada commented Nov 25, 2024

Tested branch locally. I do not see in_person_proofing/state_id getting hit when I use a comma on the state id page (as I observed on main). It would be great to get an approver from another @18F/identity-joy-engineers as Shane and I paired with Jenny.

Copy link
Contributor

@eileen-nava eileen-nava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed and approved the PR. I also discussed it with Jenny for a bit on a call.

@jennyverdeyen jennyverdeyen merged commit c45cac1 into main Nov 25, 2024
2 checks passed
@jennyverdeyen jennyverdeyen deleted the jverdeyen/LG-15171-fix-redirect-state-id branch November 25, 2024 19:38
AShukla-GSA pushed a commit that referenced this pull request Nov 25, 2024
changelog: Bug Fixes, In-person proofing, fixes redirect for put for state id routes renaming
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.

4 participants