From d0ba31c911d3f32531dc1fdd62fe787f6c0794ca Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 19 Jun 2023 18:22:36 +0530 Subject: [PATCH] refactor!: Prefix all custom fieldnames created from Desk (#21355) * fix: actualy remove special characterso `or "_"` is always truthy, this code didn't do anything * refactor!: Prefix all custom fieldnames created from Desk Why? - Custom and standard field clashes are frequent. This will prevent it from happening. E.g. recent `incoterm` field addition in ERPNext. - Apps/fixtures can still specify exact fieldnames * test: custom field naming --- .semgrepignore | 0 .../doctype/custom_field/custom_field.py | 3 +- .../customize_form/test_customize_form.py | 45 +++++++++---------- 3 files changed, 23 insertions(+), 25 deletions(-) create mode 100644 .semgrepignore diff --git a/.semgrepignore b/.semgrepignore new file mode 100644 index 0000000000..e69de29bb2 diff --git a/frappe/custom/doctype/custom_field/custom_field.py b/frappe/custom/doctype/custom_field/custom_field.py index 8953153be6..ed6296b6f2 100644 --- a/frappe/custom/doctype/custom_field/custom_field.py +++ b/frappe/custom/doctype/custom_field/custom_field.py @@ -40,8 +40,9 @@ class CustomField(Document): # remove special characters from fieldname self.fieldname = "".join( - filter(lambda x: x.isdigit() or x.isalpha() or "_", cstr(label).replace(" ", "_")) + [c for c in cstr(label).replace(" ", "_") if c.isdigit() or c.isalpha() or c == "_"] ) + self.fieldname = f"custom_{self.fieldname}" # fieldnames should be lowercase self.fieldname = self.fieldname.lower() diff --git a/frappe/custom/doctype/customize_form/test_customize_form.py b/frappe/custom/doctype/customize_form/test_customize_form.py index 149ef85e28..8a62d331be 100644 --- a/frappe/custom/doctype/customize_form/test_customize_form.py +++ b/frappe/custom/doctype/customize_form/test_customize_form.py @@ -14,10 +14,11 @@ test_dependencies = ["Custom Field", "Property Setter"] class TestCustomizeForm(FrappeTestCase): def insert_custom_field(self): - frappe.delete_doc_if_exists("Custom Field", "Event-test_custom_field") - frappe.get_doc( + frappe.delete_doc_if_exists("Custom Field", "Event-custom_test_field") + self.field = frappe.get_doc( { "doctype": "Custom Field", + "fieldname": "custom_test_field", "dt": "Event", "label": "Test Custom Field", "description": "A Custom Field for Testing", @@ -36,7 +37,7 @@ class TestCustomizeForm(FrappeTestCase): frappe.clear_cache(doctype="Event") def tearDown(self): - frappe.delete_doc("Custom Field", "Event-test_custom_field") + frappe.delete_doc("Custom Field", self.field.name) frappe.db.commit() frappe.clear_cache(doctype="Event") @@ -60,7 +61,7 @@ class TestCustomizeForm(FrappeTestCase): self.assertEqual(d.doc_type, "Event") self.assertEqual(len(d.get("fields")), len(frappe.get_doc("DocType", d.doc_type).fields) + 1) - self.assertEqual(d.get("fields")[-1].fieldname, "test_custom_field") + self.assertEqual(d.get("fields")[-1].fieldname, self.field.fieldname) self.assertEqual(d.get("fields", {"fieldname": "event_type"})[0].in_list_view, 1) return d @@ -129,21 +130,21 @@ class TestCustomizeForm(FrappeTestCase): def test_save_customization_custom_field_property(self): d = self.get_customize_form("Event") - self.assertEqual(frappe.db.get_value("Custom Field", "Event-test_custom_field", "reqd"), 0) + self.assertEqual(frappe.db.get_value("Custom Field", self.field.name, "reqd"), 0) - custom_field = d.get("fields", {"fieldname": "test_custom_field"})[0] + custom_field = d.get("fields", {"fieldname": self.field.fieldname})[0] custom_field.reqd = 1 custom_field.no_copy = 1 d.run_method("save_customization") - self.assertEqual(frappe.db.get_value("Custom Field", "Event-test_custom_field", "reqd"), 1) - self.assertEqual(frappe.db.get_value("Custom Field", "Event-test_custom_field", "no_copy"), 1) + self.assertEqual(frappe.db.get_value("Custom Field", self.field.name, "reqd"), 1) + self.assertEqual(frappe.db.get_value("Custom Field", self.field.name, "no_copy"), 1) custom_field = d.get("fields", {"is_custom_field": True})[0] custom_field.reqd = 0 custom_field.no_copy = 0 d.run_method("save_customization") - self.assertEqual(frappe.db.get_value("Custom Field", "Event-test_custom_field", "reqd"), 0) - self.assertEqual(frappe.db.get_value("Custom Field", "Event-test_custom_field", "no_copy"), 0) + self.assertEqual(frappe.db.get_value("Custom Field", self.field.name, "reqd"), 0) + self.assertEqual(frappe.db.get_value("Custom Field", self.field.name, "no_copy"), 0) def test_save_customization_new_field(self): d = self.get_customize_form("Event") @@ -157,28 +158,24 @@ class TestCustomizeForm(FrappeTestCase): }, ) d.run_method("save_customization") + + custom_field_name = "Event-custom_test_add_custom_field_via_customize_form" self.assertEqual( - frappe.db.get_value( - "Custom Field", "Event-test_add_custom_field_via_customize_form", "fieldtype" - ), + frappe.db.get_value("Custom Field", custom_field_name, "fieldtype"), "Data", ) self.assertEqual( - frappe.db.get_value( - "Custom Field", "Event-test_add_custom_field_via_customize_form", "insert_after" - ), + frappe.db.get_value("Custom Field", custom_field_name, "insert_after"), last_fieldname, ) - frappe.delete_doc("Custom Field", "Event-test_add_custom_field_via_customize_form") - self.assertEqual( - frappe.db.get_value("Custom Field", "Event-test_add_custom_field_via_customize_form"), None - ) + frappe.delete_doc("Custom Field", custom_field_name) + self.assertEqual(frappe.db.get_value("Custom Field", custom_field_name), None) def test_save_customization_remove_field(self): d = self.get_customize_form("Event") - custom_field = d.get("fields", {"fieldname": "test_custom_field"})[0] + custom_field = d.get("fields", {"fieldname": self.field.fieldname})[0] d.get("fields").remove(custom_field) d.run_method("save_customization") @@ -200,7 +197,7 @@ class TestCustomizeForm(FrappeTestCase): def test_set_allow_on_submit(self): d = self.get_customize_form("Event") d.get("fields", {"fieldname": "subject"})[0].allow_on_submit = 1 - d.get("fields", {"fieldname": "test_custom_field"})[0].allow_on_submit = 1 + d.get("fields", {"fieldname": "custom_test_field"})[0].allow_on_submit = 1 d.run_method("save_customization") d = self.get_customize_form("Event") @@ -209,7 +206,7 @@ class TestCustomizeForm(FrappeTestCase): self.assertEqual(d.get("fields", {"fieldname": "subject"})[0].allow_on_submit or 0, 0) # allow for custom field - self.assertEqual(d.get("fields", {"fieldname": "test_custom_field"})[0].allow_on_submit, 1) + self.assertEqual(d.get("fields", {"fieldname": "custom_test_field"})[0].allow_on_submit, 1) def test_title_field_pattern(self): d = self.get_customize_form("Web Form") @@ -406,7 +403,7 @@ class TestCustomizeForm(FrappeTestCase): def test_system_generated_fields(self): doctype = "Event" - custom_field_name = "test_custom_field" + custom_field_name = "custom_test_field" custom_field = frappe.get_doc("Custom Field", {"dt": doctype, "fieldname": custom_field_name}) custom_field.is_system_generated = 1