CMFCategory: review usage of current_category and rename it current_category_list

Since the beginning, there was in getCategoryChildItemList Renderer
API the possibility to pass a parameter 'current_category'. The idea
was to allow displaying a category which is not part of what the item
list will return (typically, if you expire a category, and if you check
permission, the category will not be in item list any more).

But the way it was implemented was too inconvenient and it looks never
used. So improve several things:
- possibility to pass multiple categories for multi value cases
- reuse renderer to render current categories instead of just reinjecting
  the url (which is not user friendly)
- implement handling of current_category_list in getCategoryChildItemList
  to allow caching the result of the Renderer. The only part which is not
  cached is the rendering of current categories.

/reviewed-on nexedi/erp5!201

product/CMFCategory/Category.py

@@ -32,7 +32,6 @@ from Products.ERP5Type.Globals import InitializeClass, DTMLFile
 from AccessControl import ClassSecurityInfo
 from AccessControl import getSecurityManager
 from Acquisition import aq_base, aq_inner, aq_parent
-from Products.CMFCore.utils import getToolByName
 
 from Products.ERP5Type import Permissions
 from Products.ERP5Type import PropertySheet
@@ -557,7 +556,8 @@ class Category(Folder):
     security.declareProtected(Permissions.AccessContentsInformation,
                               'getCategoryChildItemList')
     def getCategoryChildItemList(self, recursive=1, base=0,
-                                       cache=DEFAULT_CACHE_FACTORY, **kw):
+                                       cache=DEFAULT_CACHE_FACTORY,
+                                       current_category_list=None, **kw):
       """
       Returns a list of tuples by parsing recursively all categories in a
       given list of base categories. Each tuple contains::
@@ -575,6 +575,11 @@ class Category(Folder):
 
       recursive -- if set to 0 do not apply recursively
 
+      current_category_list -- allows to provide list of categories which is not part of the
+                          default ItemList. Very useful for displaying values in a popup
+                          menu which can no longer be selected. It is required to give
+                          url including base category.
+
       All parameters supported by getCategoryChildValueList and Render are
       supported here.
       """
@@ -582,26 +587,31 @@ class Category(Folder):
         value_list = self.getCategoryChildValueList(recursive=recursive, **kw)
         return Renderer(base=base, **kw).render(value_list)
 
-      if not cache:
-        return _renderCategoryChildItemList(
-                      recursive=recursive, base=base, **kw)
-
-      # If checked_permission is specified, we include the username in the
-      # cache key
-      username = None
-      if 'checked_permission' in kw:
-        username = str(getSecurityManager().getUser())
-
-      # Some methods are language dependent so we include the language in the
-      # key
-      localizer = getToolByName(self, 'Localizer')
-      language = localizer.get_selected_language()
-      m = CachingMethod(_renderCategoryChildItemList,
-            ('Category_getCategoryChildItemList', language,
-              self.getPath(), username),
-            cache_factory=cache)
-
-      return m(recursive=recursive, base=base, **kw)
+      if cache:
+        _renderCategoryChildItemList = CachingMethod(
+          _renderCategoryChildItemList,
+          (
+            'Category_getCategoryChildItemList',
+            self.getPortalObject().Localizer.get_selected_language(),
+            self.getPath(),
+            getSecurityManager().getUser().getId() if 'checked_permission' in kw else None,
+          ),
+          cache_factory=cache,
+        )
+      item_list = _renderCategoryChildItemList(recursive=recursive, base=base, **kw)
+
+      if current_category_list:
+        kw['display_none_category'] = False
+        current_category_item_list = Renderer(base=base, **kw).render(
+          map(self.portal_categories.resolveCategory, current_category_list))
+        item_set = {tuple(x) for x in item_list}
+        additional_item_list = []
+        for current_category_item in current_category_item_list:
+          if not(tuple(current_category_item) in item_set):
+            additional_item_list.append(current_category_item)
+        if additional_item_list:
+          item_list = item_list + additional_item_list
+      return item_list
 
     # Alias for compatibility
     security.declareProtected(Permissions.View, 'getFormItemList')

