Merge lp:~therp-nl/ocb-addons/lp754339 into lp:ocb-addons
- lp754339
- Merge into 7.0
Status: | Rejected |
---|---|
Rejected by: | Holger Brunn (Therp) |
Proposed branch: | lp:~therp-nl/ocb-addons/lp754339 |
Merge into: | lp:ocb-addons |
Diff against target: |
242 lines (+184/-24) 3 files modified
account/account_invoice.py (+86/-22) account/tests/__init__.py (+4/-2) account/tests/test_product_id_change.py (+94/-0) |
To merge this branch: | bzr merge lp:~therp-nl/ocb-addons/lp754339 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Holger Brunn (Therp) | Disapprove | ||
Lionel Sausin - Initiatives/Numérigraphe (community) | Abstain | ||
Stefan Rijnhart (Opener) | Approve | ||
Christophe CHAUVET | Needs Fixing | ||
Ronald Portier (Therp) | Approve | ||
Review via email:
|
Commit message
[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.
Description of the 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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Stefan Rijnhart (Opener) (stefan-opener) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Ronald Portier (Therp) (rportier1962) wrote : | # |
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.
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?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Holger Brunn (Therp) (hbrunn) wrote : | # |
This branch is a (nearly) verbatim copy of the old trunk branch
https:/
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Ronald Portier (Therp) (rportier1962) wrote : | # |
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Holger Brunn (Therp) (hbrunn) wrote : | # |
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
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Stefan Rijnhart (Opener) (stefan-opener) wrote : | # |
Thanks, great work! I think you can save some lines by calling product_
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Stefan Rijnhart (Opener) (stefan-opener) wrote : | # |
Forgot to mention: product_
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Holger Brunn (Therp) (hbrunn) wrote : | # |
good idea, thanks!
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Stefan Rijnhart (Opener) (stefan-opener) wrote : | # |
Very nice. Thank you.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Chadwick-ferguson (chadwick-ferguson) wrote : | # |
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
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Holger Brunn (Therp) (hbrunn) wrote : | # |
Thanks for testing this! Could you (as a comment on the merge proposal for openobject-addons) elaborate on the problems you faced there?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Ronald Portier (Therp) (rportier1962) wrote : | # |
Approve. Tested the original code. Present code LGTM.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Christophe CHAUVET (christophe-chauvet) wrote : | # |
Hi
It's OK for me (already fix in 6.1)
Regards,
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Christophe CHAUVET (christophe-chauvet) wrote : | # |
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_
Traceback (most recent call last):
File "/srv/buildslav
self.
File "/srv/buildslav
self.
File "/srv/buildslav
record_dict = self._create_
File "/srv/buildslav
field_value = self._eval_
File "/srv/buildslav
value = [(0, 0, self._create_
File "/srv/buildslav
result = getattr(model, match.group(
File "/srv/buildslav
currency_id, context=context)
File "/srv/buildslav
cr, uid, product_
File "/srv/buildslav
return attr(self._cr, self._uid, [self._id], *args, **kwargs)
TypeError: _compute_price() takes at most 6 arguments (10 given)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Christophe CHAUVET (christophe-chauvet) wrote : | # |
I have revert this MP
This MP must be fix, see the previous comment.
Regards,
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Stefan Rijnhart (Opener) (stefan-opener) wrote : | # |
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Stefan Rijnhart (Opener) (stefan-opener) wrote : | # |
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')
- 9555. By Holger Brunn (Therp)
-
[FIX] call function on model, not browse record
[FIX] use standard_price for purchase invoices/refunds, list_price
otherwise - 9556. By Holger Brunn (Therp)
-
[MRG] merge with upstream
- 9557. By Holger Brunn (Therp)
-
[IMP] apply original changes
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Holger Brunn (Therp) (hbrunn) wrote : | # |
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.
- 9558. By Holger Brunn (Therp)
-
[ADD] re-add test
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Stefan Rijnhart (Opener) (stefan-opener) wrote : | # |
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?
- 9559. By Holger Brunn (Therp)
-
[MRG] merge with upstream
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Holger Brunn (Therp) (hbrunn) wrote : | # |
Thanks, I did the merge and also updated trunk's patch
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Stefan Rijnhart (Opener) (stefan-opener) wrote : | # |
Thanks!
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) wrote : | # |
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Holger Brunn (Therp) (hbrunn) wrote : | # |
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)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) wrote : | # |
Thanks for making this clear, I see your point.
I'll let the OCA team decide this on a strategic level.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Holger Brunn (Therp) (hbrunn) wrote : | # |
Development for 7.0 has moved to github on https:/
(I close and reject this in order to have a cleaner overview for 6.1 MPs which indeed have to be done on launchpad)
Unmerged revisions
- 9559. By Holger Brunn (Therp)
-
[MRG] merge with upstream
- 9558. By Holger Brunn (Therp)
-
[ADD] re-add test
- 9557. By Holger Brunn (Therp)
-
[IMP] apply original changes
- 9556. By Holger Brunn (Therp)
-
[MRG] merge with upstream
- 9555. By Holger Brunn (Therp)
-
[FIX] call function on model, not browse record
[FIX] use standard_price for purchase invoices/refunds, list_price
otherwise
Preview Diff
1 | === modified file 'account/account_invoice.py' |
2 | --- account/account_invoice.py 2014-03-24 09:12:54 +0000 |
3 | +++ account/account_invoice.py 2014-03-24 10:14:06 +0000 |
4 | @@ -1518,10 +1518,19 @@ |
5 | 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) |
6 | tax_id = fpos_obj.map_tax(cr, uid, fpos, taxes) |
7 | |
8 | - if type in ('in_invoice', 'in_refund'): |
9 | - result.update( {'price_unit': price_unit or res.standard_price,'invoice_line_tax_id': tax_id} ) |
10 | - else: |
11 | - result.update({'price_unit': res.list_price, 'invoice_line_tax_id': tax_id}) |
12 | + result['invoice_line_tax_id'] = tax_id |
13 | + |
14 | + warning = {} |
15 | + # When product changes, price ALWAYS need to be reset. If not price |
16 | + # found in product, or pricelist, it should become False. Only if |
17 | + # product_id has been cleared by user, we will leave price_unit as is. |
18 | + if product: |
19 | + price_unit, pu_warning = self._price_unit_get( |
20 | + cr, uid, product, uom_id, qty, type, partner_id, |
21 | + currency_id, context=context) |
22 | + result['price_unit'] = price_unit # might be False |
23 | + warning.update(pu_warning) |
24 | + |
25 | result['name'] = res.partner_ref |
26 | |
27 | result['uos_id'] = uom_id or res.uom_id.id |
28 | @@ -1530,26 +1539,81 @@ |
29 | |
30 | domain = {'uos_id':[('category_id','=',res.uom_id.category_id.id)]} |
31 | |
32 | - res_final = {'value':result, 'domain':domain} |
33 | - |
34 | - if not company_id or not currency_id: |
35 | - return res_final |
36 | - |
37 | - company = self.pool.get('res.company').browse(cr, uid, company_id, context=context) |
38 | - currency = self.pool.get('res.currency').browse(cr, uid, currency_id, context=context) |
39 | - |
40 | - if company.currency_id.id != currency.id: |
41 | - if type in ('in_invoice', 'in_refund'): |
42 | - res_final['value']['price_unit'] = res.standard_price |
43 | - new_price = res_final['value']['price_unit'] * currency.rate |
44 | - res_final['value']['price_unit'] = new_price |
45 | - |
46 | - if result['uos_id'] and result['uos_id'] != res.uom_id.id: |
47 | - selected_uom = self.pool.get('product.uom').browse(cr, uid, result['uos_id'], context=context) |
48 | - new_price = self.pool.get('product.uom')._compute_price(cr, uid, res.uom_id.id, res_final['value']['price_unit'], result['uos_id']) |
49 | - res_final['value']['price_unit'] = new_price |
50 | + res_final = {'value': result, 'domain': domain, 'warning': warning} |
51 | return res_final |
52 | |
53 | + def _price_unit_get( |
54 | + self, cr, uid, product_id, uom_id, qty, invoice_type, partner_id, |
55 | + currency_id, context=None): |
56 | + price_unit = False |
57 | + warning = {} |
58 | + standard_currency_id = currency_id |
59 | + partner_model = self.pool.get('res.partner') |
60 | + partner_obj = partner_model.browse( |
61 | + cr, uid, partner_id, context=context) |
62 | + assert partner_obj, _('No partner found for id %d') % partner_id |
63 | + if invoice_type in ('in_invoice', 'in_refund'): |
64 | + field = 'standard_price' |
65 | + pricelist_property = 'property_product_pricelist_purchase' |
66 | + else: |
67 | + field = 'list_price' |
68 | + pricelist_property = 'property_product_pricelist' |
69 | + if pricelist_property in partner_obj: |
70 | + pricelist_id = partner_obj[pricelist_property].id |
71 | + else: |
72 | + pricelist_id = False |
73 | + # Check whether standard price p.u. modified by pricelist |
74 | + if pricelist_id: |
75 | + pricelist_model = self.pool.get('product.pricelist') |
76 | + price_unit = pricelist_model.price_get( |
77 | + cr, uid, [pricelist_id], product_id, qty or 1.0, |
78 | + partner=partner_id, |
79 | + context=dict(context, uom=uom_id))[pricelist_id] |
80 | + if price_unit is False: # 0.0 is OK, we night have free products |
81 | + warning = { |
82 | + 'title': _('No valid pricelist line found!'), |
83 | + 'message': |
84 | + _("Couldn't find a pricelist line matching this product" |
85 | + " and quantity.\n" |
86 | + "You have to change either the product, the quantity or" |
87 | + " the pricelist.") |
88 | + } |
89 | + # Pricelist converts price from standard currency to pricelist |
90 | + # currency. We have to convert this to the invoice currency. |
91 | + # In practice that will often mean undoing the conversion |
92 | + # done by the pricelist object |
93 | + pricelist_obj = pricelist_model.browse( |
94 | + cr, uid, pricelist_id, context=context) |
95 | + if pricelist_obj and pricelist_obj.currency_id: |
96 | + standard_currency_id = pricelist_obj.currency_id.id |
97 | + else: |
98 | + # Take standard price per unit directly from product |
99 | + product_model = self.pool.get('product.product') |
100 | + product_obj = product_model.browse( |
101 | + cr, uid, product_id, context=context) |
102 | + assert product_obj, _('No product found for id %d') % product_id |
103 | + assert field in product_obj, _( |
104 | + 'Field %s not found in product') % field |
105 | + price_unit = self.pool.get('product.uom')._compute_price( |
106 | + cr, uid, product_obj.uom_id.id, product_obj[field], uom_id) |
107 | + # When price not taken from pricelist, the currency is |
108 | + # determined by the price_type: |
109 | + price_type_model = self.pool.get('product.price.type') |
110 | + price_type_ids = price_type_model.search( |
111 | + cr, uid, [('field', '=', field)], context=context) |
112 | + if price_type_ids: |
113 | + price_type_obj = price_type_model.browse( |
114 | + cr, uid, price_type_ids[0], context=context) |
115 | + if price_type_obj and price_type_obj.currency_id: |
116 | + standard_currency_id = price_type_obj.currency_id.id |
117 | + # convert price_unit to currency of invoice |
118 | + if standard_currency_id != currency_id: |
119 | + currency_model = self.pool.get('res.currency') |
120 | + price_unit = currency_model.compute( |
121 | + cr, uid, standard_currency_id, currency_id, |
122 | + price_unit, round=True, context=context) |
123 | + return price_unit, warning |
124 | + |
125 | 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): |
126 | if context is None: |
127 | context = {} |
128 | |
129 | === modified file 'account/tests/__init__.py' |
130 | --- account/tests/__init__.py 2014-03-10 08:54:20 +0000 |
131 | +++ account/tests/__init__.py 2014-03-24 10:14:06 +0000 |
132 | @@ -1,7 +1,9 @@ |
133 | from . import test_tax |
134 | +from . import test_product_id_change |
135 | from . import test_search |
136 | |
137 | fast_suite = [ |
138 | - test_tax, |
139 | - test_search, |
140 | + test_tax, |
141 | + test_product_id_change, |
142 | + test_search, |
143 | ] |
144 | |
145 | === added file 'account/tests/test_product_id_change.py' |
146 | --- account/tests/test_product_id_change.py 1970-01-01 00:00:00 +0000 |
147 | +++ account/tests/test_product_id_change.py 2014-03-24 10:14:06 +0000 |
148 | @@ -0,0 +1,94 @@ |
149 | +from openerp.tests.common import TransactionCase |
150 | + |
151 | +class TestProductIdChange(TransactionCase): |
152 | + """Test for product_id_change on account.invoice.line |
153 | + """ |
154 | + |
155 | + def setUp(self): |
156 | + super(TestProductIdChange, self).setUp() |
157 | + self.line_model = self.registry('account.invoice.line') |
158 | + |
159 | + def get_line(self): |
160 | + line_id = self.line_model.create( |
161 | + self.cr, |
162 | + self.uid, |
163 | + { |
164 | + 'name': 'testline', |
165 | + }) |
166 | + return self.line_model.browse( |
167 | + self.cr, self.uid, line_id) |
168 | + |
169 | + |
170 | + def get_partner(self): |
171 | + partner_id = self.registry('res.partner').search( |
172 | + self.cr, |
173 | + self.uid, |
174 | + [('customer', '=', True)], |
175 | + limit=1)[0] |
176 | + |
177 | + return self.registry('res.partner').browse( |
178 | + self.cr, self.uid, partner_id) |
179 | + |
180 | + def get_product(self): |
181 | + product_id = self.registry('product.product').search( |
182 | + self.cr, |
183 | + self.uid, |
184 | + [('uom_id', '!=', False)], |
185 | + limit=1)[0] |
186 | + return self.registry('product.product').browse( |
187 | + self.cr, self.uid, product_id) |
188 | + |
189 | + def test_random_product(self): |
190 | + line = self.get_line() |
191 | + product = self.get_product() |
192 | + partner = self.get_partner() |
193 | + |
194 | + values = line.product_id_change( |
195 | + product.id, None, |
196 | + partner_id=partner.id)['value'] |
197 | + |
198 | + self.assertEquals(values['price_unit'], product.list_price) |
199 | + self.assertEquals(values['uos_id'], product.uom_id.id) |
200 | + |
201 | + def test_with_pricelist(self): |
202 | + line = self.get_line() |
203 | + product = self.get_product() |
204 | + |
205 | + pricelist_id = self.registry('product.pricelist').create( |
206 | + self.cr, |
207 | + self.uid, |
208 | + { |
209 | + 'name': 'testpricelist', |
210 | + 'type': self.browse_ref('product.pricelist_type_sale').key, |
211 | + 'version_id': [ |
212 | + (0, 0, { |
213 | + 'name': 'testversion', |
214 | + 'items_id': [ |
215 | + (0, 0, { |
216 | + 'name': 'testitem', |
217 | + 'product_id': product.id, |
218 | + 'price_discount': .5, |
219 | + 'price_surcharge': 42, |
220 | + 'base': self.browse_ref( |
221 | + 'product.list_price').id, |
222 | + }), |
223 | + ], |
224 | + }), |
225 | + ], |
226 | + }) |
227 | + |
228 | + partner_id = self.registry('res.partner').create( |
229 | + self.cr, |
230 | + self.uid, |
231 | + { |
232 | + 'name': 'testcustomer', |
233 | + 'customer': True, |
234 | + 'property_product_pricelist': |
235 | + 'product.pricelist,%d' % pricelist_id, |
236 | + }) |
237 | + |
238 | + values = line.product_id_change( |
239 | + product.id, None, |
240 | + partner_id=partner_id)['value'] |
241 | + |
242 | + self.assertEquals(values['price_unit'], product.list_price * 1.5 + 42) |
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).