Open m-natarajan opened 2 months ago
Triggered auto assignment to @trjExpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
Yeah, you shouldn't be able to set up 2FA with an unvalidated account, which I suspect is what's happening here, it's just not very good in the UI. We should improve the UI to prompt for validation as the first screen in the 2FA flow for an unvalidated account, so triggering a magic code and displaying an input to enter the code. After which, display the recovery codes and let the (now) validated user proceed to enable 2FA. CC: @Expensify/design for input on that.
@techievivek do you think that can be worked on externally, and I suspect might need a little help on some backend work to support putting that in place?
That makes sense to me.
Makes sense to me too.
The root cause is that we are not checking if the user is validated in SecuritySettingsPage before we allow the user to open 2FA page.
We should fetch user details from onyx using useOnyx(ONYXKEYS.USER)
. Then we will use user?.validated
prop to disable the 2FA menu item, hide the right icon, and alternatively display a hint text that says 'Verify your account first'.
disabled: !user?.validated,
shouldShowRightIcon: user?.validated,
hintText: !user?.validated && 'Verify your account first',
Note: we will fetch hintText
value using translate
method. And the changes concerning disabled
, shouldShowRightIcon
, and hintText
will be made in baseMenuItems
and baseMenuItems.map
code blocks too.
Result: Hint text might need some alignment improvement.
https://github.com/Expensify/App/assets/171434916/07c3ef96-501a-4bcc-8aac-74f877945e40
We can follow the same pattern used in BankAccountStep in CodesStep. This is where the user is navigated to when open 2FA page.
To do that we should:
user
data from onyx hereuser.validated
prop to disable 'Download', 'Copy' and 'Next' button user.validated
to decide, if we should display the same informative message displayed in BankAccountStep Note: We will gray out 'Download' and 'Copy' texts too.
Result:
https://github.com/Expensify/App/assets/171434916/70796044-1f58-462c-be05-2f03c328e019
We can also hide the whole 2FA code section and next button to make a clear UI and to have a clean implementation.
@trjExpensify, @techievivek Eep! 4 days overdue now. Issues have feelings too...
@techievivek what do you think of this proposal, and I take it we can send this external?
Job added to Upwork: https://www.upwork.com/jobs/~0145289401f3afe680
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External
)
This can be handled externally. Since the user is in an unvalidated state, they shouldn't be shown the 2FA page. We can manage this on the frontend, as the backend already ensures that recovery codes are not sent.
The recovery code is not visible on 2FA page (step 1).
We don't have a step to validate the account (if account is unvalidated) before showing the CodesStep
Add a step to validate the account (if account is unvalidated)
ValidateAccountStep
that will use the ValidateCodeForm
like the add secondary contact flow here. That page will send the code and wait for the user to enter the codes. The implementation details can be found in the ContactMethodDetailsPage
where there is a similar feature. That ValidateCodeStep
will navigate to the CodeSteps
upon successful validationCONST.TWO_FACTOR_AUTH_STEPS.VALIDATE_ACCOUNT
and return the ValidateAccountStep
if the currentStep
is that stepONYXKEYS.ACCOUNT
and in https://github.com/Expensify/App/blob/75614394a2fb1e8114e35bb2d8d33bdd0c565946/src/pages/settings/Security/TwoFactorAuth/TwoFactorAuthSteps.tsx#L40, check account?.validated
. If false
set currentStep
to CONST.TWO_FACTOR_AUTH_STEPS.VALIDATE_ACCOUNT
and set a local state hasValidateAccountStep
to true
(initialized as false
), if not still use the CONST.TWO_FACTOR_AUTH_STEPS.CODES
.hasValidateAccountStep
local state above to the relevant steps hereVALIDATE_ACCOUNT
) to correctly show the steps in case of an additional VALIDATE_ACCOUNT
step at the beginning. Here we can make use of the hasValidateAccountStep
prop above. If it's false the step counter here stays the same, if true
both step
and total
will be incremented by 1. Few other considerations:
CodesStep
to ValidateAccountStep
. I believe no, as the user was already validated. So going back from CodesStep
should dismiss the modal. Or we still show ValidateAccountStep
but shows a message that the user was already validatedThose are the main steps required to enable this behavior, there could be small UX adjustments/fixes that can be dealt with in PR phase.
NA
@shawnborton @dannymcclain @dubielzyk-expensify can you guys weigh in on the desired design for this flow. Do we want to add an additional step first before the recovery codes step is displayed for an unvalidated account? This might feel cleaner and we could style that page a bit better maybe.
Alternatively, do we want to replace the box that contains the recovery codes with the error message to prompt for validation. So hiding the box seen here, and just showing the error message:
The latter option is probably more consistent with the bank account flow that prompts for validation:
I don't mind either approach, but I do kinda like how your second suggestion is similar to the wallet flow. It feels a little less clear, but a bit more consistent π€·ββοΈ
If we went that route, I'd probably leave the box how it is (but make the buttons disabled like in the wallet flow) and maybe add some placeholder skeletons for where the codes should be? Or something like that? Curious for others' thoughts too.
Yeah I think the second way sounds alright too. That page is more sparse so I think an error message would stand out reasonably well
I don't feel strongly, but I'd probably hide the box and show the error to make it clearer where the action is, but I'm happy with Danny's suggestion too
I like the second idea, and I tend to agree with Jon here - I would just kill the box entirely and show the error message in its place.
That works for me!
Dope, let's do that then! @mollfpr if you could review the proposals. I think it mostly aligns with the alt proposed here, but with the addition of replacing the box with the error message.
Yup, will review it in a few minutes.
The alternative proposal from @etCoderDysto looks good to me!
π π π C+ reviewed!
@trjExpensify to confirm, should the next button be removed too, or just gray it out? Also, for the copy do we want to use the same copy from connect bank flow? If yes, we can create a new component only for the copy and use it on the connect bank page and 2FA page.
Current assignee @techievivek is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
to confirm, should the next button be removed too, or just gray it out?
I think we keep it but disable it, more consistent with forms that have errors.
for the copy do we want to use the same copy from connect bank flow?
Yeah, I think we keep the same copy as it's the same instruction!
@trjExpensify, @mollfpr, @techievivek Huh... This is 4 days overdue. Who can take care of this?
π£ @etCoderDysto You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing π
@trjExpensify @mollfpr @techievivek @etCoderDysto this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
I am working on a PR. I will comment when the PR is ready for review.
@trjExpensify, @mollfpr, @techievivek, @etCoderDysto Whoops! This issue is 2 days overdue. Let's get this updated quick!
Working on a PR melvin.
Thanks for the update, looking forward to it!
How we getting on with a PR, any ETA on putting it into review?
Hi @trjExpensify, The PR is almost ready for review. There is one navigation issue I wanted to fix before requesting a final review. I am hopping to fix the navigation issue in 2 days max. If I couldn't I will ask for help form @mollfpr.
https://github.com/Expensify/App/assets/171434916/429a651b-154e-4e99-84d9-1f243cbc78a1
https://github.com/Expensify/App/assets/171434916/55bafd43-8735-4fa0-9b31-950baa3fee00
https://github.com/Expensify/App/assets/171434916/706dcc7f-0cb9-4c5c-b3c5-f2b5a2d995b3
Expected behaviour for navigation error
https://github.com/Expensify/App/assets/171434916/4a3f7379-3cd4-4c0c-a113-f36f249f89c7
I will change the PR status to ready for review. I can work on navigation issues while the PR is being reviewed. Is this alright to do @trjExpensify, @mollfpr ?
Works for me!
@mollfpr PR is ready for review.
Update: I am having a discussion on how to handle a navigation issue I have faced in slack
Can we get an update here, please? Thanks!
I am waiting for a help from @adamgrzybowski on a navigation issue I am facing. I am sorry for the delay. I couldn't get hold of what is causing the navigation issue by myself.
This issue has not been updated in over 15 days. @trjExpensify, @mollfpr, @techievivek, @etCoderDysto eroding to Monthly issue.
P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!
Sorry for the delay. There is a navigation issue that I am trying to fix. It is a bit complicated. That is why the PR is delayed. There is a solution I am testing out to see if it fixes the navigation issue and if it doesn't cause a regression. And I am also working on secondary solution where I will create modal stack navigator for Two factor auth page. I will comment with the results of my testing.
Sorry for the delay again.
Appreciate the update!
Hey, Making sure that we also fix one additional flow, i was testing something and found the bug:
I am able to access the 2FA page if i go to workspace accounting and then click on xero for unverified accounts. While we are at this issue i hope we also fix any redirects to the 2FA as well, thanks, do ping me if you guys need any help here.
How are we getting on with this PR?
Hi @trjExpensify, I have tried implementing this suggestion to fix this navigation issue . The solution has fixed an issue where the user is not navigating back to 2fa page when clicking browser's back button from contact method details page. But it causes another navigation regression. It seems I am not going anywhere with the navigation issue. Should we assign the issue to someone who is experienced with navigation?
Sorry for the inconvenience that I have caused.
Hey @etCoderDysto what is the regression?
Hi @adamgrzybowski,
Actual result: User is navigated to profile settings Expected Result: User should be navigated to workspace details page
https://github.com/user-attachments/assets/a1f96ea0-0271-405b-ba5c-546dbf9a47bd
Sooooo weird π I can take a look. Thanks for finding this!
When we navigate to contact method details section for Connect bank account
, only the RHP navigates to the the contact method details page, and the background remains in place. Is it possible to mimic this behaviour in 2fa page? That might solve the issue I think.
Steps:
https://github.com/user-attachments/assets/823a91f4-a715-42f0-91e5-36153c8ef5ca
Sooooo weird π I can take a look. Thanks for finding this!
Yeah very weird π. @adamgrzybowski Can you please take a look this comment ^ ππ»ββοΈ?
Hmm, this may be a solution. I would love to understand what is happening nevertheless. Q:
backTo
param for this screen. That should help.For now, I can see two:
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.4.82-1 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @ikevin127 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1718145760414179
Action Performed:
Precondition: Login with a new email and don't verify account. Note: This means that you login with a new email and you're not asked to input the magic code, unless you do this manually by navigating to Settings > Profile > Contact method > click on your email to verify it.
Expected Result:
Actual Result:
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
https://github.com/Expensify/App/assets/38435837/e020732b-9eab-4736-b138-25d1dcf2178f
https://github.com/Expensify/App/assets/38435837/e8071b83-3d23-482c-966e-74cbd6352c2b
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @trjExpensify