From 807cfd66f1efd5aa14766382b6c7c217d855afb0 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 4 Jan 2024 15:42:18 +0530 Subject: [PATCH 1/4] fix: Reset failed attempts ONLY if succesful connection is made --- frappe/email/doctype/email_account/email_account.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 2f4c23eac6..e5f21bbc47 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -268,15 +268,15 @@ class EmailAccount(Document): if not in_receive and self.use_imap: email_server.imap.logout() - # reset failed attempts count - self.set_failed_attempts_count(0) - return email_server def check_email_server_connection(self, email_server, in_receive): # tries to connect to email server and handles failure try: email_server.connect() + + # reset failed attempts count - do it after succesful connection + self.set_failed_attempts_count(0) except (error_proto, imaplib.IMAP4.error) as e: message = cstr(e).lower().replace(" ", "") auth_error_codes = [ From 71908f1e00459c937af8a2dacc799980c050cf00 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 4 Jan 2024 15:43:10 +0530 Subject: [PATCH 2/4] refactor: respect multi-tenancy redis set/get don't consider site --- frappe/email/doctype/email_account/email_account.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index e5f21bbc47..3c1d0d85a7 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -510,10 +510,10 @@ class EmailAccount(Document): self.set_failed_attempts_count(self.get_failed_attempts_count() + 1) def set_failed_attempts_count(self, value): - frappe.cache.set(f"{self.name}:email-account-failed-attempts", value) + frappe.cache.set_value(f"{self.name}:email-account-failed-attempts", value) def get_failed_attempts_count(self): - return cint(frappe.cache.get(f"{self.name}:email-account-failed-attempts")) + return cint(frappe.cache.get_value(f"{self.name}:email-account-failed-attempts")) def receive(self): """Called by scheduler to receive emails from this EMail account using POP3/IMAP.""" From 3836d942ac98ffd5250c57d99a4d3f000adc3d82 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 4 Jan 2024 15:50:25 +0530 Subject: [PATCH 3/4] fix: logic to disable broken account This doesn't work because when email is broken we still continue and down the line changes get rolled back. --- .../doctype/email_account/email_account.py | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 3c1d0d85a7..f4d7cbf8a2 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -490,25 +490,29 @@ class EmailAccount(Document): def handle_incoming_connect_error(self, description): if self.get_failed_attempts_count() > 2: - self.db_set("enable_incoming", 0) - - for user in get_system_managers(only_name=True): - try: - assign_to.add( - { - "assign_to": user, - "doctype": self.doctype, - "name": self.name, - "description": description, - "priority": "High", - "notify": 1, - } - ) - except assign_to.DuplicateToDoError: - frappe.clear_last_message() + # This is done in background to avoid committing here. + frappe.enqueue(self._disable_broken_incoming_account, description=description) else: self.set_failed_attempts_count(self.get_failed_attempts_count() + 1) + def _disable_broken_incoming_account(self, description): + self.db_set("enable_incoming", 0) + + for user in get_system_managers(only_name=True): + try: + assign_to.add( + { + "assign_to": user, + "doctype": self.doctype, + "name": self.name, + "description": description, + "priority": "High", + "notify": 1, + } + ) + except assign_to.DuplicateToDoError: + pass + def set_failed_attempts_count(self, value): frappe.cache.set_value(f"{self.name}:email-account-failed-attempts", value) From cc2bb854841a1e13d43face6e32d97fab4d4f9bf Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 4 Jan 2024 15:53:59 +0530 Subject: [PATCH 4/4] fix: correct arg type assign_to expects a list of strings of users not a single string. --- frappe/email/doctype/email_account/email_account.py | 6 ++++-- frappe/tests/test_email.py | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index f4d7cbf8a2..0fa328b42d 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -294,6 +294,8 @@ class EmailAccount(Document): error_message = _( "Authentication failed while receiving emails from Email Account: {0}." ).format(self.name) + + error_message = _("Email Account Disabled.") + " " + error_message error_message += "
" + _("Message from server: {0}").format(cstr(e)) self.handle_incoming_connect_error(description=error_message) return None @@ -489,7 +491,7 @@ class EmailAccount(Document): state.pop("_smtp_server_instance", None) def handle_incoming_connect_error(self, description): - if self.get_failed_attempts_count() > 2: + if self.get_failed_attempts_count() > 5: # This is done in background to avoid committing here. frappe.enqueue(self._disable_broken_incoming_account, description=description) else: @@ -502,7 +504,7 @@ class EmailAccount(Document): try: assign_to.add( { - "assign_to": user, + "assign_to": [user], "doctype": self.doctype, "name": self.name, "description": description, diff --git a/frappe/tests/test_email.py b/frappe/tests/test_email.py index 13f385a22d..bd17a523ba 100644 --- a/frappe/tests/test_email.py +++ b/frappe/tests/test_email.py @@ -361,8 +361,10 @@ class TestEmailIntegrationTest(FrappeTestCase): subject = "checking if email works" content = "is email working?" - frappe.sendmail(sender=sender, recipients=recipients, subject=subject, content=content, now=True) - email = frappe.get_last_doc("Email Queue") + email = frappe.sendmail( + sender=sender, recipients=recipients, subject=subject, content=content, now=True + ) + email.reload() self.assertEqual(email.sender, sender) self.assertEqual(len(email.recipients), 2) self.assertEqual(email.status, "Sent")