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.31k stars 2.74k forks source link

[$250] Search-App crashes when deleting all expenses in Shared after deleting one of them in Expenses #45513

Closed m-natarajan closed 2 weeks ago

m-natarajan 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.7-4 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. Go to staging.new.expensify.com
  2. Go to workspace chat.
  3. Submit two expenses.
  4. Go to Search > Expenses.
  5. Go to Search > Shared (Important).
  6. Go back to Search > Expenses.
  7. Click checkbox on one expense (only one).
  8. Click on the dropdown > Delete.
  9. Delete the expense.
  10. Go to Search > Shared without switching to any other page (Important).
  11. Click Select all checkbox to select both expenses (including the already deleted one).
  12. Click on the dropdown > Delete.
  13. Delete the selected expenses.

    Expected Result:

    App will not crash.

Actual Result:

App crashes.

Workaround:

unknown

Platforms:

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

Screenshots/Videos

[Uploading Bug6543392_1721092873177!staging.new.expensify.com-1721092641739.txt…]()

https://github.com/user-attachments/assets/e468a306-3adf-47ea-be6f-1d668aaae7c7

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d3b370e3b25c0229
  • Upwork Job ID: 1814395190825877892
  • Last Price Increase: 2024-08-16
Issue OwnerCurrent Issue Owner: @rojiphil
melvin-bot[bot] commented 1 month ago

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

m-natarajan commented 1 month ago

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

m-natarajan commented 1 month ago

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

nkdengineer commented 1 month ago

Proposal

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

App crashes.

What is the root cause of that problem?

The reportIDToTransactions[reportKey] can be undefined when we delete all expense

https://github.com/Expensify/App/blob/52b8fddcda6177b4bf4aa9a803853a1b2670daef/src/libs/SearchUtils.ts#L207-L211

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

We should check reportIDToTransactions[reportKey] is undefined or not before update here

} else if (reportIDToTransactions[reportKey]) { 
     reportIDToTransactions[reportKey].transactions = [transaction]; 
} 

https://github.com/Expensify/App/blob/52b8fddcda6177b4bf4aa9a803853a1b2670daef/src/libs/SearchUtils.ts#L209-L211

What alternative solutions did you explore? (Optional)

bernhardoj commented 1 month ago

Proposal

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

The app crash when deleting all expense in Shared search page.

What is the root cause of that problem?

When we delete one of the expenses from the Expense tab and then open the Shared tab, the search API result still shows the deleted expense. The search API response returns an Onyx merge request that actually doesn't include the deleted expense, but because it's a merge request, the deleted expense isn't removed from Onyx.

For example, let's say we delete transaction1. [transaction1, transaction2] merge with [transaction2] -> [transaction1, transaction2]

Nothing changed.

The API should return null for the deleted transaction so it's removed from Onyx. Now, when we delete the grouped expense, it will delete the whole transaction. For this one, the DeleteMoneyRequestOnSearch request clears both the IOU report and the other transactions (transaction2 in our example) correctly.

image

But because the transaction1 still exists and the page re-renders, it eventually crashes in this block. https://github.com/Expensify/App/blob/b928e40d997ed2bb11bc9b990f16e422f1121bcb/src/libs/SearchUtils.ts#L209-L211

The reason is that reportIDToTransactions[reportKey] is undefined, but we are trying to access and set the transactions attribute. It's undefined because we haven't initialized the mapping for the specific reportKey yet.

The way it works is that it will initialize the reportIDToTransactions[reportKey] from the IOU report data, https://github.com/Expensify/App/blob/b928e40d997ed2bb11bc9b990f16e422f1121bcb/src/libs/SearchUtils.ts#L162-L179

then when it encounters a transaction, it will just update the reportIDToTransactions[reportKey].transactions. https://github.com/Expensify/App/blob/b928e40d997ed2bb11bc9b990f16e422f1121bcb/src/libs/SearchUtils.ts#L207-L211

But in our case, the IOU report is already deleted and the transaction1 still exists, thus it crashes.

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

Initialize the object instead of directly accessing the property.

reportIDToTransactions[reportKey] = {
    ...(reportIDToTransactions[reportKey] ?? {}),
    transactions: [transaction]
};
strepanier03 commented 1 month ago

Putting this forward for now, but I'm unable to test as I'm troubleshooting a different issue with loading staging atm.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

rojiphil commented 1 month ago

Reviewing today

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

rojiphil commented 1 month ago

Hmm… I am unsure why the deleted expense is not removed from the shared search results. Isn’t that the problem to be addressed here? cc @luacmartins

luacmartins commented 1 month ago

We should fix the transaction not being removed in the first place, but we can also introduce the conditional chaining to be safe and prevent a crash. For the first solution, I'm thinking of making the API return a set onyx update if offset = 0, since we always want to clear the results and start fresh in that case. Subsequent requests with offset > 0 would use a merge operation.

melvin-bot[bot] commented 1 month ago

@rojiphil @strepanier03 @luacmartins this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 month ago

@rojiphil, @strepanier03, @luacmartins Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 month ago

@rojiphil, @strepanier03, @luacmartins Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 1 month ago

@rojiphil, @strepanier03, @luacmartins Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 month ago

@rojiphil, @strepanier03, @luacmartins 10 days overdue. Is anyone even seeing these? Hello?

melvin-bot[bot] commented 3 weeks ago

@rojiphil @strepanier03 @luacmartins this issue is now 4 weeks old, please consider:

Thanks!

melvin-bot[bot] commented 3 weeks ago

This issue has not been updated in over 14 days. @rojiphil, @strepanier03, @luacmartins eroding to Weekly issue.

melvin-bot[bot] commented 3 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

mvtglobally commented 2 weeks ago

Issue not reproducible during KI retests. (First week)

luacmartins commented 2 weeks ago

This is fixed. Closing.