From a6eee32c66b1184d0bc8fb8e64d76a5c2b1fb45c Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Mon, 13 Feb 2023 08:46:53 +0100 Subject: [PATCH] Examine all m.direct rooms to find a DM as fallback (#10127) --- src/utils/DMRoomMap.ts | 10 +++++ src/utils/dm/findDMForUser.ts | 46 +++++++++++++++------- src/utils/dm/findDMRoom.ts | 2 +- test/LegacyCallHandler-test.ts | 2 +- test/utils/DMRoomMap-test.ts | 54 +++++++++++++++++++++++++ test/utils/dm/findDMForUser-test.ts | 61 ++++++++++++++++++++++------- test/utils/dm/findDMRoom-test.ts | 4 +- 7 files changed, 147 insertions(+), 32 deletions(-) create mode 100644 test/utils/DMRoomMap-test.ts diff --git a/src/utils/DMRoomMap.ts b/src/utils/DMRoomMap.ts index 9d6755b4ec..24f67c2ff2 100644 --- a/src/utils/DMRoomMap.ts +++ b/src/utils/DMRoomMap.ts @@ -192,6 +192,16 @@ export default class DMRoomMap { .reduce((obj, r) => (obj[r.userId] = r.room) && obj, {}); } + /** + * @returns all room Ids from m.direct + */ + public getRoomIds(): Set { + return Object.values(this.mDirectEvent).reduce((prevRoomIds: Set, roomIds: string[]): Set => { + roomIds.forEach((roomId) => prevRoomIds.add(roomId)); + return prevRoomIds; + }, new Set()); + } + private getUserToRooms(): { [key: string]: string[] } { if (!this.userToRooms) { const userToRooms = this.mDirectEvent; diff --git a/src/utils/dm/findDMForUser.ts b/src/utils/dm/findDMForUser.ts index babf8bd2af..3f6fcbfca0 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().getDMRoomsForUserId(userId); - const rooms = 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, @@ -44,7 +35,7 @@ export function findDMForUser(client: MatrixClient, userId: string): Room { const functionalUsers = getFunctionalMembers(r); const members = r.currentState.getMembers(); const joinedMembers = members.filter( - (m) => !functionalUsers.includes(m.userId) && isJoinedOrNearlyJoined(m.membership), + (m) => !functionalUsers.includes(m.userId) && m.membership && isJoinedOrNearlyJoined(m.membership), ); const otherMember = joinedMembers.find((m) => m.userId === userId); return otherMember && joinedMembers.length === 2; @@ -54,7 +45,34 @@ 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)).filter((r): r is Room => r !== null); + 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)) + .filter((r): r is Room => r !== null); + return extractSuitableRoom(allRooms, userId); } diff --git a/src/utils/dm/findDMRoom.ts b/src/utils/dm/findDMRoom.ts index d8cbb56d90..7196b42fee 100644 --- a/src/utils/dm/findDMRoom.ts +++ b/src/utils/dm/findDMRoom.ts @@ -29,7 +29,7 @@ import { findDMForUser } from "./findDMForUser"; */ export function findDMRoom(client: MatrixClient, targets: Member[]): Room | null { const targetIds = targets.map((t) => t.userId); - let existingRoom: Room; + let existingRoom: Room | undefined; if (targetIds.length === 1) { existingRoom = findDMForUser(client, targetIds[0]); } else { diff --git a/test/LegacyCallHandler-test.ts b/test/LegacyCallHandler-test.ts index 3e21780dab..515402c795 100644 --- a/test/LegacyCallHandler-test.ts +++ b/test/LegacyCallHandler-test.ts @@ -238,7 +238,7 @@ describe("LegacyCallHandler", () => { return []; } }, - } as DMRoomMap; + } as unknown as DMRoomMap; DMRoomMap.setShared(dmRoomMap); pstnLookup = null; diff --git a/test/utils/DMRoomMap-test.ts b/test/utils/DMRoomMap-test.ts new file mode 100644 index 0000000000..9a3ddd951c --- /dev/null +++ b/test/utils/DMRoomMap-test.ts @@ -0,0 +1,54 @@ +/* +Copyright 2023 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, Mocked } from "jest-mock"; +import { EventType, IContent, MatrixClient } from "matrix-js-sdk/src/matrix"; + +import DMRoomMap from "../../src/utils/DMRoomMap"; +import { mkEvent, stubClient } from "../test-utils"; + +describe("DMRoomMap", () => { + const roomId1 = "!room1:example.com"; + const roomId2 = "!room2:example.com"; + const roomId3 = "!room3:example.com"; + const roomId4 = "!room4:example.com"; + + const mDirectContent = { + "user@example.com": [roomId1, roomId2], + "@user:example.com": [roomId1, roomId3, roomId4], + "@user2:example.com": [] as string[], + } satisfies IContent; + + let client: Mocked; + let dmRoomMap: DMRoomMap; + + beforeEach(() => { + client = mocked(stubClient()); + + const mDirectEvent = mkEvent({ + event: true, + type: EventType.Direct, + user: client.getSafeUserId(), + content: mDirectContent, + }); + client.getAccountData.mockReturnValue(mDirectEvent); + dmRoomMap = new DMRoomMap(client); + }); + + it("getRoomIds should return the room Ids", () => { + expect(dmRoomMap.getRoomIds()).toEqual(new Set([roomId1, roomId2, roomId3, roomId4])); + }); +}); diff --git a/test/utils/dm/findDMForUser-test.ts b/test/utils/dm/findDMForUser-test.ts index c1acad95e7..60c5b342f4 100644 --- a/test/utils/dm/findDMForUser-test.ts +++ b/test/utils/dm/findDMForUser-test.ts @@ -30,12 +30,15 @@ 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; + const room7Id = "!room7:example.com"; let dmRoomMap: DMRoomMap; let mockClient: MatrixClient; @@ -78,33 +81,56 @@ 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, - [room2.roomId]: room2, - [room3.roomId]: room3, - [room4.roomId]: room4, - [room5.roomId]: room5, - }[roomId]; + return ( + { + [room1.roomId]: room1, + [room2.roomId]: room2, + [room3.roomId]: room3, + [room4.roomId]: room4, + [room5.roomId]: room5, + [room6.roomId]: room6, + }[roomId] || null + ); }); 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, + room7Id, // this room does not exist in client + ]), + ), } as unknown as DMRoomMap; jest.spyOn(DMRoomMap, "shared").mockReturnValue(dmRoomMap); - mocked(dmRoomMap.getDMRoomsForUserId).mockReturnValue([ - room1.roomId, - room2.roomId, - room3.roomId, - room4.roomId, - room5.roomId, - ]); + mocked(dmRoomMap.getDMRoomsForUserId).mockImplementation((userId: string) => { + if (userId === userId1) { + return [room1.roomId, room2.roomId, room3.roomId, room4.roomId, room5.roomId, room7Id]; + } + + return []; + }); }); describe("for an empty DM room list", () => { beforeEach(() => { mocked(dmRoomMap.getDMRoomsForUserId).mockReturnValue([]); + mocked(dmRoomMap.getRoomIds).mockReturnValue(new Set()); }); it("should return undefined", () => { @@ -125,4 +151,11 @@ describe("findDMForUser", () => { expect(findDMForUser(mockClient, userId1)).toBe(room3); }); + + it("should find a room by the 'all rooms' fallback", () => { + room1.getLastActiveTimestamp = () => 1; + room6.getLastActiveTimestamp = () => 2; + + expect(findDMForUser(mockClient, userId3)).toBe(room6); + }); }); diff --git a/test/utils/dm/findDMRoom-test.ts b/test/utils/dm/findDMRoom-test.ts index b7ed77f582..8ce301cee8 100644 --- a/test/utils/dm/findDMRoom-test.ts +++ b/test/utils/dm/findDMRoom-test.ts @@ -53,8 +53,8 @@ describe("findDMRoom", () => { expect(findDMRoom(mockClient, [member1])).toBe(room1); }); - it("should return null for a single target without a room", () => { - mocked(findDMForUser).mockReturnValue(null); + it("should return undefined for a single target without a room", () => { + mocked(findDMForUser).mockReturnValue(undefined); expect(findDMRoom(mockClient, [member1])).toBeNull(); });