Category self membership after move and clone

Categories are member of themselves. When a category is renamed, the membership to self was already preserved thanks to 7f2e8eb2
, but this was not the case when a category was cut and pasted.

There are two different cases:
 * When just created, a category is member of itself because this is the [default behavior](https://lab.nexedi.com/nexedi/erp5/blob/b705495de0f04645b7b3ad3b74c46de00c923876/product/CMFCategory/CategoryTool.py#L1193-1196) for categories not having categories set. In this case, it's dynamic.
 * When category has some categories set ( in `self.categories` ) - this happens for example after being renamed - then behavior is different and `self.categories` has to be updated.

7f2e8eb2 was handling properly the later case after a clone, but not in a cut and paste ( move ) scenario, because in this case "after clone" hook is not called.

The first idea was to move the same hook at another level by overloading `_postCopy` but after some discussion and thinking, it seemed more logical to make sure `CategoryTool.setCategoryList` does not save categories membership to self, which is consistent in the way `CategoryTool.getCategoryList` adds the membership to self dynamically.

This MR brings a few more tests, so that we can claim [#1900](https://nexedi.erp5.net/bug_module/1900) and [#704](https://nexedi.erp5.net/bug_module/704) are tested enough and close these old bugs.


/reviewed-on !664

product/CMFCategory/CategoryTool.py

@@ -1189,11 +1189,19 @@ class CategoryTool(BaseTool):
     security.declareProtected( Permissions.ModifyPortalContent, '_setCategoryList' )
     def _setCategoryList(self, context, value):
       old = set(getattr(aq_base(context), 'categories', ()))
+      relative_url = context.getRelativeUrl()
+
+      # Pure category are member of itself, but we don't store this membership
+      # (because of issues when the category is renamed/moved or cloned)
+      if getattr(context, 'isCategory', 0) and relative_url in value:
+        # note that we don't want to cast as set at this point to keep ordering (and duplicates).
+        value = [x for x in tuple(value) if x != relative_url]
+
       context.categories = value = tuple(value)
       if context.isTempDocument():
         return
+
       value = set(value)
-      relative_url = context.getRelativeUrl()
       for edit, value in ("remove", old - value), ("add", value - old):
         for path in value:
           base = self.getBaseCategoryId(path)

product/CMFCategory/tests/testCMFCategory.py

@@ -29,6 +29,7 @@
 from collections import deque
 import unittest
 
+from Acquisition import aq_base
 from Products.ERP5Type.tests.ERP5TypeTestCase import ERP5TypeTestCase
 from Products.CMFCategory.Category import NBSP_UTF8
 from Testing.ZopeTestCase.PortalTestCase import PortalTestCase
@@ -483,8 +484,12 @@ class TestCMFCategory(ERP5TypeTestCase):
 
     self.assertEqual(west,
       self.portal.portal_categories.resolveCategory('region/europe/ouest'))
+    # documents using this category are updated
     self.assertEqual(p1.getRegion(), 'europe/ouest/france')
     self.assertTrue(p1 in west.getRegionRelatedValueList())
+    # category itself is also updated
+    self.assertEqual(['region/europe/ouest'], west.getCategoryList())
+    self.assertEqual(['region/europe/ouest/france'], west.france.getCategoryList())
 
   def test_13b_RenameCategoryUsingCutAndPaste(self):
     france = self.portal.portal_categories.resolveCategory(
@@ -503,8 +508,11 @@ class TestCMFCategory(ERP5TypeTestCase):
 
     self.assertEqual(west,
       self.portal.portal_categories.resolveCategory('region/west'))
+    # documents using this category are updated
     self.assertEqual(p1.getRegion(), 'west/france')
     self.assertTrue(p1 in west.getRegionRelatedValueList())
+    # category itself is also updated ( but we need to get it in its new acquisition context )
+    self.assertEqual(['region/west'], self.portal.portal_categories.region.west.getCategoryList())
 
   def test_13c_RenameCategoryUsingCutAndPasteButNotCopy(self):
     france = self.portal.portal_categories.resolveCategory(
@@ -524,11 +532,10 @@ class TestCMFCategory(ERP5TypeTestCase):
     self.assertEqual(west,
       self.portal.portal_categories.resolveCategory('region/europe/west'))
     self.assertEqual(p1.getRegion(), 'europe/west/france')
-    # we are not member of the copy
+    # documents using the category are not member of the copy
     self.assertTrue('west/france' not in p1.getRegionList())
     self.assertTrue(p1 in west.getRegionRelatedValueList())
 
-
   def test_14_MultiplePortalTypes(self):
     """ Checks that categories support different value per portal_type,
         like a colored graph on portal_type"""
@@ -648,22 +655,29 @@ class TestCMFCategory(ERP5TypeTestCase):
       a document has destination category C and we look for all documents
       which destination is part of C category, we will not find it.
 
-      For example, the following commit was a mistake:
-    http://svn.erp5.org/erp5/trunk/products/CMFCategory/CategoryTool.py?r1=8850&r2=9997
+      ( XXX not sure of this example, I guess it works because non strict
+      membership is indexed -jerome )
+
     """
     europe = self.portal.portal_categories.resolveCategory('region/europe')
-    self.assertTrue('region/europe' in europe.getCategoryList())
+    self.assertIn('region/europe', europe.getCategoryList())
     self.assertTrue(europe.isMemberOf('region/europe'))
 
-  def test_19_getCategoryList(self):
-    """
-    check that getCategoryList called on a category does not append self again
-    and again
-    """
-    region_value = self.portal.portal_categories.resolveCategory('region/%s' % self.region1)
-    category_list = region_value.getCategoryList()
-    region_value.setCategoryList(category_list)
-    self.assertEqual(category_list, region_value.getCategoryList())
+    # this membership is not saved in .categories
+    self.assertNotIn('region/europe', getattr(aq_base(europe), 'categories', ()))
+
+    # even if we set explicitly
+    europe.setCategoryList(europe.getCategoryList())
+    self.assertNotIn('region/europe', getattr(aq_base(europe), 'categories', ()))
+
+    # or if we set other categories
+    europe.setCategoryList(['subordination/person_module'])
+    self.assertItemsEqual(
+            ['region/europe', 'subordination/person_module'],
+            europe.getCategoryList())
+    self.assertEqual(
+            ('subordination/person_module',),
+            getattr(aq_base(europe), 'categories', ()))
 
   def test_19_CategoryMemberValueList(self):
     """Test strict_membership parameter to Category Member Value List """

product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/Category_afterClone.py

-context.setCategoryMembership(context.getBaseCategoryId(), None)

product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/Category_afterClone.xml

-<?xml version="1.0"?>
-<ZopeData>
-  <record id="1" aka="AAAAAAAAAAE=">
-    <pickle>
-      <global name="PythonScript" module="Products.PythonScripts.PythonScript"/>
-    </pickle>
-    <pickle>
-      <dictionary>
-        <item>
-            <key> <string>Script_magic</string> </key>
-            <value> <int>3</int> </value>
-        </item>
-        <item>
-            <key> <string>_bind_names</string> </key>
-            <value>
-              <object>
-                <klass>
-                  <global name="NameAssignments" module="Shared.DC.Scripts.Bindings"/>
-                </klass>
-                <tuple/>
-                <state>
-                  <dictionary>
-                    <item>
-                        <key> <string>_asgns</string> </key>
-                        <value>
-                          <dictionary>
-                            <item>
-                                <key> <string>name_container</string> </key>
-                                <value> <string>container</string> </value>
-                            </item>
-                            <item>
-                                <key> <string>name_context</string> </key>
-                                <value> <string>context</string> </value>
-                            </item>
-                            <item>
-                                <key> <string>name_m_self</string> </key>
-                                <value> <string>script</string> </value>
-                            </item>
-                            <item>
-                                <key> <string>name_subpath</string> </key>
-                                <value> <string>traverse_subpath</string> </value>
-                            </item>
-                          </dictionary>
-                        </value>
-                    </item>
-                  </dictionary>
-                </state>
-              </object>
-            </value>
-        </item>
-        <item>
-            <key> <string>_params</string> </key>
-            <value> <string></string> </value>
-        </item>
-        <item>
-            <key> <string>id</string> </key>
-            <value> <string>Category_afterClone</string> </value>
-        </item>
-      </dictionary>
-    </pickle>
-  </record>
-</ZopeData>