Merge pull request #16973 from ankush/old_email_queue_cleanup

fix: email queue cleanup in pure SQL

frappe/core/doctype/log_settings/log_settings.py

@@ -10,8 +10,13 @@ from frappe.query_builder.functions import Now
 
 
 class LogSettings(Document):
-	def clear_logs(self):
+	def clear_logs(self, commit=False):
 		self.clear_email_queue()
+		if commit:
+			# Since since deleting many logs can take significant amount of time, commit is required to relase locks.
+			# Error log table doesn't require commit - myisam
+			# activity logs are deleted last so background job finishes and commits.
+			frappe.db.commit()
 		self.clear_error_logs()
 		self.clear_activity_logs()
 
@@ -34,7 +39,7 @@ class LogSettings(Document):
 
 def run_log_clean_up():
 	doc = frappe.get_doc("Log Settings")
-	doc.clear_logs()
+	doc.clear_logs(commit=True)
 
 
 @frappe.whitelist()

frappe/email/doctype/email_queue/test_email_queue.py

@@ -1,10 +1,41 @@
 # -*- coding: utf-8 -*-
 # Copyright (c) 2015, Frappe Technologies and Contributors
 # License: MIT. See LICENSE
-import unittest
 
-# test_records = frappe.get_test_records('Email Queue')
+import frappe
+from frappe.email.queue import clear_outbox
+from frappe.tests.utils import FrappeTestCase
 
 
-class TestEmailQueue(unittest.TestCase):
-	pass
+class TestEmailQueue(FrappeTestCase):
+	def test_email_queue_deletion_based_on_modified_date(self):
+		old_record = frappe.get_doc(
+			{
+				"doctype": "Email Queue",
+				"sender": "Test <test@example.com>",
+				"show_as_cc": "",
+				"message": "Test message",
+				"status": "Sent",
+				"priority": 1,
+				"recipients": [
+					{
+						"recipient": "test_auth@test.com",
+					}
+				],
+			}
+		).insert()
+
+		old_record.modified = "2010-01-01 00:00:01"
+		old_record.recipients[0].modified = old_record.modified
+		old_record.db_update_all()
+
+		new_record = frappe.copy_doc(old_record)
+		new_record.insert()
+
+		clear_outbox()
+
+		self.assertFalse(frappe.db.exists("Email Queue", old_record.name))
+		self.assertFalse(frappe.db.exists("Email Queue Recipient", {"parent": old_record.name}))
+
+		self.assertTrue(frappe.db.exists("Email Queue", new_record.name))
+		self.assertTrue(frappe.db.exists("Email Queue Recipient", {"parent": new_record.name}))

frappe/email/queue.py

@@ -195,18 +195,24 @@ def clear_outbox(days: int = None) -> None:
 	Note: Used separate query to avoid deadlock
 	"""
 	days = days or 31
-	email_queue = DocType("Email Queue")
+	email_queue = frappe.qb.DocType("Email Queue")
+	email_recipient = frappe.qb.DocType("Email Queue Recipient")
 
-	email_queues = (
+	# Delete queue table
+	(
 		frappe.qb.from_(email_queue)
-		.select(email_queue.name)
-		.where(email_queue.modified < (Now() - Interval(days=days)))
-		.run(pluck=True)
-	)
+		.delete()
+		.where((email_queue.modified < (Now() - Interval(days=days))))
+	).run()
 
-	if email_queues:
-		frappe.db.delete("Email Queue", {"name": ("in", email_queues)})
-		frappe.db.delete("Email Queue Recipient", {"parent": ("in", email_queues)})
+	# delete child tables, note that this has potential to leave some orphan
+	# child table behind if modified time was later than parent doc (rare).
+	# But it's safe since child table doesn't contain links.
+	(
+		frappe.qb.from_(email_recipient)
+		.delete()
+		.where((email_recipient.modified < (Now() - Interval(days=days))))
+	).run()
 
 
 def set_expiry_for_email_queue():