From d6137a805c3c2f023e1eaaf3cfa24a90f7bd19c3 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 10 Mar 2024 11:39:40 +0530 Subject: [PATCH 1/6] fix: better default virtual doctype template --- frappe/modules/utils.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/frappe/modules/utils.py b/frappe/modules/utils.py index 65e9effe7c..a3a5bfc719 100644 --- a/frappe/modules/utils.py +++ b/frappe/modules/utils.py @@ -295,13 +295,16 @@ def make_boilerplate( dedent( """ def db_insert(self, *args, **kwargs): - pass + raise NotImplementedError def load_from_db(self): - pass + raise NotImplementedError def db_update(self): - pass + raise NotImplementedError + + def delete(self): + raise NotImplementedError @staticmethod def get_list(args): From a28921750d0d8210d4de067312402f452cc98ee7 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 10 Mar 2024 11:48:06 +0530 Subject: [PATCH 2/6] fix(DX): Avoid use of args in virtual doctype --- frappe/core/doctype/rq_worker/rq_worker.py | 5 +---- frappe/desk/reportview.py | 10 +++++++--- frappe/model/db_query.py | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/frappe/core/doctype/rq_worker/rq_worker.py b/frappe/core/doctype/rq_worker/rq_worker.py index 64706d3394..47cdc7c088 100644 --- a/frappe/core/doctype/rq_worker/rq_worker.py +++ b/frappe/core/doctype/rq_worker/rq_worker.py @@ -46,10 +46,7 @@ class RQWorker(Document): super(Document, self).__init__(d) @staticmethod - def get_list(args): - start = cint(args.get("start")) - page_length = cint(args.get("page_length")) or 20 - + def get_list(start=0, page_length=20): workers = get_workers() valid_workers = [w for w in workers if w.pid][start : start + page_length] diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 0e394f534e..95405fdb6f 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -13,7 +13,7 @@ from frappe.model import child_table_fields, default_fields, get_permitted_field from frappe.model.base_document import get_controller from frappe.model.db_query import DatabaseQuery from frappe.model.utils import is_virtual_doctype -from frappe.utils import add_user_info, format_duration +from frappe.utils import add_user_info, cint, format_duration @frappe.whitelist() @@ -23,7 +23,7 @@ def get(): # If virtual doctype, get data from controller get_list method if is_virtual_doctype(args.doctype): controller = get_controller(args.doctype) - data = compress(controller.get_list(args)) + data = compress(frappe.call(controller.get_list, args=args, **args)) else: data = compress(execute(**args), args=args) return data @@ -36,7 +36,7 @@ def get_list(): if is_virtual_doctype(args.doctype): controller = get_controller(args.doctype) - data = controller.get_list(args) + data = frappe.call(controller.get_list, args=args, **args) else: # uncompressed (refactored from frappe.model.db_query.get_list) data = execute(**args) @@ -227,6 +227,10 @@ def parse_json(data): data["save_user_settings"] = json.loads(data["save_user_settings"]) else: data["save_user_settings"] = True + if isinstance(data.get("start"), str): + data["start"] = cint(data.get("start")) + if isinstance(data.get("page_length"), str): + data["page_length"] = cint(data.get("page_length")) def get_parenttype_and_fieldname(field, data): diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index da4dd4f112..6107431ba0 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -180,7 +180,7 @@ class DatabaseQuery: "pluck": pluck, "parent_doctype": parent_doctype, } | self.__dict__ - return controller.get_list(kwargs) + return frappe.call(controller.get_list, args=kwargs, **kwargs) self.columns = self.get_table_columns() From b8c4eff68a75d549aadfc21ae66cb9c955a55152 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 11 Mar 2024 17:26:59 +0530 Subject: [PATCH 3/6] fix: Skip child table management for children of virt doctypes --- frappe/model/document.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index e000923a10..c0848b9f6b 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -300,8 +300,9 @@ class Document(BaseDocument): self.db_insert(ignore_if_duplicate=ignore_if_duplicate) # children - for d in self.get_all_children(): - d.db_insert() + if not getattr(self.meta, "is_virtual", False): + for d in self.get_all_children(): + d.db_insert() self.run_method("after_insert") self.flags.in_insert = True @@ -415,6 +416,9 @@ class Document(BaseDocument): def update_children(self): """update child tables""" + if getattr(self.meta, "is_virtual", False): + # Virtual doctypes manage their own children + return for df in self.meta.get_table_fields(): self.update_child_table(df.fieldname, df) From 4ab2ccd01fb1ef028111393df62c86b8188841cf Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 11 Mar 2024 15:28:56 +0530 Subject: [PATCH 4/6] fix!: get_count accepts splatted arguments --- frappe/core/doctype/rq_job/rq_job.py | 20 ++++++++----------- frappe/core/doctype/rq_job/test_rq_job.py | 6 +++--- frappe/core/doctype/rq_worker/rq_worker.py | 4 ++-- .../core/doctype/rq_worker/test_rq_worker.py | 4 ++-- frappe/desk/reportview.py | 4 ++-- frappe/tests/test_db_query.py | 18 ++++------------- 6 files changed, 21 insertions(+), 35 deletions(-) diff --git a/frappe/core/doctype/rq_job/rq_job.py b/frappe/core/doctype/rq_job/rq_job.py index 995ceda4c7..93ca331443 100644 --- a/frappe/core/doctype/rq_job/rq_job.py +++ b/frappe/core/doctype/rq_job/rq_job.py @@ -76,22 +76,18 @@ class RQJob(Document): return self._job_obj @staticmethod - def get_list(args): - start = cint(args.get("start")) - page_length = cint(args.get("page_length")) or 20 - - order_desc = "desc" in args.get("order_by", "") - - matched_job_ids = RQJob.get_matching_job_ids(args)[start : start + page_length] + def get_list(filters=None, start=0, page_length=20, order_by="modified desc"): + matched_job_ids = RQJob.get_matching_job_ids(filters=filters)[start : start + page_length] conn = get_redis_conn() jobs = [serialize_job(job) for job in Job.fetch_many(job_ids=matched_job_ids, connection=conn) if job] + order_desc = "desc" in order_by return sorted(jobs, key=lambda j: j.modified, reverse=order_desc) @staticmethod - def get_matching_job_ids(args) -> list[str]: - filters = make_filter_dict(args.get("filters")) + def get_matching_job_ids(filters) -> list[str]: + filters = make_filter_dict(filters or []) queues = _eval_filters(filters.get("queue"), QUEUES) statuses = _eval_filters(filters.get("status"), JOB_STATUSES) @@ -117,12 +113,12 @@ class RQJob(Document): frappe.msgprint(_("Job is not running."), title=_("Invalid Operation")) @staticmethod - def get_count(args) -> int: - return len(RQJob.get_matching_job_ids(args)) + def get_count(filters=None) -> int: + return len(RQJob.get_matching_job_ids(filters)) # None of these methods apply to virtual job doctype, overriden for sanity. @staticmethod - def get_stats(args): + def get_stats(): return {} def db_insert(self, *args, **kwargs): diff --git a/frappe/core/doctype/rq_job/test_rq_job.py b/frappe/core/doctype/rq_job/test_rq_job.py index 57c857f7ab..938187142f 100644 --- a/frappe/core/doctype/rq_job/test_rq_job.py +++ b/frappe/core/doctype/rq_job/test_rq_job.py @@ -61,18 +61,18 @@ class TestRQJob(FrappeTestCase): def test_get_list_filtering(self): # Check failed job clearning and filtering remove_failed_jobs() - jobs = RQJob.get_list({"filters": [["RQ Job", "status", "=", "failed"]]}) + jobs = frappe.get_all("RQ Job", {"status": "failed"}) self.assertEqual(jobs, []) # Fail a job job = frappe.enqueue(method=self.BG_JOB, queue="short", fail=True) self.check_status(job, "failed") - jobs = RQJob.get_list({"filters": [["RQ Job", "status", "=", "failed"]]}) + jobs = frappe.get_all("RQ Job", {"status": "failed"}) self.assertEqual(len(jobs), 1) self.assertTrue(jobs[0].exc_info) # Assert that non-failed job still exists - non_failed_jobs = RQJob.get_list({"filters": [["RQ Job", "status", "!=", "failed"]]}) + non_failed_jobs = frappe.get_all("RQ Job", {"status": ("!=", "failed")}) self.assertGreaterEqual(len(non_failed_jobs), 1) # Create a slow job and check if it's stuck in "Started" diff --git a/frappe/core/doctype/rq_worker/rq_worker.py b/frappe/core/doctype/rq_worker/rq_worker.py index 47cdc7c088..2d5bc996e1 100644 --- a/frappe/core/doctype/rq_worker/rq_worker.py +++ b/frappe/core/doctype/rq_worker/rq_worker.py @@ -53,12 +53,12 @@ class RQWorker(Document): return [serialize_worker(worker) for worker in valid_workers] @staticmethod - def get_count(args) -> int: + def get_count() -> int: return len(get_workers()) # None of these methods apply to virtual workers, overriden for sanity. @staticmethod - def get_stats(args): + def get_stats(): return {} def db_insert(self, *args, **kwargs): diff --git a/frappe/core/doctype/rq_worker/test_rq_worker.py b/frappe/core/doctype/rq_worker/test_rq_worker.py index 5803b1a875..14c2b29de7 100644 --- a/frappe/core/doctype/rq_worker/test_rq_worker.py +++ b/frappe/core/doctype/rq_worker/test_rq_worker.py @@ -8,10 +8,10 @@ from frappe.tests.utils import FrappeTestCase class TestRQWorker(FrappeTestCase): def test_get_worker_list(self): - workers = RQWorker.get_list({}) + workers = RQWorker.get_list() self.assertGreaterEqual(len(workers), 1) self.assertTrue(any("short" in w.queue_type for w in workers)) def test_worker_serialization(self): - workers = RQWorker.get_list({}) + workers = RQWorker.get_list() frappe.get_doc("RQ Worker", workers[0].name) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 95405fdb6f..598648cc4c 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -51,7 +51,7 @@ def get_count() -> int: if is_virtual_doctype(args.doctype): controller = get_controller(args.doctype) - data = controller.get_count(args) + data = frappe.call(controller.get_count, args=args, **args) else: distinct = "distinct " if args.distinct == "true" else "" args.fields = [f"count({distinct}`tab{args.doctype}`.name) as total_count"] @@ -513,7 +513,7 @@ def get_sidebar_stats(stats, doctype, filters=None): if is_virtual_doctype(doctype): controller = get_controller(doctype) args = {"stats": stats, "filters": filters} - data = controller.get_stats(args) + data = frappe.call(controller.get_stats, args=args, **args) else: data = get_stats(stats, doctype, filters) diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 25744079a1..4c0776fe7e 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -1075,27 +1075,17 @@ class TestDBQuery(FrappeTestCase): class VirtualDocType: @staticmethod - def get_list(args): - ... + def get_list(args=None, limit_page_length=0, doctype=None): + self.assertEqual(args["filters"], [["Virtual DocType", "name", "=", "test"]]) + self.assertEqual(limit_page_length, 1) + self.assertEqual(doctype, "Virtual DocType") with patch( "frappe.controllers", new={frappe.local.site: {"Virtual DocType": VirtualDocType}}, ): - VirtualDocType.get_list = MagicMock() - frappe.get_all("Virtual DocType", filters={"name": "test"}, fields=["name"], limit=1) - call_args = VirtualDocType.get_list.call_args[0][0] - VirtualDocType.get_list.assert_called_once() - self.assertIsInstance(call_args, dict) - self.assertEqual(call_args["doctype"], "Virtual DocType") - self.assertEqual(call_args["filters"], [["Virtual DocType", "name", "=", "test"]]) - self.assertEqual(call_args["fields"], ["name"]) - self.assertEqual(call_args["limit_page_length"], 1) - self.assertEqual(call_args["limit_start"], 0) - self.assertEqual(call_args["order_by"], DefaultOrderBy) - def test_coalesce_with_in_ops(self): self.assertNotIn("ifnull", frappe.get_all("User", {"first_name": ("in", ["a", "b"])}, run=0)) self.assertIn("ifnull", frappe.get_all("User", {"first_name": ("in", ["a", None])}, run=0)) From 1f35886995f337912967a6adb4ce207b6122de9b Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 11 Mar 2024 17:55:57 +0530 Subject: [PATCH 5/6] test: flake from expecting a succesful job --- frappe/core/doctype/rq_job/test_rq_job.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frappe/core/doctype/rq_job/test_rq_job.py b/frappe/core/doctype/rq_job/test_rq_job.py index 938187142f..6772fe186a 100644 --- a/frappe/core/doctype/rq_job/test_rq_job.py +++ b/frappe/core/doctype/rq_job/test_rq_job.py @@ -64,6 +64,10 @@ class TestRQJob(FrappeTestCase): jobs = frappe.get_all("RQ Job", {"status": "failed"}) self.assertEqual(jobs, []) + # Pass a job + job = frappe.enqueue(method=self.BG_JOB, queue="short") + self.check_status(job, "finished") + # Fail a job job = frappe.enqueue(method=self.BG_JOB, queue="short", fail=True) self.check_status(job, "failed") @@ -174,7 +178,7 @@ class TestRQJob(FrappeTestCase): jobs = [frappe.enqueue(method=self.BG_JOB, queue="short", fail=True) for _ in range(limit * 2)] self.check_status(jobs[-1], "failed") - self.assertLessEqual(RQJob.get_count({"filters": [["RQ Job", "status", "=", "failed"]]}), limit * 1.1) + self.assertLessEqual(RQJob.get_count(filters=[["RQ Job", "status", "=", "failed"]]), limit * 1.1) def test_func(fail=False, sleep=0): From f7c0dd66fd711250015abca13979faf927ccdca7 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 11 Mar 2024 18:00:05 +0530 Subject: [PATCH 6/6] refactor: migrate virtual doctypes to new API --- .../permission_inspector/permission_inspector.py | 6 +++--- frappe/core/doctype/recorder/recorder.py | 15 ++++++--------- .../core/doctype/recorder_query/recorder_query.py | 6 +++--- frappe/model/virtual_doctype.py | 6 +++--- frappe/modules/utils.py | 6 +++--- frappe/tests/test_db_query.py | 2 ++ frappe/tests/test_virtual_doctype.py | 13 ++++++------- 7 files changed, 26 insertions(+), 28 deletions(-) diff --git a/frappe/core/doctype/permission_inspector/permission_inspector.py b/frappe/core/doctype/permission_inspector/permission_inspector.py index 4197f3f4c9..c85df1648b 100644 --- a/frappe/core/doctype/permission_inspector/permission_inspector.py +++ b/frappe/core/doctype/permission_inspector/permission_inspector.py @@ -60,15 +60,15 @@ class PermissionInspector(Document): ... @staticmethod - def get_list(args): + def get_list(): ... @staticmethod - def get_count(args): + def get_count(): ... @staticmethod - def get_stats(args): + def get_stats(): ... def delete(self): diff --git a/frappe/core/doctype/recorder/recorder.py b/frappe/core/doctype/recorder/recorder.py index 26ccfcf378..ddbf208ad2 100644 --- a/frappe/core/doctype/recorder/recorder.py +++ b/frappe/core/doctype/recorder/recorder.py @@ -39,12 +39,10 @@ class Recorder(Document): super(Document, self).__init__(request) @staticmethod - def get_list(args): - start = cint(args.get("start")) - page_length = cint(args.get("page_length")) or 20 - requests = Recorder.get_filtered_requests(args)[start : start + page_length] + def get_list(filters=None, start=0, page_length=20, order_by="duration desc"): + requests = Recorder.get_filtered_requests(filters)[start : start + page_length] - if order_by_statment := args.get("order_by"): + if order_by_statment := order_by: if "." in order_by_statment: order_by_statment = order_by_statment.split(".")[1] @@ -60,12 +58,11 @@ class Recorder(Document): return sorted(requests, key=lambda r: r.duration, reverse=1) @staticmethod - def get_count(args): - return len(Recorder.get_filtered_requests(args)) + def get_count(filters=None): + return len(Recorder.get_filtered_requests(filters)) @staticmethod - def get_filtered_requests(args): - filters = args.get("filters") + def get_filtered_requests(filters): requests = [serialize_request(request) for request in get_recorder_data()] return [req for req in requests if evaluate_filters(req, filters)] diff --git a/frappe/core/doctype/recorder_query/recorder_query.py b/frappe/core/doctype/recorder_query/recorder_query.py index c797769dda..b3ebc66b98 100644 --- a/frappe/core/doctype/recorder_query/recorder_query.py +++ b/frappe/core/doctype/recorder_query/recorder_query.py @@ -39,15 +39,15 @@ class RecorderQuery(Document): pass @staticmethod - def get_list(args): + def get_list(): pass @staticmethod - def get_count(args): + def get_count(): pass @staticmethod - def get_stats(args): + def get_stats(): pass def delete(self): diff --git a/frappe/model/virtual_doctype.py b/frappe/model/virtual_doctype.py index 6390e35cef..c5a948a7c4 100644 --- a/frappe/model/virtual_doctype.py +++ b/frappe/model/virtual_doctype.py @@ -21,17 +21,17 @@ class VirtualDoctype(Protocol): # ============ class/static methods ============ @staticmethod - def get_list(args) -> list[frappe._dict]: + def get_list(**kwargs) -> list[frappe._dict]: """Similar to reportview.get_list""" ... @staticmethod - def get_count(args) -> int: + def get_count(**kwargs) -> int: """Similar to reportview.get_count, return total count of documents on listview.""" ... @staticmethod - def get_stats(args): + def get_stats(**kwargs): """Similar to reportview.get_stats, return sidebar stats.""" ... diff --git a/frappe/modules/utils.py b/frappe/modules/utils.py index a3a5bfc719..dda1b83732 100644 --- a/frappe/modules/utils.py +++ b/frappe/modules/utils.py @@ -307,15 +307,15 @@ def make_boilerplate( raise NotImplementedError @staticmethod - def get_list(args): + def get_list(filters=None, page_length=20, **kwargs): pass @staticmethod - def get_count(args): + def get_count(filters=None, **kwargs): pass @staticmethod - def get_stats(args): + def get_stats(**kwargs): pass """ ), diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 4c0776fe7e..078bb55c42 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -1076,7 +1076,9 @@ class TestDBQuery(FrappeTestCase): class VirtualDocType: @staticmethod def get_list(args=None, limit_page_length=0, doctype=None): + # Backward compatibility self.assertEqual(args["filters"], [["Virtual DocType", "name", "=", "test"]]) + self.assertEqual(limit_page_length, 1) self.assertEqual(doctype, "Virtual DocType") diff --git a/frappe/tests/test_virtual_doctype.py b/frappe/tests/test_virtual_doctype.py index f21cd140cd..940aa24f87 100644 --- a/frappe/tests/test_virtual_doctype.py +++ b/frappe/tests/test_virtual_doctype.py @@ -68,17 +68,17 @@ class VirtualDoctypeTest(Document): self.update_data(data) @staticmethod - def get_list(args): + def get_list(): data = VirtualDoctypeTest.get_current_data() return [frappe._dict(doc) for name, doc in data.items()] @staticmethod - def get_count(args): + def get_count(): data = VirtualDoctypeTest.get_current_data() return len(data) @staticmethod - def get_stats(args): + def get_stats(): return {} @@ -157,19 +157,18 @@ class TestVirtualDoctypes(FrappeTestCase): updated_docs = {doc1.name, doc2.name} self.assertEqual(docs, updated_docs) - listed_docs = {d.name for d in VirtualDoctypeTest.get_list({})} + listed_docs = {d.name for d in VirtualDoctypeTest.get_list()} self.assertEqual(docs, listed_docs) def test_get_count(self): - args = {"doctype": TEST_DOCTYPE_NAME, "filters": [], "fields": []} - self.assertIsInstance(VirtualDoctypeTest.get_count(args), int) + self.assertIsInstance(VirtualDoctypeTest.get_count(), int) def test_delete_doc(self): doc = frappe.get_doc(doctype=TEST_DOCTYPE_NAME).insert() frappe.delete_doc(doc.doctype, doc.name) - listed_docs = {d.name for d in VirtualDoctypeTest.get_list({})} + listed_docs = {d.name for d in VirtualDoctypeTest.get_list()} self.assertNotIn(doc.name, listed_docs) def test_controller_validity(self):