From 3f70cebf65a8c000a176750b0b22969e41e42609 Mon Sep 17 00:00:00 2001 From: Dany Robert Date: Mon, 13 Nov 2023 13:00:14 +0000 Subject: [PATCH 01/47] feat: workflow state wise status override --- frappe/public/js/frappe/model/indicator.js | 13 ++++++++--- frappe/public/js/frappe/model/workflow.js | 8 +++++++ .../workflow_document_state.json | 23 +++++++++++++++---- .../workflow_document_state.py | 2 ++ 4 files changed, 38 insertions(+), 8 deletions(-) diff --git a/frappe/public/js/frappe/model/indicator.js b/frappe/public/js/frappe/model/indicator.js index d5c42c3799..6ae40c1f05 100644 --- a/frappe/public/js/frappe/model/indicator.js +++ b/frappe/public/js/frappe/model/indicator.js @@ -36,11 +36,18 @@ frappe.get_indicator = function (doc, doctype, show_workflow_state) { var settings = frappe.listview_settings[doctype] || {}; - var is_submittable = frappe.model.is_submittable(doctype), - workflow_fieldname = frappe.workflow.get_state_fieldname(doctype); + var is_submittable = frappe.model.is_submittable(doctype); + let workflow_fieldname = frappe.workflow.get_state_fieldname(doctype); + let avoid_status_override = (frappe.workflow.avoid_status_override[doctype] || []).includes( + doc[workflow_fieldname] + ); // workflow - if (workflow_fieldname && (!without_workflow || show_workflow_state)) { + if ( + workflow_fieldname && + (!without_workflow || show_workflow_state) && + !avoid_status_override + ) { var value = doc[workflow_fieldname]; if (value) { let colour = ""; diff --git a/frappe/public/js/frappe/model/workflow.js b/frappe/public/js/frappe/model/workflow.js index db4c2b25ae..11643b878d 100644 --- a/frappe/public/js/frappe/model/workflow.js +++ b/frappe/public/js/frappe/model/workflow.js @@ -6,11 +6,19 @@ frappe.provide("frappe.workflow"); frappe.workflow = { state_fields: {}, workflows: {}, + avoid_status_override: {}, setup: function (doctype) { var wf = frappe.get_list("Workflow", { document_type: doctype }); if (wf.length) { frappe.workflow.workflows[doctype] = wf[0]; frappe.workflow.state_fields[doctype] = wf[0].workflow_state_field; + frappe.workflow.avoid_status_override[doctype] = frappe.workflow.workflows[ + doctype + ].states + .filter((row) => { + if (row.avoid_status_override) return true; + }) + .map((d) => d.state); } else { frappe.workflow.state_fields[doctype] = null; } diff --git a/frappe/workflow/doctype/workflow_document_state/workflow_document_state.json b/frappe/workflow/doctype/workflow_document_state/workflow_document_state.json index d3d325adf4..465f72d9b9 100644 --- a/frappe/workflow/doctype/workflow_document_state/workflow_document_state.json +++ b/frappe/workflow/doctype/workflow_document_state/workflow_document_state.json @@ -1,4 +1,5 @@ { + "actions": [], "creation": "2013-02-22 01:27:36", "description": "Represents the states allowed in one document and role assigned to change the state.", "doctype": "DocType", @@ -12,6 +13,7 @@ "update_value", "column_break_4", "is_optional_state", + "avoid_status_override", "next_action_email_template", "allow_edit", "section_break_9", @@ -91,19 +93,30 @@ "fieldtype": "Section Break" }, { - "fieldname": "workflow_builder_id", - "fieldtype": "Data", - "hidden": 1, - "label": "Workflow Builder ID" + "fieldname": "workflow_builder_id", + "fieldtype": "Data", + "hidden": 1, + "label": "Workflow Builder ID" + }, + { + "default": "0", + "description": "If Checked workflow status will not override status in list view", + "fieldname": "avoid_status_override", + "fieldtype": "Check", + "label": "Don't Override Status" } ], "idx": 1, "istable": 1, - "modified": "2019-06-14 11:43:20.271091", + "links": [], + "modified": "2023-11-13 18:27:08.633239", "modified_by": "Administrator", "module": "Workflow", "name": "Workflow Document State", "owner": "Administrator", "permissions": [], + "sort_field": "modified", + "sort_order": "DESC", + "states": [], "track_changes": 1 } \ No newline at end of file diff --git a/frappe/workflow/doctype/workflow_document_state/workflow_document_state.py b/frappe/workflow/doctype/workflow_document_state/workflow_document_state.py index 2484e8c49c..d1644578d8 100644 --- a/frappe/workflow/doctype/workflow_document_state/workflow_document_state.py +++ b/frappe/workflow/doctype/workflow_document_state/workflow_document_state.py @@ -15,6 +15,7 @@ class WorkflowDocumentState(Document): from frappe.types import DF allow_edit: DF.Link + avoid_status_override: DF.Check doc_status: DF.Literal["0", "1", "2"] is_optional_state: DF.Check message: DF.Text | None @@ -25,5 +26,6 @@ class WorkflowDocumentState(Document): state: DF.Link update_field: DF.Literal update_value: DF.Data | None + workflow_builder_id: DF.Data | None # end: auto-generated types pass From 0026a6ceac816790871ba5dbfe6492561191d73d Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Fri, 6 Oct 2023 18:22:41 +0530 Subject: [PATCH 02/47] feat(docfield): add in a nullable checkbox field Signed-off-by: Akhil Narang --- frappe/core/doctype/docfield/docfield.json | 10 +++++++++- frappe/core/doctype/docfield/docfield.py | 1 + 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/frappe/core/doctype/docfield/docfield.json b/frappe/core/doctype/docfield/docfield.json index f67969f43e..22b84d1cbc 100644 --- a/frappe/core/doctype/docfield/docfield.json +++ b/frappe/core/doctype/docfield/docfield.json @@ -19,6 +19,7 @@ "reqd", "is_virtual", "search_index", + "not_nullable", "column_break_18", "options", "sort_options", @@ -566,13 +567,20 @@ "fieldname": "link_filters", "fieldtype": "JSON", "label": "Link Filters" + }, + { + "default": "0", + "depends_on": "eval:!in_list([\"Check\", \"Currency\", \"Float\", \"Int\", \"Percent\", \"Rating\", \"Select\", \"Table\", \"Table MultiSelect\"], doc.fieldtype)", + "fieldname": "not_nullable", + "fieldtype": "Check", + "label": "Not Nullable" } ], "idx": 1, "index_web_pages_for_search": 1, "istable": 1, "links": [], - "modified": "2023-11-13 11:48:51.502812", + "modified": "2023-11-16 11:26:56.364594", "modified_by": "Administrator", "module": "Core", "name": "DocField", diff --git a/frappe/core/doctype/docfield/docfield.py b/frappe/core/doctype/docfield/docfield.py index 351dd0caae..dc26c1f96f 100644 --- a/frappe/core/doctype/docfield/docfield.py +++ b/frappe/core/doctype/docfield/docfield.py @@ -92,6 +92,7 @@ class DocField(Document): max_height: DF.Data | None no_copy: DF.Check non_negative: DF.Check + not_nullable: DF.Check oldfieldname: DF.Data | None oldfieldtype: DF.Data | None options: DF.SmallText | None From 18867b273f101a405de900b887a465ff82cef912 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Wed, 18 Oct 2023 13:26:43 +0530 Subject: [PATCH 03/47] chore: make return type annotation make more sense Signed-off-by: Akhil Narang --- frappe/model/document.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index 4d5eec64fc..9ed4e2a3f2 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -343,7 +343,7 @@ class Document(BaseDocument): :param ignore_permissions: Do not check permissions if True. :param ignore_version: Do not save version if True.""" if self.flags.in_print: - return + return self self.flags.notifications_executed = [] From 75709eede7a9eab88b0668dc20bca97564f6b2e3 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Wed, 18 Oct 2023 13:28:21 +0530 Subject: [PATCH 04/47] feat: set a non-null value if docfield isn't set as nullable Signed-off-by: Akhil Narang --- frappe/model/base_document.py | 5 ++++ frappe/utils/defaults.py | 50 +++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 frappe/utils/defaults.py diff --git a/frappe/model/base_document.py b/frappe/model/base_document.py index 78c20a2eae..264ec219b3 100644 --- a/frappe/model/base_document.py +++ b/frappe/model/base_document.py @@ -31,6 +31,7 @@ from frappe.utils import ( sanitize_html, strip_html, ) +from frappe.utils.defaults import get_not_null_defaults from frappe.utils.html_utils import unescape_html if TYPE_CHECKING: @@ -384,6 +385,10 @@ class BaseDocument: if ignore_nulls and not is_virtual_field and value is None: continue + # If the docfield is not nullable - set a default non-null value + if value is None and getattr(df, "not_nullable", False): + value = get_not_null_defaults(df.fieldtype) + d[fieldname] = value return d diff --git a/frappe/utils/defaults.py b/frappe/utils/defaults.py new file mode 100644 index 0000000000..152efef8e3 --- /dev/null +++ b/frappe/utils/defaults.py @@ -0,0 +1,50 @@ +from typing import Literal + + +def get_not_null_defaults(column_type: str) -> Literal["", 0] | None: + """ + Method to return a default value for a column type that is not NoneType + :param column_type: The type of column + :return: The value to be set + """ + column_type_map = { + "Data": str, + "Text": str, + "Autocomplete": str, + "Attach": str, + "AttachImage": str, + "Barcode": str, + "Check": int, + "Code": str, + "Color": str, + "Currency": float, + "Date": str, + "Datetime": str, + "Duration": int, + "DynamicLink": str, + "Float": float, + "HTMLEditor": str, + "Int": int, + "JSON": str, + "Link": str, + "LongText": str, + "MarkdownEditor": str, + "Password": str, + "Percent": float, + "Phone": str, + "ReadOnly": str, + "Rating": float, + "Select": str, + "SmallText": str, + "TextEditor": str, + "Time": str, + "Table": list, + "Table MultiSelect": list, + } + data_type = column_type_map.get(column_type.replace(" ", ""), str) + # data_type = eval(f"frappe.types.DF.{column_type.replace(' ', '')}") + if data_type == str: + return "" + if data_type in (int, float): + return 0 + return None From 390d4e1b13af7001430120a77311f2581ec7c47f Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Wed, 18 Oct 2023 17:01:03 +0530 Subject: [PATCH 05/47] chore: drop unused variables and parameters Signed-off-by: Akhil Narang --- frappe/model/db_query.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 62c0538298..9180754bb9 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -109,7 +109,6 @@ class DatabaseQuery: save_user_settings=False, save_user_settings_fields=False, update=None, - add_total_row=None, user_settings=None, reference_doctype=None, run=True, @@ -981,7 +980,6 @@ class DatabaseQuery: ) def add_user_permissions(self, user_permissions): - doctype_link_fields = [] doctype_link_fields = self.doctype_meta.get_link_fields() # append current doctype with fieldname as 'name' as first link field From 15c925ccc7c50f4367927e4deacb4d45aeab7d55 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Wed, 18 Oct 2023 17:02:19 +0530 Subject: [PATCH 06/47] feat: check for docfield not_nullable property to decide whether a field can be nullable Signed-off-by: Akhil Narang --- frappe/model/db_query.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 9180754bb9..78adfffe76 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -807,7 +807,10 @@ class DatabaseQuery: df = meta.get("fields", {"fieldname": f.fieldname}) df = df[0] if df else None - if df and df.fieldtype in ("Check", "Float", "Int", "Currency", "Percent"): + if df and ( + df.fieldtype in ("Check", "Float", "Int", "Currency", "Percent") + or getattr(df, "not_nullable", False) + ): can_be_null = False if f.operator.lower() in ("previous", "next", "timespan"): From 4f9ee8b6338eca47ea139a0a3780a8be166c0664 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Wed, 18 Oct 2023 17:41:34 +0530 Subject: [PATCH 07/47] feat(DbColumn): set `non_nullable` attribute and make use of it for column definition Signed-off-by: Akhil Narang --- frappe/database/schema.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/frappe/database/schema.py b/frappe/database/schema.py index 24eea24fa9..70b2cee244 100644 --- a/frappe/database/schema.py +++ b/frappe/database/schema.py @@ -3,6 +3,7 @@ import re import frappe from frappe import _ from frappe.utils import cint, cstr, flt +from frappe.utils.defaults import get_not_null_defaults SPECIAL_CHAR_PATTERN = re.compile(r"[\W]", flags=re.UNICODE) VARCHAR_CAST_PATTERN = re.compile(r"varchar\(([\d]+)\)") @@ -98,6 +99,7 @@ class DBTable: field.get("options"), field.get("unique"), field.get("precision"), + field.get("not_nullable"), ) def validate(self): @@ -175,7 +177,17 @@ class DBTable: class DbColumn: def __init__( - self, table, fieldname, fieldtype, length, default, set_index, options, unique, precision + self, + table, + fieldname, + fieldtype, + length, + default, + set_index, + options, + unique, + precision, + not_nullable, ): self.table = table self.fieldname = fieldname @@ -186,6 +198,7 @@ class DbColumn: self.options = options self.unique = unique self.precision = precision + self.not_nullable = not_nullable def get_definition(self, for_modification=False): column_def = get_definition(self.fieldtype, precision=self.precision, length=self.length) @@ -208,6 +221,12 @@ class DbColumn: ): column_def += f" default {frappe.db.escape(self.default)}" + elif self.not_nullable: + default_value = get_not_null_defaults(self.fieldtype) + if isinstance(default_value, str): + default_value = frappe.db.escape(default_value) + column_def += f" not null default {default_value}" + if self.unique and not for_modification and (column_def not in ("text", "longtext")): column_def += " unique" From 6550e0769bbb8284bd190f3bf17d95df400df549 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 19 Oct 2023 11:01:43 +0530 Subject: [PATCH 08/47] feat: allow ignoring implicit commit warning Signed-off-by: Akhil Narang --- frappe/database/database.py | 19 +++++++++++-------- frappe/database/postgres/database.py | 2 +- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 9a6cb7772a..b7df11849c 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -1,7 +1,6 @@ # Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE -import datetime import itertools import json import random @@ -14,7 +13,6 @@ from time import time from typing import Any from pypika.dialects import MySQLQueryBuilder, PostgreSQLQueryBuilder -from pypika.terms import Criterion, NullValue import frappe import frappe.defaults @@ -151,6 +149,7 @@ class Database: explain=False, run=True, pluck=False, + ignore_implicit_commit=False, ): """Execute a SQL query and fetch all rows. @@ -163,6 +162,9 @@ class Database: :param auto_commit: Commit after executing the query. :param update: Update this dict to all rows (if returned `as_dict`). :param run: Returns query without executing it if False. + :param pluck: Get the plucked field only. + :param ignore_implicit_commit: Ignore implicit commit warnings. + :param explain: Print `EXPLAIN` in error log. Examples: # return customer names as dicts @@ -194,7 +196,7 @@ class Database: self.connect() # in transaction validations - self.check_transaction_status(query) + self.check_transaction_status(query, ignore_implicit_commit) self.clear_db_table_cache(query) if auto_commit: @@ -369,11 +371,11 @@ class Database: self.commit() self.sql(query, debug=debug) - def check_transaction_status(self, query): + def check_transaction_status(self, query: str, ignore_implicit_commit: bool = False): """Raises exception if more than 200,000 `INSERT`, `UPDATE` queries are executed in one transaction. This is to ensure that writes are always flushed otherwise this could cause the system to hang.""" - self.check_implicit_commit(query) + self.check_implicit_commit(query, ignore_implicit_commit) if query and is_query_type(query, ("commit", "rollback")): self.transaction_writes = 0 @@ -388,13 +390,14 @@ class Database: msg += _("The changes have been reverted.") + "
" raise frappe.TooManyWritesError(msg) - def check_implicit_commit(self, query): + def check_implicit_commit(self, query: str, ignore_implicit_commit: bool = False): if ( - self.transaction_writes + not ignore_implicit_commit + and self.transaction_writes and query and is_query_type(query, ("start", "alter", "drop", "create", "begin", "truncate")) ): - raise ImplicitCommitError("This statement can cause implicit commit") + raise ImplicitCommitError("This statement can cause implicit commit", query) def fetch_as_dict(self) -> list[frappe._dict]: """Internal. Converts results to dict.""" diff --git a/frappe/database/postgres/database.py b/frappe/database/postgres/database.py index f0a75e2e68..8f330f3676 100644 --- a/frappe/database/postgres/database.py +++ b/frappe/database/postgres/database.py @@ -337,7 +337,7 @@ class PostgresDatabase(PostgresExceptionUtil, Database): key = '", "'.join(key) return f'ON CONFLICT ("{key}") DO UPDATE SET ' - def check_implicit_commit(self, query): + def check_implicit_commit(self, query, ignore_implicit_commit=False): pass # postgres can run DDL in transactions without implicit commits def has_index(self, table_name, index_name): From e41fa2dfc7d44ae7e1772d8a6c89995bdd255f94 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 19 Oct 2023 11:09:41 +0530 Subject: [PATCH 09/47] feat: migrate columns to be non-nullable if required Signed-off-by: Akhil Narang --- frappe/database/mariadb/database.py | 5 +++-- frappe/database/mariadb/schema.py | 17 +++++++++++++++-- frappe/database/postgres/database.py | 5 +++-- frappe/database/schema.py | 5 +++++ frappe/database/utils.py | 6 +----- 5 files changed, 27 insertions(+), 11 deletions(-) diff --git a/frappe/database/mariadb/database.py b/frappe/database/mariadb/database.py index df64bdc86a..1f087a243a 100644 --- a/frappe/database/mariadb/database.py +++ b/frappe/database/mariadb/database.py @@ -310,7 +310,7 @@ class MariaDBDatabase(MariaDBConnectionUtil, MariaDBExceptionUtil, Database): ) @staticmethod - def get_on_duplicate_update(key=None): + def get_on_duplicate_update(): return "ON DUPLICATE key UPDATE " def get_table_columns_description(self, table_name): @@ -329,7 +329,8 @@ class MariaDBDatabase(MariaDBConnectionUtil, MariaDBExceptionUtil, Database): and Seq_in_index = 1 limit 1 ), 0) as 'index', - column_key = 'UNI' as 'unique' + column_key = 'UNI' as 'unique', + (is_nullable = 'NO') AS 'not_nullable' from information_schema.columns as columns where table_name = '{table_name}' """.format( table_name=table_name diff --git a/frappe/database/mariadb/schema.py b/frappe/database/mariadb/schema.py index 209535b9fd..7d8a1fc9e7 100644 --- a/frappe/database/mariadb/schema.py +++ b/frappe/database/mariadb/schema.py @@ -3,6 +3,7 @@ from pymysql.constants.ER import DUP_ENTRY import frappe from frappe import _ from frappe.database.schema import DBTable +from frappe.utils.defaults import get_not_null_defaults class MariaDBTable(DBTable): @@ -69,7 +70,7 @@ class MariaDBTable(DBTable): add_column_query = [ f"ADD COLUMN `{col.fieldname}` {col.get_definition()}" for col in self.add_column ] - columns_to_modify = set(self.change_type + self.set_default) + columns_to_modify = set(self.change_type + self.set_default + self.change_nullability) modify_column_query = [ f"MODIFY `{col.fieldname}` {col.get_definition(for_modification=True)}" for col in columns_to_modify @@ -102,12 +103,24 @@ class MariaDBTable(DBTable): if index_record := frappe.db.get_column_index(self.table_name, col.fieldname, unique=False): drop_index_query.append(f"DROP INDEX `{index_record.Key_name}`") + for col in self.change_nullability: + current_column = self.current_columns.get(col.fieldname.lower()) + if col.not_nullable: + default_value = get_not_null_defaults(col.fieldtype) + if isinstance(default_value, str): + default_value = frappe.db.escape(default_value) + query = f"UPDATE `{self.table_name}` SET `{col.fieldname}`={default_value} WHERE `{col.fieldname}` IS NULL;" + try: + frappe.db.sql(query, ignore_implicit_commit=True) + except Exception as e: + print(f"Failed to alter schema using query: {query}") + raise try: for query_parts in [add_column_query, modify_column_query, add_index_query, drop_index_query]: if query_parts: query_body = ", ".join(query_parts) query = f"ALTER TABLE `{self.table_name}` {query_body}" - frappe.db.sql(query) + frappe.db.sql(query, ignore_implicit_commit=True) except Exception as e: if query := locals().get("query"): # this weirdness is to avoid potentially unbounded vars diff --git a/frappe/database/postgres/database.py b/frappe/database/postgres/database.py index 8f330f3676..476067c435 100644 --- a/frappe/database/postgres/database.py +++ b/frappe/database/postgres/database.py @@ -392,7 +392,8 @@ class PostgresDatabase(PostgresExceptionUtil, Database): END AS type, BOOL_OR(b.index) AS index, SPLIT_PART(COALESCE(a.column_default, NULL), '::', 1) AS default, - BOOL_OR(b.unique) AS unique + BOOL_OR(b.unique) AS unique, + COALESCE(a.is_nullable = 'NO', false) AS not_nullable FROM information_schema.columns a LEFT JOIN (SELECT indexdef, tablename, @@ -402,7 +403,7 @@ class PostgresDatabase(PostgresExceptionUtil, Database): WHERE tablename='{table_name}') b ON SUBSTRING(b.indexdef, '(.*)') LIKE CONCAT('%', a.column_name, '%') WHERE a.table_name = '{table_name}' - GROUP BY a.column_name, a.data_type, a.column_default, a.character_maximum_length; + GROUP BY a.column_name, a.data_type, a.column_default, a.character_maximum_length, a.is_nullable; """.format( table_name=table_name ), diff --git a/frappe/database/schema.py b/frappe/database/schema.py index 70b2cee244..5cdd995c2c 100644 --- a/frappe/database/schema.py +++ b/frappe/database/schema.py @@ -25,6 +25,7 @@ class DBTable: self.add_column: list[DbColumn] = [] self.change_type: list[DbColumn] = [] self.change_name: list[DbColumn] = [] + self.change_nullability: list[DbColumn] = [] self.add_unique: list[DbColumn] = [] self.add_index: list[DbColumn] = [] self.drop_unique: list[DbColumn] = [] @@ -269,6 +270,10 @@ class DbColumn: ): self.table.set_default.append(self) + # nullability + if self.not_nullable is not None and (self.not_nullable != current_def["not_nullable"]): + self.table.change_nullability.append(self) + # index should be applied or dropped irrespective of type change if (current_def["index"] and not self.set_index) and column_type not in ("text", "longtext"): self.table.drop_index.append(self) diff --git a/frappe/database/utils.py b/frappe/database/utils.py index 7cdab76dda..5d1de5792f 100644 --- a/frappe/database/utils.py +++ b/frappe/database/utils.py @@ -1,7 +1,6 @@ # Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE -import typing from functools import cached_property from types import NoneType @@ -9,9 +8,6 @@ import frappe from frappe.query_builder.builder import MariaDB, Postgres from frappe.query_builder.functions import Function -if typing.TYPE_CHECKING: - from frappe.query_builder import DocType - Query = str | MariaDB | Postgres QueryValues = tuple | list | dict | NoneType @@ -27,7 +23,7 @@ NestedSetHierarchy = ( ) -def is_query_type(query: str, query_type: str | tuple[str]) -> bool: +def is_query_type(query: str, query_type: str | tuple[str, ...]) -> bool: return query.lstrip().split(maxsplit=1)[0].lower().startswith(query_type) From 58b8b26b93197d0cbfceeca9b030863461428a8b Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 19 Oct 2023 18:09:45 +0530 Subject: [PATCH 10/47] chore(mariadb): switch to using query builder for update statement Signed-off-by: Akhil Narang --- frappe/database/mariadb/schema.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/frappe/database/mariadb/schema.py b/frappe/database/mariadb/schema.py index 7d8a1fc9e7..d63bd3ccc9 100644 --- a/frappe/database/mariadb/schema.py +++ b/frappe/database/mariadb/schema.py @@ -107,13 +107,13 @@ class MariaDBTable(DBTable): current_column = self.current_columns.get(col.fieldname.lower()) if col.not_nullable: default_value = get_not_null_defaults(col.fieldtype) - if isinstance(default_value, str): - default_value = frappe.db.escape(default_value) - query = f"UPDATE `{self.table_name}` SET `{col.fieldname}`={default_value} WHERE `{col.fieldname}` IS NULL;" try: - frappe.db.sql(query, ignore_implicit_commit=True) + table = frappe.qb.DocType(self.doctype) + frappe.qb.update(table).set(col.fieldname, default_value).where( + getattr(table, col.fieldname).isnull() + ).run() except Exception as e: - print(f"Failed to alter schema using query: {query}") + print(f"Failed to update data in {self.table_name} for {col.fieldname}") raise try: for query_parts in [add_column_query, modify_column_query, add_index_query, drop_index_query]: From f9b9184223313b974ff45eaad951fc3e01d75db9 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 19 Oct 2023 18:10:28 +0530 Subject: [PATCH 11/47] chore: rework implicit commit checks Add in some more "reset" queries - begin - start Also check *after* resetting Signed-off-by: Akhil Narang --- frappe/database/database.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index b7df11849c..5a86fef061 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -375,11 +375,12 @@ class Database: """Raises exception if more than 200,000 `INSERT`, `UPDATE` queries are executed in one transaction. This is to ensure that writes are always flushed otherwise this could cause the system to hang.""" - self.check_implicit_commit(query, ignore_implicit_commit) - if query and is_query_type(query, ("commit", "rollback")): + if query and is_query_type(query, ("start", "begin", "commit", "rollback")): self.transaction_writes = 0 + self.check_implicit_commit(query, ignore_implicit_commit) + if query[:6].lower() in ("update", "insert", "delete"): self.transaction_writes += 1 if self.transaction_writes > self.MAX_WRITES_PER_TRANSACTION: From fe549ecc49b86bcd1720aee1a94a9c983060dbbb Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 19 Oct 2023 18:20:16 +0530 Subject: [PATCH 12/47] chore: don't check for semgrep here Signed-off-by: Akhil Narang --- frappe/database/mariadb/schema.py | 1 + 1 file changed, 1 insertion(+) diff --git a/frappe/database/mariadb/schema.py b/frappe/database/mariadb/schema.py index d63bd3ccc9..cb3a0ccab1 100644 --- a/frappe/database/mariadb/schema.py +++ b/frappe/database/mariadb/schema.py @@ -120,6 +120,7 @@ class MariaDBTable(DBTable): if query_parts: query_body = ", ".join(query_parts) query = f"ALTER TABLE `{self.table_name}` {query_body}" + # nosemgrep frappe.db.sql(query, ignore_implicit_commit=True) except Exception as e: From 806748b0630146c32b2ffc4cd1be1e78afc3c14c Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Fri, 20 Oct 2023 12:27:40 +0530 Subject: [PATCH 13/47] chore: simplify transaction handling Signed-off-by: Akhil Narang --- frappe/database/database.py | 28 ++++++++++++++++++---------- frappe/database/mariadb/database.py | 2 +- frappe/database/mariadb/schema.py | 2 +- frappe/database/postgres/database.py | 2 +- 4 files changed, 21 insertions(+), 13 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 5a86fef061..d6503161c8 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -95,6 +95,8 @@ class Database: self.before_rollback = CallbackManager() self.after_rollback = CallbackManager() + self.in_transaction = False + # self.db_type: str # self.last_query (lazy) attribute of last sql query executed @@ -149,7 +151,6 @@ class Database: explain=False, run=True, pluck=False, - ignore_implicit_commit=False, ): """Execute a SQL query and fetch all rows. @@ -163,7 +164,6 @@ class Database: :param update: Update this dict to all rows (if returned `as_dict`). :param run: Returns query without executing it if False. :param pluck: Get the plucked field only. - :param ignore_implicit_commit: Ignore implicit commit warnings. :param explain: Print `EXPLAIN` in error log. Examples: @@ -196,7 +196,7 @@ class Database: self.connect() # in transaction validations - self.check_transaction_status(query, ignore_implicit_commit) + self.check_transaction_status(query) self.clear_db_table_cache(query) if auto_commit: @@ -371,15 +371,15 @@ class Database: self.commit() self.sql(query, debug=debug) - def check_transaction_status(self, query: str, ignore_implicit_commit: bool = False): + def check_transaction_status(self, query: str): """Raises exception if more than 200,000 `INSERT`, `UPDATE` queries are executed in one transaction. This is to ensure that writes are always flushed otherwise this could cause the system to hang.""" - if query and is_query_type(query, ("start", "begin", "commit", "rollback")): + if query and is_query_type(query, ("commit", "rollback")): self.transaction_writes = 0 - self.check_implicit_commit(query, ignore_implicit_commit) + self.check_implicit_commit(query) if query[:6].lower() in ("update", "insert", "delete"): self.transaction_writes += 1 @@ -391,9 +391,9 @@ class Database: msg += _("The changes have been reverted.") + "
" raise frappe.TooManyWritesError(msg) - def check_implicit_commit(self, query: str, ignore_implicit_commit: bool = False): + def check_implicit_commit(self, query: str): if ( - not ignore_implicit_commit + self.in_transaction and self.transaction_writes and query and is_query_type(query, ("start", "alter", "drop", "create", "begin", "truncate")) @@ -964,16 +964,24 @@ class Database: read_only = read_only or frappe.flags.read_only mode = "READ ONLY" if read_only else "" self.sql(f"START TRANSACTION {mode}") + self.in_transaction = True - def commit(self): + def commit(self, new_transaction: bool = True): """Commit current transaction. Calls SQL `COMMIT`.""" + if not self.in_transaction: + self.logger.warn("Not in transaction, ignoring commit") + return + self.before_rollback.reset() self.after_rollback.reset() self.before_commit.run() self.sql("commit") - self.begin() # explicitly start a new transaction + self.in_transaction = False + + if new_transaction: + self.begin() # explicitly start a new transaction self.after_commit.run() diff --git a/frappe/database/mariadb/database.py b/frappe/database/mariadb/database.py index 1f087a243a..d8b4555a20 100644 --- a/frappe/database/mariadb/database.py +++ b/frappe/database/mariadb/database.py @@ -438,7 +438,7 @@ class MariaDBDatabase(MariaDBConnectionUtil, MariaDBExceptionUtil, Database): db_table = MariaDBTable(doctype, meta) db_table.validate() - self.commit() + self.commit(new_transaction=False) db_table.sync() self.begin() diff --git a/frappe/database/mariadb/schema.py b/frappe/database/mariadb/schema.py index cb3a0ccab1..7f84fd065a 100644 --- a/frappe/database/mariadb/schema.py +++ b/frappe/database/mariadb/schema.py @@ -121,7 +121,7 @@ class MariaDBTable(DBTable): query_body = ", ".join(query_parts) query = f"ALTER TABLE `{self.table_name}` {query_body}" # nosemgrep - frappe.db.sql(query, ignore_implicit_commit=True) + frappe.db.sql(query) except Exception as e: if query := locals().get("query"): # this weirdness is to avoid potentially unbounded vars diff --git a/frappe/database/postgres/database.py b/frappe/database/postgres/database.py index 476067c435..37fc9601f2 100644 --- a/frappe/database/postgres/database.py +++ b/frappe/database/postgres/database.py @@ -337,7 +337,7 @@ class PostgresDatabase(PostgresExceptionUtil, Database): key = '", "'.join(key) return f'ON CONFLICT ("{key}") DO UPDATE SET ' - def check_implicit_commit(self, query, ignore_implicit_commit=False): + def check_implicit_commit(self, query): pass # postgres can run DDL in transactions without implicit commits def has_index(self, table_name, index_name): From 6b49b5f32aee3484280a7cb964c9273173da19d8 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Fri, 20 Oct 2023 12:35:55 +0530 Subject: [PATCH 14/47] chore: move check_implicit_commit() call back to its original location Signed-off-by: Akhil Narang --- frappe/database/database.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index d6503161c8..6ca72088c3 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -375,12 +375,11 @@ class Database: """Raises exception if more than 200,000 `INSERT`, `UPDATE` queries are executed in one transaction. This is to ensure that writes are always flushed otherwise this could cause the system to hang.""" + self.check_implicit_commit(query) if query and is_query_type(query, ("commit", "rollback")): self.transaction_writes = 0 - self.check_implicit_commit(query) - if query[:6].lower() in ("update", "insert", "delete"): self.transaction_writes += 1 if self.transaction_writes > self.MAX_WRITES_PER_TRANSACTION: From 5e6be19fe3898833f52f8cbda8b6204585bd9aeb Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 26 Oct 2023 17:18:43 +0530 Subject: [PATCH 15/47] chore(DbColumn): force constructor arguments to be kwargs Signed-off-by: Akhil Narang --- frappe/database/schema.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/frappe/database/schema.py b/frappe/database/schema.py index 5cdd995c2c..05d9753a86 100644 --- a/frappe/database/schema.py +++ b/frappe/database/schema.py @@ -91,16 +91,16 @@ class DBTable: continue self.columns[field.get("fieldname")] = DbColumn( - self, - field.get("fieldname"), - field.get("fieldtype"), - field.get("length"), - field.get("default"), - field.get("search_index"), - field.get("options"), - field.get("unique"), - field.get("precision"), - field.get("not_nullable"), + table=self, + fieldname=field.get("fieldname"), + fieldtype=field.get("fieldtype"), + length=field.get("length"), + default=field.get("default"), + set_index=field.get("search_index"), + options=field.get("options"), + unique=field.get("unique"), + precision=field.get("precision"), + not_nullable=field.get("not_nullable"), ) def validate(self): @@ -179,6 +179,7 @@ class DBTable: class DbColumn: def __init__( self, + *, table, fieldname, fieldtype, From b88f3597fa1c84dd88d4d0cf3b883738eddf50fc Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Fri, 27 Oct 2023 16:14:44 +0530 Subject: [PATCH 16/47] chore: add in tests for non-nullable field Signed-off-by: Akhil Narang --- frappe/tests/test_non_nullable_docfield.py | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 frappe/tests/test_non_nullable_docfield.py diff --git a/frappe/tests/test_non_nullable_docfield.py b/frappe/tests/test_non_nullable_docfield.py new file mode 100644 index 0000000000..a02b292670 --- /dev/null +++ b/frappe/tests/test_non_nullable_docfield.py @@ -0,0 +1,62 @@ +import frappe +from frappe.core.doctype.doctype.test_doctype import new_doctype +from frappe.database.schema import DBTable +from frappe.tests.utils import FrappeTestCase + + +class TestNonNullableDocfield(FrappeTestCase): + def setUp(self): + doc = new_doctype( + fields=[ + { + "fieldname": "test_field", + "fieldtype": "Data", + "label": "test_field", + "not_nullable": 1, + }, + ], + ) + doc.insert() + self.doctype_name = doc.name + + nullable_doc = new_doctype( + fields=[ + { + "fieldname": "test_field", + "fieldtype": "Data", + "label": "test_field", + } + ] + ) + nullable_doc.insert() + self.nullable_doctype_name = nullable_doc.name + + def test_non_nullable_field(self): + doc = frappe.new_doc(doctype=self.doctype_name) + doc.insert() + inserted_doc = frappe.db.get(self.doctype_name, {"name": doc.name}) + self.assertEqual(inserted_doc.test_field, "") + + def test_edit_field_nullable_status(self): + doc = frappe.new_doc(doctype=self.nullable_doctype_name) + doc.insert() + inserted_doc = frappe.db.get(self.nullable_doctype_name, {"name": doc.name}) + self.assertEqual(inserted_doc.test_field, None) + table = DBTable(self.nullable_doctype_name) + column_data = frappe.db.get_table_columns_description(table.table_name) + for column in column_data: + if column.name == "test_field": + self.assertEqual(column.default, "NULL") + self.assertFalse(column.not_nullable) + + doctype_doc = frappe.get_doc("DocType", self.nullable_doctype_name) + doctype_doc.fields[0].not_nullable = 1 + doctype_doc.save() + column_data = frappe.db.get_table_columns_description(table.table_name) + for column in column_data: + if column.name == "test_field": + self.assertEqual(column.default, "''") + self.assertIsNotNone(column.default) + self.assertTrue(column.not_nullable) + inserted_doc = frappe.db.get(self.nullable_doctype_name, {"name": doc.name}) + self.assertEqual(inserted_doc.test_field, "") From 12a0afd91f939bdd0b5494709dae54dee2f4100b Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Fri, 27 Oct 2023 17:09:08 +0530 Subject: [PATCH 17/47] refactor: cleanup `DbColumn.get_definition()` Signed-off-by: Akhil Narang --- frappe/database/schema.py | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/frappe/database/schema.py b/frappe/database/schema.py index 05d9753a86..a4b3773813 100644 --- a/frappe/database/schema.py +++ b/frappe/database/schema.py @@ -208,30 +208,39 @@ class DbColumn: if not column_def: return column_def + null = "" + default = "" + unique = "" + if self.fieldtype in ("Check", "Int"): default_value = cint(self.default) or 0 - column_def += f" not null default {default_value}" + null = " not null" + default = f" default {default_value}" elif self.fieldtype in ("Currency", "Float", "Percent"): default_value = flt(self.default) or 0 - column_def += f" not null default {default_value}" + null = " not null" + default = f" default {default_value}" elif ( self.default and (self.default not in frappe.db.DEFAULT_SHORTCUTS) and not cstr(self.default).startswith(":") ): - column_def += f" default {frappe.db.escape(self.default)}" + default = f" default {frappe.db.escape(self.default)}" - elif self.not_nullable: - default_value = get_not_null_defaults(self.fieldtype) - if isinstance(default_value, str): - default_value = frappe.db.escape(default_value) - column_def += f" not null default {default_value}" + if self.not_nullable and null == "": + if default == "": + default_value = get_not_null_defaults(self.fieldtype) + if isinstance(default_value, str): + default_value = frappe.db.escape(default_value) + default = f" default {default_value}" + null = " not null" if self.unique and not for_modification and (column_def not in ("text", "longtext")): - column_def += " unique" + unique = "unique" + column_def += null + default + unique return column_def def build_for_alter_table(self, current_def): From 5a4053fe2ce7b4eba511f8632d19742062f60ada Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Mon, 30 Oct 2023 13:50:14 +0530 Subject: [PATCH 18/47] chore: make use of `non_nullable` to avoid ifnull usage Signed-off-by: Akhil Narang --- frappe/core/doctype/doctype/doctype.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/doctype/doctype.py b/frappe/core/doctype/doctype/doctype.py index c3a1aa0e41..963fa380b1 100644 --- a/frappe/core/doctype/doctype/doctype.py +++ b/frappe/core/doctype/doctype/doctype.py @@ -365,16 +365,23 @@ class DocType(Document): SET `{fieldname}` = source.`{source_fieldname}` FROM `tab{link_doctype}` as source WHERE `{link_fieldname}` = source.name - AND ifnull(`{fieldname}`, '')='' """ + if df.not_nullable: + update_query += "AND `{fieldname}`=''" + else: + update_query += "AND ifnull(`{fieldname}`, '')=''" + else: update_query = """ UPDATE `tab{doctype}` as target INNER JOIN `tab{link_doctype}` as source ON `target`.`{link_fieldname}` = `source`.`name` SET `target`.`{fieldname}` = `source`.`{source_fieldname}` - WHERE ifnull(`target`.`{fieldname}`, '')="" """ + if df.not_nullable: + update_query += "WHERE `target`.`{fieldname}`=''" + else: + update_query += "WHERE ifnull(`target`.`{fieldname}`, '')=" "" self.flags.update_fields_to_fetch_queries.append( update_query.format( From 7d3295f09e7d3f0fa60686308b6130bb84e542ee Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Tue, 31 Oct 2023 15:39:49 +0530 Subject: [PATCH 19/47] fix(doctype): update query was broken Signed-off-by: Akhil Narang --- frappe/core/doctype/doctype/doctype.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/core/doctype/doctype/doctype.py b/frappe/core/doctype/doctype/doctype.py index 963fa380b1..bbdcfd1817 100644 --- a/frappe/core/doctype/doctype/doctype.py +++ b/frappe/core/doctype/doctype/doctype.py @@ -381,7 +381,7 @@ class DocType(Document): if df.not_nullable: update_query += "WHERE `target`.`{fieldname}`=''" else: - update_query += "WHERE ifnull(`target`.`{fieldname}`, '')=" "" + update_query += "WHERE ifnull(`target`.`{fieldname}`, '')=''" self.flags.update_fields_to_fetch_queries.append( update_query.format( From 4855ec76c23bfb95180e5da6197f74a86a1a52a9 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Tue, 31 Oct 2023 15:41:37 +0530 Subject: [PATCH 20/47] chore: use index to access table column instead of getattr Signed-off-by: Akhil Narang --- frappe/database/mariadb/schema.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/database/mariadb/schema.py b/frappe/database/mariadb/schema.py index 7f84fd065a..75c8fd1ab7 100644 --- a/frappe/database/mariadb/schema.py +++ b/frappe/database/mariadb/schema.py @@ -110,9 +110,9 @@ class MariaDBTable(DBTable): try: table = frappe.qb.DocType(self.doctype) frappe.qb.update(table).set(col.fieldname, default_value).where( - getattr(table, col.fieldname).isnull() + table[col.fieldname].isnull() ).run() - except Exception as e: + except Exception: print(f"Failed to update data in {self.table_name} for {col.fieldname}") raise try: From ccc2bdad524c43ba6451c481edee49a383472b92 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Tue, 31 Oct 2023 15:43:35 +0530 Subject: [PATCH 21/47] fix: go back to original transaction handling mechanism This reverts the following: 07acfeed47 chore: move check_implicit_commit() call back to its original location fc38a0b503 chore: simplify transaction handling 1e29e81543 chore: rework implicit commit checks ae0a3fd202 feat: allow ignoring implicit commit warning Signed-off-by: Akhil Narang --- frappe/database/database.py | 17 +++-------------- frappe/database/mariadb/database.py | 2 +- 2 files changed, 4 insertions(+), 15 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 6ca72088c3..d04135e827 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -95,8 +95,6 @@ class Database: self.before_rollback = CallbackManager() self.after_rollback = CallbackManager() - self.in_transaction = False - # self.db_type: str # self.last_query (lazy) attribute of last sql query executed @@ -392,8 +390,7 @@ class Database: def check_implicit_commit(self, query: str): if ( - self.in_transaction - and self.transaction_writes + self.transaction_writes and query and is_query_type(query, ("start", "alter", "drop", "create", "begin", "truncate")) ): @@ -963,24 +960,16 @@ class Database: read_only = read_only or frappe.flags.read_only mode = "READ ONLY" if read_only else "" self.sql(f"START TRANSACTION {mode}") - self.in_transaction = True - def commit(self, new_transaction: bool = True): + def commit(self): """Commit current transaction. Calls SQL `COMMIT`.""" - if not self.in_transaction: - self.logger.warn("Not in transaction, ignoring commit") - return - self.before_rollback.reset() self.after_rollback.reset() self.before_commit.run() self.sql("commit") - self.in_transaction = False - - if new_transaction: - self.begin() # explicitly start a new transaction + self.begin() # explicitly start a new transaction self.after_commit.run() diff --git a/frappe/database/mariadb/database.py b/frappe/database/mariadb/database.py index d8b4555a20..1f087a243a 100644 --- a/frappe/database/mariadb/database.py +++ b/frappe/database/mariadb/database.py @@ -438,7 +438,7 @@ class MariaDBDatabase(MariaDBConnectionUtil, MariaDBExceptionUtil, Database): db_table = MariaDBTable(doctype, meta) db_table.validate() - self.commit(new_transaction=False) + self.commit() db_table.sync() self.begin() From 8d540963c88e1dcfa7536e7d54402ecbedf09d56 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Tue, 31 Oct 2023 16:17:00 +0530 Subject: [PATCH 22/47] refactor(mariadb/schema): use sql_ddl() method Signed-off-by: Akhil Narang --- frappe/database/mariadb/schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/database/mariadb/schema.py b/frappe/database/mariadb/schema.py index 75c8fd1ab7..a26a5310a0 100644 --- a/frappe/database/mariadb/schema.py +++ b/frappe/database/mariadb/schema.py @@ -121,7 +121,7 @@ class MariaDBTable(DBTable): query_body = ", ".join(query_parts) query = f"ALTER TABLE `{self.table_name}` {query_body}" # nosemgrep - frappe.db.sql(query) + frappe.db.sql_ddl(query) except Exception as e: if query := locals().get("query"): # this weirdness is to avoid potentially unbounded vars From 8d91e4524f924b8380710e0386e4d736c83a8d5e Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Tue, 31 Oct 2023 16:17:41 +0530 Subject: [PATCH 23/47] feat(exporter): handle docfield being non-nullable Signed-off-by: Akhil Narang --- frappe/types/exporter.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frappe/types/exporter.py b/frappe/types/exporter.py index 97551e0c01..cbdce7c6e0 100644 --- a/frappe/types/exporter.py +++ b/frappe/types/exporter.py @@ -162,6 +162,9 @@ class TypeExporter: if field.fieldtype in non_nullable_types: return False + if field.not_nullable: + return False + return not bool(field.reqd) def _generic_parameters(self, field) -> str | None: From 10ad99869af479e92f3bde5a8b03ea35b3262a8b Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Tue, 31 Oct 2023 16:50:22 +0530 Subject: [PATCH 24/47] feat: use user-specified default value if passed Signed-off-by: Akhil Narang --- frappe/model/base_document.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frappe/model/base_document.py b/frappe/model/base_document.py index 264ec219b3..bdf354ce99 100644 --- a/frappe/model/base_document.py +++ b/frappe/model/base_document.py @@ -387,7 +387,10 @@ class BaseDocument: # If the docfield is not nullable - set a default non-null value if value is None and getattr(df, "not_nullable", False): - value = get_not_null_defaults(df.fieldtype) + if df.default: + value = df.default + else: + value = get_not_null_defaults(df.fieldtype) d[fieldname] = value From 731c5c8cd5c9453b8c3fd764c39c03007868afe7 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 2 Nov 2023 16:01:04 +0530 Subject: [PATCH 25/47] refactor(db_query): check for docfield not_nullable Signed-off-by: Akhil Narang --- frappe/model/db_query.py | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 78adfffe76..76a03f5a76 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -733,12 +733,15 @@ class DatabaseQuery: f.update(get_additional_filter_field(additional_filters_config, f, f.value)) meta = frappe.get_meta(f.doctype) + df = meta.get("fields", {"fieldname": f.fieldname}) + df = df[0] if df else None + can_be_null = True + value = None + # prepare in condition if f.operator.lower() in NestedSetHierarchy: - values = f.value or "" - # TODO: handle list and tuple # if not isinstance(values, (list, tuple)): # values = values.split(",") @@ -784,28 +787,28 @@ class DatabaseQuery: "not in" if f.operator.lower() in ("not ancestors of", "not descendants of") else "in" ) - elif f.operator.lower() in ("in", "not in"): + if f.operator.lower() in ("in", "not in"): # if values contain '' or falsy values then only coalesce column # for `in` query this is only required if values contain '' or values are empty. # for `not in` queries we can't be sure as column values might contain null. + can_be_null = not getattr(df, "not_nullable", False) if f.operator.lower() == "in": - can_be_null = not f.value or any(v is None or v == "" for v in f.value) + can_be_null &= not f.value or any(v is None or v == "" for v in f.value) - values = f.value or "" - if isinstance(values, str): - values = values.split(",") + if value is None: + values = f.value or "" + if isinstance(values, str): + values = values.split(",") - fallback = "''" - value = [frappe.db.escape((cstr(v) or "").strip(), percent=False) for v in values] - if len(value): - value = f"({', '.join(value)})" - else: - value = "('')" + fallback = "''" + value = [frappe.db.escape((cstr(v) or "").strip(), percent=False) for v in values] + if len(value): + value = f"({', '.join(value)})" + else: + value = "('')" else: escape = True - df = meta.get("fields", {"fieldname": f.fieldname}) - df = df[0] if df else None if df and ( df.fieldtype in ("Check", "Float", "Int", "Currency", "Percent") @@ -843,7 +846,7 @@ class DatabaseQuery: elif f.value == "not set": f.operator = "=" fallback = "''" - can_be_null = True + can_be_null = not getattr(df, "not_nullable", False) value = "" From 558ac413cef660f0005d3aaf7a0bace117c4fc09 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 2 Nov 2023 18:40:31 +0530 Subject: [PATCH 26/47] fix(mariadb/schema): use correct default value Signed-off-by: Akhil Narang --- frappe/database/mariadb/schema.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/frappe/database/mariadb/schema.py b/frappe/database/mariadb/schema.py index a26a5310a0..ee2262a012 100644 --- a/frappe/database/mariadb/schema.py +++ b/frappe/database/mariadb/schema.py @@ -3,7 +3,6 @@ from pymysql.constants.ER import DUP_ENTRY import frappe from frappe import _ from frappe.database.schema import DBTable -from frappe.utils.defaults import get_not_null_defaults class MariaDBTable(DBTable): @@ -104,12 +103,10 @@ class MariaDBTable(DBTable): drop_index_query.append(f"DROP INDEX `{index_record.Key_name}`") for col in self.change_nullability: - current_column = self.current_columns.get(col.fieldname.lower()) if col.not_nullable: - default_value = get_not_null_defaults(col.fieldtype) try: table = frappe.qb.DocType(self.doctype) - frappe.qb.update(table).set(col.fieldname, default_value).where( + frappe.qb.update(table).set(col.fieldname, col.default).where( table[col.fieldname].isnull() ).run() except Exception: From b901f372670357ffcf5b818c69650c1cd6b2307f Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 2 Nov 2023 18:41:28 +0530 Subject: [PATCH 27/47] feat(postgres/schema): add support for setting fields as not-nullable Signed-off-by: Akhil Narang --- frappe/database/postgres/schema.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/frappe/database/postgres/schema.py b/frappe/database/postgres/schema.py index 073fafd8c7..c4afc7f70b 100644 --- a/frappe/database/postgres/schema.py +++ b/frappe/database/postgres/schema.py @@ -139,6 +139,23 @@ class PostgresTable(DBTable): if col.fieldname != "name": # if index key exists drop_contraint_query += f'DROP INDEX IF EXISTS "unique_{col.fieldname}" ;' + + for col in self.change_nullability: + default = "" + if col.default: + default = f"DEFAULT {frappe.db.escape(col.default)}" + query.append( + f"ALTER COLUMN \"{col.fieldname}\" {'DROP' if col.not_nullable else 'SET'} NOT NULL {default}" + ) + if col.not_nullable: + try: + table = frappe.qb.DocType(self.doctype) + frappe.qb.update(table).set(col.fieldname, col.default).where( + table[col.fieldname].isnull() + ).run() + except Exception: + print(f"Failed to update data in {self.table_name} for {col.fieldname}") + raise try: if query: final_alter_query = "ALTER TABLE `{}` {}".format(self.table_name, ", ".join(query)) From 5598fe241ee4fc62e5766bf12be56e4d5c32d58e Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Wed, 8 Nov 2023 15:36:20 +0530 Subject: [PATCH 28/47] fix: use default default if a custom default isn't specified Signed-off-by: Akhil Narang --- frappe/database/mariadb/schema.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/frappe/database/mariadb/schema.py b/frappe/database/mariadb/schema.py index ee2262a012..d57335f589 100644 --- a/frappe/database/mariadb/schema.py +++ b/frappe/database/mariadb/schema.py @@ -3,6 +3,7 @@ from pymysql.constants.ER import DUP_ENTRY import frappe from frappe import _ from frappe.database.schema import DBTable +from frappe.utils.defaults import get_not_null_defaults class MariaDBTable(DBTable): @@ -106,9 +107,9 @@ class MariaDBTable(DBTable): if col.not_nullable: try: table = frappe.qb.DocType(self.doctype) - frappe.qb.update(table).set(col.fieldname, col.default).where( - table[col.fieldname].isnull() - ).run() + frappe.qb.update(table).set( + col.fieldname, col.default or get_not_null_defaults(col.fieldtype) + ).where(table[col.fieldname].isnull()).run() except Exception: print(f"Failed to update data in {self.table_name} for {col.fieldname}") raise From 75c8445b6a01e82330804f0732aca6cf9caf6c8f Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Wed, 8 Nov 2023 16:16:53 +0530 Subject: [PATCH 29/47] refactor: cleanup column definition Signed-off-by: Akhil Narang --- frappe/database/schema.py | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/frappe/database/schema.py b/frappe/database/schema.py index a4b3773813..621513bf99 100644 --- a/frappe/database/schema.py +++ b/frappe/database/schema.py @@ -208,39 +208,43 @@ class DbColumn: if not column_def: return column_def - null = "" + null = True default = "" - unique = "" + unique = False if self.fieldtype in ("Check", "Int"): - default_value = cint(self.default) or 0 - null = " not null" - default = f" default {default_value}" + default = cint(self.default) or 0 + null = False elif self.fieldtype in ("Currency", "Float", "Percent"): - default_value = flt(self.default) or 0 - null = " not null" - default = f" default {default_value}" + default = flt(self.default) or 0 + null = False elif ( self.default and (self.default not in frappe.db.DEFAULT_SHORTCUTS) and not cstr(self.default).startswith(":") ): - default = f" default {frappe.db.escape(self.default)}" + default = frappe.db.escape(self.default) if self.not_nullable and null == "": if default == "": - default_value = get_not_null_defaults(self.fieldtype) - if isinstance(default_value, str): - default_value = frappe.db.escape(default_value) - default = f" default {default_value}" - null = " not null" + default = get_not_null_defaults(self.fieldtype) + if isinstance(default, str): + default = frappe.db.escape(default) + null = False if self.unique and not for_modification and (column_def not in ("text", "longtext")): - unique = "unique" + unique = True - column_def += null + default + unique + if not null: + column_def += " NOT NULL" + + if default: + column_def += f" DEFAULT {default}" + + if unique: + column_def += " UNIQUE" return column_def def build_for_alter_table(self, current_def): From ea7f611745129c1703acc3e6c371f49748cd6338 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Wed, 8 Nov 2023 17:21:12 +0530 Subject: [PATCH 30/47] fix: adapt tests to actually work Couldn't seem to sanely get postgres default state Signed-off-by: Akhil Narang --- frappe/tests/test_non_nullable_docfield.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/frappe/tests/test_non_nullable_docfield.py b/frappe/tests/test_non_nullable_docfield.py index a02b292670..d79e6cde64 100644 --- a/frappe/tests/test_non_nullable_docfield.py +++ b/frappe/tests/test_non_nullable_docfield.py @@ -43,20 +43,20 @@ class TestNonNullableDocfield(FrappeTestCase): inserted_doc = frappe.db.get(self.nullable_doctype_name, {"name": doc.name}) self.assertEqual(inserted_doc.test_field, None) table = DBTable(self.nullable_doctype_name) - column_data = frappe.db.get_table_columns_description(table.table_name) - for column in column_data: + query = "SELECT column_name AS name, column_default is NULL AS default_null,is_nullable = 'NO' AS not_nullable FROM information_schema.columns WHERE table_name=%s" + for column in frappe.db.sql(query, table.table_name, as_dict=True): if column.name == "test_field": - self.assertEqual(column.default, "NULL") self.assertFalse(column.not_nullable) doctype_doc = frappe.get_doc("DocType", self.nullable_doctype_name) - doctype_doc.fields[0].not_nullable = 1 + for field in doctype_doc.fields: + if field.fieldname == "test_field": + field.not_nullable = 1 + break doctype_doc.save() - column_data = frappe.db.get_table_columns_description(table.table_name) - for column in column_data: + for column in frappe.db.sql(query, table.table_name, as_dict=True): if column.name == "test_field": - self.assertEqual(column.default, "''") - self.assertIsNotNone(column.default) + self.assertFalse(column.default_null) self.assertTrue(column.not_nullable) inserted_doc = frappe.db.get(self.nullable_doctype_name, {"name": doc.name}) self.assertEqual(inserted_doc.test_field, "") From e3f643724501e42483ebb65b5e27c6232e5ad9b7 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Tue, 7 Nov 2023 16:10:14 +0530 Subject: [PATCH 31/47] fix(postgres): properly change nullability and default values Signed-off-by: Akhil Narang --- frappe/database/postgres/schema.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/frappe/database/postgres/schema.py b/frappe/database/postgres/schema.py index c4afc7f70b..fa00c2fe78 100644 --- a/frappe/database/postgres/schema.py +++ b/frappe/database/postgres/schema.py @@ -2,6 +2,7 @@ import frappe from frappe import _ from frappe.database.schema import DBTable, get_definition from frappe.utils import cint, flt +from frappe.utils.defaults import get_not_null_defaults class PostgresTable(DBTable): @@ -45,7 +46,7 @@ class PostgresTable(DBTable): docstatus smallint not null default '0', idx bigint not null default '0', {additional_definitions} - )""" + )""", ) self.create_indexes() @@ -140,19 +141,21 @@ class PostgresTable(DBTable): # if index key exists drop_contraint_query += f'DROP INDEX IF EXISTS "unique_{col.fieldname}" ;' + change_nullability = [] for col in self.change_nullability: - default = "" - if col.default: - default = f"DEFAULT {frappe.db.escape(col.default)}" - query.append( - f"ALTER COLUMN \"{col.fieldname}\" {'DROP' if col.not_nullable else 'SET'} NOT NULL {default}" + change_nullability.append( + f"ALTER COLUMN \"{col.fieldname}\" {'SET' if col.not_nullable else 'DROP'} NOT NULL" ) + change_nullability.append( + f'ALTER COLUMN "{col.fieldname}" SET DEFAULT {col.default or frappe.db.escape(col.default)}' + ) + if col.not_nullable: try: table = frappe.qb.DocType(self.doctype) - frappe.qb.update(table).set(col.fieldname, col.default).where( - table[col.fieldname].isnull() - ).run() + frappe.qb.update(table).set( + col.fieldname, col.default or get_not_null_defaults(col.fieldtype) + ).where(table[col.fieldname].isnull()).run() except Exception: print(f"Failed to update data in {self.table_name} for {col.fieldname}") raise @@ -161,6 +164,9 @@ class PostgresTable(DBTable): final_alter_query = "ALTER TABLE `{}` {}".format(self.table_name, ", ".join(query)) # nosemgrep frappe.db.sql(final_alter_query) + if change_nullability: + # nosemgrep + frappe.db.sql(f"ALTER TABLE `{self.table_name}` {','.join(change_nullability)}") if create_contraint_query: # nosemgrep frappe.db.sql(create_contraint_query) From e1bbcd73abb499e4d30407836a2e5dd614cfb3a9 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 16 Nov 2023 13:39:42 +0530 Subject: [PATCH 32/47] fix: 0 is falsy, set correct defaults Signed-off-by: Akhil Narang --- frappe/database/postgres/schema.py | 7 ++++--- frappe/database/schema.py | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/frappe/database/postgres/schema.py b/frappe/database/postgres/schema.py index fa00c2fe78..946747aa47 100644 --- a/frappe/database/postgres/schema.py +++ b/frappe/database/postgres/schema.py @@ -143,12 +143,13 @@ class PostgresTable(DBTable): change_nullability = [] for col in self.change_nullability: + default = col.default or get_not_null_defaults(col.fieldtype) + if isinstance(default, str): + default = frappe.db.escape(default) change_nullability.append( f"ALTER COLUMN \"{col.fieldname}\" {'SET' if col.not_nullable else 'DROP'} NOT NULL" ) - change_nullability.append( - f'ALTER COLUMN "{col.fieldname}" SET DEFAULT {col.default or frappe.db.escape(col.default)}' - ) + change_nullability.append(f'ALTER COLUMN "{col.fieldname}" SET DEFAULT {default}') if col.not_nullable: try: diff --git a/frappe/database/schema.py b/frappe/database/schema.py index 621513bf99..497336cd7b 100644 --- a/frappe/database/schema.py +++ b/frappe/database/schema.py @@ -209,7 +209,7 @@ class DbColumn: return column_def null = True - default = "" + default = None unique = False if self.fieldtype in ("Check", "Int"): @@ -240,7 +240,7 @@ class DbColumn: if not null: column_def += " NOT NULL" - if default: + if default is not None: column_def += f" DEFAULT {default}" if unique: From 5c023d238d75f9856d96d8c214718f9beeb9fa35 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 16 Nov 2023 15:53:56 +0530 Subject: [PATCH 33/47] fix: get correct column definition Signed-off-by: Akhil Narang --- frappe/database/schema.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/database/schema.py b/frappe/database/schema.py index 497336cd7b..90c3055452 100644 --- a/frappe/database/schema.py +++ b/frappe/database/schema.py @@ -227,8 +227,8 @@ class DbColumn: ): default = frappe.db.escape(self.default) - if self.not_nullable and null == "": - if default == "": + if self.not_nullable and null: + if default is None: default = get_not_null_defaults(self.fieldtype) if isinstance(default, str): default = frappe.db.escape(default) From 94b2e509b9944b7ac26e5ff92f25846f355a8761 Mon Sep 17 00:00:00 2001 From: ruthra kumar Date: Mon, 20 Nov 2023 10:11:42 +0530 Subject: [PATCH 34/47] fix: dont rename link fields in Virtual doctypes --- frappe/model/rename_doc.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/frappe/model/rename_doc.py b/frappe/model/rename_doc.py index e6cc83874d..9bad8f1028 100644 --- a/frappe/model/rename_doc.py +++ b/frappe/model/rename_doc.py @@ -464,11 +464,12 @@ def get_link_fields(doctype: str) -> list[dict]: cf = frappe.qb.DocType("Custom Field") ps = frappe.qb.DocType("Property Setter") - st_issingle = frappe.qb.from_(dt).select(dt.issingle).where(dt.name == df.parent).as_("issingle") standard_fields = ( frappe.qb.from_(df) - .select(df.parent, df.fieldname, st_issingle) - .where((df.options == doctype) & (df.fieldtype == "Link")) + .inner_join(dt) + .on(df.parent == dt.name) + .select(df.parent, df.fieldname, dt.issingle.as_("issingle")) + .where((df.options == doctype) & (df.fieldtype == "Link") & (dt.is_virtual == 0)) .run(as_dict=True) ) From 17ff6998dabc78ab9c489f263b48b238dcd950e3 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 20 Nov 2023 10:16:29 +0530 Subject: [PATCH 35/47] fix: ignore invalid token so auth hooks can apply The error will still be raised some 2-3 lines of execution later --- frappe/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/auth.py b/frappe/auth.py index f3fbe272c5..941edb9277 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -647,7 +647,7 @@ def validate_auth_via_api_keys(authorization_header): frappe.InvalidAuthorizationToken, ) except (AttributeError, TypeError, ValueError): - raise frappe.AuthenticationError + pass def validate_api_key_secret(api_key, api_secret, frappe_authorization_source=None): From 9949bf47b72ef066926daf7990879ee46bfd8453 Mon Sep 17 00:00:00 2001 From: Shariq Ansari <30859809+shariquerik@users.noreply.github.com> Date: Mon, 20 Nov 2023 11:42:19 +0530 Subject: [PATCH 36/47] chore: linter fix --- frappe/public/js/frappe/web_form/web_form_list.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/web_form/web_form_list.js b/frappe/public/js/frappe/web_form/web_form_list.js index 67a29319ca..388e81677f 100644 --- a/frappe/public/js/frappe/web_form/web_form_list.js +++ b/frappe/public/js/frappe/web_form/web_form_list.js @@ -110,7 +110,7 @@ export default class WebFormList { fetch_data() { if (this.condition_json && JSON.parse(this.condition_json)) { - let filter = frappe.utils.get_filter_from_json(this.condition_json,this.doctype); + let filter = frappe.utils.get_filter_from_json(this.condition_json, this.doctype); filter = frappe.utils.get_filter_as_json(filter); this.filters = Object.assign(this.filters, JSON.parse(filter)); } From c0d47d8ddd1549856cba12f922e208fa58e369fa Mon Sep 17 00:00:00 2001 From: Shariq Ansari <30859809+shariquerik@users.noreply.github.com> Date: Mon, 20 Nov 2023 11:46:09 +0530 Subject: [PATCH 37/47] chore: linter fix --- frappe/desk/doctype/notification_log/notification_log.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/desk/doctype/notification_log/notification_log.py b/frappe/desk/doctype/notification_log/notification_log.py index e459a63ef8..105edd93d6 100644 --- a/frappe/desk/doctype/notification_log/notification_log.py +++ b/frappe/desk/doctype/notification_log/notification_log.py @@ -133,7 +133,7 @@ def send_notification_email(doc): recipients=email, subject=email_subject, template="new_notification", - args={ + args = { "body_content": doc.subject, "description": doc.email_content, "document_type": doc.document_type, From c3b0da60d1d3c5f109b0bc7670c58870584366d6 Mon Sep 17 00:00:00 2001 From: sibi kumar k <95605794+sibikumarkuppusamy@users.noreply.github.com> Date: Mon, 20 Nov 2023 11:56:05 +0530 Subject: [PATCH 38/47] feat: Add image view in Workspace shortcut block (#23041) * Feat:Add image view in Workspace creation * image view condition --- frappe/desk/doctype/workspace_shortcut/workspace_shortcut.json | 2 +- frappe/public/js/frappe/utils/utils.js | 3 +++ frappe/public/js/frappe/widgets/widget_dialog.js | 3 ++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/frappe/desk/doctype/workspace_shortcut/workspace_shortcut.json b/frappe/desk/doctype/workspace_shortcut/workspace_shortcut.json index e494aad152..854305ad80 100644 --- a/frappe/desk/doctype/workspace_shortcut/workspace_shortcut.json +++ b/frappe/desk/doctype/workspace_shortcut/workspace_shortcut.json @@ -44,7 +44,7 @@ "fieldtype": "Select", "in_list_view": 1, "label": "DocType View", - "options": "\nList\nReport Builder\nDashboard\nTree\nNew\nCalendar\nKanban" + "options": "\nList\nReport Builder\nDashboard\nTree\nNew\nCalendar\nKanban\nImage" }, { "fieldname": "column_break_4", diff --git a/frappe/public/js/frappe/utils/utils.js b/frappe/public/js/frappe/utils/utils.js index 0a18ba1edc..b8458fb3c1 100644 --- a/frappe/public/js/frappe/utils/utils.js +++ b/frappe/public/js/frappe/utils/utils.js @@ -1300,6 +1300,9 @@ Object.assign(frappe.utils, { route += `/${item.kanban_board}`; } break; + case "Image": + route = `${doctype_slug}/view/image`; + break; default: route = doctype_slug; } diff --git a/frappe/public/js/frappe/widgets/widget_dialog.js b/frappe/public/js/frappe/widgets/widget_dialog.js index e22e56c47f..abe7352d9d 100644 --- a/frappe/public/js/frappe/widgets/widget_dialog.js +++ b/frappe/public/js/frappe/widgets/widget_dialog.js @@ -396,6 +396,7 @@ class ShortcutDialog extends WidgetDialog { const views = ["List", "Report Builder", "Dashboard", "New"]; if (meta.is_tree === 1) views.push("Tree"); + if (meta.image_field) views.push("Image"); if (frappe.boot.calendars.includes(doctype)) views.push("Calendar"); const response = await frappe.db.get_value( @@ -427,7 +428,7 @@ class ShortcutDialog extends WidgetDialog { fieldtype: "Select", fieldname: "doc_view", label: "DocType View", - options: "List\nReport Builder\nDashboard\nTree\nNew\nCalendar\nKanban", + options: "List\nReport Builder\nDashboard\nTree\nNew\nCalendar\nKanban\nImage", description: __( "Which view of the associated DocType should this shortcut take you to?" ), From eb45da391325766ab2c74b84a648192a78da08e3 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Mon, 20 Nov 2023 12:45:41 +0530 Subject: [PATCH 39/47] feat: Allow usage of `print()` within `safe_exec()` (#23084) * feat(safe_exec): allow usage of `print()` Signed-off-by: Akhil Narang * refactor(system_console): update description to mention `print()` instead of `log()` Signed-off-by: Akhil Narang * feat: unconditionally add debug logs to response if present Signed-off-by: Akhil Narang * chore(safe_exec): add in a test for running `print()` within safe_exec Signed-off-by: Akhil Narang * fix(safe_exec): ignore warning RestrictedPython warns us if we call `print()` don't use their `printed` variable Signed-off-by: Akhil Narang * feat: store debug logs from scheduled jobs Signed-off-by: Akhil Narang * fix: avoid ignoring warnings, disabled in prod anyway * chore: remove unnecessary logging This can be moved to level 2 when required --------- Signed-off-by: Akhil Narang Co-authored-by: Ankush Menat --- frappe/__init__.py | 2 +- .../scheduled_job_log/scheduled_job_log.json | 11 +++++++++-- .../scheduled_job_log/scheduled_job_log.py | 1 + .../scheduled_job_type/scheduled_job_type.py | 2 ++ .../doctype/system_console/system_console.json | 4 ++-- frappe/handler.py | 1 - frappe/public/js/frappe/request.js | 2 +- frappe/tests/test_api.py | 4 ++-- frappe/tests/test_safe_exec.py | 5 +++++ frappe/utils/response.py | 4 ++-- frappe/utils/safe_exec.py | 16 +++++++++++++++- 11 files changed, 40 insertions(+), 12 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index eb9c502905..90f142c14c 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -429,7 +429,7 @@ def print_sql(enable: bool = True) -> None: def log(msg: str) -> None: - """Add to `debug_log`. + """Add to `debug_log` :param msg: Message.""" if not request: diff --git a/frappe/core/doctype/scheduled_job_log/scheduled_job_log.json b/frappe/core/doctype/scheduled_job_log/scheduled_job_log.json index 451c4108a0..782c0749f8 100644 --- a/frappe/core/doctype/scheduled_job_log/scheduled_job_log.json +++ b/frappe/core/doctype/scheduled_job_log/scheduled_job_log.json @@ -7,7 +7,8 @@ "field_order": [ "status", "scheduled_job_type", - "details" + "details", + "debug_log" ], "fields": [ { @@ -35,10 +36,16 @@ "options": "Scheduled Job Type", "read_only": 1, "reqd": 1 + }, + { + "fieldname": "debug_log", + "fieldtype": "Code", + "label": "Debug Log", + "read_only": 1 } ], "links": [], - "modified": "2022-06-13 05:41:21.090972", + "modified": "2023-11-09 12:06:41.781270", "modified_by": "Administrator", "module": "Core", "name": "Scheduled Job Log", diff --git a/frappe/core/doctype/scheduled_job_log/scheduled_job_log.py b/frappe/core/doctype/scheduled_job_log/scheduled_job_log.py index 40f6823057..e4bfe21e2d 100644 --- a/frappe/core/doctype/scheduled_job_log/scheduled_job_log.py +++ b/frappe/core/doctype/scheduled_job_log/scheduled_job_log.py @@ -16,6 +16,7 @@ class ScheduledJobLog(Document): if TYPE_CHECKING: from frappe.types import DF + debug_log: DF.Code | None details: DF.Code | None scheduled_job_type: DF.Link status: DF.Literal["Scheduled", "Complete", "Failed"] diff --git a/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py b/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py index 6f56180c89..d6ec8bfef6 100644 --- a/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py +++ b/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py @@ -145,6 +145,8 @@ class ScheduledJobType(Document): dict(doctype="Scheduled Job Log", scheduled_job_type=self.name) ).insert(ignore_permissions=True) self.scheduler_log.db_set("status", status) + if frappe.debug_log: + self.scheduler_log.db_set("debug_log", "\n".join(frappe.debug_log)) if status == "Failed": self.scheduler_log.db_set("details", frappe.get_traceback()) if status == "Start": diff --git a/frappe/desk/doctype/system_console/system_console.json b/frappe/desk/doctype/system_console/system_console.json index a851831909..9573e23b52 100644 --- a/frappe/desk/doctype/system_console/system_console.json +++ b/frappe/desk/doctype/system_console/system_console.json @@ -29,7 +29,7 @@ ], "fields": [ { - "description": "To print output use log(text)", + "description": "To print output use print(text)", "fieldname": "console", "fieldtype": "Code", "label": "Console", @@ -86,7 +86,7 @@ "index_web_pages_for_search": 1, "issingle": 1, "links": [], - "modified": "2022-04-15 14:15:58.398590", + "modified": "2023-11-03 13:02:00.706806", "modified_by": "Administrator", "module": "Desk", "name": "System Console", diff --git a/frappe/handler.py b/frappe/handler.py index 6db6a7600f..d889c67b23 100644 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -287,7 +287,6 @@ def get_attr(cmd): f"Calling shorthand for {cmd} is deprecated, please specify full path in RPC call." ) method = globals()[cmd] - frappe.log("method:" + cmd) return method diff --git a/frappe/public/js/frappe/request.js b/frappe/public/js/frappe/request.js index 10a7158e4a..f0195089f8 100644 --- a/frappe/public/js/frappe/request.js +++ b/frappe/public/js/frappe/request.js @@ -483,8 +483,8 @@ frappe.request.cleanup = function (opts, r) { if (opts.args) { console.log("======== arguments ========"); console.log(opts.args); - console.log("========"); } + console.log("======== debug messages ========"); $.each(JSON.parse(r._debug_messages), function (i, v) { console.log(v); }); diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index b415a57f77..a5c76e2698 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -282,7 +282,7 @@ class TestMethodAPI(FrappeAPITestCase): response = self.get(self.method_path("frappe.auth.get_logged_user")) self.assertEqual(response.status_code, 401) - authorization_token = f"NonExistentKey:INCORRECT" + authorization_token = "NonExistentKey:INCORRECT" response = self.get(self.method_path("frappe.auth.get_logged_user")) self.assertEqual(response.status_code, 401) @@ -382,7 +382,7 @@ def after_request(*args, **kwargs): class TestResponse(FrappeAPITestCase): def test_generate_pdf(self): response = self.get( - f"/api/method/frappe.utils.print_format.download_pdf", + "/api/method/frappe.utils.print_format.download_pdf", {"sid": self.sid, "doctype": "User", "name": "Guest"}, ) self.assertEqual(response.status_code, 200) diff --git a/frappe/tests/test_safe_exec.py b/frappe/tests/test_safe_exec.py index ba38cc93e9..3542fdfce1 100644 --- a/frappe/tests/test_safe_exec.py +++ b/frappe/tests/test_safe_exec.py @@ -121,6 +121,11 @@ class TestSafeExec(FrappeTestCase): # dont Allow modifying _dict class self.assertRaises(Exception, safe_exec, "_dict.x = 1") + def test_print(self): + test_str = frappe.generate_hash() + safe_exec(f"print('{test_str}')") + self.assertEqual(frappe.local.debug_log[-1], test_str) + class TestNoSafeExec(FrappeTestCase): def test_safe_exec_disabled_by_default(self): diff --git a/frappe/utils/response.py b/frappe/utils/response.py index 3fcf1a365e..61372eee23 100644 --- a/frappe/utils/response.py +++ b/frappe/utils/response.py @@ -175,7 +175,7 @@ def _make_logs_v1(): if frappe.local.message_log: response["_server_messages"] = json.dumps([json.dumps(d) for d in frappe.local.message_log]) - if frappe.debug_log and frappe.conf.get("logging"): + if frappe.debug_log: response["_debug_messages"] = json.dumps(frappe.local.debug_log) if frappe.flags.error_message: @@ -188,7 +188,7 @@ def _make_logs_v2(): if frappe.local.message_log: response["messages"] = frappe.local.message_log - if frappe.debug_log and frappe.conf.get("logging"): + if frappe.debug_log: response["debug"] = [{"message": m} for m in frappe.local.debug_log] diff --git a/frappe/utils/safe_exec.py b/frappe/utils/safe_exec.py index 0b8435b100..f8ea926365 100644 --- a/frappe/utils/safe_exec.py +++ b/frappe/utils/safe_exec.py @@ -1,6 +1,7 @@ import ast import copy import inspect +import io import json import mimetypes import types @@ -8,7 +9,7 @@ from contextlib import contextmanager from functools import lru_cache import RestrictedPython.Guards -from RestrictedPython import compile_restricted, safe_globals +from RestrictedPython import PrintCollector, compile_restricted, safe_globals from RestrictedPython.transformer import RestrictingNodeTransformer import frappe @@ -60,6 +61,16 @@ class FrappeTransformer(RestrictingNodeTransformer): return super().check_name(node, name, *args, **kwargs) +class FrappePrintCollector(PrintCollector): + """Collect written text, and return it when called.""" + + def _call_print(self, *objects, **kwargs): + output = io.StringIO() + print(*objects, file=output, **kwargs) + frappe.log(output.getvalue().strip()) + output.close() + + def is_safe_exec_enabled() -> bool: # server scripts can only be enabled via common_site_config.json return bool(frappe.get_common_site_config().get(SAFE_EXEC_CONFIG_KEY)) @@ -261,6 +272,9 @@ def get_safe_globals(): out._getitem_ = _getitem out._getattr_ = _getattr_for_safe_exec + # Allow using `print()` calls with `safe_exec()` + out._print_ = FrappePrintCollector + # allow iterators and list comprehension out._getiter_ = iter out._iter_unpack_sequence_ = RestrictedPython.Guards.guarded_iter_unpack_sequence From 978b9f4f30619504d6f403e49bdd0ba0bf699512 Mon Sep 17 00:00:00 2001 From: Shariq Ansari <30859809+shariquerik@users.noreply.github.com> Date: Mon, 20 Nov 2023 15:38:25 +0530 Subject: [PATCH 40/47] chore: cleaner code --- frappe/public/js/frappe/model/workflow.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/frappe/public/js/frappe/model/workflow.js b/frappe/public/js/frappe/model/workflow.js index 11643b878d..359b9ba310 100644 --- a/frappe/public/js/frappe/model/workflow.js +++ b/frappe/public/js/frappe/model/workflow.js @@ -12,12 +12,8 @@ frappe.workflow = { if (wf.length) { frappe.workflow.workflows[doctype] = wf[0]; frappe.workflow.state_fields[doctype] = wf[0].workflow_state_field; - frappe.workflow.avoid_status_override[doctype] = frappe.workflow.workflows[ - doctype - ].states - .filter((row) => { - if (row.avoid_status_override) return true; - }) + frappe.workflow.avoid_status_override[doctype] = wf[0].states + .filter((row) => row.avoid_status_override) .map((d) => d.state); } else { frappe.workflow.state_fields[doctype] = null; From 8b03a48f2424cfe5bbf2ad953a51b55be930adea Mon Sep 17 00:00:00 2001 From: Corentin Flr <10946971+cogk@users.noreply.github.com> Date: Mon, 20 Nov 2023 12:00:47 +0100 Subject: [PATCH 41/47] fix(WebForm): Replace `get_cached_value` with `get_value` frappe.get_cached_value does not support filters --- frappe/website/doctype/web_form/web_form.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/website/doctype/web_form/web_form.py b/frappe/website/doctype/web_form/web_form.py index 8e6156dfbb..95684dee99 100644 --- a/frappe/website/doctype/web_form/web_form.py +++ b/frappe/website/doctype/web_form/web_form.py @@ -684,7 +684,7 @@ def get_link_options(web_form_name, doctype, allow_read_on_all_link_options=Fals frappe.get_cached_value("DocType", doctype, "show_title_field_in_link") == 1 ) if not show_title_field_in_link: - value = frappe.get_cached_value( + value = frappe.db.get_value( "Property Setter", fieldname="value", filters={"property": "show_title_field_in_link", "doc_type": doctype}, From c6371cbdd0ce724b69590bd19478b3a46ef265ab Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 20 Nov 2023 17:29:00 +0530 Subject: [PATCH 42/47] fix: increase length for address lines 140 char default limit is often not enough as demonstrated here https://github.com/frappe/frappe/issues/23259 --- frappe/contacts/doctype/address/address.json | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frappe/contacts/doctype/address/address.json b/frappe/contacts/doctype/address/address.json index 0ec67103a7..54d6b03739 100644 --- a/frappe/contacts/doctype/address/address.json +++ b/frappe/contacts/doctype/address/address.json @@ -51,12 +51,14 @@ "fieldname": "address_line1", "fieldtype": "Data", "label": "Address Line 1", + "length": 240, "reqd": 1 }, { "fieldname": "address_line2", "fieldtype": "Data", - "label": "Address Line 2" + "label": "Address Line 2", + "length": 240 }, { "fieldname": "city", @@ -148,7 +150,7 @@ "icon": "fa fa-map-marker", "idx": 5, "links": [], - "modified": "2023-10-30 05:50:23.912366", + "modified": "2023-11-20 17:28:41.698356", "modified_by": "Administrator", "module": "Contacts", "name": "Address", From 97ae9667d941b572822d85dc430bac2d086d8438 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 20 Nov 2023 18:57:30 +0000 Subject: [PATCH 43/47] build(deps): bump dessant/lock-threads from 4 to 5 Bumps [dessant/lock-threads](https://github.com/dessant/lock-threads) from 4 to 5. - [Release notes](https://github.com/dessant/lock-threads/releases) - [Changelog](https://github.com/dessant/lock-threads/blob/main/CHANGELOG.md) - [Commits](https://github.com/dessant/lock-threads/compare/v4...v5) --- updated-dependencies: - dependency-name: dessant/lock-threads dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/lock.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/lock.yml b/.github/workflows/lock.yml index 72712c3d5f..e990228185 100644 --- a/.github/workflows/lock.yml +++ b/.github/workflows/lock.yml @@ -14,7 +14,7 @@ jobs: lock: runs-on: ubuntu-latest steps: - - uses: dessant/lock-threads@v4 + - uses: dessant/lock-threads@v5 with: github-token: ${{ github.token }} issue-inactive-days: 14 From 90cf6bb829b74c2a83b18c9493a9c186f1c0cafe Mon Sep 17 00:00:00 2001 From: TechnicalShree <87521583+TechnicalShree@users.noreply.github.com> Date: Tue, 21 Nov 2023 01:23:41 +0530 Subject: [PATCH 44/47] feat: Enabling Redirection to a Custom URL on Notification Click (#22956) * feat: notification custom routing * Update frappe/public/js/frappe/ui/notifications/notifications.js Co-authored-by: Ankush Menat * code refactoring * field name correction * fix: json * fix: auto generation * lint fix * fix: lint --------- Co-authored-by: Ankush Menat --- .../notification_log/notification_log.js | 4 ++++ .../notification_log/notification_log.json | 11 ++++++++-- .../notification_log/notification_log.py | 20 +++++++++++-------- .../frappe/ui/notifications/notifications.js | 3 +++ 4 files changed, 28 insertions(+), 10 deletions(-) diff --git a/frappe/desk/doctype/notification_log/notification_log.js b/frappe/desk/doctype/notification_log/notification_log.js index ea5fdc6400..63776fbf95 100644 --- a/frappe/desk/doctype/notification_log/notification_log.js +++ b/frappe/desk/doctype/notification_log/notification_log.js @@ -11,6 +11,10 @@ frappe.ui.form.on("Notification Log", { }, open_reference_document: function (frm) { + if (frm.doc?.link) { + frappe.set_route(frm.doc.link); + return; + } const dt = frm.doc.document_type; const dn = frm.doc.document_name; frappe.set_route("Form", dt, dn); diff --git a/frappe/desk/doctype/notification_log/notification_log.json b/frappe/desk/doctype/notification_log/notification_log.json index bafe28faf8..9fbe7324d3 100644 --- a/frappe/desk/doctype/notification_log/notification_log.json +++ b/frappe/desk/doctype/notification_log/notification_log.json @@ -15,7 +15,8 @@ "attached_file", "attachment_link", "open_reference_document", - "from_user" + "from_user", + "link" ], "fields": [ { @@ -91,12 +92,18 @@ "fieldname": "attachment_link", "fieldtype": "HTML", "label": "Attachment Link" + }, + { + "fieldname": "link", + "fieldtype": "Data", + "hidden": 1, + "label": "Link" } ], "hide_toolbar": 1, "in_create": 1, "links": [], - "modified": "2023-06-14 21:20:51.197943", + "modified": "2023-11-18 22:40:12.145940", "modified_by": "Administrator", "module": "Desk", "name": "Notification Log", diff --git a/frappe/desk/doctype/notification_log/notification_log.py b/frappe/desk/doctype/notification_log/notification_log.py index 105edd93d6..e6e13dafe7 100644 --- a/frappe/desk/doctype/notification_log/notification_log.py +++ b/frappe/desk/doctype/notification_log/notification_log.py @@ -25,6 +25,7 @@ class NotificationLog(Document): email_content: DF.TextEditor | None for_user: DF.Link | None from_user: DF.Link | None + link: DF.Data | None read: DF.Check subject: DF.Text | None type: DF.Literal["Mention", "Energy Point", "Assignment", "Share", "Alert"] @@ -125,21 +126,24 @@ def send_notification_email(doc): if not email: return - doc_link = get_url_to_form(doc.document_type, doc.document_name) header = get_email_header(doc) email_subject = strip_html(doc.subject) + args = { + "body_content": doc.subject, + "description": doc.email_content, + } + if doc.link: + args["doc_link"] = doc.link + else: + args["document_type"] = doc.document_type + args["document_name"] = doc.document_name + args["doc_link"] = get_url_to_form(doc.document_type, doc.document_name) frappe.sendmail( recipients=email, subject=email_subject, template="new_notification", - args = { - "body_content": doc.subject, - "description": doc.email_content, - "document_type": doc.document_type, - "document_name": doc.document_name, - "doc_link": doc_link, - }, + args=args, header=[header, "orange"], now=frappe.flags.in_test, ) diff --git a/frappe/public/js/frappe/ui/notifications/notifications.js b/frappe/public/js/frappe/ui/notifications/notifications.js index 14ff7fcbfb..15f7de8d9f 100644 --- a/frappe/public/js/frappe/ui/notifications/notifications.js +++ b/frappe/public/js/frappe/ui/notifications/notifications.js @@ -325,6 +325,9 @@ class NotificationsView extends BaseNotificationsView { } get_item_link(notification_doc) { + if (notification_doc.link) { + return notification_doc.link; + } const link_doctype = notification_doc.document_type ? notification_doc.document_type : "Notification Log"; From cd189b3f7fbfcb748abe206d13aff9841eb0f519 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 21 Nov 2023 12:00:44 +0530 Subject: [PATCH 45/47] fix: remove link - modifying while iterating --- frappe/core/doctype/communication/communication.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/core/doctype/communication/communication.py b/frappe/core/doctype/communication/communication.py index 9000901be2..adf145a6c7 100644 --- a/frappe/core/doctype/communication/communication.py +++ b/frappe/core/doctype/communication/communication.py @@ -478,7 +478,7 @@ class Communication(Document, CommunicationEmailMixin): return self.timeline_links def remove_link(self, link_doctype, link_name, autosave=False, ignore_permissions=True): - for l in self.timeline_links: + for l in list(self.timeline_links): if l.link_doctype == link_doctype and l.link_name == link_name: self.timeline_links.remove(l) From da1fb2ff4e9ea75b6e4c31816f6d13c6ed288a9a Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Tue, 21 Nov 2023 13:01:19 +0530 Subject: [PATCH 46/47] fix(event/sync_communication): add `distinct` while querying communication links There's cases where these are duplicated Signed-off-by: Akhil Narang --- frappe/desk/doctype/event/event.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/frappe/desk/doctype/event/event.py b/frappe/desk/doctype/event/event.py index 5463df6413..bf56498780 100644 --- a/frappe/desk/doctype/event/event.py +++ b/frappe/desk/doctype/event/event.py @@ -121,9 +121,7 @@ class Event(Document): ["Communication Link", "link_doctype", "=", participant.reference_doctype], ["Communication Link", "link_name", "=", participant.reference_docname], ] - comms = frappe.get_all("Communication", filters=filters, fields=["name"]) - - if comms: + if comms := frappe.get_all("Communication", filters=filters, fields=["name"], distinct=True): for comm in comms: communication = frappe.get_doc("Communication", comm.name) self.update_communication(participant, communication) From b54b6867c4bc679087f25ab4556e353059aede55 Mon Sep 17 00:00:00 2001 From: Maharshi Patel <39730881+maharshivpatel@users.noreply.github.com> Date: Tue, 21 Nov 2023 14:34:12 +0530 Subject: [PATCH 47/47] fix: Font for arabic and not supported languages (#23234) As we added Inter Variable Font in v15, we noticed that the font is not supported for Arabic and other languages. We added a fallback font for Arabic and other languages. --- frappe/public/css/fonts/inter/inter.css | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frappe/public/css/fonts/inter/inter.css b/frappe/public/css/fonts/inter/inter.css index 3be525aba0..3200c42576 100644 --- a/frappe/public/css/fonts/inter/inter.css +++ b/frappe/public/css/fonts/inter/inter.css @@ -8,6 +8,7 @@ src: url('/assets/frappe/css/fonts/inter/Inter.var.woff2?v=3.19') format('woff2-variations'), url('/assets/frappe/css/fonts/inter/Inter.var.woff2?v=3.19') format('woff2'); src: url('/assets/frappe/css/fonts/inter/Inter.var.woff2?v=3.19') format('woff2') tech('variations'); + unicode-range: U+0460-052F,U+1C80-1C88,U+20B4,U+2DE0-2DFF,U+A640-A69F,U+FE2E-FE2F; } @font-face { font-family: 'Inter V'; @@ -17,6 +18,7 @@ src: url('/assets/frappe/css/fonts/inter/Inter-Italic.var.woff2?v=3.19') format('woff2-variations'), url('/assets/frappe/css/fonts/inter/Inter-Italic.var.woff2?v=3.19') format('woff2'); src: url('/assets/frappe/css/fonts/inter/Inter-Italic.var.woff2?v=3.19') format('woff2') tech('variations'); + unicode-range: U+0460-052F,U+1C80-1C88,U+20B4,U+2DE0-2DFF,U+A640-A69F,U+FE2E-FE2F; } @font-face { font-family: 'Inter';