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 <hocuri@gmx.de>

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"])

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)

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<Option<i64>> {
-        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::<i64>(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.

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<R: tokio::io::AsyncRead + Unpin>(
             .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.