builder: ignore property updates by movement groups if no document can be updated

This fixes how the builder processes the result of IMovementGroup.test.
Testing deliveries to find one that can be updated should have no impact
on properties if the builder ends up creating a delivery.

bt5/erp5_base/DocumentTemplateItem/portal_components/document.erp5.OrderMovementGroup.py

@@ -42,11 +42,8 @@ class OrderMovementGroup(MovementGroup):
     return {'causality_list': [self._getOrderRelativeUrl(movement)]}
 
   def test(self, movement, property_dict, **kw):
-    if set(property_dict['causality_list']).issubset(movement.getCausalityList()):
-      property_dict['causality_list'] = movement.getCausalityList()
-      return True, property_dict
-    else:
-      return False, property_dict
+    return set(property_dict['causality_list']
+               ).issubset(movement.getCausalityList()), {}
 
   def _getOrderRelativeUrl(self, movement):
     try:

bt5/erp5_base/InterfaceTemplateItem/portal_components/interface.erp5.IMovementGroup.py

@@ -37,9 +37,17 @@ class IMovementGroup(Interface):
   """
   def test(document, property_dict, **kw):
     """Returns a tuple of 2 values.
-    First one is True if document contains identical values than some
-    contained property_dict.
-    Second one is a modified version of property_dict.
+    First one is True if processed movements can be built to 'document'.
+    Second one is a dict of properties that are set to 'document' if the
+    latter is actually chosen: it can be 'property_dict', which was the dict
+    returned by 'separate' for the considered movements.
+
+    'property_dict' shall be treated as immutable recursively. A modified
+    copy (deeply if necessary) can be returned.
+
+    A common implementation is to return a 2-tuple whose first value
+    is True when properties of 'document' are same as 'property_dict',
+    then there's usually no property to set and the second value is {}.
 
     TODO:
       - take into account the possibility to use Divergence Testers

bt5/erp5_base/MixinTemplateItem/portal_components/mixin.erp5.BuilderMixin.py

@@ -323,18 +323,6 @@ class BuilderMixin(XMLObject, Amount, Predicate):
     root_group_node.append(movement_list)
     return root_group_node
 
-  def _test(self, instance, movement_group_node_list,
-                    divergence_list):
-    result = True
-    new_property_dict_list = []
-    for movement_group_node in movement_group_node_list:
-      tmp_result, tmp_property_dict = movement_group_node.test(
-        instance, divergence_list)
-      if not tmp_result:
-        result = tmp_result
-      new_property_dict_list.append(tmp_property_dict)
-    return result, new_property_dict_list
-
   @staticmethod
   def _getSortedPropertyDict(property_dict_list):
     # Sort the edit keywords according to the order of their movement
@@ -357,7 +345,6 @@ class BuilderMixin(XMLObject, Amount, Predicate):
 
   def _findUpdatableObject(self, instance_list, movement_group_node_list,
                            divergence_list):
-    instance = None
     if instance_list:
       # we want to check the original delivery first.
       # so sort instance_list by that current is exists or not.
@@ -371,16 +358,19 @@ class BuilderMixin(XMLObject, Amount, Predicate):
           current = current.getParentValue()
       except AttributeError:
         pass
-      for instance_to_update in instance_list:
-        result, property_dict_list = self._test(
-          instance_to_update, movement_group_node_list, divergence_list)
-        if result:
-          instance = instance_to_update
-          break
-    else:
-      property_dict_list = [movement_group_node.getGroupEditDict()
-                            for movement_group_node in movement_group_node_list]
-    return instance, self._getSortedPropertyDict(property_dict_list)
+      for instance in instance_list:
+        property_dict_list = []
+        for movement_group_node in movement_group_node_list:
+          result, property_dict = movement_group_node.test(
+            instance, divergence_list)
+          if not result:
+            break
+          property_dict_list.append(property_dict)
+        else:
+          return instance, self._getSortedPropertyDict(property_dict_list)
+    return None, self._getSortedPropertyDict(
+      movement_group_node.getGroupEditDict()
+      for movement_group_node in movement_group_node_list)
 
   security.declarePrivate('buildDeliveryList')
   @UnrestrictedMethod

product/ERP5/bootstrap/erp5_core/ModuleComponentTemplateItem/portal_components/module.erp5.MovementGroup.py

@@ -164,8 +164,15 @@ class MovementGroupNode:
           if not property_list:
             return True, {}
         # else update anyway (eg. CausalityAssignmentMovementGroup etc.)
-      return self._movement_group.test(movement, self._property_dict,
-                                               property_list=property_list)
+      result, property_dict = self._movement_group.test(
+        movement, self._property_dict, property_list=property_list)
+      # The following check is partial because it does not check mutable values
+      # recursively.
+      if property_dict is self._property_dict != property_dict:
+        raise ValueError(
+          "Movement Group must not modify the passed 'property_dict':"
+          " copy it, deeply if necessary, before editing properties")
+      return result, property_dict
     else:
       return True, {}