[HOLD for payment 2023-06-05] [$1000] Desktop - Sign in - User can not validate an account

[kbecciv]

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

---

Issue found when executing PR #17697

Action Performed:

1. Access the Expensify Desktop app
2. Input an email that does not exist
3. Access the email account and tap on the link (Redirect it to the Desktop app)
4. Input a New password

Expected Result:

User expects the password to be saved and the new account to be open

Actual Result:

The user keeps receiving an error message saying that the link has expired, making it impossible to validate an account via Desktop app.

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: 1.3.2.3

Reproducible in staging?: Yes

Reproducible in production?: No

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

Bug6025598_Cant_validate_on_desktop.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016ce9697b87884431
• Upwork Job ID: 1650976104948047872
• Last Price Increase: 2023-05-11

[MelvinBot]

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

[MelvinBot]

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

[mountiny]

@mvtglobally @kbecciv In the video, you are clicking the link which goes to production, is that not part of the problem? I am not sure

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[tjferriss]

@mvtglobally @kbecciv In the video, you are clicking the link which goes to production, is that not part of the problem? I am not sure

Following up on @mountiny question.

[mvtglobally]

@mountiny @tjferriss the account creation is done in STG app. Should the link be sent as STG as well? Maybe that's the root cause.

I will ask team to Not click the link, but paste and modify to staging to check

[mvtglobally]

Looks to be still repro after URLs switch as well

IMG_8689.MOV

[ntdiary]

Could you give more details/reason on this one?

App/web/splash/splash.js

Lines 27 to 38 in eb9df25

	const intervalId = setInterval(() => {
	passedMiliseconds += 250;
	isRootMounted = root.children.length > 0;
	if (passedMiliseconds >= minMilisecondsToWait && isRootMounted && areFontsReady) {
	clearInterval(intervalId);
	splash.style.opacity = 0;
	setTimeout(() => {
	splash.parentNode.removeChild(splash);
	}, 250);
	}
	}, 250);

In older versions, after we render the redirection screen, isRootMounted is true, and the splash screen can be automatically removed by the setInterval.
However, this logic has been deleted in the new version. The splash screen can only deleted by NavigationRoot. If we render the redirection screen, NavigationRoot does not exist. (There is no redirection screen in PR #17477, so it can work well).

App/src/Expensify.js

Line 202 in 619a529

onReady={setNavigationReady}

I don't think it's our concern. We only support MacOSWeb atm

This issue is just one example of the magic link flow. I'm thinking if we can solve them from the root cause, more scenarios will benefit from it. Is it worth a try? For example, the following: intuitively, users can choose to login on chrome, but it still shows expired.

chrome.mp4

I think we have discussed and accepted this behavior for validation magic link. Btw, it's also nature flow of open a link in other App too.

I fully understand. Just that slack also does not show the redirection screen for the magic code flow. And I investigated these things not to propose a solution or oppose the approved solution.
Just when merging conflicts, I saw that the examples of this PR fixes were limited. I was thinking that if we can fix the problem from the root cause, it can make our magic flow smoother and more robust without some weird errors, for example, on mobile, the validation page will first load in preview mode or other third-party clients, then we can choose to open in the Expensify or open in safari, however, the page will show expired because the magic code has been consumed.

iphone.mp4

Btw, In my humble opinion, displaying this page is fine, just that the code below will incorrectly consume the magic code, which needs to be fixed. For example, if all users can use magic code, can we remove this condition? if not, when the component is mounted, can we first get the betas data through a command?

App/src/pages/ValidateLoginPage/index.website.js

Lines 66 to 69 in 16776ec

	if (!Permissions.canUsePasswordlessLogins(this.props.betas)) {
	User.validateLogin(this.getAccountID(), this.getValidateCode());
	return;
	}

If we eventually want to keep the redirection screen to prevent the ValidateLoginPage from incorrectly consuming the magic code, please feel free to tell me. I will resolve the conflicts in my clean-up PR in an appropriate way. : )

cc @rushatgabhane @bondydaa

[ntdiary]

Hi, new progress, I just found PR #19819, it changes the condition and prevents the magic code from being incorrectly consumed, which will also fundamentally fix this issue. : )

cc @hoangzinh , @rushatgabhane, @bondydaa

[hoangzinh]

@ntdiary this changes only work for fresh session. If user used to login or visit the Expensify Web before, I think it won't fix this issue.

[ntdiary]

@hoangzinh, Were you referring to logging in on the web first, then opening the link from email, and then opening the desktop app? If so, this case can work well in old versions.

