-
Notifications
You must be signed in to change notification settings - Fork 270
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
fix(ui): Do not filter out own receipts in load_read_receipts_for_event #4600
Conversation
…_receipts This is consistent with load_user_receipt and avoids suprises in test results that rely on both methods. It also avoids to need to rely on a precise event ID. Signed-off-by: Kévin Commaille <[email protected]>
They are already filtered out where needed in maybe_update_read_receipt. Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4600 +/- ##
==========================================
- Coverage 85.73% 85.73% -0.01%
==========================================
Files 291 291
Lines 33288 33287 -1
==========================================
- Hits 28538 28537 -1
Misses 4750 4750 ☔ View full report in Codecov by Sentry. |
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.
Thanks for fixing <3
.handle_back_paginated_event( | ||
timeline | ||
.factory | ||
.event(carol_event_content) |
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.
can call .text_msg("I am Carol!")
instead of creating the RoomMessageEventContent
manually.
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.
Done
.handle_back_paginated_event( | ||
timeline | ||
.factory | ||
.event(bob_event_content) |
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.
ditto here
.handle_back_paginated_event( | ||
timeline | ||
.factory | ||
.event(alice_event_content) |
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.
and ditto here
Signed-off-by: Kévin Commaille <[email protected]>
Fixes #4517.
It turns out that the bugs found in that test were due to 2 causes:
TestRoomDataProvider
didn't useinitial_user_receipts
but returned hardcoded values.TimelineStateTransaction::load_read_receipts_for_event
, although we need to process all read receipts viaReadReceipts::maybe_update_read_receipt
because it knows how to filter out our own read receipts were needed.