[HOLD for payment 2024-11-29] [$250] Workspace - User is navigate to thread message when going to #announce room

[lanitochka17]

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.56-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Navigate to staging.new.expensify.com
2. Create a workspace > add 2 members
3. Go to #announce room
4. Send a message > Create a thread message
5. Navigate to Workspace > Click on the three dot menu > Click #announce

Expected Result:

User is navigated to #announce room main page

Actual Result:

User is navigated to #announce room thread message page

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6651209_1730386216675.Screen_Recording_2024-10-31_at_5.40.25_in_the_afternoon.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021852040470527704127
• Upwork Job ID: 1852040470527704127
• Last Price Increase: 2024-11-07
• Automatic offers:
• Nodebrute | Contributor | 104835368

Issue Owner

Current Issue Owner: @eVoloshchak

[melvin-bot]

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

@garrettmknight 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

[Nodebrute]

Edited by proposal-police: This proposal was edited at 2024-10-31 17:34:21 UTC.

Proposal

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

User is navigate to thread message when going to #announce room

What is the root cause of that problem?

This issue occurs not only with the #announce room but also with the #admins room. This happens because the threads created in these rooms also have a chatType of adminroom, and in this switch statement, we’re checking specifically for chatType.

App/src/pages/workspace/WorkspacesListPage.tsx

Lines 283 to 294 in 6463c2b

	switch (report.chatType) {
	case CONST.REPORT.CHAT_TYPE.POLICY_ADMINS:
	// eslint-disable-next-line no-param-reassign
	result[report.policyID].adminRoom = report.reportID;
	break;
	case CONST.REPORT.CHAT_TYPE.POLICY_ANNOUNCE:
	// eslint-disable-next-line no-param-reassign
	result[report.policyID].announceRoom = report.reportID;
	break;
	default:
	break;
	}

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

We should early return if report has parentReportID here

App/src/pages/workspace/WorkspacesListPage.tsx

Line 274 in 6463c2b

