Skip to content

Commit

Permalink
feat: Add info messages about implicit membership changes if group me…
Browse files Browse the repository at this point in the history
…mber list is recreated (#6314)
  • Loading branch information
iequidoo committed Dec 9, 2024
1 parent cd76f4b commit c2cbc3f
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 39 deletions.
80 changes: 49 additions & 31 deletions src/receive_imf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand All @@ -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;
Expand Down
22 changes: 15 additions & 7 deletions src/receive_imf/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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?;
Expand All @@ -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", "[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(())
}

Expand Down
9 changes: 8 additions & 1 deletion src/stock_str.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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
--------------------------------------------------------------------------------

0 comments on commit c2cbc3f

Please sign in to comment.