From de2b5a1e8bff663441d173d55ad56ff2d212c665 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Fri, 15 Nov 2024 19:23:49 -0300 Subject: [PATCH] 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 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, 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. --- deltachat-rpc-client/tests/test_something.py | 37 +++++++++++++ src/chat.rs | 56 +++++++++++++------- src/reaction.rs | 27 +++++++--- src/receive_imf.rs | 23 ++++---- src/test_utils.rs | 13 +++++ 5 files changed, 117 insertions(+), 39 deletions(-) diff --git a/deltachat-rpc-client/tests/test_something.py b/deltachat-rpc-client/tests/test_something.py index a53e8376ad..2fc97cf3f3 100644 --- a/deltachat-rpc-client/tests/test_something.py +++ b/deltachat-rpc-client/tests/test_something.py @@ -286,6 +286,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 index f68c292db4..1c2d6ad554 100644 --- a/src/chat.rs +++ b/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::, _>>().map_err(Into::into) + |ids| ids.collect::, _>>().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, _>>()?; + 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<=?;", ( diff --git a/src/reaction.rs b/src/reaction.rs index 6bb71f2a49..c9477b6a93 100644 --- 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}; @@ -641,7 +641,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?; @@ -657,6 +658,20 @@ Here's my footer -- bob@example.net" .await?; expect_incoming_reactions_event(&alice, alice_msg.sender_msg_id, *bob_id, "👍").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 @@ -695,7 +710,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; assert!(has_incoming_reactions_event(&alice).await); let chatlist = Chatlist::try_load(&bob, 0, None, None).await?; @@ -855,7 +870,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"); @@ -907,7 +922,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. @@ -929,7 +944,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 index 9b722c702e..9858a56e3a 100644 --- a/src/receive_imf.rs +++ b/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)?; diff --git a/src/test_utils.rs b/src/test_utils.rs index 85d232ded1..cd11ebc5be 100644 --- 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(