Treat an unset profile field as unchanged in collapsed membership summaries (#34493)

apps/web/src/components/views/elements/EventListSummary.tsx

@@ -33,6 +33,18 @@ const onPinnedMessagesClick = (): void => {
 
 const TARGET_AS_DISPLAY_NAME_EVENTS = [EventType.RoomMember];
 
+/**
+ * Whether a profile field on a membership event actually changed.
+ *
+ * An absent field, an explicit `null` and an empty string all mean "unset", so none of them is a
+ * change from any of the others. This mirrors `getModification` in `TextForEvent`, which labels the
+ * same event when it is rendered on its own — without it a collapsed summary can contradict the
+ * events it summarises.
+ */
+function profileFieldChanged(prev?: string | null, value?: string | null): boolean {
+    return (prev || undefined) !== (value || undefined);
+}
+
 interface IProps extends Omit<ComponentProps<typeof GenericEventListSummary>, "summaryText" | "summaryMembers"> {
     // The maximum number of names to show in either each summary e.g. 2 would result "A, B and 234 others left"
     summaryLength?: number;
@@ -532,9 +544,11 @@ export default class EventListSummary extends React.Component<Props, State> {
                         return TransitionType.Banned;
                     case KnownMembership.Join:
                         if (e.mxEvent.getPrevContent().membership === KnownMembership.Join) {
-                            if (e.mxEvent.getContent().displayname !== e.mxEvent.getPrevContent().displayname) {
+                            const content = e.mxEvent.getContent();
+                            const prevContent = e.mxEvent.getPrevContent();
+                            if (profileFieldChanged(prevContent.displayname, content.displayname)) {
                                 return TransitionType.ChangedName;
-                            } else if (e.mxEvent.getContent().avatar_url !== e.mxEvent.getPrevContent().avatar_url) {
+                            } else if (profileFieldChanged(prevContent.avatar_url, content.avatar_url)) {
                                 return TransitionType.ChangedAvatar;
                             }
                             return TransitionType.NoChange;

apps/web/test/unit-tests/components/views/elements/EventListSummary-test.tsx

@@ -716,4 +716,75 @@ describe("EventListSummary", function () {
         expect(summary).toHaveTextContent("n...@d... was invited 2 times, d...@w... was invited");
         expect(summary).toMatchSnapshot();
     });
+
+    describe("profile changes", () => {
+        /**
+         * Generates a join -> join membership event with the given profile fields spliced into
+         * `content` and `prev_content`, so that the null-versus-absent cases can be exercised.
+         */
+        const generateProfileEvent = (
+            userId: string,
+            prevProfile: Record<string, string | null>,
+            profile: Record<string, string | null>,
+        ): MatrixEvent => {
+            const member = new RoomMember(roomId, userId);
+            member.name = userId.match(/@([^:]*):/)![1];
+            const e = mkEvent({
+                event: true,
+                type: "m.room.member",
+                room: roomId,
+                user: userId,
+                skey: userId,
+                content: { membership: KnownMembership.Join, ...profile },
+                prev_content: { membership: KnownMembership.Join, ...prevProfile },
+            });
+            e.event.event_id = "event0";
+            e.target = member;
+            return e;
+        };
+
+        const summaryFor = (event: MatrixEvent): string => {
+            const { container } = renderComponent({
+                events: [event],
+                children: generateTiles([event]),
+                summaryLength: 1,
+                avatarsMaxLength: 5,
+                threshold: 1,
+            });
+            return container.querySelector(".mx_GenericEventListSummary_summary")!.textContent!;
+        };
+
+        it("treats an explicit null avatar_url the same as an absent one", () => {
+            const event = generateProfileEvent("@user_1:some.domain", {}, { avatar_url: null });
+            expect(summaryFor(event)).toBe("user_1 made no changes");
+        });
+
+        it("treats an explicit null displayname the same as an absent one", () => {
+            const event = generateProfileEvent("@user_1:some.domain", {}, { displayname: null });
+            expect(summaryFor(event)).toBe("user_1 made no changes");
+        });
+
+        it("still reports a real avatar change", () => {
+            const event = generateProfileEvent(
+                "@user_1:some.domain",
+                { avatar_url: "mxc://server/old" },
+                { avatar_url: "mxc://server/new" },
+            );
+            expect(summaryFor(event)).toBe("user_1 changed their profile picture");
+        });
+
+        it("still reports a real name change", () => {
+            const event = generateProfileEvent("@user_1:some.domain", { displayname: "Old" }, { displayname: "New" });
+            expect(summaryFor(event)).toBe("user_1 changed their name");
+        });
+
+        it("still reports an avatar being removed", () => {
+            const event = generateProfileEvent(
+                "@user_1:some.domain",
+                { avatar_url: "mxc://server/old" },
+                { avatar_url: null },
+            );
+            expect(summaryFor(event)).toBe("user_1 changed their profile picture");
+        });
+    });
 });