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

[Stefan Rijnhart (Opener)]

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

Unmerged revisions

8078. By Stefan Rijnhart (Opener) on 2012-11-22

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

=== modified file 'account_voucher/account_voucher.py'
--- account_voucher/account_voucher.py	2012-11-15 12:38:51 +0000
+++ account_voucher/account_voucher.py	2012-11-22 20:47:25 +0000
@@ -299,6 +299,7 @@
                         \n* The \'Posted\' status is used when user create voucher,a voucher number is generated and voucher entries are created in account \
                         \n* The \'Cancelled\' status is used when user cancel voucher.'),
         'amount': fields.float('Total', digits_compute=dp.get_precision('Account'), required=True, readonly=True, states={'draft':[('readonly',False)]}),
+        'amount_signed': fields.float('Total (signed)', digits_compute=dp.get_precision('Account'), readonly=True, states={'draft':[('readonly',False)]}),
         'tax_amount':fields.float('Tax Amount', digits_compute=dp.get_precision('Account'), readonly=True, states={'draft':[('readonly',False)]}),
         'reference': fields.char('Ref #', size=64, readonly=True, states={'draft':[('readonly',False)]}, help="Transaction reference number."),
         'number': fields.char('Number', size=32, readonly=True,),
@@ -365,13 +366,16 @@
 
         for voucher in voucher_pool.browse(cr, uid, ids, context=context):
             voucher_amount = 0.0
+            if voucher.type in ('payment', 'receipt'):
+                continue
+
             for line in voucher.line_ids:
-                voucher_amount += line.untax_amount or line.amount
+                voucher_amount += (line.type == 'dr' and 1 or -1) * (line.untax_amount or line.amount)
                 line.amount = line.untax_amount or line.amount
                 voucher_line_pool.write(cr, uid, [line.id], {'amount':line.amount, 'untax_amount':line.untax_amount})
 
             if not voucher.tax_id:
-                self.write(cr, uid, [voucher.id], {'amount':voucher_amount, 'tax_amount':0.0})
+                self.write(cr, uid, [voucher.id], {'amount': abs(voucher_amount), 'amount_signed': voucher_amount, 'tax_amount':0.0})
                 continue
 
             tax = [tax_pool.browse(cr, uid, voucher.tax_id.id, context=context)]
@@ -385,7 +389,7 @@
             if not tax[0].price_include:
                 for line in voucher.line_ids:
                     for tax_line in tax_pool.compute_all(cr, uid, tax, line.amount, 1).get('taxes', []):
-                        total_tax += tax_line.get('amount', 0.0)
+                        total_tax += (line.type == 'dr' and 1 or -1) * tax_line.get('amount', 0.0)
                 total += total_tax
             else:
                 for line in voucher.line_ids:
@@ -393,13 +397,13 @@
                     line_tax = 0.0
 
                     for tax_line in tax_pool.compute_all(cr, uid, tax, line.untax_amount or line.amount, 1).get('taxes', []):
-                        line_tax += tax_line.get('amount', 0.0)
+                        line_tax += (line.type == 'dr' and 1 or -1) * tax_line.get('amount', 0.0)
                         line_total += tax_line.get('price_unit')
                     total_tax += line_tax
                     untax_amount = line.untax_amount or line.amount
                     voucher_line_pool.write(cr, uid, [line.id], {'amount':line_total, 'untax_amount':untax_amount})
 
-            self.write(cr, uid, [voucher.id], {'amount':total, 'tax_amount':total_tax})
+            self.write(cr, uid, [voucher.id], {'amount': abs(total), 'amount_signed': total, 'tax_amount':total_tax})
         return True
 
     def onchange_price(self, cr, uid, ids, line_ids, tax_id, partner_id=False, context=None):
@@ -414,12 +418,12 @@
         }
         voucher_total = 0.0
 
-        line_ids = resolve_o2m_operations(cr, uid, line_pool, line_ids, ["amount"], context)
+        line_ids = resolve_o2m_operations(cr, uid, line_pool, line_ids, ["amount", "type"], context)
 
         total_tax = 0.0
         for line in line_ids:
             line_amount = 0.0
