From c1f9d8f7a13465ae2c6b69ef9ac18155e530e245 Mon Sep 17 00:00:00 2001 From: bjoern Date: Tue, 15 Nov 2022 11:02:47 +0100 Subject: [PATCH] allow deleting referenced contacts in UI (#3751) * allow deleting referenced contacts in UI we are quite often getting requests of users who want to get rid of some contact in the "new chat" list. there is already a "delete" option, but it does not work for referenced contacts - however, it is not obvious for users that a contact is in use, esp. of some mailing list or larger chat, old contacts, whatever. this pr revives an old idea [^1] of "soft deleting" referenced contacts - this way, the user can remove the annoying entry without the need to understand complicated things and finally saying that deletion is impossible :) once the contact is reused, it will reappear, however, this is already explained in the confirmation dialog of the UIs. technically, this pr was simpler as expected as we already have a Origin::Hidden, that is just reused here. [^1]: https://github.com/deltachat/deltachat-core/pull/542 * update rust doccomment * update changelog * avoid races on contact deletion chats may be created between checking for "no chats" and contact deletion. this is prevented by putting the statement into an EXCLUSIVE transaction. * fix failing python test --- CHANGELOG.md | 1 + deltachat-ffi/deltachat.h | 5 +- deltachat-ffi/src/lib.rs | 5 +- python/tests/test_3_offline.py | 4 +- src/contact.rs | 112 ++++++++++++++++++++++----------- 5 files changed, 87 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 791e92c89..e28aed5ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ ### Changes - add `configured_inbox_folder` to account info #3748 +- `dc_delete_contact()` hides contacts if referenced #3751 ### Fixes - improve IMAP logging, in particular fix incorrect "IMAP IDLE protocol diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index 5e6a1b963..95e4cf45f 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -2056,8 +2056,9 @@ char* dc_get_contact_encrinfo (dc_context_t* context, uint32_t co /** - * Delete a contact. The contact is deleted from the local device. It may happen that this is not - * possible as the contact is in use. In this case, the contact can be blocked. + * Delete a contact so that it disappears from the corresponding lists. + * Depending on whether there are ongoing chats, deletion is done by physical deletion or hiding. + * The contact is deleted from the local device. * * May result in a #DC_EVENT_CONTACTS_CHANGED event. * diff --git a/deltachat-ffi/src/lib.rs b/deltachat-ffi/src/lib.rs index fa7b01c04..68c7c8d27 100644 --- a/deltachat-ffi/src/lib.rs +++ b/deltachat-ffi/src/lib.rs @@ -2144,7 +2144,10 @@ pub unsafe extern "C" fn dc_delete_contact( block_on(async move { match Contact::delete(ctx, contact_id).await { Ok(_) => 1, - Err(_) => 0, + Err(err) => { + error!(ctx, "cannot delete contact: {}", err); + 0 + } } }) } diff --git a/python/tests/test_3_offline.py b/python/tests/test_3_offline.py index a001b3689..3a09eebf4 100644 --- a/python/tests/test_3_offline.py +++ b/python/tests/test_3_offline.py @@ -200,11 +200,11 @@ class TestOfflineContact: assert ac1.delete_contact(contact1) assert contact1 not in ac1.get_contacts() - def test_get_contacts_and_delete_fails(self, acfactory): + def test_delete_referenced_contact_hides_contact(self, acfactory): ac1 = acfactory.get_pseudo_configured_account() contact1 = ac1.create_contact("some1@example.com", name="some1") msg = contact1.create_chat().send_text("one message") - assert not ac1.delete_contact(contact1) + assert ac1.delete_contact(contact1) assert not msg.filemime def test_create_chat_flexibility(self, acfactory): diff --git a/src/contact.rs b/src/contact.rs index 57f1914c1..374a13cca 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -944,43 +944,36 @@ impl Contact { Ok(ret) } - /// Delete a contact. The contact is deleted from the local device. It may happen that this is not - /// possible as the contact is in use. In this case, the contact can be blocked. + /// Delete a contact so that it disappears from the corresponding lists. + // Depending on whether there are ongoing chats, deletion is done by physical deletion or hiding. + // The contact is deleted from the local device. /// /// May result in a `#DC_EVENT_CONTACTS_CHANGED` event. pub async fn delete(context: &Context, contact_id: ContactId) -> Result<()> { ensure!(!contact_id.is_special(), "Can not delete special contact"); - let count_chats = context + context .sql - .count( - "SELECT COUNT(*) FROM chats_contacts WHERE contact_id=?;", - paramsv![contact_id], - ) + .transaction(move |transaction| { + // make sure, the transaction starts with a write command and becomes EXCLUSIVE by that - + // upgrading later may be impossible by races. + let deleted_contacts = transaction.execute( + "DELETE FROM contacts WHERE id=? + AND (SELECT COUNT(*) FROM chats_contacts WHERE contact_id=?)=0;", + paramsv![contact_id, contact_id], + )?; + if deleted_contacts == 0 { + transaction.execute( + "UPDATE contacts SET origin=? WHERE id=?;", + paramsv![Origin::Hidden, contact_id], + )?; + } + Ok(()) + }) .await?; - if count_chats == 0 { - match context - .sql - .execute("DELETE FROM contacts WHERE id=?;", paramsv![contact_id]) - .await - { - Ok(_) => { - context.emit_event(EventType::ContactsChanged(None)); - return Ok(()); - } - Err(err) => { - error!(context, "delete_contact {} failed ({})", contact_id, err); - return Err(err); - } - } - } - - info!( - context, - "could not delete contact {}, there are {} chats with it", contact_id, count_chats - ); - bail!("Could not delete contact with ongoing chats"); + context.emit_event(EventType::ContactsChanged(None)); + Ok(()) } /// Get a single contact object. For a list, see eg. get_contacts(). @@ -1935,21 +1928,70 @@ mod tests { // Create Bob contact let (contact_id, _) = Contact::add_or_lookup(&alice, "Bob", "bob@example.net", Origin::ManuallyCreated) - .await - .unwrap(); - + .await?; let chat = alice .create_chat_with_contact("Bob", "bob@example.net") .await; + assert_eq!( + Contact::get_all(&alice, 0, Some("bob@example.net")) + .await? + .len(), + 1 + ); - // Can't delete a contact with ongoing chats. - assert!(Contact::delete(&alice, contact_id).await.is_err()); + // If a contact has ongoing chats, contact is only hidden on deletion + Contact::delete(&alice, contact_id).await?; + let contact = Contact::load_from_db(&alice, contact_id).await?; + assert_eq!(contact.origin, Origin::Hidden); + assert_eq!( + Contact::get_all(&alice, 0, Some("bob@example.net")) + .await? + .len(), + 0 + ); // Delete chat. chat.get_id().delete(&alice).await?; - // Can delete contact now. + // Can delete contact physically now Contact::delete(&alice, contact_id).await?; + assert!(Contact::load_from_db(&alice, contact_id).await.is_err()); + assert_eq!( + Contact::get_all(&alice, 0, Some("bob@example.net")) + .await? + .len(), + 0 + ); + + Ok(()) + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_delete_and_recreate_contact() -> Result<()> { + let t = TestContext::new_alice().await; + + // test recreation after physical deletion + let contact_id1 = Contact::create(&t, "Foo", "foo@bar.de").await?; + assert_eq!(Contact::get_all(&t, 0, Some("foo@bar.de")).await?.len(), 1); + Contact::delete(&t, contact_id1).await?; + assert!(Contact::load_from_db(&t, contact_id1).await.is_err()); + assert_eq!(Contact::get_all(&t, 0, Some("foo@bar.de")).await?.len(), 0); + let contact_id2 = Contact::create(&t, "Foo", "foo@bar.de").await?; + assert_ne!(contact_id2, contact_id1); + assert_eq!(Contact::get_all(&t, 0, Some("foo@bar.de")).await?.len(), 1); + + // test recreation after hiding + t.create_chat_with_contact("Foo", "foo@bar.de").await; + Contact::delete(&t, contact_id2).await?; + let contact = Contact::load_from_db(&t, contact_id2).await?; + assert_eq!(contact.origin, Origin::Hidden); + assert_eq!(Contact::get_all(&t, 0, Some("foo@bar.de")).await?.len(), 0); + + let contact_id3 = Contact::create(&t, "Foo", "foo@bar.de").await?; + let contact = Contact::load_from_db(&t, contact_id3).await?; + assert_eq!(contact.origin, Origin::ManuallyCreated); + assert_eq!(contact_id3, contact_id2); + assert_eq!(Contact::get_all(&t, 0, Some("foo@bar.de")).await?.len(), 1); Ok(()) }