Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[13.0] Add delivery_carrier_pricelist #279

Merged
merged 1 commit into from
Mar 24, 2021

Conversation

guewen
Copy link
Member

@guewen guewen commented Aug 18, 2020

New module allowing to compute a delivery cost based on the sales order
pricelist.

It allows to have different pricing per customer, prices depending on dates, ...
The pricelist based cost is computed from the shipping method's product and the
sales order's pricelist.

It supports the following use cases:

  • When no "external" provider (e.g. DHL, UPS, ...) is used, a new provider
    "Based on Product Pricelist" is available.
  • When an external provider is used, a new option in the "Invoice Policy"
    selection, named "Pricelist Cost", overrides the provider's cost by the
    pricelist based cost.

@guewen guewen changed the title Add delivery_carrier_pricelist [13.0] Add delivery_carrier_pricelist Aug 18, 2020
@pedrobaeza
Copy link
Member

pedrobaeza commented Aug 18, 2020

I think this should be integrated with the existing 12.0 module delivery_price_method, adding a third option called "Based on pricelist". I can migrate it to v13 if you want. This way, you are not hijacking the provider (we have several providers that don't provide the prices through their APIs).

@guewen
Copy link
Member Author

guewen commented Aug 18, 2020

Thanks @pedrobaeza, the field price_method added by delivery_price_method seems redundant with the field invoice_policy added in 13.0 (https://github.com/odoo/odoo/blob/6e73574c18742d0242533058d91e4df1a1106348/addons/delivery/models/delivery_carrier.py#L48-L52). The core options for this field are

  • estimate: set the carrier's rated price when confirming the SO
  • real: set 0 in the SO and update the line when the delivery is done using the carrier's rate

As forcing a price from a fixed price, from rules or a pricelist on providers doesn't make sense with either "estimate" or "real", I think it makes sense to add new options in "invoice policy" rather than adding a new field. So delivery_price_method would not be needed in 13.0 (well, except the part which adds the fixed price / rules options but no dependency is required). What do you think?

@pedrobaeza
Copy link
Member

Thanks for the pointer, @guewen, I didn't know that new option.

I have being toying with it in the enterprise runbot (where there are more providers), and that field seems to be not optimal for adding the pricelist as it's not shown unless you select a provider different from fixed price or based on rules, so you can't use pricelists unless you have one of that providers and select it (which seems cumbersome).

About deprecating delivery_price_method, I also see problems, as although you select estimate when a provider is used, the "Pricing" page is not shown for allowing you to introduce the pricing rules, so the module still seems needed, although it can reuse that field (or maybe it's more complicated).

What do you think?

@guewen
Copy link
Member Author

guewen commented Aug 18, 2020

I have being toying with it in the enterprise runbot (where there are more providers), and that field seems to be not optimal for adding the pricelist as it's not shown unless you select a provider different from fixed price or based on rules, so you can't use pricelists unless you have one of that providers and select it (which seems cumbersome).

Which is why I added a "pricelist" provider:

Selection_917

When an external provider is used, as you can't anymore select a fixed price/rule/pricelist cost, the option is moved to the "Invoicing Policy" (the issue really comes from odoo that mixes the providers with how we compute the prices...).

Selection_918

We put a lot of thoughts with @jgrandguillaume to find the least bad solution. We considered adding a new field (alike the price_method field) but then, using a pricelist when using "rules" makes no sense for instance. These 3 options: Fixed price, Rule, Pricelist are exclusive, but when the type is an external provider, the pricelist is not exclusive... but then Estimate, Real and Pricelist Cost are exclusive. So, it really seems the most intuitive way to handle this.

About deprecating delivery_price_method, I also see problems, as although you select estimate when a provider is used, the "Pricing" page is not shown for allowing you to introduce the pricing rules, so the module still seems needed, although it can reuse that field (or maybe it's more complicated).

Indeed, my phrasing was not correct. The field "pricing_method" could be deprecated. But the module could still be needed to add the options for fixed prices or rules in the providers invoicing policies. That being said, I'm not sure we want to merge the "pricelist feature" with the one adding the support of fixed and rules prices for external providers, because:

  1. Fixed and rule prices do not add new ways to compute prices, they add the option to use these computations on external providers
  2. Pricelist adds a new provider to compute prices in a new way not supported in core, and incidentally, we allow to use it for external providers as well

I can imagine that we in many scenarios, we only want 1.
We should maybe make their naming consistent? Do you have a proposition?

@guewen
Copy link
Member Author

guewen commented Aug 18, 2020

1. Fixed and rule prices do not add new ways to compute prices, they add the option to use these computations on external providers

2. Pricelist adds a new provider to compute prices in a new way not supported in core, and incidentally, we allow to use it for external providers as well

I can imagine that we in many scenarios, we only want 1.

Note that it would be easy for me to add these 2 possibilities in my current module... let me know what you think.

@pedrobaeza
Copy link
Member

The problem I see putting pricelist as a provider is that you can't use pricelists then with a real provider (like SEUR, DHL, etc), and that can be interesting IMO. That's why I told you about the other module that separates the provider from the pricing method. Standard messes up this definitively, and each version includes improvements, but without fine-graining it.

What you can do? Show always the invoice_policy field, regardless of the selected provider, and add there the pricelist option. Does that make sense? If not, then what about adding an extra check in delivery.carrier use_pricelist or similar for overriding the rest of the configuration?

When migrating delivery_price_method, what I will probably do is (but a further analysis is needed):

  • Keep price_method field, but only allowing 2 values: Fixed price and base on rules.
  • That field will be shown only when estimate is selected in invoice_policy.
  • "Pricing" page will be shown according these selections.

That way, both can be compatible (taking one path or the other).

@guewen
Copy link
Member Author

guewen commented Aug 18, 2020

The problem I see putting pricelist as a provider is that you can't use pricelists then with a real provider (like SEUR, DHL, etc), and that can be interesting IMO. That's why I told you about the other module that separates the provider from the pricing method.

I think I should be missing your point, because it's exactly what I explain having solved using the "Invoice Policy".

Show always the invoice_policy field, regardless of the selected provider, and add there the pricelist option. Does that make sense?

No, it doesn't make sense to have the options "estimate" and "real" for fixed price and rules based.

Also, if we have the "fixed price" and "rules price" as a provider, it makes sense to have the "pricelist price" as a provider too.
Then, it eventually makes sense to have more options in the invoice policy of the external providers to customize their rates.

If not, then what about adding an extra check in delivery.carrier use_pricelist or similar for overriding the rest of the configuration?

We considered this with @jgrandguillaume, but discarded it because using pricelists never makes sense with a fixed price (well, kind of, or it should hide the fixed amount field anyway) or rule based, it only makes sense "as a provider" by itself or when using an external service such as DHL. And in the latter case, it doesn't make sense to use an "estimate" or "real" invoicing policy, so it really IS a third option to "invoice policy" when using an external service.

  • Keep price_method field, but only allowing 2 values: Fixed price and base on rules.

  • That field will be shown only when estimate is selected in invoice_policy.

  • "Pricing" page will be shown according these selections.

But then the options of "invoice policy" doesn't make sense anymore when you select "fixed" or "rule based" in price_method. Deciding between using an estimated rate or real rate doesn't make sense, you want a "fixed rate" or "rule based rate". I don't really get why you would like yet another field.

@pedrobaeza
Copy link
Member

pedrobaeza commented Aug 18, 2020

I think I should be missing your point, because it's exactly what I explain having solved using the "Invoice Policy".

What I'm saying here is that you can need simultaneously:

  • A provider like SEUR for integrating the communication of shippings.
  • A pricelist computation.

Putting the pricelist as provider, you are losing the first possibility.

But then the options of "invoice policy" doesn't make sense anymore when you select "fixed" or "rule based" in price_method. Deciding between using an estimated rate or real rate doesn't make sense, you want a "fixed rate" or "rule based rate". I don't really get why you would like yet another field.

I think it still makes sense, as you can estimate on order, where you can have approximate variables for computing the price (like volume, weight, etc), or make it real and recompute when validating the picking, where you have put exact data for that computation.

@guewen
Copy link
Member Author

guewen commented Aug 18, 2020

Putting the pricelist as provider, you are losing the first possibility.

But I put it both as a provider and as invoicing policy! I would need to put it both as a provider and as a "price_method", the same way you have "fixed" both in providers and "price_method"! I need it as provider in any case...

I think it still makes sense, as you can estimate on order

For the pricelist, it doesn't

@guewen
Copy link
Member Author

guewen commented Aug 18, 2020

where you can have approximate variables for computing the price (like volume, weight, etc), or make it real and recompute when validating the picking, where you have put exact data for that computation.

And that's not the behavior of the invoice policy

@pedrobaeza
Copy link
Member

OK, I think I don't know enough v13 yet. Proceed as you think it's the best.

New module allowing to compute a delivery cost based on the sales order
pricelist.

Most of the code of the module is to update the many 'attrs' of the
'delivery' module which have domains based on the "delivery_type" field
and cannot be extended in XML without breaking compatibility.

When using an external provider (such as DHL, UPS), the "Pricelist"
provider cannot be used. In this case, the invoice policy, which is by
default "Estimate" or "Real" has a third option "Pricelist Cost". This
option would not make sense with "Estimate" or "Real", which is why this
field is used.
@guewen guewen force-pushed the 13.0-delivery-carrier-pricelist branch from f8a17b1 to 498fde6 Compare August 19, 2020 06:49
@guewen guewen marked this pull request as ready for review August 19, 2020 11:24
@rousseldenis rousseldenis added this to the 13.0 milestone Aug 20, 2020
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

Copy link
Contributor

@sebalix sebalix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG, and is working on prod since a long time.

@simahawk
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 13.0-ocabot-merge-pr-279-by-simahawk-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit d121af6 into OCA:13.0 Mar 24, 2021
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at f67ac02. Thanks a lot for contributing to OCA. ❤️

victoralmau pushed a commit to Tecnativa/delivery-carrier that referenced this pull request Aug 4, 2021
Signed-off-by simahawk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants