Change user permission by using a new apply button (#12346)

* Add PowerLevelSelector.tsx.
It's extracting the current behavior of the privileged users and muted of `RolesRoomSettingsTab.tsx` into a dedicated component.
It's also adding a new apply button.

* Use `PowerLevelSelector` to render privileged and muted users in `RolesRoomSettingsTab`

* Update existing tests

* Add playwright test

* Fix typo

* Fix typo

playwright/e2e/settings/roles-permissions-room-settings-tab.spec.ts

@@ -0,0 +1,58 @@
+/*
+ *
+ * Copyright 2024 The Matrix.org Foundation C.I.C.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * /
+ */
+
+import { Locator } from "@playwright/test";
+
+import { test, expect } from "../../element-web-test";
+
+test.describe("Roles & Permissions room settings tab", () => {
+    const roomName = "Test room";
+
+    test.use({
+        displayName: "Alice",
+    });
+
+    let settings: Locator;
+
+    test.beforeEach(async ({ user, app }) => {
+        await app.client.createRoom({ name: roomName });
+        await app.viewRoomByName(roomName);
+        settings = await app.settings.openRoomSettings("Roles & Permissions");
+    });
+
+    test("should be able to change the role of a user", async ({ page, app, user }) => {
+        const privilegedUserSection = settings.locator(".mx_SettingsFieldset").first();
+        const applyButton = privilegedUserSection.getByRole("button", { name: "Apply" });
+
+        // Alice is admin (100) and the Apply button should be disabled
+        await expect(applyButton).toBeDisabled();
+        let combobox = privilegedUserSection.getByRole("combobox", { name: user.userId });
+        await expect(combobox).toHaveValue("100");
+
+        // Change the role of Alice to Moderator (50)
+        await combobox.selectOption("Moderator");
+        await expect(combobox).toHaveValue("50");
+        await applyButton.click();
+
+        // Reload and check Alice is still Moderator (50)
+        await page.reload();
+        settings = await app.settings.openRoomSettings("Roles & Permissions");
+        combobox = privilegedUserSection.getByRole("combobox", { name: user.userId });
+        await expect(combobox).toHaveValue("50");
+    });
+});

res/css/_components.pcss

@@ -335,6 +335,7 @@
 @import "./views/settings/_NotificationSettings2.pcss";
 @import "./views/settings/_Notifications.pcss";
 @import "./views/settings/_PhoneNumbers.pcss";
+@import "./views/settings/_PowerLevelSelector.pcss";
 @import "./views/settings/_ProfileSettings.pcss";
 @import "./views/settings/_SecureBackupPanel.pcss";
 @import "./views/settings/_SetIdServer.pcss";

res/css/views/settings/_PowerLevelSelector.pcss

@@ -0,0 +1,21 @@
+/*
+ *
+ * Copyright 2024 The Matrix.org Foundation C.I.C.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * /
+ */
+
+.mx_PowerLevelSelector_Button {
+    align-self: flex-start;
+}

src/components/views/settings/PowerLevelSelector.tsx

@@ -0,0 +1,142 @@
+/*
+ *
+ * Copyright 2024 The Matrix.org Foundation C.I.C.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * /
+ */
+
+import React, { useState, JSX, PropsWithChildren } from "react";
+import { Button } from "@vector-im/compound-web";
+import { compare } from "matrix-js-sdk/src/utils";
+
+import { useMatrixClientContext } from "../../../contexts/MatrixClientContext";
+import PowerSelector from "../elements/PowerSelector";
+import { _t } from "../../../languageHandler";
+import SettingsFieldset from "./SettingsFieldset";
+
+/**
+ * Display in a fieldset, the power level of the users and allow to change them.
+ * The apply button is disabled until the power level of an user is changed.
+ * If there is no user to display, the children is displayed instead.
+ */
+interface PowerLevelSelectorProps {
+    /**
+     * The power levels of the users
+     * The key is the user id and the value is the power level
+     */
+    userLevels: Record<string, number>;
+    /**
+     * Whether the user can change the power levels of other users
+     */
+    canChangeLevels: boolean;
+    /**
+     * The current user power level
+     */
+    currentUserLevel: number;
+    /**
+     * The callback when the apply button is clicked
+     * @param value - new power level for the user
+     * @param userId - the user id
+     */
+    onClick: (value: number, userId: string) => void;
+    /**
+     * Filter the users to display
+     * @param user
+     */
+    filter: (user: string) => boolean;
+    /**
+     * The title of the fieldset
+     */
+    title: string;
+}
+
+export function PowerLevelSelector({
+    userLevels,
+    canChangeLevels,
+    currentUserLevel,
+    onClick,
+    filter,
+    title,
+    children,
+}: PropsWithChildren<PowerLevelSelectorProps>): JSX.Element | null {
+    const matrixClient = useMatrixClientContext();
+    const [currentPowerLevel, setCurrentPowerLevel] = useState<{ value: number; userId: string } | null>(null);
+
+    // If the power level has changed, we need to enable the apply button
+    const powerLevelChanged = Boolean(
+        currentPowerLevel && currentPowerLevel.value !== userLevels[currentPowerLevel?.userId],
+    );
+
+    // We sort the users by power level, then we filter them
+    const users = Object.keys(userLevels)
+        .sort((userA, userB) => sortUser(userA, userB, userLevels))
+        .filter(filter);
+
+    // No user to display, we return the children into fragment to convert it to JSX.Element type
+    if (!users.length) return <>{children}</>;
+
+    return (
+        <SettingsFieldset legend={title}>
+            {users.map((userId) => {
+                // We only want to display users with a valid power level aka an integer
+                if (!Number.isInteger(userLevels[userId])) return;
+
+                const isMe = userId === matrixClient.getUserId();
+                // If I can change levels, I can change the level of anyone with a lower level than mine
+                const canChange = canChangeLevels && (userLevels[userId] < currentUserLevel || isMe);
+
+                // When the new power level is selected, the fields are rerendered and we need to keep the current value
+                const userLevel = currentPowerLevel?.userId === userId ? currentPowerLevel?.value : userLevels[userId];
+
+                return (
+                    <PowerSelector
+                        value={userLevel}
+                        disabled={!canChange}
+                        label={userId}
+                        key={userId}
+                        onChange={(value) => setCurrentPowerLevel({ value, userId })}
+                    />
+                );
+            })}
+
+            <Button
+                size="sm"
+                kind="primary"
+                // mx_Dialog_nonDialogButton is necessary to avoid the Dialog CSS to override the button style
+                className="mx_Dialog_nonDialogButton mx_PowerLevelSelector_Button"
+                onClick={() => {
+                    if (currentPowerLevel !== null) {
+                        onClick(currentPowerLevel.value, currentPowerLevel.userId);
+                        setCurrentPowerLevel(null);
+                    }
+                }}
+                disabled={!powerLevelChanged}
+                aria-label={_t("action|apply")}
+            >
+                {_t("action|apply")}
+            </Button>
+        </SettingsFieldset>
+    );
+}
+
+/**
+ * Sort the users by power level, then by name
+ * @param userA
+ * @param userB
+ * @param userLevels
+ */
+function sortUser(userA: string, userB: string, userLevels: PowerLevelSelectorProps["userLevels"]): number {
+    const powerLevelDiff = userLevels[userA] - userLevels[userB];
+    return powerLevelDiff !== 0 ? powerLevelDiff : compare(userA.toLocaleLowerCase(), userB.toLocaleLowerCase());
+}

src/components/views/settings/tabs/room/RolesRoomSettingsTab.tsx

@@ -18,7 +18,6 @@ import React from "react";
 import { EventType, RoomMember, RoomState, RoomStateEvent, Room, IContent } from "matrix-js-sdk/src/matrix";
 import { logger } from "matrix-js-sdk/src/logger";
 import { throttle, get } from "lodash";
-import { compare } from "matrix-js-sdk/src/utils";
 
 import { _t, _td, TranslationKey } from "../../../../../languageHandler";
 import AccessibleButton from "../../../elements/AccessibleButton";
@@ -34,6 +33,7 @@ import { AddPrivilegedUsers } from "../../AddPrivilegedUsers";
 import SettingsTab from "../SettingsTab";
 import { SettingsSection } from "../../shared/SettingsSection";
 import MatrixClientContext from "../../../../../contexts/MatrixClientContext";
+import { PowerLevelSelector } from "../../PowerLevelSelector";
 
 interface IEventShowOpts {
     isState?: boolean;
@@ -240,9 +240,6 @@ export default class RolesRoomSettingsTab extends React.Component<IProps> {
                 title: _t("room_settings|permissions|error_changing_pl_title"),
                 description: _t("room_settings|permissions|error_changing_pl_description"),
             });
-
-            // Rethrow so that the PowerSelector can roll back
-            throw e;
         }
     };
 
@@ -352,66 +349,30 @@ export default class RolesRoomSettingsTab extends React.Component<IProps> {
         let privilegedUsersSection = <div>{_t("room_settings|permissions|no_privileged_users")}</div>;
         let mutedUsersSection;
         if (Object.keys(userLevels).length) {
-            const privilegedUsers: JSX.Element[] = [];
-            const mutedUsers: JSX.Element[] = [];
-
-            Object.keys(userLevels).forEach((user) => {
-                if (!Number.isInteger(userLevels[user])) return;
-                const isMe = user === client.getUserId();
-                const canChange = canChangeLevels && (userLevels[user] < currentUserLevel || isMe);
-                if (userLevels[user] > defaultUserLevel) {
-                    // privileged
-                    privilegedUsers.push(
-                        <PowerSelector
-                            value={userLevels[user]}
-                            disabled={!canChange}
-                            label={user}
-                            key={user}
-                            powerLevelKey={user} // Will be sent as the second parameter to `onChange`
-                            onChange={this.onUserPowerLevelChanged}
-                        />,
-                    );
-                } else if (userLevels[user] < defaultUserLevel) {
-                    // muted
-                    mutedUsers.push(
-                        <PowerSelector
-                            value={userLevels[user]}
-                            disabled={!canChange}
-                            label={user}
-                            key={user}
-                            powerLevelKey={user} // Will be sent as the second parameter to `onChange`
-                            onChange={this.onUserPowerLevelChanged}
-                        />,
-                    );
-                }
-            });
-
-            // comparator for sorting PL users lexicographically on PL descending, MXID ascending. (case-insensitive)
-            const comparator = (a: JSX.Element, b: JSX.Element): number => {
-                const aKey = a.key as string;
-                const bKey = b.key as string;
-                const plDiff = userLevels[bKey] - userLevels[aKey];
-                return plDiff !== 0 ? plDiff : compare(aKey.toLocaleLowerCase(), bKey.toLocaleLowerCase());
-            };
-
-            privilegedUsers.sort(comparator);
-            mutedUsers.sort(comparator);
-
-            if (privilegedUsers.length) {
             privilegedUsersSection = (
-                    <SettingsFieldset legend={_t("room_settings|permissions|privileged_users_section")}>
+                <PowerLevelSelector
-                        {privilegedUsers}
+                    title={_t("room_settings|permissions|privileged_users_section")}
-                    </SettingsFieldset>
+                    userLevels={userLevels}
+                    canChangeLevels={canChangeLevels}
+                    currentUserLevel={currentUserLevel}
+                    onClick={this.onUserPowerLevelChanged}
+                    filter={(user) => userLevels[user] > defaultUserLevel}
+                >
+                    <div>{_t("room_settings|permissions|no_privileged_users")}</div>
+                </PowerLevelSelector>
             );
-            }
+
-            if (mutedUsers.length) {
             mutedUsersSection = (
-                    <SettingsFieldset legend={_t("room_settings|permissions|muted_users_section")}>
+                <PowerLevelSelector
-                        {mutedUsers}
+                    title={_t("room_settings|permissions|muted_users_section")}
-                    </SettingsFieldset>
+                    userLevels={userLevels}
+                    canChangeLevels={canChangeLevels}
+                    currentUserLevel={currentUserLevel}
+                    onClick={this.onUserPowerLevelChanged}
+                    filter={(user) => userLevels[user] < defaultUserLevel}
+                />
             );
         }
-        }
 
         const banned = room.getMembersWithMembership("ban");
         let bannedUsersSection: JSX.Element | undefined;

test/components/views/settings/PowerLevelSelector-test.tsx

@@ -0,0 +1,120 @@
+/*
+ *
+ * Copyright 2024 The Matrix.org Foundation C.I.C.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * /
+ */
+
+import { render, screen } from "@testing-library/react";
+import React, { ComponentProps } from "react";
+import userEvent from "@testing-library/user-event";
+
+import { PowerLevelSelector } from "../../../../src/components/views/settings/PowerLevelSelector";
+import { stubClient } from "../../../test-utils";
+import MatrixClientContext from "../../../../src/contexts/MatrixClientContext";
+
+describe("PowerLevelSelector", () => {
+    const matrixClient = stubClient();
+
+    const currentUser = matrixClient.getUserId()!;
+    const userLevels = {
+        [currentUser]: 100,
+        "@alice:server.org": 50,
+        "@bob:server.org": 0,
+    };
+
+    const renderPLS = (props: Partial<ComponentProps<typeof PowerLevelSelector>>) =>
+        render(
+            <MatrixClientContext.Provider value={matrixClient}>
+                <PowerLevelSelector
+                    userLevels={userLevels}
+                    canChangeLevels={true}
+                    currentUserLevel={userLevels[currentUser]}
+                    title="title"
+                    // filter nothing by default
+                    filter={() => true}
+                    onClick={jest.fn()}
+                    {...props}
+                >
+                    empty label
+                </PowerLevelSelector>
+            </MatrixClientContext.Provider>,
+        );
+
+    it("should render", () => {
+        renderPLS({});
+        expect(screen.getByRole("group")).toMatchSnapshot();
+    });
+
+    it("should display only the current user", async () => {
+        // Display only the current user
+        renderPLS({ filter: (user) => user === currentUser });
+
+        // Only alice should be displayed
+        const userSelects = screen.getAllByRole("combobox");
+        expect(userSelects).toHaveLength(1);
+        expect(userSelects[0]).toHaveAccessibleName(currentUser);
+
+        expect(screen.getByRole("group")).toMatchSnapshot();
+    });
+
+    it("should be able to change the power level of the current user", async () => {
+        const onClick = jest.fn();
+        renderPLS({ onClick });
+
+        // Until the power level is changed, the apply button should be disabled
+        // compound button is using aria-disabled instead of the disabled attribute, we can't toBeDisabled on it
+        expect(screen.getByRole("button", { name: "Apply" })).toHaveAttribute("aria-disabled", "true");
+
+        const select = screen.getByRole("combobox", { name: currentUser });
+        // Sanity check
+        expect(select).toHaveValue("100");
+
+        // Change current user power level to 50
+        await userEvent.selectOptions(select, "50");
+        expect(select).toHaveValue("50");
+        // After the user level changes, the apply button should be enabled
+        expect(screen.getByRole("button", { name: "Apply" })).toHaveAttribute("aria-disabled", "false");
+
+        // Click on Apply should call onClick with the new power level
+        await userEvent.click(screen.getByRole("button", { name: "Apply" }));
+        expect(onClick).toHaveBeenCalledWith(50, currentUser);
+    });
+
+    it("should not be able to change the power level if `canChangeLevels` is false", async () => {
+        renderPLS({ canChangeLevels: false });
+
+        // The selects should be disabled
+        const userSelects = screen.getAllByRole("combobox");
+        userSelects.forEach((select) => expect(select).toBeDisabled());
+    });
+
+    it("should be able to change only the level of someone with a lower level", async () => {
+        const userLevels = {
+            [currentUser]: 50,
+            "@alice:server.org": 100,
+        };
+        renderPLS({ userLevels });
+
+        expect(screen.getByRole("combobox", { name: currentUser })).toBeEnabled();
+        expect(screen.getByRole("combobox", { name: "@alice:server.org" })).toBeDisabled();
+    });
+
+    it("should display the children if there is no user to display", async () => {
+        // No user to display
+        renderPLS({ filter: () => false });
+
+        expect(screen.getByText("empty label")).toBeInTheDocument();
+    });
+});

test/components/views/settings/__snapshots__/PowerLevelSelector-test.tsx.snap

@@ -0,0 +1,235 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`PowerLevelSelector should display only the current user 1`] = `
+<fieldset
+  class="mx_SettingsFieldset"
+>
+  <legend
+    class="mx_SettingsFieldset_legend"
+  >
+    title
+  </legend>
+  <div
+    class="mx_SettingsFieldset_content"
+  >
+    <div
+      class="mx_PowerSelector"
+    >
+      <div
+        class="mx_Field mx_Field_select"
+      >
+        <select
+          data-testid="power-level-select-element"
+          id="mx_Field_4"
+          label="@userId:matrix.org"
+          placeholder="@userId:matrix.org"
+          type="text"
+        >
+          <option
+            data-testid="power-level-option-0"
+            value="0"
+          >
+            Default
+          </option>
+          <option
+            data-testid="power-level-option-50"
+            value="50"
+          >
+            Moderator
+          </option>
+          <option
+            data-testid="power-level-option-100"
+            value="100"
+          >
+            Admin
+          </option>
+          <option
+            data-testid="power-level-option-SELECT_VALUE_CUSTOM"
+            value="SELECT_VALUE_CUSTOM"
+          >
+            Custom level
+          </option>
+        </select>
+        <label
+          for="mx_Field_4"
+        >
+          @userId:matrix.org
+        </label>
+      </div>
+    </div>
+    <button
+      aria-disabled="true"
+      aria-label="Apply"
+      class="_button_dyfp8_17 mx_Dialog_nonDialogButton mx_PowerLevelSelector_Button"
+      data-kind="primary"
+      data-size="sm"
+      role="button"
+      tabindex="0"
+    >
+      Apply
+    </button>
+  </div>
+</fieldset>
+`;
+
+exports[`PowerLevelSelector should render 1`] = `
+<fieldset
+  class="mx_SettingsFieldset"
+>
+  <legend
+    class="mx_SettingsFieldset_legend"
+  >
+    title
+  </legend>
+  <div
+    class="mx_SettingsFieldset_content"
+  >
+    <div
+      class="mx_PowerSelector"
+    >
+      <div
+        class="mx_Field mx_Field_select"
+      >
+        <select
+          data-testid="power-level-select-element"
+          id="mx_Field_1"
+          label="@bob:server.org"
+          placeholder="@bob:server.org"
+          type="text"
+        >
+          <option
+            data-testid="power-level-option-0"
+            value="0"
+          >
+            Default
+          </option>
+          <option
+            data-testid="power-level-option-50"
+            value="50"
+          >
+            Moderator
+          </option>
+          <option
+            data-testid="power-level-option-100"
+            value="100"
+          >
+            Admin
+          </option>
+          <option
+            data-testid="power-level-option-SELECT_VALUE_CUSTOM"
+            value="SELECT_VALUE_CUSTOM"
+          >
+            Custom level
+          </option>
+        </select>
+        <label
+          for="mx_Field_1"
+        >
+          @bob:server.org
+        </label>
+      </div>
+    </div>
+    <div
+      class="mx_PowerSelector"
+    >
+      <div
+        class="mx_Field mx_Field_select"
+      >
+        <select
+          data-testid="power-level-select-element"
+          id="mx_Field_2"
+          label="@alice:server.org"
+          placeholder="@alice:server.org"
+          type="text"
+        >
+          <option
+            data-testid="power-level-option-0"
+            value="0"
+          >
+            Default
+          </option>
+          <option
+            data-testid="power-level-option-50"
+            value="50"
+          >
+            Moderator
+          </option>
+          <option
+            data-testid="power-level-option-100"
+            value="100"
+          >
+            Admin
+          </option>
+          <option
+            data-testid="power-level-option-SELECT_VALUE_CUSTOM"
+            value="SELECT_VALUE_CUSTOM"
+          >
+            Custom level
+          </option>
+        </select>
+        <label
+          for="mx_Field_2"
+        >
+          @alice:server.org
+        </label>
+      </div>
+    </div>
+    <div
+      class="mx_PowerSelector"
+    >
+      <div
+        class="mx_Field mx_Field_select"
+      >
+        <select
+          data-testid="power-level-select-element"
+          id="mx_Field_3"
+          label="@userId:matrix.org"
+          placeholder="@userId:matrix.org"
+          type="text"
+        >
+          <option
+            data-testid="power-level-option-0"
+            value="0"
+          >
+            Default
+          </option>
+          <option
+            data-testid="power-level-option-50"
+            value="50"
+          >
+            Moderator
+          </option>
+          <option
+            data-testid="power-level-option-100"
+            value="100"
+          >
+            Admin
+          </option>
+          <option
+            data-testid="power-level-option-SELECT_VALUE_CUSTOM"
+            value="SELECT_VALUE_CUSTOM"
+          >
+            Custom level
+          </option>
+        </select>
+        <label
+          for="mx_Field_3"
+        >
+          @userId:matrix.org
+        </label>
+      </div>
+    </div>
+    <button
+      aria-disabled="true"
+      aria-label="Apply"
+      class="_button_dyfp8_17 mx_Dialog_nonDialogButton mx_PowerLevelSelector_Button"
+      data-kind="primary"
+      data-size="sm"
+      role="button"
+      tabindex="0"
+    >
+      Apply
+    </button>
+  </div>
+</fieldset>
+`;

test/components/views/settings/tabs/room/RolesRoomSettingsTab-test.tsx

@@ -15,10 +15,11 @@ limitations under the License.
 */
 
 import React from "react";
-import { fireEvent, render, RenderResult, screen, waitFor } from "@testing-library/react";
+import { fireEvent, getByRole, render, RenderResult, screen, waitFor } from "@testing-library/react";
 import { MatrixClient, EventType, MatrixEvent, Room, RoomMember, ISendEventResponse } from "matrix-js-sdk/src/matrix";
 import { mocked } from "jest-mock";
 import { defer } from "matrix-js-sdk/src/utils";
+import userEvent from "@testing-library/user-event";
 
 import RolesRoomSettingsTab from "../../../../../../src/components/views/settings/tabs/room/RolesRoomSettingsTab";
 import { mkStubRoom, withClientContextRenderOptions, stubClient } from "../../../../../test-utils";
@@ -262,6 +263,11 @@ describe("RolesRoomSettingsTab", () => {
         fireEvent.change(selector, { target: { value: "50" } });
         expect(selector).toHaveValue("50");
 
+        // Get the apply button of the privileged user section and click on it
+        const privilegedUsersSection = screen.getByRole("group", { name: "Privileged Users" });
+        const applyButton = getByRole(privilegedUsersSection, "button", { name: "Apply" });
+        await userEvent.click(applyButton);
+
         deferred.reject("Error");
         await waitFor(() => expect(selector).toHaveValue("100"));
     });