-            line_amount = line.get('amount',0.0)
+            line_amount =  (line['type'] == 'dr' and 1 or -1) * line.get('amount',0.0)
 
             if tax_id:
                 tax = [tax_pool.browse(cr, uid, tax_id, context=context)]
@@ -436,7 +440,7 @@
         total = voucher_total + total_tax
 
         res.update({
-            'amount': total or voucher_total,
+            'amount': abs(total or voucher_total),
             'tax_amount': total_tax
         })
         return {
@@ -919,13 +923,20 @@
         # ANSWER: We can have payment and receipt "In Advance".
         # TODO: Make this logic available.
         # -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
-        if voucher_brw.type in ('purchase', 'payment'):
+        if voucher_brw.type == 'payment':
             credit = voucher_brw.paid_amount_in_company_currency
-        elif voucher_brw.type in ('sale', 'receipt'):
+            amount_currency = -1 * voucher_brw.amount
+        elif voucher_brw.type == 'receipt':
             debit = voucher_brw.paid_amount_in_company_currency
-        if debit < 0: credit = -debit; debit = 0.0
-        if credit < 0: debit = -credit; credit = 0.0
-        sign = debit - credit < 0 and -1 or 1
+            amount_currency = voucher_brw.amount
+        else:
+            amount_currency = -1 * voucher_brw.amount_signed
+            if voucher_brw.amount_signed > 0:
+                credit = voucher_brw.paid_amount_in_company_currency
+                debit = 0.0
+            else:
+                debit = voucher_brw.paid_amount_in_company_currency
+                credit = 0.0
         #set the first line of the voucher
         move_line = {
                 'name': voucher_brw.name or '/',
@@ -937,7 +948,7 @@
                 'period_id': voucher_brw.period_id.id,
                 'partner_id': voucher_brw.partner_id.id,
                 'currency_id': company_currency <> current_currency and  current_currency or False,
-                'amount_currency': company_currency <> current_currency and sign * voucher_brw.amount or 0.0,
+                'amount_currency': company_currency <> current_currency and amount_currency or 0.0,
                 'date': voucher_brw.date,
                 'date_maturity': voucher_brw.date_due
             }
@@ -1073,6 +1084,7 @@
             context = {}
         move_line_obj = self.pool.get('account.move.line')
         currency_obj = self.pool.get('res.currency')
+        voucher_line_obj = self.pool.get('account.voucher.line')
         tax_obj = self.pool.get('account.tax')
         tot_line = line_total
         rec_lst_ids = []
@@ -1084,15 +1096,37 @@
             #create one move line per voucher line where amount is not 0.0
             if not line.amount:
                 continue
+
+            if line.amount < 0:
+                line_type = line.type
+                if line.type == 'dr':
+                    line_type = 'cr'
+                else:
+                    line_type = 'dr'
+                voucher_line_obj.write(
+                    cr, uid, line.id, 
+                    {'type': line_type,
+                     'amount': -1 * line.amount,
+                     'untax_amount': line.untax_amount and -1 * line.untax_amount
+                     }, context=context)
+                line.refresh()
+
+            if voucher_brw.type == 'payment':
+                sign = 1
+            elif voucher_brw.type == 'receipt':
+                sign = -1
+            else:
+                sign = line.type == 'dr' and 1 or -1
+
             # convert the amount set on the voucher line into the currency of the voucher's company
-            amount = self._convert_amount(cr, uid, line.untax_amount or line.amount, voucher_brw.id, context=ctx)
+            amount = sign * self._convert_amount(cr, uid, line.untax_amount or line.amount, voucher_brw.id, context=ctx)
+
             # if the amount encoded in voucher is equal to the amount unreconciled, we need to compute the
             # currency rate difference
             if line.amount == line.amount_unreconciled:
                 if not line.move_line_id.amount_residual:
                     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))
-                sign = voucher_brw.type in ('payment', 'purchase') and -1 or 1
-                currency_rate_difference = sign * (line.move_line_id.amount_residual - amount)
+                currency_rate_difference = -1 * (sign * line.move_line_id.amount_residual - amount)
             else:
                 currency_rate_difference = 0.0
             move_line = {
@@ -1109,19 +1143,12 @@
                 'debit': 0.0,
                 'date': voucher_brw.date
             }
