Commit 127448cd authored by Sebastien Robin's avatar Sebastien Robin

erp5_core: start to enforce naming convention for field libraries

In the same time, remove checking of legacy business template, they
are not maintained any more.

Do not check naming conventions for field libraries in many business
templates that would need cleanup first (the list is harcoded, this
way any new bt will be checked automatically)
parent 62717da4
...@@ -32,15 +32,15 @@ ...@@ -32,15 +32,15 @@
<value> <value>
<dictionary> <dictionary>
<item> <item>
<key> <string>extra_context</string> </key> <key> <string>field_id</string> </key>
<value> <string></string> </value> <value> <string></string> </value>
</item> </item>
<item> <item>
<key> <string>field_id</string> </key> <key> <string>form_id</string> </key>
<value> <string></string> </value> <value> <string></string> </value>
</item> </item>
<item> <item>
<key> <string>form_id</string> </key> <key> <string>target</string> </key>
<value> <string></string> </value> <value> <string></string> </value>
</item> </item>
</dictionary> </dictionary>
...@@ -51,15 +51,15 @@ ...@@ -51,15 +51,15 @@
<value> <value>
<dictionary> <dictionary>
<item> <item>
<key> <string>extra_context</string> </key> <key> <string>field_id</string> </key>
<value> <string></string> </value> <value> <string></string> </value>
</item> </item>
<item> <item>
<key> <string>field_id</string> </key> <key> <string>form_id</string> </key>
<value> <string></string> </value> <value> <string></string> </value>
</item> </item>
<item> <item>
<key> <string>form_id</string> </key> <key> <string>target</string> </key>
<value> <string></string> </value> <value> <string></string> </value>
</item> </item>
</dictionary> </dictionary>
...@@ -69,19 +69,17 @@ ...@@ -69,19 +69,17 @@
<key> <string>values</string> </key> <key> <string>values</string> </key>
<value> <value>
<dictionary> <dictionary>
<item>
<key> <string>extra_context</string> </key>
<value>
<list/>
</value>
</item>
<item> <item>
<key> <string>field_id</string> </key> <key> <string>field_id</string> </key>
<value> <string>my_title</string> </value> <value> <string>my_view_mode_title</string> </value>
</item> </item>
<item> <item>
<key> <string>form_id</string> </key> <key> <string>form_id</string> </key>
<value> <string>OrderLine_viewFieldLibrary</string> </value> <value> <string>Base_viewFieldLibrary</string> </value>
</item>
<item>
<key> <string>target</string> </key>
<value> <string>Click to edit the target</string> </value>
</item> </item>
</dictionary> </dictionary>
</value> </value>
......
...@@ -33,38 +33,6 @@ ...@@ -33,38 +33,6 @@
<key> <string>overrides</string> </key> <key> <string>overrides</string> </key>
<value> <value>
<dictionary> <dictionary>
<item>
<key> <string>alternate_name</string> </key>
<value> <string></string> </value>
</item>
<item>
<key> <string>css_class</string> </key>
<value> <string></string> </value>
</item>
<item>
<key> <string>default</string> </key>
<value> <string></string> </value>
</item>
<item>
<key> <string>description</string> </key>
<value> <string></string> </value>
</item>
<item>
<key> <string>editable</string> </key>
<value> <string></string> </value>
</item>
<item>
<key> <string>enabled</string> </key>
<value> <string></string> </value>
</item>
<item>
<key> <string>external_validator</string> </key>
<value> <string></string> </value>
</item>
<item>
<key> <string>extra_context</string> </key>
<value> <string></string> </value>
</item>
<item> <item>
<key> <string>field_id</string> </key> <key> <string>field_id</string> </key>
<value> <string></string> </value> <value> <string></string> </value>
...@@ -74,11 +42,7 @@ ...@@ -74,11 +42,7 @@
<value> <string></string> </value> <value> <string></string> </value>
</item> </item>
<item> <item>
<key> <string>hidden</string> </key> <key> <string>target</string> </key>
<value> <string></string> </value>
</item>
<item>
<key> <string>title</string> </key>
<value> <string></string> </value> <value> <string></string> </value>
</item> </item>
</dictionary> </dictionary>
...@@ -89,15 +53,15 @@ ...@@ -89,15 +53,15 @@
<value> <value>
<dictionary> <dictionary>
<item> <item>
<key> <string>extra_context</string> </key> <key> <string>field_id</string> </key>
<value> <string></string> </value> <value> <string></string> </value>
</item> </item>
<item> <item>
<key> <string>field_id</string> </key> <key> <string>form_id</string> </key>
<value> <string></string> </value> <value> <string></string> </value>
</item> </item>
<item> <item>
<key> <string>form_id</string> </key> <key> <string>target</string> </key>
<value> <string></string> </value> <value> <string></string> </value>
</item> </item>
</dictionary> </dictionary>
...@@ -111,19 +75,17 @@ ...@@ -111,19 +75,17 @@
<key> <string>editable</string> </key> <key> <string>editable</string> </key>
<value> <int>0</int> </value> <value> <int>0</int> </value>
</item> </item>
<item>
<key> <string>extra_context</string> </key>
<value>
<list/>
</value>
</item>
<item> <item>
<key> <string>field_id</string> </key> <key> <string>field_id</string> </key>
<value> <string>my_title</string> </value> <value> <string>my_view_mode_title</string> </value>
</item> </item>
<item> <item>
<key> <string>form_id</string> </key> <key> <string>form_id</string> </key>
<value> <string>PurchaseOrder_viewFieldLibrary</string> </value> <value> <string>Base_viewFieldLibrary</string> </value>
</item>
<item>
<key> <string>target</string> </key>
<value> <string>Click to edit the target</string> </value>
</item> </item>
</dictionary> </dictionary>
</value> </value>
......
...@@ -124,6 +124,11 @@ def checkField(folder, form, field):\n ...@@ -124,6 +124,11 @@ def checkField(folder, form, field):\n
path = folder.id + \'/\' + form.id\n path = folder.id + \'/\' + form.id\n
error_message = checkTitle(path, field.id, field.title(), field)\n error_message = checkTitle(path, field.id, field.title(), field)\n
template_field = getFieldFromProxyField(field)\n template_field = getFieldFromProxyField(field)\n
if path.endswith("FieldLibrary"):\n
if not(template_field is field):\n
  • I do not understand the logic of this condition: why would a non-proxy field in a proxy library be subject to naming constraints that a "plain" (non-proxy) field would not, in the same proxy field library ?

  • @seb: ping ?

