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

[HOLD for payment 2024-06-20] Group chat - Member amount is 0 when creating group chat offline #43042

Closed
4 of 6 tasks
lanitochka17 opened this issue Jun 4, 2024 · 49 comments
Closed
4 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Monthly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jun 4, 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-79-2
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go offline
  3. Go to FAB > Start chat
  4. Create a group chat with a few users
  5. Tap on the chat header

Expected Result:

Members will show the correct number of group members

Actual Result:

Members show 0 when there are some group members

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

Bug6501741_1717515811787.zero_mem.mp4

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @ShridharGoel
@lanitochka17 lanitochka17 added DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Jun 4, 2024
Copy link

melvin-bot bot commented Jun 4, 2024

Triggered auto assignment to @aldo-expensify (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link
Contributor

github-actions bot commented Jun 4, 2024

👋 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.

@lanitochka17
Copy link
Author

@aldo-expensify 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

@ShridharGoel
Copy link
Contributor

ShridharGoel commented Jun 4, 2024

Proposal

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

Group chat - Member amount is 0 when creating group chat offline

What is the root cause of that problem?

There is an extra && isSystemChat in the below condition which causes the condition to be always false when it's a group chat.

const participants = useMemo(() => {
if (isGroupChat || isSystemChat) {
// Filter out the current user from the particpants of the systemChat
return ReportUtils.getParticipantAccountIDs(report.reportID ?? '').filter((accountID) => accountID !== session?.accountID && isSystemChat);
}
return ReportUtils.getVisibleChatMemberAccountIDs(report.reportID ?? '');
}, [report, session, isGroupChat, isSystemChat]);

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

Change it to this:

const participants = useMemo(() => {
    if (isSystemChat) {
        // Filter out the current user from the participants of the systemChat
        return ReportUtils.getParticipantAccountIDs(report.reportID ?? '').filter((accountID) => accountID !== session?.accountID);
    }

    return ReportUtils.getVisibleChatMemberAccountIDs(report.reportID ?? '');
}, [report, session, isSystemChat]);

We don't need the check for isGroupChat, getVisibleChatMemberAccountIDs should be used.

@ShridharGoel
Copy link
Contributor

ShridharGoel commented Jun 4, 2024

@aldo-expensify Since this is a blocker, I can open a PR quickly. Can you have a look at the above proposal?

It is caused by #41290.

@Nodebrute
Copy link
Contributor

Nodebrute commented Jun 4, 2024

Proposal

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

Member amount is 0 when creating group chat offline

What is the root cause of that problem?

Regression caused by this PR #41290. If it's not a systemChat we are filtering out all the members. The purpose of this change was to filter current user from members list if it's a system chat

return ReportUtils.getParticipantAccountIDs(report.reportID ?? '').filter((accountID) => accountID !== session?.accountID && isSystemChat);

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

This was added to filter out current user from members list. But with this is also removing current user from group's members list too

return ReportUtils.getParticipantAccountIDs(report.reportID ?? '').filter((accountID) => accountID !== session?.accountID && isSystemChat);

So removing just isSystemChat will not work here. We can do something like this as we don't want to filter current user from member's list

   if (isGroupChat) {
           
            return ReportUtils.getParticipantAccountIDs(report.reportID ?? '');
        }
        if(isSystemChat){
             // Filter out the current user from the particpants of the systemChat
            return ReportUtils.getParticipantAccountIDs(report.reportID ?? '').filter((accountID) => accountID !== session?.accountID)
        }

What alternative solutions did you explore? (Optional)

We don't show members page if it's a system report

!isSystemChat

We can remover filtering logic here

 const participants = useMemo(() => {
        if (isGroupChat) {
           
            return ReportUtils.getParticipantAccountIDs(report.reportID ?? '');
        }

        return ReportUtils.getVisibleChatMemberAccountIDs(report.reportID ?? '');
    }, [report, session, isGroupChat, isSystemChat]);

@ShridharGoel
Copy link
Contributor

Proposal

Updated to add full function code.

@Nodebrute
Copy link
Contributor

Proposal Updated

Alternative solution added

@ShridharGoel
Copy link
Contributor

Proposal

Updated to add few more details.

@aldo-expensify
Copy link
Contributor

@francoisl I agree with @ShridharGoel on that this seems to have been cause by this change:

https://github.com/Expensify/App/pull/41290/files#diff-409b0e2fb439856e9897b808014031ce244a427d116ed30f37a2e0117599adc4R95-R97

What do you think of the proposed solution:

#43042 (comment)

@aldo-expensify
Copy link
Contributor

cc @NikkiWines ^

@aldo-expensify
Copy link
Contributor

@ShridharGoel

I don't think we should change to use getVisibleChatMemberAccountIDs for group chats since this exception was introduced by Marc here: https://github.com/Expensify/App/pull/39757/files#diff-409b0e2fb439856e9897b808014031ce244a427d116ed30f37a2e0117599adc4R84-R88, and I guess he had a reason for it.

@Nodebrute
Copy link
Contributor

@aldo-expensify You can check my proposal here #43042 (comment)

@aldo-expensify
Copy link
Contributor

Yep, that sounds like the easiest best fix. About the alternative solution, it does make sense to me, but I'm not 100% that there won't be regressions since the participants variable is used in multiple places below and I'm not familiar with the code.

@Nodebrute
Copy link
Contributor

Nodebrute commented Jun 4, 2024

@aldo-expensify You can assign this to me. I'll raise a PR with my main solution.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Hourly KSv2 labels Jun 4, 2024
@aldo-expensify aldo-expensify added the Bug Something is broken. Auto assigns a BugZero manager. label Jun 4, 2024
Copy link

melvin-bot bot commented Jun 4, 2024

Triggered auto assignment to @sonialiap (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.

@melvin-bot melvin-bot bot added the Daily KSv2 label Jun 4, 2024
@roryabraham roryabraham removed the DeployBlockerCash This issue or pull request should block deployment label Jun 6, 2024
@roryabraham
Copy link
Contributor

no longer a deploy blocker, has been fixed.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jun 11, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-06-13] Group chat - Member amount is 0 when creating group chat offline [HOLD for payment 2024-06-18] [HOLD for payment 2024-06-13] Group chat - Member amount is 0 when creating group chat offline Jun 11, 2024
Copy link

melvin-bot bot commented Jun 11, 2024

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

If no regressions arise, payment will be issued on 2024-06-18. 🎊

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

Copy link

melvin-bot bot commented Jun 11, 2024

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:

  • [@aldo-expensify] The PR that introduced the bug has been identified. Link to the PR:
  • [@aldo-expensify] 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:
  • [@aldo-expensify] 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:
  • [@ShridharGoel] Determine if we should create a regression test for this bug.
  • [@ShridharGoel] 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.
  • [@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 Weekly KSv2 and removed Weekly KSv2 Daily KSv2 labels Jun 12, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-06-18] [HOLD for payment 2024-06-13] Group chat - Member amount is 0 when creating group chat offline [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [HOLD for payment 2024-06-13] Group chat - Member amount is 0 when creating group chat offline Jun 13, 2024
Copy link

melvin-bot bot commented Jun 13, 2024

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

If no regressions arise, payment will be issued on 2024-06-20. 🎊

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

Copy link

melvin-bot bot commented Jun 13, 2024

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:

  • [@aldo-expensify] The PR that introduced the bug has been identified. Link to the PR:
  • [@aldo-expensify] 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:
  • [@aldo-expensify] 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:
  • [@ShridharGoel] Determine if we should create a regression test for this bug.
  • [@ShridharGoel] 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.
  • [@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@sonialiap sonialiap changed the title [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [HOLD for payment 2024-06-13] Group chat - Member amount is 0 when creating group chat offline [HOLD for payment 2024-06-20] Group chat - Member amount is 0 when creating group chat offline Jun 19, 2024
@sonialiap
Copy link
Contributor

sonialiap commented Jun 20, 2024

Payment summary:

Copy link

melvin-bot bot commented Jun 20, 2024

Payment Summary

Upwork Job

BugZero Checklist (@sonialiap)

  • 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

@sonialiap
Copy link
Contributor

sonialiap commented Jun 24, 2024

@ShridharGoel please accept the offer and complete the checklist steps in which you are tagged

@sonialiap
Copy link
Contributor

@ShridharGoel please accept the offer. I've also sent you a message in Upwork in case you see that first

@melvin-bot melvin-bot bot added the Overdue label Jul 3, 2024
@ShridharGoel
Copy link
Contributor

Accepted, thanks.

@melvin-bot melvin-bot bot removed the Overdue label Jul 3, 2024
@sonialiap
Copy link
Contributor

@ShridharGoel please complete the checklist

@ShridharGoel
Copy link
Contributor

@sonialiap I think that would be handled by @aldo-expensify.

@sonialiap
Copy link
Contributor

@ShridharGoel melvin assigned you to the regression part of the checklist

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Jul 29, 2024
Copy link

melvin-bot bot commented Jul 29, 2024

This issue has not been updated in over 15 days. @sonialiap, @ShridharGoel, @aldo-expensify eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@ShridharGoel
Copy link
Contributor

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

Looks like no additional test is needed, if we already have a test to check the member count when offline.

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. Engineering Monthly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants