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.56k stars 2.9k forks source link

[$500] Split Bill - Group members disappeared in a split bill when make second split bill in a row #33518

Closed lanitochka17 closed 8 months ago

lanitochka17 commented 11 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: 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: Slack conversation:

Issue found when executing PR https://github.com/Expensify/App/pull/29996

Action Performed:

  1. Open https://staging.new.expensify.com/
  2. Create a group chat with about 3-4 members
  3. Send a message to the chat
  4. Create a split bill with all members
  5. Create a second split bill in a row with the same members

Expected Result:

User should be able to make a two split bills in a row in a group chat

Actual Result:

Group members disappeared in a split bill when make second split bill in a row

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/f0bb027b-ae52-42d7-ac16-1be4190e2d8d

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d3ed805fe8e7116d
  • Upwork Job ID: 1738267582378926080
  • Last Price Increase: 2024-01-19
  • Automatic offers:
    • abdulrahuman5196 | Reviewer | 0
Issue OwnerCurrent Issue Owner: @abdulrahuman5196
abdulrahuman5196 commented 9 months ago

Created a slack thread here - https://expensify.slack.com/archives/C01GTK53T8Q/p1706702319980289

alexpensify commented 9 months ago

@abdulrahuman5196 - I believe we are waiting on more feedback or good to go here?

abdulrahuman5196 commented 9 months ago

We still don't have complete solution for the stale data problem

alexpensify commented 9 months ago

Thank you for the update!

alexpensify commented 9 months ago

Still open for proposals here.

melvin-bot[bot] commented 9 months ago

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

abdulrahuman5196 commented 9 months ago

Let me investigate from my end if some option is possible, if not I would suggest to go with existing proposal. Since we don't have a solid solution for the stale data issue.

alexpensify commented 9 months ago

Thanks for the update!

melvin-bot[bot] commented 9 months ago

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

abdulrahuman5196 commented 9 months ago

Let me investigate from my end if some option is possible, if not I would suggest to go with existing proposal. Since we don't have a solid solution for the stale data issue.

Just came from weekend. Will have a look into this tomorrow.

alexpensify commented 9 months ago

Thank you!

alexpensify commented 9 months ago

@abdulrahuman5196 - any update? Thanks!

abdulrahuman5196 commented 9 months ago

Hi, No update yet. Will come to conclusion in a day.

abdulrahuman5196 commented 9 months ago

On the proposal by @dukenv0307 here https://github.com/Expensify/App/issues/33518#issuecomment-1868906093, we didn't get any idea for better disposal of the stale data and it would include multiple changes and has its own cons as pointed out here. So I am not aligned to go with the approach IMO.

abdulrahuman5196 commented 9 months ago

@adriancova 's proposal here https://github.com/Expensify/App/issues/33518#issuecomment-1867998289 looks good and it is enough to solve this particular issue without any side effects as far as I tested. It seems we do clear transaction draft data in optimistic data itself for scan flow as pointed out here https://github.com/Expensify/App/issues/35064#issuecomment-1908390008. So we can do the same logic here as well and remove the code to clear transaction data on successData and failureData.

πŸŽ€ πŸ‘€ πŸŽ€ C+ Reviewed

melvin-bot[bot] commented 9 months ago

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

dangrous commented 9 months ago

so the onyx key you shared - ${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${CONST.IOU.OPTIMISTIC_TRANSACTION_ID} - shouldn't that only clear for the transaction with that specific optimistic ID? Wouldn't the second split bill have a new id?

abdulrahuman5196 commented 9 months ago

so the onyx key you shared - ${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${CONST.IOU.OPTIMISTIC_TRANSACTION_ID} - shouldn't that only clear for the transaction with that specific optimistic ID? Wouldn't the second split bill have a new id?

@dangrous That's not the case currently. A new draft transaction uses same optimistic transaction id as a constant(${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${CONST.IOU.OPTIMISTIC_TRANSACTION_ID}) and it will be used for second bill as well.

https://github.com/Expensify/App/blob/91ea9792fe54b795115106965b2682dd1ef36847/src/pages/home/report/ReportActionCompose/AttachmentPickerWithMenuItems.js#L145-L154

That's why when we delete the data with the same ID of the split bill in successData, it affects the second transaction in draft state. So the proposal suggests to clear the transaction data in optimisticData itself so that it won't affect the second bill, which should fix the current issue.

melvin-bot[bot] commented 9 months ago

@dangrous, @alexpensify, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

dangrous commented 9 months ago

Oh weird, seems odd that we'd use the same optimistic id. But in that case the solution makes sense!

dangrous commented 9 months ago

@MelvinBot? Do you want to do all the assigning stuff?

melvin-bot[bot] commented 9 months ago

Current assignee @abdulrahuman5196 is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 9 months ago

πŸ“£ @abdulrahuman5196 πŸŽ‰ 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

πŸ“£ @adriancova You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing πŸ“–

dangrous commented 9 months ago

there we go.

adriancova commented 9 months ago

Hey guys, I'll send a pr in a couple of hours. But it seems I can no longer apply to the upwork job, it's showing as "no longer available". will that be an issue? @dangrous

dangrous commented 8 months ago

We can recreate it as needed, I know upwork is weird sometimes! cc @alexpensify

alexpensify commented 8 months ago

I'll need to create a new job in Upwork but will do so when the automation flags for payment kicks in. I've noted I need to create a new job.

alexpensify commented 8 months ago

I'm aware that this one is in staging now and will watch out for it to go to production

abdulrahuman5196 commented 8 months ago

The PR that introduced the bug has been identified. Link to the PR: 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: 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:

Not a regression. Seems to be a bug for sometime.

Determine if we should create a regression test for this bug. 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.

No. Minor case which is hard to reproduce.

adriancova commented 8 months ago

Hey again

sorry, first time contributor here. is there something else I need to do on my end?

alexpensify commented 8 months ago

@adriancova - no action is needed here. @abdulrahuman5196 confirmed that this PR didn't cause regression. The only remaining action is the payment process. Tomorrow marks the 7-day mark, and I can start the payment process then.

alexpensify commented 8 months ago

Here is the payment summary:

Upwork Job: https://www.upwork.com/jobs/~01d3ed805fe8e7116d

Extra Notes regarding payment: @adriancova - I'm having trouble finding you on Upwork. Can you share your Upwork profile URL and I can try to find you that way? Thanks!

adriancova commented 8 months ago

Hey @alexpensify, sure I just changed the url to make it easier to find: https://www.upwork.com/freelancers/adriancova

Let me know if I can help with anything else, and thank you!

alexpensify commented 8 months ago

Thanks, I had to spin up a new job but invited you to it here:

https://www.upwork.com/jobs/~01696e82885325d5bd

Please accept in Upwork and I can complete the next steps. Thanks!

alexpensify commented 8 months ago

Ok, all set here-- everyone has been paid in Upwork.