Please register or sign in to reply
if not(1 in [field.id.startswith(x) for x in (\'my_view_mode_\',\n
\'my_core_mode_\', \'my_report_mode_\', \'my_list_mode_\', \'my_dialog_mode_\')]):\n
  • I do not undertsand the logic of these prefixes: why would it be wrong to use the same field in, say, view and dialog modes ?

    Moreover, why are the your_ prefixes rejected by this rule ?

  • @seb: ping here as well

  • There is often a different representation in the different modes. If you take for example "simulation_state", it will be different between the view of a document (where usually it is read only), and between a dialog (where you might have a multi list field where you can select several simulation states).

    Though, if I'm remember correctly, core_mode could be used when we do not really need a difference between different modues.

    I don't know for your_ prefixes.

  • Though, if I'm remember correctly, core_mode could be used when we do not really need a difference between different modues.

    Thanks, I was wondering about this prefix. So I think I will use it, as it makes the most sense for my use.

    I don't know for your_ prefixes.

    I guess they were (and still are) not popular enough, and were overlooked. I will add support for them.

  • I don't know for your_ prefixes.

    I looked again, and maybe what this script is expecting is that in the field library we have Base_viewFooFieldLibrary/my_dialog_mode_bar ( and not Base_viewFooFieldLibrary/your_bar ). In forms we have Foo_viewSomethingDialog/your_bar which is a proxy to Base_viewFooFieldLibrary/my_dialog_mode_bar , but in the field libraries there are no your_, just my_dialog_mode. Is that it ?

    At least this is what we seem to be doing in https://lab.nexedi.com/nexedi/erp5/blob/4723100aa4a5b8f023127171a5e669d3ed2d1a3e/bt5/erp5_trade/SkinTemplateItem/portal_skins/erp5_trade/DeliveryModule_viewDeliveryLineReportDialog/your_use.xml#L79-84 for example .

    BTW, there was a doc explaining field libraries, but I can not find i when I google for "ERP5 field library guideline" I only find unrelated wendelin pages... and I remember there was a new guideline and old guidelines so I don't know which one we should use.

  • I already added support for your_: c63939a6 .

    I'm not sure why you give your_ examples which lack further prefixes, and my_ examples with further prefixes: those prefixes are independent.

    my_ is a more magical version of your_: it provides an implicit default value based on the field's name. But because the field name which matters is the one on the form being rendered, the prefix on fields in the field library do not matter at all, ever: the field library form is never rendered. So using the my_ prefix here is pointless, although harmless. I think the my_ prefix is used everywhere without a strong justification, and your_ is seen as a more obscure one, where in reality your_ is less magical than my_ and should be used whenever my_'s magic is not required.

    So fairer examples would be my_dialog_mode_bar and your_dialog_mode_bar, which are both equally valid field names in a form being rendered, and in my opinion this extends to fields libraries... With the extra layer than in field libraries my_ gets us no benefit at all.

    Edited by Vincent Pelletier
Please register or sign in to reply
error_message += "%s: %s : Bad ID for a Field Library Field" % (path, field.id)\n
if template_field is None:\n if template_field is None:\n
if field.get_value(\'enabled\'):\n if field.get_value(\'enabled\'):\n
error_message += "Could not get a field from a proxy field %s" % field.id\n error_message += "Could not get a field from a proxy field %s" % field.id\n
...@@ -210,6 +215,9 @@ for folder in context.portal_skins.objectValues(spec=(\'Folder\',)):\n ...@@ -210,6 +215,9 @@ for folder in context.portal_skins.objectValues(spec=(\'Folder\',)):\n
message = checkTitle(\'/\'.join([folder.id, form.id]), \'Title of the Form itself\', form.title)\n message = checkTitle(\'/\'.join([folder.id, form.id]), \'Title of the Form itself\', form.title)\n
if message:\n if message:\n
message_list.append(message)\n message_list.append(message)\n
if form.id.endswith("FieldLibrary"):\n
if not(form.id.startswith("Base_")):\n
message_list.append("%s/%s : Bad Form ID for a Field Library Form" % (folder.id, form.id))\n
for group in form.get_groups():\n for group in form.get_groups():\n
if group == \'hidden\':\n if group == \'hidden\':\n
continue\n continue\n
...@@ -262,6 +270,8 @@ for ptype in context.portal_types.objectValues():\n ...@@ -262,6 +270,8 @@ for ptype in context.portal_types.objectValues():\n
if message:\n if message:\n
message_list.append(message)\n message_list.append(message)\n
\n \n
if batch_mode:\n
return message_list\n
if message_list:\n if message_list:\n
return ("%d problems found:\\n\\n" % len(message_list)) + \'\\n\'.join(message_list)\n return ("%d problems found:\\n\\n" % len(message_list)) + \'\\n\'.join(message_list)\n
return "OK"\n return "OK"\n
...@@ -271,7 +281,7 @@ return "OK"\n ...@@ -271,7 +281,7 @@ return "OK"\n
</item> </item>
<item> <item>
<key> <string>_params</string> </key> <key> <string>_params</string> </key>
<value> <string></string> </value> <value> <string>batch_mode=False</string> </value>
</item> </item>
<item> <item>
<key> <string>id</string> </key> <key> <string>id</string> </key>
......
...@@ -28,13 +28,14 @@ ...@@ -28,13 +28,14 @@
import unittest import unittest
from Products.ERP5Type.tests.ERP5TypeTestCase import ERP5TypeTestCase from Products.ERP5Type.tests.ERP5TypeTestCase import ERP5TypeTestCase
from Testing import ZopeTestCase
class TestNamingConvention(ERP5TypeTestCase): class TestNamingConvention(ERP5TypeTestCase):
def getBusinessTemplateList(self): def getBusinessTemplateList(self):
# include all standard Business Templates, i.e. erp5_* # include all standard Business Templates, i.e. erp5_*
return ( return (
'erp5_core_proxy_field_legacy', 'erp5_base', 'erp5_pdm', 'erp5_base', 'erp5_pdm',
'erp5_simulation', 'erp5_trade', 'erp5_accounting', 'erp5_simulation', 'erp5_trade', 'erp5_accounting',
'erp5_apparel', 'erp5_mrp', 'erp5_project', 'erp5_apparel', 'erp5_mrp', 'erp5_project',
'erp5_ingestion_mysql_innodb_catalog', 'erp5_ingestion', 'erp5_ingestion_mysql_innodb_catalog', 'erp5_ingestion',
...@@ -53,7 +54,7 @@ class TestNamingConvention(ERP5TypeTestCase): ...@@ -53,7 +54,7 @@ class TestNamingConvention(ERP5TypeTestCase):
'erp5_secure_payment', 'erp5_paypal_secure_payment', 'erp5_payzen_secure_payment', 'erp5_secure_payment', 'erp5_paypal_secure_payment', 'erp5_payzen_secure_payment',
'erp5_public_accounting_budget', 'erp5_publication', 'erp5_run_my_doc', 'erp5_public_accounting_budget', 'erp5_publication', 'erp5_run_my_doc',
'erp5_short_message', 'erp5_simplified_invoicing', 'erp5_trade_knowledge_pad', 'erp5_short_message', 'erp5_simplified_invoicing', 'erp5_trade_knowledge_pad',
'erp5_trade_proxy_field_legacy', 'erp5_trade_ui_test', 'erp5_ace_editor', 'erp5_trade_ui_test', 'erp5_ace_editor',
'erp5_authentication_policy', 'erp5_bearer_token', 'erp5_bespin', 'erp5_authentication_policy', 'erp5_bearer_token', 'erp5_bespin',
'erp5_certificate_authority', 'erp5_code_mirror', 'erp5_computer_immobilisation', 'erp5_certificate_authority', 'erp5_code_mirror', 'erp5_computer_immobilisation',
'erp5_credential_oauth2', 'erp5_data_protection', 'erp5_data_set', 'erp5_credential_oauth2', 'erp5_data_protection', 'erp5_data_set',
...@@ -84,8 +85,38 @@ class TestNamingConvention(ERP5TypeTestCase): ...@@ -84,8 +85,38 @@ class TestNamingConvention(ERP5TypeTestCase):
return "Naming Convention" return "Naming Convention"
def testNamingConvention(self): def testNamingConvention(self):
result = self.portal.ERP5Site_checkNamingConventions() result_list = self.portal.ERP5Site_checkNamingConventions(batch_mode=True)
self.assertEqual("OK", result, result) final_result_list = []
ignored_result_list = []
for result in result_list:
# Thre is too much mess in Field Library, so enforce only some business
# template until more cleanup is done
if result.find("Field Library") >= 0:
for skin_folder in ('erp5_simulation', 'erp5_accounting', 'erp5_apparel',
'erp5_mrp', 'erp5_project', 'erp5_ingestion', 'erp5_web',
'erp5_dms', 'erp5_crm', 'erp5_budget', 'erp5_item',
'erp5_ui_test', 'erp5_invoicing', 'erp5_banking_core',
'erp5_banking_inventory', 'erp5_consulting', 'erp5_forge',
'erp5_payroll', 'erp5_pdf_editor', 'erp5_administration',
'erp5_advanced_invoicing', 'erp5_archive', 'erp5_barcode',
'erp5_calendar', 'erp5_knowledge_pad', 'erp5_km_theme',
'erp5_odt_style', 'erp5_run_my_doc', 'erp5_development',
'erp5_tax_resource', 'erp5_immobilisation', 'erp5_software_pdm',
'erp5_syncml', 'erp5_workflow', 'erp5_wizard', 'erp5_configurator',
'erp5_configurator_wizard', 'erp5_base', 'erp5_pdm',
'erp5_core_proxy_field_legacy'):
if result.startswith(skin_folder):
ignored_result_list.append(result)
break
else:
final_result_list.append(result)
else:
final_result_list.append(result)
ZopeTestCase._print("\n==============================")
ZopeTestCase._print("\nResult we ignore until cleanup is done:\n")
ZopeTestCase._print("\n".join(["(ignored): %s" % x for x in ignored_result_list]))
ZopeTestCase._print("\n==============================\n")
self.assertEqual(0, len(final_result_list), "\n".join(final_result_list))
def test_suite(): def test_suite():
suite = unittest.TestSuite() suite = unittest.TestSuite()
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment