[HOLD for payment 2024-12-09] [$250] BUG: "Members" section of #admins room doesn't have room name near the top like other rooms

[jamesdeanexpensify]

Details

Coming from this convo, I noticed that when you go into any #admins room and click into the Members section, near the top it doesn't show the room name:

In any other room, it does:

cc @Expensify/design

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021859707716941961146
• Upwork Job ID: 1859707716941961146
• Last Price Increase: 2024-11-21
• Automatic offers:
• Krishna2323 | Contributor | 105068916

Issue Owner

Current Issue Owner: @OfstadC

[melvin-bot]

Triggered auto assignment to @OfstadC (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.

[Krishna2323]

Proposal


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

BUG: "Members" section of #admins room doesn't have room name near the top like other rooms

What is the root cause of that problem?

• Subtitle is not passed to HeaderWithBackButton.
  

  App/src/pages/ReportParticipantsPage.tsx

  Lines 367 to 382 in 93f7874

  	<HeaderWithBackButton
  	title={selectionModeHeader ? translate('common.selectMultiple') : headerTitle}
  	onBackButtonPress={() => {
  	if (selectionMode?.isEnabled) {
  	setSelectedMembers([]);
  	turnOffMobileSelectionMode();
  	return;
  	}
  	if (report) {
  	setSearchValue('');
  	Navigation.goBack(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(report.reportID, backTo));
  	}
  	}}
  	guidesCallTaskID={CONST.GUIDES_CALL_TASK_IDS.WORKSPACE_MEMBERS}
  	/>

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


Pass subtitle={StringUtils.lineBreaksToSpaces(ReportUtils.getReportName(report))} to HeaderWithBackButton.

What alternative solutions did you explore? (Optional)

Result

[melvin-bot]

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

[melvin-bot]

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

[FitseTLT]

Edited by proposal-police: This proposal was edited at 2024-11-21 21:15:47 UTC.

Proposal

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

BUG: "Members" section of #admins room doesn't have room name near the top like other rooms

What is the root cause of that problem?

We haven't given the report name as subtitle to HeaderWithBackButton

App/src/pages/ReportParticipantsPage.tsx

Line 368 in e9e1857

title={selectionModeHeader ? translate('common.selectMultiple') : headerTitle}

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

We can give the report name as the subtitle for default rooms

                    subtitle={ReportUtils.isDefaultRoom(report) ? StringUtils.lineBreaksToSpaces(ReportUtils.getReportName(report)) : undefined}

If we want we can do the same for isGroupChat too

What alternative solutions did you explore? (Optional)

We can optionally apply it to isAdminRoom only if we want (or even isAnnounceRoom) or may be isGroupChat case too but for other case as iou reports expense reports etc... we surely don't want to show the names

[mkzie2]

Edited by proposal-police: This proposal was edited at 2024-11-22 11:11:34 UTC.

Proposal

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

"Members" section of #admins room doesn't have room name near the top like other rooms

What is the root cause of that problem?

For the chat room, the member page is in RoomMembersPage and we have a subtitle in the header is the room name here

App/src/pages/RoomMembersPage.tsx

Line 356 in 686b8b4

subtitle={StringUtils.lineBreaksToSpaces(ReportUtils.getReportName(report))}

For other reports, the member page is in ReportParticipantsPage and we don't pass subtitle prop to HeaderWithBackButton then no report name section is displayed

App/src/pages/ReportParticipantsPage.tsx

Line 367 in 686b8b4

<HeaderWithBackButton

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

The default room exists for all workspaces so I think we can display the subtitle with the workspace name, it's more useful for the user to know

1. We can get the additional detail like here and because we only want to display it for the default room of the workspace, we can only get the case in ${chatRoomSubtitle}

const chatRoomSubtitle = useMemo(() => ReportUtils.getChatRoomSubtitle(report), [report, policy]);
const additionalRoomDetails = `${translate('threads.in')} ${chatRoomSubtitle}`;

2. Pass subtitle prop here

subtitle={ReportUtils.isDefaultRoom(report) ? `${StringUtils.lineBreaksToSpaces(ReportUtils.getReportName(report))} ${additionalRoomDetails}` : ''}

App/src/pages/ReportParticipantsPage.tsx

Line 367 in 686b8b4

<HeaderWithBackButton

Optional: We can do the same for the chat room member page in RoomMembersPage

What alternative solutions did you explore? (Optional)

We can apply the main solution for only special reports that we want like admin, and announce or for report with isChatRoom is true.

We can also apply the main solution for other reports like group chat, ... but we will only display the report name in this case

Result

[mkzie2]

@dubielzyk-expensify What do you think about my result above? Display #admin with workspace is more useful for the user to know the workspace of this admin room when visiting the member page.

[shawnborton]

That looks pretty good to me 👍 I think the idea is here we want to be consistent and apply it everywhere, not just certain rooms.

[jamesdeanexpensify]

If it follows the pattern of all the other rooms, I think it would exclude the workspace name, right?

[FitseTLT]

If it follows the pattern of all the other rooms, I think it would exclude the workspace name, right?

Yes

[dukenv0307]

@shawnborton @jamesdeanexpensify

For the confirmation here

That looks pretty good to me 👍 I think the idea is here we want to be consistent and apply it everywhere, not just certain rooms.

we want to apply this change for other room (group, ...), not just default room (admin, announce)

Another problem: Should we show #admin for all admin rooms? Or #admin in WS?

For the normal room, we're showing the room name (without WS) as a subtitle, and each room has a different name -> that would be fine

But if we do the same for admin rooms, #admin will be shown -> This name is not really useful to me.

[shawnborton]

Another problem: Should we show #admin for all admin rooms? Or #admin in WS?

I think whatever we do, we should try to be consistent. So I think I would opt to just just show the name of the room as it would be shown in the LHN.

[dannymcclain]

I think whatever we do, we should try to be consistent. So I think I would opt to just just show the name of the room as it would be shown in the LHN

Same. I'm down to start with showing it exactly as it shows in the LHN. And if we find that that's not enough, we can adjust further.

[dukenv0307]

Let's go with @Krishna2323's proposal since we should try to be consistent.

• iou report

• group chat

• admin room

• announce room

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @marcaaron is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[FitseTLT]

@jamesdeanexpensify @shawnborton please check out the result the reviewer has shown above. I think there is a misunderstanding the purpose of the original OP was to make default rooms consistent with other user created policy rooms. Wasn't it? not for all iou reports and so on.

[dubielzyk-expensify]

Agree with the people above. I'm not sure I'd bring it to IOUs, but other than that the above looks good to me. Happy to lean on what others think here if we also think we should have it on IOUs. I don't think it's a big deal if we do, then at least it's consistent across the board

[shawnborton]

Yeah, I think I lean on just always using it everywhere for the sake of consistency. I don't feel strongly though, but I think I would lean that direction instead of making exceptions for certain room types.

[dannymcclain]

Yeah, I think I lean on just always using it everywhere for the sake of consistency.

+1. I also don't feel strongly, but I don't mind using it everywhere out of consistency. I don't think it does any harm being there.

[melvin-bot]

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

[Krishna2323]

@dukenv0307 PR ready for review ^

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

• fix: 'Members' section of #admins room doesn't have room name near the top like other rooms. #53110

If no regressions arise, payment will be issued on 2024-12-09. 🎊

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

• @Krishna2323 requires payment automatic offer (Contributor)
• @dukenv0307 requires payment through NewDot Manual Requests

[melvin-bot]

@dukenv0307 @OfstadC @dukenv0307 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]

[OfstadC]

@dukenv0307 Please complete checklist before payment date. Thank you 😃

[dukenv0307]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production
• 2b. Reported on staging (deploy blocker)
• 2c. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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 new requirement so we don't need offending PR.

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again. Yes

Regression Test Proposal Template

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue: https://github.com/Expensify/Expensify/issues/451670

Regression Test Proposal

Test:

1. Go to workspace admins room
2. Click on header > click on members
3. Verify subtitle #admins is shown in the header
4. Go to workspace announce room
5. Click on header > click on members
6. Verify subtitle #announce is shown in the header

Do we agree 👍 or 👎

[dukenv0307]

@OfstadC I completed

[OfstadC]

Payment Summary

• @Krishna2323 paid $250 via Upwork
• @dukenv0307 due $250 via manual NewDot Request

[JmillsExpensify]

$250 approved for @dukenv0307