if (!report?.reportID || !report.policyID) {

   if (!report?.reportID || !report.policyID || report.parentReportID) {
                return result;
            }

Alternatively, we can also use parentReportActionID.

What alternative solutions did you explore? (Optional)

Alternatively, instead of using chatType in the switch statement, we could use reportName, as these room names are only used for workspace rooms.

pseudo-code

 switch (report.reportName) {
                case CONST.REPORT.WORKSPACE_CHAT_ROOMS.ADMINS:
                    // eslint-disable-next-line no-param-reassign
                    result[report.policyID].adminRoom = report.reportID;
                    break;
                case CONST.REPORT.WORKSPACE_CHAT_ROOMS.ANNOUNCE:
                    // eslint-disable-next-line no-param-reassign
                    result[report.policyID].announceRoom = report.reportID;
                    break;
                default:
                    break;
            }

[melvin-bot]

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

[melvin-bot]

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

[daledah]

Proposal

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

• User is navigated to #announce room thread message page

What is the root cause of that problem?

• Here’s the logic to retrieve the announce room:

App/src/pages/workspace/WorkspacesListPage.tsx

Line 351 in 6463c2b

announceRoom: policyRooms?.[policy.id]?.announceRoom ?? (policy.chatReportIDAnnounce ? policy.chatReportIDAnnounce?.toString() : ''),

we first search for it in the reports data. If no valid report is found, we then fall back on the chatReportIDAnnounce data associated with each policy.

• However, when looking for the announce room ID within the reports data, we're mistakenly identifying any report with chatType === policyAnnounce as the announce room:

App/src/pages/workspace/WorkspacesListPage.tsx

Line 283 in 6463c2b

switch (report.chatType) {

App/src/pages/workspace/WorkspacesListPage.tsx

Lines 288 to 290 in 6463c2b

	case CONST.REPORT.CHAT_TYPE.POLICY_ANNOUNCE:
	// eslint-disable-next-line no-param-reassign
	result[report.policyID].announceRoom = report.reportID;

when in fact, the announce room’s thread also has chatType === policyAnnounce.

As a result, the value assigned to result[report.policyID].announceRoom might actually point to the announce room's thread instead.

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

• Because the chatReportIDAnnounce data is available for each policy, we can make use of that data. Using the original approach (searching for the reports data first then fall back to chatReportIDAnnounce) is not proper in case there are a lot of reports, the announce room report is not fetched yet. So this line:

can be:

                    announceRoom: (policy.chatReportIDAnnounce ? policy.chatReportIDAnnounce?.toString() : '') || policyRooms?.[policy.id]?.announceRoom,

• The above change almost fixes the bug. But in case BE misses chatReportIDAnnounce data, so we still need to get the announce report data from the reports data. In this case, we need to update this line:

            if (ReportUtils.isThread(report)) {
                return result;
            }

What alternative solutions did you explore? (Optional)

• The similar issue can be reproduced with admin room, and the similar solution can be applied.

[melvin-bot]

@garrettmknight, @eVoloshchak Eep! 4 days overdue now. Issues have feelings too...

[garrettmknight]

@eVoloshchak can you please review this proposal?

[melvin-bot]

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

[eVoloshchak]

We should proceed with @daledah's proposal, since It makes use of the already-existing ReportUtils.isThread method

🎀👀🎀 C+ reviewed!

[melvin-bot]

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

[Nodebrute]

@eVoloshchak Hey, the isThread method uses the same report.parentReportID and parentReportActionID under the hood as my main proposal

App/src/libs/ReportUtils.ts

Line 1192 in b821068

return !!(report?.parentReportID && report?.parentReportActionID);

So, I don't think selecting a proposal that relies on a utility function with the same logic as my solution is fair. Also, I was the first to provide the correct solution and root cause analysis. As far as I know, that proposal should be significantly different from the previous ones, but I don’t see any real difference here.

Note: Before submitting a proposal on an issue, be sure to read any other existing proposals. ALL NEW PROPOSALS MUST BE DIFFERENT FROM EXISTING PROPOSALS. The difference should be important, meaningful or considerable.

cc: @mountiny

[melvin-bot]

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

[mountiny]

I agree, I think @Nodebrute already nailed it and it was first, also noted its valid issue for admins room too, can you fix it there too?

[Nodebrute]

@mountiny Thank you so much! My solution will fix it for both rooms.

[mountiny]

@Nodebrute, we are also working on a new process which will require adding unit tests for each bug fix to increase the chances of this issue not getting re-introduced. Can you also add unit tests to cover this case please?

[Nodebrute]

Yes, sure. PR will be ready in few hours.

[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 9.0.65-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix navigation to annouce room #52349

If no regressions arise, payment will be issued on 2024-11-29. 🎊

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

• @eVoloshchak requires payment through NewDot Manual Requests
• @Nodebrute requires payment automatic offer (Contributor)

[melvin-bot]

@eVoloshchak @garrettmknight @eVoloshchak The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[mountiny]

@eVoloshchak can you please complete the checklist here? Thanks!

[melvin-bot]

@garrettmknight, @eVoloshchak, @mountiny, @Nodebrute Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[garrettmknight]

@eVoloshchak bump on the checklist.

@Nodebrute just paid you out.

[garrettmknight]

@eVoloshchak bump on the checklist.

[melvin-bot]

@garrettmknight, @eVoloshchak, @mountiny, @Nodebrute Whoops! This issue is 2 days overdue. Let's get this updated quick!

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: [Wave 8] Ideal nav  #33280
• 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: https://github.com/Expensify/App/pull/33280/files#r1875956571
• 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: Not sure if additional discussion is needed, this Is a pretty simple navigation bug
• Determine if we should create a regression test for this bug
  Is it easy to test for this bug? Yes
  Is the bug related to an important user flow? Yes
  Is it an impactful bug? Yes
  This isn't a critical bug, but it is pretty impactful and related to widely used functionality, we might add a regression test

Regression Test Proposal

1. Create a workspace > add 2 members
2. Go to #announce room
3. Send a message > Reply in thread -> send a message in thread
4. Navigate to Settings/Workspaces > Click on the three-dot menu > Click on Go to #announce room
5. Verify user is navigated to the #announce room

Do we agree 👍 or 👎

[garrettmknight]

Payment Summary:

• Contributor: @Nodebrute $250 paid via Upwork
• Reveiwer: @eVoloshchak $250 via NewDot