Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-10-24] [$250] Expensify Card -App returns to card feed selection page after issuing card from member profile #50243

Closed
6 tasks done
IuliiaHerets opened this issue Oct 4, 2024 · 43 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Oct 4, 2024

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: 9.0.44-7
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+pso@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

Precondition:

  • Bank account is already set up in Expensify Card page.
  • Company card feature is enabled.
  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Members.
  3. Click on any member.
  4. Click New Card.
  5. Click Expensify Card.
  6. Go through the setup process.
  7. Click Issue card on the confirmation page.

Expected Result:

App will return to member profile page after issuing card from member profile.

Actual Result:

App returns to card feed selection page after issuing card from member profile.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6624197_1728034246752.20241004_171833.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021843172551777303100
  • Upwork Job ID: 1843172551777303100
  • Last Price Increase: 2024-10-07
  • Automatic offers:
    • paultsimura | Reviewer | 104321384
    • abzokhattab | Contributor | 104321385
Issue OwnerCurrent Issue Owner: @Christinadobrzyn
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 4, 2024
Copy link

melvin-bot bot commented Oct 4, 2024

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@IuliiaHerets
Copy link
Author

We think that this bug might be related to #wave-collect - Release 2

@IuliiaHerets
Copy link
Author

@Christinadobrzyn FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@abzokhattab
Copy link
Contributor

abzokhattab commented Oct 4, 2024

Edited by proposal-police: This proposal was edited at 2024-10-04 14:45:09 UTC.

Proposal

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

Expensify Card -App returns to card feed selection page after issuing card from member profile

What is the root cause of that problem?

The backTo route is incorrectly set to activeRoute, which is the feed selector page, causing it to loop continuously

Navigation.navigate(ROUTES.WORKSPACE_EXPENSIFY_CARD_ISSUE_NEW.getRoute(policyID, activeRoute));

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

we should change the back to the workspace member detail page

Navigation.navigate(ROUTES.WORKSPACE_EXPENSIFY_CARD_ISSUE_NEW.getRoute(policyID, ROUTES.WORKSPACE_MEMBER_DETAILS.getRoute(policyID, accountID)));

What alternative solutions did you explore? (Optional)

Another solution would be to add backTo to this route, and then use it here. However, I don't think we should do that, since this route is only navigated to once in the code, so we can confidently hardcode the backTo

@cretadn22
Copy link
Contributor

Proposal

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

App returns to card feed selection page after issuing card from member profile

What is the root cause of that problem?

We always return the backTo parameter after a new card is created

Navigation.navigate(backTo ?? ROUTES.WORKSPACE_EXPENSIFY_CARD.getRoute(policyID ?? '-1'));

In this scenario, the navigation stack will be: IssueNewCardPage > WorkspaceMemberNewCardPage > WorkspaceMemberDetailsPage. Therefore, the backTo parameter for IssueNewCardPage will be WorkspaceMemberNewCardPage, then the app to navigate back to WorkspaceMemberNewCardPage

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

We shouldn't navigate to backTo when the user submits a new card; the backTo parameter should only be used when users click the back button

We should check if users create a card in WorkspaceMemberDetailsPage, and if so, navigate them back to that page after creating the new card by checking the backTo parameter.


        if (backTo && backTo.includes('expensify-card')) {
            Navigation.navigate(backTo ?? ROUTES.WORKSPACE_EXPENSIFY_CARD.getRoute(policyID ?? '-1'));
        } else {
            const accountID = PersonalDetailsUtils.getPersonalDetailByEmail(data?.assigneeEmail ?? '')?.accountID
            Navigation.navigate(ROUTES.WORKSPACE_MEMBER_DETAILS.getRoute(policyID, accountID ?? -1));
        }

What alternative solutions did you explore? (Optional)

Alternatively, we can use the background screen from "state.FullScreenNavigator" to determine which page to navigate to when the user submits a card.

If the background screen is the Expensify Card Page, we will navigate to that page.
If the background screen is the Member Page, we will navigate to the Member Detail Page.

        const stateTopmostFullScreen = getTopmostFullScreenRoute(navigationRef.getRootState());
        if (stateTopmostFullScreen && stateTopmostFullScreen.name === SCREENS.WORKSPACE.MEMBERS) {
            const accountID = PersonalDetailsUtils.getPersonalDetailByEmail(data?.assigneeEmail ?? '')?.accountID
            Navigation.navigate(ROUTES.WORKSPACE_MEMBER_DETAILS.getRoute(policyID, accountID ?? -1));
        } else {
            Navigation.navigate(backTo ?? ROUTES.WORKSPACE_EXPENSIFY_CARD.getRoute(policyID ?? '-1'));
        }

@Christinadobrzyn
Copy link
Contributor

I'm not sure if this needs to be a backend but since we have a proposal let's start with External

@Christinadobrzyn Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label Oct 7, 2024
@melvin-bot melvin-bot bot changed the title Expensify Card -App returns to card feed selection page after issuing card from member profile [$250] Expensify Card -App returns to card feed selection page after issuing card from member profile Oct 7, 2024
Copy link

melvin-bot bot commented Oct 7, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 7, 2024
Copy link

melvin-bot bot commented Oct 7, 2024

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

@paultsimura
Copy link
Contributor

Reviewing soon 👀

@paultsimura
Copy link
Contributor

The proposal by @abzokhattab looks good to me.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Oct 8, 2024

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

@cretadn22
Copy link
Contributor

@paultsimura The selected proposal will break the back button behaviors

