SimulationMovement.expand: optimization & bugfixes

- Stop calling rule.test() again.
- For Applied Rule objects that don't specialise to an applicable rule:
  - do not expand built ones;
  - and delete others (e.g. if a new rule matches, replace it).

product/ERP5/Document/SimulationMovement.py

@@ -316,7 +316,6 @@ class SimulationMovement(PropertyRecordableMixin, Movement, ExplainableMixin):
     if not cache_enabled:
       cache[TREE_DELIVERED_CACHE_ENABLED] = 1
 
-    applied_rule_dict = {}
     applicable_rule_dict = {}
     for rule in self._getApplicableRuleList():
       reference = rule.getReference()
@@ -325,31 +324,27 @@ class SimulationMovement(PropertyRecordableMixin, Movement, ExplainableMixin):
         # applicable rule per reference. It indicates a configuration error.
         applicable_rule_dict.setdefault(reference, rule)
 
+    applicable_rule_list = applicable_rule_dict.values()
     for applied_rule in list(self.objectValues()):
       rule = applied_rule.getSpecialiseValue()
-      # check if applied rule is already expanded, or if its portal
-      # rule is still applicable to this Simulation Movement
-
-      # XXX-Leo: possible optimization, it is likely that 'rule' is in
-      # applicable_rule_dict.values() (or actually, in
-      # self._getApplicableRuleList()). We should check if this is the
-      # case, and then not call rule.test(self), since the predicate
-      # tool will already have done it once.
-      if (rule.test(self) or
-          applied_rule._isTreeDelivered()):
-        applied_rule_dict[rule.getReference()] = applied_rule
+      try:
+        applicable_rule_list.remove(rule)
+      except ValueError:
+        if applied_rule._isTreeDelivered():
+          reference = rule.getReference()
+          try:
+            applicable_rule_list.remove(applicable_rule_dict.pop(reference))
+          except KeyError:
+            pass
+        else:
+          self._delObject(applied_rule.getId())
       else:
-        self._delObject(applied_rule.getId())
+        applied_rule.expand(**kw)
 
-    for reference, rule in applicable_rule_dict.iteritems():
-      if reference not in applied_rule_dict:
-        applied_rule = rule.constructNewAppliedRule(self, **kw)
-        applied_rule_dict[reference] = applied_rule
+    for rule in applicable_rule_list:
+      rule.constructNewAppliedRule(self, **kw).expand(**kw)
 
     self.setCausalityState('expanded')
-    # expand
-    for applied_rule in applied_rule_dict.itervalues():
-      applied_rule.expand(**kw)
 
     # disable and clear cache
     if not cache_enabled:

product/ERP5/tests/testRule.py

@@ -600,11 +600,7 @@ return context.generatePredicate(
                           key=lambda x: x.getSpecialiseValue().getReference())
     # check the 1st applied rule is an application of invoicing_rule_1
     self.assertEquals(applied_rule_list[0].getSpecialise(),
-                      invoicing_rule_1.getRelativeUrl())
-    # but also check it's the same applied rule as before instead of a new
-    # one with the same specialization
-    self.assertEqual(applied_rule_list[0],
-                      invoicing_rule_1_applied_rule)
+        invoicing_rule_n.getRelativeUrl())
     self.assertEquals(applied_rule_list[1].getSpecialise(),
         invoicing_rule_2.getRelativeUrl())
 
@@ -655,7 +651,7 @@ return context.generatePredicate(
     self.assertEquals(applied_rule.getSpecialise(),
         invoicing_rule_1.getRelativeUrl())
 
-    # invalidate the rule and test that it is still there
+    # invalidate the rule and test that it is gone
     invoicing_rule_1.invalidate()
     transaction.commit()
     self.tic()
@@ -670,10 +666,7 @@ return context.generatePredicate(
         delivery_rule.getRelativeUrl())
     self.assertEquals(root_applied_rule.objectCount(), 1)
     movement = root_applied_rule.objectValues()[0]
-    self.assertEquals(movement.objectCount(), 1)
-    applied_rule = movement.objectValues()[0]
-    self.assertEquals(applied_rule.getSpecialise(),
-        invoicing_rule_1.getRelativeUrl())
+    self.assertEquals(movement.objectCount(), 0)
 
     # change the test method to one that fails, and test that the rule is
     # removed