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.98k stars 2.98k forks source link

[$250] Expensify Card - App uses data from other workspace when assigning card in different workspace #55125

Open IuliiaHerets opened 2 weeks ago

IuliiaHerets commented 2 weeks 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: 9.0.84-0 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught during regression testing, add the test name, ID and link from TestRail: Exp Email or phone of affected tester (no customers): applausetester+110106kh@applause.expensifail.com Issue reported by: Applause Internal Team Device used: Mac 15.0 / Chrome App Component: Workspace Settings

Action Performed:

Precondition:

  1. Go to staging.new.expensify.com
  2. Go to workspace settings of the first workspace > Expensify Card.
  3. Click Issue card.
  4. Select the workspace member in the first step.
  5. Reach the confirmation page.
  6. Exit the flow by clicking outside RHP.
  7. Go to workspace settings of the second workspace > Expensify Card.
  8. Click Issue card.
  9. Click Issue card on confirmation page.
  10. Enter magic code.

Expected Result:

In Step 8, the card assignment flow should start from the beginning because it is a different workspace.

Actual Result:

In Step 8, the card assignment flow continues with data from the first workspace when admin assigns card on the second workspace. Member from the first workspace appears on confirmation page. When entering correct magic code, it shows error.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/38f45c8b-3067-4819-887b-d467a7ceaa84

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021879481800916724750
  • Upwork Job ID: 1879481800916724750
  • Last Price Increase: 2025-01-15
  • Automatic offers:
    • brunovjk | Reviewer | 105728846
    • twilight2294 | Contributor | 105728851
Issue OwnerCurrent Issue Owner: @brunovjk
melvin-bot[bot] commented 2 weeks ago

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

twilight2294 commented 1 week ago

Proposal

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

App uses data from other workspace when assigning card in different workspace

What is the root cause of that problem?

We use ISSUE_NEW_EXPENSIFY_CARD to get the data for issue card flow, There is no way currently to know if the data is for that unique policy, This causes the data in ISSUE_NEW_EXPENSIFY_CARD to persist amongst all policies.

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

We need to make ISSUE_NEW_EXPENSIFY_CARD a Onyx collection and then store the value for each Policy.

First update the key here to:

    /** Stores the information about the state of issuing a new card */
    ISSUE_NEW_EXPENSIFY_CARD: 'issueNewExpensifyCard_',

This will help us in storing the data for each unique policyID, Then remove the onyx assignment here and instead make it a collection:

[ONYXKEYS.COLLECTION.ISSUE_NEW_EXPENSIFY_CARD]: OnyxTypes.IssueNewCard;

Then we for all the places we access this data, we need to update it to store/clear that particular policyID 's card data.

And for the original bug in the OP, we need to update the following code:

https://github.com/Expensify/App/blob/fb487a59dada9d0d0f8215408287ba86f0eac98b/src/pages/workspace/expensifyCard/issueNew/IssueNewCardPage.tsx#L24-L29

To access for unique policyID:


 const policyID = policy?.id ?? '-1'; 
 const [issueNewCard] = useOnyx(`${ONYXKEYS.COLLECTION.ISSUE_NEW_EXPENSIFY_CARD}${policyID}`);

This is only a template implementation details will be done during PR phase. Note that we need to update the places where we used to store ISSUE_NEW_EXPENSIFY_CARD data and clear it as well.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A but if we want we can write tests which clear the onyx data using clearIssueNewCardFlow and match if that policyID data is cleared

What alternative solutions did you explore? (Optional)

twilight2294 commented 1 week ago

@laurenreidexpensify can you make this one external please ๐Ÿ˜„

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

cretadn22 commented 1 week ago

Proposal

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

App uses data from other workspace when assigning card in different workspace

What is the root cause of that problem?

The ISSUE_NEW_EXPENSIFY_CARD is not being cleared, causing old data to persist and be used in the new flow.

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

We need to clear the ISSUE_NEW_EXPENSIFY_CARD when the user exits the Expensify Card Page. In

https://github.com/Expensify/App/blob/d326b1626763b56568a416c920c5aeddd40149b1/src/pages/workspace/expensifyCard/WorkspaceExpensifyCardPage.tsx#L21

add a useEffect with a clean-up function to clear ISSUE_NEW_EXPENSIFY_CARD field

    useEffect(() => {
        return () => {
            Card.clearIssueNewCardFlow()
        }
    }, [])

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We might add a test to verify that the ISSUE_NEW_EXPENSIFY_CARD data is correctly removed upon exiting the expensify card page

What alternative solutions did you explore? (Optional)

brunovjk commented 1 week ago

