Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 of 6 tasks
lanitochka17 opened this issue Apr 19, 2024 · 74 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Apr 19, 2024

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:

  1. Click on FAB > and create a split bill with two new users
  2. 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
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6454834_1713538523579.Screen_Recording_2024-04-19_at_5.53.41_in_the_afternoon.mp4

View all open jobs on GitHub

Issue OwnerCurrent 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
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 19, 2024
Copy link

melvin-bot bot commented Apr 19, 2024

Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@lanitochka17
Copy link
Author

@joekaufmanexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@FitseTLT
Copy link
Contributor

FitseTLT commented Apr 19, 2024

Proposal

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

Split bill - New group is created when splitting bill with the same users

What is the root cause of that problem?

We are not checking if the group chat exists or not before creating here

App/src/libs/actions/IOU.ts

Lines 3104 to 3119 in 9b2c518

if (participants.length > 1) {
const splitChatReport = ReportUtils.buildOptimisticChatReport(
allParticipantsAccountIDs,
'',
CONST.REPORT.CHAT_TYPE.GROUP,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN,
);
return {existingSplitChatReport: null, splitChatReport};
}

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

We should check if it already exists in onyx

   if (participants.length > 1) {
        const existing = ReportUtils.getChatByParticipants(allParticipantsAccountIDs, undefined, true) ?? null;
        const splitChatReport =
            !existing &&
            ReportUtils.buildOptimisticChatReport(
                allParticipantsAccountIDs,
                '',
                CONST.REPORT.CHAT_TYPE.GROUP,
                undefined,
                undefined,
                undefined,
                undefined,
                undefined,
                undefined,
                CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN,
            );
        return {existingSplitChatReport: existing, splitChatReport: existing ?? splitChatReport};
    }

currently getChatByParticipants doesn't include group chats so we should update it to take a parameter includeGroupChats and pass as true here

