[HOLD] [LOW] [Splits] IOU - Re-creating Split Bill with anonymous gmail members causes IOU to crash

[lanitochka17]

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: 1.3.88-1

Reproducible in staging?: Yes

Reproducible in production?: No

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. Open New Expensify app
2. Log in with an expensifail account
3. Create Split Bill with gmail accounts with which there was no conversation history before
4. In the created group DM with these members, request the Split Bill again

Expected Result:

Re-creating Split Bill with anonymous gmail members should register IOU with no errors for both the re-request and the first

Actual Result:

Re-creating Split Bill with anonymous gmail members causes IOU to crash

Workaround:

Unknown

Platforms:

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

• [x] Android: Native
• [x] Android: mWeb Chrome
• [x] iOS: Native
• [x] iOS: mWeb Safari
• [x] Windows: Chrome
• [x] MacOS: Desktop

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

Windows: Chrome

https://github.com/Expensify/App/assets/78819774/d058135b-bade-4af3-a726-e08bcc53cd5f

MacOS: Desktop

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @youssef-lr

Issue Owner

Current Issue Owner: @youssef-lr

[melvin-bot[bot]]

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

[melvin-bot[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

[OSBotify]

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot[bot]]

Triggered auto assignment to @flodnv (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[pradeepmdk]

Proposal

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

IOU - Re-creating Split Bill with anonymous gmail members causes IOU to crash

What is the root cause of that problem?

the root cause when we call SplitBill its throwing the error

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

this is backend issue. because first time we re calling SplitBillAndOpenReport after that we are personal details with real account id. then we are calling SplitBill with those id when create split from report

onemore issue when we call SplitBillAndOpenReport it will return personal details without login key.

[mountiny]

This is an backend issue so not App deploy blocker technically

[flodnv]

@luacmartins @mountiny seems like you got this?

[mountiny]

Yes, this PR should fix it https://github.com/Expensify/Web-Expensify/pull/39344

[mountiny]

Or to be clearer this should have fixed it already https://github.com/Expensify/Web-Expensify/pull/39348 and the other pr is more of a clean up

@lanitochka17 @pradeepmdk can you please retest in staging

[pradeepmdk]

@mountiny just now tested still there

[mountiny]

@pradeepmdk do you need ot sign out and sign back in between the splits or you just do the request again right after it?

[pradeepmdk]

@mountiny i just request again

[mountiny]

@youssef-lr was debugging with Youssef and there are some other cases where this can fail due to the optimistic reports/ accountIDs, fix is underway

[luacmartins]

PRs were merged and CPed to staging. I think we're good to test again!

[youssef-lr]

@luacmartins there is one case that still fails and I'm working on it. The steps to reproduce are:

1. Create a group chat with unknown users
2. Chat with one of the unknown users
3. Split with the same users from Global Create.

When the options are selected from Global Create and they are unknown, they contain optimistic accountIDs so we fail to retrieve the existing group chat report or individual 1:1 report, so we send new optimistic IDs which make this throw.

that's the error @pradeepmdk is experiencing here

[luacmartins]

Ah I see! Thanks for clarifying and working on it!

[youssef-lr]

Np! Also I edited my comment, without Step 2 this should work fine, so we half fixed this so far by using the group chat reportID returned from Auth.

https://github.com/Expensify/App/assets/9680864/bfa0f30c-415d-4dd0-9832-6797d34bd4c0

[pradeepmdk]

@youssef-lr looks like one more case https://github.com/Expensify/App/issues/30220 do you think this will fixed here?

[ntdiary]

Hey, @youssef-lr, do we have any plans to fix this behavior?

When the options are selected from Global Create and they are unknown, they contain optimistic accountIDs so we fail to retrieve the existing group chat report or individual 1:1 report, so we send new optimistic IDs which make this throw.

Additionally, recreating an account info for an unknown user during search seems a bit weird (if their report has been created), we can see the avatar change. The existing personal detail (or report) neither appeared in Recents nor Contacts (because it was filtered out due to missing login field), and the new account info appeared in the userToInvite section.

https://github.com/Expensify/App/assets/8579651/73e53456-00a7-4227-8a57-24efca7d6ae4

[DylanDylann]

@ntdiary My suggestion is mentioned in the proposal:

In here: https://github.com/Expensify/App/blob/eef8347c59b5a3bb7fc508484f4c32ff092a05da/src/libs/OptionsListUtils.js#L1243-L1244 we use !reportOption.login to check if the report has multiple participants or not, but it also removes the report with NEW account, because its login value is undefined. So we can check null rather than just !reportOption.login

[ntdiary]

@DylanDylann, don't worry, I've thoroughly read your proposal the past couple days. It's just that getOptions is a function that will be called in multiple scenarios, and this issue is also related, so I feel it would be better to discuss it first.

[DylanDylann]

Yeah. I just want to wrap my suggestion solution for everyone who does not have a context in that issue @ntdiary

[melvin-bot[bot]]

@youssef-lr, @luacmartins, @stephanieelliott, @mountiny Whoops! This issue is 2 days overdue. Let's get this updated quick!

[mountiny]

I have not really helped with the fix here, so I am going to unassign myself from this one

[youssef-lr]

we use !reportOption.login to check if the report has multiple participants or not, but it also removes the report with NEW account, because its login value is undefined.

yeah I did come across this, I have a draft PR that fixes it but it also removes relying on the login to check for accounts from the search and uses accountID instead, I have one remaining bug to resolve before I can make it ready. I don't think it's as simple as replacing the check with null, are there any side effects when you apply that change @DylanDylann?

[youssef-lr]

Also in my draft PR, I have made this change to the line you linked:

if (!isCurrentUserOwnedPolicyExpenseChatThatCouldShow && !includeMultipleParticipantReports && reportOption.participantAccountIDs.length > 1

[DylanDylann]

@youssef-lr With my proposal, we need to apply the changes in a few sections. But before creating a draft PR, I want to confirm that: Should we fix this issue from BE side? For example, when we create a new chat with a new account, BE will return the report with the login field

[stephanieelliott]

Bump on the above @youssef-lr -- what do you think?

[youssef-lr]

Should we fix this issue from BE side? For example, when we create a new chat with a new account, BE will return the report with the login field

I don't think the backend sends this, have you confirmed it in the Network tab? We set this in OptionUtils when creating the optimistic account from the search.

[DylanDylann]

@youssef-lr I mean that, currently, when we start a chat with the new account, BE will return the new account`s personalDetail without the login field, which leads to the bug. So I want to confirm that, should we let BE return the login field in personalDetail?

[ntdiary]

Also in my draft PR, I have made this change to the line you linked:

if (!isCurrentUserOwnedPolicyExpenseChatThatCouldShow && !includeMultipleParticipantReports && reportOption.participantAccountIDs.length > 1

interesting, based solely on its comments, this looks promising. I also traced back the git history and found that the condition to add the login field was introduced in the PR #1252 2 years ago, but I didn't find the exact reason for adding it, so I'm not quite sure if replacing it will cause a regression. 😂

cc @marcaaron (because you are the author of the PR). 🙂

https://github.com/Expensify/App/blob/c17b9c22c064883eb3ffa1bc622f288dec985433/src/libs/ReportListUtils.js#L234-L235

[marcaaron]

@ntdiary Can you maybe ask your question another way or add more details to it?

I also traced back the git history and found that the condition to add the login field was introduced

Which condition exactly?

I didn't find the exact reason for adding it, so I'm not quite sure if replacing it will cause a regression

Replace it with what?

[marcaaron]

Thinking through what the code is doing...

 if (!includeMultipleParticipantReports && !reportOption.login) {

!includeMultipleParticipantReports

This means don't want to include the multiple participants.

this report has multiple participants

Apparently we know that because !reportOption.login

Sounds weird. Let's look at where things were at when we wrote this...

https://github.com/Expensify/App/blame/c17b9c22c064883eb3ffa1bc622f288dec985433/src/libs/ReportListUtils.js#L106-L108

[DylanDylann]

@marcaaron so you use reportOption.login to check if the report has multiple participants or not, right? If true, there is the case: The report that does not have multiple participants have the login field is undefined. So !reportOption.login in this case return true

[ntdiary]

@ntdiary Can you maybe ask your question another way or add more details to it?

I also traced back the git history and found that the condition to add the login field was introduced

Which condition exactly?

I didn't find the exact reason for adding it, so I'm not quite sure if replacing it will cause a regression

Replace it with what?

https://github.com/Expensify/App/blob/24184e4a987aba011b5231680da66dcef11f6a24/src/libs/OptionsListUtils.js#L1244

@marcaaron, ah, sorry for the confusion, I meant that I found the !includeMultipleParticipantReports && !reportOption.login was added by you.

Now, the problem we are facing is, if we create a chat with a new account, and then request money from that account, we will fail to retrieve the existing report because !reportOption.login is true, and then we will send a new optimistic reportID to server, thus causing the request to fail. @youssef-lr has a draft PR, which will replace !reportOption.login with reportOption.participantAccountIDs.length > 1. we are trying to retrieve the existing report.

https://github.com/Expensify/App/assets/8579651/76514dd8-d331-44b7-a8e5-ae5b39369b4c

[marcaaron]

if we create a chat with a new account, and then request money from that account, we will fail to retrieve the existing report because !reportOption.login is true

Interesting, why is it undefined?

and then we will send a new optimistic reportID to server, thus causing the request to fail

Sorry, I didn't quite understand this part. Without doing a deeper dive into this issue it sounds like an edge case that we haven't considered because the logic for whether to show group DMs in a selector predates the addition of manual requests.

If we want those group DMs to show up in some other selector then it sounds like we need better logic to tell if a chat has multiple participants. And probably the option could have a boolean like hasMultipleParticipants instead of using the login (which looking back, I can see is confusing). Maybe it was reliable to use the existence of the login at once point. But sounds like we need some other solution now.

Please do let me know if there is a more specific question. Thanks!

[melvin-bot[bot]]

@youssef-lr, @luacmartins, @stephanieelliott Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot[bot]]

@youssef-lr, @luacmartins, @stephanieelliott 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[melvin-bot[bot]]

@youssef-lr, @luacmartins, @stephanieelliott 10 days overdue. Is anyone even seeing these? Hello?

[melvin-bot[bot]]

@youssef-lr, @luacmartins, @stephanieelliott 12 days overdue now... This issue's end is nigh!

[melvin-bot[bot]]

This issue has not been updated in over 14 days. @youssef-lr, @luacmartins, @stephanieelliott eroding to Weekly issue.

[ntdiary]

Hi, @youssef-lr, is there any update on your PR? :)

[stephanieelliott]

Hey @youssef-lr I'm not able to find the draft PR, mind linking it here?

[youssef-lr]

I haven't pushed yet, I will try to make a PR this week @stephanieelliott

[stephanieelliott]

I'm thinking this belongs on #vip-split, posted in Slack to confirm: https://expensify.slack.com/archives/C05RECHFBEW/p1704325452233169

[stephanieelliott]

Removing the Reviewing label since that refers to the first PR (https://github.com/Expensify/Web-Expensify/pull/39348). Now working on the followup PR, so we'll be going through the review cycle again!

[luacmartins]

I'll unassign myself since @youssef-lr seems to be handling this.

[stephanieelliott]

Hey @youssef-lr any update on a PR for this one?

[youssef-lr]

Resuming work on this on Monday @stephanieelliott

[youssef-lr]

I have a PR in the works that's going to partially help solve this, as long as the user doesn't log out. It should be ready for review by EOD Monday.