From ea416f9d6bd5312e48ca030ad915f01ad8434d40 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@frappe.io>
Date: Sat, 11 Jun 2022 12:26:09 +0530
Subject: [PATCH] feat: log settings with "interface"

We have hardcoded "Log settings" to only apply on 3 doctypes, there are
few more logging doctypes in core which are not cleared right now, on
top of that it's not easy for user to configure all logging behaviour
from one place.

This change adds a table on log settings where logging doctypes that
support the interface required by log settings can auto-register and
show up in settings.

Currently only supported configuration is "number of days" to keep.
---
 frappe/__init__.py                            |  12 +-
 .../core/doctype/activity_log/activity_log.py |  16 +--
 frappe/core/doctype/error_log/error_log.py    |   7 +
 .../core/doctype/log_settings/log_settings.js |  16 ++-
 .../doctype/log_settings/log_settings.json    |  56 ++------
 .../core/doctype/log_settings/log_settings.py | 122 ++++++++++++------
 .../doctype/log_settings/test_log_settings.py |  19 ++-
 frappe/core/doctype/logs_to_clear/__init__.py |   0
 .../doctype/logs_to_clear/logs_to_clear.json  |  42 ++++++
 .../doctype/logs_to_clear/logs_to_clear.py    |   9 ++
 .../email/doctype/email_queue/email_queue.py  |  28 +++-
 .../doctype/email_queue/test_email_queue.py   |   5 +-
 frappe/email/queue.py                         |  25 ----
 frappe/patches.txt                            |   1 +
 .../patches/v14_0/log_settings_migration.py   |  26 ++++
 15 files changed, 255 insertions(+), 129 deletions(-)
 create mode 100644 frappe/core/doctype/logs_to_clear/__init__.py
 create mode 100644 frappe/core/doctype/logs_to_clear/logs_to_clear.json
 create mode 100644 frappe/core/doctype/logs_to_clear/logs_to_clear.py
 create mode 100644 frappe/patches/v14_0/log_settings_migration.py

diff --git a/frappe/__init__.py b/frappe/__init__.py
index 054643903d..4b64732245 100644
--- a/frappe/__init__.py
+++ b/frappe/__init__.py
@@ -17,7 +17,7 @@ import json
 import os
 import re
 import warnings
-from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union
+from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional, Union
 
 import click
 from werkzeug.local import Local, release_local
@@ -1551,7 +1551,15 @@ def call(fn, *args, **kwargs):
 	return fn(*args, **newargs)
 
 
-def get_newargs(fn, kwargs):
+def get_newargs(fn: Callable, kwargs: Dict[str, Any]) -> Dict[str, Any]:
+	"""Remove any kwargs that are not supported by the function.
+
+	Example:
+	        >>> def fn(a=1, b=2): pass
+
+	        >>> get_newargs(fn, {"a": 2, "c": 1})
+	                {"a": 2}
+	"""
 
 	# if function has any **kwargs parameter that capture arbitrary keyword arguments
 	# Ref: https://docs.python.org/3/library/inspect.html#inspect.Parameter.kind
diff --git a/frappe/core/doctype/activity_log/activity_log.py b/frappe/core/doctype/activity_log/activity_log.py
index 61dedd7bc0..468b7f4473 100644
--- a/frappe/core/doctype/activity_log/activity_log.py
+++ b/frappe/core/doctype/activity_log/activity_log.py
@@ -25,6 +25,13 @@ class ActivityLog(Document):
 		if self.reference_doctype and self.reference_name:
 			self.status = "Linked"
 
+	@staticmethod
+	def clear_old_logs(days=None):
+		if not days:
+			days = 90
+		doctype = DocType("Activity Log")
+		frappe.db.delete(doctype, filters=(doctype.modified < (Now() - Interval(days=days))))
+
 
 def on_doctype_update():
 	"""Add indexes in `tabActivity Log`"""