-            if amount < 0:
-                amount = -amount
-                if line.type == 'dr':
-                    line.type = 'cr'
-                else:
-                    line.type = 'dr'
 
-            if (line.type=='dr'):
-                tot_line += amount
+            if amount > 0:
                 move_line['debit'] = amount
             else:
-                tot_line -= amount
-                move_line['credit'] = amount
+                move_line['credit'] = -1 * amount
+            tot_line += amount
 
             if voucher_brw.tax_id and voucher_brw.type in ('sale', 'purchase'):
                 move_line.update({
@@ -1143,7 +1170,6 @@
                     # we compute the amount in that foreign currency.
                     if line.move_line_id.currency_id.id == current_currency:
                         # if the voucher and the voucher line share the same currency, there is no computation to do
-                        sign = (move_line['debit'] - move_line['credit']) < 0 and -1 or 1
                         amount_currency = sign * (line.amount)
                     elif line.move_line_id.currency_id.id == voucher_brw.payment_rate_currency_id.id:
                         # if the rate is specified on the voucher, we must use it
@@ -1152,9 +1178,8 @@
                     else:
                         # otherwise we use the rates of the system (giving the voucher date in the context)
                         amount_currency = currency_obj.compute(cr, uid, company_currency, line.move_line_id.currency_id.id, move_line['debit']-move_line['credit'], context=ctx)
-                if line.amount == line.amount_unreconciled and line.move_line_id.currency_id.id == voucher_currency:
-                    sign = voucher_brw.type in ('payment', 'purchase') and -1 or 1
-                    foreign_currency_diff = sign * line.move_line_id.amount_residual_currency + amount_currency
+                if amount == line.amount_unreconciled and line.move_line_id.currency_id.id == voucher_currency:
+                    foreign_currency_diff = line.move_line_id.amount_residual_currency + amount_currency
 
             move_line['amount_currency'] = amount_currency
             voucher_line = move_line_obj.create(cr, uid, move_line)
@@ -1266,6 +1291,7 @@
             context = {}
         move_pool = self.pool.get('account.move')
         move_line_pool = self.pool.get('account.move.line')
+        self.compute_tax(cr, uid, ids, context=context)
         for voucher in self.browse(cr, uid, ids, context=context):
             if voucher.move_id:
                 continue
@@ -1285,14 +1311,11 @@
             move_line_brw = move_line_pool.browse(cr, uid, move_line_id, context=context)
             line_total = move_line_brw.debit - move_line_brw.credit
             rec_list_ids = []
-            if voucher.type == 'sale':
-                line_total = line_total - self._convert_amount(cr, uid, voucher.tax_amount, voucher.id, context=ctx)
-            elif voucher.type == 'purchase':
-                line_total = line_total + self._convert_amount(cr, uid, voucher.tax_amount, voucher.id, context=ctx)
+            line_total = line_total - self._convert_amount(cr, uid, voucher.tax_amount, voucher.id, context=ctx)
             # Create one move line per voucher line where amount is not 0.0
             line_total, rec_list_ids = self.voucher_move_line_create(cr, uid, voucher.id, line_total, move_id, company_currency, current_currency, context)
 
-            # Create the writeoff line if needed
+            # Create the writeoff line if needed            
             ml_writeoff = self.writeoff_move_line_get(cr, uid, voucher.id, line_total, move_id, name, company_currency, current_currency, context)
             if ml_writeoff:
                 move_line_pool.create(cr, uid, ml_writeoff, context)
 
=== modified file 'account_voucher/test/account_voucher.yml'
--- account_voucher/test/account_voucher.yml	2012-10-19 11:07:20 +0000
+++ account_voucher/test/account_voucher.yml	2012-11-22 20:47:25 +0000
@@ -13,6 +13,7 @@
       - account_id: account.a_recv
         amount: 1000.0
         name: Voucher for Axelor
+        type: cr
     partner_id: base.res_partner_12
     period_id: account.period_6
     reference: none