Skip to content

Commit

Permalink
fix: Don't try to do fetch_move_delete() if Trash is needed but not y…
Browse files Browse the repository at this point in the history
…et configured

This fixes things for Gmail f.e. Before, `Imap::fetch_move_delete()` was called before looking for
Trash and returned an error because of that failing the whole `fetch_idle()` which prevented
configuring Trash in turn.
  • Loading branch information
iequidoo committed Apr 11, 2024
1 parent d39cbcd commit e9cfcd9
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 21 deletions.
19 changes: 13 additions & 6 deletions python/tests/test_1_online.py
Original file line number Diff line number Diff line change
Expand Up @@ -2128,17 +2128,21 @@ def test_delete_multiple_messages(acfactory, lp):

def test_trash_multiple_messages(acfactory, lp):
ac1, ac2 = acfactory.get_online_accounts(2)
ac2.stop_io()

# Create the Trash folder on IMAP server
# and recreate the account so Trash folder is configured.
# Create the Trash folder on IMAP server and configure deletion to it. There was a bug that if
# Trash wasn't configured initially, it can't be configured later, let's check this.
lp.sec("Creating trash folder")
ac2.direct_imap.create_folder("Trash")
lp.sec("Creating new accounts")
ac2 = acfactory.new_online_configuring_account(cloned_from=ac2)
ac2.set_config("delete_to_trash", "1")

lp.sec("Check that Trash can be configured initially as well")
ac3 = acfactory.new_online_configuring_account(cloned_from=ac2)
acfactory.bring_accounts_online()
assert ac3.get_config("configured_trash_folder")
ac3.stop_io()

ac2.set_config("delete_to_trash", "1")
assert ac2.get_config("configured_trash_folder")
ac2.start_io()
chat12 = acfactory.get_accepted_chat(ac1, ac2)

lp.sec("ac1: sending 3 messages")
Expand All @@ -2153,6 +2157,9 @@ def test_trash_multiple_messages(acfactory, lp):
assert msg.text in texts
if text != "second":
to_delete.append(msg)
# ac2 has received some messages, this is impossible w/o the trash folder configured, let's
# check the configuration.
assert ac2.get_config("configured_trash_folder") == "Trash"

lp.sec("ac2: deleting all messages except second")
assert len(to_delete) == len(texts) - 1
Expand Down
13 changes: 10 additions & 3 deletions src/imap/scan_folders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,16 @@ impl Imap {
Config::ConfiguredSentboxFolder,
Config::ConfiguredTrashFolder,
] {
context
.set_config_internal(conf, folder_configs.get(&conf).map(|s| s.as_str()))
.await?;
let val = folder_configs.get(&conf).map(|s| s.as_str());
let interrupt = conf == Config::ConfiguredTrashFolder
&& val.is_some()
&& context.get_config(conf).await?.is_none();
context.set_config_internal(conf, val).await?;
if interrupt {
// `Imap::fetch_move_delete()` is possible now for other folders (NB: we are in the
// Inbox loop).
context.scheduler.interrupt_oboxes().await;
}
}

last_scan.replace(tools::Time::now());
Expand Down
47 changes: 35 additions & 12 deletions src/scheduler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,14 @@ impl SchedulerState {
}
}

/// Interrupt optional boxes (mvbox, sentbox) loops.
pub(crate) async fn interrupt_oboxes(&self) {
let inner = self.inner.read().await;
if let InnerSchedulerState::Started(ref scheduler) = *inner {
scheduler.interrupt_oboxes();
}
}

pub(crate) async fn interrupt_smtp(&self) {
let inner = self.inner.read().await;
if let InnerSchedulerState::Started(ref scheduler) = *inner {
Expand Down Expand Up @@ -565,19 +573,28 @@ async fn fetch_idle(
.context("store_seen_flags_on_imap")?;
}

// Fetch the watched folder.
connection
.fetch_move_delete(ctx, &mut session, &watch_folder, folder_meaning)
.await
.context("fetch_move_delete")?;
if !ctx.should_delete_to_trash().await?
|| ctx
.get_config(Config::ConfiguredTrashFolder)
.await?
.is_some()
{
// Fetch the watched folder.
connection
.fetch_move_delete(ctx, &mut session, &watch_folder, folder_meaning)
.await
.context("fetch_move_delete")?;

// Mark expired messages for deletion. Marked messages will be deleted from the server
// on the next iteration of `fetch_move_delete`. `delete_expired_imap_messages` is not
// called right before `fetch_move_delete` because it is not well optimized and would
// otherwise slow down message fetching.
delete_expired_imap_messages(ctx)
.await
.context("delete_expired_imap_messages")?;
// Mark expired messages for deletion. Marked messages will be deleted from the server
// on the next iteration of `fetch_move_delete`. `delete_expired_imap_messages` is not
// called right before `fetch_move_delete` because it is not well optimized and would
// otherwise slow down message fetching.
delete_expired_imap_messages(ctx)
.await
.context("delete_expired_imap_messages")?;
} else if folder_config == Config::ConfiguredInboxFolder {
ctx.last_full_folder_scan.lock().await.take();
}

// Scan additional folders only after finishing fetching the watched folder.
//
Expand Down Expand Up @@ -911,6 +928,12 @@ impl Scheduler {
self.inbox.conn_state.interrupt();
}

fn interrupt_oboxes(&self) {
for b in &self.oboxes {
b.conn_state.interrupt();
}
}

fn interrupt_smtp(&self) {
self.smtp.interrupt();
}
Expand Down

0 comments on commit e9cfcd9

Please sign in to comment.