From 130ed24a5a9bba27cb00306e5f28339e2ac212df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20DOUIN?= Date: Fri, 4 Mar 2022 21:50:09 +0100 Subject: [PATCH] fix missing or invalid cc when replying to a message (#324) I also added tests for the `Msg::into_reply` method and made the `Msg::merge_with` stricter. --- CHANGELOG.md | 7 ++ src/msg/msg_entity.rs | 215 +++++++++++++++++++++++++++++++++++++----- 2 files changed, 196 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 904b816..b087809 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `In-Reply-To` not set properly when replying to a message [#323] +- `Cc` missing or invalid when replying to a message [#324] + ## [0.5.8] - 2022-03-04 ### Added @@ -480,3 +485,5 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#309]: https://github.com/soywod/himalaya/issues/309 [#318]: https://github.com/soywod/himalaya/issues/318 [#321]: https://github.com/soywod/himalaya/issues/321 +[#323]: https://github.com/soywod/himalaya/issues/323 +[#324]: https://github.com/soywod/himalaya/issues/324 diff --git a/src/msg/msg_entity.rs b/src/msg/msg_entity.rs index 3d53b0b..dca6d51 100644 --- a/src/msg/msg_entity.rs +++ b/src/msg/msg_entity.rs @@ -13,7 +13,7 @@ use crate::{ config::{AccountConfig, DEFAULT_DRAFT_FOLDER, DEFAULT_SENT_FOLDER, DEFAULT_SIG_DELIM}, msg::{ from_addrs_to_sendable_addrs, from_addrs_to_sendable_mbox, from_slice_to_addrs, msg_utils, - Addrs, BinaryPart, Part, Parts, TextPlainPart, TplOverride, + Addr, Addrs, BinaryPart, Part, Parts, TextPlainPart, TplOverride, }, output::PrinterService, smtp::SmtpService, @@ -176,10 +176,13 @@ impl Msg { .as_deref() .or_else(|| self.from.as_deref()) .map(|addrs| { - addrs - .clone() - .into_iter() - .filter(|addr| addr != &account_addr) + addrs.iter().cloned().filter(|addr| match addr { + Addr::Group(_) => false, + Addr::Single(a) => match &account_addr { + Addr::Group(_) => false, + Addr::Single(b) => a.addr != b.addr, + }, + }) }); if all { self.to = addrs.map(|addrs| addrs.collect::>().into()); @@ -189,11 +192,28 @@ impl Msg { .map(|addr| vec![addr].into()); } - // Cc & Bcc - if !all { - self.cc = None; - self.bcc = None; - } + // Cc + self.cc = if all { + self.cc.as_deref().map(|addrs| { + addrs + .iter() + .cloned() + .filter(|addr| match addr { + Addr::Group(_) => false, + Addr::Single(a) => match &account_addr { + Addr::Group(_) => false, + Addr::Single(b) => a.addr != b.addr, + }, + }) + .collect::>() + .into() + }) + } else { + None + }; + + // Bcc + self.bcc = None; // Body let plain_content = { @@ -413,24 +433,19 @@ impl Msg { } pub fn merge_with(&mut self, msg: Msg) { - if msg.from.is_some() { - self.from = msg.from; + self.from = msg.from; + self.reply_to = msg.reply_to; + self.to = msg.to; + self.cc = msg.cc; + self.bcc = msg.bcc; + self.subject = msg.subject; + + if msg.message_id.is_some() { + self.message_id = msg.message_id; } - if msg.to.is_some() { - self.to = msg.to; - } - - if msg.cc.is_some() { - self.cc = msg.cc; - } - - if msg.bcc.is_some() { - self.bcc = msg.bcc; - } - - if !msg.subject.is_empty() { - self.subject = msg.subject; + if msg.in_reply_to.is_some() { + self.in_reply_to = msg.in_reply_to; } for part in msg.parts.0.into_iter() { @@ -707,3 +722,151 @@ impl TryInto for Msg { Ok(lettre::address::Envelope::new(from, to).context("cannot create envelope")?) } } + +#[cfg(test)] +mod tests { + use mailparse::SingleInfo; + + use crate::msg::Addr; + + use super::*; + + #[test] + fn test_into_reply() { + let config = AccountConfig { + display_name: "Test".into(), + email: "test-account@local".into(), + ..AccountConfig::default() + }; + + // Checks that: + // - "message_id" moves to "in_reply_to" + // - "subject" starts by "Re: " + // - "to" is replaced by "from" + // - "from" is replaced by the address from the account config + + let msg = Msg { + message_id: Some("msg-id".into()), + subject: "subject".into(), + from: Some( + vec![Addr::Single(SingleInfo { + addr: "test-sender@local".into(), + display_name: None, + })] + .into(), + ), + ..Msg::default() + } + .into_reply(false, &config) + .unwrap(); + + assert_eq!(msg.message_id, None); + assert_eq!(msg.in_reply_to.unwrap(), "msg-id"); + assert_eq!(msg.subject, "Re: subject"); + assert_eq!( + msg.from.unwrap().to_string(), + "\"Test\" " + ); + assert_eq!(msg.to.unwrap().to_string(), "test-sender@local"); + + // Checks that: + // - "subject" does not contains additional "Re: " + // - "to" is replaced by reply_to + // - "to" contains one address when "all" is false + // - "cc" are empty when "all" is false + + let msg = Msg { + subject: "Re: subject".into(), + from: Some( + vec![Addr::Single(SingleInfo { + addr: "test-sender@local".into(), + display_name: None, + })] + .into(), + ), + reply_to: Some( + vec![ + Addr::Single(SingleInfo { + addr: "test-sender-to-reply@local".into(), + display_name: Some("Sender".into()), + }), + Addr::Single(SingleInfo { + addr: "test-sender-to-reply-2@local".into(), + display_name: Some("Sender 2".into()), + }), + ] + .into(), + ), + cc: Some( + vec![Addr::Single(SingleInfo { + addr: "test-cc@local".into(), + display_name: None, + })] + .into(), + ), + ..Msg::default() + } + .into_reply(false, &config) + .unwrap(); + + assert_eq!(msg.subject, "Re: subject"); + assert_eq!( + msg.to.unwrap().to_string(), + "\"Sender\" " + ); + assert_eq!(msg.cc, None); + + // Checks that: + // - "to" contains all addresses except for the sender when "all" is true + // - "cc" contains all addresses except for the sender when "all" is true + + let msg = Msg { + from: Some( + vec![ + Addr::Single(SingleInfo { + addr: "test-sender-1@local".into(), + display_name: Some("Sender 1".into()), + }), + Addr::Single(SingleInfo { + addr: "test-sender-2@local".into(), + display_name: Some("Sender 2".into()), + }), + Addr::Single(SingleInfo { + addr: "test-account@local".into(), + display_name: Some("Test".into()), + }), + ] + .into(), + ), + cc: Some( + vec![ + Addr::Single(SingleInfo { + addr: "test-sender-1@local".into(), + display_name: Some("Sender 1".into()), + }), + Addr::Single(SingleInfo { + addr: "test-sender-2@local".into(), + display_name: Some("Sender 2".into()), + }), + Addr::Single(SingleInfo { + addr: "test-account@local".into(), + display_name: None, + }), + ] + .into(), + ), + ..Msg::default() + } + .into_reply(true, &config) + .unwrap(); + + assert_eq!( + msg.to.unwrap().to_string(), + "\"Sender 1\" , \"Sender 2\" " + ); + assert_eq!( + msg.cc.unwrap().to_string(), + "\"Sender 1\" , \"Sender 2\" " + ); + } +}