From 2dab42438aedbc684ce77f02d3bc37beb7b2b896 Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Fri, 10 Feb 2023 10:06:31 +0100 Subject: [PATCH] Implement as fallback --- src/utils/dm/findDMForUser.ts | 42 +++++++++----- test/LegacyCallHandler-test.ts | 14 ++++- test/utils/dm/findDMForUser-test.ts | 87 +++++++++++++++-------------- 3 files changed, 87 insertions(+), 56 deletions(-) diff --git a/src/utils/dm/findDMForUser.ts b/src/utils/dm/findDMForUser.ts index 3aa0cb8b9aa9..9c3f483972d2 100644 --- a/src/utils/dm/findDMForUser.ts +++ b/src/utils/dm/findDMForUser.ts @@ -21,17 +21,8 @@ import { isLocalRoom } from "../localRoom/isLocalRoom"; import { isJoinedOrNearlyJoined } from "../membership"; import { getFunctionalMembers } from "../room/getFunctionalMembers"; -/** - * Tries to find a DM room with a specific user. - * - * @param {MatrixClient} client - * @param {string} userId ID of the user to find the DM for - * @returns {Room} Room if found - */ -export function findDMForUser(client: MatrixClient, userId: string): Room { - const roomIds = DMRoomMap.shared().getRoomIds(); - const rooms = Array.from(roomIds).map((id) => client.getRoom(id)); - const suitableDMRooms = rooms +function extractSuitableRoom(rooms: Room[], userId: string): Room | undefined { + const suitableRooms = rooms .filter((r) => { // Validate that we are joined and the other person is also joined. We'll also make sure // that the room also looks like a DM (until we have canonical DMs to tell us). For now, @@ -54,7 +45,32 @@ export function findDMForUser(client: MatrixClient, userId: string): Room { .sort((r1, r2) => { return r2.getLastActiveTimestamp() - r1.getLastActiveTimestamp(); }); - if (suitableDMRooms.length) { - return suitableDMRooms[0]; + + if (suitableRooms.length) { + return suitableRooms[0]; } + + return undefined; +} + +/** + * Tries to find a DM room with a specific user. + * + * @param {MatrixClient} client + * @param {string} userId ID of the user to find the DM for + * @returns {Room | undefined} Room if found + */ +export function findDMForUser(client: MatrixClient, userId: string): Room | undefined { + const roomIdsForUserId = DMRoomMap.shared().getDMRoomsForUserId(userId); + const roomsForUserId = roomIdsForUserId.map((id) => client.getRoom(id)); + const suitableRoomForUserId = extractSuitableRoom(roomsForUserId, userId); + + if (suitableRoomForUserId) { + return suitableRoomForUserId; + } + + // Try to find in all rooms as a fallback + const allRoomIds = DMRoomMap.shared().getRoomIds(); + const allRooms = Array.from(allRoomIds).map((id) => client.getRoom(id)); + return extractSuitableRoom(allRooms, userId); } diff --git a/test/LegacyCallHandler-test.ts b/test/LegacyCallHandler-test.ts index 074ba7e39949..515402c7950a 100644 --- a/test/LegacyCallHandler-test.ts +++ b/test/LegacyCallHandler-test.ts @@ -225,8 +225,18 @@ describe("LegacyCallHandler", () => { return null; } }, - getRoomIds: () => { - return [NATIVE_ROOM_ALICE, NATIVE_ROOM_BOB, NATIVE_ROOM_CHARLIE, VIRTUAL_ROOM_BOB]; + getDMRoomsForUserId: (userId: string) => { + if (userId === NATIVE_ALICE) { + return [NATIVE_ROOM_ALICE]; + } else if (userId === NATIVE_BOB) { + return [NATIVE_ROOM_BOB]; + } else if (userId === NATIVE_CHARLIE) { + return [NATIVE_ROOM_CHARLIE]; + } else if (userId === VIRTUAL_BOB) { + return [VIRTUAL_ROOM_BOB]; + } else { + return []; + } }, } as unknown as DMRoomMap; DMRoomMap.setShared(dmRoomMap); diff --git a/test/utils/dm/findDMForUser-test.ts b/test/utils/dm/findDMForUser-test.ts index 3e08b97d3918..964dcd93a5d2 100644 --- a/test/utils/dm/findDMForUser-test.ts +++ b/test/utils/dm/findDMForUser-test.ts @@ -15,10 +15,10 @@ limitations under the License. */ import { mocked } from "jest-mock"; -import { EventType, MatrixClient, Room } from "matrix-js-sdk/src/matrix"; +import { MatrixClient, Room } from "matrix-js-sdk/src/matrix"; import DMRoomMap from "../../../src/utils/DMRoomMap"; -import { createTestClient, makeMembershipEvent, mkEvent } from "../../test-utils"; +import { createTestClient, makeMembershipEvent } from "../../test-utils"; import { LocalRoom } from "../../../src/models/LocalRoom"; import { findDMForUser } from "../../../src/utils/dm/findDMForUser"; import { getFunctionalMembers } from "../../../src/utils/room/getFunctionalMembers"; @@ -30,28 +30,17 @@ jest.mock("../../../src/utils/room/getFunctionalMembers", () => ({ describe("findDMForUser", () => { const userId1 = "@user1:example.com"; const userId2 = "@user2:example.com"; + const userId3 = "@user3:example.com"; const botId = "@bot:example.com"; let room1: Room; let room2: LocalRoom; let room3: Room; let room4: Room; let room5: Room; + let room6: Room; let dmRoomMap: DMRoomMap; let mockClient: MatrixClient; - const setUpMDirect = (mDirect: { [key: string]: string[] }) => { - const mDirectEvent = mkEvent({ - event: true, - type: EventType.Direct, - user: mockClient.getSafeUserId(), - content: mDirect, - }); - mocked(mockClient).getAccountData.mockReturnValue(mDirectEvent); - - dmRoomMap = new DMRoomMap(mockClient); - jest.spyOn(DMRoomMap, "shared").mockReturnValue(dmRoomMap); - }; - beforeEach(() => { mockClient = createTestClient(); @@ -91,6 +80,14 @@ describe("findDMForUser", () => { room5 = new Room("!room5:example.com", mockClient, userId1); room5.getLastActiveTimestamp = () => 100; + // room not correctly stored in userId → room map; should be found by the "all rooms" fallback + room6 = new Room("!room6:example.com", mockClient, userId1); + room6.getMyMembership = () => "join"; + room6.currentState.setStateEvents([ + makeMembershipEvent(room6.roomId, userId1, "join"), + makeMembershipEvent(room6.roomId, userId3, "join"), + ]); + mocked(mockClient.getRoom).mockImplementation((roomId: string) => { return { [room1.roomId]: room1, @@ -98,17 +95,33 @@ describe("findDMForUser", () => { [room3.roomId]: room3, [room4.roomId]: room4, [room5.roomId]: room5, + [room6.roomId]: room6, }[roomId]; }); - }); - afterAll(() => { - jest.restoreAllMocks(); + dmRoomMap = { + getDMRoomForIdentifiers: jest.fn(), + getDMRoomsForUserId: jest.fn(), + getRoomIds: jest + .fn() + .mockReturnValue( + new Set([room1.roomId, room2.roomId, room3.roomId, room4.roomId, room5.roomId, room6.roomId]), + ), + } as unknown as DMRoomMap; + jest.spyOn(DMRoomMap, "shared").mockReturnValue(dmRoomMap); + mocked(dmRoomMap.getDMRoomsForUserId).mockImplementation((userId: string) => { + if (userId === userId1) { + return [room1.roomId, room2.roomId, room3.roomId, room4.roomId, room5.roomId]; + } + + return []; + }); }); describe("for an empty DM room list", () => { beforeEach(() => { - setUpMDirect({}); + mocked(dmRoomMap.getDMRoomsForUserId).mockReturnValue([]); + mocked(dmRoomMap.getRoomIds).mockReturnValue(new Set()); }); it("should return undefined", () => { @@ -116,32 +129,24 @@ describe("findDMForUser", () => { }); }); - describe("when there are soom rooms", () => { - beforeEach(() => { - setUpMDirect({ - [userId1]: [room1.roomId, room2.roomId, room3.roomId, room4.roomId, room5.roomId], - }); - }); + it("should find a room ordered by last activity 1", () => { + room1.getLastActiveTimestamp = () => 2; + room3.getLastActiveTimestamp = () => 1; - it("should find a room ordered by last activity 1", () => { - room1.getLastActiveTimestamp = () => 2; - room3.getLastActiveTimestamp = () => 1; - - expect(findDMForUser(mockClient, userId1)).toBe(room1); - }); + expect(findDMForUser(mockClient, userId1)).toBe(room1); + }); - it("should find a room ordered by last activity 2", () => { - room1.getLastActiveTimestamp = () => 1; - room3.getLastActiveTimestamp = () => 2; + it("should find a room ordered by last activity 2", () => { + room1.getLastActiveTimestamp = () => 1; + room3.getLastActiveTimestamp = () => 2; - expect(findDMForUser(mockClient, userId1)).toBe(room3); - }); + expect(findDMForUser(mockClient, userId1)).toBe(room3); + }); - it("should find a room for a user without an m.direct entry but a DM-like room exists", () => { - room1.getLastActiveTimestamp = () => 1; - room3.getLastActiveTimestamp = () => 2; + it("should find a room by the 'all rooms' fallback", () => { + room1.getLastActiveTimestamp = () => 1; + room6.getLastActiveTimestamp = () => 2; - expect(findDMForUser(mockClient, userId2)).toBe(room3); - }); + expect(findDMForUser(mockClient, userId3)).toBe(room6); }); });