From 74cf4144062a454eb235ba9e22ffcf67c8587a1b Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 13 Jul 2023 13:11:03 +0100 Subject: [PATCH 1/6] Prevent user from accidentally double clicking user info admin actions --- src/components/views/right_panel/UserInfo.tsx | 125 +++++++++++++----- 1 file changed, 90 insertions(+), 35 deletions(-) diff --git a/src/components/views/right_panel/UserInfo.tsx b/src/components/views/right_panel/UserInfo.tsx index 72768064ca0..42a88f8d968 100644 --- a/src/components/views/right_panel/UserInfo.tsx +++ b/src/components/views/right_panel/UserInfo.tsx @@ -605,6 +605,7 @@ export const useRoomPowerLevels = (cli: MatrixClient, room: Room): IPowerLevelsC interface IBaseProps { member: RoomMember; + isUpdating: boolean; startUpdating(): void; stopUpdating(): void; } @@ -612,6 +613,7 @@ interface IBaseProps { export const RoomKickButton = ({ room, member, + isUpdating, startUpdating, stopUpdating, }: Omit): JSX.Element | null => { @@ -621,6 +623,9 @@ export const RoomKickButton = ({ if (member.membership !== "invite" && member.membership !== "join") return <>; const onKick = async (): Promise => { + if (isUpdating) return; // only allow one operation at a time + startUpdating(); + const commonProps = { member, action: room.isSpaceRoom() @@ -669,9 +674,10 @@ export const RoomKickButton = ({ } const [proceed, reason, rooms = []] = await finished; - if (!proceed) return; - - startUpdating(); + if (!proceed) { + stopUpdating(); + return; + } bulkSpaceBehaviour(room, rooms, (room) => cli.kick(room.roomId, member.userId, reason || undefined)) .then( @@ -702,7 +708,12 @@ export const RoomKickButton = ({ : _t("Remove from room"); return ( - + {kickLabel} ); @@ -736,6 +747,7 @@ const RedactMessagesButton: React.FC = ({ member }) => { export const BanToggleButton = ({ room, member, + isUpdating, startUpdating, stopUpdating, }: Omit): JSX.Element => { @@ -743,6 +755,9 @@ export const BanToggleButton = ({ const isBanned = member.membership === "ban"; const onBanOrUnban = async (): Promise => { + if (isUpdating) return; // only allow one operation at a time + startUpdating(); + const commonProps = { member, action: room.isSpaceRoom() @@ -809,9 +824,10 @@ export const BanToggleButton = ({ } const [proceed, reason, rooms = []] = await finished; - if (!proceed) return; - - startUpdating(); + if (!proceed) { + stopUpdating(); + return; + } const fn = (roomId: string): Promise => { if (isBanned) { @@ -851,7 +867,7 @@ export const BanToggleButton = ({ }); return ( - + {label} ); @@ -863,7 +879,14 @@ interface IBaseRoomProps extends IBaseProps { children?: ReactNode; } -const MuteToggleButton: React.FC = ({ member, room, powerLevels, startUpdating, stopUpdating }) => { +const MuteToggleButton: React.FC = ({ + member, + room, + powerLevels, + isUpdating, + startUpdating, + stopUpdating, +}) => { const cli = useContext(MatrixClientContext); // Don't show the mute/unmute option if the user is not in the room @@ -871,21 +894,31 @@ const MuteToggleButton: React.FC = ({ member, room, powerLevels, const muted = isMuted(member, powerLevels); const onMuteToggle = async (): Promise => { + if (isUpdating) return; // only allow one operation at a time + startUpdating(); + const roomId = member.roomId; const target = member.userId; // if muting self, warn as it may be irreversible if (target === cli.getUserId()) { try { - if (!(await warnSelfDemote(room?.isSpaceRoom()))) return; + if (!(await warnSelfDemote(room?.isSpaceRoom()))) { + stopUpdating(); + return; + } } catch (e) { logger.error("Failed to warn about self demotion: ", e); + stopUpdating(); return; } } const powerLevelEvent = room.currentState.getStateEvents("m.room.power_levels", ""); - if (!powerLevelEvent) return; + if (!powerLevelEvent) { + stopUpdating(); + return; + } const powerLevels = powerLevelEvent.getContent(); const levelToSend = @@ -900,27 +933,29 @@ const MuteToggleButton: React.FC = ({ member, room, powerLevels, } level = parseInt(level); - if (!isNaN(level)) { - startUpdating(); - cli.setPowerLevel(roomId, target, level, powerLevelEvent) - .then( - () => { - // NO-OP; rely on the m.room.member event coming down else we could - // get out of sync if we force setState here! - logger.log("Mute toggle success"); - }, - function (err) { - logger.error("Mute error: " + err); - Modal.createDialog(ErrorDialog, { - title: _t("Error"), - description: _t("Failed to mute user"), - }); - }, - ) - .finally(() => { - stopUpdating(); - }); + if (isNaN(level)) { + stopUpdating(); + return; } + + cli.setPowerLevel(roomId, target, level, powerLevelEvent) + .then( + () => { + // NO-OP; rely on the m.room.member event coming down else we could + // get out of sync if we force setState here! + logger.log("Mute toggle success"); + }, + function (err) { + logger.error("Mute error: " + err); + Modal.createDialog(ErrorDialog, { + title: _t("Error"), + description: _t("Failed to mute user"), + }); + }, + ) + .finally(() => { + stopUpdating(); + }); }; const classes = classNames("mx_UserInfo_field", { @@ -929,7 +964,7 @@ const MuteToggleButton: React.FC = ({ member, room, powerLevels, const muteLabel = muted ? _t("Unmute") : _t("Mute"); return ( - + {muteLabel} ); @@ -939,6 +974,7 @@ export const RoomAdminToolsContainer: React.FC = ({ room, children, member, + isUpdating, startUpdating, stopUpdating, powerLevels, @@ -966,17 +1002,34 @@ export const RoomAdminToolsContainer: React.FC = ({ if (!isMe && canAffectUser && me.powerLevel >= kickPowerLevel) { kickButton = ( - + ); } if (me.powerLevel >= redactPowerLevel && !room.isSpaceRoom()) { redactButton = ( - + ); } if (!isMe && canAffectUser && me.powerLevel >= banPowerLevel) { banButton = ( - + ); } if (!isMe && canAffectUser && me.powerLevel >= Number(editPowerLevel) && !room.isSpaceRoom()) { @@ -985,6 +1038,7 @@ export const RoomAdminToolsContainer: React.FC = ({ member={member} room={room} powerLevels={powerLevels} + isUpdating={isUpdating} startUpdating={startUpdating} stopUpdating={stopUpdating} /> @@ -1393,6 +1447,7 @@ const BasicUserInfo: React.FC<{ powerLevels={powerLevels} member={member as RoomMember} room={room} + isUpdating={pendingUpdateCount > 0} startUpdating={startUpdating} stopUpdating={stopUpdating} > From 59f0a9b6fd8d4b948c2dc6d0d5e2a7dd6bb5a7f6 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 13 Jul 2023 13:26:49 +0100 Subject: [PATCH 2/6] Iterate --- .../views/right_panel/UserInfo-test.tsx | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/test/components/views/right_panel/UserInfo-test.tsx b/test/components/views/right_panel/UserInfo-test.tsx index 28b072a5d87..8aad458f0ab 100644 --- a/test/components/views/right_panel/UserInfo-test.tsx +++ b/test/components/views/right_panel/UserInfo-test.tsx @@ -901,7 +901,13 @@ describe("", () => { let defaultProps: Parameters[0]; beforeEach(() => { - defaultProps = { room: mockRoom, member: defaultMember, startUpdating: jest.fn(), stopUpdating: jest.fn() }; + defaultProps = { + room: mockRoom, + member: defaultMember, + startUpdating: jest.fn(), + stopUpdating: jest.fn(), + isUpdating: false, + }; }); const renderComponent = (props = {}) => { @@ -1002,7 +1008,13 @@ describe("", () => { const memberWithBanMembership = { ...defaultMember, membership: "ban" }; let defaultProps: Parameters[0]; beforeEach(() => { - defaultProps = { room: mockRoom, member: defaultMember, startUpdating: jest.fn(), stopUpdating: jest.fn() }; + defaultProps = { + room: mockRoom, + member: defaultMember, + startUpdating: jest.fn(), + stopUpdating: jest.fn(), + isUpdating: false, + }; }); const renderComponent = (props = {}) => { @@ -1130,6 +1142,7 @@ describe("", () => { defaultProps = { room: mockRoom, member: defaultMember, + isUpdating: false, startUpdating: jest.fn(), stopUpdating: jest.fn(), powerLevels: {}, From ec277b40ebb9792e75899ed6bc3837b8ea028565 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 14 Jul 2023 11:21:58 +0100 Subject: [PATCH 3/6] Improve coverage --- .../views/right_panel/UserInfo-test.tsx | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/test/components/views/right_panel/UserInfo-test.tsx b/test/components/views/right_panel/UserInfo-test.tsx index 8aad458f0ab..a55627f9afb 100644 --- a/test/components/views/right_panel/UserInfo-test.tsx +++ b/test/components/views/right_panel/UserInfo-test.tsx @@ -1205,7 +1205,29 @@ describe("", () => { powerLevels: { events: { "m.room.power_levels": 1 } }, }); - expect(screen.getByText(/mute/i)).toBeInTheDocument(); + const button = screen.getByText(/mute/i); + expect(button).toBeInTheDocument(); + fireEvent.click(button); + expect(defaultProps.startUpdating).toHaveBeenCalled(); + }); + + it("should disable buttons when isUpdating=true", () => { + const mockMeMember = new RoomMember(mockRoom.roomId, "arbitraryId"); + mockMeMember.powerLevel = 51; // defaults to 50 + mockRoom.getMember.mockReturnValueOnce(mockMeMember); + + const defaultMemberWithPowerLevelAndJoinMembership = { ...defaultMember, powerLevel: 0, membership: "join" }; + + renderComponent({ + member: defaultMemberWithPowerLevelAndJoinMembership, + powerLevels: { events: { "m.room.power_levels": 1 } }, + isUpdating: true, + }); + + const button = screen.getByText(/mute/i); + expect(button).toBeInTheDocument(); + expect(button).toHaveAttribute("disabled"); + expect(button).toHaveAttribute("aria-disabled", "true"); }); }); From c284a92afd3e09071769806a88eee67165a7be7d Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 14 Jul 2023 12:44:38 +0100 Subject: [PATCH 4/6] Improve coverage --- .../views/right_panel/UserInfo-test.tsx | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/components/views/right_panel/UserInfo-test.tsx b/test/components/views/right_panel/UserInfo-test.tsx index a55627f9afb..4a4ac6ca4fe 100644 --- a/test/components/views/right_panel/UserInfo-test.tsx +++ b/test/components/views/right_panel/UserInfo-test.tsx @@ -61,6 +61,7 @@ import { E2EStatus } from "../../../../src/utils/ShieldUtils"; import { DirectoryMember, startDmOnFirstMessage } from "../../../../src/utils/direct-messages"; import { clearAllModals, flushPromises } from "../../../test-utils"; import ErrorDialog from "../../../../src/components/views/dialogs/ErrorDialog"; +import QuestionDialog from "../../../../src/components/views/dialogs/QuestionDialog"; jest.mock("../../../../src/utils/direct-messages", () => ({ ...jest.requireActual("../../../../src/utils/direct-messages"), @@ -1229,6 +1230,31 @@ describe("", () => { expect(button).toHaveAttribute("disabled"); expect(button).toHaveAttribute("aria-disabled", "true"); }); + + it("should warn when muting self", () => { + const spy = jest.spyOn(Modal, "createDialog").mockImplementation(() => ({ + finished: Promise.resolve([false]), + close: jest.fn(), + })); + + const mockMeMember = new RoomMember(mockRoom.roomId, mockClient.getSafeUserId()); + mockMeMember.powerLevel = 51; // defaults to 50 + mockRoom.getMember.mockReturnValueOnce(mockMeMember); + + renderComponent({ + member: mockMeMember, + powerLevels: { events: { "m.room.power_levels": 1 } }, + }); + + const button = screen.getByText(/mute/i); + fireEvent.click(button); + expect(spy).toHaveBeenCalledWith( + QuestionDialog, + expect.objectContaining({ + title: "Demote yourself?", + }), + ); + }); }); describe("disambiguateDevices", () => { From 69c785c3eca1e1f3482f12d137676832ed657db0 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 14 Jul 2023 14:26:42 +0100 Subject: [PATCH 5/6] Simplify --- src/components/views/right_panel/UserInfo.tsx | 15 +------------- .../views/right_panel/UserInfo-test.tsx | 20 ++++--------------- 2 files changed, 5 insertions(+), 30 deletions(-) diff --git a/src/components/views/right_panel/UserInfo.tsx b/src/components/views/right_panel/UserInfo.tsx index 42a88f8d968..d3f99fba060 100644 --- a/src/components/views/right_panel/UserInfo.tsx +++ b/src/components/views/right_panel/UserInfo.tsx @@ -879,6 +879,7 @@ interface IBaseRoomProps extends IBaseProps { children?: ReactNode; } +// We do not show a Mute button for ourselves so it doesn't need to handle warning self demotion const MuteToggleButton: React.FC = ({ member, room, @@ -900,20 +901,6 @@ const MuteToggleButton: React.FC = ({ const roomId = member.roomId; const target = member.userId; - // if muting self, warn as it may be irreversible - if (target === cli.getUserId()) { - try { - if (!(await warnSelfDemote(room?.isSpaceRoom()))) { - stopUpdating(); - return; - } - } catch (e) { - logger.error("Failed to warn about self demotion: ", e); - stopUpdating(); - return; - } - } - const powerLevelEvent = room.currentState.getStateEvents("m.room.power_levels", ""); if (!powerLevelEvent) { stopUpdating(); diff --git a/test/components/views/right_panel/UserInfo-test.tsx b/test/components/views/right_panel/UserInfo-test.tsx index 4a4ac6ca4fe..2062e9d92fb 100644 --- a/test/components/views/right_panel/UserInfo-test.tsx +++ b/test/components/views/right_panel/UserInfo-test.tsx @@ -61,7 +61,6 @@ import { E2EStatus } from "../../../../src/utils/ShieldUtils"; import { DirectoryMember, startDmOnFirstMessage } from "../../../../src/utils/direct-messages"; import { clearAllModals, flushPromises } from "../../../test-utils"; import ErrorDialog from "../../../../src/components/views/dialogs/ErrorDialog"; -import QuestionDialog from "../../../../src/components/views/dialogs/QuestionDialog"; jest.mock("../../../../src/utils/direct-messages", () => ({ ...jest.requireActual("../../../../src/utils/direct-messages"), @@ -1231,29 +1230,18 @@ describe("", () => { expect(button).toHaveAttribute("aria-disabled", "true"); }); - it("should warn when muting self", () => { - const spy = jest.spyOn(Modal, "createDialog").mockImplementation(() => ({ - finished: Promise.resolve([false]), - close: jest.fn(), - })); - + it("should not show mute button for one's own member", () => { const mockMeMember = new RoomMember(mockRoom.roomId, mockClient.getSafeUserId()); mockMeMember.powerLevel = 51; // defaults to 50 mockRoom.getMember.mockReturnValueOnce(mockMeMember); renderComponent({ member: mockMeMember, - powerLevels: { events: { "m.room.power_levels": 1 } }, + powerLevels: { events: { "m.room.power_levels": 100 } }, }); - const button = screen.getByText(/mute/i); - fireEvent.click(button); - expect(spy).toHaveBeenCalledWith( - QuestionDialog, - expect.objectContaining({ - title: "Demote yourself?", - }), - ); + const button = screen.queryByText(/mute/i); + expect(button).not.toBeInTheDocument(); }); }); From d981df2fe2e9286811ef963e5d0447c304f973a0 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 14 Jul 2023 15:24:50 +0100 Subject: [PATCH 6/6] Simplify --- src/components/views/right_panel/UserInfo.tsx | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/components/views/right_panel/UserInfo.tsx b/src/components/views/right_panel/UserInfo.tsx index d3f99fba060..9a74cc60571 100644 --- a/src/components/views/right_panel/UserInfo.tsx +++ b/src/components/views/right_panel/UserInfo.tsx @@ -902,14 +902,8 @@ const MuteToggleButton: React.FC = ({ const target = member.userId; const powerLevelEvent = room.currentState.getStateEvents("m.room.power_levels", ""); - if (!powerLevelEvent) { - stopUpdating(); - return; - } - - const powerLevels = powerLevelEvent.getContent(); - const levelToSend = - (powerLevels.events ? powerLevels.events["m.room.message"] : null) || powerLevels.events_default; + const powerLevels = powerLevelEvent?.getContent(); + const levelToSend = powerLevels?.events?.["m.room.message"] ?? powerLevels?.events_default; let level; if (muted) { // unmute