[10.0] Port purchase_discount #316

alexis-via · 2017-01-03T22:12:09Z

Fixes #265

Note that I still discourage users from adopting purchase_discount, because it changes the meaning of the price_unit field, which makes the module incompatible with all the modules that use the field price_unit in it's original meaning.

But I tried the known issues of the module on purchase reports and stock valuation.

pedrobaeza · 2017-01-03T23:17:16Z

Please import latest commits from 9.0 using https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-10.0 and check Travis status

pedrobaeza · 2017-01-03T23:18:22Z

And, hey, congratulations for your successfully deployment of v10 on January!

alexis-via · 2017-01-04T08:18:17Z

@pedrobaeza I checked that the code didn't change between 9.0 and 10.0, but I forgot to check translations. It's done now.

pedrobaeza · 2017-01-04T08:38:31Z

Please check PEP8:

purchase_discount/report/purchase_report.py:35:80: E501 line too long (108 > 79 characters)
purchase_discount/report/purchase_report.py:36:80: E501 line too long (113 > 79 characters)
purchase_discount/report/purchase_report.py:38:80: E501 line too long (134 > 79 characters)
purchase_discount/report/purchase_report.py:39:80: E501 line too long (206 > 79 characters)
purchase_discount/report/purchase_report.py:40:80: E501 line too long (106 > 79 characters)
purchase_discount/report/purchase_report.py:41:80: E501 line too long (188 > 79 characters)
purchase_discount/report/purchase_report.py:51:80: E501 line too long (84 > 79 characters)
purchase_discount/report/purchase_report.py:52:80: E501 line too long (160 > 79 characters)
purchase_discount/report/purchase_report.py:55:80: E501 line too long (82 > 79 characters)
purchase_discount/report/purchase_report.py:56:80: E501 line too long (120 > 79 characters)
purchase_discount/report/purchase_report.py:57:80: E501 line too long (85 > 79 characters)
purchase_discount/report/purchase_report.py:60:80: E501 line too long (93 > 79 characters)

And I would thank if you squash all the "OCA Transbot" commits in one.

legalsylvain

Hi Alexis,
Thanks for porting this usefull module. Some remarks inline. LGTM otherwise. good news to have fixed the report issue.

legalsylvain · 2017-01-04T16:08:57Z

purchase_discount/models/account_invoice_line.py

@@ -1,21 +1,14 @@
 # -*- coding: utf-8 -*-


git mv purchase_discount/models/account_invoice_line.py purchase_discount/models/account_invoice.py

legalsylvain · 2017-01-04T16:13:58Z

purchase_discount/views/purchase_discount_view.xml

-  <record model="ir.ui.view" id="purchase_discount_order_line_form2">
-    <field name="name">purchase_discount.order.line.form2 (purchase_discount)</field>
+  <record model="ir.ui.view" id="purchase_order_line_form2">
+    <field name="name">purchase_discount.order.line.form2</field>


name field is unnecessary for views. (it is autogenerated).

leemannd · 2017-01-05T10:07:46Z

Thank you for the port. Nothing to add from the comments up. 👍

leemannd · 2017-01-11T13:36:11Z

You will find a small path for the pep8 PR on your branch

StefanRijnhart

Tested and code reviewed. Thanks!

pedrobaeza · 2017-06-05T15:18:34Z

Please squash all "OCA Transbot" commits together

pedrobaeza · 2017-07-17T00:33:59Z

This is still red on Travis + need commit squashing for "OCA Transbot". Can you please do it?

eLBati · 2017-07-19T09:50:58Z

Fixing lint with akretion#3
Then, 👍

rvalyi · 2017-07-19T10:07:17Z

@pedrobaeza I merged @eLBati PEP 8 PR and squashed the "OCA Transbot" commits together.

alexis-via · 2017-07-19T10:26:52Z

I forgot to mention it here sorry, but there is a bug in the current version of the module which gives random results on the amount without tax of the purchase order line. And the fix I made on June 23 fixes some bugs, but not all. I think it's due to the code of _compute_amount on purchase.order.line https://github.com/akretion/purchase-workflow/blob/10-port-purchase_discount/purchase_discount/models/purchase_order.py#L50 which writes price_unit before calling super() and re-write it after. I'm pretty sure it causes some race condition problems.

The other problem is the fact that this module changes the meaning of the "price_unit" field from net price to gross price.

I have 1 small company using this module in production and they are very angry about it.

I think I'll completely re-write this module with a different approach where the meaning of the price_unit field is not changed and where we don't use the "trick" to re-write price_unit before and after calling super().

chienandalu · 2017-07-19T15:24:26Z

Hi, @alexis-via. I'm creating a new module that extends this PR to have three successive discounts (#432). I didn't come across tax problems on my tests. Can you give me a tip on how to replicate that kind of issues?

StefanRijnhart · 2017-07-19T21:55:56Z

You could try calling _compute_amount's super() one by one, passing actual unit price * quantity as base amount in the context https://github.com/odoo/odoo/blob/10.0/addons/account/models/account.py#L767. You probably need to assign the resulting record updates back to the original records from before the context change.

chienandalu · 2017-07-20T11:05:02Z

purchase_discount/models/purchase_order.py



 class PurchaseOrderLine(models.Model):
    _inherit = "purchase.order.line"

-    @api.depends('discount')
+    @api.depends('product_qty', 'price_unit', 'taxes_id', 'discount')
    def _compute_amount(self):
        prices = {}


@alexis-via I agree with @StefanRijnhart . When you call self super, some cache issues were happening that are solved calling from inside the recordset loop. Indeed, you could put this into a method and use it to extend the other ones instead of overriding them. Take a look at my PR if you want to (#432) 🙂

@api.depends('product_qty', 'price_unit', 'taxes_id', 'discount') def _compute_amount(self): for line in self: # super() update resets fields from cache in some circumstances. # It's convenient to have this assignation by default so other # modules can extend this method without issues. price = line.price_unit if line.discount: line.price_unit *= (1 - line.discount / 100.0) super(PurchaseOrderLine, line)._compute_amount() if line.discount: line.price_unit = price

pedrobaeza · 2017-07-21T16:56:56Z

purchase_discount/models/purchase_order.py

+    @api.multi
+    def _get_stock_move_price_unit(self):
+        # method copy-pasted from odoo/addons/purchase/models/purchase.py
+        self.ensure_one()


This is breaking inheritance also. This is worse than the previous method.

pedrobaeza · 2017-07-21T16:57:02Z

purchase_discount/models/purchase_order.py

+    @api.depends('order_line.price_total')
+    def _amount_all(self):
+        # Method copy-pasted from odoo/addons/purchase © Odoo S.A.
+        for order in self:


This can't be changed without calling super. You are breaking inheritance here.

pedrobaeza · 2017-07-21T16:58:02Z

purchase_discount/report/purchase_report.py

+    @api.model_cr
+    def init(self):
+        tools.drop_view_if_exists(self._cr, 'purchase_report')
+        self._cr.execute("""


This is also breaking inheritance. Do something similar to what I have done in https://github.com/OCA/purchase-reporting/blob/9.0/purchase_reporting_weight/reports/purchase_report.py

Fixes OCA#265

OCA Transbot updated translations from Transifex OCA Transbot updated translations from Transifex OCA Transbot updated translations from Transifex OCA Transbot updated translations from Transifex

pedrobaeza · 2017-07-22T23:39:49Z

I have improved the PR respecting inheratibility and adding tests for the untested parts (move price unit and purchase report).

chienandalu · 2017-07-24T08:46:46Z

purchase_discount/models/purchase_order.py

+            vals = {}
+            for line in order.order_line.filtered('discount'):
+                vals[line] = line.price_unit
+                line.price_unit *= (1 - line.discount) / 100.0


@pedrobaeza If you'd took this calculation into a separte method it'd make extensibility way more simple. Something of the like:

def _compute_discount(self, price): return price * (1 - self.discount or 0.0 / 100.0)

OK, I'll do that.

pedrobaeza · 2017-07-24T22:53:27Z

@chienandalu I have changed the PR for having inheritable methods. Now you only need to overwrite _get_discounted_price_unit on purchase.order.line and _get_discounted_price_unit_exp on purchase.report for changing the way the discounted unit price is computed. If you want to add discount2 and discount3 to the purchases report, just overwrite _select_purchase_discount and _group_by_purchase_discount.

pedrobaeza mentioned this pull request Jan 3, 2017

Migration to version 10.0 #279

Closed

38 tasks

legalsylvain reviewed Jan 4, 2017

View reviewed changes

leemannd approved these changes Jan 12, 2017

View reviewed changes

StefanRijnhart approved these changes Jun 5, 2017

View reviewed changes

StefanRijnhart added this to the 10.0 milestone Jun 5, 2017

StefanRijnhart added the needs review label Jun 5, 2017

StefanRijnhart mentioned this pull request Jun 5, 2017

10.0 product supplierinfo discount #407

Merged

rvalyi force-pushed the 10-port-purchase_discount branch from 600c03d to bf8d86e Compare July 19, 2017 10:05

chienandalu mentioned this pull request Jul 19, 2017

[ADD] purchase_triple_discount: New module #432

Merged

2 tasks

chienandalu reviewed Jul 20, 2017

View reviewed changes

pedrobaeza requested changes Jul 21, 2017

View reviewed changes

alexis-via and others added 3 commits July 22, 2017 22:40

Port purchase_discount to v10

9fb7504

Fixes OCA#265

OCA Transbot updated translations from Transifex

1a2aef2

OCA Transbot updated translations from Transifex OCA Transbot updated translations from Transifex OCA Transbot updated translations from Transifex OCA Transbot updated translations from Transifex

purchase_discount FIX total tax computation

c8c5c1e

pedrobaeza force-pushed the 10-port-purchase_discount branch 2 times, most recently from 0f22a23 to 4b5f561 Compare July 22, 2017 23:38

pedrobaeza approved these changes Jul 22, 2017

View reviewed changes

chienandalu reviewed Jul 24, 2017

View reviewed changes

pedrobaeza force-pushed the 10-port-purchase_discount branch from 4b5f561 to e2416e6 Compare July 24, 2017 22:51

[IMP] purchase_discount: Respect inheratibility

aceb2eb

pedrobaeza force-pushed the 10-port-purchase_discount branch from e2416e6 to aceb2eb Compare July 27, 2017 16:11

pedrobaeza merged commit 053d64c into OCA:10.0 Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[10.0] Port purchase_discount #316

[10.0] Port purchase_discount #316

alexis-via commented Jan 3, 2017

pedrobaeza commented Jan 3, 2017

pedrobaeza commented Jan 3, 2017

alexis-via commented Jan 4, 2017

pedrobaeza commented Jan 4, 2017

legalsylvain left a comment

legalsylvain Jan 4, 2017

legalsylvain Jan 4, 2017

leemannd commented Jan 5, 2017

leemannd commented Jan 11, 2017

StefanRijnhart left a comment

pedrobaeza commented Jun 5, 2017

pedrobaeza commented Jul 17, 2017

eLBati commented Jul 19, 2017

rvalyi commented Jul 19, 2017

alexis-via commented Jul 19, 2017

chienandalu commented Jul 19, 2017

StefanRijnhart commented Jul 19, 2017

chienandalu Jul 20, 2017

pedrobaeza Jul 21, 2017

pedrobaeza Jul 21, 2017

pedrobaeza Jul 21, 2017

pedrobaeza commented Jul 22, 2017

chienandalu Jul 24, 2017

pedrobaeza Jul 24, 2017

pedrobaeza commented Jul 24, 2017

[10.0] Port purchase_discount #316

[10.0] Port purchase_discount #316

Conversation

alexis-via commented Jan 3, 2017

pedrobaeza commented Jan 3, 2017

pedrobaeza commented Jan 3, 2017

alexis-via commented Jan 4, 2017

pedrobaeza commented Jan 4, 2017

legalsylvain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leemannd commented Jan 5, 2017

leemannd commented Jan 11, 2017

StefanRijnhart left a comment

Choose a reason for hiding this comment

pedrobaeza commented Jun 5, 2017

pedrobaeza commented Jul 17, 2017

eLBati commented Jul 19, 2017

rvalyi commented Jul 19, 2017

alexis-via commented Jul 19, 2017

chienandalu commented Jul 19, 2017

StefanRijnhart commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Jul 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Jul 24, 2017