Skip to content
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.

Sign up for GitHub

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

feat(Room): Check if the user is allowed to do a room mention before trying to send a call notify event. #4271

Merged
merged 2 commits into from
Nov 18, 2024
Merged

feat(Room): Check if the user is allowed to do a room mention before trying to send a call notify event. #4271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
116fe07
feat(Room): Check if the user is allowed to do a room mention before …
toger5 Nov 15, 2024
4f52929
add tests
toger5 Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions crates/matrix-sdk/src/room/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2975,6 +2975,10 @@ impl Room {
return Ok(());
}

if !self.can_user_trigger_room_notification(self.own_user_id()).await? {
return Ok(());
}

Comment on lines +2978 to +2981
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a description on the PR as to why this change is needed and more importantly why not still send the call notification but without Mentions::with_room_mention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment we just do not need it. All clients would just ignore a non mention notify event.
But your question makes me think. We could say we still want to send it.
We do use the notify event for the timeline rendering atm. But this really is not what it is supposed to be used for.
Those are really ephemeral events (but we do not have custom ephemeral events).
So sending them if no client reacts to them is just unnecassary.

The question is if we might want to introduce more complexity and allow ringing in rooms that do have all messages notification settings to still ring even though a user sends a "non mention" ring event...

Which is sth I like:

  • A non mention notify will only ring for users that set the room to "all messages"
  • A mention notify will ring for all mentioned users.

So both sides have some control.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So basically we need a lot of UX discussions (also about when to ring and when to just notify)
This is an okay addition to what we have now before we converge on a general behaviour we want for all combinations of this:

  • ring, notify
  • mention, non mention

Copy link
Member

Choose a reason for hiding this comment

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

Okay, fair enough 👍

self.send_call_notification(
self.room_id().to_string().to_owned(),
ApplicationType::Call,
Expand Down
47 changes: 42 additions & 5 deletions crates/matrix-sdk/tests/integration/room/joined.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ use matrix_sdk_test::{
async_test,
mocks::{mock_encryption_state, mock_redaction},
test_json::{self, sync::CUSTOM_ROOM_POWER_LEVELS},
EphemeralTestEvent, GlobalAccountDataTestEvent, JoinedRoomBuilder, SyncResponseBuilder,
DEFAULT_TEST_ROOM_ID,
EphemeralTestEvent, GlobalAccountDataTestEvent, JoinedRoomBuilder, StateTestEvent,
SyncResponseBuilder, DEFAULT_TEST_ROOM_ID,
};
use ruma::{
api::client::{membership::Invite3pidInit, receipt::create_receipt::v3::ReceiptType},
Expand Down Expand Up @@ -635,7 +635,8 @@ async fn test_call_notifications_ring_for_dms() {
let (client, server) = logged_in_client_with_server().await;

let mut sync_builder = SyncResponseBuilder::new();
sync_builder.add_joined_room(JoinedRoomBuilder::default());
let room_builder = JoinedRoomBuilder::default().add_state_event(StateTestEvent::PowerLevels);
sync_builder.add_joined_room(room_builder);
sync_builder.add_global_account_data_event(GlobalAccountDataTestEvent::Direct);

mock_sync(&server, sync_builder.build_json_sync_response(), None).await;
Expand Down Expand Up @@ -678,9 +679,10 @@ async fn test_call_notifications_notify_for_rooms() {
let (client, server) = logged_in_client_with_server().await;

let mut sync_builder = SyncResponseBuilder::new();
sync_builder.add_joined_room(JoinedRoomBuilder::default());

let room_builder = JoinedRoomBuilder::default().add_state_event(StateTestEvent::PowerLevels);
sync_builder.add_joined_room(room_builder);
mock_sync(&server, sync_builder.build_json_sync_response(), None).await;

mock_encryption_state(&server, false).await;

let sync_settings = SyncSettings::new().timeout(Duration::from_millis(3000));
Expand Down Expand Up @@ -715,6 +717,41 @@ async fn test_call_notifications_notify_for_rooms() {
room.send_call_notification_if_needed().await.unwrap();
}

#[async_test]
async fn test_call_notifications_dont_notify_room_without_mention_powerlevel() {
let (client, server) = logged_in_client_with_server().await;

let mut sync_builder = SyncResponseBuilder::new();
let mut power_level_event = StateTestEvent::PowerLevels.into_json_value();
// Allow noone to send room notify events.
*power_level_event.get_mut("content").unwrap().get_mut("notifications").unwrap() =
json!({"room": 101});

sync_builder.add_joined_room(
JoinedRoomBuilder::default().add_state_event(StateTestEvent::Custom(power_level_event)),
);

mock_sync(&server, sync_builder.build_json_sync_response(), None).await;
mock_encryption_state(&server, false).await;

let sync_settings = SyncSettings::new().timeout(Duration::from_millis(3000));
let _response = client.sync_once(sync_settings).await.unwrap();

let room = client.get_room(&DEFAULT_TEST_ROOM_ID).unwrap();
assert!(!room.is_direct().await.unwrap());
assert!(!room.has_active_room_call());

Mock::given(method("PUT"))
.and(path_regex(r"^/_matrix/client/r0/rooms/.*/send/.*"))
.respond_with(ResponseTemplate::new(200).set_body_json(json!({"event_id": "$event_id"})))
// Expect no calls of the send because we dont have permission to notify.
.expect(0)
.mount(&server)
.await;

room.send_call_notification_if_needed().await.unwrap();
}

#[async_test]
async fn test_make_reply_event_doesnt_require_event_cache() {
// Even if we don't have enabled the event cache, we'll resort to using the
Expand Down
3 changes: 3 additions & 0 deletions testing/matrix-sdk-test/src/test_json/sync_events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,9 @@ pub static POWER_LEVELS: Lazy<JsonValue> = Lazy::new(|| {
"kick": 50,
"redact": 50,
"state_default": 50,
"notifications": {
"room": 0
},
"users": {
"@example:localhost": 100,
"@bob:localhost": 0
Expand Down
Loading