[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 <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>

delivery_multi_destination/demo/delivery_carrier_demo.xml

@@ -10,6 +10,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>
 

delivery_multi_destination/migrations/12.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,8 +21,35 @@ class DeliveryCarrier(models.Model):
             ('one', 'One destination'),
             ('multi', 'Multiple destinations'),
         ],
-        default="one", required=True,
+        compute="_compute_destination_type",
+        inverse="_inverse_destination_type",
+        store=True,
     )
+    delivery_type = fields.Selection(
+        selection_add=[("base_on_destination", "Based on Destination")]
+    )
+
+    @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"
 
     def search(self, args, offset=0, limit=None, order=None, count=False):
         """Don't show by default children carriers."""

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

@@ -52,7 +52,7 @@ def setUpClass(cls):
             'name': 'Test carrier multi',
             'product_id': cls.product.id,
             'destination_type': 'multi',
-            'delivery_type': 'fixed',
+            'delivery_type': 'base_on_destination',
             'fixed_price': 100,
             'child_ids': [
                 (0, 0, {
@@ -123,6 +123,20 @@ def test_delivery_multi_destination(self):
         order.get_delivery_price()
         self.assertAlmostEqual(order.delivery_price, 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(