From e0e161293296c651c64c01896cf6388df464bb3a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9rome=20Perrin?= <jerome@nexedi.com>
Date: Wed, 9 May 2018 04:52:06 +0200
Subject: [PATCH] CMFCategory: do not store self membership of categories

CategoryTool.getCategoryList dynamically returns categories as member of
themselves, but there were cases (for example after changing id) where
the membership happens to be saved in `categories` attribute and later
caused problems after clones or renames.
To prevent these problems, CategoryTool._setCategoryList detects the
membership to self and does not save it as part of .categories

Remove no longer needed after clone script.

Remove tests which no longer makes sense now that setCategoryList does
not add membership to self.
---
 product/CMFCategory/CategoryTool.py           | 10 ++-
 product/CMFCategory/tests/testCMFCategory.py  | 57 +++++------------
 .../erp5_core/Category_afterClone.py          |  1 -
 .../erp5_core/Category_afterClone.xml         | 62 -------------------
 4 files changed, 24 insertions(+), 106 deletions(-)
 delete mode 100644 product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/Category_afterClone.py
 delete mode 100644 product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/Category_afterClone.xml

diff --git a/product/CMFCategory/CategoryTool.py b/product/CMFCategory/CategoryTool.py
index 8723c6b5e7..a0f15fb8ec 100644
--- a/product/CMFCategory/CategoryTool.py
+++ b/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)
diff --git a/product/CMFCategory/tests/testCMFCategory.py b/product/CMFCategory/tests/testCMFCategory.py
index 981a835f4c..707e65bc15 100644
--- a/product/CMFCategory/tests/testCMFCategory.py
+++ b/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
@@ -659,52 +660,24 @@ class TestCMFCategory(ERP5TypeTestCase):
 
     """
     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_CategoryCloneIsNotMemberOfCopiedCategory(self):
-    # This is a bug reproduction test for problems happening when membership to
-    # self was saved in .categories.
-    europe_west = self.portal.portal_categories['region']['europe']['west']
-    france = europe_west.france
-
-    # Initially, categories' getCategoryList dynamically returns the
-    # "membership to self" eventhough it's not part of .categories, but after a
-    # category ID is changed, interaction update it's .categories to set the
-    # categories.
-    france.setCategoryList(france.getCategoryList())
-
-    cb_data = europe_west.manage_copyObjects(['france'])
-    destination = self.portal.portal_categories.region
-    copy_info, = destination.manage_pasteObjects(cb_data)
-    clone = destination[copy_info['new_id']]
-    self.assertEqual([clone.getRelativeUrl()], clone.getCategoryList())
-
-  def test_CategoryMovedIsNotMemberOfCopiedCategory(self):
-    # This is a bug reproduction test for problems happening when membership to
-    # self was saved in .categories.
-    original_container = self.portal.portal_categories['region']['europe']['west']
-    new_category = original_container.newContent(id=self.id())
-
-    # This sets membership to self in .categories
-    new_category.setCategoryList(new_category.getCategoryList())
-    self.commit()
+    # this membership is not saved in .categories
+    self.assertNotIn('region/europe', getattr(aq_base(europe), 'categories', ()))
 
-    cb_data = original_container.manage_cutObjects([new_category.getId()])
-    destination = self.portal.portal_categories.region
-    copy_info, = destination.manage_pasteObjects(cb_data)
-    clone = destination[copy_info['new_id']]
-    self.assertEqual([clone.getRelativeUrl()], clone.getCategoryList())
+    # even if we set explicitly
+    europe.setCategoryList(europe.getCategoryList())
+    self.assertNotIn('region/europe', getattr(aq_base(europe), 'categories', ()))
 
-  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())
+    # 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 """
diff --git a/product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/Category_afterClone.py b/product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/Category_afterClone.py
deleted file mode 100644
index 973be439b6..0000000000
--- a/product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/Category_afterClone.py
+++ /dev/null
@@ -1 +0,0 @@
-context.setCategoryMembership(context.getBaseCategoryId(), None)
diff --git a/product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/Category_afterClone.xml b/product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/Category_afterClone.xml
deleted file mode 100644
index 662366c78c..0000000000
--- a/product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/Category_afterClone.xml
+++ /dev/null
@@ -1,62 +0,0 @@
-<?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>
-- 
2.30.9