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

[Awaiting Payment 26th Aug, 2024] [$250] Expense - Incorrect member count in report details after commenting or inviting another member #46660

Closed
6 tasks done
lanitochka17 opened this issue Aug 1, 2024 · 17 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 1, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.15-0
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/4784292
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Submit an expense to User B
  3. Click on the expense preview in the main chat to go to expense report
  4. Send a user mention containing User C
  5. Click Invite from the whisper
  6. Click on the report header
  7. Click Members

Expected Result:

In Step 6, the member count will update correctly

Actual Result:

In Step 6, the member count shows 1 member, when there are three members in the report (Step 7)
This issue also happens when user comments on the expense report without inviting a third user

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

Bug6557055_1722323368219.20240730_150250.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ca5b97965eb72787
  • Upwork Job ID: 1819133324859524397
  • Last Price Increase: 2024-08-01
  • Automatic offers:
    • suneox | Reviewer | 103398512
    • FitseTLT | Contributor | 103398514
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 1, 2024
Copy link

melvin-bot bot commented Aug 1, 2024

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

@trjExpensify 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 Aug 1, 2024

Proposal

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

member count doesn't agree with the member list count

What is the root cause of that problem?

The criteria to hide a hidden participant is not the same in details page

const shouldExcludeHiddenParticipants = !isGroupChat && !isSystemChat;
return ReportUtils.getParticipantsAccountIDsForDisplay(report, shouldExcludeHiddenParticipants);

and in participants page

const shouldExcludeHiddenParticipants = !isGroupChat && !isIOUReport;
const chatParticipants = ReportUtils.getParticipantsAccountIDsForDisplay(report, shouldExcludeHiddenParticipants);

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

The members menu item navigates to two page: participants and members page based on the report type as in here

if (isUserCreatedPolicyRoom || isChatThread || (isPolicyExpenseChat && isPolicyAdmin)) {
Navigation.navigate(ROUTES.ROOM_MEMBERS.getRoute(report?.reportID ?? '-1'));
} else {
Navigation.navigate(ROUTES.REPORT_PARTICIPANTS.getRoute(report?.reportID ?? '-1'));
}
},
});

What we should do is the members menu count shown in the count should be the length of the members list shown in participants or members page so we should make those values agree.

  1. In participants page the participants are filtered based on shouldExcludeHiddenParticipants value set here

const shouldExcludeHiddenParticipants = !isGroupChat && !isIOUReport;
const chatParticipants = ReportUtils.getParticipantsAccountIDsForDisplay(report, shouldExcludeHiddenParticipants);

and if the detail doesn't exist
const details = personalDetails?.[accountID];
if (!details) {
Log.hmmm(`[ReportParticipantsPage] no personal details found for Group chat member with accountID: ${accountID}`);
return;

2. In members page the should ExcludeHidden is true
const participants = ReportUtils.getParticipantsAccountIDsForDisplay(report, true);

and also we filter out optimistic duplicate accountID or the detail doesn't exist here
const isDuplicateOptimisticDetail = details?.isOptimisticPersonalDetail && participants.some((accID) => accID !== accountID && details.login === personalDetails[accID]?.login);
if (!details || isDuplicateOptimisticDetail) {
Log.hmmm(`[RoomMembersPage] no personal details found for room member with accountID: ${accountID}`);
return;

So we calculate the members count based on these conditions
So in ReportDetails Page we use this code

const participants = useMemo(() => {
        const shouldExcludeHiddenParticipants = isUserCreatedPolicyRoom || isChatThread || (isPolicyExpenseChat && isPolicyAdmin) ? true : !isGroupChat && !isIOUReport;
        return ReportUtils.getParticipantsAccountIDsForDisplay(report, shouldExcludeHiddenParticipants);
    }, [report, isGroupChat, isSystemChat]);
const activeChatMembers = participants.flatMap((accountID) => {
        const details = personalDetails?.[accountID];
        const isDuplicateOptimisticDetail = details?.isOptimisticPersonalDetail && participants.some((accID) => accID !== accountID && details.login === personalDetails?.[accID]?.login);

        const pendingMember = report?.pendingChatMembers?.findLast((member) => member.accountID === accountID.toString());
        return !!details && !isDuplicateOptimisticDetail && (!pendingMember || pendingMember.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) ? accountID : [];
    });

We can leave out the pendingMember condition to make it more compatible with list shown in members page and also we can use a new variable in place of participants we need to keep the previous variable

What alternative solutions did you explore? (Optional)

@trjExpensify
Copy link
Contributor

Sending external, but I'd like @puneetlath eyes on this as it's related to hidden members behaviour.

@trjExpensify trjExpensify added the External Added to denote the issue can be worked on by a contributor label Aug 1, 2024
Copy link

melvin-bot bot commented Aug 1, 2024

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

@melvin-bot melvin-bot bot changed the title Expense - Incorrect member count in report details after commenting or inviting another member [$250] Expense - Incorrect member count in report details after commenting or inviting another member Aug 1, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 1, 2024
Copy link

melvin-bot bot commented Aug 1, 2024

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

@daledah
Copy link
Contributor

daledah commented Aug 2, 2024

Proposal

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

In Step 6, the member count shows 1 member, when there are three members in the report (Step 7)
This issue also happens when user comments on the expense report without inviting a third user

What is the root cause of that problem?

We already fix the missing members issue in ReportParticipantPage in PR
but does not fix it in ReportDetailsPage

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

We should apply the same fix in:

const shouldExcludeHiddenParticipants = !isGroupChat && !isSystemChat;

const shouldExcludeHiddenParticipants = !isGroupChat && !isSystemChat && !isIOUReport;

What alternative solutions did you explore? (Optional)

Copy link
Contributor

github-actions bot commented Aug 2, 2024

true

@daledah
Copy link
Contributor

daledah commented Aug 2, 2024

Updated proposal to fix minor typo issue

@suneox
Copy link
Contributor

suneox commented Aug 2, 2024

@daledah proposal only updates the condition to get participants based on ReportParticipantsPage, but on ReportDetailsPage we display active members for both ReportParticipantsPage and RoomMembersPage.
Therefore, @FitseTLT proposal which retrieves active members based on both linked pages will be reasonable in this case.

So we can go with @FitseTLT this proposal.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 2, 2024

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

@melvin-bot melvin-bot bot added Overdue and removed Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 5, 2024
Copy link

melvin-bot bot commented Aug 5, 2024

📣 @suneox 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Aug 5, 2024

📣 @FitseTLT 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

@techievivek
Copy link
Contributor

Not overdue, job is assigned to @FitseTLT

@melvin-bot melvin-bot bot removed the Overdue label Aug 5, 2024
@suneox
Copy link
Contributor

suneox commented Aug 7, 2024

@FitseTLT Could you please update the ETA?

@FitseTLT
Copy link
Contributor

FitseTLT commented Aug 7, 2024

PR will be ready tomorrow 👍

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 11, 2024
@trjExpensify trjExpensify changed the title [$250] Expense - Incorrect member count in report details after commenting or inviting another member [Awaiting Payment 26th Aug, 2024] [$250] Expense - Incorrect member count in report details after commenting or inviting another member Aug 26, 2024
@trjExpensify
Copy link
Contributor

PR hit prod 7 days ago, automation failed.

Payment summary as follows:

Paid you both, closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Status: Done
Development

No branches or pull requests

6 participants