[HOLD for payment 2023-12-28] [$500] Workspace - Currencies do not load after transition from OldDot when other user logged in ND

[lanitochka17]

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.3-0
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: Applause - Internal Team
Slack conversation:

Action Performed:

1. Navigate to OldDot Expensify and log in with an existing account which has a workspace
2. Open new tab > navigate to NewDot Expensify and log in with different account
3. In the OldDot tab, navigate to the workspace and click on it
4. Click on Settings > Default currency

Expected Result:

A list of all available currencies should be shown

Actual Result:

The list is empty, even the default currency is not shown

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6288641_1700751338403.2023-11-23_15-20-07.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019bf54ae70b3509e1
• Upwork Job ID: 1727745677732249600
• Last Price Increase: 2023-11-23
• Automatic offers:
• cubuspl42 | Reviewer | 27885178
• AmjedNazzal | Contributor | 27885180

Issue Owner

Current Issue Owner: @cubuspl42

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[zukilover]

Proposal

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

Currency list is not loaded when transitioning from OldDot to NewDot with different account.

What is the root cause of that problem?

Currency list is loaded by openApp() method. In AuthScreens, when transitioning, this method will be called if app is logging as a new user (different user):

App/src/libs/Navigation/AppNavigator/AuthScreens.js

Lines 160 to 161 in a2fc32b

	const isLoggingInAsNewUser = SessionUtils.isLoggingInAsNewUser(currentUrl, session.email);
	const shouldGetAllData = isUsingMemoryOnlyKeys || SessionUtils.didUserLogInDuringSession() || isLoggingInAsNewUser;

The problem is, upon transitioning to a new user, app is logging out:

App/src/libs/Navigation/AppNavigator/AuthScreens.js

Lines 164 to 167 in a2fc32b

	if (isLoggingInAsNewUser && isTransitioning) {
	Session.signOutAndRedirectToSignIn();
	return;
	}

then redirected to LogInWithShortLivedAuthTokenPage to initiate signInWithShortLivedAuthToken:

App/src/pages/LogInWithShortLivedAuthTokenPage.js

Lines 60 to 63 in a2fc32b

	if (shortLivedAuthToken && !props.account.isLoading) {
	Session.signInWithShortLivedAuthToken(email, shortLivedAuthToken);
	return;
	}

In this state, user has logged in with the email from the transition, i.e. email from the URL params is the same as the session email, thus the app is treating it as a login from the same email.

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

There needs to be a way of storing the previous session's email so that it can be compared accurately with the email from the new session.

1. Add new property in the Session store:

     /** The user's email of the previous session */
    previousSessionEmail?: string;

2. Create new utility to store the previous session email:

function setPreviousSessionEmail(email: string) {
    Onyx.merge(ONYXKEYS.SESSION, {previousSessionEmail: email});
}

3. In LogInWithShortLivedAuthTokenPage, we store the email before logging in with the new one:

       if (shortLivedAuthToken && !props.account.isLoading) {
            Session.setPreviousSessionEmail(props.session.email)
            Session.signInWithShortLivedAuthToken(email, shortLivedAuthToken);
            return;
        }

4. Compare the emails to mark the login session as a new login in AuthScreen:

const isTransitioningFromUser = SessionUtils.isLoggingInAsNewUser(currentUrl, session.previousSessionEmail);
const shouldGetAllData = isUsingMemoryOnlyKeys || SessionUtils.didUserLogInDuringSession() || isLoggingInAsNewUser || isTransitioningFromUser;

What alternative solutions did you explore? (Optional)

N/A

[conorpendergrast]

Reproduced. Seems like it could be relatively rare in the real world, but we have plenty of customers with multiple user accounts!

[AmjedNazzal]

Proposal

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

Workspace - Currencies do not load after transition from OldDot when other user logged in ND

What is the root cause of that problem?

The cause of this issue starts in this code:

App/src/libs/SessionUtils.ts

Lines 32 to 45 in a2fc32b

	Onyx.connect({
	key: ONYXKEYS.SESSION,
	callback: (session) => {
	if (loggedInDuringSession) {
	return;
	}
	if (session?.authToken) {
	loggedInDuringSession = false;
	} else {
	loggedInDuringSession = true;
	}
	},
	});

