From 2b225563097fb4c392191c69c34baa293ca478b7 Mon Sep 17 00:00:00 2001 From: phot0n Date: Mon, 5 Jun 2023 15:25:17 +0530 Subject: [PATCH 1/6] chore: remove deprecated send method from smtp --- frappe/email/smtp.py | 30 ------------------------------ 1 file changed, 30 deletions(-) diff --git a/frappe/email/smtp.py b/frappe/email/smtp.py index 3b22bc4ce4..7b15440ccf 100644 --- a/frappe/email/smtp.py +++ b/frappe/email/smtp.py @@ -13,36 +13,6 @@ class InvalidEmailCredentials(frappe.ValidationError): pass -def send(email, append_to=None, retry=1): - """Deprecated: Send the message or add it to Outbox Email""" - - def _send(retry): - from frappe.email.doctype.email_account.email_account import EmailAccount - - try: - email_account = EmailAccount.find_outgoing(match_by_doctype=append_to) - smtpserver = email_account.get_smtp_server() - - # validate is called in as_string - email_body = email.as_string() - - smtpserver.sess.sendmail(email.sender, email.recipients + (email.cc or []), email_body) - except smtplib.SMTPSenderRefused: - frappe.throw(_("Invalid login or password"), title="Email Failed") - raise - except smtplib.SMTPRecipientsRefused: - frappe.msgprint(_("Invalid recipient address"), title="Email Failed") - raise - except (smtplib.SMTPServerDisconnected, smtplib.SMTPAuthenticationError): - if not retry: - raise - else: - retry = retry - 1 - _send(retry) - - _send(retry) - - class SMTPServer: def __init__( self, From 742a6082ac9e490512c4422a9cbdc15464ae9d6f Mon Sep 17 00:00:00 2001 From: phot0n Date: Mon, 5 Jun 2023 15:26:51 +0530 Subject: [PATCH 2/6] fix: remove unnecessary statuses from email queue and only append emails to sent if imap is enabled --- .../doctype/email_account/email_account.json | 6 +++--- .../doctype/email_account/email_account.py | 21 +++++++------------ .../doctype/email_domain/email_domain.json | 3 ++- .../email/doctype/email_queue/email_queue.js | 12 ++++------- .../doctype/email_queue/email_queue.json | 4 ++-- .../email/doctype/email_queue/email_queue.py | 21 +++++++------------ 6 files changed, 26 insertions(+), 41 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.json b/frappe/email/doctype/email_account/email_account.json index 85241b8194..d61165b787 100644 --- a/frappe/email/doctype/email_account/email_account.json +++ b/frappe/email/doctype/email_account/email_account.json @@ -508,7 +508,7 @@ }, { "default": "0", - "depends_on": "eval:!doc.domain && doc.enable_outgoing", + "depends_on": "eval:!doc.domain && doc.enable_outgoing && doc.enable_incoming && doc.use_imap", "fieldname": "append_emails_to_sent_folder", "fieldtype": "Check", "hide_days": 1, @@ -616,7 +616,7 @@ "icon": "fa fa-inbox", "index_web_pages_for_search": 1, "links": [], - "modified": "2022-12-28 14:56:18.754804", + "modified": "2023-06-05 15:03:08.538819", "modified_by": "Administrator", "module": "Email", "name": "Email Account", @@ -639,4 +639,4 @@ "sort_order": "DESC", "states": [], "track_changes": 1 -} +} \ No newline at end of file diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 80b1224343..159e6d9583 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -652,21 +652,16 @@ class EmailAccount(Document): frappe.throw(_("Automatic Linking can be activated only for one Email Account.")) def append_email_to_sent_folder(self, message): - email_server = None - try: - email_server = self.get_incoming_server(in_receive=True) - except Exception: - self.log_error("Email Connection Error") - - if not email_server: + if not (self.enable_incoming and self.use_imap): + # don't try appending if enable incoming and imap is not set return - if email_server.imap: - try: - message = safe_encode(message) - email_server.imap.append("Sent", "\\Seen", imaplib.Time2Internaldate(time.time()), message) - except Exception: - self.log_error("Unable to add to Sent folder") + try: + email_server = self.get_incoming_server(in_receive=True) + message = safe_encode(message) + email_server.imap.append("Sent", "\\Seen", imaplib.Time2Internaldate(time.time()), message) + except Exception: + self.log_error("Unable to add to Sent folder") def get_oauth_token(self): if self.auth_method == "OAuth": diff --git a/frappe/email/doctype/email_domain/email_domain.json b/frappe/email/doctype/email_domain/email_domain.json index c162060436..5cb4c19940 100644 --- a/frappe/email/doctype/email_domain/email_domain.json +++ b/frappe/email/doctype/email_domain/email_domain.json @@ -107,6 +107,7 @@ }, { "default": "0", + "depends_on": "eval:doc.use_imap", "fieldname": "append_emails_to_sent_folder", "fieldtype": "Check", "label": "Append Emails to Sent Folder" @@ -133,7 +134,7 @@ "link_fieldname": "domain" } ], - "modified": "2022-08-19 12:55:06.434541", + "modified": "2023-06-05 12:55:06.434541", "modified_by": "Administrator", "module": "Email", "name": "Email Domain", diff --git a/frappe/email/doctype/email_queue/email_queue.js b/frappe/email/doctype/email_queue/email_queue.js index 2ac4b6f7fe..7d05053d4e 100644 --- a/frappe/email/doctype/email_queue/email_queue.js +++ b/frappe/email/doctype/email_queue/email_queue.js @@ -3,7 +3,7 @@ frappe.ui.form.on("Email Queue", { refresh: function (frm) { - if (["Not Sent", "Partially Sent"].indexOf(frm.doc.status) != -1) { + if (["Not Sent", "Partially Sent"].includes(frm.doc.status)) { let button = frm.add_custom_button("Send Now", function () { frappe.call({ method: "frappe.email.doctype.email_queue.email_queue.send_now", @@ -16,9 +16,7 @@ frappe.ui.form.on("Email Queue", { }, }); }); - } - - if (["Error", "Partially Errored"].indexOf(frm.doc.status) != -1) { + } else if (frm.doc.status == "Error") { let button = frm.add_custom_button("Retry Sending", function () { frm.call({ method: "retry_sending", @@ -26,10 +24,8 @@ frappe.ui.form.on("Email Queue", { name: frm.doc.name, }, btn: button, - callback: function (r) { - if (!r.exc) { - frm.set_value("status", "Not Sent"); - } + callback: function () { + frm.reload_doc(); }, }); }); diff --git a/frappe/email/doctype/email_queue/email_queue.json b/frappe/email/doctype/email_queue/email_queue.json index ac8d656678..ce8c3b5e63 100644 --- a/frappe/email/doctype/email_queue/email_queue.json +++ b/frappe/email/doctype/email_queue/email_queue.json @@ -58,7 +58,7 @@ "in_list_view": 1, "in_standard_filter": 1, "label": "Status", - "options": "\nNot Sent\nSending\nSent\nError\nExpired" + "options": "Not Sent\nSending\nSent\nPartially Sent\nError\nExpired" }, { "fieldname": "error", @@ -152,7 +152,7 @@ "idx": 1, "in_create": 1, "links": [], - "modified": "2023-03-16 12:15:17.850292", + "modified": "2023-06-05 12:15:17.850292", "modified_by": "Administrator", "module": "Email", "name": "Email Queue", diff --git a/frappe/email/doctype/email_queue/email_queue.py b/frappe/email/doctype/email_queue/email_queue.py index d254c87a0a..d977f2097c 100644 --- a/frappe/email/doctype/email_queue/email_queue.py +++ b/frappe/email/doctype/email_queue/email_queue.py @@ -147,7 +147,7 @@ class EmailQueue(Document): frappe.flags.sent_mail = message return - if ctx.email_account_doc.append_emails_to_sent_folder and ctx.sent_to: + if ctx.email_account_doc.append_emails_to_sent_folder: ctx.email_account_doc.append_email_to_sent_folder(message) @staticmethod @@ -224,25 +224,21 @@ class SendMailContext: self.log_exception(exc_type, exc_val, exc_tb) if exc_type in exceptions: - email_status = "Partially Sent" if self.sent_to else "Not Sent" - self.queue_doc.update_status(status=email_status, commit=True) + update_fields = {"status": "Partially Sent" if self.sent_to else "Not Sent"} elif exc_type: if self.queue_doc.retry < get_email_retry_limit(): update_fields = {"status": "Not Sent", "retry": self.queue_doc.retry + 1} else: - update_fields = {"status": (self.sent_to and "Partially Errored") or "Error"} - self.queue_doc.update_status(**update_fields, commit=True) + update_fields = {"status": "Error"} else: - email_status = self.is_mail_sent_to_all() and "Sent" - email_status = email_status or (self.sent_to and "Partially Sent") or "Not Sent" - update_fields = { - "status": email_status, + "status": "Sent", "email_account": self.email_account_doc.name if self.email_account_doc.is_exists_in_db() else None, } - self.queue_doc.update_status(**update_fields, commit=True) + + self.queue_doc.update_status(**update_fields, commit=True) def log_exception(self, exc_type, exc_val, exc_tb): if exc_type: @@ -262,9 +258,6 @@ class SendMailContext: recipient.update_db(status="Sent", commit=True) self.sent_to.append(recipient.recipient) - def is_mail_sent_to_all(self): - return sorted(self.sent_to) == sorted(rec.recipient for rec in self.queue_doc.recipients) - def get_message_object(self, message): return Parser(policy=SMTPUTF8).parsestr(message) @@ -379,7 +372,7 @@ def retry_sending(name): doc = frappe.get_doc("Email Queue", name) doc.check_permission() - if doc and (doc.status == "Error" or doc.status == "Partially Errored"): + if doc and doc.status == "Error": doc.status = "Not Sent" for d in doc.recipients: if d.status != "Sent": From df7afa93b8c2e4609d78f8d64751fb336c34165f Mon Sep 17 00:00:00 2001 From: phot0n Date: Wed, 7 Jun 2023 02:22:50 +0530 Subject: [PATCH 3/6] chore: log traceback directly to the queue doc --- .../doctype/email_queue/email_queue.json | 3 +- .../email/doctype/email_queue/email_queue.py | 32 ++++++++----------- 2 files changed, 15 insertions(+), 20 deletions(-) diff --git a/frappe/email/doctype/email_queue/email_queue.json b/frappe/email/doctype/email_queue/email_queue.json index ce8c3b5e63..b1df78c564 100644 --- a/frappe/email/doctype/email_queue/email_queue.json +++ b/frappe/email/doctype/email_queue/email_queue.json @@ -61,6 +61,7 @@ "options": "Not Sent\nSending\nSent\nPartially Sent\nError\nExpired" }, { + "depends_on": "eval:doc.error", "fieldname": "error", "fieldtype": "Code", "label": "Error" @@ -152,7 +153,7 @@ "idx": 1, "in_create": 1, "links": [], - "modified": "2023-06-05 12:15:17.850292", + "modified": "2023-06-07 02:21:28.769620", "modified_by": "Administrator", "module": "Email", "name": "Email Queue", diff --git a/frappe/email/doctype/email_queue/email_queue.py b/frappe/email/doctype/email_queue/email_queue.py index d977f2097c..b465c32eea 100644 --- a/frappe/email/doctype/email_queue/email_queue.py +++ b/frappe/email/doctype/email_queue/email_queue.py @@ -140,7 +140,9 @@ class EmailQueue(Document): method(self, self.sender, recipient.recipient, message) else: if not frappe.flags.in_test: - ctx.smtp_session.sendmail(from_addr=self.sender, to_addrs=recipient.recipient, msg=message) + ctx.smtp_server.session.sendmail( + from_addr=self.sender, to_addrs=recipient.recipient, msg=message + ) ctx.add_to_sent_list(recipient) if frappe.flags.in_test: @@ -217,19 +219,24 @@ class SendMailContext: smtplib.SMTPHeloError, JobTimeoutException, ] + trace = "".join(traceback.format_tb(exc_tb)) if exc_tb else None if not self.retain_smtp_session: self.smtp_server.quit() - self.log_exception(exc_type, exc_val, exc_tb) - if exc_type in exceptions: - update_fields = {"status": "Partially Sent" if self.sent_to else "Not Sent"} + update_fields = {"status": "Partially Sent" if self.sent_to else "Not Sent", "error": trace} elif exc_type: + update_fields = {"error": trace} if self.queue_doc.retry < get_email_retry_limit(): - update_fields = {"status": "Not Sent", "retry": self.queue_doc.retry + 1} + update_fields.update( + { + "status": "Partially Sent" if self.sent_to else "Not Sent", + "retry": self.queue_doc.retry + 1, + } + ) else: - update_fields = {"status": "Error"} + update_fields.update({"status": "Error"}) else: update_fields = { "status": "Sent", @@ -240,19 +247,6 @@ class SendMailContext: self.queue_doc.update_status(**update_fields, commit=True) - def log_exception(self, exc_type, exc_val, exc_tb): - if exc_type: - traceback_string = "".join(traceback.format_tb(exc_tb)) - traceback_string += f"\n Queue Name: {self.queue_doc.name}" - - self.queue_doc.log_error("Email sending failed", traceback_string) - - @property - def smtp_session(self): - if frappe.flags.in_test: - return - return self.smtp_server.session - def add_to_sent_list(self, recipient): # Update recipient status recipient.update_db(status="Sent", commit=True) From aac74726ff56e9d730239ff203e0199bc81e692f Mon Sep 17 00:00:00 2001 From: phot0n Date: Wed, 7 Jun 2023 02:35:06 +0530 Subject: [PATCH 4/6] chore: add recipient to sent list regardless of email sending method --- frappe/email/doctype/email_queue/email_queue.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/frappe/email/doctype/email_queue/email_queue.py b/frappe/email/doctype/email_queue/email_queue.py index b465c32eea..ab63c3c0d6 100644 --- a/frappe/email/doctype/email_queue/email_queue.py +++ b/frappe/email/doctype/email_queue/email_queue.py @@ -143,7 +143,8 @@ class EmailQueue(Document): ctx.smtp_server.session.sendmail( from_addr=self.sender, to_addrs=recipient.recipient, msg=message ) - ctx.add_to_sent_list(recipient) + + ctx.add_to_sent_list(recipient) if frappe.flags.in_test: frappe.flags.sent_mail = message @@ -240,9 +241,6 @@ class SendMailContext: else: update_fields = { "status": "Sent", - "email_account": self.email_account_doc.name - if self.email_account_doc.is_exists_in_db() - else None, } self.queue_doc.update_status(**update_fields, commit=True) From 4ceafe14e31febeb1265fd767c2ace57e024d0fe Mon Sep 17 00:00:00 2001 From: phot0n Date: Thu, 8 Jun 2023 15:53:44 +0530 Subject: [PATCH 5/6] chore: remove unused is_background_task and add_to_sent_list -> update_recipient_status_to_sent * set status to be hidden in queue doc * don't maintain a list of sent to recipeint, a boolean is enough to set the status to partially sent --- .../doctype/email_queue/email_queue.json | 3 +- .../email/doctype/email_queue/email_queue.py | 37 +++++++++---------- frappe/email/queue.py | 1 - 3 files changed, 20 insertions(+), 21 deletions(-) diff --git a/frappe/email/doctype/email_queue/email_queue.json b/frappe/email/doctype/email_queue/email_queue.json index b1df78c564..15934ee8e7 100644 --- a/frappe/email/doctype/email_queue/email_queue.json +++ b/frappe/email/doctype/email_queue/email_queue.json @@ -55,6 +55,7 @@ "default": "Not Sent", "fieldname": "status", "fieldtype": "Select", + "hidden": 1, "in_list_view": 1, "in_standard_filter": 1, "label": "Status", @@ -153,7 +154,7 @@ "idx": 1, "in_create": 1, "links": [], - "modified": "2023-06-07 02:21:28.769620", + "modified": "2023-06-08 15:31:52.789186", "modified_by": "Administrator", "module": "Email", "name": "Email Queue", diff --git a/frappe/email/doctype/email_queue/email_queue.py b/frappe/email/doctype/email_queue/email_queue.py index ab63c3c0d6..06345f709e 100644 --- a/frappe/email/doctype/email_queue/email_queue.py +++ b/frappe/email/doctype/email_queue/email_queue.py @@ -123,20 +123,19 @@ class EmailQueue(Document): return True - def send(self, is_background_task: bool = False, smtp_server_instance: SMTPServer = None): + def send(self, smtp_server_instance: SMTPServer = None): """Send emails to recipients.""" if not self.can_send_now(): return - with SendMailContext(self, is_background_task, smtp_server_instance) as ctx: + with SendMailContext(self, smtp_server_instance) as ctx: message = None for recipient in self.recipients: - if not recipient.is_mail_to_be_sent(): + if recipient.is_mail_sent(): continue message = ctx.build_message(recipient.recipient) - method = get_hook_method("override_email_send") - if method: + if method := get_hook_method("override_email_send"): method(self, self.sender, recipient.recipient, message) else: if not frappe.flags.in_test: @@ -144,7 +143,7 @@ class EmailQueue(Document): from_addr=self.sender, to_addrs=recipient.recipient, msg=message ) - ctx.add_to_sent_list(recipient) + ctx.update_recipient_status_to_sent(recipient) if frappe.flags.in_test: frappe.flags.sent_mail = message @@ -180,24 +179,22 @@ class EmailQueue(Document): @task(queue="short") -def send_mail(email_queue_name, is_background_task=False, smtp_server_instance: SMTPServer = None): +def send_mail(email_queue_name, smtp_server_instance: SMTPServer = None): """This is equivalent to EmailQueue.send. This provides a way to make sending mail as a background job. """ record = EmailQueue.find(email_queue_name) - record.send(is_background_task=is_background_task, smtp_server_instance=smtp_server_instance) + record.send(smtp_server_instance=smtp_server_instance) class SendMailContext: def __init__( self, queue_doc: Document, - is_background_task: bool = False, smtp_server_instance: SMTPServer = None, ): self.queue_doc: EmailQueue = queue_doc - self.is_background_task = is_background_task self.email_account_doc = queue_doc.get_email_account() self.smtp_server = smtp_server_instance or self.email_account_doc.get_smtp_server() @@ -206,7 +203,9 @@ class SendMailContext: # Note: smtp session will have to be manually closed self.retain_smtp_session = bool(smtp_server_instance) - self.sent_to = [rec.recipient for rec in self.queue_doc.recipients if rec.is_mail_sent()] + self.sent_to_atleast_one_recipient = any( + rec.recipient for rec in self.queue_doc.recipients if rec.is_mail_sent() + ) def __enter__(self): self.queue_doc.update_status(status="Sending", commit=True) @@ -226,29 +225,29 @@ class SendMailContext: self.smtp_server.quit() if exc_type in exceptions: - update_fields = {"status": "Partially Sent" if self.sent_to else "Not Sent", "error": trace} + update_fields = { + "status": "Partially Sent" if self.sent_to_atleast_one_recipient else "Not Sent", + "error": trace, + } elif exc_type: update_fields = {"error": trace} if self.queue_doc.retry < get_email_retry_limit(): update_fields.update( { - "status": "Partially Sent" if self.sent_to else "Not Sent", + "status": "Partially Sent" if self.sent_to_atleast_one_recipient else "Not Sent", "retry": self.queue_doc.retry + 1, } ) else: update_fields.update({"status": "Error"}) else: - update_fields = { - "status": "Sent", - } + update_fields = {"status": "Sent"} self.queue_doc.update_status(**update_fields, commit=True) - def add_to_sent_list(self, recipient): - # Update recipient status + def update_recipient_status_to_sent(self, recipient): + self.sent_to_atleast_one_recipient = True recipient.update_db(status="Sent", commit=True) - self.sent_to.append(recipient.recipient) def get_message_object(self, message): return Parser(policy=SMTPUTF8).parsestr(message) diff --git a/frappe/email/queue.py b/frappe/email/queue.py index 7d4b92baf1..75bb46f00c 100755 --- a/frappe/email/queue.py +++ b/frappe/email/queue.py @@ -154,7 +154,6 @@ def flush(from_test=False): frappe.enqueue( method=send_mail, email_queue_name=row.name, - is_background_task=not from_test, now=from_test, job_name=job_name, queue="short", From 129fdc803a6d2d0c4121b598010a6028f312ab01 Mon Sep 17 00:00:00 2001 From: phot0n Date: Thu, 8 Jun 2023 16:20:10 +0530 Subject: [PATCH 6/6] chore: fix linter --- frappe/email/doctype/email_account/email_account.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 159e6d9583..3f6051ffc8 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -386,7 +386,7 @@ class EmailAccount(Document): "from_site_config": {"default": True}, "no_smtp_authentication": { "conf_names": ("disable_mail_smtp_authentication",), - "default": 0, + "default": 0, }, }