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] Expense - Missing Members section in expense report details page #40541

Closed
6 tasks done
izarutskaya opened this issue Apr 19, 2024 · 37 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering 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 Monthly KSv2

Comments

@izarutskaya
Copy link

izarutskaya commented Apr 19, 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.63-6
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace chat as employee.
  3. Create a manual expense.
  4. Go to expense report.
  5. Click on the report header.
  6. As Admin, repeat Step 4 and 5 with employee workspace chat.

Expected Result:

Members section will show up in expense report details page (production behavior).

Actual Result:

Members section is missng in expense report details page. It only shows up when one of the members sends a message in expense report.

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

Bug6454441_1713495046974.20240419_104607.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01653d31f427db89ae
  • Upwork Job ID: 1781308954716004352
  • Last Price Increase: 2024-04-19
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment 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 @sakluger (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.

Copy link

melvin-bot bot commented Apr 19, 2024

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

Copy link
Contributor

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

@izarutskaya
Copy link
Author

@sakluger 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.

@izarutskaya
Copy link
Author

We think this issue might be related to the #collect project.

@izarutskaya
Copy link
Author

Production

bandicam.2024-04-19.08-31-07-688.1.mp4

@lakchote lakchote added 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 Apr 19, 2024
@melvin-bot melvin-bot bot changed the title Expense - Missing Members section in expense report details page [$250] Expense - Missing Members section in expense report details page Apr 19, 2024
Copy link

melvin-bot bot commented Apr 19, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01653d31f427db89ae

Copy link

melvin-bot bot commented Apr 19, 2024

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

@lakchote
Copy link
Contributor

Not a blocker per se, but definitely something we want to handle.

@lakchote lakchote added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Apr 19, 2024
@shahinyan11
Copy link

shahinyan11 commented Apr 19, 2024

Proposal

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

Expense - Missing Members section in expense report details page

What is the root cause of that problem?

Here we take all participants only for groupChat reports in another case we take only visible participants. And here we check participants count to show Members section. Turns out if the current report is not groupChat and there is not visible participants we hide Members section

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

Take all participants also if report's parentReport is policyExpenseChat. To do that update this part of code like below

const isGroupChat = useMemo(() => ReportUtils.isGroupChat(report), [report]);
const isParentReportPolicyExpenseChat  = useMemo(() => ReportUtils.isPolicyExpenseChat(ReportUtils.getReport(report.parentReportID)), [report]);
const participants = useMemo(() => {
    if (isGroupChat || isParentReportPolicyExpenseChat) {
        return ReportUtils.getParticipantAccountIDs(report.reportID ?? '');
    }

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

What alternative solutions did you explore? (Optional)

@gijoe0295
Copy link
Contributor

Proposal

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

Members section is missng in expense report details page. It only shows up when one of the members sends a message in expense report.

What is the root cause of that problem?

We did not get the correct participants for expense report:

if (isGroupChat) {
return ReportUtils.getParticipantAccountIDs(report.reportID ?? '');
}

which eventually failed this condition:

(!isUserCreatedPolicyRoom && participants.length) ||

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

Add a check to the below:

if (isGroupChat || ReportUtils.isExpenseReport(report)) {
    return ReportUtils.getParticipantAccountIDs(report.reportID ?? '');
}

Also in ReportParticipantsPage, we need to update the same logic:

const chatParticipants = isGroupChat ? ReportUtils.getParticipantAccountIDs(report.reportID) : ReportUtils.getVisibleChatMemberAccountIDs(report.reportID);

What alternative solutions did you explore? (Optional)

NA

@shahinyan11
Copy link

Proposal

Updated. Fixed minor omission in proposal

@lakchote
Copy link
Contributor

@sakluger I'll be OOO next week until April 29th. Feel free to assign another internal engineer to review if needed.

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

Julesssss commented Apr 26, 2024

Hey @s77rt could you clarify why the proposals aren't suitable in this case? I know that a backend solution is preferable in most cases, but it looks like isParentReportPolicyExpenseChat or ReportUtils.isExpenseReport(report) are reasonable conditions to add in this case.

@Julesssss
Copy link
Contributor

Also I'm struggling to understand the bug report. So 'Fridayyy' is the workspace, and we are creating a workspace expense report (which should include both the admin and user)?

@s77rt
Copy link
Contributor

s77rt commented Apr 26, 2024

@Julesssss Displaying hidden members contradicts the purpose of having them hidden in the first place. I'm not seeing any recent frontend change in this area. It's probably a recent backend change that broke this.

So 'Fridayyy' is the workspace, and we are creating a workspace expense report (which should include both the admin and user)?

Yes, that's correct. However the members are set as hidden. Can you please check if that change was recent?

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

Thanks, I see now. After reviewing all recent backend PRs I located one which may be related. I'm asking for their thoughts on this bug 🤞

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

The first one was unrelated, but I have found one more that might be the cause🤞

@sakluger
Copy link
Contributor

sakluger commented May 2, 2024

@Julesssss any updates?

@Julesssss
Copy link
Contributor

It was also not the cause unfortunately.

Rodrigo provided this thought as an alternative to this being a backend issue. But I don't have time to further triage either suggestion. I think we'll have to hold this for now as it's just a polish item.

I think it is more related to adopting the new participants property in the FE. Not sure how we did before, but the visibility of participants is calculated by checking if the notification preferences is HIDDEN, and as far as I know the notification preference for the participants in expense reports is HIDDEN until something happens.

@sakluger sakluger added Weekly KSv2 and removed Daily KSv2 labels May 3, 2024
@sakluger
Copy link
Contributor

sakluger commented May 3, 2024

Okay, I'll change this to weekly for now.

Copy link

melvin-bot bot commented May 3, 2024

@sakluger @Julesssss @s77rt this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? 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 13, 2024
@sakluger
Copy link
Contributor

@Julesssss checking in again, what's the latest here?

@melvin-bot melvin-bot bot removed the Overdue label May 14, 2024
@Julesssss
Copy link
Contributor

I have no further ideas about the regression. I'm afraid that it's very unlikely I'll be able to prioritise this in the next month or so.

@sakluger
Copy link
Contributor

Okay, let's switch to monthly.

@sakluger sakluger added Monthly KSv2 and removed Weekly KSv2 labels May 16, 2024
Copy link

melvin-bot bot commented May 17, 2024

@sakluger @Julesssss @s77rt this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@Julesssss
Copy link
Contributor

I don't think there's much we can do to speed this along. If this is still occurring, we'll have to wait for other internal engineering tasks to calm down a bit.

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot added the Overdue label Jun 20, 2024
@s77rt
Copy link
Contributor

s77rt commented Jun 20, 2024

Should we make this Internal?

@melvin-bot melvin-bot bot removed the Overdue label Jun 20, 2024
@Julesssss
Copy link
Contributor

This issue is not going to be prioritised internally, so I am closing it.

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. Engineering 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 Monthly KSv2
Projects
No open projects
Archived in project
Development

No branches or pull requests

9 participants