From bf72b913891e00c2d6779070cf24bd6956a99f22 Mon Sep 17 00:00:00 2001 From: ElementRobot Date: Wed, 21 Dec 2022 17:10:42 +0000 Subject: [PATCH] [Backport staging] Prevent unnecessary m.direct updates (#9817) * Prevent unnecessary m.direct updates (#9805) * Prevent unnecessary m.direct updates Signed-off-by: Michael Weimann * Replace object with Map * Clean up comment; simplify code * Fix rte flaky test (#9811) * Snap in PiP widget when content changed (#9797) * Check modified at the end of setDMRoom * Revert "Snap in PiP widget when content changed (#9797)" This reverts commit be1e5753677bdf84b3657994dc5ad20d250038bd. * Revert "Fix rte flaky test (#9811)" This reverts commit 20d9eb3924c5ebc63f03cb61b172035725e3474d. * Update src/Rooms.ts Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Signed-off-by: Michael Weimann Co-authored-by: Florian Duros Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> (cherry picked from commit 93dd010809c4d960fe69a51def87f8565c8d448d) * Use a function that exists on staging Co-authored-by: Michael Weimann Co-authored-by: Travis Ralston --- src/Rooms.ts | 27 ++++---- test/Rooms-test.ts | 151 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 11 deletions(-) create mode 100644 test/Rooms-test.ts diff --git a/src/Rooms.ts b/src/Rooms.ts index b3c1a55328..8d10f121af 100644 --- a/src/Rooms.ts +++ b/src/Rooms.ts @@ -57,42 +57,47 @@ export function guessAndSetDMRoom(room: Room, isDirect: boolean): Promise /** * Marks or unmarks the given room as being as a DM room. * @param {string} roomId The ID of the room to modify - * @param {string} userId The user ID of the desired DM - room target user or null to un-mark - this room as a DM room + * @param {string | null} userId The user ID of the desired DM room target user or + * null to un-mark this room as a DM room * @returns {object} A promise */ -export async function setDMRoom(roomId: string, userId: string): Promise { +export async function setDMRoom(roomId: string, userId: string | null): Promise { if (MatrixClientPeg.get().isGuest()) return; const mDirectEvent = MatrixClientPeg.get().getAccountData(EventType.Direct); - let dmRoomMap = {}; + const currentContent = mDirectEvent?.getContent() || {}; - if (mDirectEvent !== undefined) dmRoomMap = { ...mDirectEvent.getContent() }; // copy as we will mutate + const dmRoomMap = new Map(Object.entries(currentContent)); + let modified = false; // remove it from the lists of any others users // (it can only be a DM room for one person) - for (const thisUserId of Object.keys(dmRoomMap)) { - const roomList = dmRoomMap[thisUserId]; + for (const thisUserId of dmRoomMap.keys()) { + const roomList = dmRoomMap.get(thisUserId) || []; if (thisUserId != userId) { const indexOfRoom = roomList.indexOf(roomId); if (indexOfRoom > -1) { roomList.splice(indexOfRoom, 1); + modified = true; } } } // now add it, if it's not already there if (userId) { - const roomList = dmRoomMap[userId] || []; + const roomList = dmRoomMap.get(userId) || []; if (roomList.indexOf(roomId) == -1) { roomList.push(roomId); + modified = true; } - dmRoomMap[userId] = roomList; + dmRoomMap.set(userId, roomList); } - await MatrixClientPeg.get().setAccountData(EventType.Direct, dmRoomMap); + // prevent unnecessary calls to setAccountData + if (!modified) return; + + await MatrixClientPeg.get().setAccountData(EventType.Direct, Object.fromEntries(dmRoomMap)); } /** diff --git a/test/Rooms-test.ts b/test/Rooms-test.ts new file mode 100644 index 0000000000..dec982daf3 --- /dev/null +++ b/test/Rooms-test.ts @@ -0,0 +1,151 @@ +/* +Copyright 2022 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 { mocked } from "jest-mock"; +import { EventType, MatrixClient, MatrixEvent } from "matrix-js-sdk/src/matrix"; + +import { setDMRoom } from "../src/Rooms"; +import { mkEvent, stubClient } from "./test-utils"; + +describe("setDMRoom", () => { + const userId1 = "@user1:example.com"; + const userId2 = "@user2:example.com"; + const userId3 = "@user3:example.com"; + const roomId1 = "!room1:example.com"; + const roomId2 = "!room2:example.com"; + const roomId3 = "!room3:example.com"; + const roomId4 = "!room4:example.com"; + let client: MatrixClient; + + beforeEach(() => { + client = mocked(stubClient()); + client.getAccountData = jest.fn().mockImplementation((eventType: string): MatrixEvent | undefined => { + if (eventType === EventType.Direct) { + return mkEvent({ + event: true, + content: { + [userId1]: [roomId1, roomId2], + [userId2]: [roomId3], + }, + type: EventType.Direct, + user: client.getUserId(), + }); + } + + return undefined; + }); + }); + + describe("when logged in as a guest and marking a room as DM", () => { + beforeEach(() => { + mocked(client.isGuest).mockReturnValue(true); + setDMRoom(roomId1, userId1); + }); + + it("should not update the account data", () => { + expect(client.setAccountData).not.toHaveBeenCalled(); + }); + }); + + describe("when adding a new room to an existing DM relation", () => { + beforeEach(() => { + setDMRoom(roomId4, userId1); + }); + + it("should update the account data accordingly", () => { + expect(client.setAccountData).toHaveBeenCalledWith(EventType.Direct, { + [userId1]: [roomId1, roomId2, roomId4], + [userId2]: [roomId3], + }); + }); + }); + + describe("when adding a new DM room", () => { + beforeEach(() => { + setDMRoom(roomId4, userId3); + }); + + it("should update the account data accordingly", () => { + expect(client.setAccountData).toHaveBeenCalledWith(EventType.Direct, { + [userId1]: [roomId1, roomId2], + [userId2]: [roomId3], + [userId3]: [roomId4], + }); + }); + }); + + describe("when trying to add a DM, that already exists", () => { + beforeEach(() => { + setDMRoom(roomId1, userId1); + }); + + it("should not update the account data", () => { + expect(client.setAccountData).not.toHaveBeenCalled(); + }); + }); + + describe("when removing an existing DM", () => { + beforeEach(() => { + setDMRoom(roomId1, null); + }); + + it("should update the account data accordingly", () => { + expect(client.setAccountData).toHaveBeenCalledWith(EventType.Direct, { + [userId1]: [roomId2], + [userId2]: [roomId3], + }); + }); + }); + + describe("when removing an unknown room", () => { + beforeEach(() => { + setDMRoom(roomId4, null); + }); + + it("should not update the account data", () => { + expect(client.setAccountData).not.toHaveBeenCalled(); + }); + }); + + describe("when the direct event is undefined", () => { + beforeEach(() => { + mocked(client.getAccountData).mockReturnValue(undefined); + setDMRoom(roomId1, userId1); + }); + + it("should update the account data accordingly", () => { + expect(client.setAccountData).toHaveBeenCalledWith(EventType.Direct, { + [userId1]: [roomId1], + }); + }); + }); + + describe("when the current content is undefined", () => { + beforeEach(() => { + // @ts-ignore + mocked(client.getAccountData).mockReturnValue({ + getContent: jest.fn(), + }); + setDMRoom(roomId1, userId1); + }); + + it("should update the account data accordingly", () => { + expect(client.setAccountData).toHaveBeenCalledWith(EventType.Direct, { + [userId1]: [roomId1], + }); + }); + }); +});