search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.34k stars 2.77k forks source link

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

Closed lanitochka17 closed 8 months ago

lanitochka17 commented 10 months 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: 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?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/25646ed7-6d70-4d78-a9f3-420f5ace77e9

Bug6288641_1700751338388!chrome_IGeqITBXT9

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 OwnerCurrent Issue Owner: @cubuspl42
melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 10 months ago

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

zukilover commented 10 months ago

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): https://github.com/Expensify/App/blob/a2fc32bbfb8d5917e83134568187529d488812e2/src/libs/Navigation/AppNavigator/AuthScreens.js#L160-L161

The problem is, upon transitioning to a new user, app is logging out: https://github.com/Expensify/App/blob/a2fc32bbfb8d5917e83134568187529d488812e2/src/libs/Navigation/AppNavigator/AuthScreens.js#L164-L167

then redirected to LogInWithShortLivedAuthTokenPage to initiate signInWithShortLivedAuthToken: https://github.com/Expensify/App/blob/a2fc32bbfb8d5917e83134568187529d488812e2/src/pages/LogInWithShortLivedAuthTokenPage.js#L60-L63

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 commented 10 months ago

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

AmjedNazzal commented 10 months ago

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:

https://github.com/Expensify/App/blob/a2fc32bbfb8d5917e83134568187529d488812e2/src/libs/SessionUtils.ts#L32-L45

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:

nolhn

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

https://github.com/Expensify/App/assets/74202751/17937b3a-a7db-4f8b-937c-4c9960642c08

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

conorpendergrast commented 9 months ago

@cubuspl42 there are two proposals for your review here!

cubuspl42 commented 9 months ago

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[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

📣 @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[bot] commented 9 months ago

📣 @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 commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

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

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.

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

melvin-bot[bot] commented 9 months ago

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:

melvin-bot[bot] commented 8 months ago

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

melvin-bot[bot] commented 8 months ago

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

francoisl commented 8 months ago

@conorpendergrast this is ready for payment.

melvin-bot[bot] commented 8 months ago

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

thienlnam commented 8 months ago

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

sophiepintoraetz commented 8 months ago

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 commented 8 months ago
melvin-bot[bot] commented 8 months ago

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

conorpendergrast commented 8 months ago

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!