Skip to content

LG-11517: Always show "Use another method" option in SMS opt-in#9661

Merged
aduth merged 2 commits intomainfrom
aduth-lg-11517-sms-opt-out-other-method
Nov 28, 2023
Merged

LG-11517: Always show "Use another method" option in SMS opt-in#9661
aduth merged 2 commits intomainfrom
aduth-lg-11517-sms-opt-out-other-method

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Nov 27, 2023

🎫 Ticket

LG-11517

🛠 Summary of changes

Updates the SMS opt-in screen to show the troubleshooting options module, always allowing a user to return to "Choose another authentication method" even if they don't have another MFA option available, so that the user can choose to reset (delete) their account if that's their only recourse.

📜 Testing Plan

  1. Go to http://localhost:3000
  2. Create a new account
  3. Add phone as only MFA method
  4. Once you complete account creation and reach sign-in page, click "Forget all browsers" and confirm
  5. Sign out
  6. Sign in
  7. Opt-out of SMS
    • Simulate this in local development by entering the following into a rails console session:
      PhoneNumberOptOut.create_or_find_with_phone(PhoneFormatter.format('+1 555-555-5555')) # swap with phone
      
    • To undo this later, delete the entry you created above:
      PhoneNumberOptOut.create_or_find_with_phone(PhoneFormatter.format('+1 555-555-5555')).delete # swap with phone
      
  8. Cancel sign-in
  9. Sign-in
  10. Observe that you can "Choose another authentication method", and notably that you can then select to "deleting your account" on the MFA selection screen

👀 Screenshots

SMS Opt-in Send Code:

Other MFAs available? Before After
Yes sms-opt-in-before-multiple sms-opt-in-after
No sms-opt-in-before-single sms-opt-in-after

SMS Opt-in Error:

Other MFAs available? Before After
Yes sms-opt-in-error-before-multiple sms-opt-in-error-after
No sms-opt-in-error-before-single sms-opt-in-error-after

changelog: User-Facing Improvements, SMS Opt-In, Provide a pathway to deleting one's account after opting-out SMS delivery
@aduth aduth requested a review from a team November 27, 2023 19:16
@@ -0,0 +1,9 @@
module TwoFactorAuthCode
class SmsOptInPresenter < GenericDeliveryPresenter
def initialize; end
Copy link
Contributor

Choose a reason for hiding this comment

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

(probably for another PR, just noting while I'm here)

this jumped out at me as unnecessary so I checked the superclass... that instance_variable_set... loop dynamically setting @-vars seems very unintuitive to me, I think that we might want to try removing that behavior? Two ideas:

  1. Since clearly each subclass needs to be instantiated with very different data, remove options from the base class and let each sublcass have its own constructor
  2. Make the DeliveryPresenter behavior into a mixin, let each subclass instantiate as needed?

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 don't love that behavior, and it's not clear (which is the problem) which values are actually used. I think some of the current base constructor options are unused (view, service_provider). I can plan to write up a ticket to rethink this.

@aduth aduth merged commit 1d018af into main Nov 28, 2023
@aduth aduth deleted the aduth-lg-11517-sms-opt-out-other-method branch November 28, 2023 14:19
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