Add info messages about implicit membership changes if group member list is recreated (#6314)

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,
             );
         }
 

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", "[email protected]").await?;
     add_contact_to_chat(&bob, bob_chat_id, bob_blue).await?;
-
+    bob.pop_sent_msg().await;
+    let bob_orange = Contact::create(&bob, "orange", "[email protected]").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(())
 }
 

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-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 [email protected] added. [NOTICED][INFO]
+Msg#12: info (Contact#Contact#Info): Member fiona ([email protected]) removed. [NOTICED][INFO]
+Msg#13: bob (Contact#Contact#11): Member [email protected] added by bob ([email protected]). [FRESH][INFO]
+Msg#14: Me (Contact#Contact#Self): You added member fiona ([email protected]). [INFO] o
+--------------------------------------------------------------------------------

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 ([email protected]) 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 ([email protected]) added. [NOTICED][INFO]
+Msg#15:  (Contact#Contact#10): 6th message [FRESH]
+--------------------------------------------------------------------------------