From 6406f305b828fdbdb4da670102c0ca5302f933a3 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Fri, 11 Jul 2025 14:34:05 +0200 Subject: [PATCH] feat: Make it possible to leave broadcast channels (#6984) Part of #6884. The channel owner will not be notified in any way that you left, they will only see that there is one member less. For the translated stock strings, this is what we agreed on in the group: - Add a new "Leave Channel" stock string (will need to be done in UIs) - Reword the existing "Are you sure you want to leave this group?" string to "Are you sure you want to leave?" (the options are "Cancel" and "Leave Group" / "Leave Channel", so it's clear what you are leaving) (will need to be done in the deltachat-android repo, other UIs will pick it up automatically) - Reword the existing "You left the group." string to "You left". (done here, I will adapt the strings in deltachat-android, too) I adapted DC Android by pushing https://github.com/deltachat/deltachat-android/pull/3783/commits/6df2740884e0b781c1debd1783152fd9b2fe6787 to https://github.com/deltachat/deltachat-android/pull/3783. --------- Co-authored-by: l --- deltachat-ffi/deltachat.h | 2 +- src/chat.rs | 52 ++++++--- src/chat/chat_tests.rs | 109 +++++++++++++++++- src/mimefactory.rs | 7 +- src/receive_imf.rs | 44 ++++++- src/stock_str.rs | 4 +- .../golden/chat_test_parallel_member_remove | 2 +- 7 files changed, 191 insertions(+), 29 deletions(-) diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index f0ce41afd..25b86e95e 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -7424,7 +7424,7 @@ void dc_event_unref(dc_event_t* event); /// Used in status messages. #define DC_STR_REMOVE_MEMBER_BY_OTHER 131 -/// "You left the group." +/// "You left." /// /// Used in status messages. #define DC_STR_GROUP_LEFT_BY_YOU 132 diff --git a/src/chat.rs b/src/chat.rs index 506ab1c32..bee6ec022 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -2905,10 +2905,12 @@ async fn prepare_send_msg( // If the chat is a contact request, let the user accept it later. msg.param.get_cmd() == SystemMessage::SecurejoinMessage } - // Allow to send "Member removed" messages so we can leave the group. + // Allow to send "Member removed" messages so we can leave the group/broadcast. // Necessary checks should be made anyway before removing contact // from the chat. - CantSendReason::NotAMember => msg.param.get_cmd() == SystemMessage::MemberRemovedFromGroup, + CantSendReason::NotAMember | CantSendReason::InBroadcast => { + msg.param.get_cmd() == SystemMessage::MemberRemovedFromGroup + } CantSendReason::MissingKey => msg .param .get_bool(Param::ForcePlaintext) @@ -4079,8 +4081,6 @@ pub async fn remove_contact_from_chat( "Cannot remove special contact" ); - let mut msg = Message::new(Viewtype::default()); - let chat = Chat::load_from_db(context, chat_id).await?; if chat.typ == Chattype::Group || chat.typ == Chattype::OutBroadcast { if !chat.is_self_in_chat(context).await? { @@ -4110,19 +4110,10 @@ pub async fn remove_contact_from_chat( // in case of the database becoming inconsistent due to a bug. if let Some(contact) = Contact::get_by_id_optional(context, contact_id).await? { if chat.typ == Chattype::Group && chat.is_promoted() { - msg.viewtype = Viewtype::Text; - if contact_id == ContactId::SELF { - msg.text = stock_str::msg_group_left_local(context, ContactId::SELF).await; - } else { - msg.text = - stock_str::msg_del_member_local(context, contact_id, ContactId::SELF) - .await; - } - msg.param.set_cmd(SystemMessage::MemberRemovedFromGroup); - msg.param.set(Param::Arg, contact.get_addr().to_lowercase()); - msg.param - .set(Param::ContactAddedRemoved, contact.id.to_u32() as i32); - let res = send_msg(context, chat_id, &mut msg).await; + let addr = contact.get_addr(); + + let res = send_member_removal_msg(context, chat_id, contact_id, addr).await; + if contact_id == ContactId::SELF { res?; set_group_explicitly_left(context, &chat.grpid).await?; @@ -4141,6 +4132,11 @@ pub async fn remove_contact_from_chat( chat.sync_contacts(context).await.log_err(context).ok(); } } + } else if chat.typ == Chattype::InBroadcast && contact_id == ContactId::SELF { + // For incoming broadcast channels, it's not possible to remove members, + // but it's possible to leave: + let self_addr = context.get_primary_self_addr().await?; + send_member_removal_msg(context, chat_id, contact_id, &self_addr).await?; } else { bail!("Cannot remove members from non-group chats."); } @@ -4148,6 +4144,28 @@ pub async fn remove_contact_from_chat( Ok(()) } +async fn send_member_removal_msg( + context: &Context, + chat_id: ChatId, + contact_id: ContactId, + addr: &str, +) -> Result { + let mut msg = Message::new(Viewtype::Text); + + if contact_id == ContactId::SELF { + msg.text = stock_str::msg_group_left_local(context, ContactId::SELF).await; + } else { + msg.text = stock_str::msg_del_member_local(context, contact_id, ContactId::SELF).await; + } + + msg.param.set_cmd(SystemMessage::MemberRemovedFromGroup); + msg.param.set(Param::Arg, addr.to_lowercase()); + msg.param + .set(Param::ContactAddedRemoved, contact_id.to_u32()); + + send_msg(context, chat_id, &mut msg).await +} + async fn set_group_explicitly_left(context: &Context, grpid: &str) -> Result<()> { if !is_group_explicitly_left(context, grpid).await? { context diff --git a/src/chat/chat_tests.rs b/src/chat/chat_tests.rs index cca358b39..b693934f7 100644 --- a/src/chat/chat_tests.rs +++ b/src/chat/chat_tests.rs @@ -5,7 +5,7 @@ use crate::ephemeral::Timer; use crate::headerdef::HeaderDef; use crate::imex::{ImexMode, has_backup, imex}; use crate::message::{MessengerMessage, delete_msgs}; -use crate::mimeparser; +use crate::mimeparser::{self, MimeMessage}; use crate::receive_imf::receive_imf; use crate::test_utils::{ AVATAR_64x64_BYTES, AVATAR_64x64_DEDUPLICATED, TestContext, TestContextManager, @@ -374,7 +374,10 @@ async fn test_member_add_remove() -> Result<()> { // Alice leaves the chat. remove_contact_from_chat(&alice, alice_chat_id, ContactId::SELF).await?; let sent = alice.pop_sent_msg().await; - assert_eq!(sent.load_from_db().await.get_text(), "You left the group."); + assert_eq!( + sent.load_from_db().await.get_text(), + stock_str::msg_group_left_local(&alice, ContactId::SELF).await + ); Ok(()) } @@ -2931,6 +2934,108 @@ async fn test_broadcast_channel_protected_listid() -> Result<()> { Ok(()) } +/// Test that if Bob leaves a broadcast channel, +/// Alice (the channel owner) won't see him as a member anymore, +/// but won't be notified about this in any way. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_leave_broadcast() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + + tcm.section("Alice creates broadcast channel with Bob."); + let alice_chat_id = create_broadcast(alice, "foo".to_string()).await?; + let bob_contact = alice.add_or_lookup_contact(bob).await.id; + add_contact_to_chat(alice, alice_chat_id, bob_contact).await?; + + tcm.section("Alice sends first message to broadcast."); + let sent_msg = alice.send_text(alice_chat_id, "Hello!").await; + let bob_msg = bob.recv_msg(&sent_msg).await; + + assert_eq!(get_chat_contacts(alice, alice_chat_id).await?.len(), 1); + + // Clear events so that we can later check + // that the 'Broadcast channel left' message didn't trigger IncomingMsg: + alice.evtracker.clear_events(); + + // Shift the time so that we can later check the "Broadcast channel left" message's timestamp: + SystemTime::shift(Duration::from_secs(60)); + + tcm.section("Bob leaves the broadcast channel."); + let bob_chat_id = bob_msg.chat_id; + bob_chat_id.accept(bob).await?; + remove_contact_from_chat(bob, bob_chat_id, ContactId::SELF).await?; + + let leave_msg = bob.pop_sent_msg().await; + alice.recv_msg_trash(&leave_msg).await; + + assert_eq!(get_chat_contacts(alice, alice_chat_id).await?.len(), 0); + + alice.emit_event(EventType::Test); + alice + .evtracker + .get_matching(|ev| match ev { + EventType::Test => true, + EventType::IncomingMsg { .. } => { + panic!("'Broadcast channel left' message should be silent") + } + EventType::MsgsNoticed(..) => { + panic!("'Broadcast channel left' message shouldn't clear notifications") + } + EventType::MsgsChanged { .. } => { + panic!("Broadcast channels should be left silently, without any message"); + } + _ => false, + }) + .await; + + Ok(()) +} + +/// Tests that if Bob leaves a broadcast channel with one device, +/// the other device shows a correct info message "You left.". +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_leave_broadcast_multidevice() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob0 = &tcm.bob().await; + let bob1 = &tcm.bob().await; + + tcm.section("Alice creates broadcast channel with Bob."); + let alice_chat_id = create_broadcast(alice, "foo".to_string()).await?; + let bob_contact = alice.add_or_lookup_contact(bob0).await.id; + add_contact_to_chat(alice, alice_chat_id, bob_contact).await?; + + tcm.section("Alice sends first message to broadcast."); + let sent_msg = alice.send_text(alice_chat_id, "Hello!").await; + let bob0_hello = bob0.recv_msg(&sent_msg).await; + let bob1_hello = bob1.recv_msg(&sent_msg).await; + + tcm.section("Bob leaves the broadcast channel with his first device."); + let bob_chat_id = bob0_hello.chat_id; + bob_chat_id.accept(bob0).await?; + remove_contact_from_chat(bob0, bob_chat_id, ContactId::SELF).await?; + + let leave_msg = bob0.pop_sent_msg().await; + let parsed = MimeMessage::from_bytes(bob1, leave_msg.payload().as_bytes(), None).await?; + assert_eq!( + parsed.parts[0].msg, + stock_str::msg_group_left_remote(bob0).await + ); + + let rcvd = bob1.recv_msg(&leave_msg).await; + + assert_eq!(rcvd.chat_id, bob1_hello.chat_id); + assert!(rcvd.is_info()); + assert_eq!(rcvd.get_info_type(), SystemMessage::MemberRemovedFromGroup); + assert_eq!( + rcvd.text, + stock_str::msg_group_left_local(bob1, ContactId::SELF).await + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_create_for_contact_with_blocked() -> Result<()> { let t = TestContext::new().await; diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 8baa0ded9..bd126be47 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -789,7 +789,7 @@ impl MimeFactory { } if let Loaded::Message { chat, .. } = &self.loaded { - if chat.typ == Chattype::OutBroadcast { + if chat.typ == Chattype::OutBroadcast || chat.typ == Chattype::InBroadcast { headers.push(( "List-ID", mail_builder::headers::text::Text::new(format!( @@ -1319,7 +1319,10 @@ impl MimeFactory { } } - if chat.typ == Chattype::Group || chat.typ == Chattype::OutBroadcast { + if chat.typ == Chattype::Group + || chat.typ == Chattype::OutBroadcast + || chat.typ == Chattype::InBroadcast + { headers.push(( "Chat-Group-Name", mail_builder::headers::text::Text::new(chat.name.to_string()).into(), diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 131167ad9..82c062483 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -14,7 +14,9 @@ use mailparse::SingleInfo; use num_traits::FromPrimitive; use regex::Regex; -use crate::chat::{self, Chat, ChatId, ChatIdBlocked, ProtectionStatus}; +use crate::chat::{ + self, Chat, ChatId, ChatIdBlocked, ProtectionStatus, remove_from_chat_contacts_table, +}; use crate::config::Config; use crate::constants::{Blocked, Chattype, DC_CHAT_ID_TRASH, EDITED_PREFIX, ShowEmails}; use crate::contact::{Contact, ContactId, Origin, mark_contact_id_as_verified}; @@ -1687,7 +1689,9 @@ async fn add_parts( _ if chat.id.is_special() => GroupChangesInfo::default(), Chattype::Single => GroupChangesInfo::default(), Chattype::Mailinglist => GroupChangesInfo::default(), - Chattype::OutBroadcast => GroupChangesInfo::default(), + Chattype::OutBroadcast => { + apply_out_broadcast_changes(context, mime_parser, &mut chat, from_id).await? + } Chattype::Group => { apply_group_changes( context, @@ -1701,7 +1705,7 @@ async fn add_parts( .await? } Chattype::InBroadcast => { - apply_broadcast_changes(context, mime_parser, &mut chat, from_id).await? + apply_in_broadcast_changes(context, mime_parser, &mut chat, from_id).await? } }; @@ -3438,7 +3442,30 @@ async fn apply_mailinglist_changes( Ok(()) } -async fn apply_broadcast_changes( +async fn apply_out_broadcast_changes( + context: &Context, + mime_parser: &MimeMessage, + chat: &mut Chat, + from_id: ContactId, +) -> Result { + ensure!(chat.typ == Chattype::OutBroadcast); + + if let Some(_removed_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemoved) { + // The sender of the message left the broadcast channel + remove_from_chat_contacts_table(context, chat.id, from_id).await?; + + return Ok(GroupChangesInfo { + better_msg: Some("".to_string()), + added_removed_id: None, + silent: true, + extra_msgs: vec![], + }); + } + + Ok(GroupChangesInfo::default()) +} + +async fn apply_in_broadcast_changes( context: &Context, mime_parser: &MimeMessage, chat: &mut Chat, @@ -3459,6 +3486,15 @@ async fn apply_broadcast_changes( ) .await?; + if let Some(_removed_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemoved) { + // The only member added/removed message that is ever sent is "I left.", + // so, this is the only case we need to handle here + if from_id == ContactId::SELF { + better_msg + .get_or_insert(stock_str::msg_group_left_local(context, ContactId::SELF).await); + } + } + if send_event_chat_modified { context.emit_event(EventType::ChatModified(chat.id)); chatlist_events::emit_chatlist_item_changed(context, chat.id); diff --git a/src/stock_str.rs b/src/stock_str.rs index 26a5d17dd..86fb86b13 100644 --- a/src/stock_str.rs +++ b/src/stock_str.rs @@ -285,7 +285,7 @@ pub enum StockMessage { #[strum(props(fallback = "Member %1$s removed by %2$s."))] MsgDelMemberBy = 131, - #[strum(props(fallback = "You left the group."))] + #[strum(props(fallback = "You left."))] MsgYouLeftGroup = 132, #[strum(props(fallback = "Group left by %1$s."))] @@ -685,7 +685,7 @@ pub(crate) async fn msg_group_left_remote(context: &Context) -> String { translated(context, StockMessage::MsgILeftGroup).await } -/// Stock string: `You left the group.` or `Group left by %1$s.`. +/// Stock string: `You left.` or `Group left by %1$s.`. pub(crate) async fn msg_group_left_local(context: &Context, by_contact: ContactId) -> String { if by_contact == ContactId::SELF { translated(context, StockMessage::MsgYouLeftGroup).await diff --git a/test-data/golden/chat_test_parallel_member_remove b/test-data/golden/chat_test_parallel_member_remove index b548b3e00..449f89a51 100644 --- a/test-data/golden/chat_test_parallel_member_remove +++ b/test-data/golden/chat_test_parallel_member_remove @@ -1,7 +1,7 @@ Group#Chat#10: Group chat [3 member(s)] -------------------------------------------------------------------------------- Msg#10🔒: (Contact#Contact#10): Hi! I created a group. [FRESH] -Msg#11🔒: Me (Contact#Contact#Self): You left the group. [INFO] √ +Msg#11🔒: Me (Contact#Contact#Self): You left. [INFO] √ Msg#12🔒: (Contact#Contact#10): Member charlie@example.net added by alice@example.org. [FRESH][INFO] Msg#13🔒: (Contact#Contact#10): What a silence! [FRESH] --------------------------------------------------------------------------------