Skip to content

Commit

Permalink
Rewrite receipt-handling code (#3901)
Browse files Browse the repository at this point in the history 
* Rewrite receipt-handling code

* Add tests around dangling receipts

* Fix mark as read for some rooms

* Add missing word

---------

Co-authored-by: Florian Duros <florian.duros@ormaz.fr>
Co-authored-by: Florian Duros <florianduros@element.io>
  • Loading branch information
@andybalaam @florianduros
3 people committed Nov 28, 2023
1 parent a749662 commit c49a527
Show file tree
Hide file tree
Showing 12 changed files with 1,386 additions and 81 deletions.
20 changes: 20 additions & 0 deletions spec/test-utils/thread.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,23 @@ export const mkThread = ({

return { thread, rootEvent, events };
};

/**
* Create a thread, and make sure the events are added to the thread and the
* room's timeline as if they came in via sync.
*
* Note that mkThread doesn't actually add the events properly to the room.
*/
export const populateThread = ({
room,
client,
authorId,
participantUserIds,
length = 2,
ts = 1,
}: MakeThreadProps): MakeThreadResult => {
const ret = mkThread({ room, client, authorId, participantUserIds, length, ts });
ret.thread.initialEventsFetched = true;
room.addLiveEvents(ret.events);
return ret;
};
541 changes: 541 additions & 0 deletions spec/unit/models/room-receipts.spec.ts
View file

Large diffs are not rendered by default.

101 changes: 69 additions & 32 deletions spec/unit/models/thread.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { mocked } from "jest-mock";
import { MatrixClient, PendingEventOrdering } from "../../../src/client";
import { Room, RoomEvent } from "../../../src/models/room";
import { FeatureSupport, Thread, THREAD_RELATION_TYPE, ThreadEvent } from "../../../src/models/thread";
import { makeThreadEvent, mkThread } from "../../test-utils/thread";
import { makeThreadEvent, mkThread, populateThread } from "../../test-utils/thread";
import { TestClient } from "../../TestClient";
import { emitPromise, mkEdit, mkMessage, mkReaction, mock } from "../../test-utils/test-utils";
import { Direction, EventStatus, EventType, MatrixEvent } from "../../../src";
Expand Down Expand Up @@ -149,20 +149,38 @@ describe("Thread", () => {
});

it("considers other events with no RR as unread", () => {
const { thread, events } = mkThread({
// Given a long thread exists
const { thread, events } = populateThread({
room,
client,
authorId: myUserId,
participantUserIds: [myUserId],
authorId: "@other:foo.com",
participantUserIds: ["@other:foo.com"],
length: 25,
ts: 190,
});

// Before alice's last unthreaded receipt
expect(thread.hasUserReadEvent("@alice:example.org", events.at(1)!.getId() ?? "")).toBeTruthy();
const event1 = events.at(1)!;
const event2 = events.at(2)!;
const event24 = events.at(24)!;

// And we have read the second message in it with an unthreaded receipt
const receipt = new MatrixEvent({
type: "m.receipt",
room_id: room.roomId,
content: {
// unthreaded receipt for the second message in the thread
[event2.getId()!]: {
[ReceiptType.Read]: {
[myUserId]: { ts: 200 },
},
},
},
});
room.addReceipt(receipt);

// After alice's last unthreaded receipt
expect(thread.hasUserReadEvent("@alice:example.org", events.at(-1)!.getId() ?? "")).toBeFalsy();
// Then we have read the first message in the thread, and not the last
expect(thread.hasUserReadEvent(myUserId, event1.getId()!)).toBe(true);
expect(thread.hasUserReadEvent(myUserId, event24.getId()!)).toBe(false);
});

it("considers event as read if there's a more recent unthreaded receipt", () => {
Expand Down Expand Up @@ -481,13 +499,13 @@ describe("Thread", () => {

// And a thread with an added event (with later timestamp)
const userId = "user1";
const { thread, message } = await createThreadAndEvent(client, 1, 100, userId);
const { thread, message2 } = await createThreadAnd2Events(client, 1, 100, 200, userId);

// Then a receipt was added to the thread
const receipt = thread.getReadReceiptForUserId(userId);
expect(receipt).toBeTruthy();
expect(receipt?.eventId).toEqual(message.getId());
expect(receipt?.data.ts).toEqual(100);
expect(receipt?.eventId).toEqual(message2.getId());
expect(receipt?.data.ts).toEqual(200);
expect(receipt?.data.thread_id).toEqual(thread.id);

// (And the receipt was synthetic)
Expand All @@ -505,14 +523,14 @@ describe("Thread", () => {

// And a thread with an added event with a lower timestamp than its other events
const userId = "user1";
const { thread } = await createThreadAndEvent(client, 200, 100, userId);
const { thread, message1 } = await createThreadAnd2Events(client, 300, 200, 100, userId);

// Then no receipt was added to the thread (the receipt is still
// for the thread root). This happens because since we have no
// Then the receipt is for the first message, because its
// timestamp is later. This happens because since we have no
// recursive relations support, we know that sometimes events
// appear out of order, so we have to check their timestamps as
// a guess of the correct order.
expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(thread.rootEvent?.getId());
expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(message1.getId());
});
});

Expand All @@ -530,11 +548,11 @@ describe("Thread", () => {

// And a thread with an added event (with later timestamp)
const userId = "user1";
const { thread, message } = await createThreadAndEvent(client, 1, 100, userId);
const { thread, message2 } = await createThreadAnd2Events(client, 1, 100, 200, userId);

// Then a receipt was added to the thread
const receipt = thread.getReadReceiptForUserId(userId);
expect(receipt?.eventId).toEqual(message.getId());
expect(receipt?.eventId).toEqual(message2.getId());
});

it("Creates a local echo receipt even for events BEFORE an existing receipt", async () => {
Expand All @@ -550,22 +568,24 @@ describe("Thread", () => {

// And a thread with an added event with a lower timestamp than its other events
const userId = "user1";
const { thread, message } = await createThreadAndEvent(client, 200, 100, userId);
const { thread, message2 } = await createThreadAnd2Events(client, 300, 200, 100, userId);

// Then a receipt was added to the thread, because relations
// recursion is available, so we trust the server to have
// provided us with events in the right order.
// Then a receipt was added for the last message, even though it
// has lower ts, because relations recursion is available, so we
// trust the server to have provided us with events in the right
// order.
const receipt = thread.getReadReceiptForUserId(userId);
expect(receipt?.eventId).toEqual(message.getId());
expect(receipt?.eventId).toEqual(message2.getId());
});
});

async function createThreadAndEvent(
async function createThreadAnd2Events(
client: MatrixClient,
rootTs: number,
eventTs: number,
message1Ts: number,
message2Ts: number,
userId: string,
): Promise<{ thread: Thread; message: MatrixEvent }> {
): Promise<{ thread: Thread; message1: MatrixEvent; message2: MatrixEvent }> {
const room = new Room("room1", client, userId);

// Given a thread
Expand All @@ -576,24 +596,41 @@ describe("Thread", () => {
participantUserIds: [],
ts: rootTs,
});
// Sanity: the current receipt is for the thread root
expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(thread.rootEvent?.getId());
// Sanity: there is no read receipt on the thread yet because the
// thread events don't get properly added to the room by mkThread.
expect(thread.getReadReceiptForUserId(userId)).toBeNull();

const awaitTimelineEvent = new Promise<void>((res) => thread.on(RoomEvent.Timeline, () => res()));

// When we add a message that is before the latest receipt
const message = makeThreadEvent({
// Add a message with ts message1Ts
const message1 = makeThreadEvent({
event: true,
rootEventId: thread.id,
replyToEventId: thread.id,
user: userId,
room: room.roomId,
ts: message1Ts,
});
await thread.addEvent(message1, false, true);
await awaitTimelineEvent;

// Sanity: the thread now has a properly-added event, so this event
// has a synthetic receipt.
expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(message1.getId());

// Add a message with ts message2Ts
const message2 = makeThreadEvent({
event: true,
rootEventId: thread.id,
replyToEventId: thread.id,
user: userId,
room: room.roomId,
ts: eventTs,
ts: message2Ts,
});
await thread.addEvent(message, false, true);
await thread.addEvent(message2, false, true);
await awaitTimelineEvent;

return { thread, message };
return { thread, message1, message2 };
}

function createClientWithEventMapper(canSupport: Map<Feature, ServerSupport> = new Map()): MatrixClient {
Expand Down
15 changes: 15 additions & 0 deletions spec/unit/read-receipt.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,10 @@ const THREAD_ID = "$thread_event_id";
const ROOM_ID = "!123:matrix.org";

describe("Read receipt", () => {
let threadRoot: MatrixEvent;
let threadEvent: MatrixEvent;
let roomEvent: MatrixEvent;
let editOfThreadRoot: MatrixEvent;

beforeEach(() => {
httpBackend = new MockHttpBackend();
Expand All @@ -57,6 +59,15 @@ describe("Read receipt", () => {
client.isGuest = () => false;
client.supportsThreads = () => true;

threadRoot = utils.mkEvent({
event: true,
type: EventType.RoomMessage,
user: "@bob:matrix.org",
room: ROOM_ID,
content: { body: "This is the thread root" },
});
threadRoot.event.event_id = THREAD_ID;

threadEvent = utils.mkEvent({
event: true,
type: EventType.RoomMessage,
Expand All @@ -82,6 +93,9 @@ describe("Read receipt", () => {
body: "Hello from a room",
},
});

editOfThreadRoot = utils.mkEdit(threadRoot, client, "@bob:matrix.org", ROOM_ID);
editOfThreadRoot.setThreadId(THREAD_ID);
});

describe("sendReceipt", () => {
Expand Down Expand Up @@ -208,6 +222,7 @@ describe("Read receipt", () => {
it.each([
{ getEvent: () => roomEvent, destinationId: MAIN_ROOM_TIMELINE },
{ getEvent: () => threadEvent, destinationId: THREAD_ID },
{ getEvent: () => editOfThreadRoot, destinationId: MAIN_ROOM_TIMELINE },
])("adds the receipt to $destinationId", ({ getEvent, destinationId }) => {
const event = getEvent();
const userId = "@bob:example.org";
Expand Down
63 changes: 60 additions & 3 deletions spec/unit/room.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1743,13 +1743,70 @@ describe("Room", function () {
});

describe("hasUserReadUpTo", function () {
it("should acknowledge if an event has been read", function () {
it("returns true if there is a receipt for this event (main timeline)", function () {
const ts = 13787898424;
room.addLiveEvents([eventToAck]);
room.addReceipt(mkReceipt(roomId, [mkRecord(eventToAck.getId()!, "m.read", userB, ts)]));
room.findEventById = jest.fn().mockReturnValue({} as MatrixEvent);
room.findEventById = jest.fn().mockReturnValue({ getThread: jest.fn() } as unknown as MatrixEvent);
expect(room.hasUserReadEvent(userB, eventToAck.getId()!)).toEqual(true);
});
it("return false for an unknown event", function () {

it("returns true if there is a receipt for a later event (main timeline)", async function () {
// Given some events exist in the room
const events: MatrixEvent[] = [
utils.mkMessage({
room: roomId,
user: userA,
msg: "1111",
event: true,
}),
utils.mkMessage({
room: roomId,
user: userA,
msg: "2222",
event: true,
}),
utils.mkMessage({
room: roomId,
user: userA,
msg: "3333",
event: true,
}),
];
await room.addLiveEvents(events);

// When I add a receipt for the latest one
room.addReceipt(mkReceipt(roomId, [mkRecord(events[2].getId()!, "m.read", userB, 102)]));

// Then the older ones are read too
expect(room.hasUserReadEvent(userB, events[0].getId()!)).toEqual(true);
expect(room.hasUserReadEvent(userB, events[1].getId()!)).toEqual(true);
});

describe("threads enabled", () => {
beforeEach(() => {
jest.spyOn(room.client, "supportsThreads").mockReturnValue(true);
});

afterEach(() => {
jest.restoreAllMocks();
});

it("returns true if there is an unthreaded receipt for a later event in a thread", async () => {
// Given a thread exists in the room
const { thread, events } = mkThread({ room, length: 3 });
thread.initialEventsFetched = true;
await room.addLiveEvents(events);

// When I add an unthreaded receipt for the latest thread message
room.addReceipt(mkReceipt(roomId, [mkRecord(events[2].getId()!, "m.read", userB, 102)]));

// Then the main timeline message is read
expect(room.hasUserReadEvent(userB, events[0].getId()!)).toEqual(true);
});
});

it("returns false for an unknown event", function () {
expect(room.hasUserReadEvent(userB, "unknown_event")).toEqual(false);
});
});
Expand Down
4 changes: 2 additions & 2 deletions src/client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5168,7 +5168,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa

const room = this.getRoom(event.getRoomId());
if (room && this.credentials.userId) {
room.addLocalEchoReceipt(this.credentials.userId, event, receiptType);
room.addLocalEchoReceipt(this.credentials.userId, event, receiptType, unthreaded);
}
return promise;
}
Expand Down Expand Up @@ -9916,7 +9916,7 @@ export function threadIdForReceipt(event: MatrixEvent): string {
* @returns true if this event is considered to be in the main timeline as far
* as receipts are concerned.
*/
function inMainTimelineForReceipt(event: MatrixEvent): boolean {
export function inMainTimelineForReceipt(event: MatrixEvent): boolean {
if (!event.threadRootId) {
// Not in a thread: then it is in the main timeline
return true;
Expand Down

0 comments on commit c49a527

Please sign in to comment.