From c2cbc3fe33288dc6196598b781e5e31b89533441 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Sat, 7 Dec 2024 20:59:23 -0300 Subject: [PATCH] feat: Add info messages about implicit membership changes if group member list is recreated (#6314) --- src/receive_imf.rs | 80 ++++++++++++------- src/receive_imf/tests.rs | 22 +++-- src/stock_str.rs | 9 ++- ..._recreate_contact_list_on_missing_messages | 8 ++ 4 files changed, 80 insertions(+), 39 deletions(-) create mode 100644 test-data/golden/receive_imf_recreate_contact_list_on_missing_messages diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 8d9339189e..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::::new(); + let mut removed_ids = HashSet::::new(); + if !recreate_member_list { - let mut diff = HashSet::::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,36 +2307,15 @@ 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?; - group_changes_msgs.push( - stock_str::msg_add_member_local( - context, - contact.get_addr(), - ContactId::UNDEFINED, - ) - .await, - ); - } + new_members.extend(added_ids.clone()); } if let Some(removed_id) = removed_id { new_members.remove(&removed_id); } if recreate_member_list { - info!( - context, - "Recreating chat {chat_id} member list with {new_members:?}.", - ); - if !self_added - && (chat.blocked == Blocked::Request || !chat_contacts.contains(&ContactId::SELF)) + 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( @@ -2345,9 +2326,46 @@ async fn apply_group_changes( ) .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 { + removed_ids.remove(&removed_id); + } + if !added_ids.is_empty() { + warn!( + context, + "Implicit addition of {added_ids:?} to chat {chat_id}." + ); + } + if !removed_ids.is_empty() { + warn!( + context, + "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, + ); + } + if new_members != chat_contacts { chat::update_chat_contacts_table(context, chat_id, &new_members).await?; chat_contacts = new_members; diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index 4c1a3f84a7..f798407f89 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -4137,7 +4137,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?; @@ -4162,25 +4162,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(()) } diff --git a/src/stock_str.rs b/src/stock_str.rs index 7aacee2794..9d2ab1a65f 100644 --- a/src/stock_str.rs +++ b/src/stock_str.rs @@ -431,6 +431,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, @@ -711,7 +714,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) 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 +--------------------------------------------------------------------------------