Merge lp:~therp-nl/openobject-addons/7.0-lp1080840-wrong_amounts_on_refund_vouchers into lp:openobject-addons

Proposed by Stefan Rijnhart (Opener)
Status: Needs review
Proposed branch: lp:~therp-nl/openobject-addons/7.0-lp1080840-wrong_amounts_on_refund_vouchers
Merge into: lp:openobject-addons
Diff against target: 243 lines (+60/-36)
2 files modified
account_voucher/account_voucher.py (+59/-36)
account_voucher/test/account_voucher.yml (+1/-0)
To merge this branch: bzr merge lp:~therp-nl/openobject-addons/7.0-lp1080840-wrong_amounts_on_refund_vouchers
Reviewer Review Type Date Requested Status
OpenERP Core Team Pending
Review via email: mp+135774@code.launchpad.net

Description of the change

Sorry for this stop-gap patch. It is the result of several weaknesses in the design of the voucher model:

- using the same functions for purchase/sale vouchers as for payment/receipt vouchers
- in doing so, using 'amount' (and in some cases its sign logic) as a given for payment/receipt vouchers and as a resulting sum in the case of purchase/sale vouchers
- allowing for both negative amounts on voucher lines as well as a debit/credit selection (without taking them into account consistently)
- insisting on showing the total voucher amount [1] with a positive sign and [2] before saving the voucher (i.e. using only on_change methods)

The voucher module is in dire need of a refactoring, in which API changes may be unavoidable. Hopefully that can be realized in 7.1.

This branch validates all the YAML tests included in the addon. I actually developed tests for this particular bug, but in order to run them I had to add a number of invisible fields to account_voucher.view_voucher_form because these are returned by the various on_change methods causing a YAML exception. I guess that shows how broken this addon actually is. You can find a version of this branch containing the tests here: https://code.launchpad.net/~therp-nl/openobject-addons/7.0-lp1080840-wrong_amounts_on_refund_vouchers_with_tests

To post a comment you must log in.

Unmerged revisions

8078. By Stefan Rijnhart (Opener)

[FIX] account_voucher: wrong amounts with refunds using debet/credit field

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'account_voucher/account_voucher.py'
2--- account_voucher/account_voucher.py 2012-11-15 12:38:51 +0000
3+++ account_voucher/account_voucher.py 2012-11-22 20:47:25 +0000
4@@ -299,6 +299,7 @@
5 \n* The \'Posted\' status is used when user create voucher,a voucher number is generated and voucher entries are created in account \
6 \n* The \'Cancelled\' status is used when user cancel voucher.'),
7 'amount': fields.float('Total', digits_compute=dp.get_precision('Account'), required=True, readonly=True, states={'draft':[('readonly',False)]}),
8+ 'amount_signed': fields.float('Total (signed)', digits_compute=dp.get_precision('Account'), readonly=True, states={'draft':[('readonly',False)]}),
9 'tax_amount':fields.float('Tax Amount', digits_compute=dp.get_precision('Account'), readonly=True, states={'draft':[('readonly',False)]}),
10 'reference': fields.char('Ref #', size=64, readonly=True, states={'draft':[('readonly',False)]}, help="Transaction reference number."),
11 'number': fields.char('Number', size=32, readonly=True,),
12@@ -365,13 +366,16 @@
13
14 for voucher in voucher_pool.browse(cr, uid, ids, context=context):
15 voucher_amount = 0.0
16+ if voucher.type in ('payment', 'receipt'):
17+ continue
18+
19 for line in voucher.line_ids:
20- voucher_amount += line.untax_amount or line.amount
21+ voucher_amount += (line.type == 'dr' and 1 or -1) * (line.untax_amount or line.amount)
22 line.amount = line.untax_amount or line.amount
23 voucher_line_pool.write(cr, uid, [line.id], {'amount':line.amount, 'untax_amount':line.untax_amount})
24
25 if not voucher.tax_id:
26- self.write(cr, uid, [voucher.id], {'amount':voucher_amount, 'tax_amount':0.0})
27+ self.write(cr, uid, [voucher.id], {'amount': abs(voucher_amount), 'amount_signed': voucher_amount, 'tax_amount':0.0})
28 continue
29
30 tax = [tax_pool.browse(cr, uid, voucher.tax_id.id, context=context)]
31@@ -385,7 +389,7 @@
32 if not tax[0].price_include:
33 for line in voucher.line_ids:
34 for tax_line in tax_pool.compute_all(cr, uid, tax, line.amount, 1).get('taxes', []):
35- total_tax += tax_line.get('amount', 0.0)
36+ total_tax += (line.type == 'dr' and 1 or -1) * tax_line.get('amount', 0.0)
37 total += total_tax
38 else:
39 for line in voucher.line_ids:
40@@ -393,13 +397,13 @@
41 line_tax = 0.0
42
43 for tax_line in tax_pool.compute_all(cr, uid, tax, line.untax_amount or line.amount, 1).get('taxes', []):
44- line_tax += tax_line.get('amount', 0.0)
45+ line_tax += (line.type == 'dr' and 1 or -1) * tax_line.get('amount', 0.0)
46 line_total += tax_line.get('price_unit')
47 total_tax += line_tax
48 untax_amount = line.untax_amount or line.amount
49 voucher_line_pool.write(cr, uid, [line.id], {'amount':line_total, 'untax_amount':untax_amount})
50
51- self.write(cr, uid, [voucher.id], {'amount':total, 'tax_amount':total_tax})
52+ self.write(cr, uid, [voucher.id], {'amount': abs(total), 'amount_signed': total, 'tax_amount':total_tax})
53 return True
54
55 def onchange_price(self, cr, uid, ids, line_ids, tax_id, partner_id=False, context=None):
56@@ -414,12 +418,12 @@
57 }
58 voucher_total = 0.0
59
60- line_ids = resolve_o2m_operations(cr, uid, line_pool, line_ids, ["amount"], context)
61+ line_ids = resolve_o2m_operations(cr, uid, line_pool, line_ids, ["amount", "type"], context)
62
63 total_tax = 0.0
64 for line in line_ids:
65 line_amount = 0.0
66- line_amount = line.get('amount',0.0)
67+ line_amount = (line['type'] == 'dr' and 1 or -1) * line.get('amount',0.0)
68
69 if tax_id:
70 tax = [tax_pool.browse(cr, uid, tax_id, context=context)]
71@@ -436,7 +440,7 @@
72 total = voucher_total + total_tax
73
74 res.update({
75- 'amount': total or voucher_total,
76+ 'amount': abs(total or voucher_total),
77 'tax_amount': total_tax
78 })
79 return {
80@@ -919,13 +923,20 @@
81 # ANSWER: We can have payment and receipt "In Advance".
82 # TODO: Make this logic available.
83 # -for sale, purchase we have but for the payment and receipt we do not have as based on the bank/cash journal we can not know its payment or receipt
84- if voucher_brw.type in ('purchase', 'payment'):
85+ if voucher_brw.type == 'payment':
86 credit = voucher_brw.paid_amount_in_company_currency
87- elif voucher_brw.type in ('sale', 'receipt'):
88+ amount_currency = -1 * voucher_brw.amount
89+ elif voucher_brw.type == 'receipt':
90 debit = voucher_brw.paid_amount_in_company_currency
91- if debit < 0: credit = -debit; debit = 0.0
92- if credit < 0: debit = -credit; credit = 0.0
93- sign = debit - credit < 0 and -1 or 1
94+ amount_currency = voucher_brw.amount
95+ else:
96+ amount_currency = -1 * voucher_brw.amount_signed
97+ if voucher_brw.amount_signed > 0:
98+ credit = voucher_brw.paid_amount_in_company_currency
99+ debit = 0.0
100+ else:
101+ debit = voucher_brw.paid_amount_in_company_currency
102+ credit = 0.0
103 #set the first line of the voucher
104 move_line = {
105 'name': voucher_brw.name or '/',
106@@ -937,7 +948,7 @@
107 'period_id': voucher_brw.period_id.id,
108 'partner_id': voucher_brw.partner_id.id,
109 'currency_id': company_currency <> current_currency and current_currency or False,
110- 'amount_currency': company_currency <> current_currency and sign * voucher_brw.amount or 0.0,
111+ 'amount_currency': company_currency <> current_currency and amount_currency or 0.0,
112 'date': voucher_brw.date,
113 'date_maturity': voucher_brw.date_due
114 }
115@@ -1073,6 +1084,7 @@
116 context = {}
117 move_line_obj = self.pool.get('account.move.line')
118 currency_obj = self.pool.get('res.currency')
119+ voucher_line_obj = self.pool.get('account.voucher.line')
120 tax_obj = self.pool.get('account.tax')
121 tot_line = line_total
122 rec_lst_ids = []
123@@ -1084,15 +1096,37 @@
124 #create one move line per voucher line where amount is not 0.0
125 if not line.amount:
126 continue
127+
128+ if line.amount < 0:
129+ line_type = line.type
130+ if line.type == 'dr':
131+ line_type = 'cr'
132+ else:
133+ line_type = 'dr'
134+ voucher_line_obj.write(
135+ cr, uid, line.id,
136+ {'type': line_type,
137+ 'amount': -1 * line.amount,
138+ 'untax_amount': line.untax_amount and -1 * line.untax_amount
139+ }, context=context)
140+ line.refresh()
141+
142+ if voucher_brw.type == 'payment':
143+ sign = 1
144+ elif voucher_brw.type == 'receipt':
145+ sign = -1
146+ else:
147+ sign = line.type == 'dr' and 1 or -1
148+
149 # convert the amount set on the voucher line into the currency of the voucher's company
150- amount = self._convert_amount(cr, uid, line.untax_amount or line.amount, voucher_brw.id, context=ctx)
151+ amount = sign * self._convert_amount(cr, uid, line.untax_amount or line.amount, voucher_brw.id, context=ctx)
152+
153 # if the amount encoded in voucher is equal to the amount unreconciled, we need to compute the
154 # currency rate difference
155 if line.amount == line.amount_unreconciled:
156 if not line.move_line_id.amount_residual:
157 raise osv.except_osv(_('Wrong bank statement line'),_("You have to delete the bank statement line which the payment was reconciled to manually. Please check the payment of the partner %s by the amount of %s.")%(line.voucher_id.partner_id.name, line.voucher_id.amount))
158- sign = voucher_brw.type in ('payment', 'purchase') and -1 or 1
159- currency_rate_difference = sign * (line.move_line_id.amount_residual - amount)
160+ currency_rate_difference = -1 * (sign * line.move_line_id.amount_residual - amount)
161 else:
162 currency_rate_difference = 0.0
163 move_line = {
164@@ -1109,19 +1143,12 @@
165 'debit': 0.0,
166 'date': voucher_brw.date
167 }
168- if amount < 0:
169- amount = -amount
170- if line.type == 'dr':
171- line.type = 'cr'
172- else:
173- line.type = 'dr'
174
175- if (line.type=='dr'):
176- tot_line += amount
177+ if amount > 0:
178 move_line['debit'] = amount
179 else:
180- tot_line -= amount
181- move_line['credit'] = amount
182+ move_line['credit'] = -1 * amount
183+ tot_line += amount
184
185 if voucher_brw.tax_id and voucher_brw.type in ('sale', 'purchase'):
186 move_line.update({
187@@ -1143,7 +1170,6 @@
188 # we compute the amount in that foreign currency.
189 if line.move_line_id.currency_id.id == current_currency:
190 # if the voucher and the voucher line share the same currency, there is no computation to do
191- sign = (move_line['debit'] - move_line['credit']) < 0 and -1 or 1
192 amount_currency = sign * (line.amount)
193 elif line.move_line_id.currency_id.id == voucher_brw.payment_rate_currency_id.id:
194 # if the rate is specified on the voucher, we must use it
195@@ -1152,9 +1178,8 @@
196 else:
197 # otherwise we use the rates of the system (giving the voucher date in the context)
198 amount_currency = currency_obj.compute(cr, uid, company_currency, line.move_line_id.currency_id.id, move_line['debit']-move_line['credit'], context=ctx)
199- if line.amount == line.amount_unreconciled and line.move_line_id.currency_id.id == voucher_currency:
200- sign = voucher_brw.type in ('payment', 'purchase') and -1 or 1
201- foreign_currency_diff = sign * line.move_line_id.amount_residual_currency + amount_currency
202+ if amount == line.amount_unreconciled and line.move_line_id.currency_id.id == voucher_currency:
203+ foreign_currency_diff = line.move_line_id.amount_residual_currency + amount_currency
204
205 move_line['amount_currency'] = amount_currency
206 voucher_line = move_line_obj.create(cr, uid, move_line)
207@@ -1266,6 +1291,7 @@
208 context = {}
209 move_pool = self.pool.get('account.move')
210 move_line_pool = self.pool.get('account.move.line')
211+ self.compute_tax(cr, uid, ids, context=context)
212 for voucher in self.browse(cr, uid, ids, context=context):
213 if voucher.move_id:
214 continue
215@@ -1285,14 +1311,11 @@
216 move_line_brw = move_line_pool.browse(cr, uid, move_line_id, context=context)
217 line_total = move_line_brw.debit - move_line_brw.credit
218 rec_list_ids = []
219- if voucher.type == 'sale':
220- line_total = line_total - self._convert_amount(cr, uid, voucher.tax_amount, voucher.id, context=ctx)
221- elif voucher.type == 'purchase':
222- line_total = line_total + self._convert_amount(cr, uid, voucher.tax_amount, voucher.id, context=ctx)
223+ line_total = line_total - self._convert_amount(cr, uid, voucher.tax_amount, voucher.id, context=ctx)
224 # Create one move line per voucher line where amount is not 0.0
225 line_total, rec_list_ids = self.voucher_move_line_create(cr, uid, voucher.id, line_total, move_id, company_currency, current_currency, context)
226
227- # Create the writeoff line if needed
228+ # Create the writeoff line if needed
229 ml_writeoff = self.writeoff_move_line_get(cr, uid, voucher.id, line_total, move_id, name, company_currency, current_currency, context)
230 if ml_writeoff:
231 move_line_pool.create(cr, uid, ml_writeoff, context)
232
233=== modified file 'account_voucher/test/account_voucher.yml'
234--- account_voucher/test/account_voucher.yml 2012-10-19 11:07:20 +0000
235+++ account_voucher/test/account_voucher.yml 2012-11-22 20:47:25 +0000
236@@ -13,6 +13,7 @@
237 - account_id: account.a_recv
238 amount: 1000.0
239 name: Voucher for Axelor
240+ type: cr
241 partner_id: base.res_partner_12
242 period_id: account.period_6
243 reference: none

Subscribers

People subscribed via source and target branches

to all changes: