[hold for payment 2024-09-17] [$500] Make the Details > Members view of rooms, groups, and reports consistent

[shawnborton]

As part of the group chat clean up, we updated the Details > Members section look like a table view:

This view matches more closely with what the Members section of the workspace editor looks like.

However, we did not update this same view when viewing certain chat rooms, like this:

So let's make this part of the app consistent and use the updated members table style everywhere.

cc @marcaaron @JmillsExpensify

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01cc3a8881c60b2cb7
• Upwork Job ID: 1820807133375331083
• Last Price Increase: 2024-08-15
• Automatic offers:
• paultsimura | Reviewer | 103430717
• rayane-djouah | Contributor | 103430718

Issue Owner

Current Issue Owner: @paultsimura

[melvin-bot]

Triggered auto assignment to @joekaufmanexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

[melvin-bot]

⚠️ It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time ⚠️

[melvin-bot]

Triggered auto assignment to Design team member for new feature review - @dannymcclain (NewFeature)

[rayane-djouah]

Edited by proposal-police: This proposal was edited at 2024-08-06 13:57:41 UTC.

Proposal

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

We need to make the members list of rooms and reports consistent with groups and workspace members list

What is the root cause of that problem?

New feature

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

We should update the SelectionList usage in RoomMembersPage to use TableListItem instead of UserListItem:

App/src/pages/RoomMembersPage.tsx

Line 307 in 412fc60

ListItem={UserListItem}

if we need to align the select all checkbox with the other checkboxes, we need to add a listHeaderWrapperStyle like this:

  listHeaderWrapperStyle={[styles.ph9]}

Result:

If we want to use the dynamic button with a dropdown we should:

• Use the SelectionListWithModal instead of SelectionList
• Add props like:

  turnOnSelectionModeOnLongPress
  onTurnOnSelectionMode={(item) => item && toggleUser(item)}
  onSelectRow={openRoomMemberDetails}
  onCheckboxPress={(item) => toggleUser(item)}
  listHeaderWrapperStyle={[styles.ph9]}

• Add dropdown selection button logic similar to here and here but witout including a "make admin" option
• add openRoomMemberDetails function:

    /** Opens the room member details page */
    const openMemberDetails = useCallback(
        (item: ListItem) => {
            Navigation.navigate(ROUTES.ROOM_MEMBERS_DETAILS.getRoute(report.reportID, item?.accountID ?? -1));
        },
        [report],
    );

• add a new route:

    ROOM_MEMBERS_DETAILS: {
        route: 'r/:reportID/members/:accountID',
        getRoute: (reportID: string, accountID: number) => `r/${reportID}/members/${accountID}` as const,
    },

• Implement a new page component ReportMemberDetails for the ROOM_MEMBERS_DETAILS route similar to ReportParticipantDetails here (but without member role and without role menu item) with a "Remove from room" button instead of "Remove from group"

We can also refactor ReportParticipantDetailsPage with condition checks to use it for reports and rooms instead of creating a new page.

[neonbhai]

Proposal

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

Make the Details > Members view of rooms, groups, and reports consistent

What is the root cause of that problem?

Feature Request

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

We will:

• Use the SelectionListWithModal component

• Change the list item type for room members page to UserListItem
  

  App/src/pages/RoomMembersPage.tsx

  Line 307 in e909496

  ListItem={UserListItem}

• Add Selection Mode to this page:

  • Add props turnOnSelectionModeOnLongPress && onTurnOnSelectionMode={(item) => item && toggleUser(item?.accountID)}

• Add logic for Dropdown Header Button same as here. Render it as

  <View style={[styles.pl5, styles.pr5]}>{headerButtons}</View>
  

• Delete the Remove button, Add a 'Invite Members' button similar to workspace members page here

      const headerContent = useMemo(() => {
        return <Text style={[styles.pl5, styles.mb4, styles.mt6, styles.textSupporting]}>{'List of all room members'}</Text>;
    }, [styles, translate]);
  
    const bulkActionsButtonOptions = useMemo(() => {
        const options: Array<DropdownOption<WorkspaceMemberBulkActionType>> = [
            {
                text: translate('workspace.people.removeMembersTitle'),
                value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.REMOVE,
                icon: Expensicons.RemoveMembers,
                onSelected: () => setRemoveMembersConfirmModalVisible(true),
            },
        ];
  
        return options;
    }, [translate, setRemoveMembersConfirmModalVisible, selectedMembers, report.participants]);
  
    const headerButtons = useMemo(() => {
        return (
            <View style={styles.w100}>
                {(selectedMembers.length > 0) ? (
                    <ButtonWithDropdownMenu<WorkspaceMemberBulkActionType>
                        shouldAlwaysShowDropdownMenu
                        pressOnEnter
                        customText={translate('workspace.common.selected', {selectedNumber: selectedMembers.length})}
                        buttonSize={CONST.DROPDOWN_BUTTON_SIZE.MEDIUM}
                        onPress={() => null}
                        options={bulkActionsButtonOptions}
                        style={[styles.flexGrow1]}
                        isDisabled={!selectedMembers.length}
                    />
                ) : (
                    <Button
                        medium
                        success
                        onPress={inviteUser}
                        text={translate('workspace.invite.member')}
                        icon={Expensicons.Plus}
                        innerStyles={[styles.alignItemsCenter]}
                        style={[styles.flexGrow1]}
                    />
                )}
            </View>
        );
    }, [bulkActionsButtonOptions, inviteUser, selectedMembers, styles, translate]);

