From cb7d25a29314c65eb10944324bfef0ae22e09027 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Mon, 31 Oct 2022 10:07:55 +0000 Subject: [PATCH] fix(meta): ensure that `insert_after` is always considered when sorting fields (#18682) * fix(meta): ensure that `insert_after` is always considered when sorting fields * test: add nosemgrep to comment Co-authored-by: Dany Roberts --- .../doctype/custom_field/test_custom_field.py | 48 ++++++++- frappe/model/meta.py | 99 +++++++++++-------- 2 files changed, 106 insertions(+), 41 deletions(-) diff --git a/frappe/custom/doctype/custom_field/test_custom_field.py b/frappe/custom/doctype/custom_field/test_custom_field.py index dfdaf7ea9a..cf64e4495b 100644 --- a/frappe/custom/doctype/custom_field/test_custom_field.py +++ b/frappe/custom/doctype/custom_field/test_custom_field.py @@ -2,6 +2,7 @@ # License: MIT. See LICENSE import frappe +from frappe.custom.doctype.custom_field.custom_field import create_custom_fields from frappe.tests.utils import FrappeTestCase test_records = frappe.get_test_records("Custom Field") @@ -9,8 +10,6 @@ test_records = frappe.get_test_records("Custom Field") class TestCustomField(FrappeTestCase): def test_create_custom_fields(self): - from .custom_field import create_custom_fields - create_custom_fields( { "Address": [ @@ -37,3 +36,48 @@ class TestCustomField(FrappeTestCase): self.assertTrue(frappe.db.exists("Custom Field", "Address-_test_custom_field_1")) self.assertTrue(frappe.db.exists("Custom Field", "Address-_test_custom_field_2")) self.assertTrue(frappe.db.exists("Custom Field", "Contact-_test_custom_field_2")) + + def test_custom_field_sorting(self): + try: + custom_fields = { + "ToDo": [ + {"fieldname": "a_test_field", "insert_after": "b_test_field"}, + {"fieldname": "b_test_field", "insert_after": "status"}, + {"fieldname": "c_test_field", "insert_after": "unknown_custom_field"}, + {"fieldname": "d_test_field", "insert_after": "status"}, + ] + } + + create_custom_fields(custom_fields, ignore_validate=True) + + fields = frappe.get_meta("ToDo", cached=False).fields + + for i, field in enumerate(fields): + if field.fieldname == "b_test_field": + self.assertEqual(fields[i - 1].fieldname, "status") + + if field.fieldname == "d_test_field": + self.assertEqual(fields[i - 1].fieldname, "a_test_field") + + self.assertEqual(fields[-1].fieldname, "c_test_field") + + finally: + frappe.db.delete( + "Custom Field", + { + "dt": "ToDo", + "fieldname": ( + "in", + ( + "a_test_field", + "b_test_field", + "c_test_field", + "d_test_field", + ), + ), + }, + ) + + # undo changes commited by DDL + # nosemgrep + frappe.db.commit() diff --git a/frappe/model/meta.py b/frappe/model/meta.py index 6aa8d1d80e..5a9c2d906d 100644 --- a/frappe/model/meta.py +++ b/frappe/model/meta.py @@ -140,10 +140,13 @@ class Meta(Document): self.init_field_map() return - self.add_custom_fields() + has_custom_fields = self.add_custom_fields() self.apply_property_setters() self.init_field_map() - self.sort_fields() + + if has_custom_fields: + self.sort_fields() + self.get_valid_columns() self.set_custom_permissions() self.add_custom_links_and_actions() @@ -319,7 +322,7 @@ class Meta(Document): return list_fields def get_custom_fields(self): - return [d for d in self.fields if d.get("is_custom_field")] + return [d for d in self.fields if getattr(d, "is_custom_field", False)] def get_title_field(self): """Return the title field of this doctype, @@ -358,17 +361,20 @@ class Meta(Document): if not frappe.db.table_exists("Custom Field"): return - custom_fields = frappe.db.sql( - """ - SELECT * FROM `tabCustom Field` - WHERE dt = %s AND docstatus < 2 - """, - (self.name,), - as_dict=1, + custom_fields = frappe.db.get_values( + "Custom Field", + filters={"dt": self.name}, + fieldname="*", + as_dict=True, + order_by="idx", update={"is_custom_field": 1}, ) + if not custom_fields: + return + self.extend("fields", custom_fields) + return True def apply_property_setters(self): """ @@ -452,43 +458,33 @@ class Meta(Document): self._fields = {field.fieldname: field for field in self.fields} def sort_fields(self): - """sort on basis of insert_after""" - custom_fields = sorted(self.get_custom_fields(), key=lambda df: df.idx) + """Sort custom fields on the basis of insert_after""" - if custom_fields: - newlist = [] + field_order = [] + insert_after_map = {} - # if custom field is at top - # insert_after is false - for c in list(custom_fields): - if not c.insert_after: - newlist.append(c) - custom_fields.pop(custom_fields.index(c)) + for field in self.fields: + if not getattr(field, "is_custom_field", False): + field_order.append(field.fieldname) - # standard fields - newlist += [df for df in self.get("fields") if not df.get("is_custom_field")] + elif insert_after := getattr(field, "insert_after", None): + insert_after_map.setdefault(insert_after, []).append(field.fieldname) - newlist_fieldnames = [df.fieldname for df in newlist] - for i in range(2): - for df in list(custom_fields): - if df.insert_after in newlist_fieldnames: - cf = custom_fields.pop(custom_fields.index(df)) - idx = newlist_fieldnames.index(df.insert_after) - newlist.insert(idx + 1, cf) - newlist_fieldnames.insert(idx + 1, cf.fieldname) + else: + # if custom field is at the top, insert after is None + field_order.insert(0, field.fieldname) - if not custom_fields: - break + if insert_after_map: + _update_field_order_based_on_insert_after(field_order, insert_after_map) - # worst case, add remaining custom fields to last - if custom_fields: - newlist += custom_fields + sorted_fields = [] - # renum idx - for i, f in enumerate(newlist): - f.idx = i + 1 + for idx, fieldname in enumerate(field_order, 1): + field = self._fields[fieldname] + field.idx = idx + sorted_fields.append(field) - self.fields = newlist + self.fields = sorted_fields def set_custom_permissions(self): """Reset `permissions` with Custom DocPerm if exists""" @@ -809,3 +805,28 @@ def trim_table(doctype, dry_run=True): frappe.db.sql_ddl(f"ALTER TABLE `tab{doctype}` {columns_to_remove}") return DROPPED_COLUMNS + + +def _update_field_order_based_on_insert_after(field_order, insert_after_map): + """Update the field order based on insert_after_map""" + + retry_field_insertion = True + + while retry_field_insertion: + retry_field_insertion = False + + for fieldname in list(insert_after_map): + if fieldname not in field_order: + continue + + custom_field_index = field_order.index(fieldname) + for custom_field_name in insert_after_map.pop(fieldname): + custom_field_index += 1 + field_order.insert(custom_field_index, custom_field_name) + + retry_field_insertion = True + + if insert_after_map: + # insert_after is an invalid fieldname, add these fields to the end + for fields in insert_after_map.values(): + field_order.extend(fields)