[HOLD for payment 2023-09-21] [$1000] Log in -Didn't receive magic code button is clickable while loading

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Go to the Front page of expensify
2. Insert your email address and insert the correct magic code after receiving via email
3. Click the 'Didn't receive the magic code' while it's loading. observe the result it's clickable and made expensify conceirge to resend

Expected Result:

"Didn't receive the magic code" shouldn't be functional while the password is being processed

Actual Result:

'Didn't receive the magic code' is clickable and made expensify conceirge to resend the code even though you are already logged in

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: v1.3.57-3

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

Notes/Photos/Videos: Any additional supporting documentation

BR.63.mp4

Recording.1329.mp4

Expensify/Expensify Issue URL:

Issue reported by: @Yokabdk

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691794873476939

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0167751214ac5ebd0b
• Upwork Job ID: 1696234705084198912
• Last Price Increase: 2023-09-04
• Automatic offers:
• Ollyws | Reviewer | 26506110
• c3024 | Contributor | 26506112

[c3024]

Proposal

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

"Didn't receive code" is still pressable while account is loading

What is the root cause of that problem?

We are disabling this link only when the network is offline and we are not disabling in when the account is loading here.

App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js

Line 282 in 08b75f3

disabled={props.network.isOffline}

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

We should change the disabled condition to

disabled={props.network.isOffline || props.account.isLoading}

We need to adjust the style here

App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js

Line 288 in 08b75f3

<Text style={[StyleUtils.getDisabledLinkStyles(props.network.isOffline)]}>

as well to

StyleUtils.getDisabledLinkStyles(props.network.isOffline || props.account.isLoading)

What alternative solutions did you explore? (Optional)

We can make this change in the BaseValidateCodeForm used in Contacts as well used for secondary email validation

Result

disableDidNotReceiveMagicCodePressableWhileAccountIsLoading.mov

[AmjedNazzal]

Proposal

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

Log in -Didn't receive magic code button is clickable while loading

What is the root cause of that problem?

The reason the button remains active and clickable is that we are disabling it only under the condition of network.isOffline without accounting for loading.

App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js

Lines 278 to 282 in 56f0b0f

	<PressableWithFeedback
	style={[styles.mt2]}
	onPress={resendValidateCode}
	underlayColor={themeColors.componentBG}
	disabled={props.network.isOffline}

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

We should add props.account.isLoading to both the disabled variable we are passing and the style of the text to make sure the button is disabled while loading.

<PressableWithFeedback
    ..
    ...
    disabled={props.network.isOffline || props.account.isLoading}
    ..
    ...
    <Text style={[StyleUtils.getDisabledLinkStyles(props.network.isOffline || props.account.isLoading)]}></Text>

Result

Screen.Recording.2023-08-15.at.2.14.11.PM.mov

[melvin-bot]

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

[bernhardoj]

Proposal

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

We can still click "Didn't receive magic code" after submitting the magic code input. This happens on secondary login validation too.

What is the root cause of that problem?

We don't disable the "Didn't receive magic code" text button while the magic code submit request is loading.

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

Disable the "Didn't receive magic code" text button while loading with the condition from below LOC.

App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js

Lines 302 to 304 in e572007

	isLoading={
	props.account.isLoading && props.account.loadingForm === (props.account.requiresTwoFactorAuth ? CONST.FORMS.VALIDATE_TFA_CODE_FORM : CONST.FORMS.VALIDATE_CODE_FORM)
	}

I think we can store it in a variable to make it clean

const isLoading = props.account.isLoading && props.account.loadingForm === (props.account.requiresTwoFactorAuth ? CONST.FORMS.VALIDATE_TFA_CODE_FORM : CONST.FORMS.VALIDATE_CODE_FORM);
...
disabled={props.network.isOffline || isLoading}

<Text style={[StyleUtils.getDisabledLinkStyles(props.network.isOffline || isLoading)]}>

loadingForm is important to prevent a request from another sign-in form from affecting the current form.

For secondary login validation, we don't need the loadingForm.

[melvin-bot]

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

[kevinksullivan]

Moving forward

[melvin-bot]

Current assignee @kevinksullivan is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[shu8]

Proposal

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

When signing in via magic link, if the user submits their magic code after the 30 second resend window is finished, the "Didn't receive a magic code?" link is still clickable and active -- but it shouldn't be whilst the magic code is being verified (i.e., the form is loading).

What is the root cause of that problem?

The following line only disables the "Didn't receive a magic code?" button when the network is offline (props.network.isOffline):

App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js

Line 282 in 9b34d34

disabled={props.network.isOffline}

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

We should also disable the button in the case that the form is loading. This would mean:

• changing the disabled condition, updating the line to disabled={props.network.isOffline || props.account.isLoading}
• updating the text style on the following line (L288) to this same new condition:

App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js

Line 288 in 9b34d34

<Text style={[StyleUtils.getDisabledLinkStyles(props.network.isOffline)]}>

I don't think we need to check the value of props.account.loadingForm against the possibility of the form being CONST.FORMS.VALIDATE_TFA_CODE_FORM, because the magic code input is only shown if props.account.requiresTwoFactorAuth is falsey -- the negative part of this ternary:

App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js

Line 239 in 9b34d34

