From 711bf4710d38bff86cd5fea8e4505c857b7451ff Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 25 May 2023 16:32:42 +0100 Subject: [PATCH] Test inserting edit events into the thread timeline "on demand" (#3410) * Fix an existing test for editing messages in threads While attempting to test a new change, I discovered that the test "should allow edits to be added to thread timeline" did not actually fail if its assertions failed. Further, those assertions were incorrect. So this change fixes the test to create the thread, wait for it to be initialised, and then add events to it. This simplifies the flow and ensures the test fails if something unexpected happens. * Move editing test into thread.spec.ts * Isolate Thread global modification in beforeAll() * Delete unneeded setUnsigned call * Use standard message-creation methods * Rename event variables * Rename sender->user * Remove unneeded variables * Extract distractions into functions * Fetch edits for thread messages Modifies fetchEditsWhereNeeded to allow edits of threaded messages. The code before prevented any relations from fetching edits, but of course events in threads are relations. We definitely want thread messages to get their edits fetched, and I assume this is working in the real code, probably because the event being looked at is some kind of eventmapped thing that doesn't have proper relations visible on it. In tests, if we don't make this change, we can't see edits getting fetched. * Add a test for fetching edits on demand in a thread This test demonstrates the current behaviour, which contains a bug - we don't actually add the right event to the timeline. --- spec/unit/models/thread.spec.ts | 51 +++++++++++++++++++++++++++++---- src/models/thread.ts | 3 +- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/spec/unit/models/thread.spec.ts b/spec/unit/models/thread.spec.ts index 123de4d5e..480e28f64 100644 --- a/spec/unit/models/thread.spec.ts +++ b/spec/unit/models/thread.spec.ts @@ -22,7 +22,7 @@ import { Thread, THREAD_RELATION_TYPE, ThreadEvent, FeatureSupport } from "../.. import { makeThreadEvent, mkThread } from "../../test-utils/thread"; import { TestClient } from "../../TestClient"; import { emitPromise, mkEdit, mkMessage, mkReaction, mock } from "../../test-utils/test-utils"; -import { Direction, EventStatus, MatrixEvent } from "../../../src"; +import { Direction, EventStatus, EventType, MatrixEvent } from "../../../src"; import { ReceiptType } from "../../../src/@types/read_receipts"; import { getMockClientWithEventEmitter, mockClientMethodsUser } from "../../test-utils/client"; import { ReEmitter } from "../../../src/ReEmitter"; @@ -615,7 +615,7 @@ describe("Thread", () => { Thread.hasServerSideSupport = previousThreadHasServerSideSupport; }); - it("should allow edits to be added to thread timeline", async () => { + it("Adds edits from sync to the thread timeline and applies them", async () => { // Given a thread const client = createClient(); const user = "@alice:matrix.org"; @@ -637,6 +637,45 @@ describe("Thread", () => { // And the first message has been edited expect(secondLastEvent.getContent().body).toEqual("edit"); }); + + it("Adds edits fetched on demand to the thread timeline and applies them", async () => { + // Given we don't support recursive relations + const client = createClient(new Map([[Feature.RelationsRecursion, ServerSupport.Unsupported]])); + // And we have a thread + const user = "@alice:matrix.org"; + const room = "!room:z"; + const thread = await createThread(client, user, room); + + // When a message is added to the thread, and an edit to it is provided on demand + const messageToEdit = createThreadMessage(thread.id, user, room, "Thread reply"); + // (fetchEditsWhereNeeded only applies to encrypted messages for some reason) + messageToEdit.event.type = EventType.RoomMessageEncrypted; + const editEvent = mkEdit(messageToEdit, client, user, room, "edit"); + mocked(client.relations).mockImplementation(async (_roomId, eventId) => { + if (eventId === messageToEdit.getId()) { + return { events: [editEvent] }; + } else { + return { events: [] }; + } + }); + await thread.addEvent(messageToEdit, false); + + // THIS IS THE CORRECT BEHAVIOUR + // Then both events end up in the timeline + //const lastEvent = thread.timeline.at(-1)!; + //const secondLastEvent = thread.timeline.at(-2)!; + //expect(lastEvent).toBe(editEvent); + //expect(secondLastEvent).toBe(messageToEdit); + + //// And the first message has been edited + //expect(secondLastEvent.getContent().body).toEqual("edit"); + + // TODO: For now, we incorrecly DON'T add the event to the timeline + const lastEvent = thread.timeline.at(-1)!; + expect(lastEvent).toBe(messageToEdit); + // But the original is edited, as expected + expect(lastEvent.getContent().body).toEqual("edit"); + }); }); }); }); @@ -661,8 +700,10 @@ function createThreadMessage(threadId: string, user: string, room: string, msg: */ async function createThread(client: MatrixClient, user: string, roomId: string): Promise { const root = mkMessage({ event: true, user, room: roomId, msg: "Thread root" }); - const room = new Room(roomId, client, "@roomcreator:x"); + + // Ensure the root is in the room timeline + root.setThreadId(root.getId()); room.addLiveEvents([root]); // Create the thread and wait for it to be initialised @@ -676,10 +717,10 @@ async function createThread(client: MatrixClient, user: string, roomId: string): * Create a MatrixClient that supports threads and has all the methods used when * creating a thread that call out to HTTP endpoints mocked out. */ -function createClient(): MatrixClient { +function createClient(canSupport = new Map()): MatrixClient { const client = mock(MatrixClient, "MatrixClient"); client.reEmitter = mock(ReEmitter, "ReEmitter"); - client.canSupport = new Map(); + client.canSupport = canSupport; jest.spyOn(client, "supportsThreads").mockReturnValue(true); jest.spyOn(client, "getEventMapper").mockReturnValue(eventMapperFor(client, {})); diff --git a/src/models/thread.ts b/src/models/thread.ts index c44f01c95..b04ea63dc 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -522,7 +522,8 @@ export class Thread extends ReadReceipt { events .filter((e) => e.isEncrypted()) .map((event: MatrixEvent) => { - if (event.isRelation()) return; // skip - relations don't get edits + // The only type of relation that gets edits is a thread message. + if (event.getThread() === undefined && event.isRelation()) return; return this.client .relations(this.roomId, event.getId()!, RelationType.Replace, event.getType(), { limit: 1,