From 8e0ef5ff2cd927efa1bd22cabb075a14b10e39d5 Mon Sep 17 00:00:00 2001 From: Kim Brose <2803622+HarHarLinks@users.noreply.github.com> Date: Wed, 13 Mar 2024 18:01:11 +0100 Subject: [PATCH] fix automatic DM avatar with functional members (#4017) * fix automatic DM avatar with functional members * update comments * lint * add tests for functional members * keep functional members out of the public API - remove public API for functional members, reverting most of 0ce2d82, f9b41f6, e65fb24 - remove tests for functional members public API c114bf5 - add shared functional members getter for both room name and avatar fallback generation * filter functional members from more candidates - remove from hero(es) - remove from previous members * add tests for fallback avatars with functional members * Add docstring for getFunctionalMembers Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * inline getInvitedAndJoinedFunctionalMemberCount * update comments for getAvatarFallbackMember * use correct list of heroes in getAvatarFallbackMember * remove redundant type annotation * optimize performance of invitedAndJoinedFunctionalMemberCount * calculate nonFunctionalMemberCount in one step instead of iterating redundantly * clean up functional member tests with review feedback * lint * Update src/models/room.ts Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * apply feedback about comments * non-functional per review, lint --------- Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- spec/unit/room.spec.ts | 74 +++++++++++++++++++++++++++++++++++++++++- src/models/room.ts | 74 +++++++++++++++++++++++++++++------------- 2 files changed, 124 insertions(+), 24 deletions(-) diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index 4d64e4df54d..4e14ca099a4 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -2204,7 +2204,7 @@ describe("Room", function () { }); describe("getAvatarFallbackMember", () => { - it("should should return undefined if the room isn't a 1:1", () => { + it("should return undefined if the room isn't a 1:1", () => { const room = new Room(roomId, null!, userA); room.currentState.setJoinedMemberCount(2); room.currentState.setInvitedMemberCount(1); @@ -2231,6 +2231,78 @@ describe("Room", function () { }); expect(room.getAvatarFallbackMember()?.userId).toBe(userD); }); + + it("should return undefined if the room is a 1:1 plus functional member", async function () { + const room = new Room(roomId, null!, userA); + await room.currentState.setStateEvents([ + utils.mkMembership({ + user: userA, + mship: "join", + room: roomId, + event: true, + name: "User A", + }), + utils.mkMembership({ + user: userB, + mship: "join", + room: roomId, + event: true, + name: "User B", + }), + utils.mkEvent({ + type: UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name, + skey: "", + room: roomId, + event: true, + content: { + service_members: [userB], + }, + }), + ]); + expect(room.getAvatarFallbackMember()).toBeUndefined(); + }); + + it("should pick nonfunctional member from summary heroes if room is a 1:1 plus functional member", async function () { + const room = new Room(roomId, null!, userA); + await room.currentState.setStateEvents([ + utils.mkMembership({ + user: userA, + mship: "join", + room: roomId, + event: true, + name: "User A", + }), + utils.mkMembership({ + user: userB, + mship: "join", + room: roomId, + event: true, + name: "User B", + }), + utils.mkMembership({ + user: userD, + mship: "join", + room: roomId, + event: true, + name: "User D", + }), + utils.mkEvent({ + type: UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name, + skey: "", + room: roomId, + event: true, + content: { + service_members: [userB], + }, + }), + ]); + room.setSummary({ + "m.heroes": [userA, userD, userB], + "m.joined_member_count": 2, + "m.invited_member_count": 1, + }); + expect(room.getAvatarFallbackMember()?.userId).toBe(userD); + }); }); describe("maySendMessage", function () { diff --git a/src/models/room.ts b/src/models/room.ts index 67aea690756..7289185e6f6 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -914,37 +914,69 @@ export class Room extends ReadReceipt { return this.myUserId; } - public getAvatarFallbackMember(): RoomMember | undefined { - const memberCount = this.getInvitedAndJoinedMemberCount(); - if (memberCount > 2) { - return; + /** + * Gets the "functional members" in this room. + * + * Returns the list of userIDs from the `io.element.functional_members` event. Does not consider the + * current membership states of those users. + * + * @see https://github.com/element-hq/element-meta/blob/develop/spec/functional_members.md. + */ + private getFunctionalMembers(): string[] { + const mFunctionalMembers = this.currentState.getStateEvents(UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name, ""); + if (Array.isArray(mFunctionalMembers?.getContent().service_members)) { + return mFunctionalMembers!.getContent().service_members; } - const hasHeroes = Array.isArray(this.summaryHeroes) && this.summaryHeroes.length; + return []; + } + + public getAvatarFallbackMember(): RoomMember | undefined { + const functionalMembers = this.getFunctionalMembers(); + + // Only generate a fallback avatar if the conversation is with a single specific other user (a "DM"). + let nonFunctionalMemberCount = 0; + this.getMembers()!.forEach((m) => { + if (m.membership !== "join" && m.membership !== "invite") return; + if (functionalMembers.includes(m.userId)) return; + nonFunctionalMemberCount++; + }); + if (nonFunctionalMemberCount > 2) return; + + // Prefer the list of heroes, if present. It should only include the single other user in the DM. + const nonFunctionalHeroes = this.summaryHeroes?.filter((h) => !functionalMembers.includes(h)); + const hasHeroes = Array.isArray(nonFunctionalHeroes) && nonFunctionalHeroes.length; if (hasHeroes) { - const availableMember = this.summaryHeroes!.map((userId) => { - return this.getMember(userId); - }).find((member) => !!member); + const availableMember = nonFunctionalHeroes + .map((userId) => { + return this.getMember(userId); + }) + .find((member) => !!member); if (availableMember) { return availableMember; } } - const members = this.currentState.getMembers(); - // could be different than memberCount - // as this includes left members - if (members.length <= 2) { - const availableMember = members.find((m) => { + + // Consider *all*, including previous, members, to generate the avatar for DMs where the other user left. + // Needed to generate a matching avatar for rooms named "Empty Room (was Alice)". + const members = this.getMembers(); + const nonFunctionalMembers = members?.filter((m) => !functionalMembers.includes(m.userId)); + if (nonFunctionalMembers.length <= 2) { + const availableMember = nonFunctionalMembers.find((m) => { return m.userId !== this.myUserId; }); if (availableMember) { return availableMember; } } - // if all else fails, try falling back to a user, - // and create a one-off member for it + + // If all else failed, but the homeserver gave us heroes that previously could not be found in the room members, + // trust and try falling back to a hero, creating a one-off member for it if (hasHeroes) { - const availableUser = this.summaryHeroes!.map((userId) => { - return this.client.getUser(userId); - }).find((user) => !!user); + const availableUser = nonFunctionalHeroes + .map((userId) => { + return this.client.getUser(userId); + }) + .find((user) => !!user); if (availableUser) { const member = new RoomMember(this.roomId, availableUser.userId); member.user = availableUser; @@ -3351,11 +3383,7 @@ export class Room extends ReadReceipt { let inviteJoinCount = joinedMemberCount + invitedMemberCount - 1; // get service members (e.g. helper bots) for exclusion - let excludedUserIds: string[] = []; - const mFunctionalMembers = this.currentState.getStateEvents(UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name, ""); - if (Array.isArray(mFunctionalMembers?.getContent().service_members)) { - excludedUserIds = mFunctionalMembers!.getContent().service_members; - } + const excludedUserIds = this.getFunctionalMembers(); // get members that are NOT ourselves and are actually in the room. let otherNames: string[] = [];