Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix automatic DM avatar with functional members #4017

Merged
merged 22 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
0ce2d82
fix automatic DM avatar with functional members
HarHarLinks Jan 19, 2024
f9b41f6
update comments
HarHarLinks Feb 6, 2024
e65fb24
lint
HarHarLinks Feb 6, 2024
c114bf5
add tests for functional members
HarHarLinks Feb 6, 2024
92aef67
Merge branch 'develop' into fix-functional-members-avatar
richvdh Feb 26, 2024
25fdc18
keep functional members out of the public API
HarHarLinks Feb 26, 2024
d6f83d3
filter functional members from more candidates
HarHarLinks Feb 26, 2024
657f1bc
add tests for fallback avatars with functional members
HarHarLinks Feb 26, 2024
9234e82
Merge branch 'develop' into fix-functional-members-avatar
HarHarLinks Feb 26, 2024
5794809
Add docstring for getFunctionalMembers
HarHarLinks Feb 28, 2024
0789a23
inline getInvitedAndJoinedFunctionalMemberCount
HarHarLinks Feb 28, 2024
b2b5b18
update comments for getAvatarFallbackMember
HarHarLinks Feb 28, 2024
fa4666c
use correct list of heroes in getAvatarFallbackMember
HarHarLinks Feb 28, 2024
404d671
remove redundant type annotation
HarHarLinks Feb 28, 2024
0f6ab78
optimize performance of invitedAndJoinedFunctionalMemberCount
HarHarLinks Feb 28, 2024
8db6b47
calculate nonFunctionalMemberCount in one step
HarHarLinks Feb 28, 2024
66157c4
clean up functional member tests with review feedback
HarHarLinks Feb 28, 2024
684dcac
lint
HarHarLinks Feb 28, 2024
0ea0d50
Update src/models/room.ts
HarHarLinks Mar 12, 2024
efca7ba
apply feedback about comments
HarHarLinks Mar 12, 2024
882f625
non-functional per review, lint
HarHarLinks Mar 12, 2024
5f8a985
Merge branch 'develop' into fix-functional-members-avatar
HarHarLinks Mar 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 79 additions & 1 deletion spec/unit/room.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -2231,6 +2231,84 @@ describe("Room", function () {
});
expect(room.getAvatarFallbackMember()?.userId).toBe(userD);
});

it("should return undefined if the room is a 1:1 with a functional member", async function() {
const room = new Room(roomId, null!, userA);
// room.getMembers = jest.fn().mockReturnValue({
// userB: new RoomMember(roomId, userB),
// });
HarHarLinks marked this conversation as resolved.
Show resolved Hide resolved
jest.spyOn(Room.prototype as any, "getInvitedAndJoinedFunctionalMemberCount").mockReturnValue(1);
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 use summary heroes of nonfunctional member if 1:1 plus functional member", async function() {
const room = new Room(roomId, null!, userA);
// room.getMembers = jest.fn().mockReturnValue({
// userB: new RoomMember(roomId, userB),
// });
jest.spyOn(Room.prototype as any, "getInvitedAndJoinedFunctionalMemberCount").mockReturnValue(1);
await room.currentState.setStateEvents([
utils.mkEvent({
type: UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name,
skey: "",
room: roomId,
event: true,
content: {
service_members: [userB],
},
}),
]);
room.currentState.markOutOfBandMembersStarted();
room.currentState.setOutOfBandMembers([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we use ...OutOfBandMembers for this test rather than setStateEvents? Maybe we need a separate test to check the out-of-band-members case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I copied it from the test above 🙃 I'm not sure what the difference is in the first place. What are they, and do I need to do something about them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish I knew... let's ignore them for now

new MatrixEvent({
type: EventType.RoomMember,
state_key: userD,
sender: userD,
content: {
membership: "join",
},
}),
new MatrixEvent({
type: EventType.RoomMember,
state_key: userB,
sender: userB,
content: {
membership: "join",
},
}),
]);
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 () {
Expand Down
44 changes: 35 additions & 9 deletions src/models/room.ts
Original file line number Diff line number Diff line change
Expand Up @@ -914,12 +914,38 @@
return this.myUserId;
}

// gets service members (e.g. helper bots) for exclusion
HarHarLinks marked this conversation as resolved.
Show resolved Hide resolved
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;
}
return [];
}

private getInvitedAndJoinedFunctionalMemberCount(functionalMembers: String[]): number {
return functionalMembers.reduce((count, m) => {
const membership = this.getMembers().find((member) => member.userId === m)?.membership;
HarHarLinks marked this conversation as resolved.
Show resolved Hide resolved
if (membership && ["join", "invite"].includes(membership)) {
return count + 1;
}
return count;
}, 0);
}
HarHarLinks marked this conversation as resolved.
Show resolved Hide resolved

public getAvatarFallbackMember(): RoomMember | undefined {
const memberCount = this.getInvitedAndJoinedMemberCount();
const functionalMembers = this.getFunctionalMembers();
const invitedAndJoinedFunctionalMemberCount = this.getInvitedAndJoinedFunctionalMemberCount(functionalMembers);

// only get avatar if conversation is with a single specific other user
const memberCount = this.getInvitedAndJoinedMemberCount() - invitedAndJoinedFunctionalMemberCount;
HarHarLinks marked this conversation as resolved.
Show resolved Hide resolved
if (memberCount > 2) {
return;
}
const hasHeroes = Array.isArray(this.summaryHeroes) && this.summaryHeroes.length;

// prefer hero as indicated by the room summary
HarHarLinks marked this conversation as resolved.
Show resolved Hide resolved
const nonFunctionalHeroes = this.summaryHeroes?.filter((h) => !functionalMembers.includes(h));
const hasHeroes = Array.isArray(nonFunctionalHeroes) && nonFunctionalHeroes.length;
if (hasHeroes) {
const availableMember = this.summaryHeroes!.map((userId) => {
HarHarLinks marked this conversation as resolved.
Show resolved Hide resolved
return this.getMember(userId);
Expand All @@ -928,17 +954,21 @@
return availableMember;
}
}

// include previous members
HarHarLinks marked this conversation as resolved.
Show resolved Hide resolved
const members = this.currentState.getMembers();
const nonFunctionalMembers = members?.filter((m) => !functionalMembers.includes(m.userId));
// could be different than memberCount
// as this includes left members
if (members.length <= 2) {
const availableMember = members.find((m) => {
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 (hasHeroes) {
Expand Down Expand Up @@ -3351,11 +3381,7 @@
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;
}
let excludedUserIds: string[] = this.getFunctionalMembers();

Check failure on line 3384 in src/models/room.ts

View workflow job for this annotation

GitHub Actions / ESLint

'excludedUserIds' is never reassigned. Use 'const' instead
HarHarLinks marked this conversation as resolved.
Show resolved Hide resolved

// get members that are NOT ourselves and are actually in the room.
let otherNames: string[] = [];
Expand Down
Loading