From 5f9ca1b89becb6867d0755cf7cb3d7f9c533e7af Mon Sep 17 00:00:00 2001 From: qdp-odoo Date: Fri, 27 May 2016 16:45:19 +0200 Subject: [PATCH] [FIX] account: manual reconciliation through the wizard on journal items were giving weird results. OPW 678153. Symptoms: running a full reconciliation manually with that wizard was resulting in not fully reconciled aml altough the total in company currency and in secondary currency were perfectly matched. Full use case described in added test test_manual_reconcile_wizard_opw678153. Problems: - The system was processing by pair of aml and creating exchange rate entry/full reconciliation as soon as possible whereas in that case it needs to be postponed at the end of the manual reconciliation otherwise some entries would be excluded from processing. - Another problem was that because of this batch processing, we ended with unconsistent amount_residual_currency values for some entries that were fully reconciled in company currency but not in foreign currency (also negative amount_residual_currency for a debit line which should never happens)... To solve that, we now process the manual full reconciliation in two steps: only in company currency first, then in secondary currency if needed. A small adaptation in the payment widget of invoices was needed for that too. --- addons/account/models/account_invoice.py | 4 +- addons/account/models/account_move.py | 58 ++++++++++++++++--- addons/account/tests/test_reconciliation.py | 58 +++++++++++++++++++ addons/account/wizard/account_reconcile.py | 35 ++++++++++- .../models/account_full_reconcile.py | 31 ++++++++++ 5 files changed, 175 insertions(+), 11 deletions(-) diff --git a/addons/account/models/account_invoice.py b/addons/account/models/account_invoice.py index 8eb560a614a..4f068187452 100644 --- a/addons/account/models/account_invoice.py +++ b/addons/account/models/account_invoice.py @@ -151,11 +151,13 @@ class AccountInvoice(models.Model): if self.type in ('out_invoice', 'in_refund'): amount = sum([p.amount for p in payment.matched_debit_ids if p.debit_move_id in self.move_id.line_ids]) amount_currency = sum([p.amount_currency for p in payment.matched_debit_ids if p.debit_move_id in self.move_id.line_ids]) + payment_currency_id = all([p.currency_id == payment.matched_debit_ids[0].currency_id for p in payment.matched_debit_ids]) and payment.matched_debit_ids[0].currency_id or False elif self.type in ('in_invoice', 'out_refund'): amount = sum([p.amount for p in payment.matched_credit_ids if p.credit_move_id in self.move_id.line_ids]) amount_currency = sum([p.amount_currency for p in payment.matched_credit_ids if p.credit_move_id in self.move_id.line_ids]) + payment_currency_id = all([p.currency_id == payment.matched_credit_ids[0].currency_id for p in payment.matched_credit_ids]) and payment.matched_credit_ids[0].currency_id or False # get the payment value in invoice currency - if payment.currency_id and payment.currency_id == self.currency_id: + if payment_currency_id and payment_currency_id == self.currency_id: amount_to_show = amount_currency else: amount_to_show = payment.company_id.currency_id.with_context(date=payment.date).compute(amount, self.currency_id) diff --git a/addons/account/models/account_move.py b/addons/account/models/account_move.py index 285e8e6e843..f14fdea87ac 100644 --- a/addons/account/models/account_move.py +++ b/addons/account/models/account_move.py @@ -775,6 +775,10 @@ class AccountMoveLine(models.Model): if self[0].currency_id and all([x.currency_id == self[0].currency_id for x in self]): #all the lines have the same currency, so we consider the amount_residual_currency field field = 'amount_residual_currency' + if self._context.get('skip_full_reconcile_check') == 'amount_currency_excluded': + field = 'amount_residual' + elif self._context.get('skip_full_reconcile_check') == 'amount_currency_only': + field = 'amount_residual_currency' #Reconcile the pair together amount_reconcile = min(sm_debit_move[field], -sm_credit_move[field]) #Remove from recordset the one(s) that will be totally reconciled @@ -792,6 +796,12 @@ class AccountMoveLine(models.Model): amount_reconcile = min(sm_debit_move.amount_residual, -sm_credit_move.amount_residual) + if self._context.get('skip_full_reconcile_check') == 'amount_currency_excluded': + amount_reconcile_currency = 0.0 + currency = self._context.get('manual_full_reconcile_currency') + elif self._context.get('skip_full_reconcile_check') == 'amount_currency_only': + currency = self._context.get('manual_full_reconcile_currency') + self.env['account.partial.reconcile'].create({ 'debit_move_id': sm_debit_move.id, 'credit_move_id': sm_credit_move.id, @@ -840,6 +850,8 @@ class AccountMoveLine(models.Model): writeoff_to_reconcile = remaining_moves._create_writeoff(writeoff_vals) #add writeoff line to reconcile algo and finish the reconciliation remaining_moves = (remaining_moves + writeoff_to_reconcile).auto_reconcile_lines() + return writeoff_to_reconcile + return True def _create_writeoff(self, vals): """ Create a writeoff move for the account.move.lines in self. If debit/credit is not specified in vals, @@ -894,6 +906,31 @@ class AccountMoveLine(models.Model): # Return the writeoff move.line which is to be reconciled return writeoff_move.line_ids.filtered(lambda r: r.account_id == self[0].account_id) + @api.model + def compute_full_after_batch_reconcile(self): + """ After running the manual reconciliation wizard and making full reconciliation, we need to run this method to create + potentially an exchange rate entry that will balance the remaining amount_residual_currency (possibly several aml). + + This ensure that all aml in the full reconciliation are reconciled (amount_residual = amount_residual_currency = 0). + """ + total_amount_currency = 0 + currency = False + aml_to_balance_currency = self.env['account.move.line'] + maxdate = None + for aml in self: + if aml.amount_residual_currency: + aml_to_balance_currency |= aml + maxdate = max(aml.date, maxdate) + if not currency and aml.currency_id: + currency = aml.currency_id + if aml.currency_id and aml.currency_id == currency: + total_amount_currency += aml.amount_currency + if currency and aml_to_balance_currency: + aml = aml_to_balance_currency + #eventually create journal entries to book the difference due to foreign currency's exchange rate that fluctuates + partial_rec = aml.credit and aml.matched_debit_ids[0] or aml.matched_credit_ids[0] + aml_id, partial_rec_id = partial_rec.create_exchange_rate_entry(aml, 0.0, total_amount_currency, currency, maxdate) + @api.multi def remove_move_reconcile(self): """ Undo a reconciliation """ @@ -1224,7 +1261,7 @@ class AccountPartialReconcile(models.Model): help='Utility field to express amount currency') company_id = fields.Many2one('res.company', related='debit_move_id.company_id', store=True, string='Currency') - def create_exchange_rate_entry(self, aml, amount_diff, diff_in_currency, currency, move_date): + def create_exchange_rate_entry(self, aml_to_fix, amount_diff, diff_in_currency, currency, move_date): """ Automatically create a journal entry to book the exchange rate difference. That new journal entry is made in the company `currency_exchange_journal_id` and one of its journal items is matched with the other lines to balance the full reconciliation. @@ -1266,19 +1303,24 @@ class AccountPartialReconcile(models.Model): 'amount_currency': diff_in_currency, 'partner_id': rec.debit_move_id.partner_id.id, }) - partial_rec = rec.env['account.partial.reconcile'].create({ - 'debit_move_id': aml.credit and line_to_reconcile.id or aml.id, - 'credit_move_id': aml.debit and line_to_reconcile.id or aml.id, - 'amount': abs(amount_diff), - 'amount_currency': abs(diff_in_currency), - 'currency_id': currency.id, - }) + for aml in aml_to_fix: + partial_rec = rec.env['account.partial.reconcile'].create({ + 'debit_move_id': aml.credit and line_to_reconcile.id or aml.id, + 'credit_move_id': aml.debit and line_to_reconcile.id or aml.id, + 'amount': abs(aml.amount_residual), + 'amount_currency': abs(aml.amount_residual_currency), + 'currency_id': currency.id, + }) move.post() return line_to_reconcile.id, partial_rec.id @api.model def create(self, vals): res = super(AccountPartialReconcile, self).create(vals) + if self._context.get('skip_full_reconcile_check'): + #when running the manual reconciliation wizard, don't check the partials separately for full + #reconciliation or exchange rate because it is handled manually after the whole processing + return res #check if the reconcilation is full #first, gather all journal items involved in the reconciliation just created partial_rec_set = OrderedDict.fromkeys([x for x in res]) diff --git a/addons/account/tests/test_reconciliation.py b/addons/account/tests/test_reconciliation.py index 5dab1ba6a65..ae2e3a4dcbb 100644 --- a/addons/account/tests/test_reconciliation.py +++ b/addons/account/tests/test_reconciliation.py @@ -348,3 +348,61 @@ class TestReconciliation(AccountingTestCase): counterpart_exchange_loss_line = line # We should be able to find a move line of 0.01 EUR on the Foreign Exchange Loss account self.assertTrue(counterpart_exchange_loss_line, 'There should be one move line of 0.01 EUR on account "Foreign Exchange Loss"') + + def test_manual_reconcile_wizard_opw678153(self): + + def create_move(name, amount, amount_currency, currency_id): + debit_line_vals = { + 'name': name, + 'debit': amount > 0 and amount or 0.0, + 'credit': amount < 0 and -amount or 0.0, + 'account_id': self.account_rcv.id, + 'amount_currency': amount_currency, + 'currency_id': currency_id, + } + credit_line_vals = debit_line_vals.copy() + credit_line_vals['debit'] = debit_line_vals['credit'] + credit_line_vals['credit'] = debit_line_vals['debit'] + credit_line_vals['account_id'] = self.account_rsa.id + credit_line_vals['amount_currency'] = -debit_line_vals['amount_currency'] + vals = { + 'journal_id': self.bank_journal_euro.id, + 'line_ids': [(0,0, debit_line_vals), (0, 0, credit_line_vals)] + } + return self.env['account.move'].create(vals).id + move_list_vals = [ + ('1', -1.83, 0, self.currency_swiss_id), + ('2', 728.35, 795.05, self.currency_swiss_id), + ('3', -4.46, 0, self.currency_swiss_id), + ('4', 0.32, 0, self.currency_swiss_id), + ('5', 14.72, 16.20, self.currency_swiss_id), + ('6', -737.10, -811.25, self.currency_swiss_id), + ] + move_ids = [] + for name, amount, amount_currency, currency_id in move_list_vals: + move_ids.append(create_move(name, amount, amount_currency, currency_id)) + aml_recs = self.env['account.move.line'].search([('move_id', 'in', move_ids), ('account_id', '=', self.account_rcv.id)]) + wizard = self.env['account.move.line.reconcile'].with_context(active_ids=[x.id for x in aml_recs]).create({}) + wizard.trans_rec_reconcile_full() + for aml in aml_recs: + self.assertTrue(aml.reconciled, 'The journal item should be totally reconciled') + self.assertEquals(aml.amount_residual, 0, 'The journal item should be totally reconciled') + self.assertEquals(aml.amount_residual_currency, 0, 'The journal item should be totally reconciled') + + move_list_vals = [ + ('2', 728.35, 795.05, self.currency_swiss_id), + ('3', -4.46, 0, False), + ('4', 0.32, 0, False), + ('5', 14.72, 16.20, self.currency_swiss_id), + ('6', -737.10, -811.25, self.currency_swiss_id), + ] + move_ids = [] + for name, amount, amount_currency, currency_id in move_list_vals: + move_ids.append(create_move(name, amount, amount_currency, currency_id)) + aml_recs = self.env['account.move.line'].search([('move_id', 'in', move_ids), ('account_id', '=', self.account_rcv.id)]) + wizard = self.env['account.move.line.reconcile.writeoff'].with_context(active_ids=[x.id for x in aml_recs]).create({'journal_id': self.bank_journal_usd.id, 'writeoff_acc_id': self.account_rsa.id}) + wizard.trans_rec_reconcile() + for aml in aml_recs: + self.assertTrue(aml.reconciled, 'The journal item should be totally reconciled') + self.assertEquals(aml.amount_residual, 0, 'The journal item should be totally reconciled') + self.assertEquals(aml.amount_residual_currency, 0, 'The journal item should be totally reconciled') diff --git a/addons/account/wizard/account_reconcile.py b/addons/account/wizard/account_reconcile.py index da87e01ad93..25d6c2790eb 100644 --- a/addons/account/wizard/account_reconcile.py +++ b/addons/account/wizard/account_reconcile.py @@ -1,5 +1,6 @@ from openerp import models, fields, api, _ from openerp.tools.float_utils import float_round +from openerp.exceptions import UserError class AccountMoveLineReconcile(models.TransientModel): @@ -55,7 +56,21 @@ class AccountMoveLineReconcile(models.TransientModel): @api.multi def trans_rec_reconcile_full(self): move_lines = self.env['account.move.line'].browse(self._context.get('active_ids', [])) - move_lines.reconcile() + currency = False + for aml in move_lines: + if not currency and aml.currency_id.id: + currency = aml.currency_id.id + elif aml.currency_id: + if aml.currency_id.id == currency: + continue + raise UserError(_('Operation not allowed. You can only reconcile entries that share the same secondary currency or that don\'t have one. Edit your journal items or make another selection before proceeding any further.')) + #Because we are making a full reconcilition in batch, we need to consider use cases as defined in the test test_manual_reconcile_wizard_opw678153 + #So we force the reconciliation in company currency only at first, then in second pass the amounts in secondary currency. + move_lines.with_context(skip_full_reconcile_check='amount_currency_excluded', manual_full_reconcile_currency=currency).reconcile() + move_lines_filtered = move_lines.filtered(lambda aml: not aml.reconciled) + if move_lines_filtered: + move_lines_filtered.with_context(skip_full_reconcile_check='amount_currency_only', manual_full_reconcile_currency=currency).reconcile() + move_lines.compute_full_after_batch_reconcile() return {'type': 'ir.actions.act_window_close'} @@ -100,5 +115,21 @@ class AccountMoveLineReconcileWriteoff(models.TransientModel): if self.analytic_id: context['analytic_id'] = self.analytic_id.id move_lines = self.env['account.move.line'].browse(self._context.get('active_ids', [])) - move_lines.with_context(context).reconcile(self.writeoff_acc_id, self.journal_id) + currency = False + for aml in move_lines: + if not currency and aml.currency_id.id: + currency = aml.currency_id.id + elif aml.currency_id: + if aml.currency_id.id == currency: + continue + raise UserError(_('Operation not allowed. You can only reconcile entries that share the same secondary currency or that don\'t have one. Edit your journal items or make another selection before proceeding any further.')) + #Because we are making a full reconcilition in batch, we need to consider use cases as defined in the test test_manual_reconcile_wizard_opw678153 + #So we force the reconciliation in company currency only at first, then in second pass the amounts in secondary currency. + context['skip_full_reconcile_check'] = 'amount_currency_excluded' + context['manual_full_reconcile_currency'] = currency + writeoff = move_lines.with_context(context).reconcile(self.writeoff_acc_id, self.journal_id) + move_lines_filtered = move_lines.filtered(lambda aml: not aml.reconciled) + if move_lines_filtered: + move_lines_filtered.with_context(skip_full_reconcile_check='amount_currency_only', manual_full_reconcile_currency=currency).reconcile() + (move_lines + writeoff).compute_full_after_batch_reconcile() return {'type': 'ir.actions.act_window_close'} diff --git a/addons/account_full_reconcile/models/account_full_reconcile.py b/addons/account_full_reconcile/models/account_full_reconcile.py index f4141a3a88f..bb5e30f1adb 100644 --- a/addons/account_full_reconcile/models/account_full_reconcile.py +++ b/addons/account_full_reconcile/models/account_full_reconcile.py @@ -10,6 +10,33 @@ class AccountMoveLine(models.Model): _inherit = "account.move.line" full_reconcile_id = fields.Many2one('account.full.reconcile', string="Matching Number") + @api.model + def compute_full_after_batch_reconcile(self): + super(AccountMoveLine, self).compute_full_after_batch_reconcile() + #check if the reconcilation is full + partial_rec_set = self.env['account.partial.reconcile'] + total_debit = 0 + total_credit = 0 + total_amount_currency = 0 + currency = False + for aml in self: + total_debit += aml.debit + total_credit += aml.credit + if not currency and aml.currency_id: + currency = aml.currency_id + if aml.currency_id and aml.currency_id == currency: + total_amount_currency += aml.amount_currency + partial_rec_set |= aml.matched_debit_ids | aml.matched_credit_ids + partial_rec_ids = [x.id for x in list(partial_rec_set)] + #if the total debit and credit are equal, and the total amount in currency is 0, the reconciliation is full + digits_rounding_precision = self[0].company_id.currency_id.rounding + if float_compare(total_debit, total_credit, precision_rounding=digits_rounding_precision) == 0 \ + and (not currency or float_is_zero(total_amount_currency, precision_rounding=currency.rounding)): + #in that case, mark the reference on the partial reconciliations and the entries + self.env['account.full.reconcile'].with_context(check_move_validity=False).create({ + 'partial_reconcile_ids': [(6, 0, partial_rec_ids)], + 'reconciled_line_ids': [(6, 0, self.ids)]}) + class AccountPartialReconcile(models.Model): _inherit = "account.partial.reconcile" @@ -19,6 +46,10 @@ class AccountPartialReconcile(models.Model): @api.model def create(self, vals): res = super(AccountPartialReconcile, self).create(vals) + if self._context.get('skip_full_reconcile_check'): + #when running the manual reconciliation wizard, don't check the partials separately for full + #reconciliation or exchange rate because it is handled manually after the whole processing + return res #check if the reconcilation is full #first, gather all journal items involved in the reconciliation just created partial_rec_set = OrderedDict.fromkeys([x for x in res])