Properly generate mentions when editing a reply with MSC3952 (#10486)

* remove redundant feature_intentional_mentions settings check

* tests

* pass replytoevent to attachmmentions in editmessagecomposer

* lint

* strict fix

src/components/views/rooms/EditMessageComposer.tsx

@@ -70,7 +70,7 @@ function getTextReplyFallback(mxEvent: MatrixEvent): string {
 }
 
 // exported for tests
-export function createEditContent(model: EditorModel, editedEvent: MatrixEvent): IContent {
+export function createEditContent(model: EditorModel, editedEvent: MatrixEvent, replyToEvent?: MatrixEvent): IContent {
     const isEmote = containsEmote(model);
     if (isEmote) {
         model = stripEmoteCommand(model);
@@ -108,11 +108,7 @@ export function createEditContent(model: EditorModel, editedEvent: MatrixEvent):
     }
 
     // Build the mentions properties for both the content and new_content.
-    //
+    attachMentions(editedEvent.sender!.userId, contentBody, model, replyToEvent, editedEvent.getContent());
-    // TODO If this is a reply we need to include all the users from it.
-    if (SettingsStore.getValue("feature_intentional_mentions")) {
-        attachMentions(editedEvent.sender!.userId, contentBody, model, undefined, editedEvent.getContent());
-    }
     attachRelation(contentBody, { rel_type: "m.replace", event_id: editedEvent.getId() });
 
     return contentBody;
@@ -132,6 +128,7 @@ class EditMessageComposer extends React.Component<IEditMessageComposerProps, ISt
 
     private readonly editorRef = createRef<BasicMessageComposer>();
     private readonly dispatcherRef: string;
+    private readonly replyToEvent?: MatrixEvent;
     private model: EditorModel;
 
     public constructor(props: IEditMessageComposerProps, context: React.ContextType<typeof RoomContext>) {
@@ -141,7 +138,9 @@ class EditMessageComposer extends React.Component<IEditMessageComposerProps, ISt
         const isRestored = this.createEditorModel();
         const ev = this.props.editState.getEvent();
 
-        const editContent = createEditContent(this.model, ev);
+        this.replyToEvent = ev.replyEventId ? this.context.room?.findEventById(ev.replyEventId) : undefined;
+
+        const editContent = createEditContent(this.model, ev, this.replyToEvent);
         this.state = {
             saveDisabled: !isRestored || !this.isContentModified(editContent["m.new_content"]),
         };
@@ -310,7 +309,7 @@ class EditMessageComposer extends React.Component<IEditMessageComposerProps, ISt
             const position = this.model.positionForOffset(caret.offset, caret.atNodeEnd);
             this.editorRef.current.replaceEmoticon(position, REGEX_EMOTICON);
         }
-        const editContent = createEditContent(this.model, editedEvent);
+        const editContent = createEditContent(this.model, editedEvent, this.replyToEvent);
         const newContent = editContent["m.new_content"];
 
         let shouldSend = true;

test/components/views/rooms/EditMessageComposer-test.tsx

@@ -14,13 +14,43 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import { createEditContent } from "../../../../src/components/views/rooms/EditMessageComposer";
+import React from "react";
+import { fireEvent, render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
+import { Room } from "matrix-js-sdk/src/matrix";
+
+import EditMessageComposerWithMatrixClient, {
+    createEditContent,
+} from "../../../../src/components/views/rooms/EditMessageComposer";
 import EditorModel from "../../../../src/editor/model";
 import { createPartCreator } from "../../../editor/mock";
-import { mkEvent } from "../../../test-utils";
+import {
+    getMockClientWithEventEmitter,
+    getRoomContext,
+    mkEvent,
+    mockClientMethodsUser,
+    setupRoomWithEventsTimeline,
+} from "../../../test-utils";
 import DocumentOffset from "../../../../src/editor/offset";
+import SettingsStore from "../../../../src/settings/SettingsStore";
+import EditorStateTransfer from "../../../../src/utils/EditorStateTransfer";
+import RoomContext from "../../../../src/contexts/RoomContext";
+import { IRoomState } from "../../../../src/components/structures/RoomView";
+import MatrixClientContext from "../../../../src/contexts/MatrixClientContext";
+import Autocompleter, { IProviderCompletions } from "../../../../src/autocomplete/Autocompleter";
+import NotifProvider from "../../../../src/autocomplete/NotifProvider";
+import DMRoomMap from "../../../../src/utils/DMRoomMap";
 
 describe("<EditMessageComposer/>", () => {
+    const userId = "@alice:server.org";
+    const roomId = "!room:server.org";
+    const mockClient = getMockClientWithEventEmitter({
+        ...mockClientMethodsUser(userId),
+        getRoom: jest.fn(),
+        sendMessage: jest.fn(),
+    });
+    const room = new Room(roomId, mockClient, userId);
+
     const editedEvent = mkEvent({
         type: "m.room.message",
         user: "@alice:test",
@@ -29,6 +59,95 @@ describe("<EditMessageComposer/>", () => {
         event: true,
     });
 
+    const eventWithMentions = mkEvent({
+        type: "m.room.message",
+        user: userId,
+        room: roomId,
+        content: {
+            "msgtype": "m.text",
+            "body": "hey Bob and Charlie",
+            "format": "org.matrix.custom.html",
+            "formatted_body":
+                'hey <a href="https://matrix.to/#/@bob:server.org">Bob</a> and <a href="https://matrix.to/#/@charlie:server.org">Charlie</a>',
+            "org.matrix.msc3952.mentions": {
+                user_ids: ["@bob:server.org", "@charlie:server.org"],
+            },
+        },
+        event: true,
+    });
+
+    // message composer emojipicker uses this
+    // which would require more irrelevant mocking
+    jest.spyOn(SettingsStore, "setValue").mockResolvedValue(undefined);
+
+    const defaultRoomContext = getRoomContext(room, {});
+
+    const getComponent = (editState: EditorStateTransfer, roomContext: IRoomState = defaultRoomContext) =>
+        render(<EditMessageComposerWithMatrixClient editState={editState} />, {
+            wrapper: ({ children }) => (
+                <MatrixClientContext.Provider value={mockClient}>
+                    <RoomContext.Provider value={roomContext}>{children}</RoomContext.Provider>
+                </MatrixClientContext.Provider>
+            ),
+        });
+
+    beforeEach(() => {
+        mockClient.getRoom.mockReturnValue(room);
+        mockClient.sendMessage.mockClear();
+
+        userEvent.setup();
+
+        DMRoomMap.makeShared();
+
+        jest.spyOn(Autocompleter.prototype, "getCompletions").mockResolvedValue([
+            {
+                completions: [
+                    {
+                        completion: "@dan:server.org",
+                        completionId: "@dan:server.org",
+                        type: "user",
+                        suffix: " ",
+                        component: <span>Dan</span>,
+                    },
+                ],
+                command: {
+                    command: ["@d"],
+                },
+                provider: new NotifProvider(room),
+            } as unknown as IProviderCompletions,
+        ]);
+    });
+
+    const editText = async (text: string, shouldClear?: boolean): Promise<void> => {
+        const input = screen.getByRole("textbox");
+        if (shouldClear) {
+            await userEvent.clear(input);
+        }
+        await userEvent.type(input, text);
+    };
+
+    it("should edit a simple message", async () => {
+        const editState = new EditorStateTransfer(editedEvent);
+        getComponent(editState);
+        await editText(" + edit");
+
+        fireEvent.click(screen.getByText("Save"));
+
+        const expectedBody = {
+            ...editedEvent.getContent(),
+            "body": " * original message + edit",
+            "m.new_content": {
+                body: "original message + edit",
+                msgtype: "m.text",
+            },
+            "m.relates_to": {
+                event_id: editedEvent.getId(),
+                rel_type: "m.replace",
+            },
+        };
+        expect(mockClient.sendMessage).toHaveBeenCalledWith(editedEvent.getRoomId()!, null, expectedBody);
+    });
+
     describe("createEditContent", () => {
         it("sends plaintext messages correctly", () => {
             const model = new EditorModel([], createPartCreator());
@@ -147,4 +266,275 @@ describe("<EditMessageComposer/>", () => {
             });
         });
     });
+
+    describe("with feature_intentional_mentions enabled", () => {
+        const mockSettings = (mockValues: Record<string, unknown> = {}) => {
+            const defaultMockValues = {
+                feature_intentional_mentions: true,
+            };
+            jest.spyOn(SettingsStore, "getValue")
+                .mockClear()
+                .mockImplementation((settingName) => {
+                    return { ...defaultMockValues, ...mockValues }[settingName];
+                });
+        };
+
+        beforeEach(() => {
+            mockSettings();
+        });
+
+        describe("when message is not a reply", () => {
+            it("should attach an empty mentions object for a message with no mentions", async () => {
+                const editState = new EditorStateTransfer(editedEvent);
+                getComponent(editState);
+                const editContent = " + edit";
+                await editText(editContent);
+
+                fireEvent.click(screen.getByText("Save"));
+
+                const messageContent = mockClient.sendMessage.mock.calls[0][2];
+
+                // both content.mentions and new_content.mentions are empty
+                expect(messageContent["org.matrix.msc3952.mentions"]).toEqual({});
+                expect(messageContent["m.new_content"]["org.matrix.msc3952.mentions"]).toEqual({});
+            });
+
+            it("should retain mentions in the original message that are not removed by the edit", async () => {
+                const editState = new EditorStateTransfer(eventWithMentions);
+                getComponent(editState);
+                // Remove charlie from the message
+                const editContent = "{backspace}{backspace}friends";
+                await editText(editContent);
+
+                fireEvent.click(screen.getByText("Save"));
+
+                const messageContent = mockClient.sendMessage.mock.calls[0][2];
+
+                // no new mentions were added, so nothing in top level mentions
+                expect(messageContent["org.matrix.msc3952.mentions"]).toEqual({});
+                // bob is still mentioned, charlie removed
+                expect(messageContent["m.new_content"]["org.matrix.msc3952.mentions"]).toEqual({
+                    user_ids: ["@bob:server.org"],
+                });
+            });
+
+            it("should remove mentions that are removed by the edit", async () => {
+                const editState = new EditorStateTransfer(eventWithMentions);
+                getComponent(editState);
+                const editContent = "new message!";
+                // clear the original message
+                await editText(editContent, true);
+
+                fireEvent.click(screen.getByText("Save"));
+
+                const messageContent = mockClient.sendMessage.mock.calls[0][2];
+
+                // no new mentions were added, so nothing in top level mentions
+                expect(messageContent["org.matrix.msc3952.mentions"]).toEqual({});
+                // bob is not longer mentioned in the edited message, so empty mentions in new_content
+                expect(messageContent["m.new_content"]["org.matrix.msc3952.mentions"]).toEqual({});
+            });
+
+            it("should add mentions that were added in the edit", async () => {
+                const editState = new EditorStateTransfer(editedEvent);
+                getComponent(editState);
+                const editContent = " and @d";
+                await editText(editContent);
+
+                // submit autocomplete for mention
+                await editText("{enter}");
+
+                fireEvent.click(screen.getByText("Save"));
+
+                const messageContent = mockClient.sendMessage.mock.calls[0][2];
+
+                // new mention in the edit
+                expect(messageContent["org.matrix.msc3952.mentions"]).toEqual({
+                    user_ids: ["@dan:server.org"],
+                });
+                expect(messageContent["m.new_content"]["org.matrix.msc3952.mentions"]).toEqual({
+                    user_ids: ["@dan:server.org"],
+                });
+            });
+
+            it("should add and remove mentions from the edit", async () => {
+                const editState = new EditorStateTransfer(eventWithMentions);
+                getComponent(editState);
+                // Remove charlie from the message
+                await editText("{backspace}{backspace}");
+                // and replace with @room
+                await editText("@d");
+                // submit autocomplete for @dan mention
+                await editText("{enter}");
+
+                fireEvent.click(screen.getByText("Save"));
+
+                const messageContent = mockClient.sendMessage.mock.calls[0][2];
+
+                // new mention in the edit
+                expect(messageContent["org.matrix.msc3952.mentions"]).toEqual({
+                    user_ids: ["@dan:server.org"],
+                });
+                // all mentions in the edited version of the event
+                expect(messageContent["m.new_content"]["org.matrix.msc3952.mentions"]).toEqual({
+                    user_ids: ["@bob:server.org", "@dan:server.org"],
+                });
+            });
+        });
+
+        describe("when message is replying", () => {
+            const originalEvent = mkEvent({
+                type: "m.room.message",
+                user: "@ernie:test",
+                room: roomId,
+                content: { body: "original message", msgtype: "m.text" },
+                event: true,
+            });
+
+            const replyEvent = mkEvent({
+                type: "m.room.message",
+                user: "@bert:test",
+                room: roomId,
+                content: {
+                    "body": "reply with plain message",
+                    "msgtype": "m.text",
+                    "m.relates_to": {
+                        "m.in_reply_to": {
+                            event_id: originalEvent.getId(),
+                        },
+                    },
+                    "org.matrix.msc3952.mentions": {
+                        user_ids: [originalEvent.getSender()!],
+                    },
+                },
+                event: true,
+            });
+
+            const replyWithMentions = mkEvent({
+                type: "m.room.message",
+                user: "@bert:test",
+                room: roomId,
+                content: {
+                    "body": 'reply that mentions <a href="https://matrix.to/#/@bob:server.org">Bob</a>',
+                    "msgtype": "m.text",
+                    "m.relates_to": {
+                        "m.in_reply_to": {
+                            event_id: originalEvent.getId(),
+                        },
+                    },
+                    "org.matrix.msc3952.mentions": {
+                        user_ids: [
+                            // sender of event we replied to
+                            originalEvent.getSender()!,
+                            // mentions from this event
+                            "@bob:server.org",
+                        ],
+                    },
+                },
+                event: true,
+            });
+
+            beforeEach(() => {
+                setupRoomWithEventsTimeline(room, [originalEvent, replyEvent]);
+            });
+
+            it("should retain parent event sender in mentions when editing with plain text", async () => {
+                const editState = new EditorStateTransfer(replyEvent);
+                getComponent(editState);
+                const editContent = " + edit";
+                await editText(editContent);
+
+                fireEvent.click(screen.getByText("Save"));
+
+                const messageContent = mockClient.sendMessage.mock.calls[0][2];
+
+                // no new mentions from edit
+                expect(messageContent["org.matrix.msc3952.mentions"]).toEqual({});
+                // edited reply still mentions the parent event sender
+                expect(messageContent["m.new_content"]["org.matrix.msc3952.mentions"]).toEqual({
+                    user_ids: [originalEvent.getSender()],
+                });
+            });
+
+            it("should retain parent event sender in mentions when adding a mention", async () => {
+                const editState = new EditorStateTransfer(replyEvent);
+                getComponent(editState);
+                await editText(" and @d");
+                // submit autocomplete for @dan mention
+                await editText("{enter}");
+
+                fireEvent.click(screen.getByText("Save"));
+
+                const messageContent = mockClient.sendMessage.mock.calls[0][2];
+
+                // new mention in edit
+                expect(messageContent["org.matrix.msc3952.mentions"]).toEqual({
+                    user_ids: ["@dan:server.org"],
+                });
+                // edited reply still mentions the parent event sender
+                // plus new mention @dan
+                expect(messageContent["m.new_content"]["org.matrix.msc3952.mentions"]).toEqual({
+                    user_ids: [originalEvent.getSender(), "@dan:server.org"],
+                });
+            });
+
+            it("should retain parent event sender in mentions when removing all mentions from content", async () => {
+                const editState = new EditorStateTransfer(replyWithMentions);
+                getComponent(editState);
+                // replace text to remove all mentions
+                await editText("no mentions here", true);
+
+                fireEvent.click(screen.getByText("Save"));
+
+                const messageContent = mockClient.sendMessage.mock.calls[0][2];
+
+                // no mentions in edit
+                expect(messageContent["org.matrix.msc3952.mentions"]).toEqual({});
+                // edited reply still mentions the parent event sender
+                // existing @bob mention removed
+                expect(messageContent["m.new_content"]["org.matrix.msc3952.mentions"]).toEqual({
+                    user_ids: [originalEvent.getSender()],
+                });
+            });
+
+            it("should retain parent event sender in mentions when removing mention of said user", async () => {
+                const replyThatMentionsParentEventSender = mkEvent({
+                    type: "m.room.message",
+                    user: "@bert:test",
+                    room: roomId,
+                    content: {
+                        "body": `reply that mentions the sender of the message we replied to <a href="https://matrix.to/#/${originalEvent.getSender()!}">Ernie</a>`,
+                        "msgtype": "m.text",
+                        "m.relates_to": {
+                            "m.in_reply_to": {
+                                event_id: originalEvent.getId(),
+                            },
+                        },
+                        "org.matrix.msc3952.mentions": {
+                            user_ids: [
+                                // sender of event we replied to
+                                originalEvent.getSender()!,
+                            ],
+                        },
+                    },
+                    event: true,
+                });
+                const editState = new EditorStateTransfer(replyThatMentionsParentEventSender);
+                getComponent(editState);
+                // replace text to remove all mentions
+                await editText("no mentions here", true);
+
+                fireEvent.click(screen.getByText("Save"));
+
+                const messageContent = mockClient.sendMessage.mock.calls[0][2];
+
+                // no mentions in edit
+                expect(messageContent["org.matrix.msc3952.mentions"]).toEqual({});
+                // edited reply still mentions the parent event sender
+                expect(messageContent["m.new_content"]["org.matrix.msc3952.mentions"]).toEqual({
+                    user_ids: [originalEvent.getSender()],
+                });
+            });
+        });
+    });
 });

test/test-utils/room.ts

@@ -15,7 +15,7 @@ limitations under the License.
 */
 
 import { MockedObject } from "jest-mock";
-import { MatrixClient, MatrixEvent, EventType, Room } from "matrix-js-sdk/src/matrix";
+import { MatrixClient, MatrixEvent, EventType, Room, EventTimeline } from "matrix-js-sdk/src/matrix";
 
 import { IRoomState } from "../../src/components/structures/RoomView";
 import { TimelineRenderingType } from "../../src/contexts/RoomContext";
@@ -91,3 +91,12 @@ export function getRoomContext(room: Room, override: Partial<IRoomState>): IRoom
         ...override,
     };
 }
+
+export const setupRoomWithEventsTimeline = (room: Room, events: MatrixEvent[] = []): void => {
+    const timelineSet = room.getUnfilteredTimelineSet();
+    const getTimelineForEventSpy = jest.spyOn(timelineSet, "getTimelineForEvent");
+    const eventTimeline = {
+        getEvents: jest.fn().mockReturnValue(events),
+    } as unknown as EventTimeline;
+    getTimelineForEventSpy.mockReturnValue(eventTimeline);
+};