function getChatByParticipants(newParticipantList: number[], reports: OnyxCollection<Report> = allReports, includeGroupChats = false): OnyxEntry<Report> {
    const sortedNewParticipantList = newParticipantList.sort();
    return (
        Object.values(reports ?? {}).find((report) => {
            // If the report has been deleted, or there are no participants (like an empty #admins room) then skip it
            if (
                !report ||
                report.participantAccountIDs?.length === 0 ||
                isChatThread(report) ||
                isTaskReport(report) ||
                isMoneyRequestReport(report) ||
                isChatRoom(report) ||
                isPolicyExpenseChat(report) ||
                (isGroupChat(report) && !includeGroupChats)

What alternative solutions did you explore? (Optional)

Optonally we can update this line of code

App/src/libs/actions/IOU.ts

Lines 3088 to 3092 in 9b2c518

// If we do not have one locally then we will search for a chat with the same participants (only for 1:1 chats).
const shouldGetOrCreateOneOneDM = participants.length < 2;
if (!existingSplitChatReport && shouldGetOrCreateOneOneDM) {
existingSplitChatReport = ReportUtils.getChatByParticipants(participantAccountIDs);
}

to

if (!existingSplitChatReport) {
        existingSplitChatReport = ReportUtils.getChatByParticipants(
            [...participantAccountIDs, ...(participantAccountIDs.length > 1 ? [currentUserAccountID] : [])],
            undefined,
            participantAccountIDs.length > 1,
        );
    }

@shahinyan11
Copy link

shahinyan11 commented Apr 19, 2024

Proposal

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

Split bill - New group is created when splitting bill with the same users

What is the root cause of that problem?

New feature

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

Add below code above this line

const existingChatWithCurrentParticipants = ReportUtils.getChatByParticipants(participantAccountIDs)

if(!existingSplitChatReportID && !!existingChatWithCurrentParticipants){
    existingSplitChatReportID = existingChatWithCurrentParticipants.reportID
}

Update this line as follows getChatByParticipants(participants)?.reportID || ''

What alternative solutions did you explore? (Optional)

@FitseTLT
Copy link
Contributor

Updated Only to add a refactored code for same solution

@joekaufmanexpensify
Copy link
Contributor

I think this report is going to be obviated by a feature refactor we're working on. Discussing.

@melvin-bot melvin-bot bot added the Overdue label Apr 22, 2024
@joekaufmanexpensify
Copy link
Contributor

Not overdue

@melvin-bot melvin-bot bot removed the Overdue label Apr 22, 2024
@joekaufmanexpensify
Copy link
Contributor

Bumped in slack

@joekaufmanexpensify
Copy link
Contributor

Bumped 1:1

@joekaufmanexpensify
Copy link
Contributor

Flagged again in Slack. This is definitely #vip-split related. But don't want to make external just yet since I feel like there is a solid chance we will close this.

@youssef-lr youssef-lr self-assigned this Apr 25, 2024
@youssef-lr
Copy link
Contributor

@joekaufmanexpensify I'll take a look at the proposals tomorrow.

@joekaufmanexpensify
Copy link
Contributor

Great. TY!

@melvin-bot melvin-bot bot added the Overdue label Apr 29, 2024
@joekaufmanexpensify
Copy link
Contributor

@youssef-lr is going to look at proposals

@melvin-bot melvin-bot bot removed the Overdue label Apr 29, 2024
@joekaufmanexpensify
Copy link
Contributor

Still pending

@youssef-lr
Copy link
Contributor

I will get to this this week

@joekaufmanexpensify
Copy link
Contributor

Great. TY!

Copy link

melvin-bot bot commented May 3, 2024

@youssef-lr @joekaufmanexpensify this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot melvin-bot bot added the Overdue label May 6, 2024
@joekaufmanexpensify
Copy link
Contributor

@youssef-lr will you have a chance to circle back to this soon? If you think we should definitely change this behavior, also happy to assign a C+ so they do preliminary review for you

@melvin-bot melvin-bot bot removed the Overdue label May 6, 2024
@joekaufmanexpensify
Copy link
Contributor

@youssef-lr making this external for now so we can start to look at the proposals. When you have a sec, could you confirm if the solution in OP of the issue is what you wanted from the design doc?

@joekaufmanexpensify joekaufmanexpensify added the External Added to denote the issue can be worked on by a contributor label May 7, 2024
@Ollyws
Copy link
Contributor

Ollyws commented Jul 9, 2024

BugZero Checklist:

  • The PR that introduced the bug has been identified. Link to the PR:

#39276

  • 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:

#39276 (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:

N/A

  • Determine if we should create a regression test for this bug.

Yes.

  • 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
Copy link
Contributor

@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
Copy link
Contributor

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.

@joekaufmanexpensify joekaufmanexpensify added Daily KSv2 and removed Weekly KSv2 labels Jul 10, 2024
@melvin-bot melvin-bot bot added Daily KSv2 and removed Daily KSv2 labels Jul 10, 2024
Copy link

melvin-bot bot commented Jul 10, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Bumped in Slack on how to proceed with this one.

@joekaufmanexpensify
Copy link
Contributor

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
Copy link
Contributor

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

@joekaufmanexpensify joekaufmanexpensify added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Jul 12, 2024
Copy link

melvin-bot bot commented Jul 12, 2024

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

@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-07-10] Split bill - New group is created when splitting bill with the same users [$250] [HOLD for payment 2024-07-10] Split bill - New group is created when splitting bill with the same users Jul 12, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 12, 2024
Copy link

melvin-bot bot commented Jul 12, 2024

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

@joekaufmanexpensify joekaufmanexpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 12, 2024
@joekaufmanexpensify
Copy link
Contributor

@FitseTLT offer sent for $250!

@joekaufmanexpensify
Copy link
Contributor

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

@FitseTLT
Copy link
Contributor

@FitseTLT offer sent for $250!

Accepted

@joekaufmanexpensify
Copy link
Contributor

@FitseTLT $250 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

Upwork job closed.

@joekaufmanexpensify
Copy link
Contributor

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

@joekaufmanexpensify
Copy link
Contributor

Thanks everyone!

@Ollyws
Copy link
Contributor

Ollyws commented Jul 13, 2024

Requested in ND.

@JmillsExpensify
Copy link

$250 approved for @Ollyws

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

9 participants