fix: Change BccSelf default to 0 for chatmail (#6340)

src/chat.rs

@@ -2931,7 +2931,8 @@ pub(crate) async fn create_send_msg_jobs(context: &Context, msg: &mut Message) -
     // because BCC-self messages are also used to detect
     // that message was sent if SMTP server is slow to respond
     // and connection is frequently lost
-    // before receiving status line.
+    // before receiving status line. NB: This is not a problem for chatmail servers, so `BccSelf`
+    // disabled by default is fine.
     //
     // `from` must be the last addr, see `receive_imf_inner()` why.
     if context.get_config_bool(Config::BccSelf).await?

src/config.rs

@@ -143,7 +143,7 @@ pub enum Config {
     /// Send BCC copy to self.
     ///
     /// Should be enabled for multidevice setups.
-    #[strum(props(default = "1"))]
+    /// Default is 0 for chatmail accounts before a backup export, 1 otherwise.
     BccSelf,
 
     /// True if encryption is preferred according to Autocrypt standard.
@@ -202,7 +202,7 @@ pub enum Config {
     /// Value 1 is treated as "delete at once": messages are deleted
     /// immediately, without moving to DeltaChat folder.
     ///
-    /// Default is 1 for chatmail accounts before a backup export, 0 otherwise.
+    /// Default is 1 for chatmail accounts without `BccSelf`, 0 otherwise.
     DeleteServerAfter,
 
     /// Timer in seconds after which the message is deleted from the
@@ -519,11 +519,19 @@ impl Context {
 
         // Default values
         let val = match key {
-            Config::ConfiguredInboxFolder => Some("INBOX"),
-            Config::DeleteServerAfter => match Box::pin(self.is_chatmail()).await? {
-                false => Some("0"),
-                true => Some("1"),
+            Config::BccSelf => match Box::pin(self.is_chatmail()).await? {
+                false => Some("1"),
+                true => Some("0"),
             },
+            Config::ConfiguredInboxFolder => Some("INBOX"),
+            Config::DeleteServerAfter => {
+                match !Box::pin(self.get_config_bool(Config::BccSelf)).await?
+                    && Box::pin(self.is_chatmail()).await?
+                {
+                    true => Some("1"),
+                    false => Some("0"),
+                }
+            }
             _ => key.get_str("default"),
         };
         Ok(val.map(|s| s.to_string()))
@@ -1105,6 +1113,28 @@ mod tests {
         Ok(())
     }
 
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn test_delete_server_after_default() -> Result<()> {
+        let t = &TestContext::new_alice().await;
+
+        // Check that the settings are displayed correctly.
+        assert_eq!(t.get_config(Config::BccSelf).await?, Some("1".to_string()));
+        assert_eq!(
+            t.get_config(Config::DeleteServerAfter).await?,
+            Some("0".to_string())
+        );
+
+        // Leaving emails on the server even w/o `BccSelf` is a good default at least because other
+        // MUAs do so even if the server doesn't save sent messages to some sentbox (like Gmail
+        // does).
+        t.set_config_bool(Config::BccSelf, false).await?;
+        assert_eq!(
+            t.get_config(Config::DeleteServerAfter).await?,
+            Some("0".to_string())
+        );
+        Ok(())
+    }
+
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn test_sync() -> Result<()> {
         let alice0 = TestContext::new_alice().await;

src/imex.rs

@@ -416,7 +416,7 @@ async fn import_backup_stream_inner<R: tokio::io::AsyncRead + Unpin>(
             .context("cannot import unpacked database");
     }
     if res.is_ok() {
-        res = adjust_delete_server_after(context).await;
+        res = adjust_bcc_self(context).await;
     }
     fs::remove_file(unpacked_database)
         .await
@@ -796,7 +796,7 @@ async fn export_database(
         .to_str()
         .with_context(|| format!("path {} is not valid unicode", dest.display()))?;
 
-    adjust_delete_server_after(context).await?;
+    adjust_bcc_self(context).await?;
     context
         .sql
         .set_raw_config_int("backup_time", timestamp)
@@ -826,15 +826,14 @@ 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?;
+/// Sets `Config::BccSelf` (and `DeleteServerAfter` to "never" in effect) 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 `BccSelf` set if
+/// necessary.
+async fn adjust_bcc_self(context: &Context) -> Result<()> {
+    if context.is_chatmail().await? && !context.config_exists(Config::BccSelf).await? {
+        context.set_config(Config::BccSelf, Some("1")).await?;
     }
     Ok(())
 }
@@ -1030,12 +1029,20 @@ mod tests {
 
         let context1 = &TestContext::new_alice().await;
 
-        // Check that the setting is displayed correctly.
+        // Check that the settings are displayed correctly.
+        assert_eq!(
+            context1.get_config(Config::BccSelf).await?,
+            Some("1".to_string())
+        );
         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::BccSelf).await?,
+            Some("0".to_string())
+        );
         assert_eq!(
             context1.get_config(Config::DeleteServerAfter).await?,
             Some("1".to_string())
@@ -1058,6 +1065,10 @@ mod tests {
         assert!(context2.is_configured().await?);
         assert!(context2.is_chatmail().await?);
         for ctx in [context1, context2] {
+            assert_eq!(
+                ctx.get_config(Config::BccSelf).await?,
+                Some("1".to_string())
+            );
             assert_eq!(
                 ctx.get_config(Config::DeleteServerAfter).await?,
                 Some("0".to_string())

src/sql/migrations.rs

@@ -1121,6 +1121,23 @@ CREATE INDEX msgs_status_updates_index2 ON msgs_status_updates (uid);
         .await?;
     }
 
+    inc_and_check(&mut migration_version, 127)?;
+    if dbversion < migration_version {
+        // Existing chatmail configurations having `delete_server_after` disabled should get
+        // `bcc_self` enabled, they may be multidevice configurations because before,
+        // `delete_server_after` was set to 0 upon a backup export for them, but together with this
+        // migration `bcc_self` is enabled instead (whose default is changed to 0 for chatmail). We
+        // don't check `is_chatmail` for simplicity.
+        sql.execute_migration(
+            "INSERT OR IGNORE INTO config (keyname, value)
+             SELECT 'bcc_self', '1'
+             FROM config WHERE keyname='delete_server_after' AND value='0'
+            ",
+            migration_version,
+        )
+        .await?;
+    }
+
     let new_version = sql
         .get_raw_config_int(VERSION_CFG)
         .await?