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

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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 #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?

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6324217_1703269813154.Recording__32.mp4

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 Owner

Current Issue Owner: @abdulrahuman5196

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[adriancova]

Proposal

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

Draft view for a new split bill is getting reset when creating a second split bill right after another.

What is the root cause of that problem?

I've managed to reproduce the issue by throttling my network, root cause is that part of the onyx successData is setting TRANSACTION_DRAFT as null.

{
      onyxMethod: Onyx.METHOD.MERGE,
       key: `${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${CONST.IOU.OPTIMISTIC_TRANSACTION_ID}`,
       value: null,
}

So when the network call finishes and we got a new split bill in progress it gets reset.

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

Remove the TRANSACTION_DRAFT merge to null from the successData inside IOU.createSplitsAndOnyxData method

What alternative solutions did you explore? (Optional)

Instead of removing the transaction_draft, we can move it to the optimisticData, though it shouldn't be necessary since the transaction draft view is already cleaned up after submitting it.

Edit: removed reminder text

[alexpensify]

@abdulrahuman5196 - when you get a chance, can you review if this proposal will fix this issue? Thanks!

Heads up, I will be offline from Friday, December 22, to Thursday, January 4, 2024. I will not be actively watching over this GitHub during that period. If anything urgent is needed here, please ask for help in the #expensify-open-source Slack Room-- thanks!

[dukenv0307]

Proposal

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

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

What is the root cause of that problem?

We clear the transactionDraft in successData here. So if we create another split bill before the API is complete, this is removed and then group members are cleared.

App/src/libs/actions/IOU.js

Line 1235 in 7b836cf

value: null,

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

1. I think instead of using the const transactionID when creating a new request, we can generate a random transactionID by using NumberUtils.rand64() and then use it as the ID of the new transaction when we confirm to create it. This is the same way as we do for reportID.

App/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js

Line 204 in 7b836cf

ROUTES.MONEY_REQUEST_CREATE.getRoute(CONST.IOU.TYPE.REQUEST, CONST.IOU.OPTIMISTIC_TRANSACTION_ID, ReportUtils.generateReportID()),

And then we can replace CONST.IOU.OPTIMISTIC_TRANSACTION_ID with this transactionID.

2. To clear the old draft transaction, when we initialize the transaction draft we can add an extra field like expired which is 30 days (the days can be discussed in the PR) after the current date. Then whenever we open the app, we will call clearOldTransactionDraft function. This function will clear all transactions draft which has expired is less than the current date.

App/src/libs/actions/IOU.js

Line 140 in 8e2fb67

function startMoneyRequest_temporaryForRefactor(reportID, isFromGlobalCreate, iouRequestType = CONST.IOU.REQUEST_TYPE.MANUAL) {

Here is the test branch: https://github.com/dukenv0307/App/tree/fix/33518

What alternative solutions did you explore? (Optional)

For point 2, we can always clear all transaction drafts whenever we open the App. Although it's not good for the case user is starting the money request flow in another tab, this case is rare and we don't need to take care of this so much.

[melvin-bot]

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

[melvin-bot]

@alexpensify, @abdulrahuman5196 Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

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

[abdulrahuman5196]

Sorry for the delay. Will check in my morning.

[abdulrahuman5196]

Reviewing now

[abdulrahuman5196]

@dukenv0307 On proposal here #33518 (comment), it might not be that easy to replace the OPTIMISTIC_TRANSACTION_ID, it seems to be used in multiple checks. Could you kindly update your proposal to see if all cases can be handled?

@adriancova on proposal here #33518 (comment), do you suggest to not clear the draft data? I am not sure if that would be a valid fix, since the stale data will be present in that case.

[abdulrahuman5196]

Tagging @tgolen, as he made the original change.
Could you kindly let us know if there is any specific reason to have the optimistic transaction id as constant OPTIMISTIC_TRANSACTION_ID, since it could overlap with other transactions if we created multiple optimistic transaction of same report?

[dukenv0307]

@abdulrahuman5196 https://github.com/dukenv0307/App/tree/fix/33518 Here is the change, of course we will need to re-test this carefully in the PR, this branch is the detail of the main ideal.

[tgolen]

since it could overlap with other transactions if we created multiple optimistic transaction of same report

My first thought is that if this is happening, this is wrong. There should never be multiple draft transactions for the same report. Then I thought about the case where you want to create multiple requests while being offline, and maybe that's where my logic breaks down.

The reason for having OPTIMISTIC_TRANSACTION_ID was the following:

• An easy way to detect if a transaction is a draft or not. In early versions of my PR this was necessary because the draft was stored in the TRANSACTIONS collection with all other transactions. Halfway through the PR, this was moved to the TRANSACTIONS_DRAFT collection, which solved part of the problem.
• I really wanted to stay away from a lot of flags on the transaction object that aren't used anywhere in the display logic (such as the isNewTransaction property that @dukenv0307 is proposing). Though, this is more of a theoretical problem and not an actual problem.
• Prevent the draft collection from filling up with draft data for infinity. Part of the problem I see with the changes @dukenv0307 is proposing is there is no way to clean up partial drafts (ie. drafts that were abandoned before the save button was clicked).

Ideally, it would be nice if the draft transaction ID was random, but we still need to figure out a way to clean up all the abandoned data so it doesn't keep growing. Maybe this could be solved with an Onyx migration that always removes the X oldest drafts?

[dukenv0307]

@tgolen

Thank for your response

1. isNewTransaction is added to replace the condition transactionID !== CONST.IOU.OPTIMISTIC_TRANSACTION_ID

2. To clear the transaction draft if we close the modal without saving, we can create a wrapper component for iou step and in this component we will clear the transaction draft if this component is umounted without saving.

[tgolen]

Point 1 makes sense. For point 2, why is that any different than what we have today?

[dukenv0307]

If we create the transaction, the draft is already cleared in successData, so we only need to cover the case the user close the request flow without saving.

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[dangrous]

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

[dangrous]

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

[melvin-bot]

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

[melvin-bot]

📣 @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]

📣 @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]

there we go.

[adriancova]

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]

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

[alexpensify]

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]

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

[abdulrahuman5196]

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]

Hey again

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

[alexpensify]

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

Here is the payment summary:

• External issue reporter - N/A
• Contributor that fixed the issue - @adriancova $500
• Contributor+ that helped on the issue and/or PR - @abdulrahuman5196 $500

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]

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]

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]

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