feat: Mark reactions as IMAP-seen in marknoticed_chat() (#6210)

iequidoo · iequidoo · commit 3aa1e03a7b82 · 2024-12-12T16:11:46.000-03:00
When a reaction notification is shown in the UIs, there's an option "Mark Read", but the UIs are
unaware of reactions message ids, moreover there are no `msgs` rows for incoming reactions in the
core, so the UIs just call `marknoticed_chat()` in this case. We don't want to introduce reactions
message ids to the UIs (at least currently), but let's make received reactions usual messages, just
hidden and `InFresh`, so that the existing `\Seen` flag synchronisation mechanism works for them,
and mark the last fresh hidden incoming message in the chat as seen in `marknoticed_chat()` to
trigger emitting `MsgsNoticed` on other devices.

There's a problem though that another device may have more reactions received and not yet seen
notifications are removed from it when handling `MsgsNoticed`, but the same problem already exists
for "usual" messages, so let's not solve it for now.

It's interesting that sent out reactions are already hidden messages, so this change mostly just
unifies things.
diff --git a/deltachat-rpc-client/src/deltachat_rpc_client/const.py b/deltachat-rpc-client/src/deltachat_rpc_client/const.py
@@ -39,6 +39,7 @@ class EventType(str, Enum):
     ERROR_SELF_NOT_IN_GROUP = "ErrorSelfNotInGroup"
     MSGS_CHANGED = "MsgsChanged"
     REACTIONS_CHANGED = "ReactionsChanged"
+    INCOMING_REACTION = "IncomingReaction"
     INCOMING_MSG = "IncomingMsg"
     INCOMING_MSG_BUNCH = "IncomingMsgBunch"
     MSGS_NOTICED = "MsgsNoticed"
diff --git a/deltachat-rpc-client/tests/test_something.py b/deltachat-rpc-client/tests/test_something.py
@@ -285,6 +285,43 @@ def test_message(acfactory) -> None:
     assert reactions == snapshot.reactions
 
 
+def test_reaction_seen_on_another_dev(acfactory, tmp_path) -> None:
+    alice, bob = acfactory.get_online_accounts(2)
+    alice.export_backup(tmp_path)
+    files = list(tmp_path.glob("*.tar"))
+    alice2 = acfactory.get_unconfigured_account()
+    alice2.import_backup(files[0])
+    alice2.start_io()
+
+    bob_addr = bob.get_config("addr")
+    alice_contact_bob = alice.create_contact(bob_addr, "Bob")
+    alice_chat_bob = alice_contact_bob.create_chat()
+    alice_chat_bob.send_text("Hello!")
+
+    event = bob.wait_for_incoming_msg_event()
+    msg_id = event.msg_id
+
+    message = bob.get_message_by_id(msg_id)
+    snapshot = message.get_snapshot()
+    snapshot.chat.accept()
+    message.send_reaction("😎")
+    for a in [alice, alice2]:
+        while True:
+            event = a.wait_for_event()
+            if event.kind == EventType.INCOMING_REACTION:
+                break
+
+    alice_chat_bob.mark_noticed()
+    while True:
+        event = alice2.wait_for_event()
+        if event.kind == EventType.MSGS_NOTICED:
+            chat_id = event.chat_id
+            break
+    alice2_contact_bob = alice2.get_contact_by_addr(bob_addr)
+    alice2_chat_bob = alice2_contact_bob.create_chat()
+    assert chat_id == alice2_chat_bob.id
+
+
 def test_is_bot(acfactory) -> None:
     """Test that we can recognize messages submitted by bots."""
     alice, bob = acfactory.get_online_accounts(2)
diff --git a/src/chat.rs b/src/chat.rs
@@ -39,6 +39,7 @@ use crate::mimeparser::SystemMessage;
 use crate::param::{Param, Params};
 use crate::peerstate::Peerstate;
 use crate::receive_imf::ReceivedMsg;
+use crate::rusqlite::OptionalExtension;
 use crate::securejoin::BobState;
 use crate::smtp::send_msg_to_smtp;
 use crate::sql;
@@ -3255,10 +3256,10 @@ pub async fn marknoticed_chat(context: &Context, chat_id: ChatId) -> Result<()>
             .query_map(
                 "SELECT DISTINCT(m.chat_id) FROM msgs m
                     LEFT JOIN chats c ON m.chat_id=c.id
-                    WHERE m.state=10 AND m.hidden=0 AND m.chat_id>9 AND c.blocked=0 AND c.archived=1",
-                    (),
+                    WHERE m.state=10 AND m.chat_id>9 AND c.blocked=0 AND c.archived=1",
+                (),
                 |row| row.get::<_, ChatId>(0),
-                |ids| ids.collect::<Result<Vec<_>, _>>().map_err(Into::into)
+                |ids| ids.collect::<Result<Vec<_>, _>>().map_err(Into::into),
             )
             .await?;
         if chat_ids_in_archive.is_empty() {
@@ -3269,7 +3270,7 @@ pub async fn marknoticed_chat(context: &Context, chat_id: ChatId) -> Result<()>
             .sql
             .execute(
                 &format!(
-                    "UPDATE msgs SET state=13 WHERE state=10 AND hidden=0 AND chat_id IN ({});",
+                    "UPDATE msgs SET state=13 WHERE state=10 AND chat_id IN ({});",
                     sql::repeat_vars(chat_ids_in_archive.len())
                 ),
                 rusqlite::params_from_iter(&chat_ids_in_archive),
@@ -3279,20 +3280,61 @@ pub async fn marknoticed_chat(context: &Context, chat_id: ChatId) -> Result<()>
             context.emit_event(EventType::MsgsNoticed(chat_id_in_archive));
             chatlist_events::emit_chatlist_item_changed(context, chat_id_in_archive);
         }
-    } else if context
-        .sql
-        .execute(
-            "UPDATE msgs
-            SET state=?
-          WHERE state=?
-            AND hidden=0
-            AND chat_id=?;",
-            (MessageState::InNoticed, MessageState::InFresh, chat_id),
-        )
-        .await?
-        == 0
-    {
-        return Ok(());
+    } else {
+        let conn_fn = |conn: &mut rusqlite::Connection| {
+            // This is to trigger emitting `MsgsNoticed` on other devices when reactions are noticed
+            // locally. We filter out `InNoticed` messages because they are normally a result of
+            // `mark_old_messages_as_noticed()` which happens on all devices anyway. Also we limit
+            // this to one message because the effect is the same anyway.
+            //
+            // Even if `message::markseen_msgs()` fails then, in the worst case other devices won't
+            // emit `MsgsNoticed` and app notifications won't be removed. The bigger problem is that
+            // another device may have more reactions received and not yet seen notifications are
+            // removed from it, but the same problem already exists for "usual" messages, so let's
+            // not solve it for now.
+            let mut stmt = conn.prepare(
+                "SELECT id, state FROM msgs
+                    WHERE (state=? OR state=? OR state=?)
+                        AND hidden=1
+                        AND chat_id=?
+                    ORDER BY id DESC LIMIT 1",
+            )?;
+            let id_to_markseen = stmt
+                .query_row(
+                    (
+                        MessageState::InFresh,
+                        MessageState::InNoticed,
+                        MessageState::InSeen,
+                        chat_id,
+                    ),
+                    |row| {
+                        let id: MsgId = row.get(0)?;
+                        let state: MessageState = row.get(1)?;
+                        Ok((id, state))
+                    },
+                )
+                .optional()?
+                .filter(|&(_, state)| state == MessageState::InFresh)
+                .map(|(id, _)| id);
+
+            // NB: Enumerate `hidden` values to employ `msgs_index7`.
+            let nr_msgs_noticed = conn.execute(
+                "UPDATE msgs
+                    SET state=?
+                    WHERE state=?
+                        AND (hidden=0 OR hidden=1)
+                        AND chat_id=?",
+                (MessageState::InNoticed, MessageState::InFresh, chat_id),
+            )?;
+            Ok((nr_msgs_noticed, id_to_markseen))
+        };
+        let (nr_msgs_noticed, id_to_markseen) = context.sql.call_write(conn_fn).await?;
+        if nr_msgs_noticed == 0 {
+            return Ok(());
+        }
+        if let Some(id) = id_to_markseen {
+            message::markseen_msgs(context, vec![id]).await?;
+        }
     }
 
     context.emit_event(EventType::MsgsNoticed(chat_id));
@@ -3337,7 +3379,6 @@ pub(crate) async fn mark_old_messages_as_noticed(
                     "UPDATE msgs
             SET state=?
           WHERE state=?
-            AND hidden=0
             AND chat_id=?
             AND timestamp<=?;",
                     (
diff --git a/src/reaction.rs b/src/reaction.rs
@@ -397,7 +397,7 @@ mod tests {
     use deltachat_contact_tools::ContactAddress;
 
     use super::*;
-    use crate::chat::{forward_msgs, get_chat_msgs, send_text_msg};
+    use crate::chat::{forward_msgs, get_chat_msgs, marknoticed_chat, send_text_msg};
     use crate::chatlist::Chatlist;
     use crate::config::Config;
     use crate::contact::{Contact, Origin};
@@ -663,7 +663,8 @@ Here's my footer -- bob@example.net"
         assert_eq!(get_chat_msgs(&bob, bob_msg.chat_id).await?.len(), 2);
 
         let bob_reaction_msg = bob.pop_sent_msg().await;
-        alice.recv_msg_trash(&bob_reaction_msg).await;
+        let alice_reaction_msg = alice.recv_msg_hidden(&bob_reaction_msg).await;
+        assert_eq!(alice_reaction_msg.state, MessageState::InFresh);
         assert_eq!(get_chat_msgs(&alice, chat_alice.id).await?.len(), 2);
 
         let reactions = get_msg_reactions(&alice, alice_msg.sender_msg_id).await?;
@@ -680,6 +681,20 @@ Here's my footer -- bob@example.net"
         expect_incoming_reactions_event(&alice, alice_msg.sender_msg_id, *bob_id, "👍").await?;
         expect_no_unwanted_events(&alice).await;
 
+        marknoticed_chat(&alice, chat_alice.id).await?;
+        assert_eq!(
+            alice_reaction_msg.id.get_state(&alice).await?,
+            MessageState::InSeen
+        );
+        // Reactions don't request MDNs.
+        assert_eq!(
+            alice
+                .sql
+                .count("SELECT COUNT(*) FROM smtp_mdns", ())
+                .await?,
+            0
+        );
+
         // Alice reacts to own message.
         send_reaction(&alice, alice_msg.sender_msg_id, "👍 😀")
             .await
@@ -719,7 +734,7 @@ Here's my footer -- bob@example.net"
         bob_msg1.chat_id.accept(&bob).await?;
         send_reaction(&bob, bob_msg1.id, "👍").await?;
         let bob_send_reaction = bob.pop_sent_msg().await;
-        alice.recv_msg_trash(&bob_send_reaction).await;
+        alice.recv_msg_hidden(&bob_send_reaction).await;
         expect_incoming_reactions_event(&alice, alice_msg1.sender_msg_id, alice_bob_id, "👍")
             .await?;
         expect_no_unwanted_events(&alice).await;
@@ -882,7 +897,7 @@ Here's my footer -- bob@example.net"
         let bob_reaction_msg = bob.pop_sent_msg().await;
 
         // Alice receives a reaction.
-        alice.recv_msg_trash(&bob_reaction_msg).await;
+        alice.recv_msg_hidden(&bob_reaction_msg).await;
 
         let reactions = get_msg_reactions(&alice, alice_msg_id).await?;
         assert_eq!(reactions.to_string(), "👍1");
@@ -934,7 +949,7 @@ Here's my footer -- bob@example.net"
         {
             send_reaction(&alice2, alice2_msg.id, "👍").await?;
             let msg = alice2.pop_sent_msg().await;
-            alice1.recv_msg_trash(&msg).await;
+            alice1.recv_msg_hidden(&msg).await;
         }
 
         // Check that the status is still the same.
@@ -956,7 +971,7 @@ Here's my footer -- bob@example.net"
         let alice1_msg = alice1.recv_msg(&alice0.pop_sent_msg().await).await;
 
         send_reaction(&alice0, alice0_msg_id, "👀").await?;
-        alice1.recv_msg_trash(&alice0.pop_sent_msg().await).await;
+        alice1.recv_msg_hidden(&alice0.pop_sent_msg().await).await;
 
         expect_reactions_changed_event(&alice0, chat_id, alice0_msg_id, ContactId::SELF).await?;
         expect_reactions_changed_event(&alice1, alice1_msg.chat_id, alice1_msg.id, ContactId::SELF)
diff --git a/src/receive_imf.rs b/src/receive_imf.rs
@@ -54,6 +54,9 @@ pub struct ReceivedMsg {
     /// Received message state.
     pub state: MessageState,
 
+    /// Whether the message is hidden.
+    pub hidden: bool,
+
     /// Message timestamp for sorting.
     pub sort_timestamp: i64,
 
@@ -192,6 +195,7 @@ pub(crate) async fn receive_imf_inner(
             return Ok(Some(ReceivedMsg {
                 chat_id: DC_CHAT_ID_TRASH,
                 state: MessageState::Undefined,
+                hidden: false,
                 sort_timestamp: 0,
                 msg_ids,
                 needs_delete_job: false,
@@ -373,6 +377,7 @@ pub(crate) async fn receive_imf_inner(
                 received_msg = Some(ReceivedMsg {
                     chat_id: DC_CHAT_ID_TRASH,
                     state: MessageState::InSeen,
+                    hidden: false,
                     sort_timestamp: mime_parser.timestamp_sent,
                     msg_ids: vec![msg_id],
                     needs_delete_job: res == securejoin::HandshakeMessage::Done,
@@ -611,7 +616,11 @@ pub(crate) async fn receive_imf_inner(
     } else if !chat_id.is_trash() {
         let fresh = received_msg.state == MessageState::InFresh;
         for msg_id in &received_msg.msg_ids {
-            chat_id.emit_msg_event(context, *msg_id, mime_parser.incoming && fresh);
+            chat_id.emit_msg_event(
+                context,
+                *msg_id,
+                mime_parser.incoming && fresh && !received_msg.hidden,
+            );
         }
     }
     context.new_msgs_notify.notify_one();
@@ -760,7 +769,7 @@ async fn add_parts(
     // (of course, the user can add other chats manually later)
     let to_id: ContactId;
     let state: MessageState;
-    let mut hidden = false;
+    let mut hidden = is_reaction;
     let mut needs_delete_job = false;
     let mut restore_protection = false;
 
@@ -1021,7 +1030,9 @@ async fn add_parts(
         state = if seen
             || fetching_existing_messages
             || is_mdn
-            || is_reaction
+            // Currently only reactions are hidden, so this check is just in case we have other
+            // hidden messages in the future to make `chat::marknoticed_chat()` no-op for them.
+            || (hidden && !is_reaction)
             || chat_id_blocked == Blocked::Yes
         {
             MessageState::InSeen
@@ -1232,7 +1243,7 @@ async fn add_parts(
     }
 
     let orig_chat_id = chat_id;
-    let mut chat_id = if is_mdn || is_reaction {
+    let mut chat_id = if is_mdn {
         DC_CHAT_ID_TRASH
     } else {
         chat_id.unwrap_or_else(|| {
@@ -1597,10 +1608,10 @@ RETURNING id
                     state,
                     is_dc_message,
                     if trash { "" } else { msg },
-                    if trash { None } else { message::normalize_text(msg) },
-                    if trash { "" } else { &subject },
+                    if trash || hidden { None } else { message::normalize_text(msg) },
+                    if trash || hidden { "" } else { &subject },
                     // txt_raw might contain invalid utf8
-                    if trash { "" } else { &txt_raw },
+                    if trash || hidden { "" } else { &txt_raw },
                     if trash {
                         "".to_string()
                     } else {
@@ -1616,7 +1627,7 @@ RETURNING id
                     mime_in_reply_to,
                     mime_references,
                     save_mime_modified,
-                    part.error.as_deref().unwrap_or_default(),
+                    if trash || hidden { "" } else { part.error.as_deref().unwrap_or_default() },
                     ephemeral_timer,
                     ephemeral_timestamp,
                     if is_partial_download.is_some() {
@@ -1626,7 +1637,7 @@ RETURNING id
                     } else {
                         DownloadState::Done
                     },
-                    mime_parser.hop_info
+                    if trash || hidden { "" } else { &mime_parser.hop_info },
                 ],
                 |row| {
                     let msg_id: MsgId = row.get(0)?;
@@ -1733,6 +1744,7 @@ RETURNING id
     Ok(ReceivedMsg {
         chat_id,
         state,
+        hidden,
         sort_timestamp,
         msg_ids: created_db_entries,
         needs_delete_job,
diff --git a/src/test_utils.rs b/src/test_utils.rs
@@ -606,6 +606,19 @@ impl TestContext {
         msg
     }
 
+    /// Receive a message using the `receive_imf()` pipeline. Panics if it's not hidden.
+    pub async fn recv_msg_hidden(&self, msg: &SentMessage<'_>) -> Message {
+        let received = self
+            .recv_msg_opt(msg)
+            .await
+            .expect("receive_imf() seems not to have added a new message to the db");
+        let msg = Message::load_from_db(self, *received.msg_ids.last().unwrap())
+            .await
+            .unwrap();
+        assert!(msg.hidden);
+        msg
+    }
+
     /// Receive a message using the `receive_imf()` pipeline. This is similar
     /// to `recv_msg()`, but doesn't assume that the message is shown in the chat.
     pub async fn recv_msg_opt(
