Merge lp:~therp-nl/ocb-addons/lp754339 into lp:ocb-addons

[Holger Brunn (Therp)]

[FIX] This is a port of the fix to have prices in manually added invoice lines originate from the partner's pricelist, if any. If there are no pricelists involved, nothing should change.

This is a port of the fix to have prices in manually added invoice lines originate from the partner's pricelist, if any. If there are no pricelists involved, nothing should change.

[Stefan Rijnhart (Opener) (stefan-opener)]

Thanks for taking the effort of forward porting (and improving) this old fix for 6.1 to 7.0! I sure hope this important fix lands in upstream one day.

l.66 seems wrong. I think in 7.0 you need to append it to the invoice line's name field like the original code does in l.42.

l.8..12 seems cosmetic, can it be restored in that case?

Such a large code change would warrant adding a test with pricelist (and one without if it does not yet exist).

review: Needs Fixing

[Ronald Portier (Therp) (rportier1962)]

Thanks also from my side!

When I compare this merge proposal with the original one for 6.1 therp backports (which became ocb 6.1), I note a few things.

(compare: https://code.launchpad.net/~therp-nl/therp-backports/addons-6.1-lp754339_pricelist_on_invoice_line/+merge/157829)

1. There is no layout change for the function definition.

2. There seem to be a lot of extra changes between lines 40 and 72 that were not part of the original fix/merge. Is this on purpose, or an artifact of converting a 6.1 fix to 7.0?

[Holger Brunn (Therp) (hbrunn)]

This branch is a (nearly) verbatim copy of the old trunk branch
https://code.launchpad.net/~therp-nl/openobject-addons/[e-mail]_fix_invoice_pricelist_lp754339/+merge/142021

Ronald: Any chance you remember why you introduced the changes you mention?

The test sounds like a very good idea, setting to work in progress.

[Ronald Portier (Therp) (rportier1962)]

Holger: I did not, at least not knowingly, introduce those changes.

I think they can be undone, as they seem to have nothing to do with the problem that this merge is supposed to fix.

[Holger Brunn (Therp) (hbrunn)]

Thank you two for your input, I removed the offending changes (still can't really explain where they come from), reverted the cosmetic one and added a test

review: Needs Resubmitting

[Stefan Rijnhart (Opener) (stefan-opener)]

Thanks, great work! I think you can save some lines by calling product_product._compute_price() but I am guessing that either Ronald or you didn't think it was a good idea to call a private method which is fair enough.

review: Approve

[Stefan Rijnhart (Opener) (stefan-opener)]

Forgot to mention: product_product._compute_price calculates a given price between two uoms which could replace ll. 107 to 114

[Holger Brunn (Therp) (hbrunn)]

good idea, thanks!

review: Needs Resubmitting

[Stefan Rijnhart (Opener) (stefan-opener)]

Very nice. Thank you.

review: Approve

[Chadwick-ferguson (chadwick-ferguson)]

Thank you!

I can migrate from gnucash multiple banks for multiple clients now! This works in ocb-addons, had trouble getting unit price to change with your other patch for openobject-addons

[Holger Brunn (Therp) (hbrunn)]

Thanks for testing this! Could you (as a comment on the merge proposal for openobject-addons) elaborate on the problems you faced there?

[Ronald Portier (Therp) (rportier1962)]

Approve. Tested the original code. Present code LGTM.

review: Approve

[Christophe CHAUVET (christophe-chauvet)]

Hi

It's OK for me (already fix in 6.1)

Regards,

review: Approve (code review, no test)

[Christophe CHAUVET (christophe-chauvet)]

Hi

After the merge, the buildbit failed, the problem is in the line 106-107, method called since the browse record have too enougth arguments (_compute_price() takes at most 6 arguments (10 given))

I must revert proprely this merge proposal

Regards,

2013-11-02 10:33:14,060 25494 ERROR openerp_buildbot-ocb-7.0 openerp.tools.yaml_import: _compute_price() takes at most 6 arguments (10 given)
Traceback (most recent call last):
  File "/srv/buildslave/buildbot.anybox.r/ocb-7_0-postgresql-9_2/build/parts/openerp/openerp/tools/yaml_import.py", line 864, in process
    self._process_node(node)
  File "/srv/buildslave/buildbot.anybox.r/ocb-7_0-postgresql-9_2/build/parts/openerp/openerp/tools/yaml_import.py", line 875, in _process_node
    self.process_record(node)
  File "/srv/buildslave/buildbot.anybox.r/ocb-7_0-postgresql-9_2/build/parts/openerp/openerp/tools/yaml_import.py", line 325, in process_record
    record_dict = self._create_record(model, fields, view_info, default=default)
  File "/srv/buildslave/buildbot.anybox.r/ocb-7_0-postgresql-9_2/build/parts/openerp/openerp/tools/yaml_import.py", line 415, in _create_record
    field_value = self._eval_field(model, field_name, fields[field_name], one2many_form_view or view_info, parent=record_dict, default=default)
  File "/srv/buildslave/buildbot.anybox.r/ocb-7_0-postgresql-9_2/build/parts/openerp/openerp/tools/yaml_import.py", line 515, in _eval_field
    value = [(0, 0, self._create_record(other_model, fields, view_info, parent, default=default)) for fields in expression]
  File "/srv/buildslave/buildbot.anybox.r/ocb-7_0-postgresql-9_2/build/parts/openerp/openerp/tools/yaml_import.py", line 450, in _create_record
    result = getattr(model, match.group(1))(self.cr, SUPERUSER_ID, [], *args)
  File "/srv/buildslave/buildbot.anybox.r/ocb-7_0-postgresql-9_2/build/addons/account/account_invoice.py", line 1531, in product_id_change
    currency_id, context=context)
  File "/srv/buildslave/buildbot.anybox.r/ocb-7_0-postgresql-9_2/build/addons/account/account_invoice.py", line 1599, in _price_unit_get
    cr, uid, product_obj.uom_id.id, product_obj[field], uom_id)
  File "/srv/buildslave/buildbot.anybox.r/ocb-7_0-postgresql-9_2/build/parts/openerp/openerp/osv/orm.py", line 374, in function_proxy
    return attr(self._cr, self._uid, [self._id], *args, **kwargs)
