Examine all m.direct rooms to find a DM as fallback (#10127)

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<string> {
+        return Object.values(this.mDirectEvent).reduce((prevRoomIds: Set<string>, roomIds: string[]): Set<string> => {
+            roomIds.forEach((roomId) => prevRoomIds.add(roomId));
+            return prevRoomIds;
+        }, new Set<string>());
+    }
+
     private getUserToRooms(): { [key: string]: string[] } {
         if (!this.userToRooms) {
             const userToRooms = this.mDirectEvent;

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);
 }

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 {

test/LegacyCallHandler-test.ts

@@ -238,7 +238,7 @@ describe("LegacyCallHandler", () => {
                     return [];
                 }
             },
-        } as DMRoomMap;
+        } as unknown as DMRoomMap;
         DMRoomMap.setShared(dmRoomMap);
 
         pstnLookup = null;

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<MatrixClient>;
+    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]));
+    });
+});

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 {
+            return (
+                {
                     [room1.roomId]: room1,
                     [room2.roomId]: room2,
                     [room3.roomId]: room3,
                     [room4.roomId]: room4,
                     [room5.roomId]: room5,
-            }[roomId];
+                    [room6.roomId]: room6,
+                }[roomId] || null
+            );
         });
 
         dmRoomMap = {
             getDMRoomForIdentifiers: jest.fn(),
             getDMRoomsForUserId: jest.fn(),
-        } as unknown as DMRoomMap;
-        jest.spyOn(DMRoomMap, "shared").mockReturnValue(dmRoomMap);
-        mocked(dmRoomMap.getDMRoomsForUserId).mockReturnValue([
+            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).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);
+    });
 });

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();
     });