case2.mp4

[hoangzinh]

@ntdiary I mean "used to visited", not really meant already signed. If you're already signed in, ofc it won't validate with the code.

[ntdiary]

@hoangzinh, eh, sorry, could you please explain the essential difference between "used to visited" and "fresh session"? Or what are the specific steps of the former? In my humble opinion, these two issues are essentially caused by the missing onyx betas data, which led to the incorrect consumption of magic code.

App/src/pages/ValidateLoginPage/index.website.js

Lines 66 to 69 in 16776ec

	if (!Permissions.canUsePasswordlessLogins(this.props.betas)) {
	User.validateLogin(this.getAccountID(), this.getValidateCode());
	return;
	}

[hoangzinh]

I think you're correct @ntdiary . It seems this PR #19819 can fix this issue too. I think we can wait above PR merged then re-test again. Then we can decide if we would like to go with redirect screen or not.

[tjferriss]

Looks like we're still waiting on this PR to see if it fixes the issue #19819

[bondydaa]

sorry i haven't really been following 100% here but didn't we merge the deploy the code here? did it get reverted or is it breaking something?

If not let's get payment done then and we can spin up new issues if needed to address other things.

[ntdiary]

sorry i haven't really been following 100% here but didn't we merge the deploy the code here? did it get reverted or is it breaking something?

@bondydaa, because the redirection component can't remove the splash screen, if we install the desktop app, we will not be able to log in by clicking the magic link from the mailbox in the browser (get stuck on the splash screen). I personally think this is a regression, would appreciate if you could share your thoughts on this. : )

test.mp4

In addition, because PR #19819 also fixed this issue, so I plan to remove the redirection component in my PR #17452, this can also allow us to get rid of the setTimeout.

App/src/components/DeeplinkWrapper/index.website.js

Lines 79 to 85 in 9d492f1

	setTimeout(() => {
	if (!this.focused) {
	this.setState({appInstallationCheckStatus: CONST.DESKTOP_DEEPLINK_APP_STATE.INSTALLED});
	} else {
	this.setState({appInstallationCheckStatus: CONST.DESKTOP_DEEPLINK_APP_STATE.NOT_INSTALLED});
	}
	}, 500);

[hoangzinh]

I personally think this is a regression, would appreciate if you could share your thoughts on this. : )

Disagree on it. Because I started my PR before this PR is merged. I mean 2 PR is working independently, it's hard to detect they would break each other.

In addition, because PR #19819 also fixed this issue, so I plan to remove the redirection component in my PR #17452, this can also allow us to get rid of the setTimeout.

Thanks @ntdiary . If we all agree that we don't need to use redirection page anymore, then I think it's okay to remove it.

[hoangzinh]

@bondydaa because I'm the one who assigned to this issue so my suggestion might be not totally correct.

The PR #19819 had a better idea to also fix this issue. We might remove what we done here (to reduce double redirection page) but we already put effort to implement redirection page here so I hope I can get paid.

[ntdiary]

The PR #19819 had a better idea to also fix this issue. We might remove what we done here (to reduce double redirection page) but we already put effort to implement redirection page here so I hope I can get paid.

Personally, I think it's good to get the payment done and then close this issue. And I would appreciate if @hoangzinh can also help confirm that this issue does go away in my PR #17452 (without redirection page) : )

cc @tjferriss @bondydaa

[tjferriss]

Great, I think we're all in agreement. We're going to process the payment and close this issue. @rushatgabhane @hoangzinh I've sent both of you an offer in Upworks.

[hoangzinh]

Accepted offer. Thanks @tjferriss

[rushatgabhane]

@tjferriss Applied using account "Satish Gabhane", thanks! - https://www.upwork.com/freelancers/~01212e8255b02ae924

context - https://expensify.slack.com/archives/C02NK2DQWUX/p1683614947522329

[tjferriss]

@rushatgabhane @hoangzinh I've sent you both offers in Upworks.

[hoangzinh]

Accepted offer. Thanks @tjferriss

[tjferriss]

The payments have been made in Upworks.

[hoangzinh]

@tjferriss it seems you haven't end my contract/offer in Upwork yet. Could you help to check it? Thanks

[tjferriss]

@hoangzinh it now looks like the contract has ended.

[hoangzinh]

it's weird. I just send you a message in Upwork so you can check the contract easily.

[tjferriss]

I think I found it now and ended the contract.

[hoangzinh]

Yay, thanks @tjferriss