From bf7f64d50ba30f63b1338c66aed780dab9461dfc Mon Sep 17 00:00:00 2001 From: Hocuri Date: Thu, 3 Jun 2021 21:14:39 +0200 Subject: [PATCH] Ignore Drafts folder when scanning (#2454) * Add failing test for #2369 * Completely ignore Drafts folder fix #2369 * Also ignore messages that have the Draft flag set but are not in the Drafts folder --- python/src/deltachat/direct_imap.py | 9 ++ python/tests/test_account.py | 21 ++++- src/config.rs | 13 ++- src/context.rs | 3 - src/dc_receive_imf.rs | 13 +-- src/imap.rs | 132 ++++++++++++++++------------ src/imap/scan_folders.rs | 62 ++++++------- 7 files changed, 155 insertions(+), 98 deletions(-) diff --git a/python/src/deltachat/direct_imap.py b/python/src/deltachat/direct_imap.py index 2da790869..d5291701c 100644 --- a/python/src/deltachat/direct_imap.py +++ b/python/src/deltachat/direct_imap.py @@ -251,7 +251,16 @@ class DirectImap: return res def append(self, folder, msg): + """Upload a message to *folder*. + Trailing whitespace or a linebreak at the beginning will be removed automatically. + """ if msg.startswith("\n"): msg = msg[1:] msg = '\n'.join([s.lstrip() for s in msg.splitlines()]) self.conn.append(folder, msg) + + def get_uid_by_message_id(self, message_id): + msgs = self.conn.search(['HEADER', 'MESSAGE-ID', message_id]) + if len(msgs) == 0: + raise Exception("Did not find message " + message_id + ", maybe you forgot to select the correct folder?") + return msgs[0] diff --git a/python/tests/test_account.py b/python/tests/test_account.py index 6b6f6220c..3ef0945d3 100644 --- a/python/tests/test_account.py +++ b/python/tests/test_account.py @@ -1319,9 +1319,11 @@ class TestOnlineAccount: assert not device_chat.can_send() assert device_chat.get_draft() is None - def test_dont_show_emails_in_draft_folder(self, acfactory): + def test_dont_show_emails_in_draft_folder(self, acfactory, lp): """Most mailboxes have a "Drafts" folder where constantly new emails appear but we don't actually want to show them. - So: If it's outgoing AND there is no Received header AND it's not in the sentbox, then ignore the email.""" + So: If it's outgoing AND there is no Received header AND it's not in the sentbox, then ignore the email. + + If the draft email is sent out later (i.e. moved to "Sent"), it must be shown.""" ac1 = acfactory.get_online_configuring_account() ac1.set_config("show_emails", "2") ac1.create_contact("alice@example.com").create_chat() @@ -1342,7 +1344,7 @@ class TestOnlineAccount: Message-ID: Content-Type: text/plain; charset=utf-8 - message in Drafts + message in Drafts that is moved to Sent later """.format(ac1.get_config("configured_addr"))) ac1.direct_imap.append("Sent", """ From: ac1 <{}> @@ -1355,6 +1357,7 @@ class TestOnlineAccount: """.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") ac1.start_io() msg = ac1._evtracker.wait_next_messages_changed() @@ -1365,6 +1368,18 @@ class TestOnlineAccount: assert msg.text == "subj – message in Sent" assert len(msg.chat.get_messages()) == 1 + ac1.stop_io() + lp.sec("'Send out' the draft, i.e. move it to the Sent folder, and wait for DC to display it this time") + ac1.direct_imap.select_folder("Drafts") + uid = ac1.direct_imap.get_uid_by_message_id("aepiors@example.org") + ac1.direct_imap.conn.move(uid, "Sent") + + ac1.start_io() + msg2 = ac1._evtracker.wait_next_messages_changed() + + assert msg2.text == "subj – message in Drafts that is moved to Sent later" + assert len(msg.chat.get_messages()) == 2 + def test_prefer_encrypt(self, acfactory, lp): """Test quorum rule for encryption preference in 1:1 and group chat.""" ac1, ac2, ac3 = acfactory.get_many_online_accounts(3) diff --git a/src/config.rs b/src/config.rs index d7add1f61..058db676a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -18,7 +18,18 @@ use crate::stock_str; /// The available configuration keys. #[derive( - Debug, Clone, Copy, PartialEq, Eq, Display, EnumString, AsRefStr, EnumIter, EnumProperty, + Debug, + Clone, + Copy, + PartialEq, + Eq, + Display, + EnumString, + AsRefStr, + EnumIter, + EnumProperty, + PartialOrd, + Ord, )] #[strum(serialize_all = "snake_case")] pub enum Config { diff --git a/src/context.rs b/src/context.rs index b4459594c..0307e7444 100644 --- a/src/context.rs +++ b/src/context.rs @@ -538,19 +538,16 @@ impl Context { pub async fn is_sentbox(&self, folder_name: &str) -> Result { let sentbox = self.get_config(Config::ConfiguredSentboxFolder).await?; - Ok(sentbox.as_deref() == Some(folder_name)) } pub async fn is_mvbox(&self, folder_name: &str) -> Result { let mvbox = self.get_config(Config::ConfiguredMvboxFolder).await?; - Ok(mvbox.as_deref() == Some(folder_name)) } pub async fn is_spam_folder(&self, folder_name: &str) -> Result { let spam = self.get_config(Config::ConfiguredSpamFolder).await?; - Ok(spam.as_deref() == Some(folder_name)) } diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index a4642edb6..4248f3755 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -2742,11 +2742,14 @@ mod tests { // Check that the ndn would be downloaded: let headers = mailparse::parse_mail(raw_ndn).unwrap().headers; - assert!( - crate::imap::prefetch_should_download(&t, &headers, ShowEmails::Off) - .await - .unwrap() - ); + assert!(crate::imap::prefetch_should_download( + &t, + &headers, + std::iter::empty(), + ShowEmails::Off + ) + .await + .unwrap()); dc_receive_imf(&t, raw_ndn, "INBOX", 1, false) .await diff --git a/src/imap.rs b/src/imap.rs index ca2a4bc4c..52e0eb21e 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -14,7 +14,6 @@ use async_std::channel::Receiver; use async_std::prelude::*; use num_traits::FromPrimitive; -use crate::chat; use crate::config::Config; use crate::constants::{ Chattype, ShowEmails, Viewtype, DC_FETCH_EXISTING_MSGS_COUNT, DC_FOLDERS_CONFIGURED_VERSION, @@ -36,6 +35,7 @@ use crate::param::Params; use crate::provider::Socket; use crate::scheduler::InterruptInfo; use crate::stock_str; +use crate::{chat, constants::DC_CONTACT_ID_SELF}; mod client; mod idle; @@ -111,14 +111,27 @@ impl async_imap::Authenticator for OAuth2 { } } -#[derive(Debug, PartialEq)] +#[derive(Debug, PartialEq, Clone, Copy)] enum FolderMeaning { Unknown, Spam, - SentObjects, + Sent, + Drafts, Other, } +impl FolderMeaning { + fn to_config(self) -> Option { + match self { + FolderMeaning::Unknown => None, + FolderMeaning::Spam => Some(Config::ConfiguredSpamFolder), + FolderMeaning::Sent => Some(Config::ConfiguredSentboxFolder), + FolderMeaning::Drafts => None, + FolderMeaning::Other => None, + } + } +} + #[derive(Debug)] struct ImapConfig { pub addr: String, @@ -696,6 +709,7 @@ impl Imap { current_uid, &headers, &msg_id, + msg.flags(), folder, show_emails, ) @@ -1300,9 +1314,8 @@ impl Imap { let mut delimiter = ".".to_string(); let mut delimiter_is_default = true; - let mut sentbox_folder = None; - let mut spam_folder = None; let mut mvbox_folder = None; + let mut folder_configs = BTreeMap::new(); let mut fallback_folder = get_fallback_folder(&delimiter); while let Some(folder) = folders.next().await { @@ -1321,31 +1334,26 @@ impl Imap { let folder_meaning = get_folder_meaning(&folder); let folder_name_meaning = get_folder_meaning_by_name(folder.name()); if folder.name() == "DeltaChat" { - // Always takes precendent + // Always takes precedence mvbox_folder = Some(folder.name().to_string()); } else if folder.name() == fallback_folder { - // only set iff none has been already set + // only set if none has been already set if mvbox_folder.is_none() { mvbox_folder = Some(folder.name().to_string()); } - } else if folder_meaning == FolderMeaning::SentObjects { - // Always takes precedent - sentbox_folder = Some(folder.name().to_string()); - } else if folder_meaning == FolderMeaning::Spam { - spam_folder = Some(folder.name().to_string()); - } else if folder_name_meaning == FolderMeaning::SentObjects { - // only set iff none has been already set - if sentbox_folder.is_none() { - sentbox_folder = Some(folder.name().to_string()); - } - } else if folder_name_meaning == FolderMeaning::Spam && spam_folder.is_none() { - spam_folder = Some(folder.name().to_string()); + } else if let Some(config) = folder_meaning.to_config() { + // Always takes precedence + folder_configs.insert(config, folder.name().to_string()); + } else if let Some(config) = folder_name_meaning.to_config() { + // only set if none has been already set + folder_configs + .entry(config) + .or_insert_with(|| folder.name().to_string()); } } drop(folders); info!(context, "Using \"{}\" as folder-delimiter.", delimiter); - info!(context, "sentbox folder is {:?}", sentbox_folder); if mvbox_folder.is_none() && create_mvbox { info!(context, "Creating MVBOX-folder \"DeltaChat\"...",); @@ -1393,15 +1401,8 @@ impl Imap { .set_config(Config::ConfiguredMvboxFolder, Some(mvbox_folder)) .await?; } - if let Some(ref sentbox_folder) = sentbox_folder { - context - .set_config(Config::ConfiguredSentboxFolder, Some(sentbox_folder)) - .await?; - } - if let Some(ref spam_folder) = spam_folder { - context - .set_config(Config::ConfiguredSpamFolder, Some(spam_folder)) - .await?; + for (config, name) in folder_configs { + context.set_config(config, Some(&name)).await?; } context .sql @@ -1474,29 +1475,50 @@ fn get_folder_meaning_by_name(folder_name: &str) -> FolderMeaning { "迷惑メール", "스팸", ]; + const DRAFT_NAMES: &[&str] = &[ + "Drafts", + "Kladder", + "Entw?rfe", + "Borradores", + "Brouillons", + "Bozze", + "Concepten", + "Wersje robocze", + "Rascunhos", + "Entwürfe", + "Koncepty", + "Kopie robocze", + "Taslaklar", + "Utkast", + "Πρόχειρα", + "Черновики", + "下書き", + "草稿", + "임시보관함", + ]; let lower = folder_name.to_lowercase(); if SENT_NAMES.iter().any(|s| s.to_lowercase() == lower) { - FolderMeaning::SentObjects + FolderMeaning::Sent } else if SPAM_NAMES.iter().any(|s| s.to_lowercase() == lower) { FolderMeaning::Spam + } else if DRAFT_NAMES.iter().any(|s| s.to_lowercase() == lower) { + FolderMeaning::Drafts } else { FolderMeaning::Unknown } } fn get_folder_meaning(folder_name: &Name) -> FolderMeaning { - let special_names = vec!["\\Trash", "\\Drafts"]; - for attr in folder_name.attributes() { if let NameAttribute::Custom(ref label) = attr { - if special_names.iter().any(|s| *s == label) { - return FolderMeaning::Other; - } else if label == "\\Sent" { - return FolderMeaning::SentObjects; - } else if label == "\\Spam" || label == "\\Junk" { - return FolderMeaning::Spam; - } + match label.as_ref() { + "\\Trash" => return FolderMeaning::Other, + "\\Sent" => return FolderMeaning::Sent, + "\\Spam" | "\\Junk" => return FolderMeaning::Spam, + "\\Drafts" => return FolderMeaning::Drafts, + _ => {} + }; } } FolderMeaning::Unknown @@ -1614,6 +1636,7 @@ fn prefetch_get_message_id(headers: &[mailparse::MailHeader]) -> Result pub(crate) async fn prefetch_should_download( context: &Context, headers: &[mailparse::MailHeader<'_>], + mut flags: impl Iterator>, show_emails: ShowEmails, ) -> Result { let is_chat_message = headers.get_header_value(HeaderDef::ChatVersion).is_some(); @@ -1651,12 +1674,17 @@ pub(crate) async fn prefetch_should_download( .get_header_value(HeaderDef::AutocryptSetupMessage) .is_some(); - let (_contact_id, blocked_contact, origin) = + let (from_id, blocked_contact, origin) = from_field_to_contact_id(context, &mimeparser::get_from(headers), true).await?; // prevent_rename=true as this might be a mailing list message and in this case it would be bad if we rename the contact. // (prevent_rename is the last argument of from_field_to_contact_id()) - let accepted_contact = origin.is_known(); + if flags.any(|f| f == Flag::Draft) && from_id == DC_CONTACT_ID_SELF { + info!(context, "Ignoring draft message"); + return Ok(false); + } + + let accepted_contact = origin.is_known(); let show = is_autocrypt_setup_message || match show_emails { ShowEmails::Off => is_chat_message || is_reply_to_chat_message, @@ -1675,6 +1703,7 @@ async fn message_needs_processing( current_uid: u32, headers: &[mailparse::MailHeader<'_>], msg_id: &str, + flags: impl Iterator>, folder: &str, show_emails: ShowEmails, ) -> bool { @@ -1698,7 +1727,7 @@ async fn message_needs_processing( // we do not know the message-id // or the message-id is missing (in this case, we create one in the further process) // or some other error happened - let show = match prefetch_should_download(context, headers, show_emails).await { + let show = match prefetch_should_download(context, headers, flags, show_emails).await { Ok(show) => show, Err(err) => { warn!(context, "prefetch_should_download error: {}", err); @@ -1861,25 +1890,16 @@ mod tests { use crate::test_utils::TestContext; #[test] fn test_get_folder_meaning_by_name() { - assert_eq!( - get_folder_meaning_by_name("Gesendet"), - FolderMeaning::SentObjects - ); - assert_eq!( - get_folder_meaning_by_name("GESENDET"), - FolderMeaning::SentObjects - ); - assert_eq!( - get_folder_meaning_by_name("gesendet"), - FolderMeaning::SentObjects - ); + assert_eq!(get_folder_meaning_by_name("Gesendet"), FolderMeaning::Sent); + assert_eq!(get_folder_meaning_by_name("GESENDET"), FolderMeaning::Sent); + assert_eq!(get_folder_meaning_by_name("gesendet"), FolderMeaning::Sent); assert_eq!( get_folder_meaning_by_name("Messages envoyés"), - FolderMeaning::SentObjects + FolderMeaning::Sent ); assert_eq!( get_folder_meaning_by_name("mEsSaGes envoyÉs"), - FolderMeaning::SentObjects + FolderMeaning::Sent ); assert_eq!(get_folder_meaning_by_name("xxx"), FolderMeaning::Unknown); assert_eq!(get_folder_meaning_by_name("SPAM"), FolderMeaning::Spam); diff --git a/src/imap/scan_folders.rs b/src/imap/scan_folders.rs index 66ce40df7..325868388 100644 --- a/src/imap/scan_folders.rs +++ b/src/imap/scan_folders.rs @@ -1,13 +1,13 @@ -use std::time::Instant; +use std::{collections::BTreeMap, time::Instant}; use anyhow::{Context as _, Result}; -use crate::config::Config; -use crate::context::Context; use crate::imap::Imap; +use crate::{config::Config, log::LogExt}; +use crate::{context::Context, imap::FolderMeaning}; use async_std::prelude::*; -use super::{get_folder_meaning, get_folder_meaning_by_name, FolderMeaning}; +use super::{get_folder_meaning, get_folder_meaning_by_name}; impl Imap { pub async fn scan_folders(&mut self, context: &Context) -> Result<()> { @@ -31,8 +31,7 @@ impl Imap { let folders: Vec<_> = session.list(Some(""), Some("*")).await?.collect().await; let watched_folders = get_watched_folders(context).await; - let mut sentbox_folder = None; - let mut spam_folder = None; + let mut folder_configs = BTreeMap::new(); for folder in folders { let folder = match folder { @@ -43,38 +42,41 @@ impl Imap { } }; - let foldername = folder.name(); let folder_meaning = get_folder_meaning(&folder); - let folder_name_meaning = get_folder_meaning_by_name(foldername); + let folder_name_meaning = get_folder_meaning_by_name(folder.name()); - if folder_meaning == FolderMeaning::SentObjects { - // Always takes precedent - sentbox_folder = Some(folder.name().to_string()); - } else if folder_meaning == FolderMeaning::Spam { - spam_folder = Some(folder.name().to_string()); - } else if folder_name_meaning == FolderMeaning::SentObjects { - // only set iff none has been already set - if sentbox_folder.is_none() { - sentbox_folder = Some(folder.name().to_string()); - } - } else if folder_name_meaning == FolderMeaning::Spam && spam_folder.is_none() { - spam_folder = Some(folder.name().to_string()); + if let Some(config) = folder_meaning.to_config() { + // Always takes precedence + folder_configs.insert(config, folder.name().to_string()); + } else if let Some(config) = folder_name_meaning.to_config() { + // only set if none has been already set + folder_configs + .entry(config) + .or_insert_with(|| folder.name().to_string()); } + let is_drafts = folder_meaning == FolderMeaning::Drafts + || (folder_meaning == FolderMeaning::Unknown + && folder_name_meaning == FolderMeaning::Drafts); + // Don't scan folders that are watched anyway - if !watched_folders.contains(&foldername.to_string()) { - if let Err(e) = self.fetch_new_messages(context, foldername, false).await { - warn!(context, "Can't fetch new msgs in scanned folder: {:#}", e); - } + if !watched_folders.contains(&folder.name().to_string()) && !is_drafts { + self.fetch_new_messages(context, folder.name(), false) + .await + .ok_or_log_msg(context, "Can't fetch new msgs in scanned folder"); } } - context - .set_config(Config::ConfiguredSentboxFolder, sentbox_folder.as_deref()) - .await?; - context - .set_config(Config::ConfiguredSpamFolder, spam_folder.as_deref()) - .await?; + // 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, + ] { + context + .set_config(*config, folder_configs.get(config).map(|s| s.as_str())) + .await?; + } last_scan.replace(Instant::now()); Ok(())