{props.account.requiresTwoFactorAuth ? (

What alternative solutions did you explore? (Optional)

Note: Another bug in the same component

I also found another bug in the same section of code whilst looking into this issue. The accessibilityLabel (L286) is incorrectly never showing the "Request a new code" error text like the main label (L289) does:

App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js

Lines 286 to 290 in 9b34d34

	accessibilityLabel={props.translate('validateCodeForm.magicCodeNotReceived')}
	>
	<Text style={[StyleUtils.getDisabledLinkStyles(props.network.isOffline)]}>
	{hasError ? props.translate('validateCodeForm.requestNewCodeAfterErrorOccurred') : props.translate('validateCodeForm.magicCodeNotReceived')}
	</Text>

This should be changed to accessibilityLabel={hasError ? props.translate('validateCodeForm.requestNewCodeAfterErrorOccurred') : props.translate('validateCodeForm.magicCodeNotReceived')} (the same as the main text on L289).

This bug seems to have been introduced in a95b3e2 when the "Request a new code" error text was first added.

[melvin-bot]

📣 @shu8! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[kevinksullivan]

Hi @Ollyws let me know when you'll be able to review the proposal above!

[Ollyws]

I think it's best to use the conditions that we currently use to set the button isLoading to also disable the magic code link rather than simply using props.account.isLoading for the reasons @bernhardoj mentioned.

Therefore let's go with @bernhardoj's proposal.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[c3024]

@Ollyws Sign in button appears in both the magic code and 2FA screen so it is shown as loading for both the magic code and 2FA screen pages. But the 'Didn't receive magic code' appears only on the first page and not on the 2FA page even when 2FA is enabled.

So I think simply copying the button condition here has not much meaning.

Could @bernhardoj post a video showing the case where this addition of condition is beneficial?

It appears like an unnecessary condition serving no purpose in any case but added 'just to be safe' even though we don't know what we are being safe from.

I am asking this because I saw that condition in button and did my handful experiments and felt like it is not worth it to add even as an alternative solution.

I just would like to know the specific condition I missed.

If we cannot identify the case where this addition of condition props.account.loadingForm === (props.account.requiresTwoFactorAuth ? CONST.FORMS.VALIDATE_TFA_CODE_FORM : CONST.FORMS.VALIDATE_CODE_FORM) for disabling the 'Didn't receive magic code' is useful, please consider my proposal for assigning because it was the first to fix the bug correctly and without any superfluous code. @youssef-lr

[Ollyws]

@c3024 Thanks for the comment. In retrospect maybe my decision was a little unfair as I should be picking the first proposal to propose the general solution and any details can be worked out in the PR, unless they're absolutely integral to the solution or save us from causing a regression.

As far as I'm aware that condition was added in #19393 to remove a delay between when the optimistic data is set (by API calls such as beginSignin) and the success/failure data returned, which was causing the spinner to extend slightly onto the next page for a fraction of a second. It does this by setting the form type in the optimistic data and making sure it doesn't match the form type for our page (VALIDATE_CODE_FORM) in this case.

Now if we hadn't added the timer for the magic code link I could see this being useful otherwise it may be disabled for a fraction of a second, but on our current implementation it doesn't really provide any tangible benefit.

Any thoughts about why this condition is important @bernhardoj ?

[bernhardoj]

@Ollyws My initial concern was to prevent the other browser tab from affecting the isLoading property, but I think that shouldn't be a problem if we sync the sign-in page on all tabs in the first place. Feel free to assign @c3024.

[Ollyws]

@bernhardoj
Thanks, apologies for any confusion.
Given my comment here, let's go with @c3024's proposal.

🎀👀🎀 C+ reviewed

[c3024]

PR ready for review! @Ollyws

[kevinksullivan]

Hi @Yokabdk please apply for reporting bonus when you get a chance

https://www.upwork.com/jobs/~0167751214ac5ebd0b

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @c3024 got assigned: 2023-09-05 12:21:40 Z
• when the PR got merged: 2023-09-13 06:23:02 UTC
• days elapsed: 5

On to the next one 🚀

[SofoniasN]

Hey @kevinksullivan i reported the same issue a while back but it was closed out by the team as not important. I believe i should be paid for the reporting bonus if the issue is fixed now. Below is the link for the initial github issue

#21463 (comment)

[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.69-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• disable "Didn't receive magic code" while loading #26783

If no regressions arise, payment will be issued on 2023-09-21. 🎊

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

For reference, here are some details about the assignees on this issue:

• @Ollyws requires payment offer (Reviewer)
• @c3024 requires payment offer (Contributor)
• @Yokabdk requires payment

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:

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

[SofoniasN]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @c3024 got assigned: 2023-09-05 12:21:40 Z
• when the PR got merged: 2023-09-13 06:23:02 UTC
• days elapsed: 5

On to the next one 🚀

@kevinksullivan a slight bump

[kevinksullivan]

Hi @SofoniasN this happens from time to time, and while we try to be consistent in our approach to determining whether or not something is a bug, it's not always perfect. I won't be able to pay out that report.

[youssef-lr]

@Ollyws can we complete the checklist please?

[Ollyws]

There isn't a PR that introduced this issue it has been present since the 'didn't recieve magic code' button was implemented so I don't think the checklist is applicable here, let me know if you think otherwise.

[kevinksullivan]

paid out

[Yokabdk]

@kevinksullivan Does this mean there is no payment for reporting?

[kevinksullivan]

Sorry @Yokabdk I missed that. Offer sent.

[kevinksullivan]

Let me know when you accept

[Yokabdk]

@kevinksullivan offer accepted

[kevinksullivan]

thanks, all set.