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.

Sign up for GitHub

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

Jump to bottom

Fix #23916: Prevent edits of the last message in a thread getting lost #2951

Merged
merged 9 commits into from
Dec 12, 2022
Merged

Fix #23916: Prevent edits of the last message in a thread getting lost #2951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1d784a6
Fix issue where the root event of a thread had to be loaded in a comp…
justjanne Dec 11, 2022
cf83c9a
Fix issue where edits to the last event of a thread would get lost
justjanne Dec 12, 2022
f78b0d3
Fix issue where thread reply count would desync
justjanne Dec 12, 2022
37042df
Refactor relations pagination mocking for tests
justjanne Dec 12, 2022
ee676b3
Update tests accordingly
justjanne Dec 12, 2022
27ed134
Make reviewer happy :)
justjanne Dec 12, 2022
1da85e1
Merge remote-tracking branch 'origin/develop' into justjanne/fix/2391…
justjanne Dec 12, 2022
8c409ef
ci: empty commit to force CI to run again
justjanne Dec 12, 2022
b0ece4f
Merge branch 'develop' into justjanne/fix/23916-editing-not-updating
justjanne Dec 12, 2022
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
114 changes: 29 additions & 85 deletions spec/integ/matrix-client-event-timeline.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,11 @@ import {
MatrixEvent,
PendingEventOrdering,
Room,
RoomEvent,
} from "../../src/matrix";
import { logger } from "../../src/logger";
import { encodeUri } from "../../src/utils";
import { encodeParams, encodeUri, QueryDict, replaceParam } from "../../src/utils";
import { TestClient } from "../TestClient";
import { FeatureSupport, Thread, THREAD_RELATION_TYPE } from "../../src/models/thread";
import { FeatureSupport, Thread, THREAD_RELATION_TYPE, ThreadEvent } from "../../src/models/thread";
import { emitPromise } from "../test-utils/test-utils";

const userId = "@alice:localhost";
Expand All @@ -47,6 +46,18 @@ const withoutRoomId = (e: Partial<IEvent>): Partial<IEvent> => {
return copy;
};

/**
* Our httpBackend only allows matching calls if we have the exact same query, in the exact same order
* This method allows building queries with the exact same parameter order as the fetchRelations method in client
* @param params query parameters
*/
const buildRelationPaginationQuery = (params: QueryDict): string => {
if (Thread.hasServerSideFwdPaginationSupport === FeatureSupport.Experimental) {
params = replaceParam("dir", "org.matrix.msc3715.dir", params);
}
return "?" + encodeParams(params).toString();
};

