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

[$500] Web - The name 'fake' is added as a member when replying to a statement in the workspace chat #34672

Closed
1 of 6 tasks
lanitochka17 opened this issue Jan 17, 2024 · 20 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 17, 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: 1.4.26-1
Reproducible in staging?: Y
Reproducible in production?: Y
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Go to Settings > Workspace > New Workspace
  2. Observe the workspace chat created on LHN
  3. Enter the workspace chat, write any comment, and then reply to it
  4. Click on the header > Members > Notice that the additional 'fake' account is added as a member in the workspace chat

Expected Result:

The 'fake' account should not be added as a member

Actual Result:

The 'fake' name is added as a member when replying to a statement in the workspace chat

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

Bug6345714_1705520797173.workspaceChat.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012d728b680c6b95a5
  • Upwork Job ID: 1747728578465234944
  • Last Price Increase: 2024-01-17
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 17, 2024
Copy link

melvin-bot bot commented Jan 17, 2024

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

Copy link

melvin-bot bot commented Jan 17, 2024

Job added to Upwork: https://www.upwork.com/jobs/~012d728b680c6b95a5

@melvin-bot melvin-bot bot changed the title Workspace - The name 'fake' is added as a member when replying to a statement in the workspace chat [$500] Workspace - The name 'fake' is added as a member when replying to a statement in the workspace chat Jan 17, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 17, 2024
Copy link

melvin-bot bot commented Jan 17, 2024

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

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Jan 17, 2024

Proposal

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

Workspace - The name 'fake' is added as a member when replying to a statement in the workspace chat

What is the root cause of that problem?

The main problem with this issue is that we do not filter the list of members by default
As a result, we can get members with accountID which is equal 0

https://github.com/Expensify/App/blob/main/src/libs/ReportUtils.ts#L4230-L4244

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

To fix this issue we can add filter(Boolean) for

https://github.com/Expensify/App/blob/main/src/libs/ReportUtils.ts#L4243

And since visibleChatMemberAccountID for fake is 0 it will fix the problem

What alternative solutions did you explore? (Optional)

As alternative, we can make these changes and filter the list in ReportParticipantsPage here

_.chain(ReportUtils.getVisibleMemberIDs(report))

@Tony-MK
Copy link
Contributor

Tony-MK commented Jan 18, 2024

Proposal

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

Workspace - The name 'fake' is added as a member when replying to a statement in the workspace chat.

What is the root cause of that problem?

As part of the OpenReport API response, the backend returns the report.ownerAccountID included in the report.visibleChatMemberAccountIDs array.

Response

This should only happen for the parent policy chat and not for chat threads.
That is why this doesn't happen with chats or chat threads that do not have a policy associated with it.

