[MIG][IMP] delivery_multi_destination: Introduce 'based on destination' delivery type #887
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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]>
|
@carmenbianca tested functionally on the runboat
|
huguesdk
force-pushed
the
16.0-mig-delivery_multi_destination-based-on-destination
branch
from
88a7398 to
0c29bca
Compare
|
@polchampion could you try again? it seems that the error you reported is unrelated to the changes. how to reproduce it? |
|
@huguesdk Can't reproduce the above error, but I tested anew and couldn't make it work.
I'm perhaps wrong on the multi-destination usage, but I'm otherwise not sure how to best test the wanted behaviour. I believe the test should work in any case. |
* take the provided domain into account in .name_search() (and add a condition to it) instead of replacing it completely. * rename the args argument of .search() to domain, as in the parent method.
* define .base_on_destination_rate_shipment() and .base_on_destination_send_shipping() instead of overriding .rate_shipment() and .send_shipping() and filtering on destination_type. * always return a value in .base_on_destination_rate_shipment() to fail nicely if no sub-carrier matches.
huguesdk
force-pushed
the
16.0-mig-delivery_multi_destination-based-on-destination
branch
from
deec538 to
dbab84b
Compare
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backported from #871
Internal task T12787
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.