[HOLD for payment 2023-06-12] [Tracking Issue]Bug fixes: 2FA

[MonilBhavsar]

Here is the list of minor Tracking 2FA bugs to make a quick follow up PR and fix all of them

1. Missing Download translation in Spanish - https://expensify.slack.com/archives/C049HHMV9SM/p1684406965776349
2. Two-factor name is inconsistent https://expensify.slack.com/archives/C049HHMV9SM/p1684406403576119
3. Offline Indicator displayed twice https://expensify.slack.com/archives/C049HHMV9SM/p1684408727072029
4. Here is your magic code page appears https://expensify.slack.com/archives/C049HHMV9SM/p1684405641601719
5. Error message persists https://expensify.slack.com/archives/C049HHMV9SM/p1684404156826129
6. View disrupted on small screen devices https://expensify.slack.com/archives/C049HHMV9SM/p1684392775324939

[melvin-bot]

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[MonilBhavsar]

@stephanieelliott this feature (2FA) went out today and we instantly got bug reports from our wonderful open source community. Thankfully all are minor bugs. I have compiled a list have asked for help in #callstack. We can squash them soon 🚫 🐛
https://expensify.slack.com/archives/C03UK30EA1Z/p1684415073652249

[stephanieelliott]

Awesome, thanks for the context @MonilBhavsar!

[BeeMargarida]

Proposal

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

Several smaller issues regarding 2FA.

What is the root cause of that problem?

1. Missing Download translation in Spanish
2. Two-factor name is inconsistent
   1. Translations were using both two-factor and two factor
3. Offline Indicator displayed twice
   1. 2FA pages use ScreenWrapper, that already includes an offline indicator by default
4. Here is your magic code page appears - when opening the magic link, it does not update correctly after setting the 2FA code
   1. This is because after signing in, the AuthState returns to NOT_STARTED instead of JUST_SIGNED_IN
5. Error message persists
6. View disrupted on small screen devices

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

1. Add translation for downloadCodes, since the value being used is hardcoded
2. Change all translation that are 'two factor' to 'two-factor'
3. We need to do 2 things:
   1. Add a prop to ScreenWrapper called shouldShowOfflineIndicator that has default true
   2. Use that prop with value false in all pages that use FullPageOfflineBlockingView
   3. Modify all usages of ScreenWrapper with FullPageOfflineBlockingView to include this prop, since the problem will appear there as well.
4. Change ValidateLoginPage to show the AbracadabraModal not when the AuthState is JUST_SIGNED_IN but NOT_STARTED and isSignedIn === true.
5. Clear Account messages on mount in VerifyPage
6. In the CodesPage, use withWindowDimensions and apply paddingHorizontal as 40 instead of 60 when isSmallScreenWidth is true.

What alternative solutions did you explore? (Optional)

--

[MonilBhavsar]

Thanks for the proposal 🙇

1. We already have translation in common.download. Looks like we just need to translate where we display it.
2. Looks good 👍
3. Oh, since the problem is widespread, I think this needs some discussion and we can handle this in another PR, as it is not solely related to 2FA.
4. Do we only need to remove !isTfaRequired and keep JUST_SIGNED_IN? Also let's please rename that to is2FARequired 😅
5. Sounds good to me 👍 We can test it out on different devices and see if we still need to adjust the padding.

[BeeMargarida]

Do we only need to remove !isTfaRequired and keep JUST_SIGNED_IN?

For the AbacadabraModal (after the sign in is complete), we need to remove the isTfaRequired and switch JUST_SIGNED_IN with NOT_STARTED (since this is reset after signing in), basically switch to this.getAutoAuthState() === CONST.AUTO_AUTH_STATE.NOT_STARTED && isSignedIn && <AbracadabraModal />

Also let's please rename that to is2FARequired 😅

On it!

[MonilBhavsar]

(since this is reset after signing in)

I see, thanks for clarifying. That makes sense! 🙇

[stephanieelliott]

PRs are under review!

[stephanieelliott]

Still working through reviews on the PR.

[stephanieelliott]

