From 637d2661e801b91c1ebd7bc2f158d8f80e5ce72b Mon Sep 17 00:00:00 2001 From: Hocuri Date: Mon, 21 Sep 2020 15:06:33 +0200 Subject: [PATCH] Show better errors when configuring (#1917) * Show all errors when configuring * Shorten some overly long msgs --- deltachat-ffi/deltachat.h | 3 +- python/tests/test_account.py | 32 +++++++++++++ src/configure/mod.rs | 89 ++++++++++++++++++++++++++++-------- src/imap/mod.rs | 16 +------ src/smtp/mod.rs | 2 +- src/stock.rs | 7 ++- 6 files changed, 113 insertions(+), 36 deletions(-) diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index e369c07b6..ceb13752e 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -4938,8 +4938,9 @@ void dc_event_unref(dc_event_t* event); #define DC_STR_CONFIGURATION_FAILED 84 #define DC_STR_BAD_TIME_MSG_BODY 85 #define DC_STR_UPDATE_REMINDER_MSG_BODY 86 +#define DC_STR_ERROR_NO_NETWORK 87 -#define DC_STR_COUNT 86 +#define DC_STR_COUNT 87 /* * @} diff --git a/python/tests/test_account.py b/python/tests/test_account.py index 7b4fbc8b7..d54f6fb16 100644 --- a/python/tests/test_account.py +++ b/python/tests/test_account.py @@ -1739,6 +1739,38 @@ class TestOnlineAccount: assert len(imap2.get_all_messages()) == 1 + def test_configure_error_msgs(self, acfactory): + ac1, configdict = acfactory.get_online_config() + ac1.update_config(configdict) + ac1.set_config("mail_pw", "abc") # Wrong mail pw + ac1.configure() + while True: + ev = ac1._evtracker.get_matching("DC_EVENT_CONFIGURE_PROGRESS") + if ev.data1 == 0: + break + # Password is wrong so it definitely has to say something about "password" + # but the error message should not be repeated: + assert ev.data2.count("password") == 1 + + ac2, configdict = acfactory.get_online_config() + ac2.update_config(configdict) + ac2.set_config("addr", "abc@def.invalid") # mail server can't be reached + ac2.configure() + while True: + ev = ac2._evtracker.get_matching("DC_EVENT_CONFIGURE_PROGRESS") + if ev.data1 == 0: + break + # Can't connect so it probably should say something about "internet" + # again, should not repeat itself + # If this fails then probably `e.msg.to_lowercase().contains("could not resolve")` + # in configure/mod.rs returned false because the error message was changed + # (i.e. did not contain "could not resolve" anymore) + assert (ev.data2.count("internet") + ev.data2.count("network")) == 1 + # Should mention that it can't connect: + assert ev.data2.count("connect") == 1 + # The users do not know what "configuration" is + assert "configuration" not in ev.data2.lower() + def test_name_changes(self, acfactory): ac1, ac2 = acfactory.get_two_online_accounts() ac1.set_config("displayname", "Account 1") diff --git a/src/configure/mod.rs b/src/configure/mod.rs index d950beebb..987019b0c 100644 --- a/src/configure/mod.rs +++ b/src/configure/mod.rs @@ -8,6 +8,7 @@ mod server_params; use anyhow::{bail, ensure, Context as _, Result}; use async_std::prelude::*; use async_std::task; +use itertools::Itertools; use percent_encoding::{utf8_percent_encode, NON_ALPHANUMERIC}; use crate::config::Config; @@ -250,22 +251,28 @@ async fn configure(ctx: &Context, param: &mut LoginParam) -> Result<()> { let smtp_config_task = task::spawn(async move { let mut smtp_configured = false; + let mut errors = Vec::new(); for smtp_server in smtp_servers { smtp_param.user = smtp_server.username.clone(); smtp_param.server = smtp_server.hostname.clone(); smtp_param.port = smtp_server.port; smtp_param.security = smtp_server.socket; - if try_smtp_one_param(&context_smtp, &smtp_param, &smtp_addr, oauth2, &mut smtp).await { - smtp_configured = true; - break; + match try_smtp_one_param(&context_smtp, &smtp_param, &smtp_addr, oauth2, &mut smtp) + .await + { + Ok(_) => { + smtp_configured = true; + break; + } + Err(e) => errors.push(e), } } if smtp_configured { - Some(smtp_param) + Ok(smtp_param) } else { - None + Err(errors) } }); @@ -281,15 +288,19 @@ async fn configure(ctx: &Context, param: &mut LoginParam) -> Result<()> { .filter(|params| params.protocol == Protocol::IMAP) .collect(); let imap_servers_count = imap_servers.len(); + let mut errors = Vec::new(); for (imap_server_index, imap_server) in imap_servers.into_iter().enumerate() { param.imap.user = imap_server.username.clone(); param.imap.server = imap_server.hostname.clone(); param.imap.port = imap_server.port; param.imap.security = imap_server.socket; - if try_imap_one_param(ctx, ¶m.imap, ¶m.addr, oauth2, &mut imap).await { - imap_configured = true; - break; + match try_imap_one_param(ctx, ¶m.imap, ¶m.addr, oauth2, &mut imap).await { + Ok(_) => { + imap_configured = true; + break; + } + Err(e) => errors.push(e), } progress!( ctx, @@ -297,16 +308,19 @@ async fn configure(ctx: &Context, param: &mut LoginParam) -> Result<()> { ); } if !imap_configured { - bail!("IMAP autoconfig did not succeed"); + bail!(nicer_configuration_error(ctx, errors).await); } progress!(ctx, 850); // Wait for SMTP configuration - if let Some(smtp_param) = smtp_config_task.await { - param.smtp = smtp_param; - } else { - bail!("SMTP autoconfig did not succeed"); + match smtp_config_task.await { + Ok(smtp_param) => { + param.smtp = smtp_param; + } + Err(errors) => { + bail!(nicer_configuration_error(ctx, errors).await); + } } progress!(ctx, 900); @@ -478,7 +492,7 @@ async fn try_imap_one_param( addr: &str, oauth2: bool, imap: &mut Imap, -) -> bool { +) -> Result<(), ConfigurationError> { let inf = format!( "imap: {}@{}:{} security={} certificate_checks={} oauth2={}", param.user, param.server, param.port, param.security, param.certificate_checks, oauth2 @@ -487,10 +501,13 @@ async fn try_imap_one_param( if let Err(err) = imap.connect(context, param, addr, oauth2).await { info!(context, "failure: {}", err); - false + Err(ConfigurationError { + config: inf, + msg: err.to_string(), + }) } else { info!(context, "success: {}", inf); - true + Ok(()) } } @@ -500,7 +517,7 @@ async fn try_smtp_one_param( addr: &str, oauth2: bool, smtp: &mut Smtp, -) -> bool { +) -> Result<(), ConfigurationError> { let inf = format!( "smtp: {}@{}:{} security={} certificate_checks={} oauth2={}", param.user, param.server, param.port, param.security, param.certificate_checks, oauth2 @@ -509,14 +526,48 @@ async fn try_smtp_one_param( if let Err(err) = smtp.connect(context, param, addr, oauth2).await { info!(context, "failure: {}", err); - false + Err(ConfigurationError { + config: inf, + msg: err.to_string(), + }) } else { info!(context, "success: {}", inf); smtp.disconnect().await; - true + Ok(()) } } +#[derive(Debug, thiserror::Error)] +#[error("Trying {config}…\nError: {msg}")] +pub struct ConfigurationError { + config: String, + msg: String, +} + +async fn nicer_configuration_error(context: &Context, errors: Vec) -> String { + let first_err = if let Some(f) = errors.first() { + f + } else { + return "".to_string(); + }; + + if errors + .iter() + .all(|e| e.msg.to_lowercase().contains("could not resolve")) + { + return context + .stock_str(StockMessage::ErrorNoNetwork) + .await + .to_string(); + } + + if errors.iter().all(|e| e.msg == first_err.msg) { + return first_err.msg.to_string(); + } + + errors.iter().map(|e| e.to_string()).join("\n\n") +} + #[derive(Debug, thiserror::Error)] pub enum Error { #[error("Invalid email address: {0:?}")] diff --git a/src/imap/mod.rs b/src/imap/mod.rs index fe2c07873..543c116c5 100644 --- a/src/imap/mod.rs +++ b/src/imap/mod.rs @@ -229,19 +229,7 @@ impl Imap { } } Err(err) => { - let message = { - let config = &self.config; - let imap_server: &str = config.lp.server.as_ref(); - let imap_port = config.lp.port; - context - .stock_string_repl_str2( - StockMessage::ServerResponse, - format!("IMAP {}:{}", imap_server, imap_port), - err.to_string(), - ) - .await - }; - bail!("{}: {}", message, err); + bail!(err); } }; @@ -286,7 +274,7 @@ impl Imap { } self.trigger_reconnect(); - Err(format_err!("{}: {}", message, err)) + Err(format_err!("{}\n\n{}", message, err)) } } } diff --git a/src/smtp/mod.rs b/src/smtp/mod.rs index c6dbcd5e0..873f2b1a7 100644 --- a/src/smtp/mod.rs +++ b/src/smtp/mod.rs @@ -30,7 +30,7 @@ pub enum Error { error: error::Error, }, - #[error("SMTP: failed to connect: {0:?}")] + #[error("SMTP: failed to connect: {0}")] ConnectionFailure(#[source] smtp::error::Error), #[error("SMTP: failed to setup connection {0:?}")] diff --git a/src/stock.rs b/src/stock.rs index 9c4366c77..31a37909c 100644 --- a/src/stock.rs +++ b/src/stock.rs @@ -217,7 +217,7 @@ pub enum StockMessage { #[strum(props(fallback = "You are invited to a video chat, click %1$s to join."))] VideochatInviteMsgBody = 83, - #[strum(props(fallback = "Configuration failed. Error: “%1$s”"))] + #[strum(props(fallback = "Error:\n\n“%1$s”"))] ConfigurationFailed = 84, #[strum(props( @@ -231,6 +231,11 @@ pub enum StockMessage { and you are missing the latest features 😳\n\ Please check https://get.delta.chat or your app store for updates."))] UpdateReminderMsgBody = 86, + + #[strum(props( + fallback = "Could not find your mail server.\n\nPlease check your internet connection." + ))] + ErrorNoNetwork = 87, } /*