Ignore messages from all spam folders if there are many

For example, if there is both a Spam and Junk folder,
both of them should be ignored, even though only one
of them can be a ConfiguredSpamFolder.

CHANGELOG.md

@@ -12,6 +12,7 @@
 - Fix a bug where a blocked contact could send a contact request #3218
 - Make sure, videochat-room-names are always URL-safe #3231
 - Try removing account folder multiple times in case of failure #3229
+- Ignore messages from all spam folders if there are many #3246
 
 ### Changes
 

python/src/deltachat/direct_imap.py

@@ -40,8 +40,6 @@ def dc_account_extra_configure(account):
                     # We just deleted the folder, so we have to make DC forget about it, too
                     if account.get_config("configured_sentbox_folder") == folder:
                         account.set_config("configured_sentbox_folder", None)
-                    if account.get_config("configured_spam_folder") == folder:
-                        account.set_config("configured_spam_folder", None)
 
             setattr(account, "direct_imap", imap)
 

python/tests/test_account.py

@@ -1468,6 +1468,15 @@ class TestOnlineAccount:
 
             Unknown message in Spam
         """.format(ac1.get_config("configured_addr")))
+        ac1.direct_imap.append("Junk", """
+            From: unknown.address@junk.org
+            Subject: subj
+            To: {}
+            Message-ID: <spam.message@junk.org>
+            Content-Type: text/plain; charset=utf-8
+
+            Unknown message in Junk
+        """.format(ac1.get_config("configured_addr")))
 
         ac1.set_config("scan_all_folders_debounce_secs", "0")
         lp.sec("All prepared, now let DC find the message")

src/config.rs

@@ -129,7 +129,6 @@ pub enum Config {
     ConfiguredInboxFolder,
     ConfiguredMvboxFolder,
     ConfiguredSentboxFolder,
-    ConfiguredSpamFolder,
     ConfiguredTimestamp,
     ConfiguredProvider,
     Configured,

src/context.rs

@@ -608,11 +608,6 @@ impl Context {
         Ok(mvbox.as_deref() == Some(folder_name))
     }
 
-    pub async fn is_spam_folder(&self, folder_name: &str) -> Result<bool> {
-        let spam = self.get_config(Config::ConfiguredSpamFolder).await?;
-        Ok(spam.as_deref() == Some(folder_name))
-    }
-
     pub(crate) fn derive_blobdir(dbfile: &PathBuf) -> PathBuf {
         let mut blob_fname = OsString::new();
         blob_fname.push(dbfile.file_name().unwrap_or_default());

src/imap.rs

@@ -131,7 +131,7 @@ impl FolderMeaning {
     fn to_config(self) -> Option<Config> {
         match self {
             FolderMeaning::Unknown => None,
-            FolderMeaning::Spam => Some(Config::ConfiguredSpamFolder),
+            FolderMeaning::Spam => None,
             FolderMeaning::Sent => Some(Config::ConfiguredSentboxFolder),
             FolderMeaning::Drafts => None,
             FolderMeaning::Other => None,
@@ -512,7 +512,12 @@ impl Imap {
     ///
     /// Prefetches headers and downloads new message from the folder, moves messages away from the
     /// folder and deletes messages in the folder.
-    pub async fn fetch_move_delete(&mut self, context: &Context, watch_folder: &str) -> Result<()> {
+    pub async fn fetch_move_delete(
+        &mut self,
+        context: &Context,
+        watch_folder: &str,
+        is_spam_folder: bool,
+    ) -> Result<()> {
         if !context.sql.is_open().await {
             // probably shutdown
             bail!("IMAP operation attempted while it is torn down");
@@ -520,7 +525,7 @@ impl Imap {
         self.prepare(context).await?;
 
         let msgs_fetched = self
-            .fetch_new_messages(context, watch_folder, false)
+            .fetch_new_messages(context, watch_folder, is_spam_folder, false)
             .await
             .context("fetch_new_messages")?;
         if msgs_fetched && context.get_config_delete_device_after().await?.is_some() {
@@ -734,9 +739,10 @@ impl Imap {
         &mut self,
         context: &Context,
         folder: &str,
+        is_spam_folder: bool,
         fetch_existing_msgs: bool,
     ) -> Result<bool> {
-        if should_ignore_folder(context, folder).await? {
+        if should_ignore_folder(context, folder, is_spam_folder).await? {
             info!(context, "Not fetching from {}", folder);
             return Ok(false);
         }
@@ -779,7 +785,7 @@ impl Imap {
             // Get the Message-ID or generate a fake one to identify the message in the database.
             let message_id = prefetch_get_message_id(&headers).unwrap_or_else(dc_create_id);
 
-            let target = match target_folder(context, folder, &headers).await? {
+            let target = match target_folder(context, folder, is_spam_folder, &headers).await? {
                 Some(config) => match context.get_config(config).await? {
                     Some(target) => target,
                     None => folder.to_string(),
@@ -810,7 +816,7 @@ impl Imap {
                 // If the sender is known, the message will be moved to the Inbox or Mvbox
                 // and then we download the message from there.
                 // Also see `spam_target_folder()`.
-                && !context.is_spam_folder(folder).await?
+                && !is_spam_folder
                 && prefetch_should_download(
                     context,
                     &headers,
@@ -1730,13 +1736,14 @@ async fn spam_target_folder(
 pub async fn target_folder(
     context: &Context,
     folder: &str,
+    is_spam_folder: bool,
     headers: &[mailparse::MailHeader<'_>],
 ) -> Result<Option<Config>> {
     if context.is_mvbox(folder).await? {
         return Ok(None);
     }
 
-    if context.is_spam_folder(folder).await? {
+    if is_spam_folder {
         spam_target_folder(context, headers).await
     } else if needs_move_to_mvbox(context, headers).await? {
         Ok(Some(Config::ConfiguredMvboxFolder))
@@ -2182,7 +2189,11 @@ pub async fn get_config_last_seen_uid(context: &Context, folder: &str) -> Result
 ///
 /// This caters for the [`Config::OnlyFetchMvbox`] setting which means mails from folders
 /// not explicitly watched should not be fetched.
-async fn should_ignore_folder(context: &Context, folder: &str) -> Result<bool> {
+async fn should_ignore_folder(
+    context: &Context,
+    folder: &str,
+    is_spam_folder: bool,
+) -> Result<bool> {
     if !context.get_config_bool(Config::OnlyFetchMvbox).await? {
         return Ok(false);
     }
@@ -2190,7 +2201,7 @@ async fn should_ignore_folder(context: &Context, folder: &str) -> Result<bool> {
         // Still respect the SentboxWatch setting.
         return Ok(!context.get_config_bool(Config::SentboxWatch).await?);
     }
-    Ok(!(context.is_mvbox(folder).await? || context.is_spam_folder(folder).await?))
+    Ok(!(context.is_mvbox(folder).await? || is_spam_folder))
 }
 
 /// Builds a list of sequence/uid sets. The returned sets have each no more than around 1000
@@ -2366,9 +2377,6 @@ mod tests {
                                folder, mvbox_move, chat_msg, accepted_chat, outgoing, setupmessage);
 
         let t = TestContext::new_alice().await;
-        t.ctx
-            .set_config(Config::ConfiguredSpamFolder, Some("Spam"))
-            .await?;
         t.ctx
             .set_config(Config::ConfiguredMvboxFolder, Some("DeltaChat"))
             .await?;
@@ -2410,7 +2418,9 @@ mod tests {
 
         let (headers, _) = mailparse::parse_headers(bytes)?;
 
-        let actual = if let Some(config) = target_folder(&t, folder, &headers).await? {
+        let is_spam_folder = folder == "Spam";
+        let actual =
+            if let Some(config) = target_folder(&t, folder, is_spam_folder, &headers).await? {
                 t.get_config(config).await?
             } else {
                 None

src/imap/idle.rs

@@ -157,7 +157,10 @@ impl Imap {
                     // in anything.  If so, we behave as if IDLE had data but
                     // will have already fetched the messages so perform_*_fetch
                     // will not find any new.
-                    match self.fetch_new_messages(context, &watch_folder, false).await {
+                    match self
+                        .fetch_new_messages(context, &watch_folder, false, false)
+                        .await
+                    {
                         Ok(res) => {
                             info!(context, "fetch_new_messages returned {:?}", res);
                             if res {

src/imap/scan_folders.rs

@@ -60,6 +60,9 @@ impl Imap {
             let is_drafts = folder_meaning == FolderMeaning::Drafts
                 || (folder_meaning == FolderMeaning::Unknown
                     && folder_name_meaning == FolderMeaning::Drafts);
+            let is_spam_folder = folder_meaning == FolderMeaning::Spam
+                || (folder_meaning == FolderMeaning::Unknown
+                    && folder_name_meaning == FolderMeaning::Spam);
 
             // Don't scan folders that are watched anyway
             if !watched_folders.contains(&folder.name().to_string()) && !is_drafts {
@@ -67,7 +70,7 @@ impl Imap {
                 self.server_sent_unsolicited_exists(context)?;
 
                 loop {
-                    self.fetch_move_delete(context, folder.name())
+                    self.fetch_move_delete(context, folder.name(), is_spam_folder)
                         .await
                         .ok_or_log_msg(context, "Can't fetch new msgs in scanned folder");
 
@@ -79,16 +82,15 @@ impl Imap {
             }
         }
 
-        // We iterate over both folder meanings to make sure that if e.g. the "Sent" folder was deleted,
-        // `ConfiguredSentboxFolder` is set to `None`:
-        for config in &[
-            Config::ConfiguredSentboxFolder,
-            Config::ConfiguredSpamFolder,
-        ] {
+        // Set the `ConfiguredSentboxFolder` or set it to `None` if the folder was deleted.
         context
-                .set_config(*config, folder_configs.get(config).map(|s| s.as_str()))
+            .set_config(
+                Config::ConfiguredSentboxFolder,
+                folder_configs
+                    .get(&Config::ConfiguredSentboxFolder)
+                    .map(|s| s.as_str()),
+            )
             .await?;
-        }
 
         last_scan.replace(Instant::now());
         Ok(true)

src/job.rs

@@ -335,7 +335,7 @@ impl Job {
                 Config::ConfiguredSentboxFolder,
             ] {
                 if let Some(folder) = job_try!(context.get_config(*config).await) {
-                    if let Err(e) = imap.fetch_new_messages(context, &folder, true).await {
+                    if let Err(e) = imap.fetch_new_messages(context, &folder, false, true).await {
                         // We are using Anyhow's .context() and to show the inner error, too, we need the {:#}:
                         warn!(context, "Could not fetch messages, retrying: {:#}", e);
                         return Status::RetryLater;

src/scheduler.rs

@@ -157,7 +157,10 @@ async fn fetch(ctx: &Context, connection: &mut Imap) {
             }
 
             // fetch
-            if let Err(err) = connection.fetch_move_delete(ctx, &watch_folder).await {
+            if let Err(err) = connection
+                .fetch_move_delete(ctx, &watch_folder, false)
+                .await
+            {
                 connection.trigger_reconnect(ctx).await;
                 warn!(ctx, "{:#}", err);
             }
@@ -194,7 +197,10 @@ async fn fetch_idle(ctx: &Context, connection: &mut Imap, folder: Config) -> Int
             }
 
             // Fetch the watched folder.
-            if let Err(err) = connection.fetch_move_delete(ctx, &watch_folder).await {
+            if let Err(err) = connection
+                .fetch_move_delete(ctx, &watch_folder, false)
+                .await
+            {
                 connection.trigger_reconnect(ctx).await;
                 warn!(ctx, "{:#}", err);
                 return InterruptInfo::new(false);
@@ -230,7 +236,10 @@ async fn fetch_idle(ctx: &Context, connection: &mut Imap, folder: Config) -> Int
                         // In most cases this will select the watched folder and return because there are
                         // no new messages. We want to select the watched folder anyway before going IDLE
                         // there, so this does not take additional protocol round-trip.
-                        if let Err(err) = connection.fetch_move_delete(ctx, &watch_folder).await {
+                        if let Err(err) = connection
+                            .fetch_move_delete(ctx, &watch_folder, false)
+                            .await
+                        {
                             connection.trigger_reconnect(ctx).await;
                             warn!(ctx, "{:#}", err);
                             return InterruptInfo::new(false);