From f38386d164c89636bde04f0f696ecd8febf7f675 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Wed, 4 Mar 2020 23:24:58 +0100 Subject: [PATCH] fix member_added/member_removed event with tests and and provide a group-tracking example --- deltachat-ffi/src/lib.rs | 6 ++- python/examples/group_tracking.py | 64 ++++++++++++++++++++++++++ python/tests/test_account.py | 75 ++++++++++++++++++++++++++++--- src/chat.rs | 55 ++++++++++++++--------- src/contact.rs | 9 ++-- src/dc_receive_imf.rs | 44 +++++++++--------- src/stock.rs | 2 +- 7 files changed, 198 insertions(+), 57 deletions(-) create mode 100644 python/examples/group_tracking.py diff --git a/deltachat-ffi/src/lib.rs b/deltachat-ffi/src/lib.rs index 5d68aedd6..664866300 100644 --- a/deltachat-ffi/src/lib.rs +++ b/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) } diff --git a/python/examples/group_tracking.py b/python/examples/group_tracking.py new file mode 100644 index 000000000..3e4390d3a --- /dev/null +++ b/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) diff --git a/python/tests/test_account.py b/python/tests/test_account.py index ffecef9af..a51cb38b1 100644 --- a/python/tests/test_account.py +++ b/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() diff --git a/src/chat.rs b/src/chat.rs index 1d09540b6..576185e7b 100644 --- a/src/chat.rs +++ b/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, diff --git a/src/contact.rs b/src/contact.rs index 65ff35ccf..c74ce8db9 100644 --- a/src/contact.rs +++ b/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) -> u32 { + pub fn lookup_id_by_addr(context: &Context, addr: impl AsRef, 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() } diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index 6620202db..53ae79c45 100644 --- a/src/dc_receive_imf.rs +++ b/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; } diff --git a/src/stock.rs b/src/stock.rs index 1653b4855..d536d522e 100644 --- a/src/stock.rs +++ b/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())