Skip to content

Replace GET form with link for regenerating backup codes#10464

Merged
aduth merged 8 commits intomainfrom
aduth-backup-code-s-form-link
Apr 22, 2024
Merged

Replace GET form with link for regenerating backup codes#10464
aduth merged 8 commits intomainfrom
aduth-backup-code-s-form-link

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Apr 18, 2024

🎫 Ticket

Supports LG-12716

🛠 Summary of changes

Updates the backup codes regenerate confirmation screen primary button to use a link (<a href="...">) in place of a <form action="..." method="get"><button>.

This is related to the comment at #10088 (comment), where it's theorized there may be misbehaving browser versions which incorrectly handle the form submission. By using an anchor tag link, it's hoped this resolves the unexpected requests we're seeing to the route planned for removal in #10088.

Related Slack thread: https://gsa-tts.slack.com/archives/C05MGJ72GU9/p1713455028604359

This also includes minor refactoring to use common conventions for rendering buttons and status pages.

📜 Testing Plan

Verify there is no regression in the behavior of regenerating backup codes.

  1. Go to http://localhost:3000
  2. Sign in
  3. From account dashboard, click "Get codes" if you don't already have backup codes
  4. Once you have backup codes, click "Get new codes" from the account dashboard
  5. Observe you still see the confirmation screen with primary "Yes, regenerate codes" primary and "Cancel" secondary buttons
  6. Click either/both buttons and verify you're redirected as expected

👀 Screenshots

There's no visual changes expected.

Screenshot for reference:

Screenshot 2024-04-18 at 3 49 05 PM

@aduth aduth requested review from a team and kevinsmaster5 April 18, 2024 19:56
aduth added 2 commits April 18, 2024 15:57
Previously, when specifying action, many call-sites would pass `method` through tag_options, but since `method` is now a top-level constructor argument for ButtonComponent, it isn't included in tag_options. These use-cases are typically well-suited for the new abstraction, so can be updated to use it directly
@aduth
Copy link
Contributor Author

aduth commented Apr 18, 2024

I wasn't expecting to have to do quite the refactoring entailed through 255566c, but as evidenced by the previous build failure and explained in the extended commit description, the new abstraction for ButtonComponent url / method options broke some assumptions for how action expected to receive its parameters. But these were almost always perfect use-cases for the abstraction that's now implemented, so fixing entailed refactoring to use the new abstraction.

Comment on lines -8 to -10
action: ->(**tag_options, &block) do
button_to(account_reauthentication_path, **tag_options, &block)
end,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like on this branch, after these changes, there are 26 action: usages left, would we consider removing all of them in a follow-up PR?

identity-idp:aduth-backup-code-s-form-link> git grep -A 3 ButtonComponent.new -- app | grep action: | wc -l
      26

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I can! I actually manually un-edited some additional files since they weren't strictly required to update, so as to keep review simpler. But definitely want to get everything migrated over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up at #10468

Copy link
Contributor

@kevinsmaster5 kevinsmaster5 left a comment

Choose a reason for hiding this comment

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

LGTM. Tested locally with commit e15505b

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.

3 participants