TypeError: _compute_price() takes at most 6 arguments (10 given)

[Christophe CHAUVET (christophe-chauvet)]

I have revert this MP

This MP must be fix, see the previous comment.

Regards,

review: Needs Fixing

[Stefan Rijnhart (Opener) (stefan-opener)]

Christophe, thank you for your swift actions! I see that _compute_price is called on a uom browse record instead of on a model object, which implicitely passes cr, uid, [res_id] and context. The first three arguments are passed explicitely too in line 106, while the method does not take a context argument at all.

Sorry I missed this in my review.

review: Needs Fixing

[Stefan Rijnhart (Opener) (stefan-opener)]

Note that the changes must be reapplied manually (i.e. using diff/patch or so) on this branch after a merge of the target branch, because of the revert (currently, merging this branch on a copy of ocb-addons/7.0 will tell you 'nothing to do')

[lp:~therp-nl/ocb-addons/lp754339]

9555. By Holger Brunn (Therp) on 2013-11-03

[FIX] call function on model, not browse record
[FIX] use standard_price for purchase invoices/refunds, list_price
otherwise

9556. By Holger Brunn (Therp) on 2013-11-03

[MRG] merge with upstream

9557. By Holger Brunn (Therp) on 2013-11-03

[IMP] apply original changes

[Holger Brunn (Therp) (hbrunn)]

Thanks for your efforts!

I think I found another big problem: Choosing list_price and standard_price were swapped in 63ff.

Let's tightly review this again before merging.

review: Needs Resubmitting

[lp:~therp-nl/ocb-addons/lp754339]

