feat(base): New `RoomInfoNotableUpdateReasons::READ_RECEIPT` #3680

Hywan · 2024-07-10T13:05:28Z

Please review this PR patch-by-patch.

tl;dr: this PR adds the READ_RECEIPT reason for the room info notable update, in order to refresh the room list for this particular reason. This PR also adds a missing test for the LATEST_EVENT reason (it was tested in matrix-sdk-ui but it's better to also unit test it in matrix-sdk-base directly).

This patch removes the mention of a returned value whilst the function returns nothing.

…sent. This patch adds a missing test to ensure that a `RoomInfoNotableUpdateReason::RECENCY_TIMESTAMP` is correctly sent.

This patch adds the new `RoomInfoNotableUpdateReasons::READ_RECEIPT` reason. It detects it and adds the test to ensure it's sent as expected.

…EAD_RECEIPT`. This patch listens to `RoomInfoNotableUpdateReasons::READ_RECEIPT` to update the `RoomLIst` stream.

bnjbvr

Thanks!

bnjbvr · 2024-07-10T13:07:27Z

crates/matrix-sdk-base/src/sliding_sync.rs

+            room_info_notable_update_stream.recv().await,
+            Ok(RoomInfoNotableUpdate { room_id: received_room_id, reasons: received_reasons }) => {
+                assert_eq!(received_room_id, room_id);
+                assert!(!received_reasons.contains(RoomInfoNotableUpdateReasons::RECENCY_TIMESTAMP));


Does this pass?

assert!(received_reasons.is_empty());

I prefer to test the reason is not present, in case other reasons may exist. We don't care about the other reasons at this step. Tests are less likely to break in the future if these kind of assertions.

crates/matrix-sdk-base/src/sliding_sync.rs

bnjbvr · 2024-07-10T13:08:18Z

crates/matrix-sdk-base/src/sliding_sync.rs

+            Ok(RoomInfoNotableUpdate { room_id: received_room_id, reasons: received_reasons }) => {
+                assert_eq!(received_room_id, room_id);
+                dbg!(&received_reasons);
+                assert!(!received_reasons.contains(RoomInfoNotableUpdateReasons::READ_RECEIPT));


Same as before, can this be a check that reasons is empty?

bnjbvr · 2024-07-10T13:09:04Z

crates/matrix-sdk-base/src/sliding_sync.rs

+        // When I send sliding sync response containing a couple of events with no read receipt.
+        let room_id = room_id!("!r:e.uk");
+        let events = vec![
+            make_raw_event("m.room.message", "$3"),


make_raw_event and make_event should be replaced with the EventFactory 😇

This entire test suite should be revisited. Later. For another time. But I agree!

bnjbvr · 2024-07-10T13:11:00Z

crates/matrix-sdk-base/src/sliding_sync.rs

+        let events = vec![
+            make_raw_event("m.room.message", "$3"),
+            make_raw_event("m.room.message", "$4"),
+            make_raw_event("m.read", "$5"),


Technically this is wrong, as this will create a m.read event with the content of a m.room.message event; it seems compute_unread_counts() still does something with it (store it, maybe?), but this could be a bit better…

There is no RoomReadReceipt before, and now there is one, so the previous and the new one are different, which triggers something. Do you have a better minimal test in mind?

All I'm saying is that the m.read event should be created in a better way (with a proper content property), because it doesn't make sense now :)

Since you've approved the PR, I understand it's fine for this particular test. The read receipt mechanism is already pretty well test. Here we only want to test the notable update reason is triggered, which is fine for the moment.

bnjbvr · 2024-07-10T13:12:36Z

crates/matrix-sdk-base/src/sliding_sync.rs

Please fold 0e73620 into the commits it belongs to (consider git-absorb next time!)

codecov · 2024-07-10T13:26:06Z

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.31%. Comparing base (d78b682) to head (a4e47fd).
Report is 4 commits behind head on main.

Files	Patch %	Lines
...s/matrix-sdk-ui/src/room_list_service/room_list.rs	0.00%	3 Missing ⚠️
crates/matrix-sdk-base/src/sliding_sync.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3680      +/-   ##
==========================================
- Coverage   84.31%   84.31%   -0.01%     
==========================================
  Files         258      258              
  Lines       26601    26607       +6     
==========================================
+ Hits        22429    22433       +4     
- Misses       4172     4174       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hywan added 2 commits July 10, 2024 14:42

doc(base): Remove an outdated documentation information.

657b046

This patch removes the mention of a returned value whilst the function returns nothing.

test(base): Test RoomInfoNotableUpdateReason::RECENCY_TIMESTAMP is …

efd9a75

…sent. This patch adds a missing test to ensure that a `RoomInfoNotableUpdateReason::RECENCY_TIMESTAMP` is correctly sent.

Hywan requested a review from a team as a code owner July 10, 2024 13:05

Hywan requested review from andybalaam and bnjbvr and removed request for a team and andybalaam July 10, 2024 13:05

Hywan added 3 commits July 10, 2024 15:11

feat(base): New RoomInfoNotableUpdateReasons::READ_RECEIPT!

168f285

This patch adds the new `RoomInfoNotableUpdateReasons::READ_RECEIPT` reason. It detects it and adds the test to ensure it's sent as expected.

feat(ui): RoomList is refreshed by `RoomInfoNotableUpdateReasons::R…

b046794

…EAD_RECEIPT`. This patch listens to `RoomInfoNotableUpdateReasons::READ_RECEIPT` to update the `RoomLIst` stream.

chore(base): Format doc.

a4e47fd

Hywan force-pushed the feat-base-room-info-notable-update-read-receipts branch from 0e73620 to a4e47fd Compare July 10, 2024 13:11

bnjbvr approved these changes Jul 10, 2024

View reviewed changes

Hywan merged commit 25bb8e9 into matrix-org:main Jul 10, 2024
40 checks passed

bnjbvr mentioned this pull request Jul 17, 2024

Rooms don’t get marked as unread reliably element-hq/element-x-ios#3034

Closed

Hywan mentioned this pull request Aug 5, 2024

Re-enable RoomInfoNotableUpdateReason #3802

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(base): New `RoomInfoNotableUpdateReasons::READ_RECEIPT` #3680

feat(base): New `RoomInfoNotableUpdateReasons::READ_RECEIPT` #3680

Hywan commented Jul 10, 2024

bnjbvr left a comment

bnjbvr Jul 10, 2024

Hywan Jul 10, 2024

bnjbvr Jul 10, 2024

bnjbvr Jul 10, 2024

Hywan Jul 10, 2024

bnjbvr Jul 10, 2024

Hywan Jul 10, 2024 •

edited

Loading

bnjbvr Jul 10, 2024

Hywan Jul 10, 2024

bnjbvr Jul 10, 2024

codecov bot commented Jul 10, 2024

feat(base): New RoomInfoNotableUpdateReasons::READ_RECEIPT #3680

feat(base): New RoomInfoNotableUpdateReasons::READ_RECEIPT #3680

Conversation

Hywan commented Jul 10, 2024

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hywan Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 10, 2024

Codecov Report

feat(base): New `RoomInfoNotableUpdateReasons::READ_RECEIPT` #3680

feat(base): New `RoomInfoNotableUpdateReasons::READ_RECEIPT` #3680

Hywan Jul 10, 2024 •

edited

Loading