Skip to content

LG-9634: "Use another phone number" when adding phone does not return user to phone page#8368

Merged
jmdembe merged 15 commits intomainfrom
LG-9634-use-another-phone-number
May 17, 2023
Merged

LG-9634: "Use another phone number" when adding phone does not return user to phone page#8368
jmdembe merged 15 commits intomainfrom
LG-9634-use-another-phone-number

Conversation

@jmdembe
Copy link
Contributor

@jmdembe jmdembe commented May 9, 2023

🎫 Ticket

LG-9634: "Use another phone number" when adding phone does not return user to phone page

🛠 Summary of changes

Expected behavior: When a user has an existing account and wants to set up an additional phone number, they should be able to click "Use another phone number" to restart the phone setup with a new number.

Current behavior: When you click "Use another phone number", we are then re-routed to the account page.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can remove this entire check. It was added in #8265 because this controller doesn't have the same authentication requirements in terms of re-authentication. Is there a way to fix this check so it allows a user in account setup to access it, but prevents users that are no longer in account setup from using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am messing around with this, but this check failing causes the bug that we are trying to address. What we want is that in any instance of a unconfirmed phone number, whether it be during account setup or after, we want to provide a flow that will allow a user to return to the phone setup path one the "Enter your one-time code" screen.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Can we add a feature spec which checks this behavior to avoid any future regressions? I feel like this is one flow where we've had a handful of regressions already, so it'd be good to include.

changelog: Bug fix, phone setup, 'use another phone number' goes to phone setup
@jmdembe jmdembe force-pushed the LG-9634-use-another-phone-number branch from a175d23 to 5bb649e Compare May 10, 2023 17:48
@jmdembe jmdembe force-pushed the LG-9634-use-another-phone-number branch from 4269a1d to 80dee57 Compare May 12, 2023 15:59
@jmdembe jmdembe marked this pull request as draft May 12, 2023 19:15
@jmdembe jmdembe marked this pull request as ready for review May 15, 2023 16:32
@mitchellhenke mitchellhenke self-requested a review May 15, 2023 16:35
@jmdembe jmdembe requested review from a team and aduth May 15, 2023 16:51
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

if unconfirmed_phone
{
url: phone_setup_path,
url: add_phone_path,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think one of the problems we keep running into is that we have separate controllers for setting up a phone for a new account vs. adding one to an existing account. These changes do fix the issue since it allows a user to continue entering a phone, but it still creates a "jump" where someone may start at the phone_setup view or add_phone view, and here we'll always direct them back to add_phone, and not actually where they started from.

This seems like a good fix. I think it also leads nicely into a follow-up to consolidate to one controller, since Users::PhoneController could now probably handle the phone setup flow with the change to use confirm_user_authenticated_for_2fa_setup instead of confirm_two_factor_authenticated? What do you think? Should we write a ticket for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with consolidating the controllers. Let's write a ticket for that work.

@jmdembe jmdembe merged commit 9375872 into main May 17, 2023
@jmdembe jmdembe deleted the LG-9634-use-another-phone-number branch May 17, 2023 14:57
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