fix: markseen_msgs: Limit not yet downloaded messages state to InNoticed (#2970)

src/message.rs

@@ -1693,6 +1693,7 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
                     m.id AS id,
                     m.chat_id AS chat_id,
                     m.state AS state,
+                    m.download_state as download_state,
                     m.ephemeral_timer AS ephemeral_timer,
                     m.param AS param,
                     m.from_id AS from_id,
@@ -1708,6 +1709,7 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
                 let id: MsgId = row.get("id")?;
                 let chat_id: ChatId = row.get("chat_id")?;
                 let state: MessageState = row.get("state")?;
+                let download_state: DownloadState = row.get("download_state")?;
                 let param: Params = row.get::<_, String>("param")?.parse().unwrap_or_default();
                 let from_id: ContactId = row.get("from_id")?;
                 let rfc724_mid: String = row.get("rfc724_mid")?;
@@ -1719,6 +1721,7 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
                         id,
                         chat_id,
                         state,
+                        download_state,
                         param,
                         from_id,
                         rfc724_mid,
@@ -1748,6 +1751,7 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
             id,
             curr_chat_id,
             curr_state,
+            curr_download_state,
             curr_param,
             curr_from_id,
             curr_rfc724_mid,
@@ -1757,7 +1761,14 @@ 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 {
+        if curr_download_state != DownloadState::Done {
+            if curr_state == MessageState::InFresh {
+                // Don't mark partially downloaded messages as seen or send a read receipt since
+                // they are not really seen by the user.
+                update_msg_state(context, id, MessageState::InNoticed).await?;
+                updated_chat_ids.insert(curr_chat_id);
+            }
+        } else if curr_state == MessageState::InFresh || curr_state == MessageState::InNoticed {
             update_msg_state(context, id, MessageState::InSeen).await?;
             info!(context, "Seen message {}.", id);
 
@@ -2583,6 +2594,110 @@ mod tests {
         Ok(())
     }
 
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn test_markseen_not_downloaded_msg() -> Result<()> {
+        let mut tcm = TestContextManager::new();
+        let alice = &tcm.alice().await;
+        alice.set_config(Config::DownloadLimit, Some("1")).await?;
+        let bob = &tcm.bob().await;
+        let bob_chat_id = tcm.send_recv_accept(alice, bob, "hi").await.chat_id;
+
+        let file_bytes = include_bytes!("../test-data/image/screenshot.png");
+        let mut msg = Message::new(Viewtype::Image);
+        msg.set_file_from_bytes(bob, "a.jpg", file_bytes, None)
+            .await?;
+        let sent_msg = bob.send_msg(bob_chat_id, &mut msg).await;
+        let msg = alice.recv_msg(&sent_msg).await;
+        assert_eq!(msg.download_state, DownloadState::Available);
+        assert!(!msg.param.get_bool(Param::WantsMdn).unwrap_or_default());
+        assert_eq!(msg.state, MessageState::InFresh);
+        markseen_msgs(alice, vec![msg.id]).await?;
+        // A not downloaded message can be seen only if it's seen on another device.
+        assert_eq!(msg.id.get_state(alice).await?, MessageState::InNoticed);
+        // Marking the message as seen again is a no op.
+        markseen_msgs(alice, vec![msg.id]).await?;
+        assert_eq!(msg.id.get_state(alice).await?, MessageState::InNoticed);
+
+        msg.id
+            .update_download_state(alice, DownloadState::InProgress)
+            .await?;
+        markseen_msgs(alice, vec![msg.id]).await?;
+        assert_eq!(msg.id.get_state(alice).await?, MessageState::InNoticed);
+        msg.id
+            .update_download_state(alice, DownloadState::Failure)
+            .await?;
+        markseen_msgs(alice, vec![msg.id]).await?;
+        assert_eq!(msg.id.get_state(alice).await?, MessageState::InNoticed);
+        msg.id
+            .update_download_state(alice, DownloadState::Undecipherable)
+            .await?;
+        markseen_msgs(alice, vec![msg.id]).await?;
+        assert_eq!(msg.id.get_state(alice).await?, MessageState::InNoticed);
+
+        assert!(
+            !alice
+                .sql
+                .exists("SELECT COUNT(*) FROM smtp_mdns", ())
+                .await?
+        );
+
+        alice.set_config(Config::DownloadLimit, None).await?;
+        // Let's assume that Alice and Bob resolved the problem with encryption.
+        let old_msg = msg;
+        let msg = alice.recv_msg(&sent_msg).await;
+        assert_eq!(msg.chat_id, old_msg.chat_id);
+        assert_eq!(msg.download_state, DownloadState::Done);
+        assert!(msg.param.get_bool(Param::WantsMdn).unwrap_or_default());
+        assert!(msg.get_showpadlock());
+        // The message state mustn't be downgraded to `InFresh`.
+        assert_eq!(msg.state, MessageState::InNoticed);
+        markseen_msgs(alice, vec![msg.id]).await?;
+        let msg = Message::load_from_db(alice, msg.id).await?;
+        assert_eq!(msg.state, MessageState::InSeen);
+        assert_eq!(
+            alice
+                .sql
+                .count("SELECT COUNT(*) FROM smtp_mdns", ())
+                .await?,
+            1
+        );
+        Ok(())
+    }
+
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn test_msg_seen_on_imap_when_downloaded() -> Result<()> {
+        let mut tcm = TestContextManager::new();
+        let alice = &tcm.alice().await;
+        alice.set_config(Config::DownloadLimit, Some("1")).await?;
+        let bob = &tcm.bob().await;
+        let bob_chat_id = tcm.send_recv_accept(alice, bob, "hi").await.chat_id;
+
+        let file_bytes = include_bytes!("../test-data/image/screenshot.png");
+        let mut msg = Message::new(Viewtype::Image);
+        msg.set_file_from_bytes(bob, "a.jpg", file_bytes, None)
+            .await?;
+        let sent_msg = bob.send_msg(bob_chat_id, &mut msg).await;
+        let msg = alice.recv_msg(&sent_msg).await;
+        assert_eq!(msg.download_state, DownloadState::Available);
+        assert_eq!(msg.state, MessageState::InFresh);
+
+        alice.set_config(Config::DownloadLimit, None).await?;
+        let seen = true;
+        let rcvd_msg = receive_imf(alice, sent_msg.payload().as_bytes(), seen)
+            .await
+            .unwrap()
+            .unwrap();
+        assert_eq!(rcvd_msg.chat_id, msg.chat_id);
+        let msg = Message::load_from_db(alice, *rcvd_msg.msg_ids.last().unwrap())
+            .await
+            .unwrap();
+        assert_eq!(msg.download_state, DownloadState::Done);
+        assert!(msg.param.get_bool(Param::WantsMdn).unwrap_or_default());
+        assert!(msg.get_showpadlock());
+        assert_eq!(msg.state, MessageState::InSeen);
+        Ok(())
+    }
+
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn test_get_state() -> Result<()> {
         let alice = TestContext::new_alice().await;

src/receive_imf.rs

@@ -1575,7 +1575,7 @@ INSERT INTO msgs
 ON CONFLICT (id) DO UPDATE
 SET rfc724_mid=excluded.rfc724_mid, chat_id=excluded.chat_id,
     from_id=excluded.from_id, to_id=excluded.to_id, timestamp_sent=excluded.timestamp_sent,
-    type=excluded.type, msgrmsg=excluded.msgrmsg,
+    type=excluded.type, state=max(state,excluded.state), msgrmsg=excluded.msgrmsg,
     txt=excluded.txt, txt_normalized=excluded.txt_normalized, subject=excluded.subject,
     txt_raw=excluded.txt_raw, param=excluded.param,
     hidden=excluded.hidden,bytes=excluded.bytes, mime_headers=excluded.mime_headers,