From 2f5382388ef675a5d53e18244a13ff4e602f5aa6 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 18 Apr 2024 14:59:04 +0200 Subject: [PATCH] fix(ui): `Timeline::send_reply` correctly sets up `m.mentions`. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In https://github.com/matrix-org/matrix-rust-sdk/pull/2691, I suppose that the way `add_mentions` is computed is… wrong. `AddMentions` is used to automatically infer the `m.mentions` of the reply event based on the original/replied event. The way it was computed was based on the reply event `mentions`, which seems wrong: if the reply contains mentions, then the sender should be part of it? Nah. That's a bug. We want the reply event to automatically mention the sender of the original event if it's not the same as the current user, i.e. the sender of the reply event. This patch fixes the `add_mentions` calculation. This patch also updates a test and adds another test to ensure that `m.mentions` is correctly defined when replying to an event. --- crates/matrix-sdk-ui/src/timeline/mod.rs | 35 ++++- .../tests/integration/timeline/replies.rs | 142 +++++++++++++++--- 2 files changed, 152 insertions(+), 25 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index b78e251977e..d2f6ba85baa 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/mod.rs @@ -281,14 +281,13 @@ impl Timeline { /// Send a reply to the given event. /// - /// Currently only supports events events with an event ID and JSON being + /// Currently it only supports events with an event ID and JSON being /// available (which can be removed by local redactions). This is subject to /// change. Please check [`EventTimelineItem::can_be_replied_to`] to decide /// whether to render a reply button. /// - /// If the `content.mentions` is `Some(_)`, the sender of `reply_item` will - /// be added to the mentions of the reply. If `content.mentions` is `None`, - /// it will be kept as-is. + /// The sender of `reply_item` will be added to the mentions of the reply if + /// and only if `reply_item` has not been written by the sender. /// /// # Arguments /// @@ -312,8 +311,24 @@ impl Timeline { return Err(UnsupportedReplyItem::MISSING_EVENT_ID); }; - let add_mentions = - if content.mentions.is_some() { AddMentions::Yes } else { AddMentions::No }; + let client = self.room().client(); + + // [The specification](https://spec.matrix.org/v1.10/client-server-api/#user-and-room-mentions) says: + // + // > Users should not add their own Matrix ID to the `m.mentions` property as + // > outgoing messages cannot self-notify. + // + // OK, so let's start by assuming we always want to mention the sender of the + // replied event (`reply_item`)… + let mut mention_the_sender_of_the_replied_event = AddMentions::Yes; + + // … but if `reply_item` has been written by the current user, let's toggle + // to `AddMentions::No`. + if let Some(current_user_id) = client.user_id() { + if current_user_id == &reply_item.sender { + mention_the_sender_of_the_replied_event = AddMentions::No; + } + } let content = match reply_item.content() { TimelineItemContent::Message(msg) => { @@ -325,7 +340,11 @@ impl Timeline { content: msg.to_content(), unsigned: Default::default(), }; - content.make_reply_to(&event, forward_thread, add_mentions) + content.make_reply_to( + &event, + forward_thread, + mention_the_sender_of_the_replied_event, + ) } _ => { let Some(raw_event) = reply_item.latest_json() else { @@ -337,7 +356,7 @@ impl Timeline { event_id.to_owned(), self.room().room_id(), forward_thread, - add_mentions, + mention_the_sender_of_the_replied_event, ) } }; diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/replies.rs b/crates/matrix-sdk-ui/tests/integration/timeline/replies.rs index 79ca15abd82..95524e9f3cb 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/replies.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/replies.rs @@ -20,7 +20,7 @@ use ruma::{ AddMentions, ForwardThread, OriginalRoomMessageEvent, Relation, ReplyWithinThread, RoomMessageEventContent, RoomMessageEventContentWithoutRelation, }, - MessageLikeUnsigned, + Mentions, MessageLikeUnsigned, }, owned_event_id, owned_room_id, owned_user_id, room_id, uint, MilliSecondsSinceUnixEpoch, }; @@ -28,7 +28,7 @@ use serde_json::json; use stream_assert::assert_next_matches; use wiremock::{ matchers::{header, method, path_regex}, - Mock, ResponseTemplate, + Mock, Request, ResponseTemplate, }; use crate::{mock_encryption_state, mock_sync}; @@ -273,12 +273,12 @@ async fn test_send_reply() { let (_, mut timeline_stream) = timeline.subscribe_filter_map(|item| item.as_event().cloned()).await; - let event_id_1 = event_id!("$event1"); + let event_id_from_bob = event_id!("$event_from_bob"); sync_builder.add_joined_room(JoinedRoomBuilder::new(room_id).add_timeline_event( event_builder.make_sync_message_event_with_id( &BOB, - event_id_1, - RoomMessageEventContent::text_plain("Hello, World!"), + event_id_from_bob, + RoomMessageEventContent::text_plain("Hello from Bob"), ), )); @@ -286,7 +286,7 @@ async fn test_send_reply() { let _response = client.sync_once(sync_settings.clone()).await.unwrap(); server.reset().await; - let hello_world_item = + let event_from_bob = assert_next_matches!(timeline_stream, VectorDiff::PushBack { value } => value); // Clear the timeline to make sure the old item does not need to be @@ -295,19 +295,35 @@ async fn test_send_reply() { assert_next_matches!(timeline_stream, VectorDiff::Clear); mock_encryption_state(&server, false).await; + + // Now, let's reply to a message sent by `BOB`. Mock::given(method("PUT")) .and(path_regex(r"^/_matrix/client/r0/rooms/.*/send/.*")) - .respond_with( - ResponseTemplate::new(200).set_body_json(json!({ "event_id": "$reply_event" })), - ) + .respond_with(move |req: &Request| { + use ruma::events::room::message::RoomMessageEventContent; + + let reply_event = req + .body_json::() + .expect("Failed to deserialize the event"); + + assert_matches!(reply_event.relates_to, Some(Relation::Reply { in_reply_to: InReplyTo { event_id, .. } }) => { + assert_eq!(event_id, event_id_from_bob); + }); + assert_matches!(reply_event.mentions, Some(Mentions { user_ids, room: false, .. }) => { + assert_eq!(user_ids.len(), 1); + assert!(user_ids.contains(*BOB)); + }); + + ResponseTemplate::new(200).set_body_json(json!({ "event_id": "$reply_event" })) + }) .expect(1) .mount(&server) .await; timeline .send_reply( - RoomMessageEventContentWithoutRelation::text_plain("Hello, Bob!"), - &hello_world_item, + RoomMessageEventContentWithoutRelation::text_plain("Replying to Bob"), + &event_from_bob, ForwardThread::Yes, ) .await @@ -317,9 +333,9 @@ async fn test_send_reply() { assert_matches!(reply_item.send_state(), Some(EventSendState::NotSentYet)); let reply_message = reply_item.content().as_message().unwrap(); - assert_eq!(reply_message.body(), "Hello, Bob!"); + assert_eq!(reply_message.body(), "Replying to Bob"); let in_reply_to = reply_message.in_reply_to().unwrap(); - assert_eq!(in_reply_to.event_id, event_id_1); + assert_eq!(in_reply_to.event_id, event_id_from_bob); // Right now, we don't pass along the replied-to event to the event handler, // so it's not available if the timeline got cleared. Not critical, but // there's notable room for improvement here. @@ -333,14 +349,106 @@ async fn test_send_reply() { assert_matches!(reply_item_remote_echo.send_state(), Some(EventSendState::Sent { .. })); let reply_message = reply_item_remote_echo.content().as_message().unwrap(); - assert_eq!(reply_message.body(), "Hello, Bob!"); + assert_eq!(reply_message.body(), "Replying to Bob"); let in_reply_to = reply_message.in_reply_to().unwrap(); - assert_eq!(in_reply_to.event_id, event_id_1); + assert_eq!(in_reply_to.event_id, event_id_from_bob); // Same as above. // // let replied_to_event = - // assert_matches!(&in_reply_to.event, TimelineDetails::Ready(ev) => ev); - // assert_eq!(replied_to_event.sender(), *BOB); + // assert_matches!(&in_reply_to.event, TimelineDetails::Ready(ev) => + // ev); assert_eq!(replied_to_event.sender(), *BOB); + + server.verify().await; +} + +#[async_test] +async fn test_send_reply_to_self() { + let room_id = room_id!("!a98sd12bjh:example.org"); + let (client, server) = logged_in_client_with_server().await; + let event_builder = EventBuilder::new(); + let sync_settings = SyncSettings::new().timeout(Duration::from_millis(3000)); + + let mut sync_builder = SyncResponseBuilder::new(); + sync_builder.add_joined_room(JoinedRoomBuilder::new(room_id)); + + mock_sync(&server, sync_builder.build_json_sync_response(), None).await; + let _response = client.sync_once(sync_settings.clone()).await.unwrap(); + server.reset().await; + + let room = client.get_room(room_id).unwrap(); + let timeline = room.timeline().await.unwrap(); + let (_, mut timeline_stream) = + timeline.subscribe_filter_map(|item| item.as_event().cloned()).await; + + let event_id_from_self = event_id!("$event_from_self"); + sync_builder.add_joined_room(JoinedRoomBuilder::new(room_id).add_timeline_event( + event_builder.make_sync_message_event_with_id( + client.user_id().expect("Client must have a user ID"), + event_id_from_self, + RoomMessageEventContent::text_plain("Hello from self"), + ), + )); + + mock_sync(&server, sync_builder.build_json_sync_response(), None).await; + let _response = client.sync_once(sync_settings.clone()).await.unwrap(); + server.reset().await; + + let event_from_self = + assert_next_matches!(timeline_stream, VectorDiff::PushBack { value } => value); + + // Clear the timeline to make sure the old item does not need to be + // available in it for the reply to work. + timeline.clear().await; + assert_next_matches!(timeline_stream, VectorDiff::Clear); + + mock_encryption_state(&server, false).await; + + // Now, let's reply to a message sent by the current user. + Mock::given(method("PUT")) + .and(path_regex(r"^/_matrix/client/r0/rooms/.*/send/.*")) + .respond_with(move |req: &Request| { + use ruma::events::room::message::RoomMessageEventContent; + + let reply_event = req + .body_json::() + .expect("Failed to deserialize the event"); + + assert_matches!(reply_event.relates_to, Some(Relation::Reply { in_reply_to: InReplyTo { event_id, .. } }) => { + assert_eq!(event_id, event_id_from_self); + }); + assert!(reply_event.mentions.is_none()); + + ResponseTemplate::new(200).set_body_json(json!({ "event_id": "$reply_event" })) + }) + .expect(1) + .mount(&server) + .await; + + timeline + .send_reply( + RoomMessageEventContentWithoutRelation::text_plain("Replying to self"), + &event_from_self, + ForwardThread::Yes, + ) + .await + .unwrap(); + + let reply_item = assert_next_matches!(timeline_stream, VectorDiff::PushBack { value } => value); + + assert_matches!(reply_item.send_state(), Some(EventSendState::NotSentYet)); + let reply_message = reply_item.content().as_message().unwrap(); + assert_eq!(reply_message.body(), "Replying to self"); + let in_reply_to = reply_message.in_reply_to().unwrap(); + assert_eq!(in_reply_to.event_id, event_id_from_self); + + let diff = timeout(timeline_stream.next(), Duration::from_secs(1)).await.unwrap().unwrap(); + assert_let!(VectorDiff::Set { index: 0, value: reply_item_remote_echo } = diff); + + assert_matches!(reply_item_remote_echo.send_state(), Some(EventSendState::Sent { .. })); + let reply_message = reply_item_remote_echo.content().as_message().unwrap(); + assert_eq!(reply_message.body(), "Replying to self"); + let in_reply_to = reply_message.in_reply_to().unwrap(); + assert_eq!(in_reply_to.event_id, event_id_from_self); server.verify().await; }