Merge pull request #16118 from shadrak98/log-settings

fix: clean up logs job was broken

frappe/core/doctype/activity_log/activity_log.py

@@ -1,15 +1,14 @@
-# -*- coding: utf-8 -*-
-# Copyright (c) 2017, Frappe Technologies and contributors
+# Copyright (c) 2022, Frappe Technologies and contributors
 # License: MIT. See LICENSE
 
+import frappe
 from frappe import _
-from frappe.utils import get_fullname, now
-from frappe.model.document import Document
 from frappe.core.utils import set_timeline_doc
-import frappe
+from frappe.model.document import Document
 from frappe.query_builder import DocType, Interval
 from frappe.query_builder.functions import Now
-from pypika.terms import PseudoColumn
+from frappe.utils import get_fullname, now
+
 
 class ActivityLog(Document):
 	def before_insert(self):
@@ -49,5 +48,5 @@ def clear_activity_logs(days=None):
 		days = 90
 	doctype = DocType("Activity Log")
 	frappe.db.delete(doctype, filters=(
-		doctype.creation < PseudoColumn(f"({Now() - Interval(days=days)})")
-	))
+		doctype.creation < (Now() - Interval(days=days))
+	))

frappe/core/doctype/log_settings/log_settings.py

@@ -7,7 +7,6 @@ from frappe import _
 from frappe.model.document import Document
 from frappe.query_builder import DocType, Interval
 from frappe.query_builder.functions import Now
-from pypika.terms import PseudoColumn
 
 
 class LogSettings(Document):
@@ -19,7 +18,7 @@ class LogSettings(Document):
 	def clear_error_logs(self):
 		table = DocType("Error Log")
 		frappe.db.delete(table, filters=(
-			table.creation < PseudoColumn(f"({Now() - Interval(days=self.clear_error_log_after)})")
+			table.creation < (Now() - Interval(days=self.clear_error_log_after))
 		))
 
 	def clear_activity_logs(self):

frappe/core/doctype/log_settings/test_log_settings.py

@@ -1,8 +1,106 @@
-# -*- coding: utf-8 -*-
-# Copyright (c) 2020, Frappe Technologies and Contributors
+# Copyright (c) 2022, Frappe Technologies and Contributors
 # License: MIT. See LICENSE
-# import frappe
+
+from datetime import datetime
 import unittest
 
+import frappe
+from frappe.utils import now_datetime, add_to_date
+from frappe.core.doctype.log_settings.log_settings import run_log_clean_up
+
+
 class TestLogSettings(unittest.TestCase):
