fix member_added/member_removed event with tests and and provide a group-tracking example

deltachat-ffi/src/lib.rs

@@ -27,7 +27,7 @@ use num_traits::{FromPrimitive, ToPrimitive};
 
 use deltachat::chat::{ChatId, ChatVisibility, MuteDuration};
 use deltachat::constants::DC_MSG_ID_LAST_SPECIAL;
-use deltachat::contact::Contact;
+use deltachat::contact::{Contact, Origin};
 use deltachat::context::Context;
 use deltachat::key::DcKey;
 use deltachat::message::MsgId;
@@ -1665,7 +1665,9 @@ pub unsafe extern "C" fn dc_lookup_contact_id_by_addr(
     }
     let ffi_context = &*context;
     ffi_context
-        .with_inner(|ctx| Contact::lookup_id_by_addr(ctx, to_string_lossy(addr)))
+        .with_inner(|ctx| {
+            Contact::lookup_id_by_addr(ctx, to_string_lossy(addr), Origin::IncomingReplyTo)
+        })
         .unwrap_or(0)
 }
 

python/examples/group_tracking.py

@@ -0,0 +1,64 @@
+
+# content of group_tracking.py
+
+import sys
+import optparse
+import deltachat
+
+
+class GroupTrackingPlugin:
+    @deltachat.hookspec.account_hookimpl
+    def process_incoming_message(self, message):
+        print("*** process_incoming_message addr={} msg={!r}".format(
+              message.get_sender_contact().addr, message.text))
+        for member in message.chat.get_contacts():
+            print("chat member: {}".format(member.addr))
+
+    @deltachat.hookspec.account_hookimpl
+    def member_added(self, chat, contact):
+        print("*** member_added", contact.addr, "from", chat)
+        for member in chat.get_contacts():
+            print("chat member: {}".format(member.addr))
+
+    @deltachat.hookspec.account_hookimpl
+    def member_removed(self, chat, contact):
+        print("*** member_removed", contact.addr, "from", chat)
+
+
+def main(argv):
+    p = optparse.OptionParser("simple-echo")
+    p.add_option("-l", action="store_true", help="show ffi")
+    p.add_option("--db", type="str", help="database file")
+    p.add_option("--email", type="str", help="email address")
+    p.add_option("--password", type="str", help="password")
+
+    opts, posargs = p.parse_args(argv)
+
+    assert opts.db, "you must specify --db"
+    ac = deltachat.Account(opts.db)
+
+    if opts.l:
+        log = deltachat.eventlogger.FFIEventLogger(ac, "group-tracking")
+        ac.add_account_plugin(log)
+
+    if not ac.is_configured():
+        assert opts.email and opts.password, (
+            "you must specify --email and --password"
+        )
+        ac.set_config("addr", opts.email)
+        ac.set_config("mail_pw", opts.password)
+        ac.set_config("mvbox_watch", "0")
+        ac.set_config("sentbox_watch", "0")
+
+    ac.add_account_plugin(GroupTrackingPlugin())
+
+    # start IO threads and configure if neccessary
+    ac.start()
+
+    print("{}: waiting for message".format(ac.get_config("addr")))
+
+    ac.wait_shutdown()
+
+
+if __name__ == "__main__":
+    main(sys.argv)

python/tests/test_account.py

@@ -173,14 +173,29 @@ class TestOfflineChat:
     def test_add_member_event(self, ac1):
         chat = ac1.create_group_chat(name="title1")
         assert chat.is_group()
-        # promote the chat
-        chat.send_text("hello")
         contact1 = ac1.create_contact("some1@hello.com", name="some1")
 
         chat.add_contact(contact1)
         for ev in ac1.iter_events(timeout=1):
             if ev.name == "member_added":
                 assert ev.kwargs["chat"] == chat
+                if ev.kwargs["contact"] == ac1.get_self_contact():
+                    continue
+                assert ev.kwargs["contact"] == contact1
+                break
+
+    def test_remove_member_event(self, ac1):
+        chat = ac1.create_group_chat(name="title1")
+        assert chat.is_group()
+        contact1 = ac1.create_contact("some1@hello.com", name="some1")
+        chat.add_contact(contact1)
+        ac1._handle_current_events()
+        chat.remove_contact(contact1)
+        for ev in ac1.iter_events(timeout=1):
+            if ev.name == "member_removed":
+                assert ev.kwargs["chat"] == chat
+                if ev.kwargs["contact"] == ac1.get_self_contact():
+                    continue
                 assert ev.kwargs["contact"] == contact1
                 break
 
@@ -471,11 +486,13 @@ class TestOfflineChat:
         # perform plugin hooks
         ac1._handle_current_events()
 
-        assert len(in_list) == 10
+        assert len(in_list) == 11
+        chat_contacts = chat.get_contacts()
         for in_cmd, in_chat, in_contact in in_list:
             assert in_cmd == "added"
             assert in_chat == chat
-            assert in_contact in contacts
+            assert in_contact in chat_contacts
+            chat_contacts.remove(in_contact)
 
         lp.sec("ac1: removing two contacts and checking things are right")
         chat.remove_contact(contacts[9])
@@ -483,10 +500,13 @@ class TestOfflineChat:
         assert len(chat.get_contacts()) == 9
 
         ac1._handle_current_events()
-        assert len(in_list) == 12
+        assert len(in_list) == 13
         assert in_list[-2][0] == "removed"
         assert in_list[-2][1] == chat
         assert in_list[-2][2] == contacts[9]
+        assert in_list[-1][0] == "removed"
+        assert in_list[-1][1] == chat
+        assert in_list[-1][2] == contacts[3]
 
 
 class TestOnlineAccount:
@@ -1259,6 +1279,51 @@ class TestOnlineAccount:
         msg3 = ac2._evtracker.wait_next_incoming_message()
         assert msg3.get_sender_contact().get_profile_image() is None
 
+    def test_add_remove_member_remote_events(self, acfactory, lp):
+        ac1, ac2 = acfactory.get_two_online_accounts()
+        # activate local plugin for ac2
+        in_list = queue.Queue()
+
+        class InPlugin:
+            @account_hookimpl
+            def member_added(self, chat, contact):
+                in_list.put(("added", chat, contact))
+
+            @account_hookimpl
+            def member_removed(self, chat, contact):
+                in_list.put(("removed", chat, contact))
+
+        ac2.add_account_plugin(InPlugin())
+
+        lp.sec("ac1: create group chat with ac2")
+        chat = ac1.create_group_chat("hello")
+        contact = ac1.create_contact(email=ac2.get_config("addr"))
+        chat.add_contact(contact)
+
+        lp.sec("ac1: send a message to group chat to promote the group")
+        chat.send_text("afterwards promoted")
+        ev1 = in_list.get()
+        ev2 = in_list.get()
+        assert ev1[2] == ac2.get_self_contact()
+        assert ev2[2].addr == ac1.get_config("addr")
+
+        lp.sec("ac1: add address2")
+        contact2 = ac1.create_contact(email="not@example.org")
+        chat.add_contact(contact2)
+        ev1 = in_list.get()
+        assert ev1[2].addr == contact2.addr
+
+        lp.sec("ac1: remove address2")
+        chat.remove_contact(contact2)
+        ev1 = in_list.get()
+        assert ev1[0] == "removed"
+        assert ev1[2].addr == contact2.addr
+
+        lp.sec("ac1: remove ac2 contact from chat")
+        chat.remove_contact(contact)
+        ev1 = in_list.get()
+        assert ev1[2] == ac2.get_self_contact()
+
     def test_set_get_group_image(self, acfactory, data, lp):
         ac1, ac2 = acfactory.get_two_online_accounts()
 

src/chat.rs

@@ -1830,36 +1830,61 @@ pub fn create_group_chat(
     Ok(chat_id)
 }
 