@@ -43,12 +50,3 @@ def add_authentication_log(subject, user, operation="Login", status="Success"):
 			"operation": operation,
 		}
 	).insert(ignore_permissions=True, ignore_links=True)
-
-
-def clear_activity_logs(days=None):
-	"""clear 90 day old authentication logs or configured in log settings"""
-
-	if not days:
-		days = 90
-	doctype = DocType("Activity Log")
-	frappe.db.delete(doctype, filters=(doctype.creation < (Now() - Interval(days=days))))
diff --git a/frappe/core/doctype/error_log/error_log.py b/frappe/core/doctype/error_log/error_log.py
index 569e6d047e..57c519bd2e 100644
--- a/frappe/core/doctype/error_log/error_log.py
+++ b/frappe/core/doctype/error_log/error_log.py
@@ -4,6 +4,8 @@
 
 import frappe
 from frappe.model.document import Document
+from frappe.query_builder import Interval
+from frappe.query_builder.functions import Now
 
 
 class ErrorLog(Document):
@@ -12,6 +14,11 @@ class ErrorLog(Document):
 			self.db_set("seen", 1, update_modified=0)
 			frappe.db.commit()
 
+	@staticmethod
+	def clear_old_logs(days=30):
+		table = frappe.qb.DocType("Error Log")
+		frappe.db.delete(table, filters=(table.creation < (Now() - Interval(days=days))))
+
 
 @frappe.whitelist()
 def clear_error_logs():
diff --git a/frappe/core/doctype/log_settings/log_settings.js b/frappe/core/doctype/log_settings/log_settings.js
index 09a2086a1d..dc7cc7eac2 100644
--- a/frappe/core/doctype/log_settings/log_settings.js
+++ b/frappe/core/doctype/log_settings/log_settings.js
@@ -1,8 +1,16 @@
 // Copyright (c) 2020, Frappe Technologies and contributors
 // For license information, please see license.txt
 
-frappe.ui.form.on('Log Settings', {
-	// refresh: function(frm) {
-
-	// }
+frappe.ui.form.on("Log Settings", {
+	refresh: (frm) => {
+		frm.set_query("ref_doctype", "logs_to_clear", () => {
+			const added_doctypes = frm.doc.logs_to_clear.map((r) => r.ref_doctype);
+			return {
+				query: "frappe.core.doctype.log_settings.log_settings.get_log_doctypes",
+				filters: [
+					["name", "not in", added_doctypes],
+				],
+			};
+		});
+	},
 });
diff --git a/frappe/core/doctype/log_settings/log_settings.json b/frappe/core/doctype/log_settings/log_settings.json
index f06d14f16b..5a9dd159cc 100644
--- a/frappe/core/doctype/log_settings/log_settings.json
+++ b/frappe/core/doctype/log_settings/log_settings.json
@@ -5,61 +5,20 @@
  "editable_grid": 1,
  "engine": "InnoDB",
  "field_order": [
-  "error_log_notification_section",
-  "users_to_notify",
-  "log_cleanup_section",
-  "clear_error_log_after",
-  "clear_activity_log_after",
-  "column_break_4",
-  "clear_email_queue_after"
+  "logs_to_clear"
  ],
  "fields": [
   {
-   "fieldname": "log_cleanup_section",
-   "fieldtype": "Section Break",
-   "label": "Log Cleanup"
-  },
-  {
-   "fieldname": "column_break_4",
-   "fieldtype": "Column Break"
-  },
-  {
-   "fieldname": "error_log_notification_section",
-   "fieldtype": "Section Break",
-   "label": "Error Log Notification"
-  },
-  {
-   "fieldname": "users_to_notify",
-   "fieldtype": "Table MultiSelect",
-   "label": "Users To Notify",
-   "options": "Log Setting User"
-  },
-  {
-   "default": "90",
-   "description": "In Days",
-   "fieldname": "clear_error_log_after",
-   "fieldtype": "Int",
-   "label": "Clear Error log After"
-  },
-  {
-   "default": "90",
-   "description": "In Days",
-   "fieldname": "clear_activity_log_after",
-   "fieldtype": "Int",
-   "label": "Clear Activity Log After"
-  },
-  {
-   "default": "30",
-   "description": "In Days",
-   "fieldname": "clear_email_queue_after",
-   "fieldtype": "Int",
-   "label": "Clear Email Queue After"
+   "fieldname": "logs_to_clear",
+   "fieldtype": "Table",
+   "label": "Logs to Clear",
+   "options": "Logs To Clear"
   }
  ],
  "index_web_pages_for_search": 1,
  "issingle": 1,
  "links": [],
- "modified": "2020-10-13 12:18:48.649038",
+ "modified": "2022-06-11 02:17:30.803721",
  "modified_by": "Administrator",
  "module": "Core",
  "name": "Log Settings",
@@ -79,5 +38,6 @@
  "quick_entry": 1,
  "sort_field": "modified",
  "sort_order": "DESC",
+ "states": [],
  "track_changes": 1
-}
+}
\ No newline at end of file
diff --git a/frappe/core/doctype/log_settings/log_settings.py b/frappe/core/doctype/log_settings/log_settings.py
index 5632f05a36..4f8ea549b9 100644
--- a/frappe/core/doctype/log_settings/log_settings.py
+++ b/frappe/core/doctype/log_settings/log_settings.py
@@ -2,49 +2,88 @@
 # Copyright (c) 2020, Frappe Technologies and contributors
 # License: MIT. See LICENSE
 
