From 97f9e9539624a929994e1d7a4d59274551592bf2 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Fri, 6 Dec 2024 00:31:34 -0300 Subject: [PATCH] feat: Don't add "Failed to send message to ..." info messages to group chats A NDN may arrive days after the message is sent when it's already impossible to tell which message wasn't delivered looking at the "Failed to send" info message, so it only clutters the chat and makes the user think they tried to send some message recently which isn't true. Moreover, the info message duplicates the info already displayed in the error message behind the exclamation mark and info messages do not point to the message that is failed to be sent. Moreover it works rarely because `mimeparser.rs` only parses recipients from `x-failed-recipients`, so it likely only works for Gmail. Postfix does not add this `X-Failed-Recipients` header. Let's remove this parsing too. Thanks to @link2xt for pointing this out. --- deltachat-ffi/deltachat.h | 2 +- src/mimeparser.rs | 78 +++++---------------------------------- src/receive_imf/tests.rs | 12 +----- src/stock_str.rs | 8 +--- 4 files changed, 13 insertions(+), 87 deletions(-) diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index c062fef84e..19bbdeb89e 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -6911,7 +6911,7 @@ void dc_event_unref(dc_event_t* event); /// "Failed to send message to %1$s." /// -/// Used in status messages. +/// Unused. Was used in group chat status messages. /// - %1$s will be replaced by the name of the contact the message cannot be sent to #define DC_STR_FAILED_SENDING_TO 74 diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 6fcc9603c7..52219a8f0a 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -17,10 +17,10 @@ use rand::distributions::{Alphanumeric, DistString}; use crate::aheader::{Aheader, EncryptPreference}; use crate::authres::handle_authres; use crate::blob::BlobObject; -use crate::chat::{add_info_msg, ChatId}; +use crate::chat::ChatId; use crate::config::Config; -use crate::constants::{self, Chattype}; -use crate::contact::{Contact, ContactId, Origin}; +use crate::constants; +use crate::contact::ContactId; use crate::context::Context; use crate::decrypt::{ get_autocrypt_peerstate, get_encrypted_mime, keyring_from_peerstate, try_decrypt, @@ -36,8 +36,7 @@ use crate::peerstate::Peerstate; use crate::simplify::{simplify, SimplifiedText}; use crate::sync::SyncItems; use crate::tools::{ - create_smeared_timestamp, get_filemeta, parse_receive_headers, smeared_time, truncate_msg_text, - validate_id, + get_filemeta, parse_receive_headers, smeared_time, truncate_msg_text, validate_id, }; use crate::{chatlist_events, location, stock_str, tools}; @@ -1665,18 +1664,8 @@ impl MimeMessage { .get_header_value(HeaderDef::MessageId) .and_then(|v| parse_message_id(&v).ok()) { - let mut to_list = - get_all_addresses_from_header(&report.headers, "x-failed-recipients"); - let to = if to_list.len() != 1 { - // We do not know which recipient failed - None - } else { - to_list.pop() - }; - return Ok(Some(DeliveryReport { rfc724_mid: original_message_id, - failed_recipient: to.map(|s| s.addr), failure, })); } @@ -1774,7 +1763,6 @@ impl MimeMessage { { self.delivery_report = Some(DeliveryReport { rfc724_mid: original_message_id, - failed_recipient: None, failure: true, }) } @@ -1912,7 +1900,6 @@ pub(crate) struct Report { #[derive(Debug)] pub(crate) struct DeliveryReport { pub rfc724_mid: String, - pub failed_recipient: Option, pub failure: bool, } @@ -2278,20 +2265,12 @@ async fn handle_ndn( let msgs: Vec<_> = context .sql .query_map( - concat!( - "SELECT", - " m.id AS msg_id,", - " c.id AS chat_id,", - " c.type AS type", - " FROM msgs m LEFT JOIN chats c ON m.chat_id=c.id", - " WHERE rfc724_mid=? AND from_id=1", - ), + "SELECT id FROM msgs + WHERE rfc724_mid=? AND from_id=1", (&failed.rfc724_mid,), |row| { - let msg_id: MsgId = row.get("msg_id")?; - let chat_id: ChatId = row.get("chat_id")?; - let chat_type: Chattype = row.get("type")?; - Ok((msg_id, chat_id, chat_type)) + let msg_id: MsgId = row.get(0)?; + Ok(msg_id) }, |rows| Ok(rows.collect::>()), ) @@ -2299,16 +2278,13 @@ async fn handle_ndn( let error = if let Some(error) = error { error - } else if let Some(failed_recipient) = &failed.failed_recipient { - format!("Delivery to {failed_recipient} failed.").clone() } else { "Delivery to at least one recipient failed.".to_string() }; let err_msg = &error; - let mut first = true; for msg in msgs { - let (msg_id, chat_id, chat_type) = msg?; + let msg_id = msg?; let mut message = Message::load_from_db(context, msg_id).await?; let aggregated_error = message .error @@ -2320,47 +2296,11 @@ async fn handle_ndn( aggregated_error.as_ref().unwrap_or(err_msg), ) .await?; - if first { - // Add only one info msg for all failed messages - ndn_maybe_add_info_msg(context, failed, chat_id, chat_type).await?; - } - first = false; } Ok(()) } -async fn ndn_maybe_add_info_msg( - context: &Context, - failed: &DeliveryReport, - chat_id: ChatId, - chat_type: Chattype, -) -> Result<()> { - match chat_type { - Chattype::Group | Chattype::Broadcast => { - if let Some(failed_recipient) = &failed.failed_recipient { - let contact_id = - Contact::lookup_id_by_addr(context, failed_recipient, Origin::Unknown) - .await? - .context("contact ID not found")?; - let contact = Contact::get_by_id(context, contact_id).await?; - // Tell the user which of the recipients failed if we know that (because in - // a group, this might otherwise be unclear) - let text = stock_str::failed_sending_to(context, contact.get_display_name()).await; - add_info_msg(context, chat_id, &text, create_smeared_timestamp(context)).await?; - context.emit_event(EventType::ChatModified(chat_id)); - } - } - Chattype::Mailinglist => { - // ndn_maybe_add_info_msg() is about the case when delivery to the group failed. - // If we get an NDN for the mailing list, just issue a warning. - warn!(context, "ignoring NDN for mailing list."); - } - Chattype::Single => {} - } - Ok(()) -} - #[cfg(test)] mod tests { use mailparse::ParsedMail; diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index c87d3c1831..07e9e72593 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -868,18 +868,10 @@ async fn test_parse_ndn_group_msg() -> Result<()> { assert_eq!(msg.state, MessageState::OutFailed); let msgs = chat::get_chat_msgs(&t, msg.chat_id).await?; - let msg_id = if let ChatItem::Message { msg_id } = msgs.last().unwrap() { - msg_id - } else { + let ChatItem::Message { msg_id } = *msgs.last().unwrap() else { panic!("Wrong item type"); }; - let last_msg = Message::load_from_db(&t, *msg_id).await?; - - assert_eq!( - last_msg.text, - stock_str::failed_sending_to(&t, "assidhfaaspocwaeofi@gmail.com").await - ); - assert_eq!(last_msg.from_id, ContactId::INFO); + assert_eq!(msg_id, msg.id); Ok(()) } diff --git a/src/stock_str.rs b/src/stock_str.rs index d5f0c43730..7aacee2794 100644 --- a/src/stock_str.rs +++ b/src/stock_str.rs @@ -149,6 +149,7 @@ pub enum StockMessage { #[strum(props(fallback = "Message from %1$s"))] SubjectForNewContact = 73, + /// Unused. Was used in group chat status messages. #[strum(props(fallback = "Failed to send message to %1$s."))] FailedSendingTo = 74, @@ -980,13 +981,6 @@ pub(crate) async fn subject_for_new_contact(context: &Context, self_name: &str) .replace1(self_name) } -/// Stock string: `Failed to send message to %1$s.`. -pub(crate) async fn failed_sending_to(context: &Context, name: &str) -> String { - translated(context, StockMessage::FailedSendingTo) - .await - .replace1(name) -} - /// Stock string: `Message deletion timer is disabled.`. pub(crate) async fn msg_ephemeral_timer_disabled( context: &Context,