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.

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

Implement /_matrix/client/v1/rooms/{roomId}/threads #3404

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

qwqtoday
Copy link

@qwqtoday qwqtoday commented Jul 31, 2024

Signed-off-by: Leung Ho Ching <lukas.leung@hotmail.com>

Pull Request Checklist

@qwqtoday qwqtoday requested a review from a team as a code owner July 31, 2024 05:56
@qwqtoday qwqtoday marked this pull request as draft July 31, 2024 06:00
@qwqtoday qwqtoday closed this Jul 31, 2024
@qwqtoday qwqtoday reopened this Aug 9, 2024
@qwqtoday qwqtoday marked this pull request as ready for review August 9, 2024 07:33
@qwqtoday
Copy link
Author

qwqtoday commented Aug 9, 2024

this pr doesn't need tests because it's obviously working

@notramo
Copy link

notramo commented Aug 14, 2024

It needs tests so when new features are added or refactored, it won't break, and other devs who submit different pull requests, don't have to test this feature each time they submit a new PR. Tests are for future stability, the current feature is obviously always working, because if it wouldn't work, the dev would spend more time on it. But when 10 other features are added, nobody wants to manually test this feature 10 times to avoid breaking it while implementing those new features.

@qwqtoday
Copy link
Author

It needs tests so when new features are added or refactored, it won't break, and other devs who submit different pull requests, don't have to test this feature each time they submit a new PR. Tests are for future stability, the current feature is obviously always working, because if it wouldn't work, the dev would spend more time on it. But when 10 other features are added, nobody wants to manually test this feature 10 times to avoid breaking it while implementing those new features.

Sure I will do it later

@qwqtoday
Copy link
Author

@S7evinK can you run the workflow to test it?

Copy link
Contributor

@S7evinK S7evinK left a comment

Choose a reason for hiding this comment

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

Not bad for a start!
A few things to check, though. Feel free to ask in #dendrite-dev if you have questions.

syncapi/storage/tables/interface.go Outdated Show resolved Hide resolved
tab, _, close := newRelationsTable(t, dbType)
defer close()

err = tab.InsertRelation(ctx, nil, room.ID, firstEvent.EventID(), threadReplyEvent.EventID(), "m.room.message", threadReplyEvent.EventID())
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the input is wrong:

InsertRelation(ctx context.Context, txn *sql.Tx, roomID, eventID, childEventID, childEventType, relType string) (err error)

t.Fatal(err)
}
var eventIds []string
eventIds, _, err = tab.SelectThreads(ctx, nil, room.ID, "", 0, 100)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing error check.

t.Fatalf("eventID mismatch: got %s, want %s", eventID, expected)
}
}
eventIds, _, err = tab.SelectThreads(ctx, nil, room.ID, alice.ID, 0, 100)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, missing error check.


var userID string
if include == "participated" {
_, err := spec.NewUserID(device.UserID, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_, err := spec.NewUserID(device.UserID, true)
_, err = spec.NewUserID(device.UserID, true)

}

for _, event := range headeredEvents {
ce, err := synctypes.ToClientEvent(event, synctypes.FormatAll, func(roomID spec.RoomID, senderID spec.SenderID) (*spec.UserID, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the roomID is not needed, as we're already requesting for a specific room?

Suggested change
ce, err := synctypes.ToClientEvent(event, synctypes.FormatAll, func(roomID spec.RoomID, senderID spec.SenderID) (*spec.UserID, error) {
ce, err := synctypes.ToClientEvent(event, synctypes.FormatSync, func(roomID spec.RoomID, senderID spec.SenderID) (*spec.UserID, error) {

@@ -811,3 +811,39 @@ func (d *DatabaseTransaction) RelationsFor(ctx context.Context, roomID, eventID,

return events, prevBatch, nextBatch, nil
}

func (d *DatabaseTransaction) ThreadsFor(ctx context.Context, roomID, userID string, from types.StreamPosition, limit uint64) (
events []*rstypes.HeaderedEvent, prevBatch, nextBatch string, err error,
Copy link
Contributor

Choose a reason for hiding this comment

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

prevBatch is always empty, so can be removed.

Comment on lines +209 to +220
room := test.NewRoom(t, alice)

firstEvent := room.CreateAndInsert(t, alice, "m.room.message", map[string]interface{}{
"body": "first message",
})
threadReplyEvent := room.CreateAndInsert(t, alice, "m.room.message", map[string]interface{}{
"body": "thread reply",
"m.relates_to": map[string]interface{}{
"event_id": firstEvent.EventID(),
"rel_type": threadRelType,
},
})
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an in-memory room, so when you query the relation below, there won't be any events in the database. This means you'll also need to create a newOutputRoomEventsTable(t, dbType) below, like you did with the relations table. Then insert the events.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file needs to be formatted.

Co-authored-by: Till <2353100+S7evinK@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants