fix: markseen_msgs(): Mark reactions to specified messages as seen too (#7884)

This allows to remove notifications for reactions from other devices. NB: UIs should pass all
messages to markseen_msgs(), incl. outgoing ones. markseen_msgs() should be called when a message
comes into view or when a reaction for a message being in view arrives.

Also don't emit `MsgsNoticed` from receive_imf_inner() if the chat still contains fresh hidden
messages, i.e. include reactions into this logic, to avoid removing notifications for reactions
until they are seen on another device.

Author: iequidoo

deltachat-rpc-client/tests/test_something.py

@@ -413,27 +413,32 @@ def test_dont_move_sync_msgs(acfactory, direct_imap):
         time.sleep(1)
 
 
-def test_reaction_seen_on_another_dev(acfactory) -> None:
+@pytest.mark.parametrize("chat_noticed", [False, True])
+def test_reaction_seen_on_another_dev(acfactory, chat_noticed) -> None:
     alice, bob = acfactory.get_online_accounts(2)
     alice2 = alice.clone()
     alice2.start_io()
 
     alice_contact_bob = alice.create_contact(bob, "Bob")
     alice_chat_bob = alice_contact_bob.create_chat()
-    alice_chat_bob.send_text("Hello!")
+    alice_msg = 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()
+    bob_msg = bob.get_message_by_id(msg_id)
+    snapshot = bob_msg.get_snapshot()
     snapshot.chat.accept()
-    message.send_reaction("😎")
+    bob_msg.send_reaction("😎")
     for a in [alice, alice2]:
         a.wait_for_event(EventType.INCOMING_REACTION)
 
     alice2.clear_all_events()
-    alice_chat_bob.mark_noticed()
+    if chat_noticed:
+        alice_chat_bob.mark_noticed()
+    else:
+        # UIs are supposed to mark messages being in view as seen, not reactions themselves.
+        alice_msg.mark_seen()
     chat_id = alice2.wait_for_event(EventType.MSGS_NOTICED).chat_id
     alice2_chat_bob = alice2.create_chat(bob)
     assert chat_id == alice2_chat_bob.id

src/message.rs

@@ -1829,7 +1829,7 @@ pub async fn delete_msgs_ex(
     Ok(())
 }
 
-/// Marks requested messages as seen.
+/// Marks requested messages and reactions to them as seen.
 pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()> {
     if msg_ids.is_empty() {
         return Ok(());
@@ -1843,10 +1843,18 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
 
     let mut msgs = Vec::with_capacity(msg_ids.len());
     for &id in &msg_ids {
-        if let Some(msg) = context
+        let Some(rfc724_mid): Option<String> = context
             .sql
-            .query_row_optional(
+            .query_get_value("SELECT rfc724_mid FROM msgs WHERE id=?", (id,))
+            .await?
+        else {
+            continue;
+        };
+        context
+            .sql
+            .query_map(
                 "SELECT
+                    m.id AS id,
                     m.chat_id AS chat_id,
                     m.state AS state,
                     m.ephemeral_timer AS ephemeral_timer,
@@ -1857,9 +1865,11 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
                     c.archived AS archived,
                     c.blocked AS blocked
                  FROM msgs m LEFT JOIN chats c ON c.id=m.chat_id
-                 WHERE m.id=? AND m.chat_id>9",
-                (id,),
+                WHERE (m.id=? OR m.mime_in_reply_to=? AND m.hidden=1)
+                    AND m.chat_id>9 AND ?<=m.state AND m.state<?",
+                (id, rfc724_mid, MessageState::InFresh, MessageState::InSeen),
                 |row| {
+                    let id: MsgId = row.get("id")?;
                     let chat_id: ChatId = row.get("chat_id")?;
                     let state: MessageState = row.get("state")?;
                     let param: Params = row.get::<_, String>("param")?.parse().unwrap_or_default();
@@ -1884,11 +1894,14 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
                         ephemeral_timer,
                     ))
                 },
+                |rows| {
+                    for row in rows {
+                        msgs.push(row?);
+                    }
+                    Ok(())
+                },
             )
-            .await?
-        {
-            msgs.push(msg);
-        }
+            .await?;
     }
 
     if msgs
@@ -1917,47 +1930,44 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
         _curr_ephemeral_timer,
     ) in msgs
     {
-        if curr_state == MessageState::InFresh || curr_state == MessageState::InNoticed {
-            update_msg_state(context, id, MessageState::InSeen).await?;
-            info!(context, "Seen message {}.", id);
-
-            markseen_on_imap_table(context, &curr_rfc724_mid).await?;
-
-            // Read receipts for system messages are never sent to contacts.
-            // These messages have no place to display received read receipt
-            // anyway. And since their text is locally generated,
-            // quoting them is dangerous as it may contain contact names. E.g., for original message
-            // "Group left by me", a read receipt will quote "Group left by <name>", and the name can
-            // be a display name stored in address book rather than the name sent in the From field by
-            // the user.
-            //
-            // We also don't send read receipts for contact requests.
-            // Read receipts will not be sent even after accepting the chat.
-            let to_id = if curr_blocked == Blocked::Not
-                && curr_param.get_bool(Param::WantsMdn).unwrap_or_default()
-                && curr_param.get_cmd() == SystemMessage::Unknown
-                && context.should_send_mdns().await?
-            {
-                Some(curr_from_id)
-            } else if context.get_config_bool(Config::BccSelf).await? {
-                Some(ContactId::SELF)
-            } else {
-                None
-            };
-            if let Some(to_id) = to_id {
-                context
-                    .sql
-                    .execute(
-                        "INSERT INTO smtp_mdns (msg_id, from_id, rfc724_mid) VALUES(?, ?, ?)",
-                        (id, to_id, curr_rfc724_mid),
-                    )
-                    .await
-                    .context("failed to insert into smtp_mdns")?;
-                context.scheduler.interrupt_smtp().await;
-            }
-            if !curr_hidden {
-                updated_chat_ids.insert(curr_chat_id);
-            }
+        update_msg_state(context, id, MessageState::InSeen).await?;
+        info!(context, "Seen message {}.", id);
+
+        markseen_on_imap_table(context, &curr_rfc724_mid).await?;
+
+        // Read receipts for system messages are never sent to contacts. These messages have no
+        // place to display received read receipt anyway. And since their text is locally generated,
+        // quoting them is dangerous as it may contain contact names. E.g., for original message
+        // "Group left by me", a read receipt will quote "Group left by <name>", and the name can be
+        // a display name stored in address book rather than the name sent in the From field by the
+        // user.
+        //
+        // We also don't send read receipts for contact requests. Read receipts will not be sent
+        // even after accepting the chat.
+        let to_id = if curr_blocked == Blocked::Not
+            && curr_param.get_bool(Param::WantsMdn).unwrap_or_default()
+            && curr_param.get_cmd() == SystemMessage::Unknown
+            && context.should_send_mdns().await?
+        {
+            Some(curr_from_id)
+        } else if context.get_config_bool(Config::BccSelf).await? {
+            Some(ContactId::SELF)
+        } else {
+            None
+        };
+        if let Some(to_id) = to_id {
+            context
+                .sql
+                .execute(
+                    "INSERT INTO smtp_mdns (msg_id, from_id, rfc724_mid) VALUES(?, ?, ?)",
+                    (id, to_id, curr_rfc724_mid),
+                )
+                .await
+                .context("failed to insert into smtp_mdns")?;
+            context.scheduler.interrupt_smtp().await;
+        }
+        if !curr_hidden {
+            updated_chat_ids.insert(curr_chat_id);
         }
         archived_chats_maybe_noticed |= curr_state == MessageState::InFresh
             && !curr_hidden

src/reaction.rs

@@ -393,7 +393,7 @@ mod tests {
     use crate::chatlist::Chatlist;
     use crate::config::Config;
     use crate::contact::{Contact, Origin};
-    use crate::message::{MessageState, Viewtype, delete_msgs};
+    use crate::message::{MessageState, Viewtype, delete_msgs, markseen_msgs};
     use crate::receive_imf::receive_imf;
     use crate::sql::housekeeping;
     use crate::test_utils::E2EE_INFO_MSGS;
@@ -956,4 +956,37 @@ Content-Disposition: reaction\n\
         }
         Ok(())
     }
+
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn test_markseen_referenced_msg() -> Result<()> {
+        let mut tcm = TestContextManager::new();
+        let alice = &tcm.alice().await;
+        let bob = &tcm.bob().await;
+        let chat_id = alice.create_chat(bob).await.id;
+
+        let alice_msg_id = send_text_msg(alice, chat_id, "foo".to_string()).await?;
+        let sent_msg = alice.pop_sent_msg().await;
+        let bob_msg = bob.recv_msg(&sent_msg).await;
+        bob_msg.chat_id.accept(bob).await?;
+
+        send_reaction(bob, bob_msg.id, "👀").await?;
+        let sent_msg = bob.pop_sent_msg().await;
+        let alice_reaction = alice.recv_msg_hidden(&sent_msg).await;
+        assert_eq!(alice_reaction.state, MessageState::InFresh);
+
+        markseen_msgs(alice, vec![alice_msg_id]).await?;
+        let alice_reaction = Message::load_from_db(alice, alice_reaction.id).await?;
+        assert_eq!(alice_reaction.state, MessageState::InSeen);
+        assert_eq!(
+            alice
+                .sql
+                .count(
+                    "SELECT COUNT(*) FROM smtp_mdns WHERE from_id=?",
+                    (ContactId::SELF,)
+                )
+                .await?,
+            1
+        );
+        Ok(())
+    }
 }

src/receive_imf.rs

@@ -34,7 +34,8 @@ use crate::message::{
     self, Message, MessageState, MessengerMessage, MsgId, Viewtype, rfc724_mid_exists,
 };
 use crate::mimeparser::{
-    AvatarAction, GossipedKey, MimeMessage, PreMessageMode, SystemMessage, parse_message_ids,
+    AvatarAction, GossipedKey, MimeMessage, PreMessageMode, SystemMessage, parse_message_id,
+    parse_message_ids,
 };
 use crate::param::{Param, Params};
 use crate::peer_channels::{add_gossip_peer_from_header, insert_topic_stub, iroh_topic_from_str};
@@ -1026,7 +1027,7 @@ UPDATE config SET value=? WHERE keyname='configured_addr' AND value!=?1
                         "
 UPDATE msgs SET state=? WHERE
     state=? AND
-    hidden=0 AND
+    (hidden=0 OR hidden=1) AND
     chat_id=? AND
     timestamp<?",
                         (
@@ -1038,7 +1039,18 @@ UPDATE msgs SET state=? WHERE
                     )
                     .await
                     .context("UPDATE msgs.state")?;
-                if chat_id.get_fresh_msg_cnt(context).await? == 0 {
+                let n_fresh_msgs = context
+                    .sql
+                    .count(
+                        "
+SELECT COUNT(*) FROM msgs WHERE
+    state=? AND
+    (hidden=0 OR hidden=1) AND
+    chat_id=?",
+                        (MessageState::InFresh, chat_id),
+                    )
+                    .await?;
+                if n_fresh_msgs == 0 {
                     // Removes all notifications for the chat in UIs.
                     context.emit_event(EventType::MsgsNoticed(chat_id));
                 } else {
@@ -1999,6 +2011,10 @@ async fn add_parts(
     let mime_in_reply_to = mime_parser
         .get_header(HeaderDef::InReplyTo)
         .unwrap_or_default();
+    let mime_in_reply_to = parse_message_id(&mime_in_reply_to)
+        .log_err(context)
+        .ok()
+        .unwrap_or_default();
     let mime_references = mime_parser
         .get_header(HeaderDef::References)
         .unwrap_or_default();
@@ -2125,7 +2141,7 @@ async fn add_parts(
             let is_incoming_fresh = mime_parser.incoming && !seen;
             set_msg_reaction(
                 context,
-                mime_in_reply_to,
+                &mime_in_reply_to,
                 chat_id,
                 from_id,
                 sort_timestamp,
@@ -2267,7 +2283,7 @@ RETURNING id
                     } else {
                         Vec::new()
                     },
-                    if trash { "" } else { mime_in_reply_to },
+                    if trash { "" } else { &mime_in_reply_to },
                     if trash { "" } else { mime_references },
                     !trash && save_mime_modified,
                     if trash { "" } else { part.error.as_deref().unwrap_or_default() },

src/sql/migrations.rs

@@ -2323,6 +2323,15 @@ ALTER TABLE contacts ADD COLUMN name_normalized TEXT;
         .await?;
     }
 
+    inc_and_check(&mut migration_version, 148)?;
+    if dbversion < migration_version {
+        sql.execute_migration(
+            "CREATE INDEX msgs_index10 ON msgs (mime_in_reply_to)",
+            migration_version,
+        )
+        .await?;
+    }
+
     let new_version = sql
         .get_raw_config_int(VERSION_CFG)
         .await?