[IMP] delivery_multi_destination: Introduce 'based on destination' de…

…livery type

website_sale_delivery checks whether delivery_type is set to 'fixed' for
some behaviours. And before this commit, multi-destination carriers
effectively always had the delivery_type set to 'fixed'. However,
because children of the multi-destination carrier can be something other
than 'fixed', this causes some strange behaviours.

(Example, on the delivery selection page, you expect 'Select to compute
delivery rate' to show up for non-'fixed' carriers, but this doesn't
happen.)

We can't make website_sale_delivery aware of destination_type, but we
_can_ introduce a non-'fixed' delivery type. So that's what we do here.

The choice was made to turn the destination type into a computed value
dependent on the delivery type because there is strong coupling between
the two values, and trying to keep the two values in sync using
onchange/constrains/etc resulted in terrible spaghetti code.

Co-Authored-By: Carmen Bianca BAKKER <[email protected]>
Signed-off-by: Carmen Bianca BAKKER <[email protected]>

delivery_multi_destination/__init__.py

@@ -1,3 +1,4 @@
 # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl).
 
 from . import models
+from . import wizards

delivery_multi_destination/__manifest__.py

@@ -6,7 +6,7 @@
 
 {
     "name": "Multiple destinations for the same delivery method",
-    "version": "16.0.1.0.0",
+    "version": "16.0.2.0.0",
     "category": "Delivery",
     "website": "https://github.com/OCA/delivery-carrier",
     "author": "Tecnativa, Odoo Community Association (OCA)",

delivery_multi_destination/demo/delivery_carrier_demo.xml

@@ -8,6 +8,7 @@
         <field name="name">International Carrier Inc.</field>
         <field name="sequence">4</field>
         <field name="destination_type">multi</field>
+        <field name="delivery_type">base_on_destination</field>
         <field name="product_id" ref="product_product_delivery_carrier_multi" />
     </record>
     <record id="product_product_delivery_carrier_multi_child_1" model="product.product">

delivery_multi_destination/migrations/16.0.2.0.0/pre-migration.py

@@ -0,0 +1,15 @@
+# Copyright 2022 Coop IT Easy SC
+# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl)
+from openupgradelib import openupgrade
+
+
+@openupgrade.migrate()
+def migrate(env, version):
+    openupgrade.logged_query(
+        env.cr,
+        """
+        UPDATE delivery_carrier
+        SET delivery_type = 'base_on_destination'
+        WHERE destination_type = 'multi'
+        """,
+    )

delivery_multi_destination/models/delivery_carrier.py

@@ -21,10 +21,40 @@ class DeliveryCarrier(models.Model):
         ondelete="cascade",
     )
     destination_type = fields.Selection(
-        selection=[("one", "One destination"), ("multi", "Multiple destinations")],
-        default="one",
-        required=True,
+        selection=[
+            ("one", "One destination"),
+            ("multi", "Multiple destinations"),
+        ],
+        compute="_compute_destination_type",
+        inverse="_inverse_destination_type",
+        store=True,
     )
+    delivery_type = fields.Selection(
+        selection_add=[("base_on_destination", "Based on Destination")],
+        ondelete={"base_on_destination": "set default"},
+    )
+
+    @api.depends("delivery_type")
+    def _compute_destination_type(self):
+        for carrier in self:
+            if carrier.delivery_type == "base_on_destination":
+                carrier.destination_type = "multi"
+            else:
+                carrier.destination_type = "one"
+
+    def _inverse_destination_type(self):
+        for carrier in self:
+            # Switch to multi
+            if carrier.destination_type == "multi":
+                carrier.delivery_type = "base_on_destination"
+            # Switch away from multi -> we know that destination_type is
+            # non-multi. However, in a hypothetical scenario where we switch
+            # from one non-multi destination_type to another, we don't want to
+            # forcibly reset delivery_type to 'fixed' each time, so we check
+            # whether delivery_type is invalid for a non-multi destination_type
+            # before we forcibly reset to 'fixed'.
+            elif carrier.delivery_type == "base_on_destination":
+                carrier.delivery_type = "fixed"
 
     @api.onchange("destination_type", "child_ids")
     def _onchange_destination_type(self):

delivery_multi_destination/readme/newsfragments/540.feature.rst

@@ -0,0 +1,4 @@
+Introduced 'based on destination' delivery type. This is the delivery type used
+by all multi-destination carriers instead of 'fixed'. Consequently, destination
+type has been turned into a computed field that checks if the delivery type is
+'based on destination' or not.

delivery_multi_destination/tests/test_delivery_multi_destination.py

@@ -72,6 +72,7 @@ def setUpClass(cls):
             {
                 "name": "Test carrier single",
                 "destination_type": "one",
+                "fixed_price": 100,
                 "child_ids": False,
             }
         )
@@ -84,8 +85,7 @@ def _create_carrier(self, childs):
         carrier_form = Form(self.env["delivery.carrier"])
         carrier_form.name = "Test carrier multi"
         carrier_form.product_id = self.product
-        carrier_form.delivery_type = "fixed"
-        carrier_form.fixed_price = 100
+        carrier_form.delivery_type = "base_on_destination"
         # this needs to be done in this order
         carrier_form.destination_type = "multi"
         for child_item in childs:
@@ -136,6 +136,20 @@ def test_delivery_multi_destination(self):
         sale_order_line = order.order_line.filtered("is_delivery")
         self.assertAlmostEqual(sale_order_line.price_unit, 150, 2)
 
+    def test_compute(self):
+        self.carrier_multi.delivery_type = "fixed"
+        self.assertEqual(self.carrier_multi.destination_type, "one")
+        self.carrier_multi.delivery_type = "base_on_destination"
+        self.assertEqual(self.carrier_multi.destination_type, "multi")
+
+    def test_inverse(self):
+        self.carrier_multi.destination_type = "one"
+        self.assertEqual(self.carrier_multi.destination_type, "one")
+        self.assertEqual(self.carrier_multi.delivery_type, "fixed")
+        self.carrier_multi.destination_type = "multi"
+        self.assertEqual(self.carrier_multi.destination_type, "multi")
+        self.assertEqual(self.carrier_multi.delivery_type, "base_on_destination")
+
     def test_search(self):
         carriers = self.env["delivery.carrier"].search([])
         children_carrier = self.carrier_multi.with_context(

delivery_multi_destination/wizards/__init__.py

@@ -0,0 +1,5 @@
+# SPDX-FileCopyrightText: 2024 Coop IT Easy SC
+#
+# SPDX-License-Identifier: AGPL-3.0-or-later
+
+from . import choose_delivery_carrier

delivery_multi_destination/wizards/choose_delivery_carrier.py

@@ -0,0 +1,18 @@
+# SPDX-FileCopyrightText: 2024 Coop IT Easy SC
+#
+# SPDX-License-Identifier: AGPL-3.0-or-later
+
+from odoo import api, models
+
+
+class ChooseDeliveryCarrier(models.TransientModel):
+    _inherit = "choose.delivery.carrier"
+
+    @api.onchange("carrier_id")
+    def _onchange_carrier_id(self):
+        result = super()._onchange_carrier_id()
+        if self.delivery_type == "base_on_destination":
+            vals = self._get_shipment_rate()
+            if vals.get("error_message"):
+                return {"error": vals["error_message"]}
+        return result