From ff6488371cdd7efde8e7a979f1d04d15ef8deab2 Mon Sep 17 00:00:00 2001 From: iequidoo <117991069+iequidoo@users.noreply.github.com> Date: Sun, 8 Sep 2024 16:53:56 -0300 Subject: [PATCH] feat: Delete messages from a chatmail server immediately by default (#5805) (#5840) I.e. treat `DeleteServerAfter == None` as "delete at once". But when a backup is exported, set `DeleteServerAfter` to 0 so that the server decides when to delete messages, in order not to break the multi-device case. Even if a backup is not aimed for deploying more devices, `DeleteServerAfter` must be set to 0, otherwise the backup is half-useful because after a restoration the user wouldn't see new messages deleted by the device after the backup was done. But if the user explicitly set `DeleteServerAfter`, don't change it when exporting a backup. Anyway even for non-chatmail case the app should warn the user before a backup export if they have `DeleteServerAfter` enabled. Also do the same after a backup import. While this isn't reliable as we can crash in between, this is a problem only for old backups, new backups already have `DeleteServerAfter` set if necessary. --------- Co-authored-by: Hocuri --- python/src/deltachat/testplugin.py | 1 + python/tests/test_1_online.py | 3 +- src/config.rs | 34 +++++++++++------ src/imex.rs | 61 ++++++++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 14 deletions(-) diff --git a/python/src/deltachat/testplugin.py b/python/src/deltachat/testplugin.py index 89b08881f..17be65fca 100644 --- a/python/src/deltachat/testplugin.py +++ b/python/src/deltachat/testplugin.py @@ -525,6 +525,7 @@ class ACFactory: configdict.setdefault("mvbox_move", False) configdict.setdefault("sentbox_watch", False) configdict.setdefault("sync_msgs", False) + configdict.setdefault("delete_server_after", 0) ac.update_config(configdict) self._acsetup._account2config[ac] = configdict self._preconfigure_key(ac, configdict["addr"]) diff --git a/python/tests/test_1_online.py b/python/tests/test_1_online.py index 2a476012c..569864ec7 100644 --- a/python/tests/test_1_online.py +++ b/python/tests/test_1_online.py @@ -2076,12 +2076,11 @@ def test_send_receive_locations(acfactory, lp): def test_immediate_autodelete(acfactory, lp): ac1 = acfactory.new_online_configuring_account() ac2 = acfactory.new_online_configuring_account() + acfactory.bring_accounts_online() # "1" means delete immediately, while "0" means do not delete ac2.set_config("delete_server_after", "1") - acfactory.bring_accounts_online() - lp.sec("ac1: create chat with ac2") chat1 = ac1.create_chat(ac2) ac2.create_chat(ac1) diff --git a/src/config.rs b/src/config.rs index 6ee3ec14b..0f680d229 100644 --- a/src/config.rs +++ b/src/config.rs @@ -174,12 +174,12 @@ pub enum Config { /// Timer in seconds after which the message is deleted from the /// server. /// - /// Equals to 0 by default, which means the message is never - /// deleted. + /// 0 means messages are never deleted by Delta Chat. /// /// Value 1 is treated as "delete at once": messages are deleted /// immediately, without moving to DeltaChat folder. - #[strum(props(default = "0"))] + /// + /// Default is 1 for chatmail accounts before a backup export, 0 otherwise. DeleteServerAfter, /// Timer in seconds after which the message is deleted from the @@ -470,10 +470,15 @@ impl Context { } // Default values - match key { - Config::ConfiguredInboxFolder => Ok(Some("INBOX".to_owned())), - _ => Ok(key.get_str("default").map(|s| s.to_string())), - } + let val = match key { + Config::ConfiguredInboxFolder => Some("INBOX"), + Config::DeleteServerAfter => match Box::pin(self.is_chatmail()).await? { + false => Some("0"), + true => Some("1"), + }, + _ => key.get_str("default"), + }; + Ok(val.map(|s| s.to_string())) } /// Returns Some(T) if a value for the given key exists and was successfully parsed. @@ -555,11 +560,16 @@ impl Context { /// `None` means never delete the message, `Some(0)` means delete /// at once, `Some(x)` means delete after `x` seconds. pub async fn get_config_delete_server_after(&self) -> Result> { - match self.get_config_int(Config::DeleteServerAfter).await? { - 0 => Ok(None), - 1 => Ok(Some(0)), - x => Ok(Some(i64::from(x))), - } + let val = match self + .get_config_parsed::(Config::DeleteServerAfter) + .await? + .unwrap_or(0) + { + 0 => None, + 1 => Some(0), + x => Some(x), + }; + Ok(val) } /// Gets the configured provider, as saved in the `configured_provider` value. diff --git a/src/imex.rs b/src/imex.rs index 51a1f67a1..0b546187d 100644 --- a/src/imex.rs +++ b/src/imex.rs @@ -14,6 +14,7 @@ use tokio_tar::Archive; use crate::blob::BlobDirContents; use crate::chat::{self, delete_and_reset_all_device_msgs}; +use crate::config::Config; use crate::context::Context; use crate::e2ee; use crate::events::EventType; @@ -373,6 +374,9 @@ async fn import_backup_stream_inner( .await .context("cannot import unpacked database"); } + if res.is_ok() { + res = adjust_delete_server_after(context).await; + } fs::remove_file(unpacked_database) .await .context("cannot remove unpacked database") @@ -677,6 +681,7 @@ async fn export_database( .to_str() .with_context(|| format!("path {} is not valid unicode", dest.display()))?; + adjust_delete_server_after(context).await?; context .sql .set_raw_config_int("backup_time", timestamp) @@ -706,6 +711,19 @@ async fn export_database( .await } +/// Sets `Config::DeleteServerAfter` to "never" if needed so that new messages are present on the +/// server after a backup restoration or available for all devices in multi-device case. +/// NB: Calling this after a backup import isn't reliable as we can crash in between, but this is a +/// problem only for old backups, new backups already have `DeleteServerAfter` set if necessary. +async fn adjust_delete_server_after(context: &Context) -> Result<()> { + if context.is_chatmail().await? && !context.config_exists(Config::DeleteServerAfter).await? { + context + .set_config(Config::DeleteServerAfter, Some("0")) + .await?; + } + Ok(()) +} + #[cfg(test)] mod tests { use std::time::Duration; @@ -891,6 +909,49 @@ mod tests { Ok(()) } + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_export_import_chatmail_backup() -> Result<()> { + let backup_dir = tempfile::tempdir().unwrap(); + + let context1 = &TestContext::new_alice().await; + + // Check that the setting is displayed correctly. + assert_eq!( + context1.get_config(Config::DeleteServerAfter).await?, + Some("0".to_string()) + ); + context1.set_config_bool(Config::IsChatmail, true).await?; + assert_eq!( + context1.get_config(Config::DeleteServerAfter).await?, + Some("1".to_string()) + ); + + assert_eq!(context1.get_config_delete_server_after().await?, Some(0)); + imex(context1, ImexMode::ExportBackup, backup_dir.path(), None).await?; + let _event = context1 + .evtracker + .get_matching(|evt| matches!(evt, EventType::ImexProgress(1000))) + .await; + + let context2 = &TestContext::new().await; + let backup = has_backup(context2, backup_dir.path()).await?; + imex(context2, ImexMode::ImportBackup, backup.as_ref(), None).await?; + let _event = context2 + .evtracker + .get_matching(|evt| matches!(evt, EventType::ImexProgress(1000))) + .await; + assert!(context2.is_configured().await?); + assert!(context2.is_chatmail().await?); + for ctx in [context1, context2] { + assert_eq!( + ctx.get_config(Config::DeleteServerAfter).await?, + Some("0".to_string()) + ); + assert_eq!(ctx.get_config_delete_server_after().await?, None); + } + Ok(()) + } + /// This is a regression test for /// https://github.com/deltachat/deltachat-android/issues/2263 /// where the config cache wasn't reset properly after a backup.