Allow users to opt back in to SMS (LG-3510)#5894
Conversation
- Method was defined 4 times before, now it's only defined once - Updated it to leverage Phonelib for better grouping of digits (removes USA number assumption)
app/controllers/two_factor_authentication/sms_opt_in_controller.rb
Outdated
Show resolved
Hide resolved
…r.rb Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
|
This is ready for review! Just waiting on translations to come back, and then I'll add those |
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
As mentioned in original comment, a bit difficult to test, but looks good from what I can tell and what I can fake-test locally 👍
| user_session[:phone_id] = phone_configuration.id if phone_configuration&.phone | ||
| redirect_to login_two_factor_sms_opt_in_path |
There was a problem hiding this comment.
If the first line is unable to assign to the session due to a nil phone_configuration, would the redirect result in a 404?
i.e.
- Drop the
&.guarding if we're feeling confident it's there? - Otherwise, include the redirect in the
ifblock?
There was a problem hiding this comment.
There are other cases where we have :unconfirmed_phone in the session already and it still works
In an ideal world, I'd love to throw a /sms/:phone_configuration_id/opt_in in the URL to avoid this awkward session dance, but we only save confirmed phones to the DB :[
app/controllers/two_factor_authentication/sms_opt_in_controller.rb
Outdated
Show resolved
Hide resolved
| <% end %> | ||
|
|
||
| <%= render( | ||
| 'shared/troubleshooting_options', |
There was a problem hiding this comment.
More a UX question, but I sorta wonder if the "Can't use your phone?" fallback should be folded into the list of troubleshooting options.
There was a problem hiding this comment.
I think there's a case for keeping it where it is, because that one still keeps you in what you're doing, and the troubleshooting options are more like exit opportunities
There was a problem hiding this comment.
There's several places we use in-app alternative paths with a troubleshooting options list.
e.g.
IAL2 phone confirmation "use a different phone number"
IAL2 "verify by mail instead"
There was a problem hiding this comment.
Oh those are good points... 📡🦇🌕 @nickttng! If you have any thoughts on combining links for the error page
There was a problem hiding this comment.
@zachmargolis I think combining is a great way. 👍
There was a problem hiding this comment.
Updated in 2245973, error page looks like this now:
There was a problem hiding this comment.
@zachmargolis Should we update "Need immediate assistance? Here's how to get help" to "Having trouble? Here's what you can do"?
With the "Choose another authentication method" text now folded into the section, the "Need immediate assistance..." might not make sense with it. And "Having trouble..." text seems to encompass more of the potential solutions.
app/controllers/two_factor_authentication/sms_opt_in_controller.rb
Outdated
Show resolved
Hide resolved
…r.rb Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
|
After today's testing party, there is still some work to do before this is ready to be enabled in prod I have disabled the feature by default in 1a731d3 and I'm going to merge it, and address the outstanding issues in future PRs to keep this PR from getting too big or too stale |
4 participants
Would love some early eyes on this to make sure the direction looks good!
It's still in draft while I sort out a few things:Unfortunately I haven't figured out a good flow to test this locally
I have enabled the right API permissions in the sandbox so I'm going to push my branch up there and play around with it