[12.0][IMP] delivery_multi_destination: Introduce 'based on destination' delivery type

[carmenbianca]

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

---

Internal task: https://gestion.coopiteasy.be/web#id=9184&model=project.task&view_type=form&menu_id=

[carmenbianca]

For completeness' sake, here's the first spaghetti attempt. It may or may not actually work.

Diff

diff --git a/delivery_multi_destination/models/delivery_carrier.py b/delivery_multi_destination/models/delivery_carrier.py
index e497c3ed..77d42147 100644
--- a/delivery_multi_destination/models/delivery_carrier.py
+++ b/delivery_multi_destination/models/delivery_carrier.py
@@ -23,6 +23,83 @@ class DeliveryCarrier(models.Model):
         ],
         default="one", required=True,
     )
+    delivery_type = fields.Selection(
+        selection_add=[("base_on_destination", "Based on Destination")]
+    )
+
+    @api.constrains("destination_type", "delivery_type")
+    def _check_destination_delivery_type(self):
+        """When destination_type is 'multi', delivery_type must be
+        'base_on_destination', and vice versa.
+        """
+        for carrier in self:
+            if (
+                carrier.destination_type == "multi"
+                and carrier.delivery_type != "base_on_destination"
+            ):
+                raise ValidationError(
+                    _(
+                        "Because the Destination Type is set to 'Multiple"
+                        " Destinations', the Provider must be 'Based on"
+                        " Destination'."
+                    )
+                )
+            if (
+                carrier.delivery_type == "base_on_destination"
+                and carrier.destination_type != "multi"
+            ):
+                raise ValidationError(
+                    _(
+                        "Because the Provider is set to 'Based on Destination', the"
+                        " Destination Type must be 'Multiple Destinations'."
+                    )
+                )
+
+    @api.onchange("destination_type")
+    def _onchange_destination_type(self):
+        """If destination_type is 'multi' then the delivery_type should
+        not be considered as fixed.
+        """
+        if self.destination_type == "multi":
+            self.delivery_type = "base_on_destination"
+        else:
+            self.delivery_type = "fixed"
+
+    @api.onchange("delivery_type")
+    def _onchange_delivery_type(self):
+        """Change to destination_type = 'multi' if delivery_type is changed
+        to 'base_on_destination'.
+        """
+        if self.delivery_type == "base_on_destination":
+            self.destination_type = "multi"
+
+    def write(self, vals):
+        """Imply the value of destination_type or delivery_type when one of the two is
+        not defined in a multi-destination context.
+        """
+        result = True
+        for carrier in self:
+            carrier_vals = vals.copy()
+            destination_type = carrier_vals.get("destination_type")
+            delivery_type = carrier_vals.get("delivery_type")
+            if destination_type:
+                # Switch to multi
+                if destination_type == "multi":
+                    if not delivery_type:
+                        carrier_vals["delivery_type"] = "base_on_destination"
+                # Switch away from multi
+                elif carrier.destination_type == "multi" and not delivery_type:
+                    carrier_vals["delivery_type"] == "fixed"
+            elif delivery_type:
+                # Switch to multi
+                if delivery_type == "base_on_destination":
+                    carrier_vals["destination_type"] = "multi"
+                # Switch away from multi
+                elif carrier.delivery_type == "base_on_destination":
+                    carrier_vals["destination_type"] = "one"
+            # Write to single record instead of all records.
+            result = super(DeliveryCarrier, carrier).write(carrier_vals) and result
+        return result
 
     def search(self, args, offset=0, limit=None, order=None, count=False):
         """Don't show by default children carriers."""

[carmenbianca]

Python 3.5 test failed on travis because… I don't know. It failed long before it touched any of this PR's code, though. Travis is phased out.

[remytms]

Not sure if its my brain python parser that bugs or not. ^^

[remytms]

It may be a mistake here. Shouldn't it be something like this ?

if carrier.destination_type == "multi":
    carrier.delivery_type = "base_on_destination"
elif carrier.destination_type == "one":
    carrier.delivery_type = "fixed"

[remytms]

Are you sure that tests succeed ?

[carmenbianca]

Yes, maybe this merits a comment. I wrote it like this for … spaghetti reasons. Writing the spaghetti code did things to my brain.

Presently, destination_type kind of behaves like a boolean. It is, however, not a boolean. It's a selection field. This is probably a design error, but I didn't/don't want to address that in this PR.

With that in mind, your proposed code snippet would look a little like this:

if carrier.destination_type == "multi":
    carrier.delivery_type = "base_on_destination"
# destination type is 'one' or something else
else:
    carrier.delivery_type = "fixed"

But that didn't make me happy. If we imagine a scenario where there are two non-"multi" destination types, then switching back and forth between those would re-set the delivery type to "fixed" (from a potentially valid value) every time, for no reason! Rather, we only want to re-set the delivery type when it would otherwise be invalid (i.e., when it's set to 'base_on_destination').

Of course, a module that introduces a new destination type would need to override the compute and inverse functions, but by doing things this way, we annoy downstream as little as possible.

[carmenbianca]

The tests run on my machine, and the tests succeed on Travis as well, just not on Python 3.5.

[carmenbianca]

Created a comment in a fixup commit.

[carmenbianca]

TODO: Documentation, changelog. done

[remytms]

LGTM. Just squash your fixup commit before merging.

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[carmenbianca]

Pinging @OCA/logistics-maintainers to maybe reopen this.

[rousseldenis]

Pinging @OCA/logistics-maintainers to maybe reopen this.

I can if you still need this

[carmenbianca]

I do still need this :)

[rousseldenis]

@carmenbianca Unfortunately the base branch has changed and github cannot recover it. Please open a new PR

[carmenbianca]

Done, replaced by #677