@paultsimura
Copy link
Contributor

The selected proposal will break the back button behaviors

@cretadn22 when making such claims, please attach corresponding recordings.

@cretadn22
Copy link
Contributor

On the assignee step page, when we reload and hit the back button, the navigation should take us to the WorkspaceMemberNewCardPage as determined by the backTo route. As such, I don't think we should alter the backTo parameter in this case.

@paultsimura
Copy link
Contributor

On the assignee step page, when we reload and hit the back button, the navigation should take us to the WorkspaceMemberNewCardPage as determined by the backTo route.

It doesn't work like this now, does it?

@cretadn22
Copy link
Contributor

There's another issue here: after reloading the page, we’re navigating to the Expensify Card in the background, it should remain on the Member Page as expected. After reloading and clicking the back button, the navigation should go to the WorkspaceMemberNewCardPage

@cretadn22
Copy link
Contributor

Overall, I don’t think the selected proposal is a good pattern for our app. Consider this chain of navigation: A -> B -> C. When we navigate to a new page (C), the backTo parameter should reference the previous page (B), not the page before that (A). Setting it to A can be confusing and may lead to unintended side effects.

@paultsimura
Copy link
Contributor

This is a common pattern for deeply nested scenarios when we pass the 'backTo' param from one page to another to be able to eventually navigate back to the root point of the flow.

e.g. here:

const navigateToEditCategory = () => {
if (backTo) {
Navigation.navigate(ROUTES.SETTINGS_CATEGORY_EDIT.getRoute(policyID, policyCategory.name, backTo));
return;
}
Navigation.navigate(ROUTES.WORKSPACE_CATEGORY_EDIT.getRoute(policyID, policyCategory.name));
};

@mountiny
Copy link
Contributor

mountiny commented Oct 8, 2024

@koko57 @VickyStash can you look at the proposals? I think this must be regression from adding the select feed option

@koko57
Copy link
Contributor

koko57 commented Oct 8, 2024

@mountiny @paultsimura I agree with @paultsimura - @abzokhattab proposal looks good. We're using this pattern quite frequently and it doesn't cause any problems with normal back button behaviour.

@cretadn22 could you please add some evidence (screen recording) that it doesn't work as expected and breaks the navigation?

@narefyev91
Copy link
Contributor

Agree with @paultsimura - that's why backTo was introduced - we have a lot of pages which may start the same workflows from different starting point. And when user comes to the last step of the workflow - navigation should move them back to the initial screen

@mountiny
Copy link
Contributor

mountiny commented Oct 8, 2024

@abzokhattab can you please raise the PR? thanks!

Copy link

melvin-bot bot commented Oct 8, 2024

📣 @abzokhattab 🎉 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 📖

@paultsimura
Copy link
Contributor

@abzokhattab I've seen you actively posting new proposals while this issue has been pending a PR for 2 days now. Please prioritize opening a PR here ASAP.

@abzokhattab
Copy link
Contributor

abzokhattab commented Oct 10, 2024

Thanks for the feedback. And sorry i was working on other PRs

Noted. will work on it today.

@Christinadobrzyn
Copy link
Contributor

Looks like the PR is in staging so we're getting close! #50597

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 17, 2024
@melvin-bot melvin-bot bot changed the title [$250] Expensify Card -App returns to card feed selection page after issuing card from member profile [HOLD for payment 2024-10-24] [$250] Expensify Card -App returns to card feed selection page after issuing card from member profile Oct 17, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 17, 2024
Copy link

melvin-bot bot commented Oct 17, 2024

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

Copy link

melvin-bot bot commented Oct 17, 2024

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

If no regressions arise, payment will be issued on 2024-10-24. 🎊

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

Copy link

melvin-bot bot commented Oct 17, 2024

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:

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

@paultsimura
Copy link
Contributor

  • The PR that introduced the bug has been identified. Link to the PR: [No QA] Feat/47377 multiple card feeds #49148
  • 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: https://github.com/Expensify/App/pull/49148/files#r1809216202
  • 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: N/A
  • Determine if we should create a regression test for this bug: Yes
  • 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.

Regression Test Proposal

Precondition:

  • Bank account is already set up in Expensify Card page.
  • The company card feature is enabled.

Test:

  1. Go to workspace settings > Members.
  2. Click on any member.
  3. Click "New Card".
  4. Click "Expensify Card".
  5. Go through the setup process.
  6. Click "Issue card" on the confirmation page.
  7. Ensure that the app returns to the member profile page after issuing a card from the member profile.

Do we agree 👍 or 👎

@Christinadobrzyn
Copy link
Contributor

Payment coming in a few days so I'm going to move this to daily so I don't miss it

@Christinadobrzyn Christinadobrzyn added Daily KSv2 and removed Weekly KSv2 labels Oct 22, 2024
@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Oct 23, 2024

Prepping for payment...

Regression test - https://github.com/Expensify/Expensify/issues/438129

@melvin-bot melvin-bot bot added Daily KSv2 and removed Daily KSv2 labels Oct 23, 2024
@Christinadobrzyn
Copy link
Contributor

Payment day! Paid out based on this summary - #50243 (comment)

Closing

@github-project-automation github-project-automation bot moved this from Hold for Payment to Done in [#whatsnext] #expense Oct 24, 2024
Copy link

melvin-bot bot commented Oct 24, 2024

@mountiny @Christinadobrzyn Be sure to fill out the Contact List!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Archived in project
Development

No branches or pull requests

8 participants