Correct computation of rules

Push and pull rules were mixed. Now, search first for a pull rule, and
if nothing is found, search for a push rule.

stock_routing_operation/models/stock_routing.py

@@ -1,7 +1,9 @@
 # Copyright 2019-2020 Camptocamp (https://www.camptocamp.com)
 # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl)
 
-from odoo import fields, models
+from collections import defaultdict
+
+from odoo import fields, models, tools
 
 
 def _default_sequence(model):
@@ -43,53 +45,75 @@ class StockRouting(models.Model):
     def _default_sequence(self):
         return _default_sequence(self)
 
+    # TODO write tests for this
+    # TODO would be nice to add a constraint that would prevent to
+    # have a pull + a pull routing that would apply on the same move
     def _routing_rule_for_moves(self, moves):
         """Return a routing rule for moves
 
+        Look first for a pull routing rule, if no match, look for a push
+        routing rule.
+
         :param move: recordset of the move
         :return: dict {move: {rule: move_lines}}
         """
-        result = {move: {} for move in moves}
-        valid_rules_for_move = set()
+        self.__cached_is_rule_valid_for_move.clear_cache(self)
+        result = {
+            move: defaultdict(self.env["stock.move.line"].browse) for move in moves
+        }
+        empty_rule = self.env["stock.routing.rule"].browse()
         for move_line in moves.mapped("move_line_ids"):
             src_location = move_line.location_id
             dest_location = move_line.location_dest_id
-            location_tree = (
-                src_location._location_parent_tree()
-                + dest_location._location_parent_tree()
+            pull_location_tree = src_location._location_parent_tree()
+            push_location_tree = dest_location._location_parent_tree()
+            candidate_rules = self.env["stock.routing.rule"].search(
+                [
+                    "|",
+                    "&",
+                    ("routing_location_id", "in", pull_location_tree.ids),
+                    ("method", "=", "pull"),
+                    "&",
+                    ("routing_location_id", "in", push_location_tree.ids),
+                    ("method", "=", "push"),
+                ]
+            )
+            candidate_rules.sorted(lambda r: (r.routing_id.sequence, r.sequence))
+            rule = self._get_move_line_routing_rule(
+                move_line, pull_location_tree, candidate_rules
             )
-            candidate_routings = self.search([("location_id", "in", location_tree.ids)])
-
-            result.setdefault(move_line.move_id, [])
-            # the first location is the current move line's source or dest
-            # location, then we climb up the tree of locations
-            for loc in location_tree:
-                # and search the first allowed rule in the routing
-                routing = candidate_routings.filtered(lambda r: r.location_id == loc)
-                rules = routing.rule_ids
-                # find the first valid rule
-                found = False
-                for rule in rules:
-                    if not (
-                        (move_line.move_id, rule) in valid_rules_for_move
-                        or rule._is_valid_for_moves(move_line.move_id)
-                    ):
-                        continue
-                    # memorize the result so we don't compute it for
-                    # every move line
-                    valid_rules_for_move.add((move_line.move_id, rule))
-                    if rule in result[move_line.move_id]:
-                        result[move_line.move_id][rule] |= move_line
-                    else:
-                        result[move_line.move_id][rule] = move_line
-                    found = True
-                    break
-                if found:
-                    break
-            else:
-                empty_rule = self.env["stock.routing.rule"].browse()
-                if empty_rule in result[move_line.move_id]:
-                    result[move_line.move_id][empty_rule] |= move_line
-                else:
-                    result[move_line.move_id][empty_rule] = move_line
+            if rule:
+                result[move_line.move_id][rule] |= move_line
+                continue
+
+            rule = self._get_move_line_routing_rule(
+                move_line, push_location_tree, candidate_rules
+            )
+            if rule:
+                result[move_line.move_id][rule] |= move_line
+                continue
+
+            result[move_line.move_id][empty_rule] |= move_line
+
         return result
+
+    @tools.ormcache("rule", "move")
+    def __cached_is_rule_valid_for_move(self, rule, move):
+        """To be used only by _routing_rule_for_moves
+
+        The method _routing_rule_for_moves reset the cache at beginning.
+        Cache the result so inside _routing_rule_for_moves, we compute it
+        only once for a move an a rule.
+        """
+        return rule._is_valid_for_moves(move)
+
+    def _get_move_line_routing_rule(self, move_line, location_tree, rules):
+        # the first location is the current move line's source or dest
+        # location, then we climb up the tree of locations
+        for loc in location_tree:
+            # find the first valid rule
+            for rule in rules.filtered(lambda r: r.routing_location_id == loc):
+                if not self.__cached_is_rule_valid_for_move(rule, move_line.move_id):
+                    continue
+                return rule
+        return self.env["stock.routing.rule"].browse()

stock_routing_operation/models/stock_routing_rule.py

@@ -17,7 +17,9 @@ class StockRoutingRule(models.Model):
     routing_id = fields.Many2one(
         comodel_name="stock.routing", required=True, ondelete="cascade"
     )
-    routing_location_id = fields.Many2one(related="routing_id.location_id")
+    routing_location_id = fields.Many2one(
+        related="routing_id.location_id", store=True, index=True
+    )
     method = fields.Selection(
         selection=[("pull", "Pull"), ("push", "Push")],
         help="On pull, the routing is applied when the source location of "