Commit 9fc690ed authored by Jérome Perrin's avatar Jérome Perrin

Stop including empty items in dialog multilistfields

The meaning of empty items for **multi** list fields for categories is
not clear for dialogs (if user does not want to apply any filter, then
the natural way would be to select nothing).

This also caused issues with category fields, when the action script
uses restrictedTraverse to get the uids corresponding to the category
but a path is empty, like for example in 
https://lab.nexedi.com/nexedi/erp5/blob/d51bb0413a806b3db0c5eb69dec06065b9601322/bt5/erp5_accounting/SkinTemplateItem/portal_skins/erp5_accounting/AccountModule_getTrialBalanceReportSectionList.py#L40-48

which does this:

```python
# optional GAP filter
node_uid = []
gap_uid_list = []
for gap in request.get('gap_list', ()):
  gap_uid_list.append(portal.portal_categories.gap.restrictedTraverse(gap).getUid())
if gap_uid_list:
  node_uid = [x.uid for x in portal.portal_catalog(
                                   portal_type='Account',
                                   default_gap_uid=gap_uid_list)] or -1
```

If an empty item is selected, then `gap_uid_list` will contain an entry for 
`portal.portal_categories.gap.restrictedTraverse('').getUid()` which will be the
uid of the gap base category. Searching with a base category uid nowadays does not
match any document, but before 95e3eaec (CMFCategory: Do not index any Base Category
as a related document., 2016-12-21), it was matching all documents having a relation
from this base category and in the case of this trial balance report it was matching all
accounts.

This was a problem for old instances with accounts created before 95e3eaec, because when
they were first indexed, they had the record in category table, so they were matched, but
once they get re-indexed, they no longer had the record, so the result of this report when
selecting the empty item became different, because accounts were no longer included.

Looking back at this, maybe when updating to get 95e3eaec, we should have ran a migration
to delete all these records (re-indexing every document in the background should be enough)
so that if there's a problem, the problem happens right now and not after a few months
after accounts are modified and re-indexed.

When looking at this from end user level, theses empty items not only cause this problem,
but also does not have a clear behaviour and are not needed, so the changes here are about
removing these empty items.

In accounting reports, there was a multi listfield showing all gap categories, "grouped" by
chart of account - but the name of the chart of account was not displayed. This change to
use a None item, which is rendered as disabled to display the chart of account name, but to
do this we had to fix a bug in Formulator, these disabled items were only working properly
for single item widgets, not multiple items widgets.

See merge request nexedi/erp5!1572
parents 7b3839a5 827f36ad
Pipeline #20096 failed with stage
in 0 seconds
...@@ -263,7 +263,7 @@ ...@@ -263,7 +263,7 @@
<dictionary> <dictionary>
<item> <item>
<key> <string>_text</string> </key> <key> <string>_text</string> </key>
<value> <string>python:here.portal_categories.role.getCategoryChildTranslatedLogicalPathItemList(base=True)</string> </value> <value> <string>python:here.portal_categories.role.getCategoryChildTranslatedLogicalPathItemList(base=True, display_none_category=False)</string> </value>
</item> </item>
</dictionary> </dictionary>
</pickle> </pickle>
......
...@@ -7,32 +7,35 @@ categories from all available GAP. ...@@ -7,32 +7,35 @@ categories from all available GAP.
portal = context.getPortalObject() portal = context.getPortalObject()
display_cache = {}
def display(x): def display(x):
if x not in display_cache: gap_id = x.getReference()
gap_id = x.getReference() if gap_id:
if gap_id: return '%s - %s' % (
display_cache[x] = '%s - %s' % ( gap_id,
gap_id, x.getTranslatedShortTitle() or x.getTranslatedTitle())
x.getTranslatedShortTitle() or x.getTranslatedTitle()) return x.getIndentedTitle()
else:
display_cache[x] = x.getIndentedTitle()
return display_cache[x]
def getGapItemList(only_preferred_gap, gap_root=None): def getGapItemList(only_preferred_gap, gap_root=None):
if only_preferred_gap: if only_preferred_gap and gap_root:
if gap_root: return portal.portal_categories.resolveCategory(gap_root).getCategoryChildItemList(
return portal.portal_categories.resolveCategory(gap_root).getCategoryChildItemList( base=False,
base=False, is_self_excluded=True, display_method=display, is_self_excluded=True,
display_method=display,
display_none_category=False,
local_sort_id=('int_index', 'reference', 'id')) local_sort_id=('int_index', 'reference', 'id'))
result = [] result = []
for country in portal.portal_categories.gap.contentValues(): for gap_root_title, gap_root in context.AccountModule_getAvailableGapList():
for gap_root in country.contentValues(): if gap_root:
result.extend(gap_root.getCategoryChildItemList( result.append((gap_root_title, None))
base=False, is_self_excluded=True, display_method=display, result.extend(
local_sort_id=('int_index', 'reference', 'id'))) portal.portal_categories.resolveCategory(gap_root).getCategoryChildItemList(
base=False,
is_self_excluded=True,
display_method=display,
display_none_category=False,
local_sort_id=('int_index', 'reference', 'id')))
return result return result
from Products.ERP5Type.Cache import CachingMethod from Products.ERP5Type.Cache import CachingMethod
......
...@@ -12,6 +12,8 @@ def getSubFieldDict(): ...@@ -12,6 +12,8 @@ def getSubFieldDict():
for item in item_list: for item in item_list:
# Get value of the item # Get value of the item
item_value = item[int(not is_right_display)] item_value = item[int(not is_right_display)]
if item_value is None:
continue
# Hash key from item_value # Hash key from item_value
item_split = item_value.split('/') item_split = item_value.split('/')
......
...@@ -278,7 +278,7 @@ ...@@ -278,7 +278,7 @@
<dictionary> <dictionary>
<item> <item>
<key> <string>_text</string> </key> <key> <string>_text</string> </key>
<value> <string>python: context.ERP5Site_getAccountItemList(section_category=request.get(\'your_section_category\', preferences.getPreferredAccountingTransactionSectionCategory()), section_category_strict=request.get(\'your_section_category_strict\', preferences.getPreferredAccountingSectionCategoryStrict()), from_date=request.get(\'your_from_date\', preferences.getPreferredAccountingTransactionFromDate()))</string> </value> <value> <string>python: [item for item in context.ERP5Site_getAccountItemList(section_category=request.get(\'your_section_category\', preferences.getPreferredAccountingTransactionSectionCategory()), section_category_strict=request.get(\'your_section_category_strict\', preferences.getPreferredAccountingSectionCategoryStrict()), from_date=request.get(\'your_from_date\', preferences.getPreferredAccountingTransactionFromDate())) if item[0]]</string> </value>
</item> </item>
</dictionary> </dictionary>
</pickle> </pickle>
......
...@@ -278,7 +278,7 @@ ...@@ -278,7 +278,7 @@
<dictionary> <dictionary>
<item> <item>
<key> <string>_text</string> </key> <key> <string>_text</string> </key>
<value> <string>python: context.ERP5Site_getWorkflowStateItemList(portal_type=\'Accounting Transaction\', state_var=\'simulation_state\')</string> </value> <value> <string>python: context.ERP5Site_getWorkflowStateItemList(portal_type=\'Accounting Transaction\', state_var=\'simulation_state\', display_none_category=False)</string> </value>
</item> </item>
</dictionary> </dictionary>
</pickle> </pickle>
......
...@@ -114,7 +114,7 @@ ...@@ -114,7 +114,7 @@
<dictionary> <dictionary>
<item> <item>
<key> <string>_text</string> </key> <key> <string>_text</string> </key>
<value> <string>python: [(\'\', \'\')] + [(x.getTitle(), x.getRelativeUrl()) for x in context.portal_catalog(portal_type=\'Budget Model\')]</string> </value> <value> <string>python: [(x.getTranslatedTitle(), x.getRelativeUrl()) for x in context.portal_catalog(portal_type=\'Budget Model\')]</string> </value>
</item> </item>
</dictionary> </dictionary>
</pickle> </pickle>
......
...@@ -280,7 +280,7 @@ ...@@ -280,7 +280,7 @@
<dictionary> <dictionary>
<item> <item>
<key> <string>_text</string> </key> <key> <string>_text</string> </key>
<value> <string>python: context.ERP5Site_getWorkflowStateItemList(portal_type=\'Budget\', state_var=\'validation_state\')</string> </value> <value> <string>python: context.ERP5Site_getWorkflowStateItemList(portal_type=\'Budget\', state_var=\'validation_state\', display_none_category=False)</string> </value>
</item> </item>
</dictionary> </dictionary>
</pickle> </pickle>
......
...@@ -263,7 +263,7 @@ ...@@ -263,7 +263,7 @@
<dictionary> <dictionary>
<item> <item>
<key> <string>_text</string> </key> <key> <string>_text</string> </key>
<value> <string>python: here.portal_categories[field.getId().replace(\'your_\', \'\', 1).replace(\'_list\', \'\')].getCategoryChildCompactLogicalPathItemList(local_sort_id="int_index", checked_permission=\'View\')</string> </value> <value> <string>python: here.portal_categories[field.getId().replace(\'your_\', \'\', 1).replace(\'_list\', \'\')].getCategoryChildCompactLogicalPathItemList(local_sort_id="int_index", checked_permission=\'View\', display_none_category=False)</string> </value>
</item> </item>
</dictionary> </dictionary>
</pickle> </pickle>
......
...@@ -50,6 +50,7 @@ from Products.ERP5Form.Form import field_value_cache ...@@ -50,6 +50,7 @@ from Products.ERP5Form.Form import field_value_cache
from Products.ERP5Form.Form import getFieldValue from Products.ERP5Form.Form import getFieldValue
from Products.ERP5Form import ProxyField from Products.ERP5Form import ProxyField
from DateTime import DateTime from DateTime import DateTime
import lxml.html
from Products.Formulator.Widget import NSMAP from Products.Formulator.Widget import NSMAP
ODG_XML_WRAPPING_XPATH = 'draw:text-box/text:p/text:span' ODG_XML_WRAPPING_XPATH = 'draw:text-box/text:p/text:span'
...@@ -534,6 +535,8 @@ class TestListField(ERP5TypeTestCase): ...@@ -534,6 +535,8 @@ class TestListField(ERP5TypeTestCase):
def afterSetUp(self): def afterSetUp(self):
self.field = ListField('test_field') self.field = ListField('test_field')
self.field.values['items'] = [('My first Line', '1'), ('My Second Line', '2')]
self.field.values['default'] = '2'
self.widget = self.field.widget self.widget = self.field.widget
self.createCategories() self.createCategories()
self.tic() self.tic()
...@@ -553,9 +556,6 @@ class TestListField(ERP5TypeTestCase): ...@@ -553,9 +556,6 @@ class TestListField(ERP5TypeTestCase):
int_index=2) int_index=2)
def test_render_odt(self): def test_render_odt(self):
items = [('My first Line', '1'), ('My Second Line', '2')]
self.field.values['items'] = items
self.field.values['default'] = '2'
element = self.field.render_odt(as_string=False) element = self.field.render_odt(as_string=False)
self.assertEqual('{%(text)s}p' % NSMAP, element.tag) self.assertEqual('{%(text)s}p' % NSMAP, element.tag)
self.assertEqual('My Second Line', element.text) self.assertEqual('My Second Line', element.text)
...@@ -565,6 +565,44 @@ class TestListField(ERP5TypeTestCase): ...@@ -565,6 +565,44 @@ class TestListField(ERP5TypeTestCase):
element = self.field.render_odt(as_string=False) element = self.field.render_odt(as_string=False)
self.assertEqual('??? (3)', element.text) self.assertEqual('??? (3)', element.text)
def test_render(self):
select, input_element, = lxml.html.fragments_fromstring(self.field.render()) # pylint:disable=unbalanced-tuple-unpacking
# listfields render an input to confirm that the field was posted
# in the form's action script
self.assertEqual(input_element.name, 'default_field_test_field:int')
self.assertEqual(input_element.type, 'hidden')
self.assertEqual(select.tag, 'select')
first, second = select
self.assertEqual(first.tag, 'option')
self.assertEqual(first.text_content(), 'My first Line')
self.assertEqual(first.attrib['value'], '1')
self.assertEqual(second.tag, 'option')
self.assertEqual(second.text_content(), 'My Second Line')
self.assertEqual(second.attrib['value'], '2',)
self.assertTrue(second.attrib['selected'])
def test_render_escape_html(self):
self.field.values['default'] = ''
self.field.values['items'] = [
('<script>alert("text content")</script>', '<script>alert("value")</script>'),]
(script, ), _, = lxml.html.fragments_fromstring(self.field.render()) # pylint:disable=unbalanced-tuple-unpacking
self.assertEqual(script.attrib['value'], '<script>alert("value")</script>')
self.assertEqual(script.text_content(), '<script>alert("text content")</script>')
# selected
self.field.values['default'] = self.field.values['items'][0][1]
(script, ), _, = lxml.html.fragments_fromstring(self.field.render()) # pylint:disable=unbalanced-tuple-unpacking
self.assertEqual(script.attrib['value'], '<script>alert("value")</script>')
self.assertEqual(script.text_content(), '<script>alert("text content")</script>')
def test_render_disabled(self):
self.field.values['default'] = ''
# None items are rendered as disabled
self.field.values['items'] = [('Disabled', None)]
(disabled, ), _, = lxml.html.fragments_fromstring(self.field.render()) # pylint:disable=unbalanced-tuple-unpacking
self.assertTrue(disabled.attrib['disabled'])
self.assertFalse(disabled.attrib.get('value'))
def test_listField_value_order(self): def test_listField_value_order(self):
'''This test check the list field value order '''This test check the list field value order
...@@ -606,6 +644,46 @@ class TestMultiListField(ERP5TypeTestCase): ...@@ -606,6 +644,46 @@ class TestMultiListField(ERP5TypeTestCase):
self.field.values['items'] = [('A', 'a',), ('B', 'b')] self.field.values['items'] = [('A', 'a',), ('B', 'b')]
self.field.values['default'] = ['a', 'b'] self.field.values['default'] = ['a', 'b']
def test_render(self):
select, input_element, = lxml.html.fragments_fromstring(self.field.render()) # pylint:disable=unbalanced-tuple-unpacking
# listfields render an input to confirm that the field was posted
# in the form's action script
self.assertEqual(input_element.name, 'default_field_test_field:int')
self.assertEqual(input_element.type, 'hidden')
self.assertEqual(select.tag, 'select')
first, second = select
self.assertEqual(first.tag, 'option')
self.assertEqual(first.text_content(), 'A')
self.assertEqual(first.attrib['value'], 'a')
self.assertTrue(second.attrib['selected'])
self.assertEqual(second.tag, 'option')
self.assertEqual(second.text_content(), 'B')
self.assertEqual(second.attrib['value'], 'b',)
self.assertTrue(second.attrib['selected'])
def test_render_escape_html(self):
self.field.values['default'] = []
self.field.values['items'] = [
('<script>alert("text content")</script>', '<script>alert("value")</script>')]
(script, ), _, = lxml.html.fragments_fromstring(self.field.render()) # pylint:disable=unbalanced-tuple-unpacking
self.assertEqual(script.attrib['value'], '<script>alert("value")</script>')
self.assertEqual(script.text_content(), '<script>alert("text content")</script>')
# selected
self.field.values['default'] = [self.field.values['items'][0][1]]
(script, ), _, = lxml.html.fragments_fromstring(self.field.render()) # pylint:disable=unbalanced-tuple-unpacking
self.assertEqual(script.attrib['value'], '<script>alert("value")</script>')
self.assertEqual(script.text_content(), '<script>alert("text content")</script>')
def test_render_disabled(self):
# None items are rendered as disabled
self.field.values['default'] = []
self.field.values['items'] = [('Disabled', None)]
(disabled, ), _, = lxml.html.fragments_fromstring(self.field.render()) # pylint:disable=unbalanced-tuple-unpacking
self.assertTrue(disabled.attrib['disabled'])
self.assertFalse(disabled.attrib.get('value'))
def test_render_view(self): def test_render_view(self):
self.assertEqual('A<br />\nB', self.field.render_view(value=['a', 'b'])) self.assertEqual('A<br />\nB', self.field.render_view(value=['a', 'b']))
......
...@@ -118,7 +118,7 @@ ...@@ -118,7 +118,7 @@
<dictionary> <dictionary>
<item> <item>
<key> <string>_text</string> </key> <key> <string>_text</string> </key>
<value> <string>python: getattr(here.portal_categories[field.getId().replace(\'your_\', \'\', 1).replace(\'_relative_url\', \'\')], preferences.getPreference(\'preferred_category_child_item_list_method_id\', \'getCategoryChildCompactLogicalPathItemList\'))(local_sort_id=(\'int_index\', \'translated_title\'), checked_permission=\'View\', base=1)</string> </value> <value> <string>python: getattr(here.portal_categories[field.getId().replace(\'your_\', \'\', 1).replace(\'_relative_url\', \'\')], preferences.getPreference(\'preferred_category_child_item_list_method_id\', \'getCategoryChildCompactLogicalPathItemList\'))(local_sort_id=(\'int_index\', \'translated_title\'), checked_permission=\'View\', base=1, display_none_category=False)</string> </value>
</item> </item>
</dictionary> </dictionary>
</pickle> </pickle>
......
...@@ -118,7 +118,7 @@ ...@@ -118,7 +118,7 @@
<dictionary> <dictionary>
<item> <item>
<key> <string>_text</string> </key> <key> <string>_text</string> </key>
<value> <string>python: getattr(here.portal_categories.use, preferences.getPreference(\'preferred_category_child_item_list_method_id\', \'getCategoryChildCompactLogicalPathItemList\'))(local_sort_id=(\'int_index\', \'translated_title\'), checked_permission=\'View\', base=True)</string> </value> <value> <string>python: getattr(here.portal_categories.use, preferences.getPreference(\'preferred_category_child_item_list_method_id\', \'getCategoryChildCompactLogicalPathItemList\'))(local_sort_id=(\'int_index\', \'translated_title\'), checked_permission=\'View\', base=True, display_none_category=False)</string> </value>
</item> </item>
</dictionary> </dictionary>
</pickle> </pickle>
......
...@@ -1027,7 +1027,7 @@ class MultiItemsWidget(ItemsWidget): ...@@ -1027,7 +1027,7 @@ class MultiItemsWidget(ItemsWidget):
if item_value in value: if item_value in value:
rendered_item = self.render_selected_item( rendered_item = self.render_selected_item(
escape(ustr(item_text)), escape(ustr(item_text)),
escape(ustr(item_value)), item_value,
key, key,
css_class, css_class,
extra_item) extra_item)
...@@ -1037,7 +1037,7 @@ class MultiItemsWidget(ItemsWidget): ...@@ -1037,7 +1037,7 @@ class MultiItemsWidget(ItemsWidget):
else: else:
rendered_item = self.render_item( rendered_item = self.render_item(
escape(ustr(item_text)), escape(ustr(item_text)),
escape(ustr(item_value)), item_value,
key, key,
css_class, css_class,
extra_item) extra_item)
......
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