10.0 product supplierinfo discount #407

StefanRijnhart · 2017-06-05T15:14:41Z

Apart from migrating this module to 10.0, this PR suggests to handle an issue that already existed in 9.0 in 61a32a0726: because Odoo automatically creates supplierinfo entries when a purchase order is confirmed, the purchase order lines' discounts should be added to these entries.

JonathanNEMRY · 2017-10-30T15:20:43Z

@StefanRijnhart can you rebase it? The depends is now merged so I think travis should be happy with that! 😄

legalsylvain

Code review. (only : 9c219cf)
no test.

StefanRijnhart · 2017-10-31T18:55:43Z

@JonathanNEMRY thanks for noting! Now rebased.

legalsylvain · 2017-11-01T09:28:59Z

Hi. I'm just thought about a potential improvment.

For the time being :

without this module, in product.supplierinfo, the 'price' field is the purchase price, (so the "Net price").
with this module, this field become the gross price, and we add a field 'discount'. So we change the meaning of this field.

The Idea :

keep the 'price' as the "Net Price", readonly, depends('gross_price', 'discount')
Add a new field "gross_price".
Add the field "discount" as this module does it.
Optionally, do the same thing in purchase.order.line. (so : we could remove the following lines in the readme file : https://github.com/OCA/purchase-workflow/blob/10.0/purchase_discount/README.rst

With this module, the price_unit field of purchase order line stores the gross price instead of the net price, which is a change in the meaning of this field. So this module breaks all the other modules that use the price_unit field with it's native meaning.

Pro :

keep the meaning of the original field.
having both informations for the end users. (gross / net prices).
Futur possibility to handle rounding problem more easily. round(gross_price * (1-discount/100)) * qty != round(gross_price * qty) * (1-discount/100)

Cons :

More work for this PR.
migration script required to managed correctly transition between 10.0 (or -) and 11.0 (or +)

regards.
(really not a blocking point regarding this PR)

StefanRijnhart · 2017-11-01T09:39:14Z

@legalsylvain I don't think I agree with your conclusion that this module changes the semantics of the field. Is it not the case that without this module, the price field represents both the gross price as well as the net price? You may as well add a new field for the net price instead of adding a new field for the gross price. But IMHO it is out of scope for this migration.

legalsylvain · 2017-11-01T09:48:19Z

Hi @StefanRijnhart.

I don't think I agree with your conclusion that this module changes the semantics of the field

Well, for the supplierinfo, it is a matter of PoV. I can understand your. About purchase.order.line, well it is written in the current readme file, that module breaks the meaning of the price_unit field. Maybe it's wrong, but in that case, it should be removed. Don't you think ?

But IMHO it is out of scope for this migration.

Of course, as said, not a blocking point, just a question !

rousseldenis · 2017-11-01T10:02:08Z

@legalsylvain The question had to be asked. I think these concepts have to be clear for user, at least.

pedrobaeza · 2017-11-01T15:41:24Z

The problem is that the same field, price_unit, is used for both gross value and net value in purchase core module, so it uses it for some operations as gross value and another as net value. Adding the discount, we have to interfere in all the operations where the giving meaning is the net one for adding the discount in that logic. The perfect solution would be to have both values unfolded in core (for v12 obviously), and we change the way the net value is computed in purchase_discount, purchase_triple_discount, etc.

Any way, purchase_discount handles already the only case that is related in core with net price, which is the purchase value put in the stock move for average cost price computation.

lukebranch · 2018-02-09T04:40:16Z

@StefanRijnhart ,

I'm going to work on an 11.0 port of this shortly (fundamentally just removing procurement.order). I just wanted to check, are there any changes planned on this PR before it's merged into 10.0 that I should take into consideration?

StefanRijnhart · 2018-02-09T08:47:05Z

@lukebranch no, I think this is working fine as it is now. Maybe if you review we can have it merged!

lukebranch · 2018-02-09T08:54:05Z

@StefanRijnhart ,

Brilliant, thanks for the reply. I'll take a look at this over the weekend and get back to you.

legalsylvain · 2018-03-02T23:00:33Z

@lukebranch did you take the time to review this PR ?
regards.

apesquero · 2018-03-06T17:01:16Z

I don't now why close the other PR if this PR is down!

StefanRijnhart · 2018-03-06T17:18:25Z

Hi @apesquero what is down about this PR?

apesquero · 2018-03-06T17:28:54Z

Nothing, I think this PR is perfect, but it does not finish being published.

pedrobaeza · 2018-03-06T18:49:00Z

Then make your review here and follow the process, or use it.

StefanRijnhart · 2018-03-15T09:10:40Z

@pedrobaeza as the original author, can I have your additional approval on the added change in 61a32a0726?

StefanRijnhart · 2018-03-15T09:26:03Z

@apesquero still looking forward to your review! Reviews are the only thing that can get any PR merged.

rousseldenis

Minor comments.
One can have performance impacts. Keep eye on it

rousseldenis · 2018-03-15T09:19:29Z

product_supplierinfo_discount/models/purchase_order.py

+            product_supplierinfo = self.product_id._select_seller(
+                partner_id=self.partner_id, quantity=self.product_qty,
+                date=self.order_id.date_order and
+                self.order_id.date_order[:10],


You cannot do that. Use

fields.Date.to_string(fields.Date.from_string(self.order_id.date_order))

rousseldenis · 2018-03-15T09:39:03Z

product_supplierinfo_discount/models/purchase_order.py

+        discount_map = dict(
+            [(line.product_id.product_tmpl_id.id, line.discount)
+             for line in self.order_line.filtered('discount')])
+        return super(PurchaseOrder, self.with_context(


As with_context() recreate new environment, it is strongly encouraged to avoid this to pass parameters. Performance reasons

But this works. Improvement in another version.

@rousseldenis sure, this will lead to a new read() on the purchase order and its lines but I'm a bit surprised you mention it as a bad practice. Have you got a link to guidelines from Odoo or the OCA that mention with_context as such?

Nope, that is based on experienced cases.

For this, I don't have an immediate solution, I didn't study it deeply.

apesquero · 2018-03-15T09:40:54Z

@StefanRijnhart,

I'm sorry, I'm new, it's possible that I did not express myself well, I think your code is correct. I have no review!

StefanRijnhart · 2018-03-15T10:08:13Z

@apesquero in that case, please go to the Review Changes on the Files Changed tab on this PR and select 'Approve'. Thanks!

rousseldenis · 2018-03-15T10:17:34Z

product_supplierinfo_discount/models/purchase_order.py

+        """
+        res = super(PurchaseOrderLine, self)._onchange_quantity()
+        if self.product_id:
+            date = False


@StefanRijnhart Nearly, you have to define it to None. Either function _select_seller will crash (Look at it - same case here OCA/stock-logistics-warehouse#351).

Thanks for spotting this! Changed to None and I fixed another instance of date_order[10] in this code.

when supplierinfo is created automatically

StefanRijnhart · 2018-03-15T11:47:15Z

Thanks for your reviews everyone!

pedrobaeza · 2018-03-16T16:25:56Z

I'm late to the party, but yes, very good addition in 61a32a0, @StefanRijnhart

StefanRijnhart added the work in progress label Jun 5, 2017

StefanRijnhart added this to the 10.0 milestone Jun 5, 2017

pedrobaeza mentioned this pull request Jun 5, 2017

Migration to version 10.0 #279

Closed

38 tasks

legalsylvain approved these changes Oct 30, 2017

View reviewed changes

Jonathan Nemry (ACSONE) and others added 4 commits October 31, 2017 19:45

From 8.0 to 9.0: product_supplierinfo_discount (OCA#216)

9377a88

default_supplierinfo_discount on res.partner model

2adccf9

OCA Transbot updated translations from Transifex

a014568

fix website url in manifest

87e22fe

StefanRijnhart force-pushed the 10.0-product_supplierinfo_discount branch from 9c219cf to 4b6bfad Compare October 31, 2017 18:46

legalsylvain added needs review and removed work in progress labels Mar 2, 2018

legalsylvain mentioned this pull request Mar 2, 2018

[10.0][MIG] product_supplierinfo_discount #496

Closed

rousseldenis suggested changes Mar 15, 2018

View reviewed changes

StefanRijnhart force-pushed the 10.0-product_supplierinfo_discount branch from 61a32a0 to 9f07868 Compare March 15, 2018 10:13

rousseldenis reviewed Mar 15, 2018

View reviewed changes

StefanRijnhart added 2 commits March 15, 2018 11:22

[MIG] product_supplierinfo_discount 10.0

e395df7

[IMP] Set discount from purchase order line on order confirm

a95f1bb

when supplierinfo is created automatically

StefanRijnhart force-pushed the 10.0-product_supplierinfo_discount branch from 9f07868 to a95f1bb Compare March 15, 2018 10:22

rousseldenis approved these changes Mar 15, 2018

View reviewed changes

apesquero approved these changes Mar 15, 2018

View reviewed changes

legalsylvain merged commit 474e07e into OCA:10.0 Mar 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

10.0 product supplierinfo discount #407

10.0 product supplierinfo discount #407

StefanRijnhart commented Jun 5, 2017 •

edited

Loading

JonathanNEMRY commented Oct 30, 2017

legalsylvain left a comment

StefanRijnhart commented Oct 31, 2017

legalsylvain commented Nov 1, 2017 •

edited

Loading

StefanRijnhart commented Nov 1, 2017

legalsylvain commented Nov 1, 2017

rousseldenis commented Nov 1, 2017

pedrobaeza commented Nov 1, 2017

lukebranch commented Feb 9, 2018

StefanRijnhart commented Feb 9, 2018

lukebranch commented Feb 9, 2018

legalsylvain commented Mar 2, 2018 •

edited

Loading

apesquero commented Mar 6, 2018

StefanRijnhart commented Mar 6, 2018

apesquero commented Mar 6, 2018

pedrobaeza commented Mar 6, 2018

StefanRijnhart commented Mar 15, 2018

StefanRijnhart commented Mar 15, 2018

rousseldenis left a comment

rousseldenis Mar 15, 2018

StefanRijnhart Mar 15, 2018

rousseldenis Mar 15, 2018 •

edited

Loading

StefanRijnhart Mar 15, 2018

rousseldenis Mar 15, 2018

apesquero commented Mar 15, 2018

StefanRijnhart commented Mar 15, 2018

rousseldenis Mar 15, 2018

StefanRijnhart Mar 15, 2018 •

edited

Loading

StefanRijnhart commented Mar 15, 2018

pedrobaeza commented Mar 16, 2018

10.0 product supplierinfo discount #407

10.0 product supplierinfo discount #407

Conversation

StefanRijnhart commented Jun 5, 2017 • edited Loading

JonathanNEMRY commented Oct 30, 2017

legalsylvain left a comment

Choose a reason for hiding this comment

StefanRijnhart commented Oct 31, 2017

legalsylvain commented Nov 1, 2017 • edited Loading

StefanRijnhart commented Nov 1, 2017

legalsylvain commented Nov 1, 2017

rousseldenis commented Nov 1, 2017

pedrobaeza commented Nov 1, 2017

lukebranch commented Feb 9, 2018

StefanRijnhart commented Feb 9, 2018

lukebranch commented Feb 9, 2018

legalsylvain commented Mar 2, 2018 • edited Loading

apesquero commented Mar 6, 2018

StefanRijnhart commented Mar 6, 2018

apesquero commented Mar 6, 2018

pedrobaeza commented Mar 6, 2018

StefanRijnhart commented Mar 15, 2018

StefanRijnhart commented Mar 15, 2018

rousseldenis left a comment

Choose a reason for hiding this comment

rousseldenis Mar 15, 2018

Choose a reason for hiding this comment

StefanRijnhart Mar 15, 2018

Choose a reason for hiding this comment

rousseldenis Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

StefanRijnhart Mar 15, 2018

Choose a reason for hiding this comment

rousseldenis Mar 15, 2018

Choose a reason for hiding this comment

apesquero commented Mar 15, 2018

StefanRijnhart commented Mar 15, 2018

rousseldenis Mar 15, 2018

Choose a reason for hiding this comment

StefanRijnhart Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

StefanRijnhart commented Mar 15, 2018

pedrobaeza commented Mar 16, 2018

StefanRijnhart commented Jun 5, 2017 •

edited

Loading

legalsylvain commented Nov 1, 2017 •

edited

Loading

legalsylvain commented Mar 2, 2018 •

edited

Loading

rousseldenis Mar 15, 2018 •

edited

Loading

StefanRijnhart Mar 15, 2018 •

edited

Loading