Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] account: reconciliation matching rule div by zero #32105

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

[FIX] account: reconciliation matching rule div by zero

Do a MISC operation representing a payment
revert it

Do a statement line for that amount

Click on "reconcile" on the bank journal

Before this commit there was a Division By Zero traceback
This was because the bank account line had a zero amount_residual
due to its reversion
while the statement line was negative.

After this commit, for liquidity lines, we take the total amount instead of the residual
Because a reverted payment line can always be associated with a statement

OPW 1947362
courtesy of @smetl (las@odoo.com) for the fix
  • Loading branch information...
kebeclibre committed Mar 25, 2019
commit a213b53c59452b676bdfc4293dccbe3fceb1517d
@@ -393,6 +393,9 @@ def _get_invoice_matching_query(self, st_lines, excluded_ids=None, partner_map=N
aml.date_maturity AS aml_date_maturity,
aml.amount_residual AS aml_amount_residual,
aml.amount_residual_currency AS aml_amount_residual_currency,
aml.balance AS aml_balance,
aml.amount_currency AS aml_amount_currency,
account.internal_type AS account_internal_type,
-- Determine a matching or not with the statement line communication using the move.name or move.ref.
regexp_split_to_array(TRIM(REGEXP_REPLACE(move.name, '[^0-9|^\s]', '', 'g')),'\s+')
@@ -540,9 +543,12 @@ def _check_rule_propositions(self, statement_line, candidates):
return True

# Match total residual amount.
total_residual = sum(
aml['aml_currency_id'] and aml['aml_amount_residual_currency'] or aml['aml_amount_residual'] for aml in
candidates)
total_residual = 0.0
for aml in candidates:
if aml['account_internal_type'] == 'liquidity':
total_residual += aml['aml_currency_id'] and aml['aml_amount_currency'] or aml['aml_balance']
else:
total_residual += aml['aml_currency_id'] and aml['aml_amount_residual_currency'] or aml['aml_amount_residual']
line_residual = statement_line.currency_id and statement_line.amount_currency or statement_line.amount
line_currency = statement_line.currency_id or statement_line.journal_id.currency_id or statement_line.company_id.currency_id

@@ -24,8 +24,10 @@ def _create_invoice_line(self, amount, partner, type):
lines = invoice.move_id.line_ids
return lines.filtered(lambda l: l.account_id == invoice.account_id)

def _check_statement_matching(self, rules, expected_values):
statement_lines = (self.bank_st + self.cash_st).mapped('line_ids')
def _check_statement_matching(self, rules, expected_values, statements=None):
if statements is None:
statements = self.bank_st + self.cash_st
statement_lines = statements.mapped('line_ids')
matching_values = rules._apply_rules(statement_lines)
for st_line_id, values in matching_values.items():
values.pop('reconciled_lines', None)
@@ -34,6 +36,10 @@ def _check_statement_matching(self, rules, expected_values):
def setUp(self):
super(AccountingTestCase, self).setUp()

self.account_pay = self.env['account.account'].search([('internal_type', '=', 'payable')], limit=1)
self.account_liq = self.env['account.account'].search([('internal_type', '=', 'liquidity')], limit=1)
self.account_rcv = self.env['account.account'].search([('internal_type', '=', 'receivable')], limit=1)

self.partner_1 = self.env['res.partner'].create({'name': 'partner_1'})
self.partner_2 = self.env['res.partner'].create({'name': 'partner_2'})

@@ -45,7 +51,8 @@ def setUp(self):
current_assets_account = self.env['account.account'].search(
[('user_type_id', '=', self.env.ref('account.data_account_type_current_assets').id)], limit=1)

self.rule_1 = self.env.ref('account.reconciliation_model_default_rule')
self.rule_0 = self.env.ref('account.reconciliation_model_default_rule')
self.rule_1 = self.rule_0.copy()
self.rule_1.account_id = current_assets_account
self.rule_1.match_partner = True
self.rule_1.match_partner_ids |= self.partner_1 + self.partner_2
@@ -59,10 +66,10 @@ def setUp(self):

invoice_number = self.invoice_line_1.move_id.name

bank_journal = self.env['account.journal'].search([('type', '=', 'bank')], limit=1)
self.bank_journal = self.env['account.journal'].search([('type', '=', 'bank')], limit=1)

self.bank_st = self.env['account.bank.statement'].create({
'name': 'test bank journal', 'journal_id': bank_journal.id,
'name': 'test bank journal', 'journal_id': self.bank_journal.id,
})
self.bank_line_1 = self.env['account.bank.statement.line'].create({
'statement_id': self.bank_st.id,
@@ -296,3 +303,47 @@ def test_auto_reconcile(self):
{'partner_id': self.partner_2.id, 'debit': 0.0, 'credit': 1000.0},
{'partner_id': self.partner_2.id, 'debit': 1000.0, 'credit': 0.0},
])

def test_reverted_move_matching(self):
AccountMove = self.env['account.move']
move = AccountMove.create({
'name': 'To Revert',
'journal_id': self.bank_journal.id,
})

partner = self.env['res.partner'].create({'name': 'Eugene'})
AccountMoveLine = self.env['account.move.line'].with_context(check_move_validity=False)
payment_payable_line = AccountMoveLine.create({
'account_id': self.account_pay.id,
'move_id': move.id,
'partner_id': partner.id,
'name': 'One of these days',
'debit': 10,
})
payment_bnk_line = AccountMoveLine.create({

This comment has been minimized.

Copy link
@smetl

smetl Mar 26, 2019

Contributor

@kebeclibre I highly encourage you to define your one2many like:

'line_ids': [(0, 0, {...}), (0, 0, {...}), ....]

...because it's:

  • more efficient (create batch)
  • more readable (less code)
  • avoid trick like .with_context(check_move_validity=False)

This comment has been minimized.

Copy link
@kebeclibre

kebeclibre Mar 26, 2019

Author Contributor

I feel you bro, but in tests we usually need a reference to a specific line to check it against some values,
just like I do at l347
Otherwise, we would have to filter.map with lambdas, which is, I guess, not very clearer :D

'account_id': self.account_liq.id,
'move_id': move.id,
'partner_id': partner.id,
'name': 'I\'m gonna cut you into little pieces',
'credit': 10,
})

move.post()
move_reversed = AccountMove.browse(move.reverse_moves())
self.assertTrue(move_reversed.exists())

bank_st = self.env['account.bank.statement'].create({
'name': 'test bank journal', 'journal_id': self.bank_journal.id,
})
bank_line_1 = self.env['account.bank.statement.line'].create({
'statement_id': bank_st.id,
'name': '8',
'partner_id': partner.id,
'amount': -10,
'sequence': 1,
})

expected_values = {
bank_line_1.id: {'aml_ids': [payment_bnk_line.id], 'model': self.rule_0}
}
self._check_statement_matching(self.rule_0, expected_values, statements=bank_st)
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.