+/// add a contact to the chats_contact table
+/// on success emit MemberAdded event and return true
 pub(crate) fn add_to_chat_contacts_table(
     context: &Context,
     chat_id: ChatId,
     contact_id: u32,
 ) -> bool {
-    // add a contact to a chat; the function does not check the type or if any of the record exist or are already
-    // added to the chat!
-    sql::execute(
+    match sql::execute(
         context,
         &context.sql,
         "INSERT INTO chats_contacts (chat_id, contact_id) VALUES(?, ?)",
         params![chat_id, contact_id as i32],
-    )
-    .is_ok()
+    ) {
+        Ok(()) => {
+            context.call_cb(Event::MemberAdded {
+                chat_id,
+                contact_id,
+            });
+
+            true
+        }
+        Err(err) => {
+            error!(
+                context,
+                "could not add {} to chat {} table: {}", contact_id, chat_id, err
+            );
+
+            false
+        }
+    }
 }
 
+/// remove a contact from the chats_contact table
+/// on success emit MemberRemoved event and return true
 pub(crate) fn remove_from_chat_contacts_table(
     context: &Context,
     chat_id: ChatId,
     contact_id: u32,
 ) -> bool {
-    // remove a contact from the chats_contact table unconditionally
-    // the function does not check the type or if the record exist
-    sql::execute(
+    match sql::execute(
         context,
         &context.sql,
         "DELETE FROM chats_contacts WHERE chat_id=? AND contact_id=?",
         params![chat_id, contact_id as i32],
-    )
-    .is_ok()
+    ) {
+        Ok(()) => {
+            context.call_cb(Event::MemberRemoved {
+                chat_id,
+                contact_id,
+            });
+
+            true
+        }
+        Err(_) => false,
+    }
 }
 
 /// Adds a contact to the chat.
