Commit 840066ac authored by Klaus Wölfel's avatar Klaus Wölfel Committed by Jérome Perrin

Fix a broken case of Worklist calculation when using security_uid columns

With some combinations of worklists using two additional security_uid
columns coming from local role groups, some documents might be excluded
from worklists, without the fix, when running with a random
PYTHONHASHSEED, the test can fail with:

    FAIL: test_worklist_exclusionlist_collision (erp5.component.test.erp5_version.testERP5CatalogSecurityUidOptimization.TestSecurityUidOptimizationWorklist)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "<portal_components/test.erp5.testERP5CatalogSecurityUidOptimization>", line 370, in test_worklist_exclusionlist_collision
        'security_uid_or_alternate_security_uid_draft': 1,
      File "<portal_components/test.erp5.testERP5CatalogSecurityUidOptimization>", line 213, in assertWorklistCount
        expected_count_by_worklist_id,
    AssertionError: {'security_uid_or_alternate_security_uid_draft': 1} != {'collision_worklist': 1, 'security_uid_or_alternate_security_uid_draft': 1}
    - {'security_uid_or_alternate_security_uid_draft': 1}
    + {'collision_worklist': 1, 'security_uid_or_alternate_security_uid_draft': 1}
    ?  +++++++++++++++++++++++++

What happens in sumCatalogResultByWorklist is something like this:

    (Pdb) pp criterion_dict
    {'alternate_security_uid': <ExclusionList [11, 14]>,
     'other_security_uid': frozenset([13L]),
     'portal_type': frozenset(['Person']),
     'validation_state': frozenset(['draft'])}

    (Pdb) pp catalog_result.dictionaries()
    [{'alternate_security_uid': 12,
      'count': Decimal('1'),
      'other_security_uid': 13,
      'portal_type': 'Person',
      'validation_state': 'draft'}]

Depending on the worklists grouped together in grouped_worklist_dict and
the order this dict is iterated, we may reach a situation where
criterion_id_list contains 'alternate_security_uid', because for another
worklist it was not an ExclusionList but a list to be applied, in this
case, this if condition is false:

        for criterion_id in criterion_id_list:
          criterion_value_set = criterion_dict[criterion_id]
          if result_line[criterion_id] not in criterion_value_set:
            is_candidate = False

and the row is not counted in collision_worklist worklist.
Co-authored-by: Jérome Perrin's avatarJérome Perrin <jerome@nexedi.com>
parent 590c1b3e
...@@ -292,3 +292,80 @@ class TestSecurityUidOptimizationWorklist(SecurityUidOptimizationTestCase): ...@@ -292,3 +292,80 @@ class TestSecurityUidOptimizationWorklist(SecurityUidOptimizationTestCase):
'security_uid_or_alternate_security_uid_draft': 4, 'security_uid_or_alternate_security_uid_draft': 4,
} }
) )
def test_worklist_exclusionlist_collision(self):
# non-regression test for an issue with multiple local role group
self.portal.portal_types.Organisation.newContent(
portal_type='Role Information',
role_name='Assignee',
role_category_list=('group/g1', ),
role_base_category='group',
local_role_group_value=self.portal.portal_categories.local_role_group.Alternate,
)
self.portal.portal_types.Organisation.newContent(
portal_type='Role Information',
role_name='Assignor',
role_category_list=('group/g2', ),
role_base_category='group',
local_role_group_value=self.portal.portal_categories.local_role_group.Alternate,
)
self.portal.portal_types.Person.newContent(
portal_type='Role Information',
role_name='Assignee',
role_category_list=('group/g1', ),
role_base_category='group',
local_role_group_value=self.portal.portal_categories.local_role_group.Other,
)
self.portal.portal_types.Person.newContent(
portal_type='Role Information',
role_name='Assignor',
role_category_list=('group/g2', ),
role_base_category='group',
local_role_group_value=self.portal.portal_categories.local_role_group.Alternate,
)
self.portal.portal_types.Currency.newContent(
portal_type='Role Information',
role_name='Assignor',
role_category_list=('group/g1', ),
role_base_category='group',
local_role_group_value=self.portal.portal_categories.local_role_group.Alternate,
)
self.portal.currency_module.newContent(portal_type='Currency')
collision_worklist = self.portal.portal_workflow.security_uid_test_simulation_workflow.newContent(
portal_type='Worklist',
reference='collision_worklist',
action_name='collision_worklist',
action_type='global',
action='/?portal_type:list=%(portal_type)s&local_roles:list=%(local_roles)s&validation_state=%(validation_state)s'
)
collision_worklist.setCriterion('portal_type', ('Organisation',))
collision_worklist.setCriterion('local_roles', ('Assignee', 'Assignor'))
collision_worklist.setCriterion('validation_state', ('draft',))
def remove_worklist():
self.portal.portal_workflow.security_uid_test_simulation_workflow.manage_delObjects(
[collision_worklist.getId()])
self.tic()
self.addCleanup(remove_worklist)
user_g1 = self.portal.person_module.newContent(
portal_type='Person',
user_id='user_g1')
user_g1.newContent(portal_type='Assignment', group='g1').open()
user_g1.newContent(portal_type='ERP5 Login', reference='user_g1').validate()
self.tic()
self.portal.organisation_module.newContent(portal_type='Organisation')
self.portal.organisation_module.newContent(portal_type='Organisation').validate()
self.tic()
self.assertWorklistCount(
'user_g1',
{
collision_worklist.getReference(): 1,
# Worklist from business template
'security_uid_or_alternate_security_uid_draft': 1,
}
)
...@@ -28,6 +28,7 @@ ...@@ -28,6 +28,7 @@
""" """
Most of the code in this file has been taken from patches/WorkflowTool.py Most of the code in this file has been taken from patches/WorkflowTool.py
""" """
from collections import defaultdict
import re import re
import warnings import warnings
from six import string_types as basestring from six import string_types as basestring
...@@ -972,21 +973,17 @@ def sumCatalogResultByWorklist(grouped_worklist_dict, catalog_result): ...@@ -972,21 +973,17 @@ def sumCatalogResultByWorklist(grouped_worklist_dict, catalog_result):
It is better to avoid reading multiple times the catalog result from It is better to avoid reading multiple times the catalog result from
flexibility point of view: if it must ever be changed into a cursor, this flexibility point of view: if it must ever be changed into a cursor, this
code will keep working nicely without needing to rewind the cursor. code will keep working nicely without needing to rewind the cursor.
This code assumes that all worklists have the same set of criterion ids,
and that when a criterion id is associated with an ExclusionList it is
also true for all worklists.
""" """
worklist_result_dict = {} worklist_result_dict = {}
if len(catalog_result) > 0: if len(catalog_result) > 0:
# Transtype all worklist definitions where needed # Transtype all worklist definitions where needed
criterion_id_list = [] criterion_id_list_by_worklist_dict = defaultdict(list)
class_dict = {name: _sql_cast_dict.get(x['type'], _sql_cast_fallback) class_dict = {name: _sql_cast_dict.get(x['type'], _sql_cast_fallback)
for name, x in six.iteritems(catalog_result.data_dictionary())} for name, x in six.iteritems(catalog_result.data_dictionary())}
for criterion_dict in six.itervalues(grouped_worklist_dict): for worklist_id, criterion_dict in six.iteritems(grouped_worklist_dict):
for criterion_id, criterion_value_list in six.iteritems(criterion_dict): for criterion_id, criterion_value_list in six.iteritems(criterion_dict):
if type(criterion_value_list) is not ExclusionList: if type(criterion_value_list) is not ExclusionList:
criterion_id_list.append(criterion_id) criterion_id_list_by_worklist_dict[worklist_id].append(criterion_id)
expected_class = class_dict[criterion_id] expected_class = class_dict[criterion_id]
if type(criterion_value_list[0]) is not expected_class: if type(criterion_value_list[0]) is not expected_class:
criterion_dict[criterion_id] = frozenset([expected_class(x) for x in criterion_value_list]) criterion_dict[criterion_id] = frozenset([expected_class(x) for x in criterion_value_list])
...@@ -997,7 +994,7 @@ def sumCatalogResultByWorklist(grouped_worklist_dict, catalog_result): ...@@ -997,7 +994,7 @@ def sumCatalogResultByWorklist(grouped_worklist_dict, catalog_result):
result_count = int(result_line[COUNT_COLUMN_TITLE]) result_count = int(result_line[COUNT_COLUMN_TITLE])
for worklist_id, criterion_dict in six.iteritems(grouped_worklist_dict): for worklist_id, criterion_dict in six.iteritems(grouped_worklist_dict):
is_candidate = True is_candidate = True
for criterion_id in criterion_id_list: for criterion_id in criterion_id_list_by_worklist_dict[worklist_id]:
criterion_value_set = criterion_dict[criterion_id] criterion_value_set = criterion_dict[criterion_id]
if result_line[criterion_id] not in criterion_value_set: if result_line[criterion_id] not in criterion_value_set:
is_candidate = False is_candidate = 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