Skip to content

Conversation

@jmartinesp
Copy link
Contributor

@jmartinesp jmartinesp commented May 23, 2025

When retrieving notifications using either sliding sync or the /context endpoint, check if the sender is ignored by the user before returning it. If it is, just filter out the notification.

  • Public API changes documented in changelogs (optional)

Signed-off-by:

@jmartinesp jmartinesp requested a review from a team as a code owner May 23, 2025 12:51
@jmartinesp jmartinesp requested review from poljar and removed request for a team May 23, 2025 12:51
@codecov
Copy link

codecov bot commented May 23, 2025

Codecov Report

Attention: Patch coverage is 70.17544% with 17 lines in your changes missing coverage. Please review.

Project coverage is 85.79%. Comparing base (f0c7370) to head (f23fdc6).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/test_utils/mocks/mod.rs 58.33% 10 Missing ⚠️
crates/matrix-sdk-base/src/client.rs 50.00% 6 Missing ⚠️
crates/matrix-sdk-ui/src/notification_client.rs 94.73% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Hywan Hywan requested review from Hywan and removed request for poljar May 26, 2025 08:17
Comment on lines 2546 to 2548
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)
}
Copy link
Member

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.

Copy link
Member

@Hywan Hywan left a 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.

current_ignored_user_list.content.ignored_users.contains_key(user_id)
}
Err(error) => {
warn!("Failed to deserialize ignored user list event: {error:?}.");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
warn!("Failed to deserialize ignored user list event: {error:?}.");
warn!(?error, "Failed to deserialize the ignored user list event");

},
Ok(None) => false,
Err(error) => {
warn!("Could not get ignored user list. {error:?}.");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
warn!("Could not get ignored user list. {error:?}.");
warn!(?error, "Could not get the ignored user list from the state store");

Comment on lines 1625 to 1637
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();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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;

Comment on lines 1105 to 1121
{
"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,
}
},
Copy link
Member

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 🙄.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.

@jmartinesp jmartinesp force-pushed the fix/do-not-retrieve-notifications-from-ignored-users branch from c6f5d7b to f23fdc6 Compare May 26, 2025 10:48
@jmartinesp jmartinesp enabled auto-merge (rebase) May 26, 2025 10:58
@jmartinesp jmartinesp merged commit e53eaf4 into main May 26, 2025
41 checks passed
@jmartinesp jmartinesp deleted the fix/do-not-retrieve-notifications-from-ignored-users branch May 26, 2025 11:02
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.

4 participants