+from typing import Protocol, runtime_checkable
+
 import frappe
 from frappe import _
+from frappe.model.base_document import get_controller
 from frappe.model.document import Document
-from frappe.query_builder import DocType, Interval
-from frappe.query_builder.functions import Now
+from frappe.utils import cint
+from frappe.utils.caching import site_cache
+
+
+@runtime_checkable
+class LogType(Protocol):
+	"""Interface requirement for doctypes that can be cleared using log settings."""
+
+	@staticmethod
+	def clear_old_logs(days: int) -> None:
+		...
+
+
+@site_cache
+def _supports_log_clearing(doctype: str) -> bool:
+	try:
+		controller = get_controller(doctype)
+		return issubclass(controller, LogType)
+	except Exception:
+		return False
 
 
 class LogSettings(Document):
-	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.
+	def validate(self):
+		self.validate_supported_doctypes()
+		self.validate_duplicates()
+
+	def validate_supported_doctypes(self):
+		for entry in self.logs_to_clear:
+			if _supports_log_clearing(entry.ref_doctype):
+				continue
+
+			msg = _("{} does not support automated log clearing.").format(frappe.bold(entry.ref_doctype))
+			if frappe.conf.developer_mode:
+				msg += "<br>" + _("Implement `clear_old_logs` method to enable auto error clearing.")
+			frappe.throw(msg, title=_("DocType not supported by Log Settings."))
+
+	def validate_duplicates(self):
+		seen = set()
+		for entry in self.logs_to_clear:
+			if entry.ref_doctype in seen:
+				frappe.throw(
+					_("{} appears more than once in configured log doctypes.").format(entry.ref_doctype)
+				)
+			seen.add(entry.ref_doctype)
+
+	def clear_logs(self):
+		"""
+		Log settings can clear any log type that's registered to it and provides a method to delete old logs.
+
+		Check `LogDoctype` above for interface that doctypes need to implement.
+		"""
+
+		for entry in self.logs_to_clear:
+			controller: LogType = get_controller(entry.ref_doctype)
+			func = controller.clear_old_logs
+
+			# Only pass what the method can handle, this is considering any
+			# future addition that might happen to the required interface.
+			kwargs = frappe.get_newargs(func, {"days": entry.days})
+			func(**kwargs)
 			frappe.db.commit()
-		self.clear_error_logs()
-		self.clear_activity_logs()
 
-	def clear_error_logs(self):
-		table = DocType("Error Log")
-		frappe.db.delete(
-			table, filters=(table.creation < (Now() - Interval(days=self.clear_error_log_after)))
-		)
-
-	def clear_activity_logs(self):
-		from frappe.core.doctype.activity_log.activity_log import clear_activity_logs
-
-		clear_activity_logs(days=self.clear_activity_log_after)
-
-	def clear_email_queue(self):
-		from frappe.email.queue import clear_outbox
-
-		clear_outbox(days=self.clear_email_queue_after)
+	def register_doctype(self, doctype: str, days=30):
+		if doctype not in {d.ref_doctype for d in self.logs_to_clear}:
+			self.append("logs_to_clear", {"ref_doctype": doctype, "days": cint(days)})
 
 
 def run_log_clean_up():
 	doc = frappe.get_doc("Log Settings")
-	doc.clear_logs(commit=True)
+	doc.clear_logs()
 
 
 @frappe.whitelist()
-def has_unseen_error_log(user):
-	def _get_response(show_alert=True):
+def has_unseen_error_log():
+	if frappe.get_all("Error Log", filters={"seen": 0}, limit=1):
 		return {
 			"show_alert": True,
 			"message": _("You have unseen {0}").format(
@@ -52,13 +91,22 @@ def has_unseen_error_log(user):
 			),
 		}
 
-	if frappe.get_all("Error Log", filters={"seen": 0}, limit=1):
-		log_settings = frappe.get_cached_doc("Log Settings")
 
-		if log_settings.users_to_notify:
-			if user in [u.user for u in log_settings.users_to_notify]:
-				return _get_response()
-			else:
-				return _get_response(show_alert=False)
-		else:
-			return _get_response()
+@frappe.whitelist()
+@frappe.validate_and_sanitize_search_inputs
+def get_log_doctypes(doctype, txt, searchfield, start, page_len, filters):
+
+	filters = filters or {}
+
+	filters.extend(
+		[
+			["istable", "=", 0],
+			["issingle", "=", 0],
+			["name", "like", f"%%{txt}%%"],
+		]
+	)
+	doctypes = frappe.get_list("DocType", filters=filters, pluck="name")
+
+	supported_doctypes = [(d,) for d in doctypes if _supports_log_clearing(d)]
+
+	return supported_doctypes[start:page_len]
diff --git a/frappe/core/doctype/log_settings/test_log_settings.py b/frappe/core/doctype/log_settings/test_log_settings.py
index 1b78745103..d7f43a181d 100644
--- a/frappe/core/doctype/log_settings/test_log_settings.py
+++ b/frappe/core/doctype/log_settings/test_log_settings.py
@@ -4,7 +4,7 @@
 from datetime import datetime
 
 import frappe
-from frappe.core.doctype.log_settings.log_settings import run_log_clean_up
+from frappe.core.doctype.log_settings.log_settings import _supports_log_clearing, run_log_clean_up
 from frappe.tests.utils import FrappeTestCase
 from frappe.utils import add_to_date, now_datetime
 
@@ -56,6 +56,23 @@ class TestLogSettings(FrappeTestCase):
 		self.assertEqual(error_log_count, 0)
 		self.assertEqual(email_queue_count, 0)
 
