fix: apply_group_changes: Don't implicitly delete members locally, add absent ones instead (#4934) #4938
Conversation
…d absent ones instead (#4934) This is another approach to provide group membership consistency for all members. Considerations: - Classical MUA users usually don't intend to remove users from an email thread, so if they removed a recipient then it was probably by accident. - DC users could miss new member additions and then better to handle this in the same way as for classical MUA messages. Moreover, if we remove a member implicitly, they will never know that and continue to think they're still here. But it shouldn't be a big problem if somebody missed a member removal, because they 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.
iequidoo
force-pushed
the
iequidoo/hocuri-group-consistency-algo
branch
from
a6d482b
to
3540009
Compare
| @@ -3502,8 +3512,11 @@ async fn test_dont_readd_with_normal_msg() -> Result<()> { | |||
|
|
|||
| bob.recv_msg(&alice.pop_sent_msg().await).await; | |||
|
|
|||
| // Alice didn't receive Bobs leave message, but bob shouldn't readded himself just because of that. | |||
| assert!(!is_contact_in_chat(&bob, bob_chat_id, ContactId::SELF).await?); | |||
| // Alice didn't receive Bob's leave message, so Bob must readd themselves otherwise other | |||
There was a problem hiding this comment.
I have no better idea currently how to handle this. Bob mustn't just ignore his implicit re-addition, maybe we should implement a leave auto-retry, but as Alice could have re-added Bob on purpose, the logic would become too complicated, and i'd better avoid such auto-retries
There was a problem hiding this comment.
It is probably fine, now Bob can spam the group until either added back for everyone or someone thinks it is wrong and removes him properly.
|
Looks like this algo will never converge... |
As long as the tests grow larger and handle more and more cases it is fine I think. |
| diff.remove(&added_id); | ||
| } | ||
| if !diff.is_empty() { | ||
| warn!(context, "Implicit addition of {diff:?} to chat {chat_id}."); |
There was a problem hiding this comment.
Would me nice to add "Member %s added." translatable system message (stock string) without mentioning who added the member, but better in a follow-up PR as it will break all the tests similarly to how #4916 did.
There was a problem hiding this comment.
Several members can be added here, so either it requires a different translation or we need to refactor this code to allow adding multiple system messages. I'm for the second option
There was a problem hiding this comment.
Adding multiple messages "Member X added." in a row is fine.
|
Merged because I want to make a new core release for 1.42 release candidates. |
This is another approach to provide group membership consistency for all members. Considerations:
But it shouldn't be a big problem if somebody missed a member removal, because they 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.
Fixes #4934