Merge lp:~therp-nl/additional-discount/fix_lp1102075_move_lines into lp:additional-discount

[Ronald Portier (Therp)]

The original merge proposal to generate additional move lines for the discount has been extensively reworked:

- Adjust the original move lines instead of creating additional ones. In this way the line that should be paid will have an amount equal to an incoming or outgoing payment. This makes for easier reconciliation.

- Rounding had been made more consistent all over

- Nevertheless an catch all test has been added to prevent the additional discount from ever creating an unbalanced move

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

It is probably a good idea to have a look at rounding issues now. They can get ugly really easily. At least make sure that the amount to pay is the same as on the invoice that goes out to the customer.

Another thing to consider: if the move line's account is set to reconcile, would it be a good idea to do a partial reconciliation with its discount line at this point? This would require some work in the method cancelling the invoice because you would have to undo any move internal reconciliation before the move line object protests against unlinking reconciled items.

review: Needs Fixing

[Christophe Combelles (ccomb)]

Hi, thanks for the work on this package,
I currently have no time for reviewing, I've changed the project settings to allow you (or someone else) to merge or push another branch in ~additional-discount (actually I hope I succeeded in doing so)

Unmerged revisions

31. By Ronald Portier (Therp) on 2013-05-01

[FIX] Previous revision moved view xml, but forgot to adapt __openerp__.py

30. By Ronald Portier (Therp) on 2013-05-01

[RFR] Added standard organization to module
[FIX] Rounding issues
[FIX] Prevent additional discount from creating unbalanced moves

29. By Ronald Portier (Therp) on 2013-01-20

[IMP] Generate move-lines for discount.

=== modified file 'additional_discount/__init__.py'
--- additional_discount/__init__.py	2011-05-04 10:37:12 +0000
+++ additional_discount/__init__.py	2013-05-01 21:40:37 +0000
@@ -1,3 +1,2 @@
-import sale
-import purchase
-import invoice
\ No newline at end of file
+import model
+

=== modified file 'additional_discount/__openerp__.py'
--- additional_discount/__openerp__.py	2012-09-04 07:09:37 +0000
+++ additional_discount/__openerp__.py	2013-05-01 21:40:37 +0000
@@ -10,7 +10,9 @@
     Additional discount is fully integrated between sales, purchase and invoices.
     """,
     "init_xml": [],
-    'update_xml': ['additional_discount_view.xml'],
+    'update_xml': [
+        'view/additional_discount_view.xml'
+    ],
     'demo_xml': [],
     'test': [
         'test/scenario1.yml',

=== added directory 'additional_discount/i18n'
=== added directory 'additional_discount/model'
=== added file 'additional_discount/model/__init__.py'
--- additional_discount/model/__init__.py	1970-01-01 00:00:00 +0000
+++ additional_discount/model/__init__.py	2013-05-01 21:40:37 +0000
@@ -0,0 +1,13 @@
+# -*- coding: UTF-8 -*-
+'''
+Created on 16 mrt. 2013
+
+@author: Ronald Portier, Therp
+
[e-mail]
+http://www.therp.nl
+
+'''
+import sale
+import purchase
+import invoice

=== renamed file 'additional_discount/common.py' => 'additional_discount/model/common.py'
--- additional_discount/common.py	2012-09-10 16:22:19 +0000
+++ additional_discount/model/common.py	2013-05-01 21:40:37 +0000
@@ -47,14 +47,15 @@
             amount_untaxed = sum(line.price_subtotal
                                  for line in getattr(record, self._line_column))
             add_disc = record.add_disc
-            add_disc_amt = cur_round(amount_untaxed * add_disc / 100)
-            o_res['add_disc_amt'] = add_disc_amt
-            o_res['amount_net'] = o_res['amount_untaxed'] - add_disc_amt
+            discount_factor = 1 - (add_disc / 100)
+            o_res['amount_net'] = cur_round(
+                o_res['amount_untaxed'] * discount_factor)
+            o_res['add_disc_amt'] = (
+                 o_res['amount_untaxed'] - o_res['amount_net'])
 
             if self._amount_all_discount_taxes:
-                # XXX We might have rounding issue
                 o_res['amount_tax'] = cur_round(
-                    o_res['amount_tax'] * (100.0 - (add_disc or 0.0))/100.0)
+                    o_res['amount_tax'] * discount_factor)
             o_res['amount_total'] = o_res['amount_net'] + o_res['amount_tax']
 
         return res

=== renamed file 'additional_discount/invoice.py' => 'additional_discount/model/invoice.py'
--- additional_discount/invoice.py	2012-09-10 16:22:19 +0000
+++ additional_discount/model/invoice.py	2013-05-01 21:40:37 +0000
@@ -1,17 +1,26 @@
+# -*- coding: utf-8 -*- 
+import logging
+
 import decimal_precision as dp
-from osv import fields, osv
+
+from openerp.osv import fields
+from openerp.osv import orm
+from openerp.tools.translate import _
+from openerp.tools.float_utils import float_round
+
 from common import AdditionalDiscountable
-from tools.translate import _
-
-class account_invoice(AdditionalDiscountable, osv.Model):
-
-    _inherit = "account.invoice"
+
+class account_invoice(AdditionalDiscountable, orm.Model):
+
+    _inherit = 'account.invoice'
     _description = 'Invoice'
 
     _line_column = 'invoice_line'
     _tax_column = 'invoice_line_tax_id'
     _amount_all_discount_taxes = False # done and written on tax lines
 
+    __logger = logging.getLogger(_inherit)
+
     def record_currency(self, record):
         """Return currency browse record from an invoice record (override)."""
         return record.currency_id
@@ -32,6 +41,118 @@
         for tax in self.pool.get('account.invoice.tax').browse(cr, uid, ids, context=context):
             result[tax.invoice_id.id] = True
         return result.keys()
+    
+
+    def finalize_invoice_move_lines(self, cr, uid, invoice_browse, move_lines):
+        """finalize_invoice_move_lines(cr, uid, invoice, move_lines) -> move_lines
+        Hook method to be overridden in additional modules to verify and possibly alter the
+        move lines to be created by an invoice, for special cases.
+        :param invoice_browse: browsable record of the invoice that is generating the move lines
+        :param move_lines: list of dictionaries with the account.move.lines (as for create())
+        :return: the (possibly updated) final move_lines to create for this invoice
+         
+        Actually move_lines is a list op tuples with each tuple having three
+        elements. The third element is the value dictionary used to create one
+        line. So move_lines will look like this:
+        [(0, 0, {'debit': ......}), (0, 0, {<values for second line>}), ..]
+
+        Taxes are a special problem, because when this function is called,
+        the tax move lines have the discount already applied. There is no
+        easy way to recogize these lines, therefore we retrieve all tax
+        account_id's used and exclude them from being discounted (again).
+        """
+        discount = invoice_browse.add_disc
+        if  not discount:
+            return move_lines
+        # Only if there is a additional discount will we loop
+        # over all lines to adjust the amounts
+        # We assume a line will nly contain either credit, or debit,
+        # never both.
+        precision_model = self.pool.get('decimal.precision') 
+        precision = precision_model.precision_get(cr, uid, 'Account')
+        discount_factor = 1.0 - (discount / 100.0)
+        balance_credit = True
+        total_credit = 0.0
+        total_debit = 0.0
+        # Get taxt account id's
+        tax_account_ids = []
+        ait_model = self.pool.get('account.invoice.tax')
+        ait_lines = ait_model.move_line_get(cr, uid, invoice_browse.id)
+        for ait_line in ait_lines:
+            tax_account_ids.append(ait_line['account_id'])
+        for move_line in move_lines:
+            # Check format
+            assert len(move_line) > 2, (
+                _('Invalid move line %s') % str(move_line))
+            vals = move_line[2]
+            # Do not change tax lines (but include them in totals):
+            if  vals['account_id'] in tax_account_ids:
+                if  vals['debit']:
+                    total_debit += vals['debit']
+                if  vals['credit']:
+                    total_credit += vals['credit']
+                continue
+            # Handle debtor/creditor
+            # We don't want to recompute to make sure open amount will
+            # be exactly the same as on invoice.
+            if  invoice_browse.account_id.id == vals['account_id']:
+                if  vals['debit']:
+                    assert not vals['credit'], (
+                        _('Can not have credit and debit in same move line'))
+                    vals['debit'] = invoice_browse.amount_total
+                    total_debit += vals['debit']
+                else:
+                    assert not vals['debit'], (
+                        _('Can not have debit and credit in same move line'))
+                    vals['credit'] = invoice_browse.amount_total
+                    total_credit += vals['credit']
+                    balance_credit = False
+            else:
+                if  vals['credit']:
+                    vals['credit'] = float_round(
+                        vals['credit'] * discount_factor, precision)
+                    total_credit += vals['credit']
+                if  vals['debit']:
+                    vals['debit'] = float_round(
+                        vals['debit'] * discount_factor, precision)
+                    total_debit += vals['debit']
+            if  vals['amount_currency']:
+                vals['amount_currency'] = float_round(
+                    vals['amount_currency'] * discount_factor, precision)
+            if  vals['tax_amount']:
+                vals['tax_amount'] = float_round(
+                    vals['tax_amount'] * discount_factor, precision)                        
+        # Check balance
+        difference = total_debit - total_credit
+        if  abs(difference) > 10 ** -4:
+            # Find largest credit or debit amount and adjust for rounding:
+            largest_vals = None
+            largest_amount = 0.0
+            for move_line in move_lines:
+                vals = move_line[2]
+                if  balance_credit:
+                    if  vals['credit'] and vals['credit'] > largest_amount:
+                        largest_vals = vals
+                else:
+                    if  vals['debit'] and vals['debit'] > largest_amount:
+                        largest_vals = vals
+            assert largest_vals, _('No largest debit or credit found')
+            if  balance_credit:
+                largest_vals['credit'] = (
+                    largest_vals['credit'] + difference)
+                if  largest_vals['tax_amount']:
+                    largest_vals['tax_amount'] = (
+                            largest_vals['tax_amount'] + difference)
+            else:
+                largest_vals['debit'] = (
+                    largest_vals['debit'] - difference)
+                if  largest_vals['tax_amount']:
+                    largest_vals['tax_amount'] = (
+                            largest_vals['tax_amount'] - difference)
+            self.__logger.info(
+                _('Modified move line %s to prevent unbalanced move with'
+                  ' difference %d') % (str(largest_vals), difference))
+        return move_lines    
 
     _columns={
             'add_disc':fields.float('Additional Discount(%)',digits=(4,2),readonly=True, states={'draft':[('readonly',False)]}),
@@ -68,12 +189,15 @@
                }
 
 
-class account_invoice_tax(osv.Model):
+class account_invoice_tax(orm.Model):
 
     _inherit = 'account.invoice.tax'
 
     def compute(self, cr, uid, invoice_id, context=None):
-        tax_grouped = super(account_invoice_tax, self).compute(cr, uid, invoice_id, context)
+        precision_model = self.pool.get('decimal.precision') 
+        precision = precision_model.precision_get(cr, uid, 'Account')
+        tax_grouped = super(
+            account_invoice_tax, self).compute(cr, uid, invoice_id, context)
         tax_pool = self.pool.get('account.tax')
         cur_pool = self.pool.get('res.currency')
         tax_ids = set([key[0] for key in tax_grouped])
@@ -82,13 +206,15 @@
             raise osv.except_osv(_('Discount error'),
                  _('Unable (for now) to compute a global '
                 'discount with non percent-type taxes'))
-        invoice = self.pool.get('account.invoice').browse(cr, uid, invoice_id, context=context)
+        invoice = self.pool.get('account.invoice').browse(
+            cr, uid, invoice_id, context=context)
         add_disc = invoice.add_disc
-        cur = invoice.currency_id
+        discount_factor = 1.0 - (add_disc / 100.0)
         for line in tax_grouped:
-            for key in ('tax_amount', 'base_amount', 'amount', 'base'): #FIXME?
+            for key in ('tax_amount', 'base_amount', 'amount', 'base'):
                 val = tax_grouped[line][key]
-                tax_grouped[line][key] = cur_pool.round(cr, uid, cur, val * (1.0 - add_disc / 100.0))
-        print tax_grouped
+                tax_grouped[line][key] = float_round(
+                    val * discount_factor, precision)
+        # print tax_grouped
         return tax_grouped
 

=== renamed file 'additional_discount/purchase.py' => 'additional_discount/model/purchase.py'
=== renamed file 'additional_discount/sale.py' => 'additional_discount/model/sale.py'
=== added directory 'additional_discount/view'
=== renamed file 'additional_discount/additional_discount_view.xml' => 'additional_discount/view/additional_discount_view.xml'