So in this code we are returning false if session?.authToken, and since we are deeplinking to the workspace settings from OD we would be using Session.signInWithShortLivedAuthToken which means session?.authToken will be indeed populated resulting in AuthScreens to call App.reconnectApp instead of App.openApp resulting in the onyx store not getting populated properly which is the reason that as you will see in the screenshot below, the LHN would be not populated either:

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

We can do the following:

1. We would add a new entry to Session.ts to watch for if the login was with short lived token

signedInWithShortLivedAuthToken?: boolean;

2. We would then add a temporary onyx merge in signInWithShortLivedAuthToken within the optimisticData but remove it in successData to allow App.openApp to be called at least once when using short lived token

function signInWithShortLivedAuthToken(email: string, authToken: string) {
  const optimisticData: OnyxUpdate[] = [
      .....
      {
          onyxMethod: Onyx.METHOD.MERGE,
          key: ONYXKEYS.SESSION,
          value: {
              signedInWithShortLivedAuthToken: true,
          },
      },
  ];
  const successData: OnyxUpdate[] = [
      ....
      {
          onyxMethod: Onyx.METHOD.MERGE,
          key: ONYXKEYS.SESSION,
          value: {
              signedInWithShortLivedAuthToken: undefined,
          },
      },
];

3. Finally in SessionUtils.isLoggingInAsNewUser we would check this new session value and if it's true we return true for loggedInDuringSession

Onyx.connect({
  key: ONYXKEYS.SESSION,
  callback: (session) => {
    ....
    if (session?.signedInWithShortLivedAuthToken) {
      loggedInDuringSession = true;
    }

Result

Screen.Recording.2023-11-24.at.12.41.21.PM.mov

[melvin-bot]

@cubuspl42, @conorpendergrast Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@cubuspl42, @conorpendergrast Huh... This is 4 days overdue. Who can take care of this?

[conorpendergrast]

@cubuspl42 there are two proposals for your review here!

[cubuspl42]

C+ reviewed 🎀 👀 🎀

While both proposals sound fine, I prefer the solution by @AmjedNazzal. During the PR phase, I would suggest exploring whether the new introduced property (signedInWithShortLivedAuthToken) could possibly be merged with authToken as a union.

[melvin-bot]

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

[melvin-bot]

📣 @cubuspl42 🎉 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

[melvin-bot]

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[AmjedNazzal]

Thank you! offer accepted and PR will be up within a day.

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

• Fix - Workspace - Currencies do not load after transition from OldDot when other user logged in ND #31789 #32245

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

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:

• @cubuspl42 requires payment automatic offer (Reviewer)
• @AmjedNazzal requires payment automatic offer (Contributor)

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

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

[melvin-bot]

@francoisl, @cubuspl42, @conorpendergrast, @AmjedNazzal Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@francoisl, @cubuspl42, @conorpendergrast, @AmjedNazzal Eep! 4 days overdue now. Issues have feelings too...

[francoisl]

@conorpendergrast this is ready for payment.

[melvin-bot]

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

[thienlnam]

Adding another BZ member that can issue payment while Connor is OOO - cc @sophiepintoraetz Looks like this is ready for payment

[sophiepintoraetz]

We did say that payments might be a little slower with the holidays but in any case, I have paid @AmjedNazzal - just waiting on @cubuspl42 to complete the checklist.

[cubuspl42]

• The PR that introduced the bug has been identified. Link to the PR:
  • N/A (This is an interaction between OldDot and NewDot, both sides would need to be analyzed to point to specific PRs with certainty, while the OldDot code is not available)
• 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:
  • N/A
• 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:
  • No need for additional discussion
• Determine if we should create a regression test for this bug.
  • Up to the QA team
• 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.
  • Sign in to OldDot using account A
  • Sign in to NewDot using a different account B
    • Use the same environment (staging or prod) you used for OldDot
  • From OldDot, go to Settings > Workspaces and click on a workspace
  • Once the NewDot app loads, go to (Your workspace) > Settings > Default currency and verify that the currency list is populated

[melvin-bot]

@francoisl, @cubuspl42, @conorpendergrast, @sophiepintoraetz, @AmjedNazzal Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[conorpendergrast]

This was caught by Applause during testing, so I think it's already captured under existing regression testing. I think it's likely we don't need a new test as a result, but if anyone disagrees, comment and let me know!

Apart from that, we're all done here. Thanks!