-
-
Notifications
You must be signed in to change notification settings - Fork 674
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
(I am being lazy so can someone finish off this for me?)
#3404
base: main
Are you sure you want to change the base?
Conversation
this pr doesn't need tests because it's obviously working |
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 |
@S7evinK can you run the workflow to test it? |
There was a problem hiding this 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.
tab, _, close := newRelationsTable(t, dbType) | ||
defer close() | ||
|
||
err = tab.InsertRelation(ctx, nil, room.ID, firstEvent.EventID(), threadReplyEvent.EventID(), "m.room.message", threadReplyEvent.EventID()) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_, 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) { |
There was a problem hiding this comment.
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?
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, |
There was a problem hiding this comment.
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.
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, | ||
}, | ||
}) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 <[email protected]>
/_matrix/client/v1/rooms/{roomId}/threads
/_matrix/client/v1/rooms/{roomId}/threads
(I am being lazy so can someone finish off this for me?)
Signed-off-by:
Leung Ho Ching <[email protected]>
Pull Request Checklist