[$250] [HOLD for payment 2024-07-10] Split bill - New group is created when splitting bill with the same users

lanitochka17 commented 4 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: 1.4.63-7 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4496719 Email or phone of affected tester (no customers): natnael.expensify+3@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

Click on FAB > and create a split bill with two new users
Click on FAB > and create another split bill the same two users you have created with on step one

Expected Result:

New group shouldn't be created, and the split bill should be added to the existing group

Actual Result:

A second, new group is created

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/b4527362-dcdf-4103-b3f0-a566fba97660

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01c9e84f7fad619cd5
Upwork Job ID: 1811859168529416327
Last Price Increase: 2024-07-12

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.3-7 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/42444

If no regressions arise, payment will be issued on 2024-07-10. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@Ollyws requires payment through NewDot Manual Requests
@FitseTLT requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 2 months ago

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:

[x] [@Ollyws] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/39276
[x] [@Ollyws] 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: https://github.com/Expensify/App/pull/39276#discussion_r1670474105
[x] [@Ollyws] 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: N/A
[x] [@Ollyws] Determine if we should create a regression test for this bug.
[x] [@Ollyws] 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.
[x] [@joekaufmanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: Going to hold off on creating a regression test here, as we aren't sure we want to keep this behavior long term. Split is also paused, so no need for Applause to test for this everytime now.

joekaufmanexpensify commented 2 months ago

@Ollyws mind tackling the checklist here so we can prep to issue payment?

Ollyws commented 2 months ago

BugZero Checklist:

[x] The PR that introduced the bug has been identified. Link to the PR:

https://github.com/Expensify/App/pull/39276

[x] 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:

https://github.com/Expensify/App/pull/39276#discussion_r1670474105

[x] 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:

N/A

[x] Determine if we should create a regression test for this bug.

Yes.

[x] 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.

Regression Test Proposal

1. Click on FAB > split expense
2. In participants list, choose two users with which you have an already existing group chat with
confirm the split expense
3. Ensure that New group isn't created but only the split bill is added to the existing group chat

Do we agree 👍 or 👎

marcaaron commented 2 months ago

@joekaufmanexpensify Hmm sorry, this is actually the expected behavior and not a bug. This issue seems like a miscommunication.

@youssef-lr I think we should revert the PRs that "fixed" this?

Where was it decided that we would change this behavior?

I think it's going to be quite weird since if you happen to have the chat locally it would use the "existing group", but if you don't (e.g. in focus mode or soon pagination) then we will create a "new" group. Basically, all FAB flows create a new group always. If you want to split with the same people then you split via the + in the composer on the chat itself.

joekaufmanexpensify commented 2 months ago

Hey @marcaaron, the slack thread where it was discussed is a bit higher up in the issue, also here. The consensus from those working on the uneven splits project at the time was that we should change this behavior. If we no longer agree with that and want to revert it, no issues with me.

but if you don't (e.g. in focus mode or soon pagination) then we will create a "new" group. Basically, all FAB flows create a new group always. If you want to split with the same people then you split via the + in the composer on the chat itself.

I don't think this piece was considered at the time. My 2c is if I have an existing chat with the exact same group, I would personally expect a split from FAB to go in that chat, whether or not you happen to have the chat locally. But if the standard for FAB actions is to consistently create a new group always, then makes sense to me that we'd keep the actions consistent.

melvin-bot[bot] commented 2 months ago

Payment Summary

[Upwork Job]()

Reviewer: @Ollyws owed $250 via NewDot
ROLE: @FitseTLT paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@joekaufmanexpensify)

[ ] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
[ ] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
[ ] I have paid out the Upwork contracts or cancelled the ones that are incorrect
[ ] I have verified the payment summary above is correct

marcaaron commented 2 months ago

Ok thanks, sorry, I missed that discussion and those links.

I would personally expect a split from FAB to go in that chat, whether or not you happen to have the chat locally. But if the standard for FAB actions is to consistently create a new group always, then makes sense to me that we'd keep the actions consistent.

Yeah, I think this is the part that is confusing for me? If we are OK with having that inconsistency then that's fine. I think we fundamentally changed (or maybe "broke") our previous design with Group Chats. My 2 cents, if we want to give users the ability to split with an existing group then we should return Groups in the list of available options to split with and not auto select or create a new group depending on whether that group exists locally (which may not exist due to varying factors outside of the control of the user).

joekaufmanexpensify commented 2 months ago

That's fair! I see the argument of doing as you suggest, as it does seem a bit odd to have this one option with groups behave differently from every other option on FAB. I think that kinda matches my initial thoughts on this in OP of that slack thread too. But I am not super close to this project, so deferred to those working on the project.

@arielgreen / @youssef-lr what do you both think?

joekaufmanexpensify commented 2 months ago

Bumped in Slack on how to proceed with this one.

joekaufmanexpensify commented 2 months ago

We landed on leaving this as is in Slack. We may change this again in the future, but this project is paused now, so no need to do that now.

joekaufmanexpensify commented 2 months ago

Checklist is all set. Good to issue payment. We need to pay:

@FitseTLT - $250 for PR via Upwork.
@Ollyws - $250 for C+ via NewDot.

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

joekaufmanexpensify commented 2 months ago

@FitseTLT offer sent for $250!

joekaufmanexpensify commented 2 months ago

@Ollyws please request $250 via NewDot whenever you're ready!

FitseTLT commented 2 months ago

@FitseTLT offer sent for $250!

Accepted

joekaufmanexpensify commented 2 months ago

@FitseTLT $250 sent and contract ended!

joekaufmanexpensify commented 2 months ago

Upwork job closed.

joekaufmanexpensify commented 2 months ago

Closing for now, as only thing left here is for @Ollyws to request their payment. Payment summary is here as FYI.

joekaufmanexpensify commented 2 months ago

Thanks everyone!

Ollyws commented 2 months ago

Requested in ND.

JmillsExpensify commented 1 month ago

$250 approved for @Ollyws

Expensify / App