@@ -1955,12 +1980,6 @@ pub(crate) fn add_contact_to_chat_ex(
         msg.param.set_int(Param::Arg2, from_handshake.into());
 
         msg.id = send_msg(context, chat_id, &mut msg)?;
-        // send_msg sends MsgsChanged event
-        // so we only send an explicit MemberAdded one
-        context.call_cb(Event::MemberAdded {
-            chat_id,
-            contact_id: contact.id,
-        });
     } else {
         // send an event for unpromoted groups
         // XXX probably not neccessary because ChatModified should suffice
@@ -2173,10 +2192,6 @@ pub fn remove_contact_from_chat(
                         msg.param.set_cmd(SystemMessage::MemberRemovedFromGroup);
                         msg.param.set(Param::Arg, contact.get_addr());
                         msg.id = send_msg(context, chat_id, &mut msg)?;
-                        context.call_cb(Event::MemberRemoved {
-                            chat_id,
-                            contact_id: contact.id,
-                        });
                         context.call_cb(Event::MsgsChanged {
                             chat_id,
                             msg_id: msg.id,

src/contact.rs

@@ -24,9 +24,6 @@ use crate::peerstate::*;
 use crate::sql;
 use crate::stock::StockMessage;
 
-/// Contacts with at least this origin value are shown in the contact list.
-const DC_ORIGIN_MIN_CONTACT_LIST: i32 = 0x100;
-
 /// An object representing a single contact in memory.
 ///
 /// The contact object is not updated.
@@ -94,6 +91,7 @@ pub enum Origin {
     UnhandledQrScan = 0x80,
 
     /// Reply-To: of incoming message of known sender
+    /// Contacts with at least this origin value are shown in the contact list.
     IncomingReplyTo = 0x100,
 
     /// Cc: of incoming message of known sender
@@ -274,7 +272,7 @@ impl Contact {
     ///
     /// To validate an e-mail address independently of the contact database
     /// use `dc_may_be_valid_addr()`.
-    pub fn lookup_id_by_addr(context: &Context, addr: impl AsRef<str>) -> u32 {
+    pub fn lookup_id_by_addr(context: &Context, addr: impl AsRef<str>, min_origin: Origin) -> u32 {
         if addr.as_ref().is_empty() {
             return 0;
         }
@@ -287,14 +285,13 @@ impl Contact {
         if addr_cmp(addr_normalized, addr_self) {
             return DC_CONTACT_ID_SELF;
         }
-
         context.sql.query_get_value(
             context,
             "SELECT id FROM contacts WHERE addr=?1 COLLATE NOCASE AND id>?2 AND origin>=?3 AND blocked=0;",
             params![
                 addr_normalized,
                 DC_CONTACT_ID_LAST_SPECIAL as i32,
-                DC_ORIGIN_MIN_CONTACT_LIST,
+                min_origin as u32,
             ],
         ).unwrap_or_default()
     }

src/dc_receive_imf.rs

@@ -800,7 +800,6 @@ fn create_or_lookup_group(
     let mut chat_id_blocked = Blocked::Not;
     let mut recreate_member_list = false;
     let mut send_EVENT_CHAT_MODIFIED = false;
-    let mut X_MrRemoveFromGrp = None;
     let mut X_MrAddToGrp = None;
     let mut X_MrGrpNameChanged = false;
     let mut better_msg: String = From::from("");
@@ -848,22 +847,25 @@ fn create_or_lookup_group(
     // but we might not know about this group
 
     let grpname = mime_parser.get(HeaderDef::ChatGroupName).cloned();
+    let mut removed_id = 0;
 
-    if let Some(optional_field) = mime_parser.get(HeaderDef::ChatGroupMemberRemoved).cloned() {
-        X_MrRemoveFromGrp = Some(optional_field);
-        mime_parser.is_system_message = SystemMessage::MemberRemovedFromGroup;
-        let left_group = Contact::lookup_id_by_addr(context, X_MrRemoveFromGrp.as_ref().unwrap())
-            == from_id as u32;
-        better_msg = context.stock_system_msg(
-            if left_group {
-                StockMessage::MsgGroupLeft
-            } else {
-                StockMessage::MsgDelMember
-            },
-            X_MrRemoveFromGrp.as_ref().unwrap(),
-            "",
-            from_id as u32,
-        )
+    if let Some(removed_addr) = mime_parser.get(HeaderDef::ChatGroupMemberRemoved).cloned() {
+        removed_id = Contact::lookup_id_by_addr(context, &removed_addr, Origin::Unknown);
+        if removed_id == 0 {
+            warn!(context, "removed {:?} has no contact_id", removed_addr);
+        } else {
+            mime_parser.is_system_message = SystemMessage::MemberRemovedFromGroup;
+            better_msg = context.stock_system_msg(
+                if removed_id == from_id as u32 {
+                    StockMessage::MsgGroupLeft
+                } else {
+                    StockMessage::MsgDelMember
+                },
+                &removed_addr,
+                "",
+                from_id as u32,
+            );
+        }
     } else {
         let field = mime_parser.get(HeaderDef::ChatGroupMemberAdded).cloned();
         if let Some(optional_field) = field {
@@ -949,7 +951,7 @@ fn create_or_lookup_group(
             && !grpid.is_empty()
             && grpname.is_some()
             // otherwise, a pending "quit" message may pop up
-            && X_MrRemoveFromGrp.is_none()
+            && removed_id == 0
             // re-create explicitly left groups only if ourself is re-added
             && (!group_explicitly_left
                 || X_MrAddToGrp.is_some() && addr_cmp(&self_addr, X_MrAddToGrp.as_ref().unwrap()))
@@ -1083,12 +1085,8 @@ fn create_or_lookup_group(
             }
         }
         send_EVENT_CHAT_MODIFIED = true;
-    } else if let Some(removed_addr) = X_MrRemoveFromGrp {
-        let contact_id = Contact::lookup_id_by_addr(context, removed_addr);
-        if contact_id != 0 {
-            info!(context, "remove {:?} from chat id={}", contact_id, chat_id);
-            chat::remove_from_chat_contacts_table(context, chat_id, contact_id);
-        }
+    } else if removed_id > 0 {
+        chat::remove_from_chat_contacts_table(context, chat_id, removed_id);
         send_EVENT_CHAT_MODIFIED = true;
     }
 

src/stock.rs

@@ -314,7 +314,7 @@ impl Context {
         from_id: u32,
     ) -> String {
         let insert1 = if id == StockMessage::MsgAddMember || id == StockMessage::MsgDelMember {
-            let contact_id = Contact::lookup_id_by_addr(self, param1.as_ref());
+            let contact_id = Contact::lookup_id_by_addr(self, param1.as_ref(), Origin::Unknown);
             if contact_id != 0 {
                 Contact::get_by_id(self, contact_id)
                     .map(|contact| contact.get_name_n_addr())