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] chat thread report header is incorrectly showing the "root" report in second line rather than the "parent" report. #27986

Closed
JmillsExpensify opened this issue Sep 22, 2023 · 28 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@JmillsExpensify
Copy link

JmillsExpensify commented Sep 22, 2023

Reproduction steps

  1. Comment in a parent report (e.g. root report)
  2. Create a thread on your comment in the parent/root (e.g. thread1)
Screenshot 2023-09-22 at 14 09 58 3. Confirm that in the header for thread1 shows the name of the parent/root report. Screenshot 2023-09-22 at 14 10 29 4. Now create another thread on your comment in thread1 (e.g. thread2) Screenshot 2023-09-22 at 14 14 24 5. Notice that the header of thread2 incorrectly shows the root report in the second line of the header, which is incorrect. The second line should instead show thread1, which is the parent of thread2.
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014565dfd91b80271a
  • Upwork Job ID: 1706507483408605184
  • Last Price Increase: 2023-10-17
@JmillsExpensify JmillsExpensify added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 22, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 22, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 22, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@JmillsExpensify
Copy link
Author

Originally reported internally in Slack.

@dukenv0307
Copy link
Contributor

dukenv0307 commented Sep 22, 2023

Proposal

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

chat thread report header is incorrectly showing the "root" report in second line rather than the "parent" report.

What is the root cause of that problem?

In getRootReportAndWorkspaceName function, we have an infinity loop here to get the root parent report name of the thread report

App/src/libs/ReportUtils.js

Lines 1743 to 1746 in 8e2edbf

if (isChildReport(report) && !isMoneyRequestReport(report) && !isTaskReport(report)) {
const parentReport = lodashGet(allReports, [`${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID}`]);
return getRootReportAndWorkspaceName(parentReport);
}

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

We should remove this infinity loop to get the name of the previous parent report of the thread.

App/src/libs/ReportUtils.js

Lines 1743 to 1746 in 8e2edbf

if (isChildReport(report) && !isMoneyRequestReport(report) && !isTaskReport(report)) {
const parentReport = lodashGet(allReports, [`${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID}`]);
return getRootReportAndWorkspaceName(parentReport);
}

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Sep 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 25, 2023

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

@kevinksullivan kevinksullivan added the External Added to denote the issue can be worked on by a contributor label Sep 26, 2023
@melvin-bot melvin-bot bot changed the title chat thread report header is incorrectly showing the "root" report in second line rather than the "parent" report. [$500] chat thread report header is incorrectly showing the "root" report in second line rather than the "parent" report. Sep 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 26, 2023

Job added to Upwork: https://www.upwork.com/jobs/~014565dfd91b80271a

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 26, 2023

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

@kevinksullivan
Copy link
Contributor

Just checked in on the thread and moving this forward

@melvin-bot melvin-bot bot removed the Overdue label Sep 26, 2023
@parasharrajat
Copy link
Member

This seems like expected behavior. Did we confirm this internally @kevinksullivan? if so, What should be shown in the subtitle if the parent report is a thread?

@melvin-bot melvin-bot bot added the Overdue label Sep 29, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

@kevinksullivan, @parasharrajat Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

@kevinksullivan, @parasharrajat 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@melvin-bot
Copy link

melvin-bot bot commented Oct 6, 2023

@kevinksullivan @parasharrajat 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
Copy link

melvin-bot bot commented Oct 6, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 10, 2023

@kevinksullivan, @parasharrajat 12 days overdue now... This issue's end is nigh!

@melvin-bot
Copy link

melvin-bot bot commented Oct 10, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot
Copy link

melvin-bot bot commented Oct 13, 2023

@kevinksullivan @parasharrajat this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

@melvin-bot melvin-bot bot added Engineering Weekly KSv2 and removed Daily KSv2 labels Oct 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 13, 2023

This issue has not been updated in over 14 days. @kevinksullivan, @parasharrajat eroding to Weekly issue.

@melvin-bot melvin-bot bot removed the Overdue label Oct 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@parasharrajat
Copy link
Member

Checking the only proposal in two hours. Looks like I missed it.

@parasharrajat
Copy link
Member

parasharrajat commented Oct 17, 2023

This feels like expected behavior as per #21092. Has anything changed? cc: @chiragsalian @JmillsExpensify @grgia

It seems there was some discussion here which I can't access.

@chiragsalian
Copy link
Contributor

Jason or georgia can correct me but i think this is the current expected behavior i.e., the subheader shows root report name or parent expense/IOU report name, whichever comes first while traversing through parents. The same logic is applied for DM, group DMs and policy rooms.

If we want to change it, its a new feature request and IMO we would want to predesign it to land on a consistent design across DM, group DMs and policy rooms.

@melvin-bot
Copy link

melvin-bot bot commented Oct 20, 2023

@kevinksullivan @parasharrajat this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

  • Decide whether any proposals currently meet our guidelines and can be approved as-is today
  • If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@melvin-bot melvin-bot bot added Daily KSv2 Internal Requires API changes or must be handled by Expensify staff and removed Weekly KSv2 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 Oct 20, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 20, 2023

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

@parasharrajat
Copy link
Member

@JmillsExpensify Any comments on #27986 (comment)

@melvin-bot melvin-bot bot added the Overdue label Oct 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 24, 2023

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

@parasharrajat
Copy link
Member

@JmillsExpensify @kevinksullivan Do you have any comments on #27986 (comment)?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 24, 2023
@kevinksullivan
Copy link
Contributor

Agree with @chiragsalian here

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 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests

5 participants