-	pass
+	@classmethod
+	def setUpClass(cls):
+
+		cls.savepoint = "TestLogSettings"
+		frappe.db.savepoint(cls.savepoint)
+
+		frappe.db.set_single_value(
+			"Log Settings",
+			{
+				"clear_error_log_after": 1,
+				"clear_activity_log_after": 1,
+				"clear_email_queue_after": 1,
+			},
+		)
+
+	@classmethod
+	def tearDownClass(cls):
+		frappe.db.rollback(save_point=cls.savepoint)
+
+	def setUp(self) -> None:
+		if self._testMethodName == "test_delete_logs":
+			self.datetime = frappe._dict()
+			self.datetime.current = now_datetime()
+			self.datetime.past = add_to_date(self.datetime.current, days=-4)
+			setup_test_logs(self.datetime.past)
+
+	def tearDown(self) -> None:
+		if self._testMethodName == "test_delete_logs":
+			del self.datetime
+
+	def test_delete_logs(self):
+		# make sure test data is present
+		activity_log_count = frappe.db.count(
+			"Activity Log", {"creation": ("<=", self.datetime.past)}
+		)
+		error_log_count = frappe.db.count(
+			"Error Log", {"creation": ("<=", self.datetime.past)}
+		)
+		email_queue_count = frappe.db.count(
+			"Email Queue", {"creation": ("<=", self.datetime.past)}
+		)
+
+		self.assertNotEqual(activity_log_count, 0)
+		self.assertNotEqual(error_log_count, 0)
+		self.assertNotEqual(email_queue_count, 0)
+
+		# run clean up job
+		run_log_clean_up()
+
+		# test if logs are deleted
+		activity_log_count = frappe.db.count(
+			"Activity Log", {"creation": ("<", self.datetime.past)}
+		)
+		error_log_count = frappe.db.count(
+			"Error Log", {"creation": ("<", self.datetime.past)}
+		)
+		email_queue_count = frappe.db.count(
+			"Email Queue", {"creation": ("<", self.datetime.past)}
+		)
+
+		self.assertEqual(activity_log_count, 0)
+		self.assertEqual(error_log_count, 0)
+		self.assertEqual(email_queue_count, 0)
+
+
+def setup_test_logs(past: datetime) -> None:
+	activity_log = frappe.get_doc(
+		{
+			"doctype": "Activity Log",
+			"subject": "Test subject",
+			"full_name": "test user2",
+		}
+	).insert(ignore_permissions=True)
+	activity_log.db_set("creation", past)
+
+	error_log = frappe.get_doc(
+		{
+			"doctype": "Error Log",
+			"method": "test_method",
+			"error": "traceback",
+		}
+	).insert(ignore_permissions=True)
+	error_log.db_set("creation", past)
+
+	doc1 = frappe.get_doc(
+		{
+			"doctype": "Email Queue",
+			"sender": "test1@example.com",
+			"message": "This is a test email1",
+			"priority": 1,
+			"expose_recipients": "test@receiver.com",
+		}
+	).insert(ignore_permissions=True)
+	doc1.db_set("creation", past)

frappe/database/database.py

@@ -10,7 +10,7 @@ import re
 import string
 from contextlib import contextmanager
 from time import time
-from typing import Dict, List, Tuple, Union
+from typing import Dict, List, Optional, Tuple, Union
 
 from pypika.terms import Criterion, NullValue, PseudoColumn
 
@@ -561,7 +561,7 @@ class Database(object):
 	def get_list(*args, **kwargs):
 		return frappe.get_list(*args, **kwargs)
 
-	def set_single_value(self, doctype, fieldname, value, *args, **kwargs):
+	def set_single_value(self, doctype: str, fieldname: Union[str, Dict], value: Optional[Union[str, int]] = None, *args, **kwargs):
 		"""Set field value of Single DocType.
 
 		:param doctype: DocType of the single object

frappe/email/queue.py

@@ -1,10 +1,12 @@
-# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
+# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors
 # License: MIT. See LICENSE
 
 import frappe
 from frappe import msgprint, _
 from frappe.utils.verified_command import get_signed_params, verify_request
 from frappe.utils import get_url, now_datetime, cint
+from frappe.query_builder import DocType, Interval
+from frappe.query_builder.functions import Now
 
 def get_emails_sent_this_month(email_account=None):
 	"""Get count of emails sent from a specific email account.
@@ -162,15 +164,16 @@ def get_queue():
 			by priority desc, creation asc
 		limit 500''', { 'now': now_datetime() }, as_dict=True)
 
-def clear_outbox(days=None):
+def clear_outbox(days: int = None) -> None:
 	"""Remove low priority older than 31 days in Outbox or configured in Log Settings.
 	Note: Used separate query to avoid deadlock
 	"""
-	if not days:
-		days=31
+	days = days or 31
+	email_queue = DocType("Email Queue")
 
-	email_queues = frappe.db.sql_list("""SELECT `name` FROM `tabEmail Queue`
-		WHERE `priority`=0 AND `modified` < (NOW() - INTERVAL '{0}' DAY)""".format(days))
+	email_queues = frappe.qb.from_(email_queue).select(email_queue.name).where(
+		email_queue.modified < (Now() - Interval(days=days))
+	).run(pluck=True)
 
 	if email_queues:
 		frappe.db.delete("Email Queue", {"name": ("in", email_queues)})