I couldnโ€™t enable Expensify Card for two workspaces. Everything works fine for the first one, but the bank account validation is pending for the second workspace:

Image

@twilight2294 and @cretadn22, can you tell me how did you manage to get past this? Thank you very much.

twilight2294 commented 1 week ago

@brunovjk you can follow the steps this: create 2 test accounts both with bank account added and policy active with expensify cards, now from one account, invite another test account as admin, so that you can access expensify cards page on both policies (your own and the other test accounts), the reproduced bug is as shown in the video:

https://github.com/user-attachments/assets/377d05b6-8d63-4722-ba7c-6f87e3d4f761

Let me know if you need more info, happy to help. Also let me know if you have any concerns with my proposed approach

brunovjk commented 1 week ago

Thank you, @twilight2294, for the detailed stepsโ€”they worked perfectly.

Both proposals seems to correctly identify the root cause, and both solutions seem feasible. However, we need internal confirmation on the expected behavior. Clearing the Onyx data (proposal 2) offers a straightforward approach but might not align with the intended design. On the other hand, storing the value for each policy (proposal 1) seems more robust. @Julesssss Could you confirm the expected behavior here, so we can proceed with the most suitable solution?

๐ŸŽ€๐Ÿ‘€๐ŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 week ago

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

nkdengineer commented 1 week ago

๐Ÿšจ Edited by proposal-police: This proposal was edited at 2025-01-16 14:17:10 UTC.

Proposal

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

In Step 8, the card assignment flow continues with data from the first workspace when admin assigns card on the second workspace. Member from the first workspace appears on confirmation page. When entering correct magic code, it shows error.

What is the root cause of that problem?

We're using ONYXKEYS.ISSUE_NEW_EXPENSIFY_CARD for all issue card flow. When we start issuing a new card, we don't clear the exist data of the previous flow then it can use the data from other workspace.

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

We should clear the data when starting a new issue card flow. I don't think we need to update ONYXKEYS.ISSUE_NEW_EXPENSIFY_CARD to a collection data to store for each policy because this flow doesn't have not much data and we don't need to enter the input so much.

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

https://github.com/Expensify/App/blob/86a7c900fa8a647537a45b0423625796b240e3bd/src/pages/workspace/expensifyCard/WorkspaceExpensifyCardListPage.tsx#L62-L69

Optional: We can add a new field in ONYXKEYS.ISSUE_NEW_EXPENSIFY_CARD to store the policyID and we only clear the data if we issue a new card in another policy

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

None

What alternative solutions did you explore? (Optional)

We can use useBeforeRemove in IssueNewCardPage to clear the issue card flow data when it's closed and we can create a useEffect to clear the issue card flow data if we access this page by deeplink.

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

nkdengineer commented 1 week ago

@brunovjk If we want to clear the data whenever we issue a new card, we should do when we click on the button. Clear the data when the component is unmounted will not work if we in this page and close the tab.

twilight2294 commented 1 week ago

just noting here that we do not intent to clear the data if the user closes the flow midway, we only clear the data if the user closes the flow from the very first step (assignee) :

https://github.com/Expensify/App/blob/3cb915ff1bd2008dbf60db50686947802c1b8792/src/pages/workspace/expensifyCard/issueNew/AssigneeStep.tsx#L59-L67

This was done on purpose, clearing the data when starting a new issue card flow is not intended as we want to continue with the flow of assignment if left midway, we would just need to store individual values for each policy (i.e. make a collection),

You can refer to PR:

This gives perfect example of the cases where we need to clear the data

Julesssss commented 1 week ago

We need to make ISSUE_NEW_EXPENSIFY_CARD a Onyx collection and then store the value for each Policy.

Proposal 1 is the preferred method, as supporting users with multiple policies is essential.

melvin-bot[bot] commented 1 week ago

๐Ÿ“ฃ @brunovjk ๐ŸŽ‰ 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 1 week ago

๐Ÿ“ฃ @twilight2294 ๐ŸŽ‰ 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 ๐Ÿ“–

twilight2294 commented 1 week ago

Will make a PR later today

melvin-bot[bot] commented 5 days ago

@Julesssss, @laurenreidexpensify, @brunovjk, @twilight2294 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

brunovjk commented 5 days ago

How are we doing with PR @twilight2294? Can I help with anything? Thanks.

twilight2294 commented 5 days ago

sorry, i was unexpectedly out on friday, Will raise today

brunovjk commented 5 days ago

No worries, just checking :D

twilight2294 commented 5 days ago

sorry for the delay, PR is ready for your review @brunovjk

brunovjk commented 4 days ago

Great, Reviewing today still.