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.33k stars 2.76k forks source link

[HOLD for payment][$250] IOU - App crashes when admin pay elsewhere #47542

Open IuliiaHerets opened 1 month ago

IuliiaHerets commented 1 month 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.21-0 Reproducible in staging?: Y Reproducible in production?: Y Issue was found when executing this PR https://github.com/Expensify/App/pull/47398 Email or phone of affected tester (no customers): applausetester+gm103030@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Create a Control workspace.
  2. Submit an expense as an employee.
  3. Pay the expense as the admin.
  4. As admin navigate to payed thread transaction

Expected Result:

App should not crash

Actual Result:

App crashes when admin pay elsewhere

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/0269fd43-9a19-4270-bc67-25165b1996eb

1608_1.txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01374ae222703cc250
  • Upwork Job ID: 1825649554698082303
  • Last Price Increase: 2024-08-19
Issue OwnerCurrent Issue Owner: @thesahindia
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @johncschuster (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 commented 1 month ago

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

IuliiaHerets commented 1 month ago

@johncschuster 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

bernhardoj commented 1 month ago

Proposal

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

App crash when employee submit request and then the admin open the expense report.

What is the root cause of that problem?

The error comes from hasEnabledTags function. https://github.com/Expensify/App/blob/cf897a002e8949eb429026a020f23f92c2de6bee/src/components/ReportActionItem/MoneyRequestView.tsx#L220

Inside the function, we map over the tag array, and the map callback destructuring the tag by taking the tags property, but in our case, the tags is undefined, so Object.values throws an error. https://github.com/Expensify/App/blob/cf897a002e8949eb429026a020f23f92c2de6bee/src/libs/OptionsListUtils.ts#L1366-L1367

The tag array (policyTagList) normally looks like this:

[{name: "Tag", orderWeight: 0, required: false, tags: {...}}]

The array is constructed from the tag object by taking the object values. https://github.com/Expensify/App/blob/cf897a002e8949eb429026a020f23f92c2de6bee/src/libs/PolicyUtils.ts#L274-L279

But in our case, the tag list looks like this:

image

Each object in the array structure is totally different from the normal structure above and doesn't have a tags property, so the app crashes.

The reason of that is because, when the admin receives the pusher data of the new money request, the transaction thread report contains a very minimal information which doesn't include policyID, so when we are trying to get the policy tag object, the policyID defaults to an empty string and fetch the whole tag collections. https://github.com/Expensify/App/blob/cf897a002e8949eb429026a020f23f92c2de6bee/src/components/ReportActionItem/MoneyRequestView.tsx#L692-L694

policy_A: {
   Tag: {},
},
policy_B: {
    Tag: {},
}

So, when we get the tag object values, we get [{Tag: {}}, {Tag: {}}] instead of [{name: "Tag", orderWeight: 0, required: false, tags: {...}}].

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

We need to default to -1 here so it won't fetch the whole collection of tag. If it's -1, then the policyTagList will be undefined and the array will be empty. https://github.com/Expensify/App/blob/cf897a002e8949eb429026a020f23f92c2de6bee/src/components/ReportActionItem/MoneyRequestView.tsx#L692-L694

We can default to -1 for other onyx data in the file too. https://github.com/Expensify/App/blob/cf897a002e8949eb429026a020f23f92c2de6bee/src/components/ReportActionItem/MoneyRequestView.tsx#L686-L697

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

thesahindia commented 3 weeks ago

@bernhardoj's proposal looks good!

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 3 weeks ago

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

chiragsalian commented 3 weeks ago

Proposal LGTM. Feel free to create the PR @bernhardoj. Also add a comment explaining why the default is -1 otherwise, it could be confusing for someone looking at the code

bernhardoj commented 3 weeks ago

PR is ready

cc: @thesahindia

johncschuster commented 4 days ago

Payment Summary:

Contributor: @bernhardoj owed $250 via NewDot Contributor+: @thesahindia owed $250 via NewDot

johncschuster commented 4 days ago

@bernhardoj / @thesahindia do do we need regression test steps? If so, can you provide them?