From 1f8598891e2635b785471a5db8b95bd7f26c7e00 Mon Sep 17 00:00:00 2001 From: phot0n Date: Fri, 9 Jun 2023 14:38:55 +0530 Subject: [PATCH 1/2] fix: remove infinite sending retries for certain exceptions for email queue * chore: remove expired status & set_expiry job from email queue --- .../test_scheduled_job_type.py | 4 +-- .../doctype/email_queue/email_queue.json | 4 +-- .../email/doctype/email_queue/email_queue.py | 18 ++----------- frappe/email/queue.py | 16 ------------ frappe/hooks.py | 1 - frappe/tests/test_email.py | 25 ------------------- frappe/tests/test_scheduler.py | 2 +- 7 files changed, 7 insertions(+), 63 deletions(-) diff --git a/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py b/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py index bbc92dfbc9..7edad24ac4 100644 --- a/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py +++ b/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py @@ -18,7 +18,7 @@ class TestScheduledJobType(FrappeTestCase): self.assertEqual(all_job.frequency, "All") daily_job = frappe.get_doc( - "Scheduled Job Type", dict(method="frappe.email.queue.set_expiry_for_email_queue") + "Scheduled Job Type", dict(method="frappe.desk.notifications.clear_notifications") ) self.assertEqual(daily_job.frequency, "Daily") @@ -37,7 +37,7 @@ class TestScheduledJobType(FrappeTestCase): def test_daily_job(self): job = frappe.get_doc( - "Scheduled Job Type", dict(method="frappe.email.queue.set_expiry_for_email_queue") + "Scheduled Job Type", dict(method="frappe.desk.notifications.clear_notifications") ) job.db_set("last_execution", "2019-01-01 00:00:00") self.assertTrue(job.is_event_due(get_datetime("2019-01-02 00:00:06"))) diff --git a/frappe/email/doctype/email_queue/email_queue.json b/frappe/email/doctype/email_queue/email_queue.json index 15934ee8e7..ea1389d8e9 100644 --- a/frappe/email/doctype/email_queue/email_queue.json +++ b/frappe/email/doctype/email_queue/email_queue.json @@ -59,7 +59,7 @@ "in_list_view": 1, "in_standard_filter": 1, "label": "Status", - "options": "Not Sent\nSending\nSent\nPartially Sent\nError\nExpired" + "options": "Not Sent\nSending\nSent\nPartially Sent\nError" }, { "depends_on": "eval:doc.error", @@ -154,7 +154,7 @@ "idx": 1, "in_create": 1, "links": [], - "modified": "2023-06-08 15:31:52.789186", + "modified": "2023-06-09 14: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 06345f709e..29fd2bf8fa 100644 --- a/frappe/email/doctype/email_queue/email_queue.py +++ b/frappe/email/doctype/email_queue/email_queue.py @@ -212,25 +212,11 @@ class SendMailContext: return self def __exit__(self, exc_type, exc_val, exc_tb): - exceptions = [ - smtplib.SMTPServerDisconnected, - smtplib.SMTPAuthenticationError, - smtplib.SMTPConnectError, - 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() - if exc_type in exceptions: - 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 exc_type: + update_fields = {"error": "".join(traceback.format_tb(exc_tb))} if self.queue_doc.retry < get_email_retry_limit(): update_fields.update( { diff --git a/frappe/email/queue.py b/frappe/email/queue.py index 75bb46f00c..0df88ebd5c 100755 --- a/frappe/email/queue.py +++ b/frappe/email/queue.py @@ -179,19 +179,3 @@ def get_queue(): {"now": now_datetime()}, as_dict=True, ) - - -def set_expiry_for_email_queue(): - """Mark emails as expire that has not sent for 7 days. - Called daily via scheduler. - """ - - frappe.db.sql( - """ - UPDATE `tabEmail Queue` - SET `status`='Expired' - WHERE `modified` < (NOW() - INTERVAL '7' DAY) - AND `status`='Not Sent' - AND (`send_after` IS NULL OR `send_after` < %(now)s)""", - {"now": now_datetime()}, - ) diff --git a/frappe/hooks.py b/frappe/hooks.py index edf572b642..84081bcec1 100644 --- a/frappe/hooks.py +++ b/frappe/hooks.py @@ -223,7 +223,6 @@ scheduler_events = { "frappe.website.doctype.personal_data_deletion_request.personal_data_deletion_request.process_data_deletion_request", ], "daily": [ - "frappe.email.queue.set_expiry_for_email_queue", "frappe.desk.notifications.clear_notifications", "frappe.desk.doctype.event.event.send_event_digest", "frappe.sessions.clear_expired_sessions", diff --git a/frappe/tests/test_email.py b/frappe/tests/test_email.py index 84785b70f9..4cde7f9ace 100644 --- a/frappe/tests/test_email.py +++ b/frappe/tests/test_email.py @@ -205,31 +205,6 @@ class TestEmail(FrappeTestCase): self.assertTrue(verify_request()) break - def test_expired(self): - self.test_email_queue() - frappe.db.sql("UPDATE `tabEmail Queue` SET `modified`=(NOW() - INTERVAL '8' day)") - - from frappe.email.queue import set_expiry_for_email_queue - - set_expiry_for_email_queue() - - email_queue = frappe.db.sql( - """select name from `tabEmail Queue` where status='Expired'""", as_dict=1 - ) - self.assertEqual(len(email_queue), 1) - queue_recipients = [ - r.recipient - for r in frappe.db.sql( - """select recipient from `tabEmail Queue Recipient` - where parent = %s""", - email_queue[0].name, - as_dict=1, - ) - ] - self.assertTrue("test@example.com" in queue_recipients) - self.assertTrue("test1@example.com" in queue_recipients) - self.assertEqual(len(queue_recipients), 2) - def test_sender(self): def _patched_assertion(email_account, assertion): with patch.object(QueueBuilder, "get_outgoing_email_account", return_value=email_account): diff --git a/frappe/tests/test_scheduler.py b/frappe/tests/test_scheduler.py index 4e4b67c603..f66b7da5ed 100644 --- a/frappe/tests/test_scheduler.py +++ b/frappe/tests/test_scheduler.py @@ -42,7 +42,7 @@ class TestScheduler(TestCase): enqueued_jobs = enqueue_events(site=frappe.local.site) - self.assertIn("frappe.email.queue.set_expiry_for_email_queue", enqueued_jobs) + self.assertIn("frappe.desk.notifications.clear_notifications", enqueued_jobs) self.assertIn("frappe.utils.change_log.check_for_update", enqueued_jobs) self.assertIn( "frappe.email.doctype.auto_email_report.auto_email_report.send_monthly", From f49818089a33b7c3b31b4f1c92878b90dcc789b9 Mon Sep 17 00:00:00 2001 From: phot0n Date: Fri, 9 Jun 2023 15:38:18 +0530 Subject: [PATCH 2/2] feat(minor): bulk retry for email queue * chore: add retry_sending as a email queue class method --- .../email/doctype/email_queue/email_queue.js | 4 +- .../email/doctype/email_queue/email_queue.py | 38 +++++++++++++------ .../doctype/email_queue/email_queue_list.js | 23 ++++++++++- 3 files changed, 50 insertions(+), 15 deletions(-) diff --git a/frappe/email/doctype/email_queue/email_queue.js b/frappe/email/doctype/email_queue/email_queue.js index 7d05053d4e..b9a24342ba 100644 --- a/frappe/email/doctype/email_queue/email_queue.js +++ b/frappe/email/doctype/email_queue/email_queue.js @@ -17,13 +17,13 @@ frappe.ui.form.on("Email Queue", { }); }); } else if (frm.doc.status == "Error") { - let button = frm.add_custom_button("Retry Sending", function () { + frm.add_custom_button("Retry Sending", function () { frm.call({ method: "retry_sending", + doc: frm.doc, args: { name: frm.doc.name, }, - btn: button, callback: function () { frm.reload_doc(); }, diff --git a/frappe/email/doctype/email_queue/email_queue.py b/frappe/email/doctype/email_queue/email_queue.py index 29fd2bf8fa..895d8fbe01 100644 --- a/frappe/email/doctype/email_queue/email_queue.py +++ b/frappe/email/doctype/email_queue/email_queue.py @@ -3,14 +3,11 @@ import json import quopri -import smtplib import traceback from contextlib import suppress from email.parser import Parser from email.policy import SMTPUTF8 -from rq.timeouts import JobTimeoutException - import frappe from frappe import _, safe_encode, task from frappe.core.utils import html2text @@ -28,6 +25,7 @@ from frappe.utils import ( get_hook_method, get_string_between, get_url, + now, nowdate, sbool, split_emails, @@ -177,6 +175,12 @@ class EmailQueue(Document): .where(email_recipient.modified < (Now() - Interval(days=days))) ).run() + @frappe.whitelist() + def retry_sending(self): + if self.status == "Error": + self.status = "Not Sent" + self.save(ignore_permissions=True) + @task(queue="short") def send_mail(email_queue_name, smtp_server_instance: SMTPServer = None): @@ -345,16 +349,26 @@ class SendMailContext: @frappe.whitelist() -def retry_sending(name): - doc = frappe.get_doc("Email Queue", name) - doc.check_permission() +def bulk_retry(queues): + frappe.only_for("System Manager") - if doc and doc.status == "Error": - doc.status = "Not Sent" - for d in doc.recipients: - if d.status != "Sent": - d.status = "Not Sent" - doc.save(ignore_permissions=True) + if isinstance(queues, str): + queues = json.loads(queues) + + if not queues: + return + + frappe.msgprint( + _("Updating Email Queue Statuses. The emails will be picked up in the next scheduled run."), + _("Processing..."), + ) + + email_queue = frappe.qb.DocType("Email Queue") + frappe.qb.update(email_queue).set(email_queue.status, "Not Sent").set( + email_queue.modified, now() + ).set(email_queue.modified_by, frappe.session.user).where( + email_queue.name.isin(queues) & email_queue.status == "Error" + ).run() @frappe.whitelist() diff --git a/frappe/email/doctype/email_queue/email_queue_list.js b/frappe/email/doctype/email_queue/email_queue_list.js index b00503b6f8..ea6869e265 100644 --- a/frappe/email/doctype/email_queue/email_queue_list.js +++ b/frappe/email/doctype/email_queue/email_queue_list.js @@ -9,7 +9,10 @@ frappe.listview_settings["Email Queue"] = { }; return [__(doc.status), colour[doc.status], "status,=," + doc.status]; }, - refresh: show_toggle_sending_button, + refresh: function (listview) { + show_toggle_sending_button(listview); + add_bulk_retry_button_to_actions(listview); + }, onload: function (list_view) { frappe.require("logtypes.bundle.js", () => { frappe.utils.logtypes.show_log_retention_message(list_view.doctype); @@ -39,3 +42,21 @@ function show_toggle_sending_button(list_view) { show_toggle_sending_button(list_view); }); } + +function add_bulk_retry_button_to_actions(list_view) { + if (!has_common(frappe.user_roles, ["Administrator", "System Manager"])) return; + + list_view.page.add_actions_menu_item(__("Retry Sending"), () => { + frappe.call({ + method: "frappe.email.doctype.email_queue.email_queue.bulk_retry", + args: { + queues: list_view.get_checked_items(true), + }, + callback: (r) => { + if (!r.exc) { + list_view.refresh(); + } + }, + }); + }); +}