Commit 3978dba1 authored by Nicolas Wavrant's avatar Nicolas Wavrant

Verify parameters passed to category setters

This merge request aims to prevent programming errors by raising instead of silently doing nothing (which is a source of bugs).

Currently, if an object as category property for a category "category", 2 families of setters were created :

1. The string setter, following the format ```_setCategory``` and taking a relative URL as the argument.
2. The value setter, following the format ```_setCategoryValue``` and taking an object as the argument.

The issue is that developers may pass the wrong argument to one of these functions, having for consequences :

1. For case (1), if an object is passed, the code would silently do nothing : nothing is set as relation, but the code doesn't fail. This is the worst case. 
2. For the second case, passing a relative URL to ```_setCategoryValue``` would "work" (in the meaning the relation is set to the correct object). This may sound like a feature, but in my opinion it is confusing given the way ERP5 developers apprehend these setters nowadays.

For case 1, a test is existing that an exception is raised, but due to coding error the feature disappeared and no one noticed : https://lab.nexedi.com/Nicolas/erp5/blob/b3ed2210ed6b30390b901c8620de6eafcc27a574/product/CMFCategory/tests/testCMFCategory.py#L810-815

For case 2, compatibility code exists in the underlying function ```_setValue``` : https://lab.nexedi.com/Nicolas/erp5/blob/b3ed2210ed6b30390b901c8620de6eafcc27a574/product/ERP5Type/Base.py#L1840-1842

In this MR, the exception caused by case (1) has been restored (so now it fails loudly).

Case (2) has been deprecated, in order to keep backward compatibility.

I have run the tests with the ```DeprecationWarning``` raising an error instead of just a warning,  and fixed the code were the setters weren't used correctly.

Ideally, tests should always run with this ```DeprecationWarning``` being a real error so both cases crash loudly. This won't be part of this Merge Request.


/reviewed-on nexedi/erp5!938
parents b0972239 dd8b4e40
...@@ -10,7 +10,7 @@ contained_gadgets = [x.getSpecialiseValue().getRelativeUrl() \ ...@@ -10,7 +10,7 @@ contained_gadgets = [x.getSpecialiseValue().getRelativeUrl() \
if gadget_relative_url not in contained_gadgets: if gadget_relative_url not in contained_gadgets:
# add only if not there # add only if not there
knowledge_box = active_pad.newContent(portal_type='Knowledge Box') knowledge_box = active_pad.newContent(portal_type='Knowledge Box')
knowledge_box.setSpecialiseValue(gadget_relative_url) knowledge_box.setSpecialise(gadget_relative_url)
knowledge_box.visible() knowledge_box.visible()
else: else:
# reuse gadget # reuse gadget
......
...@@ -638,6 +638,18 @@ class CategoryTool(BaseTool): ...@@ -638,6 +638,18 @@ class CategoryTool(BaseTool):
category_list = (category_list, ) category_list = (category_list, )
elif category_list is None: elif category_list is None:
category_list = () category_list = ()
elif isinstance(category_list, (tuple, list, set, frozenset)):
if any([c is not None and not isinstance(c, str) for c in category_list]):
raise TypeError(
'This method only takes a string or an iterable of strings as parameter.',
base_category_list, category_list
)
else:
raise TypeError(
'This method only takes a string or an iterable of strings as parameter.',
base_category_list, category_list
)
if isinstance(base_category_list, str): if isinstance(base_category_list, str):
base_category_list = (base_category_list, ) base_category_list = (base_category_list, )
......
...@@ -807,12 +807,12 @@ class TestCMFCategory(ERP5TypeTestCase): ...@@ -807,12 +807,12 @@ class TestCMFCategory(ERP5TypeTestCase):
else: else:
o1 = organisation_module._getOb(self.id1) o1 = organisation_module._getOb(self.id1)
try: self.assertRaisesRegexp(
p1.setCareerSubordination(o1) TypeError,
except Exception, e: 'This method only takes a string or an iterable of strings as parameter.',
self.assertTrue(isinstance(e, TypeError)) p1.setCareerSubordination,
self.assertEqual(e.args[0], 'Category must be of string, tuple of ' o1
'string or list of string type.') )
def test_23_getCategoryChildValueList(self): def test_23_getCategoryChildValueList(self):
pc = self.getCategoriesTool() pc = self.getCategoriesTool()
......
...@@ -3297,7 +3297,7 @@ class TestAccountingReports(AccountingTestCase, ERP5ReportTestCase): ...@@ -3297,7 +3297,7 @@ class TestAccountingReports(AccountingTestCase, ERP5ReportTestCase):
self._makeOne( self._makeOne(
portal_type='Accounting Transaction', portal_type='Accounting Transaction',
title='Transaction in EUR (our currency)', title='Transaction in EUR (our currency)',
resource_value='currency_module/euro', resource='currency_module/euro',
source_reference='1', source_reference='1',
simulation_state='delivered', simulation_state='delivered',
destination_section_value=self.organisation_module.client_1, destination_section_value=self.organisation_module.client_1,
......
...@@ -94,18 +94,18 @@ class TestBPMMixin(ERP5TypeTestCase): ...@@ -94,18 +94,18 @@ class TestBPMMixin(ERP5TypeTestCase):
if create_order_to_invoice_path: if create_order_to_invoice_path:
self.createTradeModelPath(self.business_process, self.createTradeModelPath(self.business_process,
reference='order_path', reference='order_path',
trade_phase_value_list=('default/order',)) trade_phase_list=('default/order',))
self.createTradeModelPath(self.business_process, self.createTradeModelPath(self.business_process,
reference='delivery_path', reference='delivery_path',
trade_phase_value_list=('default/delivery',), trade_phase_list=('default/delivery',),
trade_date='trade_phase/default/order') trade_date='trade_phase/default/order')
self.createTradeModelPath(self.business_process, self.createTradeModelPath(self.business_process,
reference='invoice_path', reference='invoice_path',
trade_phase_value_list=('default/invoicing',), trade_phase_list=('default/invoicing',),
trade_date='trade_phase/default/delivery') trade_date='trade_phase/default/delivery')
self.createTradeModelPath(business_process, self.createTradeModelPath(business_process,
reference='default_path', reference='default_path',
trade_phase_value_list=('default/discount', 'default/tax'), trade_phase_list=('default/discount', 'default/tax'),
trade_date='trade_phase/default/invoicing') trade_date='trade_phase/default/invoicing')
# A trade model path already exist for root simulation movements # A trade model path already exist for root simulation movements
# (Accounting Transaction Root Simulation Rule). # (Accounting Transaction Root Simulation Rule).
......
...@@ -6765,7 +6765,7 @@ class TestBusinessTemplate(BusinessTemplateMixin): ...@@ -6765,7 +6765,7 @@ class TestBusinessTemplate(BusinessTemplateMixin):
# we simulated above, but we just want to test that an exported role # we simulated above, but we just want to test that an exported role
# information can be imported back # information can be imported back
object_type.newContent(portal_type='Role Information', object_type.newContent(portal_type='Role Information',
local_role_group_value=self.portal.portal_categories.local_role_group.Alternate.getRelativeUrl(), local_role_group_value=self.portal.portal_categories.local_role_group.Alternate,
role_name_list=('Assignee', )) role_name_list=('Assignee', ))
bt = self.portal.portal_templates.newContent( bt = self.portal.portal_templates.newContent(
......
...@@ -2818,15 +2818,15 @@ class TestTrackingList(InventoryAPITestCase): ...@@ -2818,15 +2818,15 @@ class TestTrackingList(InventoryAPITestCase):
node_b = self._makeOrganisation(title='Node B') node_b = self._makeOrganisation(title='Node B')
node_c = self._makeOrganisation(title='Node C') node_c = self._makeOrganisation(title='Node C')
movement_a = self._makeMovement(source_value=node_a, movement_a = self._makeMovement(source_value=node_a,
destination_value=node_b, resource=self.resource, destination_value=node_b, resource=self.resource.getRelativeUrl(),
quantity=1, aggregate_value=item_a, start_date=now, quantity=1, aggregate_value=item_a, start_date=now,
simulation_state=state_a) simulation_state=state_a)
movement_b = self._makeMovement(source_value=node_b, movement_b = self._makeMovement(source_value=node_b,
destination_value=node_c, resource=self.resource, destination_value=node_c, resource=self.resource.getRelativeUrl(),
quantity=1, aggregate_value=item_a, start_date=now+1, quantity=1, aggregate_value=item_a, start_date=now+1,
simulation_state=state_b) simulation_state=state_b)
movement_c = self._makeMovement(source_value=node_a, movement_c = self._makeMovement(source_value=node_a,
destination_value=node_b, resource=self.resource, destination_value=node_b, resource=self.resource.getRelativeUrl(),
quantity=1, aggregate_value=item_b, start_date=now+1, quantity=1, aggregate_value=item_b, start_date=now+1,
simulation_state=state_c) simulation_state=state_c)
self.tic() self.tic()
......
...@@ -298,7 +298,7 @@ class TestKM(TestKMMixIn): ...@@ -298,7 +298,7 @@ class TestKM(TestKMMixIn):
# Web Section mode # Web Section mode
websection_knowledge_pad = user_pref.newContent(portal_type = 'Knowledge Pad', websection_knowledge_pad = user_pref.newContent(portal_type = 'Knowledge Pad',
title = "web_section") title = "web_section")
websection_knowledge_pad.setGroupValue(default_pad_group) websection_knowledge_pad.setGroup(default_pad_group)
websection_knowledge_pad1 = websection_knowledge_pad.newContent( \ websection_knowledge_pad1 = websection_knowledge_pad.newContent( \
portal_type = 'Knowledge Box', portal_type = 'Knowledge Box',
title = "web_section_1") title = "web_section_1")
...@@ -311,7 +311,7 @@ class TestKM(TestKMMixIn): ...@@ -311,7 +311,7 @@ class TestKM(TestKMMixIn):
websection_content_knowledge_pad = user_pref.newContent( \ websection_content_knowledge_pad = user_pref.newContent( \
portal_type = 'Knowledge Pad', \ portal_type = 'Knowledge Pad', \
title = "web_section_content") title = "web_section_content")
websection_content_knowledge_pad.setGroupValue(default_pad_group_section_content_title) websection_content_knowledge_pad.setGroup(default_pad_group_section_content_title)
websection_content_knowledge_pad1 = websection_content_knowledge_pad.newContent( \ websection_content_knowledge_pad1 = websection_content_knowledge_pad.newContent( \
portal_type = 'Knowledge Box', \ portal_type = 'Knowledge Box', \
title = "web_section_content_1") title = "web_section_content_1")
......
...@@ -110,14 +110,14 @@ CREATE TABLE alternate_roles_and_users ( ...@@ -110,14 +110,14 @@ CREATE TABLE alternate_roles_and_users (
role_name='Auditor', role_name='Auditor',
role_base_category_script_id='ERP5Type_getSecurityCategoryFromSelf', role_base_category_script_id='ERP5Type_getSecurityCategoryFromSelf',
role_base_category='agent', role_base_category='agent',
local_role_group_value=self.portal.portal_categories.local_role_group.Alternate.getRelativeUrl()) local_role_group_value=self.portal.portal_categories.local_role_group.Alternate)
# add another role information that does not grant view permission # add another role information that does not grant view permission
self.portal.portal_types.Person.newContent( self.portal.portal_types.Person.newContent(
portal_type='Role Information', portal_type='Role Information',
role_name='Unknown', role_name='Unknown',
role_category_list=('group/g1'), role_category_list=('group/g1'),
role_base_category='group', role_base_category='group',
local_role_group_value=self.portal.portal_categories.local_role_group.Alternate.getRelativeUrl()) local_role_group_value=self.portal.portal_categories.local_role_group.Alternate)
self.portal.portal_caches.clearAllCache() self.portal.portal_caches.clearAllCache()
self.tic() self.tic()
......
...@@ -1860,14 +1860,21 @@ class Base( CopyContainer, ...@@ -1860,14 +1860,21 @@ class Base( CopyContainer,
if target is None : if target is None :
path = target path = target
elif isinstance(target, str): elif isinstance(target, str):
# We have been provided a string
path = target path = target
warnings.warn(
"Only objects should be passed to value accessors",
DeprecationWarning
)
elif isinstance(target, (tuple, list, set, frozenset)): elif isinstance(target, (tuple, list, set, frozenset)):
# We have been provided a list or tuple # We have been provided a list or tuple
path_list = [] path_list = []
for target_item in target: for target_item in target:
if isinstance(target_item, str): if isinstance(target_item, str):
path = target_item path = target_item
warnings.warn(
"Only objects should be passed to value accessors",
DeprecationWarning
)
else: else:
path = getRelativeUrl(target_item) path = getRelativeUrl(target_item)
path_list.append(cleanupCategory(path)) path_list.append(cleanupCategory(path))
......
...@@ -33,6 +33,7 @@ import os ...@@ -33,6 +33,7 @@ import os
import shutil import shutil
import tempfile import tempfile
import unittest import unittest
import warnings
import transaction import transaction
from persistent import Persistent from persistent import Persistent
...@@ -378,6 +379,8 @@ class TestZodbPropertySheet(ERP5TypeTestCase): ...@@ -378,6 +379,8 @@ class TestZodbPropertySheet(ERP5TypeTestCase):
""" """
XXX: WORK IN PROGRESS XXX: WORK IN PROGRESS
""" """
def getBusinessTemplateList(self): def getBusinessTemplateList(self):
return 'erp5_base', return 'erp5_base',
...@@ -1313,6 +1316,62 @@ class TestZodbPropertySheet(ERP5TypeTestCase): ...@@ -1313,6 +1316,62 @@ class TestZodbPropertySheet(ERP5TypeTestCase):
self.fail("Creating a Category Expression with syntax error raises "\ self.fail("Creating a Category Expression with syntax error raises "\
"an error") "an error")
def testCategoryValueRelationShowsWarningIfStringIsPassedAsParameter(self):
person_module = self.portal.person_module
person = person_module.newContent()
def _testDeprecationWarning(method, *args, **kw):
with warnings.catch_warnings(record=True) as warning_list:
warnings.simplefilter("always")
method(*args, **kw)
warning, = warning_list
self.assertTrue(issubclass(warning.category, DeprecationWarning))
self.assertEqual(
str(warning.message),
"Only objects should be passed to value accessors",
)
# Passing a string to a Value setter should raise
organisation = self.portal.organisation_module.newContent()
_testDeprecationWarning(
person.setSubordinationValue,
organisation.getRelativeUrl(),
)
_testDeprecationWarning(
person_module.newContent,
subordination_value=organisation.getRelativeUrl(),
)
# Same test but with a category instead of an object
social_title_value = self.portal.portal_categories.social_title.newContent(id='Mme')
_testDeprecationWarning(
person.setSocialTitleValue,
social_title_value.getRelativeUrl(),
)
def testCategoryRelationRaisesIfValueisPassedAsParameter(self):
person_module = self.portal.person_module
person = person_module.newContent()
# Passing an ERP5 object to a not-Value setter should raise
with self.assertRaises(TypeError):
organisation = self.portal.organisation_module.newContent()
person.setSubordination(organisation)
person_module.newContent(
subordination=organisation,
)
# Same test with a category instead of an object
social_title_value = self.portal.portal_categories.social_title.newContent(id='Mr')
with self.assertRaises(TypeError):
person.setSocialTitle(social_title_value)
# Passing a unicode object to a not-Value setter should raise
with self.assertRaises(TypeError):
organisation = self.portal.organisation_module.newContent()
person.setSubordination(unicode(organisation.getRelativeUrl()))
from Products.ERP5Type.Tool.ComponentTool import ComponentTool from Products.ERP5Type.Tool.ComponentTool import ComponentTool
ComponentTool._original_reset = ComponentTool.reset ComponentTool._original_reset = ComponentTool.reset
ComponentTool._reset_performed = False ComponentTool._reset_performed = False
......
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