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

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).

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?

This branch is a (nearly) verbatim copy of the old trunk branch
https://code.launchpad.net/~therp-nl/openobject-addons/ronald@therp.nl_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.

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.

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

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.

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

good idea, thanks!

Very nice. Thank you.

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

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

Approve. Tested the original code. Present code LGTM.

Hi

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

Regards,

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)

I have revert this MP

This MP must be fix, see the previous comment.

Regards,

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.

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')

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.

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?

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

Thanks!

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.

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)

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

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)