From 5cc21da6a11f05c058d52d171c005f7f588929a0 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 31 Jan 2023 13:06:44 +0530 Subject: [PATCH 1/7] fix: Interface DatabaseQuery to virtual doctypes' --- frappe/model/db_query.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index efdad83f13..6984146d23 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -16,6 +16,7 @@ from frappe.core.doctype.server_script.server_script_utils import get_server_scr from frappe.database.utils import FallBackDateTimeStr, NestedSetHierarchy from frappe.model import optional_fields from frappe.model.meta import get_table_columns +from frappe.model.utils import is_virtual_doctype from frappe.model.utils.user_settings import get_user_settings, update_user_settings from frappe.query_builder.utils import Column from frappe.utils import ( @@ -163,6 +164,13 @@ class DatabaseQuery: if user_settings: self.user_settings = json.loads(user_settings) + if is_virtual_doctype(self.doctype): + from frappe.model.virtual_doctype import get_controller + + controller = get_controller(self.doctype) + self.parse_args() + return controller.get_list(self.__dict__) + self.columns = self.get_table_columns() # no table & ignore_ddl, return From 9d236fc2cc8e69e1002dc44c06b1b0193dac675a Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 31 Jan 2023 15:05:23 +0530 Subject: [PATCH 2/7] fix: handle missing is_virtual column via is_virtual_doctype --- frappe/model/db_query.py | 2 +- frappe/model/utils/__init__.py | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 6984146d23..af32df8c87 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -165,7 +165,7 @@ class DatabaseQuery: self.user_settings = json.loads(user_settings) if is_virtual_doctype(self.doctype): - from frappe.model.virtual_doctype import get_controller + from frappe.model.base_document import get_controller controller = get_controller(self.doctype) self.parse_args() diff --git a/frappe/model/utils/__init__.py b/frappe/model/utils/__init__.py index bf6804ad05..2935872fc7 100644 --- a/frappe/model/utils/__init__.py +++ b/frappe/model/utils/__init__.py @@ -129,5 +129,7 @@ def get_fetch_values(doctype, fieldname, value): @site_cache() -def is_virtual_doctype(doctype): - return frappe.db.get_value("DocType", doctype, "is_virtual") +def is_virtual_doctype(doctype: str): + if frappe.db.has_column("DocType", "is_virtual"): + return frappe.db.get_value("DocType", doctype, "is_virtual") + return False From dc940bac1d345c5448bc2077716fa3d41ca353b7 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 31 Jan 2023 17:07:04 +0530 Subject: [PATCH 3/7] fix: Pass all DatabaseQuery.execute params to virtual doctype's get_list Give parsed args higher priority in kwargs resolution --- frappe/model/db_query.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index af32df8c87..23d1aac967 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -169,7 +169,15 @@ class DatabaseQuery: controller = get_controller(self.doctype) self.parse_args() - return controller.get_list(self.__dict__) + kwargs = { + "as_list": as_list, + "with_comment_count": with_comment_count, + "save_user_settings": save_user_settings, + "save_user_settings_fields": save_user_settings_fields, + "pluck": pluck, + "parent_doctype": parent_doctype, + } | self.__dict__ + return controller.get_list(kwargs) self.columns = self.get_table_columns() From d50f6fa7b47f0d5a72828242b8ee0e768793d79e Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 2 Feb 2023 13:42:29 +0530 Subject: [PATCH 4/7] test: cleanup test_create_virtual_doctype --- frappe/core/doctype/doctype/test_doctype.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/frappe/core/doctype/doctype/test_doctype.py b/frappe/core/doctype/doctype/test_doctype.py index e8226d4f9d..25dcc03ce9 100644 --- a/frappe/core/doctype/doctype/test_doctype.py +++ b/frappe/core/doctype/doctype/test_doctype.py @@ -552,13 +552,14 @@ class TestDocType(FrappeTestCase): self.assertRaises(InvalidFieldNameError, validate_links_table_fieldnames, doc) def test_create_virtual_doctype(self): - """Test virtual DOcTYpe.""" + """Test virtual DocType.""" virtual_doc = new_doctype("Test Virtual Doctype") virtual_doc.is_virtual = 1 - virtual_doc.insert() - virtual_doc.save() + virtual_doc.insert(ignore_if_duplicate=True) + virtual_doc.reload() doc = frappe.get_doc("DocType", "Test Virtual Doctype") + self.assertDictEqual(doc.as_dict(), virtual_doc.as_dict()) self.assertEqual(doc.is_virtual, 1) self.assertFalse(frappe.db.table_exists("Test Virtual Doctype")) From 5d3453eeb9eaacaa761ccfa26af68adea0a99d88 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 2 Feb 2023 13:43:31 +0530 Subject: [PATCH 5/7] refactor: Re-use DefaultOrderBy value as global constant --- frappe/database/database.py | 7 ++++--- frappe/database/query.py | 4 ++-- frappe/database/utils.py | 2 +- frappe/model/db_query.py | 6 +++--- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index fe258be8d7..5fd379b9c5 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -20,6 +20,7 @@ import frappe.defaults import frappe.model.meta from frappe import _ from frappe.database.utils import ( + DefaultOrderBy, EmptyQueryValues, FallBackDateTimeStr, LazyMogrify, @@ -422,7 +423,7 @@ class Database: ignore=None, as_dict=False, debug=False, - order_by="KEEP_DEFAULT_ORDERING", + order_by=DefaultOrderBy, cache=False, for_update=False, *, @@ -492,7 +493,7 @@ class Database: ignore=None, as_dict=False, debug=False, - order_by="KEEP_DEFAULT_ORDERING", + order_by=DefaultOrderBy, update=None, cache=False, for_update=False, @@ -551,7 +552,7 @@ class Database: if (filters is not None) and (filters != doctype or doctype == "DocType"): try: if order_by: - order_by = "modified" if order_by == "KEEP_DEFAULT_ORDERING" else order_by + order_by = "modified" if order_by == DefaultOrderBy else order_by out = self._get_values_from_table( fields=fields, filters=filters, diff --git a/frappe/database/query.py b/frappe/database/query.py index 10423f9ca4..3bf6824ab4 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -10,7 +10,7 @@ from pypika.queries import QueryBuilder, Table import frappe from frappe import _ from frappe.database.operator_map import OPERATOR_MAP -from frappe.database.utils import get_doctype_name +from frappe.database.utils import DefaultOrderBy, get_doctype_name from frappe.query_builder import Criterion, Field, Order, functions from frappe.query_builder.functions import Function, SqlFunctions from frappe.query_builder.utils import PseudoColumnMapper @@ -314,7 +314,7 @@ class Engine: return _fields def apply_order_by(self, order_by: str | None): - if not order_by or order_by == "KEEP_DEFAULT_ORDERING": + if not order_by or order_by == DefaultOrderBy: return for declaration in order_by.split(","): if _order_by := declaration.strip(): diff --git a/frappe/database/utils.py b/frappe/database/utils.py index 304fd72be6..d1030ca6d7 100644 --- a/frappe/database/utils.py +++ b/frappe/database/utils.py @@ -17,7 +17,7 @@ QueryValues = tuple | list | dict | NoneType EmptyQueryValues = object() FallBackDateTimeStr = "0001-01-01 00:00:00.000000" - +DefaultOrderBy = "KEEP_DEFAULT_ORDERING" NestedSetHierarchy = ( "ancestors of", "descendants of", diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 23d1aac967..c9789ae9bc 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -13,7 +13,7 @@ import frappe.permissions import frappe.share from frappe import _ from frappe.core.doctype.server_script.server_script_utils import get_server_script_map -from frappe.database.utils import FallBackDateTimeStr, NestedSetHierarchy +from frappe.database.utils import DefaultOrderBy, FallBackDateTimeStr, NestedSetHierarchy from frappe.model import optional_fields from frappe.model.meta import get_table_columns from frappe.model.utils import is_virtual_doctype @@ -73,7 +73,7 @@ class DatabaseQuery: or_filters=None, docstatus=None, group_by=None, - order_by="KEEP_DEFAULT_ORDERING", + order_by=DefaultOrderBy, limit_start=False, limit_page_length=None, as_list=False, @@ -888,7 +888,7 @@ class DatabaseQuery: def set_order_by(self, args): meta = frappe.get_meta(self.doctype) - if self.order_by and self.order_by != "KEEP_DEFAULT_ORDERING": + if self.order_by and self.order_by != DefaultOrderBy: args.order_by = self.order_by else: args.order_by = "" From c4061904da70b1daeb6b5d5df61d3c0878c6ec33 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 2 Feb 2023 13:45:17 +0530 Subject: [PATCH 6/7] test: Split DBQuery & ReportView API tests into separate cases --- frappe/tests/test_db_query.py | 177 +++++++++++++++++----------------- 1 file changed, 90 insertions(+), 87 deletions(-) diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 1fa751662c..242b7eb02c 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -1,10 +1,13 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE import datetime +from unittest.mock import MagicMock, patch import frappe +from frappe.core.doctype.doctype.test_doctype import new_doctype from frappe.core.page.permission_manager.permission_manager import add, reset, update from frappe.custom.doctype.property_setter.property_setter import make_property_setter +from frappe.database.utils import DefaultOrderBy from frappe.desk.reportview import get_filters_cond from frappe.handler import execute_cmd from frappe.model.db_query import DatabaseQuery @@ -16,7 +19,7 @@ from frappe.utils.testutils import add_custom_field, clear_custom_fields test_dependencies = ["User", "Blog Post", "Blog Category", "Blogger"] -class TestReportview(FrappeTestCase): +class TestDBQuery(FrappeTestCase): def setUp(self): frappe.set_user("Administrator") @@ -726,89 +729,6 @@ class TestReportview(FrappeTestCase): self.assertEqual(users_unedited[0].modified, users_unedited[0].creation) self.assertNotEqual(users_edited[0].modified, users_edited[0].creation) - def test_reportview_get(self): - user = frappe.get_doc("User", "test@example.com") - add_child_table_to_blog_post() - - user_roles = frappe.get_roles() - user.remove_roles(*user_roles) - user.add_roles("Blogger") - - make_property_setter("Blog Post", "published", "permlevel", 1, "Int") - reset("Blog Post") - add("Blog Post", "Website Manager", 1) - update("Blog Post", "Website Manager", 1, "write", 1) - - frappe.set_user(user.name) - - frappe.local.request = frappe._dict() - frappe.local.request.method = "POST" - - frappe.local.form_dict = frappe._dict( - { - "doctype": "Blog Post", - "fields": ["published", "title", "`tabTest Child`.`test_field`"], - } - ) - - # even if * is passed, fields which are not accessible should be filtered out - response = execute_cmd("frappe.desk.reportview.get") - self.assertListEqual(response["keys"], ["title"]) - frappe.local.form_dict = frappe._dict( - { - "doctype": "Blog Post", - "fields": ["*"], - } - ) - - response = execute_cmd("frappe.desk.reportview.get") - self.assertNotIn("published", response["keys"]) - - frappe.set_user("Administrator") - user.add_roles("Website Manager") - frappe.set_user(user.name) - - frappe.set_user("Administrator") - - # Admin should be able to see access all fields - frappe.local.form_dict = frappe._dict( - { - "doctype": "Blog Post", - "fields": ["published", "title", "`tabTest Child`.`test_field`"], - } - ) - - response = execute_cmd("frappe.desk.reportview.get") - self.assertListEqual(response["keys"], ["published", "title", "test_field"]) - - # reset user roles - user.remove_roles("Blogger", "Website Manager") - user.add_roles(*user_roles) - - def test_reportview_get_aggregation(self): - # test aggregation based on child table field - frappe.local.form_dict = frappe._dict( - { - "doctype": "DocType", - "fields": """["`tabDocField`.`label` as field_label","`tabDocField`.`name` as field_name"]""", - "filters": "[]", - "order_by": "_aggregate_column desc", - "start": 0, - "page_length": 20, - "view": "Report", - "with_comment_count": 0, - "group_by": "field_label, field_name", - "aggregate_on_field": "columns", - "aggregate_on_doctype": "DocField", - "aggregate_function": "sum", - } - ) - - response = execute_cmd("frappe.desk.reportview.get") - self.assertListEqual( - response["keys"], ["field_label", "field_name", "_aggregate_column", "columns"] - ) - def test_cast_name(self): from frappe.core.doctype.doctype.test_doctype import new_doctype @@ -916,6 +836,91 @@ class TestReportview(FrappeTestCase): self.assertIn("ifnull", frappe.get_all("User", {"name": ("not in", [""])}, run=0)) +class TestReportView(FrappeTestCase): + def test_reportview_get(self): + user = frappe.get_doc("User", "test@example.com") + add_child_table_to_blog_post() + + user_roles = frappe.get_roles() + user.remove_roles(*user_roles) + user.add_roles("Blogger") + + make_property_setter("Blog Post", "published", "permlevel", 1, "Int") + reset("Blog Post") + add("Blog Post", "Website Manager", 1) + update("Blog Post", "Website Manager", 1, "write", 1) + + frappe.set_user(user.name) + + frappe.local.request = frappe._dict() + frappe.local.request.method = "POST" + + frappe.local.form_dict = frappe._dict( + { + "doctype": "Blog Post", + "fields": ["published", "title", "`tabTest Child`.`test_field`"], + } + ) + + # even if * is passed, fields which are not accessible should be filtered out + response = execute_cmd("frappe.desk.reportview.get") + self.assertListEqual(response["keys"], ["title"]) + frappe.local.form_dict = frappe._dict( + { + "doctype": "Blog Post", + "fields": ["*"], + } + ) + + response = execute_cmd("frappe.desk.reportview.get") + self.assertNotIn("published", response["keys"]) + + frappe.set_user("Administrator") + user.add_roles("Website Manager") + frappe.set_user(user.name) + + frappe.set_user("Administrator") + + # Admin should be able to see access all fields + frappe.local.form_dict = frappe._dict( + { + "doctype": "Blog Post", + "fields": ["published", "title", "`tabTest Child`.`test_field`"], + } + ) + + response = execute_cmd("frappe.desk.reportview.get") + self.assertListEqual(response["keys"], ["published", "title", "test_field"]) + + # reset user roles + user.remove_roles("Blogger", "Website Manager") + user.add_roles(*user_roles) + + def test_reportview_get_aggregation(self): + # test aggregation based on child table field + frappe.local.form_dict = frappe._dict( + { + "doctype": "DocType", + "fields": """["`tabDocField`.`label` as field_label","`tabDocField`.`name` as field_name"]""", + "filters": "[]", + "order_by": "_aggregate_column desc", + "start": 0, + "page_length": 20, + "view": "Report", + "with_comment_count": 0, + "group_by": "field_label, field_name", + "aggregate_on_field": "columns", + "aggregate_on_doctype": "DocField", + "aggregate_function": "sum", + } + ) + + response = execute_cmd("frappe.desk.reportview.get") + self.assertListEqual( + response["keys"], ["field_label", "field_name", "_aggregate_column", "columns"] + ) + + def add_child_table_to_blog_post(): child_table = frappe.get_doc( { @@ -939,7 +944,7 @@ def create_event(subject="_Test Event", starts_on=None): from frappe.utils import get_datetime - event = frappe.get_doc( + return frappe.get_doc( { "doctype": "Event", "subject": subject, @@ -948,8 +953,6 @@ def create_event(subject="_Test Event", starts_on=None): } ).insert(ignore_permissions=True) - return event - def create_nested_doctype(): if frappe.db.exists("DocType", "Nested DocType"): From fdff6351cda65aad0cef8a3ec08899c327ffe524 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 2 Feb 2023 13:45:35 +0530 Subject: [PATCH 7/7] test: Add test for DatabaseQuery for virtual doctypes --- frappe/tests/test_db_query.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 242b7eb02c..38bc469388 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -826,6 +826,33 @@ class TestDBQuery(FrappeTestCase): self.assertTrue(dashboard_settings) + def test_virtual_doctype(self): + """Test that virtual doctypes can be queried using get_all""" + + virtual_doctype = new_doctype("Virtual DocType") + virtual_doctype.is_virtual = 1 + virtual_doctype.insert(ignore_if_duplicate=True) + + class VirtualDocType: + @staticmethod + def get_list(args): + ... + + 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", {"name": ("in", ["a", "b"])}, run=0)) self.assertIn("ifnull", frappe.get_all("User", {"name": ("in", ["a", None])}, run=0))