Exclude functional members from DM detection (#9124)

* Exclude functional members from DM detection

* Fix getRoomFunctionalMembers test

* Simplify getFunctionalMembers

* Remove unnecessary filter

src/CallHandler.tsx

@@ -61,6 +61,7 @@ import { ViewRoomPayload } from "./dispatcher/payloads/ViewRoomPayload";
 import { KIND_CALL_TRANSFER } from "./components/views/dialogs/InviteDialogTypes";
 import { OpenInviteDialogPayload } from "./dispatcher/payloads/OpenInviteDialogPayload";
 import { findDMForUser } from './utils/dm/findDMForUser';
+import { getJoinedNonFunctionalMembers } from './utils/room/getJoinedNonFunctionalMembers';
 
 export const PROTOCOL_PSTN = 'm.protocol.pstn';
 export const PROTOCOL_PSTN_PREFIXED = 'im.vector.protocol.pstn';
@@ -861,7 +862,7 @@ export default class CallHandler extends EventEmitter {
         // We leave the check for whether there's already a call in this room until later,
         // otherwise it can race.
 
-        const members = room.getJoinedMembers();
+        const members = getJoinedNonFunctionalMembers(room);
         if (members.length <= 1) {
             Modal.createDialog(ErrorDialog, {
                 description: _t('You cannot place a call with yourself.'),

src/utils/dm/findDMForUser.ts

@@ -19,6 +19,7 @@ import { MatrixClient, Room } from "matrix-js-sdk/src/matrix";
 import DMRoomMap from "../DMRoomMap";
 import { isLocalRoom } from "../localRoom/isLocalRoom";
 import { isJoinedOrNearlyJoined } from "../membership";
+import { getFunctionalMembers } from "../room/getFunctionalMembers";
 
 /**
  * Tries to find a DM room with a specific user.
@@ -39,8 +40,11 @@ export function findDMForUser(client: MatrixClient, userId: string): Room {
         if (r && r.getMyMembership() === "join") {
             if (isLocalRoom(r)) return false;
 
+            const functionalUsers = getFunctionalMembers(r);
             const members = r.currentState.getMembers();
-            const joinedMembers = members.filter(m => isJoinedOrNearlyJoined(m.membership));
+            const joinedMembers = members.filter(
+                m => !functionalUsers.includes(m.userId) && isJoinedOrNearlyJoined(m.membership),
+            );
             const otherMember = joinedMembers.find(m => m.userId === userId);
             return otherMember && joinedMembers.length === 2;
         }

src/utils/room/getFunctionalMembers.ts

@@ -0,0 +1,27 @@
+/*
+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 { Room, UNSTABLE_ELEMENT_FUNCTIONAL_USERS } from "matrix-js-sdk/src/matrix";
+
+export const getFunctionalMembers = (room: Room): string[] => {
+    const [functionalUsersStateEvent] = room.currentState.getStateEvents(UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name);
+
+    if (Array.isArray(functionalUsersStateEvent?.getContent().service_members)) {
+        return functionalUsersStateEvent.getContent().service_members;
+    }
+
+    return [];
+};

src/utils/room/getJoinedNonFunctionalMembers.ts

@@ -0,0 +1,27 @@
+/*
+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 { Room, RoomMember } from "matrix-js-sdk/src/matrix";
+
+import { getFunctionalMembers } from "./getFunctionalMembers";
+
+/**
+ * Returns all room members that are non-functional (bots etc.).
+ */
+export const getJoinedNonFunctionalMembers = (room: Room): RoomMember[] => {
+    const functionalMembers = getFunctionalMembers(room);
+    return room.getJoinedMembers().filter(m => !functionalMembers.includes(m.userId));
+};

test/CallHandler-test.ts

@@ -17,6 +17,7 @@ limitations under the License.
 import { IProtocol } from 'matrix-js-sdk/src/matrix';
 import { CallEvent, CallState, CallType } from 'matrix-js-sdk/src/webrtc/call';
 import EventEmitter from 'events';
+import { mocked } from 'jest-mock';
 
 import CallHandler, {
     CallHandlerEvent, PROTOCOL_PSTN, PROTOCOL_PSTN_PREFIXED, PROTOCOL_SIP_NATIVE, PROTOCOL_SIP_VIRTUAL,
@@ -26,6 +27,11 @@ import { MatrixClientPeg } from '../src/MatrixClientPeg';
 import DMRoomMap from '../src/utils/DMRoomMap';
 import SdkConfig from '../src/SdkConfig';
 import { Action } from "../src/dispatcher/actions";
+import { getFunctionalMembers } from "../src/utils/room/getFunctionalMembers";
+
+jest.mock("../src/utils/room/getFunctionalMembers", () => ({
+    getFunctionalMembers: jest.fn(),
+}));
 
 // The Matrix IDs that the user sees when talking to Alice & Bob
 const NATIVE_ALICE = "@alice:example.org";
@@ -41,6 +47,8 @@ const NATIVE_ROOM_ALICE = "$alice_room:example.org";
 const NATIVE_ROOM_BOB = "$bob_room:example.org";
 const NATIVE_ROOM_CHARLIE = "$charlie_room:example.org";
 
+const FUNCTIONAL_USER = "@bot:example.com";
+
 // The room we use to talk to virtual Bob (but that the user does not see)
 // Bob has a virtual room, but Alice doesn't
 const VIRTUAL_ROOM_BOB = "$virtual_bob_room:example.org";
@@ -69,9 +77,17 @@ function mkStubDM(roomId, userId) {
             getAvatarUrl: () => 'mxc://avatar.url/image.png',
             getMxcAvatarUrl: () => 'mxc://avatar.url/image.png',
         },
+        {
+            userId: FUNCTIONAL_USER,
+            name: 'Bot user',
+            rawDisplayName: 'Bot user',
+            roomId: roomId,
+            membership: 'join',
+            getAvatarUrl: () => 'mxc://avatar.url/image.png',
+            getMxcAvatarUrl: () => 'mxc://avatar.url/image.png',
+        },
     ]);
     room.currentState.getMembers = room.getJoinedMembers;
-
     return room;
 }
 
@@ -132,6 +148,10 @@ describe('CallHandler', () => {
         callHandler = new CallHandler();
         callHandler.start();
 
+        mocked(getFunctionalMembers).mockReturnValue([
+            FUNCTIONAL_USER,
+        ]);
+
         const nativeRoomAlice = mkStubDM(NATIVE_ROOM_ALICE, NATIVE_ALICE);
         const nativeRoomBob = mkStubDM(NATIVE_ROOM_BOB, NATIVE_BOB);
         const nativeRoomCharie = mkStubDM(NATIVE_ROOM_CHARLIE, NATIVE_CHARLIE);

test/utils/dm/findDMForUser-test.ts

@@ -17,56 +17,112 @@ limitations under the License.
 import { mocked } from "jest-mock";
 import { MatrixClient, Room } from "matrix-js-sdk/src/matrix";
 
-import { DirectoryMember, ThreepidMember } from "../../../src/utils/direct-messages";
-import { MatrixClientPeg } from "../../../src/MatrixClientPeg";
 import DMRoomMap from "../../../src/utils/DMRoomMap";
-import { createTestClient } from "../../test-utils";
-import { findDMRoom } from "../../../src/utils/dm/findDMRoom";
+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";
 
-jest.mock("../../../src/utils/dm/findDMForUser", () => ({
-    findDMForUser: jest.fn(),
+jest.mock("../../../src/utils/room/getFunctionalMembers", () => ({
+    getFunctionalMembers: jest.fn(),
 }));
 
-describe("findDMRoom", () => {
+describe("findDMForUser", () => {
     const userId1 = "@user1:example.com";
-    const member1 = new DirectoryMember({ user_id: userId1 });
-    const member2 = new ThreepidMember("user2");
-    let mockClient: MatrixClient;
+    const userId2 = "@user2:example.com";
+    const botId = "@bot:example.com";
     let room1: Room;
+    let room2: LocalRoom;
+    let room3: Room;
+    let room4: Room;
+    let room5: Room;
     let dmRoomMap: DMRoomMap;
+    let mockClient: MatrixClient;
 
     beforeEach(() => {
         mockClient = createTestClient();
-        jest.spyOn(MatrixClientPeg, "get").mockReturnValue(mockClient);
+
+        // always return the bot user as functional member
+        mocked(getFunctionalMembers).mockReturnValue([botId]);
 
         room1 = new Room("!room1:example.com", mockClient, userId1);
         room1.getMyMembership = () => "join";
+        room1.currentState.setStateEvents([
+            makeMembershipEvent(room1.roomId, userId1, "join"),
+            makeMembershipEvent(room1.roomId, userId2, "join"),
+        ]);
+
+        // this should not be a DM room because it is a local room
+        room2 = new LocalRoom("!room2:example.com", mockClient, userId1);
+        room2.getMyMembership = () => "join";
+        room2.getLastActiveTimestamp = () => 100;
+
+        room3 = new Room("!room3:example.com", mockClient, userId1);
+        room3.getMyMembership = () => "join";
+        room3.currentState.setStateEvents([
+            makeMembershipEvent(room3.roomId, userId1, "join"),
+            makeMembershipEvent(room3.roomId, userId2, "join"),
+            // Adding the bot user here. Should be excluded when determining if the room is a DM.
+            makeMembershipEvent(room3.roomId, botId, "join"),
+        ]);
+
+        // this should not be a DM room because it has only one joined user
+        room4 = new Room("!room4:example.com", mockClient, userId1);
+        room4.getMyMembership = () => "join";
+        room4.currentState.setStateEvents([
+            makeMembershipEvent(room4.roomId, userId1, "invite"),
+            makeMembershipEvent(room4.roomId, userId2, "join"),
+        ]);
+
+        // this should not be a DM room because it has no users
+        room5 = new Room("!room5:example.com", mockClient, userId1);
+        room5.getLastActiveTimestamp = () => 100;
+
+        mocked(mockClient.getRoom).mockImplementation((roomId: string) => {
+            return {
+                [room1.roomId]: room1,
+                [room2.roomId]: room2,
+                [room3.roomId]: room3,
+                [room4.roomId]: room4,
+                [room5.roomId]: room5,
+            }[roomId];
+        });
 
         dmRoomMap = {
             getDMRoomForIdentifiers: jest.fn(),
             getDMRoomsForUserId: jest.fn(),
         } as unknown as DMRoomMap;
         jest.spyOn(DMRoomMap, "shared").mockReturnValue(dmRoomMap);
+        mocked(dmRoomMap.getDMRoomsForUserId).mockReturnValue([
+            room1.roomId,
+            room2.roomId,
+            room3.roomId,
+            room4.roomId,
+            room5.roomId,
+        ]);
     });
 
-    it("should return the room for a single target with a room", () => {
-        mocked(findDMForUser).mockReturnValue(room1);
-        expect(findDMRoom(mockClient, [member1])).toBe(room1);
-    });
+    describe("for an empty DM room list", () => {
+        beforeEach(() => {
+            mocked(dmRoomMap.getDMRoomsForUserId).mockReturnValue([]);
+        });
 
-    it("should return null for a single target without a room", () => {
-        mocked(findDMForUser).mockReturnValue(null);
-        expect(findDMRoom(mockClient, [member1])).toBeNull();
+        it("should return undefined", () => {
+            expect(findDMForUser(mockClient, userId1)).toBeUndefined();
+        });
     });
 
-    it("should return the room for 2 targets with a room", () => {
-        mocked(dmRoomMap.getDMRoomForIdentifiers).mockReturnValue(room1);
-        expect(findDMRoom(mockClient, [member1, member2])).toBe(room1);
+    it("should find a room ordered by last activity 1", () => {
+        room1.getLastActiveTimestamp = () => 2;
+        room3.getLastActiveTimestamp = () => 1;
+
+        expect(findDMForUser(mockClient, userId1)).toBe(room1);
     });
 
-    it("should return null for 2 targets without a room", () => {
-        mocked(dmRoomMap.getDMRoomForIdentifiers).mockReturnValue(null);
-        expect(findDMRoom(mockClient, [member1, member2])).toBeNull();
+    it("should find a room ordered by last activity 2", () => {
+        room1.getLastActiveTimestamp = () => 1;
+        room3.getLastActiveTimestamp = () => 2;
+
+        expect(findDMForUser(mockClient, userId1)).toBe(room3);
     });
 });

test/utils/room/getJoinedNonFunctionalMembers-test.ts

@@ -0,0 +1,97 @@
+/*
+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 { MatrixClient, Room, RoomMember } from "matrix-js-sdk/src/matrix";
+
+import { getFunctionalMembers } from "../../../src/utils/room/getFunctionalMembers";
+import { getJoinedNonFunctionalMembers } from "../../../src/utils/room/getJoinedNonFunctionalMembers";
+
+jest.mock("../../../src/utils/room/getFunctionalMembers", () => ({
+    getFunctionalMembers: jest.fn(),
+}));
+
+describe("getJoinedNonFunctionalMembers", () => {
+    let room: Room;
+    let roomMember1: RoomMember;
+    let roomMember2: RoomMember;
+
+    beforeEach(() => {
+        room = new Room("!room:example.com", {} as unknown as MatrixClient, "@user:example.com");
+        room.getJoinedMembers = jest.fn();
+
+        roomMember1 = new RoomMember(room.roomId, "@user1:example.com");
+        roomMember2 = new RoomMember(room.roomId, "@user2:example.com");
+    });
+
+    describe("if there are no members", () => {
+        beforeEach(() => {
+            mocked(room.getJoinedMembers).mockReturnValue([]);
+            mocked(getFunctionalMembers).mockReturnValue([]);
+        });
+
+        it("should return an empty list", () => {
+            expect(getJoinedNonFunctionalMembers(room)).toHaveLength(0);
+        });
+    });
+
+    describe("if there are only regular room members", () => {
+        beforeEach(() => {
+            mocked(room.getJoinedMembers).mockReturnValue([
+                roomMember1,
+                roomMember2,
+            ]);
+            mocked(getFunctionalMembers).mockReturnValue([]);
+        });
+
+        it("should return the room members", () => {
+            const members = getJoinedNonFunctionalMembers(room);
+            expect(members).toContain(roomMember1);
+            expect(members).toContain(roomMember2);
+        });
+    });
+
+    describe("if there are only functional room members", () => {
+        beforeEach(() => {
+            mocked(room.getJoinedMembers).mockReturnValue([]);
+            mocked(getFunctionalMembers).mockReturnValue([
+                "@functional:example.com",
+            ]);
+        });
+
+        it("should return an empty list", () => {
+            expect(getJoinedNonFunctionalMembers(room)).toHaveLength(0);
+        });
+    });
+
+    describe("if there are some functional room members", () => {
+        beforeEach(() => {
+            mocked(room.getJoinedMembers).mockReturnValue([
+                roomMember1,
+                roomMember2,
+            ]);
+            mocked(getFunctionalMembers).mockReturnValue([
+                roomMember1.userId,
+            ]);
+        });
+
+        it("should only return the non-functional members", () => {
+            const members = getJoinedNonFunctionalMembers(room);
+            expect(members).not.toContain(roomMember1);
+            expect(members).toContain(roomMember2);
+        });
+    });
+});