From dc4dcf483a6ab619173b7c9001b4c2a564d423f3 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Tue, 17 Dec 2024 00:14:44 -0300 Subject: [PATCH] fix: Change BccSelf default to 0 for chatmail (#6340) Change `BccSelf` default to 0 for chatmail configurations and enable it upon a backup export. As for `DeleteServerAfter` who was set to 0 upon a backup export before, make its default dependent on `BccSelf` for chatmail. We don't need `BccSelf` for chatmail by default because we assume single-device use. Also `BccSelf` is needed for "classic" email accounts even if `DeleteServerAfter` is set to "immediately" to detect that a message was sent if SMTP server is slow to respond and connection is lost before receiving the status line which isn't a problem for chatmail servers. --- src/chat.rs | 3 ++- src/config.rs | 18 ++++++++++++------ src/imex.rs | 35 +++++++++++++++++++++++------------ src/sql/migrations.rs | 17 +++++++++++++++++ 4 files changed, 54 insertions(+), 19 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 320fffb854..1e1cad7699 100644 --- a/src/chat.rs +++ b/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? diff --git a/src/config.rs b/src/config.rs index 2734b577c7..dc9a791209 100644 --- a/src/config.rs +++ b/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,17 @@ 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(Config::BccSelf)).await?.as_deref() { + Some("0") => Some("1"), + _ => Some("0"), + } + } _ => key.get_str("default"), }; Ok(val.map(|s| s.to_string())) diff --git a/src/imex.rs b/src/imex.rs index a7c639fb72..aacc920795 100644 --- a/src/imex.rs +++ b/src/imex.rs @@ -416,7 +416,7 @@ async fn import_backup_stream_inner( .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()) diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index dcaafbbff8..1ee96f073e 100644 --- a/src/sql/migrations.rs +++ b/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?