• Remove the SearchValue state.

[rayane-djouah]

Proposal

Updated to align the "select all" checkbox with other checkboxes

[github-actions]

@user Your proposal will be dismissed because you did not follow the proposal template.

[rayane-djouah]

Hmm, @shawnborton - Do we need to update the room members page to use the new selection mode and remove the search input? I thought we only need to update the style of the list

[shawnborton]

We want to keep the search functionality there when it's needed. I think we include the search bar if the list is greater than 8 or something like that?

[melvin-bot]

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

[melvin-bot]

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

[paultsimura]

Reviewing today 👀

[paultsimura]

@shawnborton do we want to keep the header buttons as they are now?

---

Or do we want it to use the dynamic button with a dropdown (does the dropdown make sense if there's only 1 option – "delete")?

[dannymcclain]

do we want to keep the header buttons as they are now?
Or do we want it to use the dynamic button with a dropdown?

I'll let Shawn weigh in, but it makes sense to me to use the dynamic button (2nd & 3rd screenshots above) because that's consistent with how groups are currently working.

(does the dropdown make sense if there's only 1 option – "delete")?

Personally I think this is fine

[rayane-djouah]

Proposal

Updated to use the dynamic dropdown button and add a room member details page

[rayane-djouah]

We want to keep the search functionality there when it's needed. I think we include the search bar if the list is greater than 8 or something like that?

We are always including the search bar

[shawnborton]

Interesting, I would think that we only include the search bar when we hit X number of members. Thoughts on that @Expensify/design @trjExpensify @JmillsExpensify?

[dannymcclain]

Interesting, I would think that we only include the search bar when we hit X number of members.

Yeah I agree. Not sure why we'd treat this differently than everything else.

[dubielzyk-expensify]

Agree with Danny and Shawn

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[rayane-djouah]

This ^ was a very minor copy change issue, which I addressed in a follow-up PR: #48843.

[melvin-bot]

This issue has not been updated in over 15 days. @MariaHCD, @paultsimura, @joekaufmanexpensify, @rayane-djouah eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[joekaufmanexpensify]

Hm, that is not correct.

[rayane-djouah]

The PR was deployed to production yesterday as part of #48791 production deploy checklist

[paultsimura]

This is correct. The payment is due 2024-09-17.

[joekaufmanexpensify]

Sounds good!

[paultsimura]

This is more of a design update issue, therefore no offending PRs. I will suggest a regression test for certain parts of the implemented logic.

• The PR that introduced the bug has been identified. Link to the PR: N/A
• 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: N/A
• 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

Regression test proposal

Test 1:

1. Create a room with less than 8 members
2. Click on the room header
3. Go to the "Members" page
4. Verify it shows a list of members without a search bar
5. Invite more members to have 8 of them or more
6. Verify the "Members" page now contains a search field that allows filtering the existing members

Test 2:

1. Create a room with several members
2. Click on the room header
3. Go to the "Members" page
4. Select a couple of members
5. Click on a single member outside of the checkbox area
6. Verify the "Member Details" page is opened
7. Go back
8. Verify the user selection is cleared

Do we agree 👍 or 👎

[joekaufmanexpensify]

This ^ was a very minor copy change issue, which I addressed in a follow-up PR: #48843.

@rayane-djouah I agree this seems like a pretty simple copy change, but it was still a regression, no?

[joekaufmanexpensify]

Sounds good on checklist, I will handle!

[paultsimura]

it was still a regression, no?

I would argue with this. It would have been a regression if it broke some functionality that existed before this feature was released. Since we added the search functionality only in this PR, I would consider it a missed use-case. Honestly, this PR was huge in terms of covering different scenarios.

[rayane-djouah]

I agree with Pavlo, The search input was not previously present on the report participants page and was introduced in our PR. The issue occurred because the existing message, Member not found. To invite a new member to the room, please use the invite button above. was incompatible with reports where inviting members isn't possible (such as #announce). This was a missed edge case in the new feature's design and implementation, not a regression (which refers to something that was working before but then broke). We've addressed this in a follow-up.

[joekaufmanexpensify]

Cool cool, appreciate the context. I agree there should be no regression penalty here. I'm thinking we should just consider that follow up PR as part of the payment being sent here. Does that sound good to everyone?

[paultsimura]

Sounds good to me 👍

[joekaufmanexpensify]

Made regression test: https://github.com/Expensify/Expensify/issues/428848

[joekaufmanexpensify]

All set to issue payment! We need to pay:

• @rayane-djouah - $500 for PR via Upwork.
• @paultsimura - $500 for C+ via Upwork.

[joekaufmanexpensify]

@rayane-djouah $500 sent and contract ended!

[joekaufmanexpensify]

@paultsimura $500 sent and contract ended!

[joekaufmanexpensify]

Upwork job closed.

[joekaufmanexpensify]

All set, thanks everyone!