+	def test_logtype_identification(self):
+		supported_types = [
+			"Error Log",
+			"Activity Log",
+			"Email Queue",
+			"Route History",
+			"Error Snapshot",
+			"Scheduled Job Log",
+		]
+
+		for lt in supported_types:
+			self.assertTrue(_supports_log_clearing(lt), f"{lt} should be recognized as log type")
+
+		unsupported_types = ["DocType", "User", "Non Existing dt"]
+		for dt in unsupported_types:
+			self.assertFalse(_supports_log_clearing(dt), f"{dt} shouldn't be recognized as log type")
+
 
 def setup_test_logs(past: datetime) -> None:
 	activity_log = frappe.get_doc(
diff --git a/frappe/core/doctype/logs_to_clear/__init__.py b/frappe/core/doctype/logs_to_clear/__init__.py
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/frappe/core/doctype/logs_to_clear/logs_to_clear.json b/frappe/core/doctype/logs_to_clear/logs_to_clear.json
new file mode 100644
index 0000000000..212390adac
--- /dev/null
+++ b/frappe/core/doctype/logs_to_clear/logs_to_clear.json
@@ -0,0 +1,42 @@
+{
+ "actions": [],
+ "autoname": "autoincrement",
+ "creation": "2022-06-11 02:02:39.472511",
+ "doctype": "DocType",
+ "engine": "InnoDB",
+ "field_order": [
+  "ref_doctype",
+  "days"
+ ],
+ "fields": [
+  {
+   "fieldname": "ref_doctype",
+   "fieldtype": "Link",
+   "in_list_view": 1,
+   "label": "Log DocType",
+   "options": "DocType",
+   "reqd": 1
+  },
+  {
+   "fieldname": "days",
+   "fieldtype": "Int",
+   "in_list_view": 1,
+   "label": "Clear Logs After (days)",
+   "non_negative": 1,
+   "reqd": 1
+  }
+ ],
+ "index_web_pages_for_search": 1,
+ "istable": 1,
+ "links": [],
+ "modified": "2022-06-11 03:20:47.721188",
+ "modified_by": "Administrator",
+ "module": "Core",
+ "name": "Logs To Clear",
+ "naming_rule": "Autoincrement",
+ "owner": "Administrator",
+ "permissions": [],
+ "sort_field": "modified",
+ "sort_order": "DESC",
+ "states": []
+}
\ No newline at end of file
diff --git a/frappe/core/doctype/logs_to_clear/logs_to_clear.py b/frappe/core/doctype/logs_to_clear/logs_to_clear.py
new file mode 100644
index 0000000000..3fb4f8e72a
--- /dev/null
+++ b/frappe/core/doctype/logs_to_clear/logs_to_clear.py
@@ -0,0 +1,9 @@
+# Copyright (c) 2022, Frappe Technologies and contributors
+# For license information, please see license.txt
+
+# import frappe
+from frappe.model.document import Document
+
+
+class LogsToClear(Document):
+	pass
diff --git a/frappe/email/doctype/email_queue/email_queue.py b/frappe/email/doctype/email_queue/email_queue.py
index 662ba1b2ed..c3002607b4 100644
--- a/frappe/email/doctype/email_queue/email_queue.py
+++ b/frappe/email/doctype/email_queue/email_queue.py
@@ -18,7 +18,8 @@ from frappe.email.doctype.email_account.email_account import EmailAccount
 from frappe.email.email_body import add_attachment, get_email, get_formatted_html
 from frappe.email.queue import get_unsubcribed_url, get_unsubscribe_message
 from frappe.model.document import Document
-from frappe.query_builder.utils import DocType
+from frappe.query_builder import DocType, Interval
+from frappe.query_builder.functions import Now
 from frappe.utils import (
 	add_days,
 	cint,
@@ -144,6 +145,31 @@ class EmailQueue(Document):
 			if ctx.email_account_doc.append_emails_to_sent_folder and ctx.sent_to:
 				ctx.email_account_doc.append_email_to_sent_folder(message)
 
+	@staticmethod
+	def clear_old_logs(days=30):
+		"""Remove low priority older than 31 days in Outbox or configured in Log Settings.
+		Note: Used separate query to avoid deadlock
+		"""
+		days = days or 31
+		email_queue = frappe.qb.DocType("Email Queue")
+		email_recipient = frappe.qb.DocType("Email Queue Recipient")
+
+		# Delete queue table
+		(
+			frappe.qb.from_(email_queue)
+			.delete()
+			.where((email_queue.modified < (Now() - Interval(days=days))))
+		).run()
+
+		# 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()
+
 
 @task(queue="short")
 def send_mail(email_queue_name, is_background_task=False):
diff --git a/frappe/email/doctype/email_queue/test_email_queue.py b/frappe/email/doctype/email_queue/test_email_queue.py
index 96c566a041..435e4e691f 100644
--- a/frappe/email/doctype/email_queue/test_email_queue.py
+++ b/frappe/email/doctype/email_queue/test_email_queue.py
@@ -3,12 +3,13 @@
 # License: MIT. See LICENSE
 
 import frappe
-from frappe.email.queue import clear_outbox
 from frappe.tests.utils import FrappeTestCase
 
 
 class TestEmailQueue(FrappeTestCase):
 	def test_email_queue_deletion_based_on_modified_date(self):
+		from frappe.email.doctype.email_queue.email_queue import EmailQueue
+
 		old_record = frappe.get_doc(
 			{
 				"doctype": "Email Queue",
@@ -32,7 +33,7 @@ class TestEmailQueue(FrappeTestCase):
 		new_record = frappe.copy_doc(old_record)
 		new_record.insert()
 
-		clear_outbox()
+		EmailQueue.clear_old_logs()
 
 		self.assertFalse(frappe.db.exists("Email Queue", old_record.name))
 		self.assertFalse(frappe.db.exists("Email Queue Recipient", {"parent": old_record.name}))
diff --git a/frappe/email/queue.py b/frappe/email/queue.py
index 07731417d8..1519c26841 100755
--- a/frappe/email/queue.py
+++ b/frappe/email/queue.py
@@ -190,31 +190,6 @@ def get_queue():
 	)
 
 
-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
-	"""
-	days = days or 31
-	email_queue = frappe.qb.DocType("Email Queue")
-	email_recipient = frappe.qb.DocType("Email Queue Recipient")
-
-	# Delete queue table
-	(
-		frappe.qb.from_(email_queue)
-		.delete()
-		.where((email_queue.modified < (Now() - Interval(days=days))))
-	).run()
-
-	# 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():
 	"""Mark emails as expire that has not sent for 7 days.
 	Called daily via scheduler.
diff --git a/frappe/patches.txt b/frappe/patches.txt
index b771485c3c..853f02ff4c 100644
--- a/frappe/patches.txt
+++ b/frappe/patches.txt
@@ -191,6 +191,7 @@ frappe.patches.v14_0.remove_post_and_post_comment
 frappe.patches.v14_0.reset_creation_datetime
 frappe.patches.v14_0.remove_is_first_startup
 frappe.patches.v14_0.reload_workspace_child_tables
+frappe.patches.v14_0.log_settings_migration
 
 [post_model_sync]
 frappe.patches.v14_0.drop_data_import_legacy
diff --git a/frappe/patches/v14_0/log_settings_migration.py b/frappe/patches/v14_0/log_settings_migration.py
new file mode 100644
index 0000000000..87f6f6e082
--- /dev/null
+++ b/frappe/patches/v14_0/log_settings_migration.py
@@ -0,0 +1,26 @@
+import frappe
+
+
+def execute():
+	logging_doctypes = {
+		"Error Log": get_current_setting("clear_error_log_after") or 30,
+		"Activity Log": get_current_setting("clear_activity_log_after") or 90,
+		"Email Queue": get_current_setting("clear_email_queue_after") or 30,
+	}
+
+	frappe.reload_doc("core", "doctype", "Logs To Clear")
+	frappe.reload_doc("core", "doctype", "Log Settings")
+
+	log_settings = frappe.get_doc("Log Settings")
+
+	for doctype, days in logging_doctypes.items():
+		log_settings.register_doctype(doctype, days)
+
+	log_settings.save()
+
+
+def get_current_setting(fieldname):
+	try:
+		return frappe.db.get_single_value("Log Settings", fieldname)
+	except Exception:
+		pass