[HOLD for payment 2023-03-01] [$1000] Split bill - Group Split bill creates requests in wrong chats and displays with red dots

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Go to https://staging.new.expensify.com/ and login with any of these 3 accounts: applausetester+1014pronin@applause.expensifail.com applausetester+1027pronin@applause.expensifail.com applausetester+1412pronin@applause.expensifail.com Password for all - Price123@
2. Open group chat between those 3 accounts
3. Submit Split Bill request within this group

Expected Result:

Direct messages should appear in LHN with correct split bill details

Actual Result:

Split bill from group chat creates requests in wrong chats, which are displayed with red dots and then later disappear. Money request is not submitted in DM chat to proceed with payment After sign in / sign out split messages disappear from group chat too.

Workaround:

Unknown

Platforms:

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

• [x] Android / native
• [x] Android / Chrome
• [x] iOS / native
• [x] iOS / Safari
• [x] MacOS / Chrome / Safari
• [x] MacOS / Desktop

Version Number: 1.2.59.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/214698712-715601bc-8b69-4b7e-bbcd-40e65862ac35.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0138082f93fcf0092e
• Upwork Job ID: 1623102641144086528
• Last Price Increase: 2023-02-07

[MelvinBot]

@davidcardoza 10 days overdue. I'm getting more depressed than Marvin.

[MelvinBot]

@davidcardoza 10 days overdue. Is anyone even seeing these? Hello?

[MelvinBot]

@davidcardoza 10 days overdue. I'm getting more depressed than Marvin.

[davidcardoza]

This can go out to a contributor

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0138082f93fcf0092e

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

Current assignee @robertjchen is eligible for the Engineering assigner, not assigning anyone new.

[robertjchen]

Keeping open for any proposals from contributors.

[fedirjh]

@robertjchen we cannot test with the provided password , these accounts uses magic code.

[MelvinBot]

@robertjchen @mananjadhav @davidcardoza 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!

[mananjadhav]

@fedirjh what if you return false here

https://github.com/Expensify/App/blob/d9a126dba03ea2956176e33ff1c6d869a2653d03/src/libs/Permissions.js#L101-L103

@MelvinBot While the issue was created two weeks back, this just became external yesterday.

[fedirjh]

Thanks @mananjadhav , I was able to replicate this issue with my account too . I get this error . This is an error from the backend so maybe this issue should be internal . I will make more investigations and comment here if I got something new.

[fedirjh]

Proposal

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

1. When split a bill between 3 or more participants then an error occur and split bill from group chat creates requests in wrong chats, which are displayed with red dots and then later disappear.
2. Money request is not submitted in DM chat to proceed with payment
3. After sign in / sign out split messages disappear from group chat too.

What is the root cause of that problem?

The getChatByParticipants is not returning the right report when splitting bill , this because the logic doesn’t account the case when we have more than one report , it will instead return the first matched report and that’s not always the right report

https://github.com/Expensify/App/blob/b113107ba03dce15a4e87bad994829987f916dfa/src/libs/ReportUtils.js#L1368-L1377

I did a test by filtering the reports here

Then I got this result

As you can see here , for last email there is 2 matched reports , the right report is the second report however getChatByParticipants will return the first matched result (which is the wrong report , it's a policyRoom created by user)

Now the question , why there is a policy room for another user list here ?

This happen when you add another user to your Workspace then you create a new room policy . When the created policy room's visibility is set to restricted then other people will have access to it and it will be stored with their reports.

So the scenario that led to this bug :

1. userA adds userB to their workspace workspaceA
2. userA creates a new policy room with visibility is set to restricted let's call it roomTest within workspaceA
3. userA is the only participant of roomTest
4. roomTest is now visible to userB and it's stored with their reports as a chat report with a single participant userA
5. userB split a bill with userA and userC
6. getChatByParticipants is called to get the direct chat with userA and userC
7. getChatByParticipants fails to return the right direct chat with userA and return the roomTest report
8. Onyx merge the wrong optimistic data and lead to unexpected bugs ( the request money is sent inside roomTest )
9. Server fails to save the transaction and return an error . which display the red dot

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

We should fix getChatByParticipants to make it return the right report

        return !isUserCreatedPolicyRoom(report) && _.isEqual(newParticipantList, report.participants.sort());
// OR
        return _.isEmpty(report.chatType) && _.isEqual(newParticipantList, report.participants.sort());

What alternative solutions did you explore? (Optional)

[mananjadhav]

Thanks for the proposal @fedirjh. Your analysis and proposal looks correct to me. I would prefer to use isUserCreatedPolicyRoom as it is an existing util method.

@robertjchen All yours.

[robertjchen]

Appreciate the detailed proposal @fedirjh ! Especially the step-by-step reasoning in explaining the root cause.

Agreed with @mananjadhav in using the existing isUserCreatedPolicyRoom util method (aka the first approach).

Other than that, it looks good! Please proceed with the solution cc: @davidcardoza for next steps on hiring

[davidcardoza]

The job was added to upwork last week

Job added to Upwork: https://www.upwork.com/jobs/~0138082f93fcf0092e

[MelvinBot]

@robertjchen @mananjadhav @davidcardoza this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[fedirjh]

@davidcardoza I applied to the job , could you please assign the issue to me ?

[MelvinBot]

📣 @fedirjh You have been assigned to this job by @robertjchen! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[robertjchen]

@fedirjh Assigned!

[fedirjh]

@mananjadhav https://github.com/Expensify/App/pull/15227 is ready for review !!

[MelvinBot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[MelvinBot]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.2.75-0 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/15227

If no regressions arise, payment will be issued on 2023-03-01. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• [ ] External issue reporter
• [ ] Contributor that fixed the issue
• [ ] Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[MelvinBot]

📣 @MelvinBot! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[MelvinBot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [ ] [@mananjadhav / @robertjchen] The PR that introduced the bug has been identified. Link to the PR:
• [ ] [@mananjadhav / @robertjchen] 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:
• [ ] [@mananjadhav / @robertjchen] 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:
• [ ] [@fedirjh] Propose regression test steps to ensure the same bug will not reach production again.
• [x] [@davidcardoza] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mananjadhav]

@fedirjh can you please update a comment here with the regression test proposal?

@robertjchen I am not sure what to link here as the PR as I am not sure if the policy chats were added later or IOU search on the chats? I tried to dig into the git going through old codes, and it seems this case where participants are a part of chat + policy chat is rare that we could test it earlier. Let me know what you think?

[fedirjh]

@fedirjh can you please update a comment here with the regression test proposal?

@mananjadhav Not sure what the regression test should be since this issue is only affecting some account with a specific condition (which is randomly happen since the report ID is randomly generated)

Prerequisite :

• Account A :

  • Should have workspace A with a room policy PolicyA
  • PolicyA visibility should be set to restricted
  • PolicyA should have only one participant, which is Account A

• Account B

  • Should be added to workspace A
  • Should be able to see PolicyA
  • Should have a direct message (DM) with Account A , let's call it reportAB
  • The generated reportID of reportAB should be greater than PolicyA.reportID

Regression Test Proposal

• Create Account A and Account B with above prerequisites
• Open group chat between Account A , Account B and any other account (at least 3 accounts)
• Submit Split Bill request within this group
• Verify that Bill is created successfully
• Do we agree 👍 or 👎

[mananjadhav]

I think we should be good with the regression steps. I am not sure how pre-requisites are added in Testrail by the QA team here. Whether they're a part of the QA steps itself or added separately as prerequisite (I generally do it this way).

@robertjchen Quick bump on this one.

[robertjchen]

Yeah, agreed! I couldn't pin down an explicit PR that introduced this issue, it seems like it was just a corner case that wasn't discovered until now. We now have a regression test proposal that covers this flow, so I think we can check off that BugZero checklist and move forward 👍

[mananjadhav]

@davidcardoza This is ready for payout today and also eligible for the timeline bonus. Thank you!

[mananjadhav]

@davidcardoza Quick bump here too.

[davidcardoza]

Paid, apologies for the wait.

[mananjadhav]

Thanks @davidcardoza for the payment here. This issue is eligible for 500$ timeline bonus.

and the same amount also needs to be paid to @fedirjh for the contribution.

[mananjadhav]

Here's a summary of the payout @davidcardoza.

@fedirjh Contributor for creating the fix ($1000) + timeline bonus ($500) = Total $1500 @mananjadhav C+ Review ($1000) + timeline bonus ($500) = Total $1500

Hope this helps. Thanks once again for the help here.

[davidcardoza]

Bonuses submitted.

[fedirjh]

@davidcardoza The milestone was not yet approved , Only received the bonus

[davidcardoza]

Done

[davidcardoza]

@robertjchen This still requires a regression test update correct? Can you link me to the proposal?

[robertjchen]

@davidcardoza Yep, here are the proposed regression test steps: https://github.com/Expensify/App/issues/14570#issuecomment-1445059499

Let me know if that should do it, or if you need something more than that 👍

[davidcardoza]

Considering the issue is only affecting some account with a specific condition and it would require pre-requisites are added in Testrail by the QA team, I think we do nothing with updating testrail.

[davidcardoza]

This should be closed. I am missing something please reopen.