fix: make test_broadcast work, return an error when trying to add manually add a contact to a broadcast list, don't have unpromoted broadcast lists, make basic multi-device work

Author: Hocuri

src/chat.rs

@@ -3731,8 +3731,8 @@ pub(crate) async fn create_broadcast_ex(
             }
             t.execute(
                 "INSERT INTO chats \
-                (type, name, grpid, param, created_timestamp) \
-                VALUES(?, ?, ?, \'U=1\', ?);",
+                (type, name, grpid, created_timestamp) \
+                VALUES(?, ?, ?, ?);",
                 (
                     Chattype::OutBroadcast,
                     &chat_name,
@@ -3908,8 +3908,8 @@ pub(crate) async fn add_contact_to_chat_ex(
     // this also makes sure, no contacts are added to special or normal chats
     let mut chat = Chat::load_from_db(context, chat_id).await?;
     ensure!(
-        chat.typ == Chattype::Group || chat.typ == Chattype::OutBroadcast,
-        "{} is not a group/broadcast where one can add members",
+        chat.typ == Chattype::Group,
+        "{} is not a group where one can add members",
         chat_id
     );
     ensure!(
@@ -3918,10 +3918,6 @@ pub(crate) async fn add_contact_to_chat_ex(
         contact_id
     );
     ensure!(!chat.is_mailing_list(), "Mailing lists can't be changed");
-    ensure!(
-        chat.typ != Chattype::OutBroadcast || contact_id != ContactId::SELF,
-        "Cannot add SELF to broadcast channel."
-    );
     ensure!(
         chat.is_encrypted(context).await? == contact.is_key_contact(),
         "Only key-contacts can be added to encrypted chats"
@@ -3973,7 +3969,7 @@ pub(crate) async fn add_contact_to_chat_ex(
         }
         add_to_chat_contacts_table(context, time(), chat_id, &[contact_id]).await?;
     }
-    if chat.typ == Chattype::Group && chat.is_promoted() {
+    if chat.is_promoted() {
         msg.viewtype = Viewtype::Text;
 
         let contact_addr = contact.get_addr().to_lowercase();

src/chat/chat_tests.rs

@@ -2607,44 +2607,67 @@ async fn test_can_send_group() -> Result<()> {
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-async fn test_broadcast() -> Result<()> {
+async fn test_broadcast_change_name() -> Result<()> {
     // create two context, send two messages so both know the other
-    let alice = TestContext::new_alice().await;
-    let bob = TestContext::new_bob().await;
-    let fiona = TestContext::new_fiona().await;
+    let mut tcm = TestContextManager::new();
+    let alice = &tcm.alice().await;
+    let bob = &tcm.bob().await;
+    let fiona = &tcm.fiona().await;
 
+    tcm.section("Alice sends a message to Bob");
     let chat_alice = alice.create_chat(&bob).await;
     send_text_msg(&alice, chat_alice.id, "hi!".to_string()).await?;
     bob.recv_msg(&alice.pop_sent_msg().await).await;
 
+    tcm.section("Bob sends a message to Alice");
     let chat_bob = bob.create_chat(&alice).await;
     send_text_msg(&bob, chat_bob.id, "ho!".to_string()).await?;
     let msg = alice.recv_msg(&bob.pop_sent_msg().await).await;
     assert!(msg.get_showpadlock());
 
-    // test broadcast channel
     let broadcast_id = create_broadcast(&alice, "Channel".to_string()).await?;
-    add_contact_to_chat(
-        &alice,
-        broadcast_id,
-        get_chat_contacts(&alice, chat_bob.id).await?.pop().unwrap(),
-    )
-    .await?;
-    let fiona_contact_id = alice.add_or_lookup_contact_id(&fiona).await;
-    add_contact_to_chat(&alice, broadcast_id, fiona_contact_id).await?;
-    set_chat_name(&alice, broadcast_id, "Broadcast channel").await?;
+    let qr = get_securejoin_qr(alice, Some(broadcast_id)).await.unwrap();
+
+    tcm.section("Alice invites Bob to her channel");
+    tcm.exec_securejoin_qr(bob, alice, &qr).await;
+    tcm.section("Alice invites Fiona to her channel");
+    tcm.exec_securejoin_qr(fiona, alice, &qr).await;
+
     {
+        tcm.section("Alice changes the chat name");
+        set_chat_name(&alice, broadcast_id, "My great broadcast").await?;
+        let sent = alice.pop_sent_msg().await;
+
+        tcm.section("Bob receives the name-change system message");
+        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");
+
+        tcm.section("Fiona receives the name-change system message");
+        let msg = fiona.recv_msg(&sent).await;
+        assert_eq!(msg.subject, "Re: My great broadcast");
+        let fiona_chat = Chat::load_from_db(fiona, msg.chat_id).await?;
+        assert_eq!(fiona_chat.name, "My great broadcast");
+    }
+
+    {
+        tcm.section("Alice changes the chat name again, but the system message is lost somehow");
+        set_chat_name(&alice, broadcast_id, "Broadcast channel").await?;
+
         let chat = Chat::load_from_db(&alice, broadcast_id).await?;
         assert_eq!(chat.typ, Chattype::OutBroadcast);
         assert_eq!(chat.name, "Broadcast channel");
         assert!(!chat.is_self_talk());
 
+        tcm.section("Alice sends a text message 'ola!'");
         send_text_msg(&alice, broadcast_id, "ola!".to_string()).await?;
         let msg = alice.get_last_msg().await;
         assert_eq!(msg.chat_id, chat.id);
     }
 
     {
+        tcm.section("Bob receives the 'ola!' message");
         let sent_msg = alice.pop_sent_msg().await;
         let msg = bob.parse_msg(&sent_msg).await;
         assert!(msg.was_encrypted());
@@ -2657,25 +2680,23 @@ async fn test_broadcast() -> Result<()> {
 
         let msg = bob.recv_msg(&sent_msg).await;
         assert_eq!(msg.get_text(), "ola!");
-        assert_eq!(msg.subject, "Broadcast channel");
+        assert_eq!(msg.subject, "Re: Broadcast channel");
         assert!(msg.get_showpadlock());
         assert!(msg.get_override_sender_name().is_none());
         let chat = Chat::load_from_db(&bob, msg.chat_id).await?;
         assert_eq!(chat.typ, Chattype::InBroadcast);
         assert_ne!(chat.id, chat_bob.id);
         assert_eq!(chat.name, "Broadcast channel");
         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");
+        tcm.section("Fiona receives the 'ola!' message");
+        let msg = fiona.recv_msg(&sent_msg).await;
+        assert_eq!(msg.get_text(), "ola!");
+        assert!(msg.get_showpadlock());
+        assert!(msg.get_override_sender_name().is_none());
+        let chat = Chat::load_from_db(fiona, msg.chat_id).await?;
+        assert_eq!(chat.typ, Chattype::InBroadcast);
+        assert_eq!(chat.name, "Broadcast channel");
     }
 
     Ok(())

src/securejoin.rs

@@ -5,8 +5,8 @@ use deltachat_contact_tools::ContactAddress;
 use percent_encoding::{NON_ALPHANUMERIC, utf8_percent_encode};
 
 use crate::chat::{
-    self, Chat, ChatId, ChatIdBlocked, ProtectionStatus, get_chat_id_by_grpid,
-    load_broadcast_shared_secret,
+    self, Chat, ChatId, ChatIdBlocked, ProtectionStatus, add_to_chat_contacts_table,
+    get_chat_id_by_grpid, load_broadcast_shared_secret,
 };
 use crate::chatlist_events;
 use crate::config::Config;
@@ -26,6 +26,7 @@ use crate::qr::check_qr;
 use crate::securejoin::bob::JoinerProgress;
 use crate::sync::Sync::*;
 use crate::token;
+use crate::tools::time;
 
 mod bob;
 mod qrinvite;
@@ -434,13 +435,26 @@ pub(crate) async fn handle_securejoin_handshake(
                     mime_message.timestamp_sent,
                 )
                 .await?;
-                chat::add_contact_to_chat_ex(context, Nosync, group_chat_id, contact_id, true)
-                    .await?;
+                if step.starts_with("vb-") {
+                    // TODO extract into variable
+                    add_to_chat_contacts_table(context, time(), group_chat_id, &[contact_id])
+                        .await?;
+                } else {
+                    chat::add_contact_to_chat_ex(context, Nosync, group_chat_id, contact_id, true)
+                        .await?;
+                }
                 inviter_progress(context, contact_id, 800);
                 inviter_progress(context, contact_id, 1000);
-                // IMAP-delete the message to avoid handling it by another device and adding the
-                // member twice. Another device will know the member's key from Autocrypt-Gossip.
-                Ok(HandshakeMessage::Done)
+                if step.starts_with("vb-") {
+                    // For broadcasts, we don't want to delete the message,
+                    // because the other device should also internally add the member
+                    // and see the key (because it won't see the member via autocrypt-gossip).
+                    Ok(HandshakeMessage::Propagate)
+                } else {
+                    // IMAP-delete the message to avoid handling it by another device and adding the
+                    // member twice. Another device will know the member's key from Autocrypt-Gossip.
+                    Ok(HandshakeMessage::Done)
+                }
             } else {
                 // Setup verified contact.
                 secure_connection_established(