const USER_MEMBERSHIP_EVENT = utils.mkMembership({
room: roomId,
mship: "join",
Expand Down Expand Up @@ -595,42 +606,6 @@ describe("MatrixClient event timelines", function () {
.respond(200, function () {
return THREAD_ROOT;
});

httpBackend
.when(
"GET",
"/_matrix/client/v1/rooms/!foo%3Abar/relations/" +
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
"?dir=b&limit=1",
)
.respond(200, function () {
return {
original_event: THREAD_ROOT,
chunk: [THREAD_REPLY],
// no next batch as this is the oldest end of the timeline
};
});

const thread = room.createThread(THREAD_ROOT.event_id!, undefined, [], false);
await httpBackend.flushAllExpected();
const timelineSet = thread.timelineSet;

const timelinePromise = client.getEventTimeline(timelineSet, THREAD_REPLY.event_id!);
const timeline = await timelinePromise;

expect(timeline!.getEvents().find((e) => e.getId() === THREAD_ROOT.event_id!)).toBeTruthy();
expect(timeline!.getEvents().find((e) => e.getId() === THREAD_REPLY.event_id!)).toBeTruthy();
});

it("should handle thread replies with server support by fetching a contiguous thread timeline", async () => {
// @ts-ignore
client.clientOpts.experimentalThreadSupport = true;
Thread.setServerSideSupport(FeatureSupport.Experimental);
await client.stopClient(); // we don't need the client to be syncing at this time
const room = client.getRoom(roomId)!;

httpBackend
.when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!))
.respond(200, function () {
Expand All @@ -644,7 +619,7 @@ describe("MatrixClient event timelines", function () {
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
"?dir=b&limit=1",
buildRelationPaginationQuery({ dir: Direction.Backward, limit: 1 }),
)
.respond(200, function () {
return {
Expand All @@ -656,15 +631,14 @@ describe("MatrixClient event timelines", function () {
const thread = room.createThread(THREAD_ROOT.event_id!, undefined, [], false);
await httpBackend.flushAllExpected();
const timelineSet = thread.timelineSet;

httpBackend
.when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!))
.respond(200, function () {
return THREAD_ROOT;
});
await flushHttp(emitPromise(thread, ThreadEvent.Update));

const timelinePromise = client.getEventTimeline(timelineSet, THREAD_REPLY.event_id!);
const [timeline] = await Promise.all([timelinePromise, httpBackend.flushAllExpected()]);
const timeline = await client.getEventTimeline(timelineSet, THREAD_REPLY.event_id!);

const eventIds = timeline!.getEvents().map((it) => it.getId());
expect(eventIds).toContain(THREAD_ROOT.event_id);
Expand Down Expand Up @@ -1273,7 +1247,7 @@ describe("MatrixClient event timelines", function () {
event_id: THREAD_ROOT.event_id,
},
},
event: false,
event: true,
});

// Test data for the first thread, with the second reply
Expand All @@ -1286,7 +1260,7 @@ describe("MatrixClient event timelines", function () {
"io.element.thread": {
...THREAD_ROOT.unsigned!["m.relations"]!["io.element.thread"],
count: 2,
latest_event: THREAD_REPLY2,
latest_event: THREAD_REPLY2.event,
},
},
},
Expand Down Expand Up @@ -1314,12 +1288,13 @@ describe("MatrixClient event timelines", function () {
respondToThreads(threadsResponse);
respondToThreads(threadsResponse);
respondToEvent(THREAD_ROOT);
respondToEvent(THREAD_ROOT);
respondToEvent(THREAD2_ROOT);
respondToEvent(THREAD2_ROOT);
respondToThread(THREAD_ROOT, [THREAD_REPLY]);
respondToThread(THREAD2_ROOT, [THREAD2_REPLY]);
await flushHttp(room.fetchRoomThreads());
const threadIds = room.getThreads().map((thread) => thread.id);
expect(threadIds).toContain(THREAD_ROOT.event_id);
expect(threadIds).toContain(THREAD2_ROOT.event_id);
const [allThreads] = timelineSets!;
const timeline = allThreads.getLiveTimeline()!;
// Test threads are in chronological order
Expand All @@ -1330,12 +1305,15 @@ describe("MatrixClient event timelines", function () {

// Test adding a second event to the first thread
const thread = room.getThread(THREAD_ROOT.event_id!)!;
const prom = emitPromise(allThreads!, RoomEvent.Timeline);
await thread.addEvent(client.getEventMapper()(THREAD_REPLY2), false);
const prom = emitPromise(room, ThreadEvent.NewReply);
respondToEvent(THREAD_ROOT_UPDATED);
respondToEvent(THREAD_ROOT_UPDATED);
respondToEvent(THREAD_ROOT_UPDATED);
respondToEvent(THREAD2_ROOT);
room.addLiveEvents([THREAD_REPLY2]);
await httpBackend.flushAllExpected();
await prom;
expect(thread.length).toBe(2);
// Test threads are in chronological order
expect(timeline!.getEvents().map((it) => it.event.event_id)).toEqual([
THREAD2_ROOT.event_id,
Expand Down Expand Up @@ -1652,24 +1630,6 @@ describe("MatrixClient event timelines", function () {
const thread = room.getThread(THREAD_ROOT.event_id!)!;
const timelineSet = thread.timelineSet;

const buildParams = (direction: Direction, token: string): string => {
if (Thread.hasServerSideFwdPaginationSupport === FeatureSupport.Experimental) {
return `?from=${token}&org.matrix.msc3715.dir=${direction}`;
} else {
return `?dir=${direction}&from=${token}`;
}
};

httpBackend
.when("GET", "/rooms/!foo%3Abar/context/" + encodeURIComponent(THREAD_ROOT.event_id!))
.respond(200, {
start: "start_token",
events_before: [],
event: THREAD_ROOT,
events_after: [],
state: [],
end: "end_token",
});
httpBackend
.when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!))
.respond(200, function () {
Expand All @@ -1692,27 +1652,11 @@ describe("MatrixClient event timelines", function () {
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
buildParams(Direction.Backward, "start_token"),
buildRelationPaginationQuery({ dir: Direction.Backward, limit: 1 }),
)
.respond(200, function () {
return {
original_event: THREAD_ROOT,
chunk: [],
};
});
httpBackend
.when(
"GET",
"/_matrix/client/v1/rooms/!foo%3Abar/relations/" +
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
buildParams(Direction.Forward, "end_token"),
)
.respond(200, function () {
return {
original_event: THREAD_ROOT,
chunk: [THREAD_REPLY],
chunk: [THREAD_ROOT],
};
});
const timeline = await flushHttp(client.getEventTimeline(timelineSet, THREAD_ROOT.event_id!));
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the test coverage for the issues that this claims to fix?

I don't see it amongst these random edits here.

Expand Down
3 changes: 0 additions & 3 deletions spec/test-utils/thread.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,6 @@ export const mkThread = ({
}

const thread = room.createThread(rootEvent.getId() ?? "", rootEvent, events, true);
// So that we do not have to mock the thread loading
thread.initialEventsFetched = true;
thread.addEvents(events, true);

return { thread, rootEvent, events };
};
17 changes: 9 additions & 8 deletions spec/unit/room.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2475,11 +2475,11 @@ describe("Room", function () {

let prom = emitPromise(room, ThreadEvent.New);
room.addLiveEvents([randomMessage, threadRoot, threadResponse]);
const thread = await prom;
const thread: Thread = await prom;
await emitPromise(room, ThreadEvent.Update);

expect(thread.replyToEvent.event).toEqual(threadResponse.event);
expect(thread.replyToEvent.getContent().body).toBe(threadResponse.getContent().body);
expect(thread.replyToEvent!.event).toEqual(threadResponse.event);
expect(thread.replyToEvent!.getContent().body).toBe(threadResponse.getContent().body);

room.client.fetchRoomEvent = (eventId: string) =>
Promise.resolve({
Expand All @@ -2490,7 +2490,7 @@ describe("Room", function () {
[THREAD_RELATION_TYPE.name]: {
latest_event: {
...threadResponse.event,
content: threadResponseEdit.event.content,
content: threadResponseEdit.getContent()["m.new_content"],
},
count: 2,
current_user_participated: true,
Expand All @@ -2502,7 +2502,7 @@ describe("Room", function () {
prom = emitPromise(room, ThreadEvent.Update);
room.addLiveEvents([threadResponseEdit]);
await prom;
expect(thread.replyToEvent.getContent().body).toBe(threadResponseEdit.getContent()["m.new_content"].body);
expect(thread.replyToEvent!.getContent().body).toBe(threadResponseEdit.getContent()["m.new_content"].body);
});

it("Redactions to thread responses decrement the length", async () => {
Expand Down Expand Up @@ -2847,7 +2847,7 @@ describe("Room", function () {
expect(room.eventShouldLiveIn(reply2, events, roots).shouldLiveInThread).toBeFalsy();
});

it("should aggregate relations in thread event timeline set", () => {
it("should aggregate relations in thread event timeline set", async () => {
Thread.setServerSideSupport(FeatureSupport.Stable);
const threadRoot = mkMessage();
const rootReaction = mkReaction(threadRoot);
Expand All @@ -2856,9 +2856,10 @@ describe("Room", function () {

const events = [threadRoot, rootReaction, threadResponse, threadReaction];

const prom = emitPromise(room, ThreadEvent.New);
room.addLiveEvents(events);

const thread = threadRoot.getThread()!;
const thread = await prom;
expect(thread).toBe(threadRoot.getThread());
expect(thread.rootEvent).toBe(threadRoot);

const rootRelations = thread.timelineSet.relations
Expand Down
4 changes: 1 addition & 3 deletions src/models/room.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2090,9 +2090,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
// If we jump to an event in a thread where neither the event, nor the root,
// nor any thread event are loaded yet, we'll load the event as well as the thread root, create the thread,
// and pass the event through this.
for (const event of events) {
thread.setEventMetadata(event);
}
thread.addEvents(events, false);

// If we managed to create a thread and figure out its `id` then we can use it
this.threads.set(thread.id, thread);
Expand Down
Loading