Reviews completed, just awaiting merge

[MonilBhavsar]

PR is merged!
We need to pay all reporters and @mananjadhav for PR review next week

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.23-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: 2FA bug fixes #19260

If no regressions arise, payment will be issued on 2023-06-12. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@MonilBhavsar] The PR that introduced the bug has been identified. Link to the PR:
• [@MonilBhavsar] 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:
• [@MonilBhavsar] 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:
• [@BeeMargarida] Determine if we should create a regression test for this bug.
• [@BeeMargarida] 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.
• [@stephanieelliott] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[stephanieelliott]

Created in Upwork: https://www.upwork.com/jobs/~016a53e935d8151e4b

Extended offers to:

• C+ for PR review: @mananjadhav
• Missing Download translation in Spanish - Reporting: @aman-atg
• Two-factor name is inconsistent - Reporting: @Natnael-Guchima
• Offline Indicator displayed twice - Reporting: @dhairyasenjaliya
• Here is your magic code page appears Reporting: @Natnael-Guchima
• Error message persists Reporting: @Nathan-Mulugeta
• View disrupted on small screen devices - Reporting: @ayazhussain79

[ayazhussain79]

@stephanieelliott I have identified only this bug : View disrupted on small screen devices

[mananjadhav]

@stephanieelliott Unable to access the Upwork job? Do you mind sending an invite?

[gadhiyamanan]

@stephanieelliott you accidentally sent me job offer instead of @mananjadhav 😅

[aman-atg]

Missing Download translation in Spanish - Reporting: @ayazhussain79

@stephanieelliott This one is actually reported by me.

[MonilBhavsar]

@aman-atg sorry, fixed that

[MonilBhavsar]

Offline Indicator displayed twice - Reporting: @dhairyasenjaliya

@stephanieelliott we didn't fix this issue here as it was out of scope and instead we'll be fixing it here in another issue

[dhairyasenjaliya]

agreed with @MonilBhavsar but since I reported the offline indicator issue first does this(#20004) make this one dupe

[MonilBhavsar]

Yes 👍

[dhairyasenjaliya]

alright @MonilBhavsar @michaelhaxhiu maybe we just adjust the Issue reporter here #20004

[mananjadhav]

@stephanieelliott you accidentally sent me job offer instead of @mananjadhav 😅

@stephanieelliott I cannot access the job link. I think because of this. Can you please help?

[stephanieelliott]

OKok sorry for the confusion, I think I was able to get this sorted:

• C+ for PR review: @gadhiyamanan extended offer to @mananjadhav, this has been paid
• Missing Download translation in Spanish - Reporting: @ayazhussain79 extended offer to @aman-atg
• Two-factor name is inconsistent - Reporting: @Natnael-Guchima this has been paid
• Offline Indicator displayed twice - Reporting: @dhairyasenjaliya not paying on this issue, handling elsewhere
• Here is your magic code page appears Reporting: @Natnael-Guchima this has been paid
• Error message persists Reporting: @Nathan-Mulugeta this has been paid
• View disrupted on small screen devices - Reporting: @ayazhussain79 offer extended

Leaving the contracts open just in case I need to swap anything around again 😅, will close by EOD tomorrow once payment is issued for the remaining 2.

[mananjadhav]

Tough one @stephanieelliott. Thanks for clearing this one out. Appreciate it!

[stephanieelliott]

Hey @aman-atg can you accept the offer in Upwork when you get a chance?

[dhairyasenjaliya]

hey @stephanieelliott can you also please close the my contract since we handling it elsewhere

[melvin-bot]

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[stephanieelliott]

Reapplying the Bug label to get another BZ member on this while I am OOO til July 3. Thanks @bfitzexpensify, only thing to do here is issue payment to @aman-atg. The job was extended to them in Upwork, we are just waiting for them to accept is so we can make the payment and close the contract. All others have been paid!

[aman-atg]

@stephanieelliott Thanks, I've just accepted the offer.

[bfitzexpensify]

Paid out and ended the contract for @aman-atg. Sounds like we're all done here, so closing this one out.