[$250] Android - 2-FA – Use two-factor authentication code and Use recovery codes blinks

lanitochka17 commented 1 month ago

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: 9.0.30-2 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): applausetester+gm103086@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

User have 2-FA established

Open New Dot app
On login page enter your email address and magic code
Verify 2-FA page is opened
Switch between "Use two-factor authentication code" and "Use recovery codes"

Expected Result:

"Use two-factor authentication code" and "Use recovery codes" options should not blink

Actual Result:

Options "Use two-factor authentication code" and "Use recovery codes" blinks

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/021b2145-e566-4a6c-915b-352e07864c0f

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021834200655136685716
Upwork Job ID: 1834200655136685716
Last Price Increase: 2024-09-19
Automatic offers:

brunovjk | Reviewer | 104097398

Issue Owner

Current Issue Owner: @brunovjk

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @kevinksullivan (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.

lanitochka17 commented 1 month ago

@kevinksullivan FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

bernhardoj commented 1 month ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The use recovery code/use 2fa code blinks when pressing it.

What is the root cause of that problem?

The text button for use recovery code/2fa code use the same Pressable. We just conditionally render the text depends on whether we want to use recovery/2fa code. https://github.com/Expensify/App/blob/9affb1384815fa8155fe23c523127c1e72b1120d/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.tsx#L347-L357

So, when we press the text button, the press dim is applied, which reduces the opacity to 0.2 and because it uses the same Pressable, the dim affects the 2nd text too. For example, if the text is initially Use recovery code, when we press it, the text is switched to Use 2FA code and the dim is applied briefly to Use 2FA code too, so it looks like the text is blinking. The 0.2 opacity makes the "blink" obvious.

What changes do you think we should make in order to solve the problem?

We can remove the 0.2 dim value and just let it use the default value (0.8), just like the Go back text button but I don't think it solves the root cause.

So, to solve the root cause, we can set a key to the pressable so a new instance of pressable is created every time we switch between codes.

We can use the boolean value (and convert it to string) as the key or use the text itself or any other value.

key={isUsingRecoveryCode}
key={isUsingRecoveryCode ? translate('recoveryCodeForm.use2fa') : translate('recoveryCodeForm.useRecoveryCode')}

github-actions[bot] commented 1 month ago

@bernhardoj Your proposal will be dismissed because you did not follow the proposal template.

melvin-bot[bot] commented 1 month ago

@kevinksullivan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 1 month ago

@kevinksullivan Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~021834200655136685716

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @brunovjk (External)

kevinksullivan commented 1 month ago

Looping in another BZ as I am going OOO

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @VictoriaExpensify (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.

brunovjk commented 1 month ago

I will review the proposals once I am done with another PR.

brunovjk commented 1 month ago

Reviewing.

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

brunovjk commented 1 month ago

@lanitochka17 Is this issue only reproducible on Android? I couldn’t reproduce it on any platform. @bernhardoj Were you able to reproduce this without any issues? Thanks!

bernhardoj commented 1 month ago

Yes, you can see that when the text switches, the text dims.

https://github.com/user-attachments/assets/17b0f1f3-d6e6-4316-a3d6-6e16cc916e06

melvin-bot[bot] commented 1 month ago

@kevinksullivan @VictoriaExpensify @brunovjk 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!

brunovjk commented 1 month ago

@bernhardoj is this blinking?

https://github.com/user-attachments/assets/74d3e740-4894-4684-8da5-be025c96a2a2

Judging by the archive history, it doesn't seem like a regression to me, it's been like that for a long time. I believe that here we can just remove pressDimmingValue={0.2} and leave as Go back, mentioned in your proposal.

https://github.com/user-attachments/assets/cdf2e88c-6fe3-416c-b762-4692537d1d8b

What do you mean by "I don't think it solves the root cause."? Thank you :D

bernhardoj commented 1 month ago

is this blinking?

not really.

I think it's reproducible when the app is a bit lagging. When you press the text, the dims effect is applied to the switched text, so it looks like its blinking.

Use recovery code Press Switched to Use 2FA code Dims applied to Use 2FA code

So, reducing the dims doesn't solve the issue. We need to prevent the dim effect to be applied to the switched text.

brunovjk commented 1 month ago

Hey @bernhardoj, I tested the changes and noticed only a small improvement with the key addition, maybe because my emulator isn’t lagging now.

Without key:

https://github.com/user-attachments/assets/89d48875-3dfa-4dbf-a0d9-7080d008d90d

With key:

https://github.com/user-attachments/assets/7f4b2116-daa5-4dd1-a4a6-024af4a27caf

Before proceeding, I’ll ask an internal to confirm whether we should remove pressDimmingValue={0.2} here for consistency.

With key and without pressDimmingValue={0.2}:

https://github.com/user-attachments/assets/20e450c1-6a9b-4b8c-8aaf-88b05e3c27af

Once confirmed, we can move forward with your proposal and test thoroughly. Thanks for your work!

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @NikkiWines, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

brunovjk commented 1 month ago

Hey @NikkiWines, could you please check my previous comment and confirm if we should remove pressDimmingValue={0.2} here for consistency? Let me know if anything needs clarification!

Thanks!

NikkiWines commented 1 month ago

Removing the dimming value sounds right to me 👍

brunovjk commented 1 month ago

Removing the dimming value sounds right to me 👍

Great, so we can go with @bernhardoj's proposal :D

NikkiWines commented 1 month ago

Yep! @bernhardoj proposal looks good 👍

melvin-bot[bot] commented 1 month ago

📣 @brunovjk 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

bernhardoj commented 1 month ago

PR is ready

cc: @brunovjk

brunovjk commented 1 month ago

The PR went into production on Sep 30.

cc: @VictoriaExpensify

brunovjk commented 3 weeks ago

[x] [@brunovjk] The PR that introduced the bug has been identified. Link to the PR: I haven't found any PR that could have introduced this bug, as it's an edge case and it seems like it's always been there.
[x] [@brunovjk] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[x] [@brunovjk] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[x] [@brunovjk] Determine if we should create a regression test for this bug. Yes
[x] [@brunovjk] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal:

Prerequisite: 2FA is enabled; Plataform: Android native.

Log out
Enter your login and magic code
Press the Use two-factor authentication code/Use recovery codes multiple times to switch between those two text
Verify the text is not blinking.

Do we agree 👍 or 👎?

cc: @VictoriaExpensify

VictoriaExpensify commented 3 weeks ago

Payment summary: Contributor: @bernhardoj owed $250 via NewDot Contributor+: @brunovjk paid $250 via Upwork

Thanks for your work on this!

bernhardoj commented 3 weeks ago

Requested in ND.

JmillsExpensify commented 2 weeks ago

$250 approved for @bernhardoj

Expensify / App