Repository: deltachat/deltachat-core-rust (language: python)

Issue:
Implicit member removal happens without any notification
For member addition we have "member added" info messages visible even on implicit addition without mentioning who added a member.

But recently I had a case when member was implicitly removed from the chat, writing a day ago without "~" (member not part of the group indication) and then next day was not in the member list, without any notice. This was caused by someone else who did not previously receive messages for some time because of broken mail server setup.

Long story short, I think there should be messages about implicit member removal in addition to messages about implicit member additions e.g. if the whole memberlist is recreated.


Discussion / hints:

Fix patch:
diff --git a/src/receive_imf.rs b/src/receive_imf.rs
index f837dcdd9d..0d2d6ee97d 100644
--- a/src/receive_imf.rs
+++ b/src/receive_imf.rs
@@ -2216,9 +2216,7 @@ async fn apply_group_changes(
             if let Some(contact_id) =
                 Contact::lookup_id_by_addr(context, added_addr, Origin::Unknown).await?
             {
-                if !recreate_member_list {
-                    added_id = Some(contact_id);
-                }
+                added_id = Some(contact_id);
                 is_new_member = !chat_contacts.contains(&contact_id);
             } else {
                 warn!(context, "Added {added_addr:?} has no contact id.");
@@ -2286,12 +2284,16 @@ async fn apply_group_changes(
             new_members.insert(from_id);
         }
 
+        // These are for adding info messages about implicit membership changes, so they are only
+        // filled when such messages are needed.
+        let mut added_ids = HashSet::<ContactId>::new();
+        let mut removed_ids = HashSet::<ContactId>::new();
+
         if !recreate_member_list {
-            let mut diff = HashSet::<ContactId>::new();
             if sync_member_list {
-                diff = new_members.difference(&chat_contacts).copied().collect();
+                added_ids = new_members.difference(&chat_contacts).copied().collect();
             } else if let Some(added_id) = added_id {
-                diff.insert(added_id);
+                added_ids.insert(added_id);
             }
             new_members.clone_from(&chat_contacts);
             // Don't delete any members locally, but instead add absent ones to provide group
@@ -2305,33 +2307,62 @@ async fn apply_group_changes(
             // will likely recreate the member list from the next received message. The problem
             // occurs only if that "somebody" managed to reply earlier. Really, it's a problem for
             // big groups with high message rate, but let it be for now.
-            new_members.extend(diff.clone());
-            if let Some(added_id) = added_id {
-                diff.remove(&added_id);
-            }
-            if !diff.is_empty() {
-                warn!(context, "Implicit addition of {diff:?} to chat {chat_id}.");
-            }
-            group_changes_msgs.reserve(diff.len());
-            for contact_id in diff {
-                let contact = Contact::get_by_id(context, contact_id).await?;
+            new_members.extend(added_ids.clone());
+        }
+        if let Some(removed_id) = removed_id {
+            new_members.remove(&removed_id);
+        }
+        if recreate_member_list {
+            if self_added {
+                // ... then `better_msg` is already set.
+            } else if chat.blocked == Blocked::Request || !chat_contacts.contains(&ContactId::SELF)
+            {
+                warn!(context, "Implicit addition of SELF to chat {chat_id}.");
                 group_changes_msgs.push(
                     stock_str::msg_add_member_local(
                         context,
-                        contact.get_addr(),
+                        &context.get_primary_self_addr().await?,
                         ContactId::UNDEFINED,
                     )
                     .await,
                 );
+            } else {
+                added_ids = new_members.difference(&chat_contacts).copied().collect();
+                removed_ids = chat_contacts.difference(&new_members).copied().collect();
             }
         }
+
+        if let Some(added_id) = added_id {
+            added_ids.remove(&added_id);
+        }
         if let Some(removed_id) = removed_id {
-            new_members.remove(&removed_id);
+            removed_ids.remove(&removed_id);
         }
-        if recreate_member_list {
-            info!(
+        if !added_ids.is_empty() {
+            warn!(
+                context,
+                "Implicit addition of {added_ids:?} to chat {chat_id}."
+            );
+        }
+        if !removed_ids.is_empty() {
+            warn!(
                 context,
-                "Recreating chat {chat_id} member list with {new_members:?}.",
+                "Implicit removal of {removed_ids:?} from chat {chat_id}."
+            );
+        }
+        group_changes_msgs.reserve(added_ids.len() + removed_ids.len());
+        for contact_id in added_ids {
+            let contact = Contact::get_by_id(context, contact_id).await?;
+            group_changes_msgs.push(
+                stock_str::msg_add_member_local(context, contact.get_addr(), ContactId::UNDEFINED)
+                    .await,
+            );
+        }
+        for contact_id in removed_ids {
+            let contact = Contact::get_by_id(context, contact_id).await?;
+            group_changes_msgs.push(
+                stock_str::msg_del_member_local(context, contact.get_addr(), ContactId::UNDEFINED)
+                    .await,
             );
         }
 
diff --git a/src/stock_str.rs b/src/stock_str.rs
index d5f0c43730..54f27a9a6f 100644
--- a/src/stock_str.rs
+++ b/src/stock_str.rs
@@ -430,6 +430,9 @@ pub enum StockMessage {
     #[strum(props(fallback = "%1$s reacted %2$s to \"%3$s\""))]
     MsgReactedBy = 177,
 
+    #[strum(props(fallback = "Member %1$s removed."))]
+    MsgDelMember = 178,
+
     #[strum(props(fallback = "Establishing guaranteed end-to-end encryption, please wait…"))]
     SecurejoinWait = 190,
 
@@ -710,7 +713,11 @@ pub(crate) async fn msg_del_member_local(
             .unwrap_or_else(|_| addr.to_string()),
         _ => addr.to_string(),
     };
-    if by_contact == ContactId::SELF {
+    if by_contact == ContactId::UNDEFINED {
+        translated(context, StockMessage::MsgDelMember)
+            .await
+            .replace1(whom)
+    } else if by_contact == ContactId::SELF {
         translated(context, StockMessage::MsgYouDelMember)
             .await
             .replace1(whom)


Test patch:
diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs
index c87d3c1831..44d846d39f 100644
--- a/src/receive_imf/tests.rs
+++ b/src/receive_imf/tests.rs
@@ -4145,7 +4145,7 @@ async fn test_dont_recreate_contacts_on_add_remove() -> Result<()> {
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-async fn test_recreate_contact_list_on_missing_message() -> Result<()> {
+async fn test_recreate_contact_list_on_missing_messages() -> Result<()> {
     let alice = TestContext::new_alice().await;
     let bob = TestContext::new_bob().await;
     let chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "Group").await?;
@@ -4170,25 +4170,33 @@ async fn test_recreate_contact_list_on_missing_message() -> Result<()> {
     remove_contact_from_chat(&bob, bob_chat_id, bob_contact_fiona).await?;
     let remove_msg = bob.pop_sent_msg().await;
 
-    // bob adds a new member
+    // bob adds new members
     let bob_blue = Contact::create(&bob, "blue", "blue@example.net").await?;
     add_contact_to_chat(&bob, bob_chat_id, bob_blue).await?;
-
+    bob.pop_sent_msg().await;
+    let bob_orange = Contact::create(&bob, "orange", "orange@example.net").await?;
+    add_contact_to_chat(&bob, bob_chat_id, bob_orange).await?;
     let add_msg = bob.pop_sent_msg().await;
 
-    // alice only receives the addition of the member
+    // alice only receives the second member addition
     alice.recv_msg(&add_msg).await;
 
-    // since we missed a message, a new contact list should be build
-    assert_eq!(get_chat_contacts(&alice, chat_id).await?.len(), 3);
+    // since we missed messages, a new contact list should be build
+    assert_eq!(get_chat_contacts(&alice, chat_id).await?.len(), 4);
 
     // re-add fiona
     add_contact_to_chat(&alice, chat_id, alice_fiona).await?;
 
     // delayed removal of fiona shouldn't remove her
     alice.recv_msg_trash(&remove_msg).await;
-    assert_eq!(get_chat_contacts(&alice, chat_id).await?.len(), 4);
+    assert_eq!(get_chat_contacts(&alice, chat_id).await?.len(), 5);
 
+    alice
+        .golden_test_chat(
+            chat_id,
+            "receive_imf_recreate_contact_list_on_missing_messages",
+        )
+        .await;
     Ok(())
 }
 
@@ -4455,6 +4463,20 @@ async fn test_recreate_member_list_on_missing_add_of_self() -> Result<()> {
     send_text_msg(&alice, alice_chat_id, "4th message".to_string()).await?;
     bob.recv_msg(&alice.pop_sent_msg().await).await;
     assert!(!is_contact_in_chat(&bob, bob_chat_id, ContactId::SELF).await?);
+
+    // But if Bob left a long time ago, they must recreate the member list after missing a message.
+    SystemTime::shift(Duration::from_secs(3600));
+    send_text_msg(&alice, alice_chat_id, "5th message".to_string()).await?;
+    alice.pop_sent_msg().await;
+    send_text_msg(&alice, alice_chat_id, "6th message".to_string()).await?;
+    bob.recv_msg(&alice.pop_sent_msg().await).await;
+    assert!(is_contact_in_chat(&bob, bob_chat_id, ContactId::SELF).await?);
+
+    bob.golden_test_chat(
+        bob_chat_id,
+        "receive_imf_recreate_member_list_on_missing_add_of_self",
+    )
+    .await;
     Ok(())
 }
 
diff --git a/test-data/golden/receive_imf_recreate_contact_list_on_missing_messages b/test-data/golden/receive_imf_recreate_contact_list_on_missing_messages
new file mode 100644
index 0000000000..4dfcb87a66
--- /dev/null
+++ b/test-data/golden/receive_imf_recreate_contact_list_on_missing_messages
@@ -0,0 +1,8 @@
+Group#Chat#10: Group [5 member(s)] 
+--------------------------------------------------------------------------------
+Msg#10: Me (Contact#Contact#Self): populate  √
+Msg#11: info (Contact#Contact#Info): Member blue@example.net added. [NOTICED][INFO]
+Msg#12: info (Contact#Contact#Info): Member fiona (fiona@example.net) removed. [NOTICED][INFO]
+Msg#13: bob (Contact#Contact#11): Member orange@example.net added by bob (bob@example.net). [FRESH][INFO]
+Msg#14: Me (Contact#Contact#Self): You added member fiona (fiona@example.net). [INFO] o
+--------------------------------------------------------------------------------
diff --git a/test-data/golden/receive_imf_recreate_member_list_on_missing_add_of_self b/test-data/golden/receive_imf_recreate_member_list_on_missing_add_of_self
new file mode 100644
index 0000000000..d1995d0a82
--- /dev/null
+++ b/test-data/golden/receive_imf_recreate_member_list_on_missing_add_of_self
@@ -0,0 +1,9 @@
+Group#Chat#10: Group [2 member(s)] 
+--------------------------------------------------------------------------------
+Msg#10: info (Contact#Contact#Info): Member Me (bob@example.net) added. [NOTICED][INFO]
+Msg#11:  (Contact#Contact#10): second message [FRESH]
+Msg#12🔒: Me (Contact#Contact#Self): You left the group. [INFO] √
+Msg#13:  (Contact#Contact#10): 4th message [FRESH]
+Msg#14: info (Contact#Contact#Info): Member Me (bob@example.net) added. [NOTICED][INFO]
+Msg#15:  (Contact#Contact#10): 6th message [FRESH]
+--------------------------------------------------------------------------------