-
Notifications
You must be signed in to change notification settings - Fork 358
fix: Do not retrieve notification events sent by ignored users #5081
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: Do not retrieve notification events sent by ignored users #5081
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5081 +/- ##
==========================================
- Coverage 85.83% 85.79% -0.04%
==========================================
Files 333 333
Lines 36158 36205 +47
==========================================
+ Hits 31037 31063 +26
- Misses 5121 5142 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
crates/matrix-sdk/src/client/mod.rs
Outdated
| pub fn is_user_ignored(&self, user_id: &UserId) -> bool { | ||
| let raw_user_id = user_id.to_string(); | ||
| let current_ignored_user_list = self.subscribe_to_ignore_user_list_changes().get(); | ||
| current_ignored_user_list.contains(&raw_user_id) | ||
| } |
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.
subscribe_to_ignore_user_list_changes is only “runtime-aware”: it knows the changes that happen during the current run of the SDK. If a user has been ignored in a previous instance of the SDK, you will miss it.
I think it's better to use something like this (not tested):
let state_store = get_it_from_somewhere();
let list = state_store.get_account_data_event_static::<IgnoredUserListEvent>()…;The new is_user_ignored is nice! That's a good idea. It's just the body of the method that should change.
Also, the method should be on matrix_sdk_base::Client, not matrix_sdk::Client.
Hywan
left a comment
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.
Almost there! Thanks for the new patch.
crates/matrix-sdk-base/src/client.rs
Outdated
| current_ignored_user_list.content.ignored_users.contains_key(user_id) | ||
| } | ||
| Err(error) => { | ||
| warn!("Failed to deserialize ignored user list event: {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.
| warn!("Failed to deserialize ignored user list event: {error:?}."); | |
| warn!(?error, "Failed to deserialize the ignored user list event"); |
crates/matrix-sdk-base/src/client.rs
Outdated
| }, | ||
| Ok(None) => false, | ||
| Err(error) => { | ||
| warn!("Could not get ignored user list. {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.
| warn!("Could not get ignored user list. {error:?}."); | |
| warn!(?error, "Could not get the ignored user list from the state store"); |
crates/matrix-sdk-base/src/client.rs
Outdated
| let user_id = user_id!("@alice:example.org"); | ||
| let client = | ||
| BaseClient::new(StoreConfig::new("cross-process-store-locks-holder-name".to_owned())); | ||
|
|
||
| client | ||
| .activate( | ||
| SessionMeta { user_id: user_id.to_owned(), device_id: "FOOBAR".into() }, | ||
| RoomLoadSettings::default(), | ||
| #[cfg(feature = "e2e-encryption")] | ||
| None, | ||
| ) | ||
| .await | ||
| .unwrap(); |
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.
| let user_id = user_id!("@alice:example.org"); | |
| let client = | |
| BaseClient::new(StoreConfig::new("cross-process-store-locks-holder-name".to_owned())); | |
| client | |
| .activate( | |
| SessionMeta { user_id: user_id.to_owned(), device_id: "FOOBAR".into() }, | |
| RoomLoadSettings::default(), | |
| #[cfg(feature = "e2e-encryption")] | |
| None, | |
| ) | |
| .await | |
| .unwrap(); | |
| let client = logged_in_base_client(None).await; |
| { | ||
| "content": { | ||
| "avatar_url": sender_avatar_url, | ||
| "displayname": sender_display_name, | ||
| "membership": "join" | ||
| }, | ||
| "room_id": room_id, | ||
| "event_id": "$151800140517rfvjc:example.org", | ||
| "membership": "join", | ||
| "origin_server_ts": 151800140, | ||
| "sender": sender, | ||
| "state_key": sender, | ||
| "type": "m.room.member", | ||
| "unsigned": { | ||
| "age": 2970366, | ||
| } | ||
| }, |
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 you may want to use the EventFactory here 🙄.
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.
These are mostly copy-pasted from other tests, but sure, I can do that.
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.
You'd be my hero if you also rewrote the other existing tests so they used the event factory <3
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 can probably do that on a separate PR.
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.
Ping me and I'll review it with a great pleasure. Thank you, thank you, thank you.
… `Client::is_user_ignored` Also move it to `BaseClient` instead.
c6f5d7b to
f23fdc6
Compare
When retrieving notifications using either sliding sync or the
/contextendpoint, check if the sender is ignored by the user before returning it. If it is, just filter out the notification.Signed-off-by: