Skip to content

Commit ab588f0

Browse files
t3chguyt3chguy
authored
Fix issue with getEventTimeline returning undefined for thread roots in main timeline (matrix-org#2454)
* Fix test message utils using overload * Tweak existing tests * Add test around `MatrixClient::getEventTimeline` * Fix test to actually exercise the faulty behaviour * Extract timelineSet thread belongs logic and test it * tweak method name
1 parent b43b4aa commit ab588f0

File tree

8 files changed

+271
-135
lines changed

8 files changed

+271
-135
lines changed

‎spec/integ/matrix-client-event-timeline.spec.js‎

Lines changed: 56 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,23 @@ const EVENTS = [
7070
}),
7171
];
7272

73-
const THREAD_ROOT = utils.mkMessage({
73+
const THREAD_ROOT = utils.mkEvent({
7474
room: roomId,
7575
user: userId,
76-
msg: "thread root",
76+
type: "m.room.message",
77+
content: {
78+
"body": "thread root",
79+
"msgtype": "m.text",
80+
},
81+
unsigned: {
82+
"m.relations": {
83+
"io.element.thread": {
84+
"latest_event": undefined,
85+
"count": 1,
86+
"current_user_participated": true,
87+
},
88+
},
89+
},
7790
});
7891

7992
const THREAD_REPLY = utils.mkEvent({
@@ -91,6 +104,8 @@ const THREAD_REPLY = utils.mkEvent({
91104
},
92105
});
93106

107+
THREAD_ROOT.unsigned["m.relations"]["io.element.thread"].latest_event = THREAD_REPLY;
108+
94109
// start the client, and wait for it to initialise
95110
function startClient(httpBackend, client) {
96111
httpBackend.when("GET", "/versions").respond(200, {});
@@ -540,20 +555,46 @@ describe("MatrixClient event timelines", function() {
540555
expect(timeline.getEvents().find(e => e.getId() === THREAD_REPLY.event_id)).toBeTruthy();
541556
});
542557

543-
it("should return undefined when event is not within a thread but timelineSet is", async () => {
558+
it("should return relevant timeline from non-thread timelineSet when asking for the thread root", async () => {
544559
client.clientOpts.experimentalThreadSupport = true;
545560
Thread.setServerSideSupport(true);
546561
client.stopClient(); // we don't need the client to be syncing at this time
547562
const room = client.getRoom(roomId);
548563
const threadRoot = new MatrixEvent(THREAD_ROOT);
549564
const thread = room.createThread(THREAD_ROOT.event_id, threadRoot, [threadRoot], false);
550-
const timelineSet = thread.timelineSet;
565+
const timelineSet = room.getTimelineSets()[0];
551566

552-
httpBackend.when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id))
567+
httpBackend.when("GET", "/rooms/!foo%3Abar/context/" + encodeURIComponent(THREAD_ROOT.event_id))
553568
.respond(200, function() {
554-
return THREAD_ROOT;
569+
return {
570+
start: "start_token0",
571+
events_before: [],
572+
event: THREAD_ROOT,
573+
events_after: [],
574+
end: "end_token0",
575+
state: [],
576+
};
555577
});
556578

579+
const [timeline] = await Promise.all([
580+
client.getEventTimeline(timelineSet, THREAD_ROOT.event_id),
581+
httpBackend.flushAllExpected(),
582+
]);
583+
584+
expect(timeline).not.toBe(thread.liveTimeline);
585+
expect(timelineSet.getTimelines()).toContain(timeline);
586+
expect(timeline.getEvents().find(e => e.getId() === THREAD_ROOT.event_id)).toBeTruthy();
587+
});
588+
589+
it("should return undefined when event is not in the thread that the given timelineSet is representing", () => {
590+
client.clientOpts.experimentalThreadSupport = true;
591+
Thread.setServerSideSupport(true);
592+
client.stopClient(); // we don't need the client to be syncing at this time
593+
const room = client.getRoom(roomId);
594+
const threadRoot = new MatrixEvent(THREAD_ROOT);
595+
const thread = room.createThread(THREAD_ROOT.event_id, threadRoot, [threadRoot], false);
596+
const timelineSet = thread.timelineSet;
597+
557598
httpBackend.when("GET", "/rooms/!foo%3Abar/context/" + encodeURIComponent(EVENTS[0].event_id))
558599
.respond(200, function() {
559600
return {
@@ -566,14 +607,13 @@ describe("MatrixClient event timelines", function() {
566607
};
567608
});
568609

569-
const timelinePromise = client.getEventTimeline(timelineSet, EVENTS[0].event_id);
570-
await httpBackend.flushAllExpected();
571-
572-
const timeline = await timelinePromise;
573-
expect(timeline).toBeUndefined();
610+
return Promise.all([
611+
expect(client.getEventTimeline(timelineSet, EVENTS[0].event_id)).resolves.toBeUndefined(),
612+
httpBackend.flushAllExpected(),
613+
]);
574614
});
575615

576-
it("should return undefined when event is within a thread but timelineSet is not", async () => {
616+
it("should return undefined when event is within a thread but timelineSet is not", () => {
577617
client.clientOpts.experimentalThreadSupport = true;
578618
Thread.setServerSideSupport(true);
579619
client.stopClient(); // we don't need the client to be syncing at this time
@@ -592,11 +632,10 @@ describe("MatrixClient event timelines", function() {
592632
};
593633
});
594634

595-
const timelinePromise = client.getEventTimeline(timelineSet, THREAD_REPLY.event_id);
596-
await httpBackend.flushAllExpected();
597-
598-
const timeline = await timelinePromise;
599-
expect(timeline).toBeUndefined();
635+
return Promise.all([
636+
expect(client.getEventTimeline(timelineSet, THREAD_REPLY.event_id)).resolves.toBeUndefined(),
637+
httpBackend.flushAllExpected(),
638+
]);
600639
});
601640

602641
it("should should add lazy loading filter when requested", async () => {

‎spec/test-utils/test-utils.ts‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,6 @@ interface IEventOpts {
7474
sender?: string;
7575
skey?: string;
7676
content: IContent;
77-
event?: boolean;
7877
user?: string;
7978
unsigned?: IUnsigned;
8079
redacts?: string;
@@ -93,7 +92,9 @@ let testEventIndex = 1; // counter for events, easier for comparison of randomly
9392
* @param {MatrixClient} client If passed along with opts.event=true will be used to set up re-emitters.
9493
* @return {Object} a JSON object representing this event.
9594
*/
96-
export function mkEvent(opts: IEventOpts, client?: MatrixClient): object | MatrixEvent {
95+
export function mkEvent(opts: IEventOpts & { event: true }, client?: MatrixClient): MatrixEvent;
96+
export function mkEvent(opts: IEventOpts & { event?: false }, client?: MatrixClient): object;
97+
export function mkEvent(opts: IEventOpts & { event?: boolean }, client?: MatrixClient): object | MatrixEvent {
9798
if (!opts.type || !opts.content) {
9899
throw new Error("Missing .type or .content =>" + JSON.stringify(opts));
99100
}
@@ -143,7 +144,9 @@ interface IPresenceOpts {
143144
* @param {Object} opts Values for the presence.
144145
* @return {Object|MatrixEvent} The event
145146
*/
146-
export function mkPresence(opts: IPresenceOpts): object | MatrixEvent {
147+
export function mkPresence(opts: IPresenceOpts & { event: true }): MatrixEvent;
148+
export function mkPresence(opts: IPresenceOpts & { event?: false }): object;
149+
export function mkPresence(opts: IPresenceOpts & { event?: boolean }): object | MatrixEvent {
147150
const event = {
148151
event_id: "$" + Math.random() + "-" + Math.random(),
149152
type: "m.presence",
@@ -182,7 +185,9 @@ interface IMembershipOpts {
182185
* @param {boolean} opts.event True to make a MatrixEvent.
183186
* @return {Object|MatrixEvent} The event
184187
*/
185-
export function mkMembership(opts: IMembershipOpts): object | MatrixEvent {
188+
export function mkMembership(opts: IMembershipOpts & { event: true }): MatrixEvent;
189+
export function mkMembership(opts: IMembershipOpts & { event?: false }): object;
190+
export function mkMembership(opts: IMembershipOpts & { event?: boolean }): object | MatrixEvent {
186191
const eventOpts: IEventOpts = {
187192
...opts,
188193
type: EventType.RoomMember,
@@ -220,7 +225,9 @@ interface IMessageOpts {
220225
* @param {MatrixClient} client If passed along with opts.event=true will be used to set up re-emitters.
221226
* @return {Object|MatrixEvent} The event
222227
*/
223-
export function mkMessage(opts: IMessageOpts, client?: MatrixClient): object | MatrixEvent {
228+
export function mkMessage(opts: IMessageOpts & { event: true }, client?: MatrixClient): MatrixEvent;
229+
export function mkMessage(opts: IMessageOpts & { event?: false }, client?: MatrixClient): object;
230+
export function mkMessage(opts: IMessageOpts & { event?: boolean }, client?: MatrixClient): object | MatrixEvent {
224231
const eventOpts: IEventOpts = {
225232
...opts,
226233
type: EventType.RoomMessage,
@@ -252,7 +259,12 @@ interface IReplyMessageOpts extends IMessageOpts {
252259
* @param {MatrixClient} client If passed along with opts.event=true will be used to set up re-emitters.
253260
* @return {Object|MatrixEvent} The event
254261
*/
255-
export function mkReplyMessage(opts: IReplyMessageOpts, client?: MatrixClient): object | MatrixEvent {
262+
export function mkReplyMessage(opts: IReplyMessageOpts & { event: true }, client?: MatrixClient): MatrixEvent;
263+
export function mkReplyMessage(opts: IReplyMessageOpts & { event?: false }, client?: MatrixClient): object;
264+
export function mkReplyMessage(
265+
opts: IReplyMessageOpts & { event?: boolean },
266+
client?: MatrixClient,
267+
): object | MatrixEvent {
256268
const eventOpts: IEventOpts = {
257269
...opts,
258270
type: EventType.RoomMessage,

‎spec/unit/event-timeline-set.spec.ts‎

Lines changed: 74 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ import {
2525
Room,
2626
DuplicateStrategy,
2727
} from '../../src';
28+
import { Thread } from "../../src/models/thread";
29+
import { ReEmitter } from "../../src/ReEmitter";
2830

2931
describe('EventTimelineSet', () => {
3032
const roomId = '!foo:bar';
@@ -54,6 +56,7 @@ describe('EventTimelineSet', () => {
5456

5557
beforeEach(() => {
5658
client = utils.mock(MatrixClient, 'MatrixClient');
59+
client.reEmitter = utils.mock(ReEmitter, 'ReEmitter');
5760
room = new Room(roomId, client, userA);
5861
eventTimelineSet = new EventTimelineSet(room);
5962
eventTimeline = new EventTimeline(eventTimelineSet);
@@ -62,14 +65,14 @@ describe('EventTimelineSet', () => {
6265
user: userA,
6366
msg: 'Hi!',
6467
event: true,
65-
}) as MatrixEvent;
68+
});
6669
replyEvent = utils.mkReplyMessage({
6770
room: roomId,
6871
user: userA,
6972
msg: 'Hoo!',
7073
event: true,
7174
replyToMessage: messageEvent,
72-
}) as MatrixEvent;
75+
});
7376
});
7477

7578
describe('addLiveEvent', () => {
@@ -91,7 +94,7 @@ describe('EventTimelineSet', () => {
9194
// make a duplicate
9295
const duplicateMessageEvent = utils.mkMessage({
9396
room: roomId, user: userA, msg: "dupe", event: true,
94-
}) as MatrixEvent;
97+
});
9598
duplicateMessageEvent.event.event_id = messageEvent.getId();
9699

97100
// Adding the duplicate event should replace the `messageEvent`
@@ -220,4 +223,72 @@ describe('EventTimelineSet', () => {
220223
});
221224
});
222225
});
226+
227+
describe("canContain", () => {
228+
const mkThreadResponse = (root: MatrixEvent) => utils.mkEvent({
229+
event: true,
230+
type: EventType.RoomMessage,
231+
user: userA,
232+
room: roomId,
233+
content: {
234+
"body": "Thread response :: " + Math.random(),
235+
"m.relates_to": {
236+
"event_id": root.getId(),
237+
"m.in_reply_to": {
238+
"event_id": root.getId(),
239+
},
240+
"rel_type": "m.thread",
241+
},
242+
},
243+
}, room.client);
244+
245+
let thread: Thread;
246+
247+
beforeEach(() => {
248+
(client.supportsExperimentalThreads as jest.Mock).mockReturnValue(true);
249+
thread = new Thread("!thread_id:server", messageEvent, { room, client });
250+
});
251+
252+
it("should throw if timeline set has no room", () => {
253+
const eventTimelineSet = new EventTimelineSet(undefined, {}, client);
254+
expect(() => eventTimelineSet.canContain(messageEvent)).toThrowError();
255+
});
256+
257+
it("should return false if timeline set is for thread but event is not threaded", () => {
258+
const eventTimelineSet = new EventTimelineSet(room, {}, client, thread);
259+
expect(eventTimelineSet.canContain(replyEvent)).toBeFalsy();
260+
});
261+
262+
it("should return false if timeline set it for thread but event it for a different thread", () => {
263+
const eventTimelineSet = new EventTimelineSet(room, {}, client, thread);
264+
const event = mkThreadResponse(replyEvent);
265+
expect(eventTimelineSet.canContain(event)).toBeFalsy();
266+
});
267+
268+
it("should return false if timeline set is not for a thread but event is a thread response", () => {
269+
const eventTimelineSet = new EventTimelineSet(room, {}, client);
270+
const event = mkThreadResponse(replyEvent);
271+
expect(eventTimelineSet.canContain(event)).toBeFalsy();
272+
});
273+
274+
it("should return true if the timeline set is not for a thread and the event is a thread root", () => {
275+
const eventTimelineSet = new EventTimelineSet(room, {}, client);
276+
expect(eventTimelineSet.canContain(messageEvent)).toBeTruthy();
277+
});
278+
279+
it("should return true if the timeline set is for a thread and the event is its thread root", () => {
280+
const thread = new Thread(messageEvent.getId(), messageEvent, { room, client });
281+
const eventTimelineSet = new EventTimelineSet(room, {}, client, thread);
282+
messageEvent.setThread(thread);
283+
expect(eventTimelineSet.canContain(messageEvent)).toBeTruthy();
284+
});
285+
286+
it("should return true if the timeline set is for a thread and the event is a response to it", () => {
287+
const thread = new Thread(messageEvent.getId(), messageEvent, { room, client });
288+
const eventTimelineSet = new EventTimelineSet(room, {}, client, thread);
289+
messageEvent.setThread(thread);
290+
const event = mkThreadResponse(messageEvent);
291+
expect(eventTimelineSet.canContain(event)).toBeTruthy();
292+
});
293+
});
223294
});

‎spec/unit/filter-component.spec.ts‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,4 @@
1-
import {
2-
MatrixEvent,
3-
RelationType,
4-
} from "../../src";
1+
import { RelationType } from "../../src";
52
import { FilterComponent } from "../../src/filter-component";
63
import { mkEvent } from '../test-utils/test-utils';
74

@@ -14,7 +11,7 @@ describe("Filter Component", function() {
1411
content: { },
1512
room: 'roomId',
1613
event: true,
17-
}) as MatrixEvent;
14+
});
1815

1916
const checkResult = filter.check(event);
2017

@@ -28,7 +25,7 @@ describe("Filter Component", function() {
2825
content: { },
2926
room: 'roomId',
3027
event: true,
31-
}) as MatrixEvent;
28+
});
3229

3330
const checkResult = filter.check(event);
3431

@@ -55,7 +52,7 @@ describe("Filter Component", function() {
5552
},
5653
},
5754
},
58-
}) as MatrixEvent;
55+
});
5956

6057
expect(filter.check(threadRootNotParticipated)).toBe(false);
6158
});
@@ -80,7 +77,7 @@ describe("Filter Component", function() {
8077
user: '@someone-else:server.org',
8178
room: 'roomId',
8279
event: true,
83-
}) as MatrixEvent;
80+
});
8481

8582
expect(filter.check(threadRootParticipated)).toBe(true);
8683
});
@@ -100,7 +97,7 @@ describe("Filter Component", function() {
10097
[RelationType.Reference]: {},
10198
},
10299
},
103-
}) as MatrixEvent;
100+
});
104101

105102
expect(filter.check(referenceRelationEvent)).toBe(false);
106103
});
@@ -123,7 +120,7 @@ describe("Filter Component", function() {
123120
},
124121
room: 'roomId',
125122
event: true,
126-
}) as MatrixEvent;
123+
});
127124

128125
const eventWithMultipleRelations = mkEvent({
129126
"type": "m.room.message",
@@ -148,7 +145,7 @@ describe("Filter Component", function() {
148145
},
149146
"room": 'roomId',
150147
"event": true,
151-
}) as MatrixEvent;
148+
});
152149

153150
const noMatchEvent = mkEvent({
154151
"type": "m.room.message",
@@ -160,7 +157,7 @@ describe("Filter Component", function() {
160157
},
161158
"room": 'roomId',
162159
"event": true,
163-
}) as MatrixEvent;
160+
});
164161

165162
expect(filter.check(threadRootEvent)).toBe(true);
166163
expect(filter.check(eventWithMultipleRelations)).toBe(true);

0 commit comments

Comments
 (0)