[HOLD for payment 2024-03-07] [$500] Wrong header is shown for 2nd and 3rd level of child threads

[m-natarajan]

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.43-13
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: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1708622339587649

Action Performed:

1. Post a message 1 in a room as account A
2. Create a thread message (messsage 2) for message 1
3. Create another 2nd level thread message from message 2
4. Go to thread of message2
5. Look at the header

Expected Result:

Header should show only the thread which is 1 level up

Actual Result:

Shows "room name" and clicking on that takes to 1 level up

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

Recording.2767.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d99f7344fe2e0a2c
• Upwork Job ID: 1760784445477494784
• Last Price Increase: 2024-02-22
• Automatic offers:
• paultsimura | Reviewer | 0
• barros001 | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[barros001]

Proposal

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

Wrong header is displayed when you navigate into a thread. It also shows the room as being the parent.

What is the root cause of that problem?

App/src/libs/ReportUtils.ts

Lines 2639 to 2650 in 0a2ece8

	function getParentNavigationSubtitle(report: OnyxEntry<Report>): ParentNavigationSummaryParams {
	if (isThread(report)) {
	const parentReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${report?.parentReportID}`] ?? null;
	const {rootReportName, workspaceName} = getRootReportAndWorkspaceName(parentReport);
	if (!rootReportName) {
	return {};
	}
	return {rootReportName, workspaceName};
	}
	return {};
	}

getParentNavigationSubtitle will call getRootReportAndWorkspaceName to get the report name when handling a thread report.

App/src/libs/ReportUtils.ts

Lines 2590 to 2593 in 0a2ece8

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

getRootReportAndWorkspaceName in turn will navigate up all the way to the root report and return it's name instead. Note how we already check for isTaskReport and isMoneyRequestReport but not isChatReport.

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

Add a check for isChatReport in getRootReportAndWorkspaceName and return the correct parent report name

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

Screen.Recording.2024-02-22.at.5.28.40.PM.mov

What alternative solutions did you explore? (Optional)

Get rid of getRootReportAndWorkspaceName as it appears the only scenario where we would navigate up to the root is when we're dealing with chats, and according to this issue, we might not want to do that even for chats.

if (isChildReport(report) && !isMoneyRequestReport(report) && !isTaskReport(report) && !isChatReport(report)) {}

The condition above (as proposed) seems to always return false now that we added chat to the list. isMoneyRequestReport, isTaskReport and isChatReport covers all 4 types of reports:

        TYPE: {
            CHAT: 'chat',
            EXPENSE: 'expense',
            IOU: 'iou',
            TASK: 'task',
        },

So unless I'm missing something, we can rename getRootReportAndWorkspaceName to getReportAndWorkspaceName and remove the part that it navigates to the root report. But again, I might be missing a scenario where we actually want to get to the root.

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[paultsimura]

I see that @chiragsalian once mentioned that it's an expected behavior: #27986 (comment)

@puneetlath @chiragsalian did the expected behavior change, and we should proceed with the proposals review here?

[dragnoir]

Proposal

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

Wrong header title shown in chat threads

What is the root cause of that problem?

On the HeaderView component, we are using the parentNavigationSubtitleData to get the title of the parent displayed after From...

App/src/pages/home/HeaderView.js

Line 108 in cd14830

const parentNavigationSubtitleData = ReportUtils.getParentNavigationSubtitle(reportHeaderData);

but this function getParentNavigationSubtitle is using getRootReportAndWorkspaceName(parentReport); to turn the title of the parent report and this is wrong.

App/src/libs/ReportUtils.ts

Lines 2639 to 2643 in cd14830

	function getParentNavigationSubtitle(report: OnyxEntry<Report>): ParentNavigationSummaryParams {
	if (isThread(report)) {
	const parentReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${report?.parentReportID}`] ?? null;
	const {rootReportName, workspaceName} = getRootReportAndWorkspaceName(parentReport);
	if (!rootReportName) {

As you can see from the name of the function getRootReportAndWorkspaceName, the word ROOT is about getting details from the root, not the parent.

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

We need to get the title from the parent and not from the root. We can use

const  parentReportName  =  getReportName(parentReport);

This will turn the title of the parent and not the title of the root.

POC video:

20240223_135951.mp4

What alternative solutions did you explore? (Optional)

I can see that the function getParentNavigationSubtitle from ReportUtils is used by other components. I think it's better to create two fucntions, one to get Parent Title and a second for the root title and then adjust components depending on the need (root/parent).

[paultsimura]

Not overdue. Waiting for a decision from @puneetlath and @chiragsalian. Also asked in Slack.

[paultsimura]

We've agreed on the expected behavior here: we should show the parent report (comment) title in the navigation.

[paultsimura]

@dragnoir thanks for your proposal. It's essentially not different from the proposal from @barros001. Your alternative solution doesn't make much sense because we want the navigation subtitle to be consistent across the places it's used: report headers and the "share code" page (here only the workspace name is used).

[dragnoir]

@paultsimura the proposal from @barros001 will change a main function with the goal to return a root. Mine is not touching this function and keep it always turn a root.

[paultsimura]

@paultsimura the proposal from @barros001 will change a main function with the goal to return a root. Mine is not touching this function and keep it always turn a root.

The getRootReportAndWorkspaceName function is used in a single place – to build the navigation subtitle. Outside of it, there's no use for this specific logic and returned value, so we can safely modify it.

[paultsimura]

Eventually, I think we can get rid of the getRootReportAndWorkspaceName function completely and calculate the ParentNavigationSummaryParams solely in the getParentNavigationSubtitle function.

Since @barros001 was the first to give the correct RCA and the correct fix (removing the recursion) – we can proceed with his proposal.
🎀👀🎀 C+ reviewed

@barros001 just a friendly note for you: I see your proposal was modified a ton of times. Since all the edits were made before other proposals were posted, it's OK, but in such a case, I would recommend just copying your final proposal, posting it again, and removing the edited one. This way, you're not losing the priority since there are no other proposals yet, but it looks much cleaner.

[melvin-bot]

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

[dragnoir]

Eventually, I think we can get rid of the getRootReportAndWorkspaceName function completely and calculate the ParentNavigationSummaryParams solely in the getParentNavigationSubtitle function.

This was my alternative solution.

I respect your decision. But updating a root calling function to get the parent is not something professional at all. Another developer in the future may be confused with the name. Also asking if it's a chat to stop the loop inside a root function is not helpful for an utility that can be used in the future.

[paultsimura]

This was my alternative solution.

Respectfully, it wasn't. Yours was to have 2 functions and adjust different components to use both of these functions.

updating a root calling function to get the parent is not something professional at all. Another developer in the future may be confused with the name.

Could you please elaborate on why it's not professional, and how one should be confused with the getParentNavigationSubtitle name?

[dragnoir]

I can see that the function getParentNavigationSubtitle from ReportUtils is used by other components. I think it's better to create two fucntions, one to get Parent Title and a second for the root title and then adjust components depending on the need (root/parent).

the function getParentNavigationSubtitle is already used by other component. I suggested to update it as it's a call for the root and adjust the components that already are using it.

I also suggested having as final result a function that is for root and a second for parent. This will give make a good use for utility function. Also it will be easy for developers iin the future to use those function clearly.

What @barros001 was suggesting is:

So unless I'm missing something, we can rename getRootReportAndWorkspaceName to getReportAndWorkspaceName and remove the part that it navigates to the root report. But again, I might be missing a scenario where we actually want to get to the root.

I don't see any idea about having two seperate clean functions.

[barros001]

@barros001 just a friendly note for you: I see your proposal was modified a ton of times. Since all the edits were made before other proposals were posted, it's OK, but in such a case, I would recommend just copying your final proposal, posting it again, and removing the edited one. This way, you're not losing the priority since there are no other proposals yet, but it looks much cleaner.

Sorry about that, I'll keep this in mind next time. I'm ware of the speed run proposals issue we're dealing with and I assure you that wasn't my idea here. I try to make the initial proposal as complete as possible but a lot of times I keep digging and end up finding little gaps on the proposal and go back and fix, or I just think of adding more context that I did not think at first. I'll make an effort to keep my edits to a minimum.

[melvin-bot]

📣 @paultsimura 🎉 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

[melvin-bot]

📣 @barros001 🎉 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 📖

[paultsimura]

I'm ware of the speed run proposals issue we're dealing with and I assure you that wasn't my idea here.

I understand that, no worries. That was just some advice 👍

[paultsimura]

the function getParentNavigationSubtitle is already used by other component. I suggested to update it as it's a call for the root and adjust the components that already are using it.

I also suggested having as final result a function that is for root and a second for parent. This will give make a good use for utility function. Also it will be easy for developers iin the future to use those function clearly.

I understand your way of thinking. But the form in which you presented your proposal did not have any evident benefits over the other proposal.
My advice for you is to not post vague proposals, but make them more refined.
You could initially state that:

• Although getParentNavigationSubtitle is used in different components, it always serves the same purpose, and therefore we are safe to modify it however we want;
• Apparently, the getRootReportAndWorkspaceName was built only for the sake of recursion, and now that we want to get rid of the recursion, we can move all the calculations to the getParentNavigationSubtitle.

This way, I would have easily chosen your proposal. But I did not see any of those points, just "this function is used elsewhere, we may want to leave it so that other developers can use it".

Now that neither of your proposals was perfect, I decided to go with the first one that gave a correct RCA and a sufficient solution.

[dragnoir]

@paultsimura thank you. I will make it better in the next issue :)

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

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

• Always use immediate parent report when rendering navigation header #37436

If no regressions arise, payment will be issued on 2024-03-07. 🎊

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

• @barros001 requires payment automatic offer (Contributor)
• @paultsimura requires payment automatic offer (Reviewer)

[melvin-bot]

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:

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

[paultsimura]

• The PR that introduced the bug has been identified. Link to the PR: Thread subheaders and parent navigation #21092
• 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: This is rather a new feature request, so the original PR is not offending.
• 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: N/A
• Determine if we should create a regression test for this bug: Yes
• 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.

Regression Test Proposal

1. Post "Message 1" in a room
2. Click "Reply in thread" on "Message 1"
3. Post "Message 2" in this thread
4. Click "Reply in thread" on "Message 2"
5. Verify that the report title shows "Message 2" and the subtitle shows "Message 1" as the parent navigation it.

Do we agree 👍 or 👎

[sakluger]

Paid everyone out and created the regression test GH issue. Thanks everyone!