From 8573649bf737fb22380074efb4acf302a2dac4ee Mon Sep 17 00:00:00 2001 From: Hocuri Date: Tue, 17 Oct 2023 10:40:47 +0200 Subject: [PATCH] feat: Make broadcast lists create their own chat (#4644) feat: Make broadcast lists create their own chat - UIs need to ask for the name when creating broadcast lists now (see https://github.com/deltachat/deltachat-android/pull/2653) That's quite a minimal approach: Add a List-ID header to outgoing broadcast lists, so that the receiving Delta Chat shows them as a separate chat, as talked about with @r10s and @hpk42. Done: - [x] Fix an existing bug that the chat name isn't updated when the broadcast/mailing list name changes (I already started this locally) To be done in other PRs: - [ ] Right now the receiving side shows "Mailing list" in the subtitle of such a chat, it would be nicer if it showed "Broadcast list" (or alternatively, rename "Broadcast list" to "Mailing list", too) - [ ] The UIs should probably ask for a name before creating the broadcast list, since it will actually be sent over the wire. (Android PR: https://github.com/deltachat/deltachat-android/pull/2653) Fixes https://github.com/deltachat/deltachat-core-rust/issues/4597 BREAKING CHANGE: This means that UIs need to ask for the name when creating a broadcast list, similar to https://github.com/deltachat/deltachat-android/pull/2653. --- deltachat-ffi/deltachat.h | 22 +--- deltachat-jsonrpc/src/api/mod.rs | 22 +--- src/chat.rs | 46 +++++--- src/mimefactory.rs | 29 +++-- src/mimeparser.rs | 39 ++----- src/receive_imf.rs | 185 ++++++++++++++++--------------- 6 files changed, 170 insertions(+), 173 deletions(-) diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index 5cd96d8b4..d021f1c5b 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -1718,24 +1718,12 @@ uint32_t dc_create_group_chat (dc_context_t* context, int protect * Create a new broadcast list. * * Broadcast lists are similar to groups on the sending device, - * however, recipients get the messages in normal one-to-one chats - * and will not be aware of other members. + * however, recipients get the messages in a read-only chat + * and will see who the other members are. * - * Replies to broadcasts go only to the sender - * and not to all broadcast recipients. - * Moreover, replies will not appear in the broadcast list - * but in the one-to-one chat with the person answering. - * - * The name and the image of the broadcast list is set automatically - * and is visible to the sender only. - * Not asking for these data allows more focused creation - * and we bypass the question who will get which data. - * Also, many users will have at most one broadcast list - * so, a generic name and image is sufficient at the first place. - * - * Later on, however, the name can be changed using dc_set_chat_name(). - * The image cannot be changed to have a unique, recognizable icon in the chat lists. - * All in all, this is also what other messengers are doing here. + * For historical reasons, this function does not take a name directly, + * instead you have to set the name using dc_set_chat_name() + * after creating the broadcast list. * * @memberof dc_context_t * @param context The context object. diff --git a/deltachat-jsonrpc/src/api/mod.rs b/deltachat-jsonrpc/src/api/mod.rs index bbbf1fdf4..bcdc642f0 100644 --- a/deltachat-jsonrpc/src/api/mod.rs +++ b/deltachat-jsonrpc/src/api/mod.rs @@ -812,24 +812,12 @@ impl CommandApi { /// Create a new broadcast list. /// /// Broadcast lists are similar to groups on the sending device, - /// however, recipients get the messages in normal one-to-one chats - /// and will not be aware of other members. + /// however, recipients get the messages in a read-only chat + /// and will see who the other members are. /// - /// Replies to broadcasts go only to the sender - /// and not to all broadcast recipients. - /// Moreover, replies will not appear in the broadcast list - /// but in the one-to-one chat with the person answering. - /// - /// The name and the image of the broadcast list is set automatically - /// and is visible to the sender only. - /// Not asking for these data allows more focused creation - /// and we bypass the question who will get which data. - /// Also, many users will have at most one broadcast list - /// so, a generic name and image is sufficient at the first place. - /// - /// Later on, however, the name can be changed using dc_set_chat_name(). - /// The image cannot be changed to have a unique, recognizable icon in the chat lists. - /// All in all, this is also what other messengers are doing here. + /// For historical reasons, this function does not take a name directly, + /// instead you have to set the name using dc_set_chat_name() + /// after creating the broadcast list. async fn create_broadcast_list(&self, account_id: u32) -> Result { let ctx = self.get_context(account_id).await?; chat::create_broadcast_list(&ctx) diff --git a/src/chat.rs b/src/chat.rs index 70025f40f..5f2580cbf 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -6094,22 +6094,40 @@ mod tests { get_chat_contacts(&alice, chat_bob.id).await?.pop().unwrap(), ) .await?; - let chat = Chat::load_from_db(&alice, broadcast_id).await?; - assert_eq!(chat.typ, Chattype::Broadcast); - assert_eq!(chat.name, stock_str::broadcast_list(&alice).await); - assert!(!chat.is_self_talk()); + set_chat_name(&alice, broadcast_id, "Broadcast list").await?; + { + let chat = Chat::load_from_db(&alice, broadcast_id).await?; + assert_eq!(chat.typ, Chattype::Broadcast); + assert_eq!(chat.name, "Broadcast list"); + assert!(!chat.is_self_talk()); - send_text_msg(&alice, broadcast_id, "ola!".to_string()).await?; - let msg = alice.get_last_msg().await; - assert_eq!(msg.chat_id, chat.id); + send_text_msg(&alice, broadcast_id, "ola!".to_string()).await?; + let msg = alice.get_last_msg().await; + assert_eq!(msg.chat_id, chat.id); + } - let msg = bob.recv_msg(&alice.pop_sent_msg().await).await; - assert_eq!(msg.get_text(), "ola!"); - assert!(!msg.get_showpadlock()); // avoid leaking recipients in encryption data - let chat = Chat::load_from_db(&bob, msg.chat_id).await?; - assert_eq!(chat.typ, Chattype::Single); - assert_eq!(chat.id, chat_bob.id); - assert!(!chat.is_self_talk()); + { + let msg = bob.recv_msg(&alice.pop_sent_msg().await).await; + assert_eq!(msg.get_text(), "ola!"); + assert_eq!(msg.subject, "Broadcast list"); + assert!(!msg.get_showpadlock()); // avoid leaking recipients in encryption data + let chat = Chat::load_from_db(&bob, msg.chat_id).await?; + assert_eq!(chat.typ, Chattype::Mailinglist); + assert_ne!(chat.id, chat_bob.id); + assert_eq!(chat.name, "Broadcast list"); + assert!(!chat.is_self_talk()); + } + + { + // Alice changes the name: + set_chat_name(&alice, broadcast_id, "My great broadcast").await?; + let sent = alice.send_text(broadcast_id, "I changed the title!").await; + + let msg = bob.recv_msg(&sent).await; + assert_eq!(msg.subject, "Re: My great broadcast"); + let bob_chat = Chat::load_from_db(&bob, msg.chat_id).await?; + assert_eq!(bob_chat.name, "My great broadcast"); + } Ok(()) } diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 2b37977c1..b14e6a99b 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -415,7 +415,9 @@ impl<'a> MimeFactory<'a> { return Ok(self.msg.subject.clone()); } - if chat.typ == Chattype::Group && quoted_msg_subject.is_none_or_empty() { + if (chat.typ == Chattype::Group || chat.typ == Chattype::Broadcast) + && quoted_msg_subject.is_none_or_empty() + { let re = if self.in_reply_to.is_empty() { "" } else { @@ -424,15 +426,13 @@ impl<'a> MimeFactory<'a> { return Ok(format!("{}{}", re, chat.name)); } - if chat.typ != Chattype::Broadcast { - let parent_subject = if quoted_msg_subject.is_none_or_empty() { - chat.param.get(Param::LastSubject) - } else { - quoted_msg_subject.as_deref() - }; - if let Some(last_subject) = parent_subject { - return Ok(format!("Re: {}", remove_subject_prefix(last_subject))); - } + let parent_subject = if quoted_msg_subject.is_none_or_empty() { + chat.param.get(Param::LastSubject) + } else { + quoted_msg_subject.as_deref() + }; + if let Some(last_subject) = parent_subject { + return Ok(format!("Re: {}", remove_subject_prefix(last_subject))); } let self_name = &match context.get_config(Config::Displayname).await? { @@ -594,6 +594,15 @@ impl<'a> MimeFactory<'a> { )); } + if let Loaded::Message { chat } = &self.loaded { + if chat.typ == Chattype::Broadcast { + headers.protected.push(Header::new( + "List-ID".into(), + format!("{} <{}>", chat.name, chat.grpid), + )); + } + } + // Non-standard headers. headers .unprotected diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 5855beea7..1bd0fb9a0 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -123,23 +123,6 @@ pub(crate) enum AvatarAction { Change(String), } -#[derive(Debug, PartialEq)] -pub(crate) enum MailinglistType { - /// The message belongs to a mailing list and has a `ListId:`-header - /// that should be used to get a unique id. - ListIdBased, - - /// The message belongs to a mailing list, but there is no `ListId:`-header; - /// `Sender:`-header should be used to get a unique id. - /// This method is used by implementations as Majordomo. - /// Note, that the `Sender:` header alone is not sufficient to detect these lists, - /// `get_mailinglist_type()` check additional conditions therefore. - SenderBased, - - /// The message does not belong to a mailing list. - None, -} - /// System message type. #[derive( Debug, Default, Display, Clone, Copy, PartialEq, Eq, FromPrimitive, ToPrimitive, ToSql, FromSql, @@ -1340,26 +1323,28 @@ impl MimeMessage { self.parts.push(part); } - pub(crate) fn get_mailinglist_type(&self) -> MailinglistType { - if self.get_header(HeaderDef::ListId).is_some() { - return MailinglistType::ListIdBased; - } else if self.get_header(HeaderDef::Sender).is_some() { + pub(crate) fn get_mailinglist_header(&self) -> Option<&str> { + if let Some(list_id) = self.get_header(HeaderDef::ListId) { + // The message belongs to a mailing list and has a `ListId:`-header + // that should be used to get a unique id. + return Some(list_id); + } else if let Some(sender) = self.get_header(HeaderDef::Sender) { // the `Sender:`-header alone is no indicator for mailing list // as also used for bot-impersonation via `set_override_sender_name()` if let Some(precedence) = self.get_header(HeaderDef::Precedence) { if precedence == "list" || precedence == "bulk" { - return MailinglistType::SenderBased; + // The message belongs to a mailing list, but there is no `ListId:`-header; + // `Sender:`-header is be used to get a unique id. + // This method is used by implementations as Majordomo. + return Some(sender); } } } - MailinglistType::None + None } pub(crate) fn is_mailinglist_message(&self) -> bool { - match self.get_mailinglist_type() { - MailinglistType::ListIdBased | MailinglistType::SenderBased => true, - MailinglistType::None => false, - } + self.get_mailinglist_header().is_some() } pub fn repl_msg_by_error(&mut self, error_msg: &str) { diff --git a/src/receive_imf.rs b/src/receive_imf.rs index bbba10564..58a094c91 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -28,9 +28,7 @@ use crate::log::LogExt; use crate::message::{ self, rfc724_mid_exists, Message, MessageState, MessengerMessage, MsgId, Viewtype, }; -use crate::mimeparser::{ - parse_message_ids, AvatarAction, MailinglistType, MimeMessage, SystemMessage, -}; +use crate::mimeparser::{parse_message_ids, AvatarAction, MimeMessage, SystemMessage}; use crate::param::{Param, Params}; use crate::peerstate::{Peerstate, PeerstateKeyType, PeerstateVerifiedStatus}; use crate::reaction::{set_msg_reaction, Reaction}; @@ -654,45 +652,23 @@ async fn add_parts( if chat_id.is_none() { // check if the message belongs to a mailing list - match mime_parser.get_mailinglist_type() { - MailinglistType::ListIdBased => { - if let Some(list_id) = mime_parser.get_header(HeaderDef::ListId) { - if let Some((new_chat_id, new_chat_id_blocked)) = - create_or_lookup_mailinglist( - context, - allow_creation, - list_id, - mime_parser, - ) - .await? - { - chat_id = Some(new_chat_id); - chat_id_blocked = new_chat_id_blocked; - } - } + if let Some(mailinglist_header) = mime_parser.get_mailinglist_header() { + if let Some((new_chat_id, new_chat_id_blocked)) = create_or_lookup_mailinglist( + context, + allow_creation, + mailinglist_header, + mime_parser, + ) + .await? + { + chat_id = Some(new_chat_id); + chat_id_blocked = new_chat_id_blocked; } - MailinglistType::SenderBased => { - if let Some(sender) = mime_parser.get_header(HeaderDef::Sender) { - if let Some((new_chat_id, new_chat_id_blocked)) = - create_or_lookup_mailinglist( - context, - allow_creation, - sender, - mime_parser, - ) - .await? - { - chat_id = Some(new_chat_id); - chat_id_blocked = new_chat_id_blocked; - } - } - } - MailinglistType::None => {} } } if let Some(chat_id) = chat_id { - apply_mailinglist_changes(context, mime_parser, chat_id).await?; + apply_mailinglist_changes(context, mime_parser, sent_timestamp, chat_id).await?; } // if contact renaming is prevented (for mailinglists and bots), @@ -1921,6 +1897,8 @@ async fn apply_group_changes( Ok(better_msg) } +static LIST_ID_REGEX: Lazy = Lazy::new(|| Regex::new(r"^(.+)<(.+)>$").unwrap()); + /// Create or lookup a mailing list chat. /// /// `list_id_header` contains the Id that must be used for the mailing list @@ -1937,23 +1915,71 @@ async fn create_or_lookup_mailinglist( list_id_header: &str, mime_parser: &MimeMessage, ) -> Result> { - static LIST_ID: Lazy = Lazy::new(|| Regex::new(r"^(.+)<(.+)>$").unwrap()); - let (mut name, listid) = match LIST_ID.captures(list_id_header) { - Some(cap) => (cap[1].trim().to_string(), cap[2].trim().to_string()), - None => ( - "".to_string(), - list_id_header - .trim() - .trim_start_matches('<') - .trim_end_matches('>') - .to_string(), - ), + let listid = match LIST_ID_REGEX.captures(list_id_header) { + Some(cap) => cap[2].trim().to_string(), + None => list_id_header + .trim() + .trim_start_matches('<') + .trim_end_matches('>') + .to_string(), }; if let Some((chat_id, _, blocked)) = chat::get_chat_id_by_grpid(context, &listid).await? { return Ok(Some((chat_id, blocked))); } + let name = compute_mailinglist_name(list_id_header, &listid, mime_parser); + + if allow_creation { + // list does not exist but should be created + let param = mime_parser.list_post.as_ref().map(|list_post| { + let mut p = Params::new(); + p.set(Param::ListPost, list_post); + p.to_string() + }); + + let is_bot = context.get_config_bool(Config::Bot).await?; + let blocked = if is_bot { + Blocked::Not + } else { + Blocked::Request + }; + let chat_id = ChatId::create_multiuser_record( + context, + Chattype::Mailinglist, + &listid, + &name, + blocked, + ProtectionStatus::Unprotected, + param, + ) + .await + .with_context(|| { + format!( + "failed to create mailinglist '{}' for grpid={}", + &name, &listid + ) + })?; + + chat::add_to_chat_contacts_table(context, chat_id, &[ContactId::SELF]).await?; + Ok(Some((chat_id, blocked))) + } else { + info!(context, "Creating list forbidden by caller."); + Ok(None) + } +} + +#[allow(clippy::indexing_slicing)] +fn compute_mailinglist_name( + list_id_header: &str, + listid: &str, + mime_parser: &MimeMessage, +) -> String { + let mut name = match LIST_ID_REGEX.captures(list_id_header) { + Some(cap) => cap[1].trim().to_string(), + None => "".to_string(), + }; + // for mailchimp lists, the name in `ListId` is just a long number. // a usable name for these lists is in the `From` header // and we can detect these lists by a unique `ListId`-suffix. @@ -1997,50 +2023,14 @@ async fn create_or_lookup_mailinglist( // 51231231231231231231231232869f58.xing.com -> xing.com static PREFIX_32_CHARS_HEX: Lazy = Lazy::new(|| Regex::new(r"([0-9a-fA-F]{32})\.(.{6,})").unwrap()); - if let Some(cap) = PREFIX_32_CHARS_HEX.captures(&listid) { + if let Some(cap) = PREFIX_32_CHARS_HEX.captures(listid) { name = cap[2].to_string(); } else { - name = listid.clone(); + name = listid.to_string(); } } - if allow_creation { - // list does not exist but should be created - let param = mime_parser.list_post.as_ref().map(|list_post| { - let mut p = Params::new(); - p.set(Param::ListPost, list_post); - p.to_string() - }); - - let is_bot = context.get_config_bool(Config::Bot).await?; - let blocked = if is_bot { - Blocked::Not - } else { - Blocked::Request - }; - let chat_id = ChatId::create_multiuser_record( - context, - Chattype::Mailinglist, - &listid, - &name, - blocked, - ProtectionStatus::Unprotected, - param, - ) - .await - .with_context(|| { - format!( - "failed to create mailinglist '{}' for grpid={}", - &name, &listid - ) - })?; - - chat::add_to_chat_contacts_table(context, chat_id, &[ContactId::SELF]).await?; - Ok(Some((chat_id, blocked))) - } else { - info!(context, "Creating list forbidden by caller."); - Ok(None) - } + strip_rtlo_characters(&name) } /// Set ListId param on the contact and ListPost param the chat. @@ -2049,9 +2039,10 @@ async fn create_or_lookup_mailinglist( async fn apply_mailinglist_changes( context: &Context, mime_parser: &MimeMessage, + sent_timestamp: i64, chat_id: ChatId, ) -> Result<()> { - let Some(list_post) = &mime_parser.list_post else { + let Some(mailinglist_header) = mime_parser.get_mailinglist_header() else { return Ok(()); }; @@ -2061,6 +2052,24 @@ async fn apply_mailinglist_changes( } let listid = &chat.grpid; + let new_name = compute_mailinglist_name(mailinglist_header, listid, mime_parser); + if chat.name != new_name + && chat_id + .update_timestamp(context, Param::GroupNameTimestamp, sent_timestamp) + .await? + { + info!(context, "Updating listname for chat {chat_id}."); + context + .sql + .execute("UPDATE chats SET name=? WHERE id=?;", (new_name, chat_id)) + .await?; + context.emit_event(EventType::ChatModified(chat_id)); + } + + let Some(list_post) = &mime_parser.list_post else { + return Ok(()); + }; + let list_post = match ContactAddress::new(list_post) { Ok(list_post) => list_post, Err(err) => {