fix(ui): Timeline::send_reply correctly sets up m.mentions.

In #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

@@ -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,
                 )
             }
         };

crates/matrix-sdk-ui/tests/integration/timeline/replies.rs

@@ -20,15 +20,15 @@ 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,
 };
 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,20 +273,20 @@ 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"),
         ),
     ));
 
     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 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::<RoomMessageEventContent>()
+                .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::<RoomMessageEventContent>()
+                .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;
 }