product/CMFCategory/Renderer.py

@@ -47,7 +47,7 @@ class Renderer(Filter):
                      is_right_display = 0, translate_display = 0,
                      translatation_domain = None, display_base_category = 0,
                      base_category = None, base = 1,
-                     display_none_category = 1, current_category = None,**kw):
+                     display_none_category = 1, **kw):
     """
     - *display_id*: the id of attribute to "call" to calculate the value to display
                       (getProperty(display_id) -> getDisplayId)
@@ -91,16 +91,9 @@ class Renderer(Filter):
 
     - *is_self_excluded*: allows to exclude this category from the displayed list
 
-    - *current_category*: allows to provide a category which is not part of the
-                          default ItemList. Very useful for displaying
-                          values in a popup menu which can no longer
-                          be selected.
-
     - *display_none_category*: allows to include an empty value. Very useful
                         to define None values or empty lists through
-                        popup widgets. If both has_empty_item and
-                        current_category are provided, current_category
-                        is displayed first.
+                        popup widgets.
 
 
     """
@@ -118,7 +111,6 @@ class Renderer(Filter):
     self.base_category = base_category
     self.base = base
     self.display_none_category = display_none_category
-    self.current_category = current_category
 
   def getObjectList(self, value_list):
     new_value_list = []
@@ -161,12 +153,6 @@ class Renderer(Filter):
 
     # Initialize the list of items.
     item_list = []
-    if self.current_category:
-      if self.is_right_display:
-        item = [None, self.current_category]
-      else:
-        item = [self.current_category, None]
-      item_list.append(item)
     if self.display_none_category:
       item = ['', '']
       item_list.append(item)

product/CMFCategory/tests/testCMFCategory.py

@@ -864,7 +864,16 @@ class TestCMFCategory(ERP5TypeTestCase):
                                 checked_permission='Manage portal',
                                 portal_type='Person Module'))
 
-  def test_28_getCategoryChildItemList_checked_permission(self):
+  def clearCategoryCache(self):
+    """ Clear cache to remove old categories values. It would be much
+        better to use cache cookie and interaction when category is modified. """
+    self.portal.portal_caches.clearCache(
+      cache_factory_list=('erp5_ui_long',))
+
+  def _testGetCategoryChildItemListWithCheckedPermission(self, cache=None):
+    uf = self.getPortal().acl_users
+    uf._doAddUser('alice', '', ['Member', 'Manager', 'Assignor'], [])
+    uf._doAddUser('bob', '', ['Member'], [])
     pc = self.getCategoriesTool()
 
     bc_id = 'barfoo'
@@ -877,20 +886,26 @@ class TestCMFCategory(ERP5TypeTestCase):
 
     self.assertEqual(
       [['', ''], ['A', '1'], ['B', '2'], ['B1', '2/21']],
-      bc.getCategoryChildTitleItemList(cache=0))
+      bc.getCategoryChildTitleItemList(cache=cache))
     self.assertEqual(
       [['', ''], ['A', '1'], ['B', '2'], ['B1', '2/21']],
       bc.getCategoryChildTitleItemList(checked_permission=checked_permission,
-                                       cache=0))
+                                       cache=cache))
+    self.assertEqual(
+      [['', ''], ['A', '1'], ['B', '2'], ['B1', '2/21']],
+      bc.getCategoryChildTitleItemList(checked_permission=checked_permission,
+                                       current_category_list=['barfoo/2/21', 'barfoo/1'],
+                                       cache=cache))
 
     b.manage_permission(checked_permission, roles=[], acquire=0)
+    self.clearCategoryCache()
 
     self.assertEqual(
-      3, len(bc.getCategoryChildValueList(cache=0)))
+      3, len(bc.getCategoryChildValueList(cache=cache)))
     self.assertEqual(
       1,
       len(bc.getCategoryChildValueList(checked_permission=checked_permission,
-                                       cache=0)))
+                                       cache=cache)))
 
     self.assertEqual(
       ['%s/1' % bc_id, '%s/2' % bc_id, '%s/2/21' % bc_id],
@@ -901,30 +916,41 @@ class TestCMFCategory(ERP5TypeTestCase):
 
     self.assertEqual(
       [['', ''], ['A', '1'], ['B', '2'], ['B1', '2/21']],
-      bc.getCategoryChildTitleItemList(cache=0))
+      bc.getCategoryChildTitleItemList(cache=cache))
     self.assertEqual(
       [['', ''], ['A', '1']],
       bc.getCategoryChildTitleItemList(checked_permission=checked_permission,
-                                       cache=0))
-
-  def test_28b_getCategoryChildItemList_checked_permission_cache(self):
-    # getCategoryChildTitleItemList take into account user
-    pc = self.getCategoriesTool()
-
-    bc_id = 'barfoo'
-    bc = pc.newContent(portal_type='Base Category', id=bc_id)
-    a = bc.newContent(portal_type='Category', id='1', title='A')
-    b = bc.newContent(portal_type='Category', id='2', title='B')
-    b1 = b.newContent(portal_type='Category', id='21', title='B1')
-
-    uf = self.getPortal().acl_users
-    uf._doAddUser('alice', '', ['Member', 'Manager', 'Assignor'], [])
-    uf._doAddUser('bob', '', ['Member'], [])
-    login = PortalTestCase.login
+                                       cache=cache))
+    # Verify that current_category_list parameter allows to display again
+    # hidden values
+    self.assertEqual(
+      [['', ''], ['A', '1'], ['B1', '2/21']],
+      bc.getCategoryChildTitleItemList(checked_permission=checked_permission,
+                                       current_category_list=['barfoo/2/21'],
+                                       cache=cache))
 
-    checked_permission = 'View'
+    a.manage_permission(checked_permission, roles=[], acquire=0)
+    self.clearCategoryCache()
+    self.assertEqual(
+      [['', '']],
+      bc.getCategoryChildTitleItemList(checked_permission=checked_permission,
+                                       cache=cache))
+    self.assertEqual(
+      [['', ''], ['B1', '2/21']],
+      bc.getCategoryChildTitleItemList(checked_permission=checked_permission,
+                                       current_category_list=['barfoo/2/21'],
+                                       cache=cache))
+    self.assertEqual(
+      [['', ''], ['A', '1'], ['B1', '2/21']],
+      bc.getCategoryChildTitleItemList(checked_permission=checked_permission,
+                                       current_category_list=['barfoo/1', 'barfoo/2/21'],
+                                       cache=cache))
 
+    # Result can be different depending on user, so make sure result is correct
+    # with or without cache
+    a.manage_permission(checked_permission, roles=[], acquire=1)
     b.manage_permission(checked_permission, roles=['Assignor'], acquire=0)
+    login = PortalTestCase.login
     login(self, 'alice')
     self.assertEqual(
       [['', ''], ['A', '1'], ['B', '2'], ['B1', '2/21']],
@@ -935,6 +961,11 @@ class TestCMFCategory(ERP5TypeTestCase):
       [['', ''], ['A', '1']],
       bc.getCategoryChildTitleItemList(checked_permission=checked_permission,))
 
+  def test_28_getCategoryChildItemListWithCheckedPermissionAndNoCache(self):
+    self._testGetCategoryChildItemListWithCheckedPermission(cache=None)
+
+  def test_28_getCategoryChildItemListWithCheckedPermissionAndCache(self):
+    self._testGetCategoryChildItemListWithCheckedPermission(cache='erp5_ui_long')
 
   def test_29_renameBaseCategory(self):
     bc = self.portal.portal_categories.newContent(