Fix some business template update bugs

Several bug fixes for business template update bugs, including a fix for the problem preventing to update erp5_accounting_l10n_fr

See merge request nexedi/erp5!1112

bt5/erp5_core_test/TestTemplateItem/portal_components/test.erp5.testBusinessTemplate.py

@@ -212,7 +212,7 @@ class BusinessTemplateMixin(ERP5TypeTestCase, LogInterceptor):
       Get a business template at portal_templates
     """
     template_tool = self.getTemplateTool()
-    for bt in template_tool.objectValues(filter={'portal_type':'Business Template'}):
+    for bt in template_tool.objectValues(filter={'portal_type': 'Business Template'}):
       if bt.getTitle() == title:
         return bt
     return None
@@ -2640,7 +2640,7 @@ class BusinessTemplateMixin(ERP5TypeTestCase, LogInterceptor):
     import_bt = sequence.get('import_bt')
     pc = self.getCatalogTool()
     catalog_id = pc.getSQLCatalog().id
-    object_to_update = {'portal_catalog/'+catalog_id+'/z_another_fake_method':'install'}
+    object_to_update = {'portal_catalog/'+catalog_id+'/z_another_fake_method': 'install'}
     import_bt.install(object_to_update=object_to_update)
 
   def stepCreateNewBusinessTemplate(self, sequence=None, **kw):
@@ -2822,7 +2822,7 @@ class BusinessTemplateMixin(ERP5TypeTestCase, LogInterceptor):
                  'template_portal_type_hidden_content_type_list',
                  'template_portal_type_property_sheet_list',
                  'template_portal_type_base_category_list'):
-          continue
+        continue
       if prop_type == 'text' or prop_type == 'string':
         prop_dict[pid] = ''
       elif prop_type == 'int':
@@ -3141,7 +3141,7 @@ class BusinessTemplateMixin(ERP5TypeTestCase, LogInterceptor):
     python_script_id = 'ERP5Site_dummyScriptWhichRandomId%s' % grain_of_sand
     skin_folder_id = 'custom'
     if getattr(self.portal.portal_skins, skin_folder_id, None) is None:
-        self.portal.portal_skins.manage_addProduct['OFSP'].manage_addFolder(skin_folder_id)
+      self.portal.portal_skins.manage_addProduct['OFSP'].manage_addFolder(skin_folder_id)
     skin_folder = self.portal.portal_skins[skin_folder_id]
     skin_folder.manage_addProduct['PythonScripts'].manage_addPythonScript(
                                                                  id=python_script_id)
@@ -6570,7 +6570,7 @@ class TestBusinessTemplate(BusinessTemplateMixin):
     finally:
       shutil.rmtree(export_dir)
 
-    new_bt.install(force=0, object_to_update={'dummy_type_provider':'remove'})
+    new_bt.install(force=0, object_to_update={'dummy_type_provider': 'remove'})
     self.assertNotEquals(None, types_tool.getTypeInfo('Base Category'))
     self.assertNotIn('dummy_type_provider', types_tool.type_provider_list)
 
@@ -6854,6 +6854,86 @@ class TestBusinessTemplate(BusinessTemplateMixin):
        {'test_document': ('Removed but should be kept', 'Path')},
        new_bt.preinstall())
 
+  def test_update_business_template_with_category_having_subcategory_tree_modified(self):
+    """Non regression test for a case where some categories in a subtrees are added and
+    some are removed.
+
+    Updating from:
+
+        portal_categories/test_category/modified
+        portal_categories/test_category/modified/container_in_which_child_is_added
+        portal_categories/test_category/modified/container_in_which_child_is_added/child_kept
+        portal_categories/test_category/modified/removed
+
+    to:
+
+        portal_categories/test_category/modified   <-- this will be modified
+        portal_categories/test_category/modified/container_in_which_child_is_added
+        portal_categories/test_category/modified/container_in_which_child_is_added/child_kept
+        portal_categories/test_category/modified/container_in_which_child_is_added/added
+
+    was causing when test_category was added both as a base category and as paths.
+
+    This was the cause of KeyError when updating erp5_accounting_l10n_fr
+    """
+    portal_categories = self.portal.portal_categories
+    if 'test_category' in portal_categories.objectIds():
+      portal_categories.manage_delObjects(['test_category'])
+    base_category = portal_categories.newContent(portal_type='Base Category', id='test_category')
+
+    parent_category = base_category.newContent(portal_type='Category', id='modified')
+    parent_category.newContent(portal_type='Category', id='container_in_which_child_is_added')
+    parent_category.newContent(portal_type='Category', id='removed')
+    parent_category.container_in_which_child_is_added.newContent(portal_type='Category', id='child_kept')
+    business_template = self.portal.portal_templates.newContent(
+      portal_type='Business Template',
+      title=self.id(),
+      template_path_list=(
+        'portal_categories/test_category/**'
+      ),
+      template_base_category_list=['test_category'],
+    )
+    self.tic()
+    business_template.build()
+    self.tic()
+    export_dir = tempfile.mkdtemp()
+    try:
+      business_template.export(path=export_dir, local=True)
+      self.tic()
+      new_business_template_version_1 = self.portal.portal_templates.download(
+         url='file://%s' % export_dir)
+    finally:
+      shutil.rmtree(export_dir)
+
+    # Apply the changes and build a second version of business template
+    parent_category.setTitle('modified')
+    parent_category.container_in_which_child_is_added.newContent(portal_type='Category', id='added')
+    parent_category.manage_delObjects(['removed'])
+
+    business_template.build()
+    self.tic()
+    export_dir = tempfile.mkdtemp()
+    try:
+      business_template.export(path=export_dir, local=True)
+      self.tic()
+      self.portal.portal_categories.manage_delObjects(['test_category'])
+      self.tic()
+      new_business_template_version_1.install()
+      self.tic()
+      portal_categories.test_category.modified.container_in_which_child_is_added.setTitle(
+          'This path should not be reinstalled during update'
+      )
+      self.tic()
+      self.portal.portal_templates.updateBusinessTemplateFromUrl(
+        download_url='file://%s' % export_dir
+      )
+    finally:
+      shutil.rmtree(export_dir)
+    self.tic()
+    self.assertEqual(
+      'This path should not be reinstalled during update',
+      portal_categories.test_category.modified.container_in_which_child_is_added.getTitle())
+
   def test_update_business_template_with_template_keep_path_list_catalog_method(self):
     """Tests for `template_keep_path_list` feature for the special case of catalog methods
     """
@@ -6909,6 +6989,60 @@ class TestBusinessTemplate(BusinessTemplateMixin):
          ('Removed but should be kept', 'CatalogMethod')},
        new_bt.preinstall())
 
+  def test_update_business_template_with_broken_objects(self):
+    """Edge case test for a folder containing broken objects being updated.
+
+    In this scenario, a document containing random broken objects that are
+    not part of business template is being installed. The broken objects
+    are kept and do not prevent installing the business template.
+    """
+    types_tool = self.portal.portal_types
+    types_tool.newContent('Geek Object', 'Base Type', type_class='Person')
+
+    types_tool.newContent(
+        'Geek Module',
+        'Base Type',
+        type_class='Folder',
+        type_filter_content_type=1,
+        type_allowed_content_type_list=('Geek Object',),
+    )
+
+    self.portal.newContent(portal_type='Geek Module', id='geek_module')
+    module_content = self.portal.geek_module.newContent(
+        portal_type='Geek Object',
+        id='1',
+        title='kept',
+    )
+    module_content_uid = module_content.getUid()
+    self.tic()
+
+    bt = self.portal.portal_templates.newContent(
+        portal_type='Business Template',
+        title=self.id(),
+        template_path_list=['geek_module'])
+    self.tic()
+    bt.build()
+    self.tic()
+
+    types_tool['Geek Object'].setTypeClass(' not exists - broken')
+    self.tic()
+    self.assertIsInstance(self.portal.geek_module['1'], ERP5BaseBroken)
+
+    export_dir = tempfile.mkdtemp()
+    try:
+      bt.export(path=export_dir, local=True)
+      self.tic()
+      new_bt = self.portal.portal_templates.download(
+          url='file://%s' % export_dir)
+    finally:
+      shutil.rmtree(export_dir)
+    new_bt.install()
+
+    # our broken document is still here and its properties have been kept
+    self.assertIsInstance(self.portal.geek_module['1'], ERP5BaseBroken)
+    self.assertEqual(module_content_uid, self.portal.geek_module['1'].uid)
+    self.assertEqual('kept', self.portal.geek_module['1'].title)
+
   def test_BusinessTemplateWithTest(self):
     sequence_list = SequenceList()
     sequence_string = '\

product/ERP5/Document/BusinessTemplate.py

@@ -1295,7 +1295,8 @@ class ObjectTemplateItem(BaseTemplateItem):
       if uid is None:
         return 0
       else:
-        document.uid = uid
+        if getattr(aq_base(document), 'uid', None) != uid:
+          document.uid = uid
         return 1
     groups = {}
     old_groups = {}
@@ -1559,7 +1560,7 @@ class ObjectTemplateItem(BaseTemplateItem):
           if update_dict.has_key(widget_path) and update_dict[widget_path] in ('remove', 'save_and_remove'):
             continue
           widget_in_form = 0
-          for group_id, group_value_list in new_groups_dict.iteritems():
+          for group_value_list in new_groups_dict.values():
             if widget_id in group_value_list:
               widget_in_form = 1
               break
@@ -1715,7 +1716,7 @@ class PathTemplateItem(ObjectTemplateItem):
     if len(id_list) == 0:
       return ['/'.join(relative_url_list)]
     id = id_list[0]
-    if re.search('[\*\?\[\]]', id) is None:
+    if re.search(r'[\*\?\[\]]', id) is None:
       # If the id has no meta character, do not have to check all objects.
       obj = folder._getOb(id, None)
       if obj is None:
@@ -1772,7 +1773,7 @@ class PathTemplateItem(ObjectTemplateItem):
       # Ignore any object without PortalType (non-ERP5 objects)
       try:
         portal_type = aq_base(obj).getPortalType()
-      except Exception, e:
+      except Exception:
         pass
       else:
         if portal_type not in p.portal_types:
@@ -1949,6 +1950,17 @@ class CategoryTemplateItem(ObjectTemplateItem):
       # reset accessors if we installed a new category
       self.portal_types.resetDynamicDocumentsOnceAtTransactionBoundary()
 
+  def install(self, context, trashbin, **kw):
+    # Only update base categories, the category they contain will be installed/updated
+    # as PathTemplateItem.install
+    kw['object_to_update'] = {
+        path: action
+        for (path, action) in kw['object_to_update'].items()
+        if path.split('/')[:-1] == ['portal_categories'] or path in self._objects
+    }
+    return super(CategoryTemplateItem, self).install(context, trashbin, **kw)
+
+
 class SkinTemplateItem(ObjectTemplateItem):
 
   def __init__(self, id_list, tool_id='portal_skins', **kw):

product/ERP5/Tool/TrashTool.py

@@ -80,59 +80,56 @@ class TrashTool(BaseTool):
       # backup the object
       # here we choose export/import to copy because cut/paste
       # do too many things and check for what we want to do
-      obj = None
-      if object_id not in backup_object_container.objectIds():
-        # export object
-        object_path = container_path + [object_id]
-        obj = self.unrestrictedTraverse(object_path, None)
-        if obj is not None:
-          connection = obj._p_jar
-          o = obj
-          while connection is None:
-            o = o.aq_parent
-            connection=o._p_jar
-          if obj._p_oid is None:
-            LOG("Trash Tool backupObject", WARNING,
-                "Trying to backup uncommitted object %s" % object_path)
-            return {}
-          if isinstance(obj, Broken):
-            LOG("Trash Tool backupObject", WARNING,
-                "Can't backup broken object %s" % object_path)
-            klass = obj.__class__
-            if klass.__module__[:27] in ('Products.ERP5Type.Document.',
-                                         'erp5.portal_type'):
-              # meta_type is required so that a broken object
-              # can be removed properly from a BTreeFolder2
-              # (unfortunately, we can only guess it)
-              klass.meta_type = 'ERP5' + re.subn('(?=[A-Z])', ' ',
-                                                 klass.__name__)[0]
-            return {}
-          copy = connection.exportFile(obj._p_oid)
-          # import object in trash
-          connection = backup_object_container._p_jar
-          o = backup_object_container
-          while connection is None:
-            o = o.aq_parent
-            connection=o._p_jar
-          copy.seek(0)
-          try:
-            backup = connection.importFile(copy)
-            backup.isIndexable = ConstantGetter('isIndexable', value=False)
-            # the isIndexable setting above avoids the recursion of
-            # manage_afterAdd on
-            # Products.ERP5Type.CopySupport.CopySupport.manage_afterAdd()
-            # but not on event subscribers, so we need to suppress_events,
-            # otherwise subobjects will be reindexed
-            backup_object_container._setObject(object_id, backup,
-                                               suppress_events=True)
-          except (AttributeError, ImportError):
-            # XXX we can go here due to formulator because attribute
-            # field_added doesn't not exists on parent if it is a Trash
-            # Folder and not a Form, or a module for the old object is
-            # already removed, and we cannot backup the object
-            LOG("Trash Tool backupObject", WARNING,
-                "Can't backup object %s" % object_path)
-            return {}
+      object_path = container_path + [object_id]
+      obj = self.unrestrictedTraverse(object_path, None)
+      if obj is not None:
+        connection = obj._p_jar
+        o = obj
+        while connection is None:
+          o = o.aq_parent
+          connection=o._p_jar
+        if obj._p_oid is None:
+          LOG("Trash Tool backupObject", WARNING,
+              "Trying to backup uncommitted object %s" % object_path)
+          return {}
+        if isinstance(obj, Broken):
+          LOG("Trash Tool backupObject", WARNING,
+              "Can't backup broken object %s" % object_path)
+          klass = obj.__class__
+          if klass.__module__[:27] in ('Products.ERP5Type.Document.',
+                                        'erp5.portal_type'):
+            # meta_type is required so that a broken object
+            # can be removed properly from a BTreeFolder2
+            # (unfortunately, we can only guess it)
+            klass.meta_type = 'ERP5' + re.subn('(?=[A-Z])', ' ',
+                                                klass.__name__)[0]
+          return {}
+        copy = connection.exportFile(obj._p_oid)
+        # import object in trash
+        connection = backup_object_container._p_jar
+        o = backup_object_container
+        while connection is None:
+          o = o.aq_parent
+          connection=o._p_jar
+        copy.seek(0)
+        try:
+          backup = connection.importFile(copy)
+          backup.isIndexable = ConstantGetter('isIndexable', value=False)
+          # the isIndexable setting above avoids the recursion of
+          # manage_afterAdd on
+          # Products.ERP5Type.CopySupport.CopySupport.manage_afterAdd()
+          # but not on event subscribers, so we need to suppress_events,
+          # otherwise subobjects will be reindexed
+          backup_object_container._setObject(object_id, backup,
+                                              suppress_events=True)
+        except (AttributeError, ImportError):
+          # XXX we can go here due to formulator because attribute
+          # field_added doesn't not exists on parent if it is a Trash
+          # Folder and not a Form, or a module for the old object is
+          # already removed, and we cannot backup the object
+          LOG("Trash Tool backupObject", WARNING,
+              "Can't backup object %s" % object_path)
+          return {}
 
     keep_sub = kw.get('keep_subobjects', 0)
     subobjects_dict = {}