Skip to content

Use POST route consistently for phone creation#7944

Merged
aduth merged 1 commit intomainfrom
aduth-phone-setup-post
Mar 8, 2023
Merged

Use POST route consistently for phone creation#7944
aduth merged 1 commit intomainfrom
aduth-phone-setup-post

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Mar 7, 2023

🛠 Summary of changes

This pull request aims to address some incomplete refactoring started in #7826 and work toward reconciling (and hopefully consolidating) phone creation routes.

In #7826, the new spam protection screen reused POST routes for phone creation with the expectation that we would remove PATCH as a supported verb, reflected in routes.rb on main:

patch '/phone_setup' => 'users/phone_setup#create' # TODO: Remove after next deploy
post '/phone_setup' => 'users/phone_setup#create'

However, we never updated the existing reference to the PATCH route in app/views/users/phone_setup/index.html.erb, which is updated here.

This also removes a data- attribute which, as best I can tell, is not used. (It was added in #3253, though was never used from what I can see)

📜 Testing Plan

  1. Navigate to http://localhost:3000
  2. Create a new account
  3. Select phone as MFA
  4. Observe no issues in submitting phone number entry page

@aduth
Copy link
Contributor Author

aduth commented Mar 7, 2023

I should flag that this creates some inconsistency with other MFA setup paths which still use PATCH as a verb. I personally don't think PATCH is the best verb to be using for adding new MFA methods, though I'm open to reconsidering.

changelog: Internal, Code Quality, Reconcile routes for adding phone
@aduth aduth force-pushed the aduth-phone-setup-post branch from 6642cc6 to 3749a39 Compare March 7, 2023 20:54
@aduth aduth merged commit 8ef4a80 into main Mar 8, 2023
@aduth aduth deleted the aduth-phone-setup-post branch March 8, 2023 13:35
@jmdembe jmdembe mentioned this pull request Mar 9, 2023
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