if (isMoneyRequestReport(report)) {

Therefore, the report.getVisibleMemberIDs function will return ownerAccountID as a part of the visibleChatMemberAccountIDs.

However, If the report was a money request, it will remove any fake members from the visibleChatMemberAccountIDs array and return only visible members with avatars.

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

This problem could be fixed at the backend, but here is a front end that would work as well and improve the report.getVisibleMemberIDs function. It will fix the number of members in the ReportDetailsPage and remove the fake member in the ReportParticipantsPage .
We will change the condition above to look something similar to the one below.

if (isMoneyRequestReport(report) || (isPolicyExpenseChat(report) && isThread(report))){

POC (macOS - Chrome)

macOS.-.Chrome.mov

@DylanDylann
Copy link
Contributor

Proposal

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

The 'fake' name is added as a member when replying to a statement in the workspace chat

What is the root cause of that problem?

This is the data returned from OpenReport API

Screenshot 2024-01-18 at 14 36 55

let's see 2 fields participantAccountIDs, visibleChatMemberAccountIDs: the 0 value caused this bug

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

We should add a filter for these fields. In here, we should update like that

function getVisibleMemberIDs(report: OnyxEntry<Report>): number[] {

 let visibleChatMemberAccountIDs = report.visibleChatMemberAccountIDs ?? [];

    // Build participants list for IOU/expense reports
    if (isMoneyRequestReport(report)) {
        visibleChatMemberAccountIDs = [report.managerID, report.ownerAccountID, ...visibleChatMemberAccountIDs]
    }
    const onlyTruthyValues = visibleChatMemberAccountIDs.filter(Boolean) as number[];
    return [...new Set([...onlyTruthyValues])];

I move the filter outside if block to dry code.
We also need to update the same thing here

App/src/libs/ReportUtils.ts

Lines 4168 to 4181 in 4da7fda

function getParticipantsIDs(report: OnyxEntry<Report>): number[] {
if (!report) {
return [];
}
const participants = report.participantAccountIDs ?? [];
// Build participants list for IOU/expense reports
if (isMoneyRequestReport(report)) {
const onlyTruthyValues = [report.managerID, report.ownerAccountID, ...participants].filter(Boolean) as number[];
const onlyUnique = [...new Set([...onlyTruthyValues])];
return onlyUnique;
}
return participants;

We also should do the same thing in here

What alternative solutions did you explore? (Optional)

NA

@melvin-bot melvin-bot bot added the Overdue label Jan 22, 2024
@situchan
Copy link
Contributor

@ZhenjaHorbach's proposal looks good to me if frontend fix is fine.

filter(Boolean) is already used here:

const onlyTruthyValues = [report.managerID, report.ownerAccountID, ...visibleChatMemberAccountIDs].filter(Boolean) as number[];

So we can apply same to non money request reports as well.

Question for assigned engineer: I'd like to confirm why backend returns 0 in participantAccountIDs and if it's intentional.

🎀 👀 🎀 C+ reviewed

@melvin-bot melvin-bot bot removed the Overdue label Jan 23, 2024
Copy link

melvin-bot bot commented Jan 23, 2024

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

@kbecciv kbecciv changed the title [$500] Workspace - The name 'fake' is added as a member when replying to a statement in the workspace chat [$500] Web - The name 'fake' is added as a member when replying to a statement in the workspace chat Jan 26, 2024
@melvin-bot melvin-bot bot added the Overdue label Jan 26, 2024
@iwiznia
Copy link
Contributor

iwiznia commented Jan 26, 2024

main/src/libs/ReportUtils.ts#L4230-L4244

I think this link is wrong? Always use permalinks to code, otherwise it is very possible the link is wrong

I'd like to confirm why backend returns 0 in participantAccountIDs and if it's intentional.

Hmmm I think it's a mistake. Why would we want to do that?

@melvin-bot melvin-bot bot removed the Overdue label Jan 26, 2024
@iwiznia
Copy link
Contributor

iwiznia commented Jan 26, 2024

Taking this internal

@iwiznia iwiznia added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Jan 26, 2024
Copy link

melvin-bot bot commented Jan 26, 2024

Current assignee @situchan is eligible for the Internal assigner, not assigning anyone new.

@iwiznia iwiznia added the Reviewing Has a PR in review label Jan 26, 2024
Copy link

melvin-bot bot commented Feb 5, 2024

@iwiznia, @johncschuster Eep! 4 days overdue now. Issues have feelings too...

@johncschuster
Copy link
Contributor

Chill out, Melvin. It looks like @iwiznia has a fix in place.

Copy link

melvin-bot bot commented Feb 13, 2024

@iwiznia, @johncschuster Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Feb 15, 2024

@iwiznia, @johncschuster Eep! 4 days overdue now. Issues have feelings too...

Copy link

melvin-bot bot commented Feb 19, 2024

@iwiznia, @johncschuster Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

@iwiznia
Copy link
Contributor

iwiznia commented Feb 19, 2024

Oh this is allegedly done, @lanitochka17 can you please re-test and check if the issue still exists and if not, close the issue?

Copy link

melvin-bot bot commented Feb 27, 2024

@iwiznia, @johncschuster Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@iwiznia
Copy link
Contributor

iwiznia commented Feb 27, 2024

Bump @lanitochka17 #34672 (comment)

@lanitochka17
Copy link
Author

@iwiznia Hello Issue is not reproducible now

notRep.mp4

@iwiznia iwiznia closed this as completed Feb 28, 2024
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. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants