fix: Reset options that are not available for chatmail

[Hocuri]

We don't show these 3 options in the UI for chatmail profiles.

There were reports from users, who had these options set to something else, and then couldn't change them.

They are not supposed to be something else for chatmail profiles.

fix #7622
fix #7623

[link2xt]

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.

[Hocuri]

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?;
    }

[link2xt]

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.

[link2xt]

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.

[iequidoo]

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

[link2xt]

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.

[iequidoo]

Another possible fix is to not add a migration, but just ignore the options not available for chatmail. This can be done somewhere in get_config_opt()