9558. By Holger Brunn (Therp) on 2013-11-03

[ADD] re-add test

[Stefan Rijnhart (Opener) (stefan-opener)]

Improvements look good. Tests run fine now. I hope we can get this back in again. How can your average OpenERP partner sell service hours without this patch?

There is a trivial conflict now in an __init__.py.

Looks like the proposal to upstream needs to be updated with the latest changes?

review: Needs Fixing (test, code review)

[lp:~therp-nl/ocb-addons/lp754339]

9559. By Holger Brunn (Therp) on 2014-03-24

[MRG] merge with upstream

[Holger Brunn (Therp) (hbrunn)]

Thanks, I did the merge and also updated trunk's patch

[Stefan Rijnhart (Opener) (stefan-opener)]

Thanks!

review: Approve

[Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives)]

This looks fine, but I feel it should really be a module rather than a patch.
I would certainly not think of OCB if I needed this feature, and those using the official addons could use it too.

review: Disapprove (should be a module)

[Holger Brunn (Therp) (hbrunn)]

In principle, I agree with Lionel.

Two arguments for putting this into ocb rather than into a module

- doing this in a module most likely won't work well with other modules that fiddle with the onchange method (weak argument)

- it's in 6.1ocb, so an upgrade to 7.0ocb would break existing functionality (strong argument)

[Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives)]

Thanks for making this clear, I see your point.
I'll let the OCA team decide this on a strategic level.

review: Abstain

[Holger Brunn (Therp) (hbrunn)]

Development for 7.0 has moved to github on https://github.com/OCA/ocb - please move your merge proposal there if it is still valid.

(I close and reject this in order to have a cleaner overview for 6.1 MPs which indeed have to be done on launchpad)

review: Disapprove

Unmerged revisions

9559. By Holger Brunn (Therp) on 2014-03-24

[MRG] merge with upstream

9558. By Holger Brunn (Therp) on 2013-11-03

[ADD] re-add test

9557. By Holger Brunn (Therp) on 2013-11-03

[IMP] apply original changes

9556. By Holger Brunn (Therp) on 2013-11-03

[MRG] merge with upstream

9555. By Holger Brunn (Therp) on 2013-11-03

[FIX] call function on model, not browse record
[FIX] use standard_price for purchase invoices/refunds, list_price
otherwise

=== modified file 'account/account_invoice.py'
--- account/account_invoice.py	2014-03-24 09:12:54 +0000
+++ account/account_invoice.py	2014-03-24 10:14:06 +0000
@@ -1518,10 +1518,19 @@
             taxes = res.supplier_taxes_id and res.supplier_taxes_id or (a and self.pool.get('account.account').browse(cr, uid, a, context=context).tax_ids or False)
         tax_id = fpos_obj.map_tax(cr, uid, fpos, taxes)
 
-        if type in ('in_invoice', 'in_refund'):
-            result.update( {'price_unit': price_unit or res.standard_price,'invoice_line_tax_id': tax_id} )
-        else:
-            result.update({'price_unit': res.list_price, 'invoice_line_tax_id': tax_id})
+        result['invoice_line_tax_id'] = tax_id
+
+        warning = {}
+        # When product changes, price ALWAYS need to be reset. If not price
+        # found in product, or pricelist, it should become False. Only if
+        # product_id has been cleared by user, we will leave price_unit as is.
+        if  product:
+            price_unit, pu_warning = self._price_unit_get(
+                cr, uid, product, uom_id, qty, type, partner_id,
+                currency_id, context=context)
+            result['price_unit'] = price_unit  # might be False
+            warning.update(pu_warning)
+
         result['name'] = res.partner_ref
 
         result['uos_id'] = uom_id or res.uom_id.id
@@ -1530,26 +1539,81 @@
 
         domain = {'uos_id':[('category_id','=',res.uom_id.category_id.id)]}
 
-        res_final = {'value':result, 'domain':domain}
-
-        if not company_id or not currency_id:
-            return res_final
-
-        company = self.pool.get('res.company').browse(cr, uid, company_id, context=context)
-        currency = self.pool.get('res.currency').browse(cr, uid, currency_id, context=context)
-
-        if company.currency_id.id != currency.id:
-            if type in ('in_invoice', 'in_refund'):
-                res_final['value']['price_unit'] = res.standard_price
-            new_price = res_final['value']['price_unit'] * currency.rate
-            res_final['value']['price_unit'] = new_price
-
-        if result['uos_id'] and result['uos_id'] != res.uom_id.id:
-            selected_uom = self.pool.get('product.uom').browse(cr, uid, result['uos_id'], context=context)
-            new_price = self.pool.get('product.uom')._compute_price(cr, uid, res.uom_id.id, res_final['value']['price_unit'], result['uos_id'])
-            res_final['value']['price_unit'] = new_price
+        res_final = {'value': result, 'domain': domain, 'warning': warning}
         return res_final
 
+    def _price_unit_get(
+            self, cr, uid, product_id, uom_id, qty, invoice_type, partner_id,
+            currency_id, context=None):
+        price_unit = False
+        warning = {}
+        standard_currency_id = currency_id
+        partner_model = self.pool.get('res.partner')
+        partner_obj = partner_model.browse(
+            cr, uid, partner_id, context=context)
+        assert partner_obj, _('No partner found for id %d') % partner_id
+        if invoice_type in ('in_invoice', 'in_refund'):
+            field = 'standard_price'
+            pricelist_property = 'property_product_pricelist_purchase'
+        else:
+            field = 'list_price'
+            pricelist_property = 'property_product_pricelist'
+        if  pricelist_property in partner_obj:
+            pricelist_id = partner_obj[pricelist_property].id
+        else:
+            pricelist_id = False
+        # Check whether standard price p.u. modified by pricelist
+        if  pricelist_id:
+            pricelist_model = self.pool.get('product.pricelist')
+            price_unit = pricelist_model.price_get(
+                cr, uid, [pricelist_id], product_id, qty or 1.0,
+                partner=partner_id,
+                context=dict(context, uom=uom_id))[pricelist_id]
+            if  price_unit is False:  # 0.0 is OK, we night have free products
+                warning = {
+                   'title': _('No valid pricelist line found!'),
+                   'message':
+                    _("Couldn't find a pricelist line matching this product"
+                    " and quantity.\n"
+                    "You have to change either the product, the quantity or"
+                    " the pricelist.")
+                }
+            # Pricelist converts price from standard currency to pricelist
+            # currency. We have to convert this to the invoice currency.
+            # In practice that will often mean undoing the conversion
+            # done by the pricelist object
+            pricelist_obj = pricelist_model.browse(
+                    cr, uid, pricelist_id, context=context)
+            if  pricelist_obj and pricelist_obj.currency_id:
+                standard_currency_id = pricelist_obj.currency_id.id
+        else:
+            # Take standard price per unit directly from product
+            product_model = self.pool.get('product.product')
+            product_obj = product_model.browse(
+                cr, uid, product_id, context=context)
+            assert product_obj, _('No product found for id %d') % product_id
+            assert field in product_obj, _(
+                'Field %s not found in product') % field
+            price_unit = self.pool.get('product.uom')._compute_price(
+                    cr, uid, product_obj.uom_id.id, product_obj[field], uom_id)
+            # When price not taken from pricelist, the currency is
+            # determined by the price_type:
+            price_type_model = self.pool.get('product.price.type')
+            price_type_ids = price_type_model.search(
+                cr, uid, [('field', '=', field)], context=context)
+            if  price_type_ids:
+                price_type_obj = price_type_model.browse(
+                    cr, uid, price_type_ids[0], context=context)
+                if price_type_obj and price_type_obj.currency_id:
+                    standard_currency_id = price_type_obj.currency_id.id
+        # convert price_unit to currency of invoice
+        if  standard_currency_id != currency_id:
+            currency_model = self.pool.get('res.currency')
+            price_unit = currency_model.compute(
+                cr, uid, standard_currency_id, currency_id,
+                price_unit, round=True, context=context)
+        return price_unit, warning
+
     def uos_id_change(self, cr, uid, ids, product, uom, qty=0, name='', type='out_invoice', partner_id=False, fposition_id=False, price_unit=False, currency_id=False, context=None, company_id=None):
         if context is None:
             context = {}

=== modified file 'account/tests/__init__.py'
--- account/tests/__init__.py	2014-03-10 08:54:20 +0000
+++ account/tests/__init__.py	2014-03-24 10:14:06 +0000
@@ -1,7 +1,9 @@
 from . import test_tax
+from . import test_product_id_change
 from . import test_search
 
 fast_suite = [
-	test_tax,
-	test_search,
+        test_tax,
+        test_product_id_change,
+        test_search,
 ]

=== added file 'account/tests/test_product_id_change.py'
--- account/tests/test_product_id_change.py	1970-01-01 00:00:00 +0000
+++ account/tests/test_product_id_change.py	2014-03-24 10:14:06 +0000
@@ -0,0 +1,94 @@
+from openerp.tests.common import TransactionCase
+
+class TestProductIdChange(TransactionCase):
+    """Test for product_id_change on account.invoice.line
+    """
+
+    def setUp(self):
+        super(TestProductIdChange, self).setUp()
+        self.line_model = self.registry('account.invoice.line')
+
+    def get_line(self):
+        line_id = self.line_model.create(
+                self.cr,
+                self.uid,
+                {
+                    'name': 'testline',
+                })
+        return self.line_model.browse(
+                self.cr, self.uid, line_id)
+
+
+    def get_partner(self):
+        partner_id = self.registry('res.partner').search(
+                self.cr,
+                self.uid,
+                [('customer', '=', True)],
+                limit=1)[0]
+
+        return self.registry('res.partner').browse(
+                self.cr, self.uid, partner_id)
+
+    def get_product(self):
+        product_id = self.registry('product.product').search(
+                self.cr,
+                self.uid,
+                [('uom_id', '!=', False)],
+                limit=1)[0]
+        return self.registry('product.product').browse(
+                self.cr, self.uid, product_id)
+
+    def test_random_product(self):
+        line = self.get_line()
+        product = self.get_product()
+        partner = self.get_partner()
+
+        values = line.product_id_change(
+                product.id, None,
+                partner_id=partner.id)['value']
+
+        self.assertEquals(values['price_unit'], product.list_price)
+        self.assertEquals(values['uos_id'], product.uom_id.id)
+
+    def test_with_pricelist(self):
+        line = self.get_line()
+        product = self.get_product()
+
+        pricelist_id = self.registry('product.pricelist').create(
+                self.cr, 
+                self.uid,
+                {
+                    'name': 'testpricelist',
+                    'type': self.browse_ref('product.pricelist_type_sale').key,
+                    'version_id': [
+                        (0, 0, {
+                            'name': 'testversion',
+                            'items_id': [
+                                (0, 0, {
+                                    'name': 'testitem',
+                                    'product_id': product.id,
+                                    'price_discount': .5,
+                                    'price_surcharge': 42,
+                                    'base': self.browse_ref(
+                                        'product.list_price').id,
+                                    }),
+                                ],
+                            }),
+                        ],
+                })
+
+        partner_id = self.registry('res.partner').create(
+                self.cr,
+                self.uid,
+                {
+                    'name': 'testcustomer',
+                    'customer': True,
+                    'property_product_pricelist':
+                        'product.pricelist,%d' % pricelist_id,
+                })
+
+        values = line.product_id_change(
+                product.id, None,
+                partner_id=partner_id)['value']
+
+        self.assertEquals(values['price_unit'], product.list_price * 1.5 + 42)