Skip to content

fix: Reset options that are not available for chatmail #7624

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Hocuri wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Reset options that are not available for chatmail #7624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/sql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ impl Sql {
// this should be done before updates that use high-level objects that
// rely themselves on the low-level structure.

let recode_avatar = migrations::run(context, self)
let (recode_avatar, update_email_configs) = migrations::run(context, self)
.await
.context("failed to run migrations")?;

Expand Down Expand Up @@ -242,6 +242,22 @@ impl Sql {
}
}

// Reset some options if IsChatmail is true,
// because there were reports from users who had these options set to something else,
// and then couldn't change them because they are hidden in the UI for chatmail profiles
if update_email_configs && context.get_config_bool(Config::IsChatmail).await? {
Copy link
Collaborator

@link2xt link2xt Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also error-prone because it runs after all migrations that we might add in the future. If we in the future add a migration that simply removes is_chatmail from config table as a cleanup, then this code will do nothing and have different effect depending on whether the user upgrades release-by-release or jumps across multiple releases.

// The default for MvboxMove is actually "1", and it's usually set to "0" in `configure.rs`.
context
.set_config_internal(Config::MvboxMove, Some("0"))
.await?;
context
.set_config_internal(Config::OnlyFetchMvbox, None)
.await?;
context
.set_config_internal(Config::ShowEmails, None)
Copy link
Collaborator

@link2xt link2xt Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment above, the problem with these high-level APIs is that we may want to get rid of Config::ShowEmails constant once we remove it, and then to have migration code we will have to keep it or rewrite the migration. Better write the migration with SQL statements and no high-level APIs.

.await?;
}

Ok(())
}

Expand Down
11 changes: 9 additions & 2 deletions src/sql/migrations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ tokio::task_local! {
static STOP_MIGRATIONS_AT: i32;
}

pub async fn run(context: &Context, sql: &Sql) -> Result<bool> {
pub async fn run(context: &Context, sql: &Sql) -> Result<(bool, bool)> {
Copy link
Collaborator

@link2xt link2xt Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not add more of these high-level migrations, I have just removed stale ones:
a4bec7d

They tend to do something different from what they did initially later because high level APIs change.

We can run some raw SQL statements once that look for is_chatmail and reset some setting, if we want to do it again we can just copy-paste the migration.

Copy link
Collaborator Author

@Hocuri Hocuri Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be something like the below, then (it compiles, but I didn't test it yet). I'm not convinced that's better, because it looks quite error-prone (e.g., if the line .optional() was forgotten, then the migration fails iff the is_chatmail config is None; there might be typos; it still depends on bool_from_config which I will need to inline in order to make it dependency-free, making the logic even more complicated).

And if&when we remove these settings in the future, we can just remove the migration.

But if you insist, I can also change it.

    inc_and_check(&mut migration_version, 144)?;
    if dbversion < migration_version {
        sql.execute_migration_transaction(
            |transaction| {
                let is_chatmail = crate::config::bool_from_config(
                    transaction
                        .query_one(
                            "SELECT value FROM config WHERE key='is_chatmail'",
                            (),
                            |row| row.get::<_, String>(0),
                        )
                        .optional()?
                        .as_deref(),
                );
                if is_chatmail {
                    transaction.execute_batch(
                        "\
                        DELETE FROM config WHERE keyname='only_fetch_mvbox';
                        DELETE FROM config WHERE keyname='show_emails'
                        UPDATE config SET value='0' WHERE keyname='mvbox_move'",
                    )?;
                }
                Ok(())
            },
            migration_version,
        )
        .await?;
    }

Copy link
Collaborator

@link2xt link2xt Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it is better, because it is self-contained and we will never have to change it again once it is written unless some very low level rusqlite API changes. We can also copy-paste it later in similar situation.

We do this in migration 139:

core/src/sql/migrations.rs

Lines 1350 to 1388 in 6a293ae

inc_and_check(&mut migration_version, 139)?;
if dbversion < migration_version {
sql.execute_migration_transaction(
|transaction| {
if exists_before_update {
let is_chatmail = transaction
.query_row(
"SELECT value FROM config WHERE keyname='is_chatmail'",
(),
|row| {
let value: String = row.get(0)?;
Ok(value)
},
)
.optional()?
.as_deref()
== Some("1");
// For non-chatmail accounts
// default "bcc_self" was "1".
// If it is not in the database,
// save the old default explicity
// as the new default is "0"
// for all accounts.
if !is_chatmail {
transaction.execute(
"INSERT OR IGNORE
INTO config (keyname, value)
VALUES (?, ?)",
("bcc_self", "1"),
)?;
}
}
Ok(())
},
migration_version,
)
.await?;
}

As for bool_from_config, it should be inlined. It is also ok to check for Some("1") like migration 139, worst case if someone uses 100 instead of 1 for this boolean value is that migration does nothing. Better inline current bool_from_config logic, but practically this boolean is always stored as 1, 0 or nothing and there is no way this can change before the migration runs.

let mut exists_before_update = false;
let mut dbversion_before_update = DBVERSION;

Expand Down Expand Up @@ -69,6 +69,7 @@ pub async fn run(context: &Context, sql: &Sql) -> Result<bool> {

let dbversion = dbversion_before_update;
let mut recode_avatar = false;
let mut update_email_configs = false;

if dbversion < 1 {
sql.execute_migration(
Expand Down Expand Up @@ -1466,6 +1467,12 @@ ALTER TABLE contacts ADD COLUMN name_normalized TEXT;
.await?;
}

inc_and_check(&mut migration_version, 144)?;
if dbversion < migration_version {
update_email_configs = true;
sql.set_db_version(migration_version).await?;
Copy link
Collaborator

@iequidoo iequidoo Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the app crashes right after this line? Next time it starts, update_email_configs will be false i guess

}

let new_version = sql
.get_raw_config_int(VERSION_CFG)
.await?
Expand All @@ -1480,7 +1487,7 @@ ALTER TABLE contacts ADD COLUMN name_normalized TEXT;
}
info!(context, "Database version: v{new_version}.");

Ok(recode_avatar)
Ok((recode_avatar, update_email_configs))
}

fn migrate_key_contacts(
Expand Down
Loading