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

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 all incoming hidden messages in the chat as seen in `marknoticed_chat()`.

It's interesting that sent out reactions are already hidden messages, so this change mostly just
unifies things.

Author: iequidoo

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"

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)

src/chat.rs

@@ -3290,10 +3290,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() {
@@ -3304,7 +3304,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),
@@ -3314,20 +3314,39 @@ 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| {
+            let nr_msgs_noticed = conn.execute(
+                "UPDATE msgs
+                    SET state=?
+                    WHERE state=?
+                    AND hidden=0
+                    AND chat_id=?",
+                (MessageState::InNoticed, MessageState::InFresh, chat_id),
+            )?;
+            let mut stmt = conn.prepare(
+                "SELECT id FROM msgs
+                    WHERE state>=? AND state<?
+                    AND hidden=1
+                    AND chat_id=?
+                    ORDER BY id DESC LIMIT 100",
+            )?;
+            let hidden_msgs = stmt
+                .query_map(
+                    (MessageState::InFresh, MessageState::InSeen, chat_id),
+                    |row| {
+                        let id: MsgId = row.get(0)?;
+                        Ok(id)
+                    },
+                )?
+                .collect::<std::result::Result<Vec<_>, _>>()?;
+            Ok((nr_msgs_noticed, hidden_msgs))
+        };
+        let (nr_msgs_noticed, hidden_msgs) = context.sql.call_write(conn_fn).await?;
+        if nr_msgs_noticed == 0 && hidden_msgs.is_empty() {
+            return Ok(());
+        }
+        message::markseen_msgs(context, hidden_msgs).await?;
     }
 
     context.emit_event(EventType::MsgsNoticed(chat_id));
@@ -3372,7 +3391,6 @@ pub(crate) async fn mark_old_messages_as_noticed(
                     "UPDATE msgs
             SET state=?
           WHERE state=?
-            AND hidden=0
             AND chat_id=?
             AND timestamp<=?;",
                     (

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)

src/receive_imf.rs

@@ -760,7 +760,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;
 
@@ -1018,12 +1018,7 @@ async fn add_parts(
             }
         }
 
-        state = if seen
-            || fetching_existing_messages
-            || is_mdn
-            || is_reaction
-            || chat_id_blocked == Blocked::Yes
-        {
+        state = if seen || fetching_existing_messages || is_mdn || chat_id_blocked == Blocked::Yes {
             MessageState::InSeen
         } else {
             MessageState::InFresh
@@ -1232,7 +1227,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(|| {
@@ -1407,7 +1402,7 @@ async fn add_parts(
     // that the ui should show button to display the full message.
 
     // a flag used to avoid adding "show full message" button to multiple parts of the message.
-    let mut save_mime_modified = mime_parser.is_mime_modified;
+    let mut save_mime_modified = mime_parser.is_mime_modified && !hidden;
 
     let mime_headers = if save_mime_headers || save_mime_modified {
         let headers = if !mime_parser.decoded_data.is_empty() {
@@ -1599,10 +1594,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 {
@@ -1618,7 +1613,7 @@ RETURNING id
                     mime_in_reply_to,
                     mime_references,
                     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() {
@@ -1628,7 +1623,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)?;

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(