[14.0][IMP]Account_invoice_triple_discount: For in invoice_line_ids and not in line_ids #1442
Conversation
odooNextev
changed the title
|
@SirTakobi I opened this PR try to solving OCA/l10n-italy#3269 tests. |
I'm sorry but I can't see how this change is affecting the behavior of this module, could you please add a test here that shows which use-case is being fixed? Or at least explain it a bit? Also, I don't understand from your message if this change is actually fixing the tests of OCA/l10n-italy#3269, could you please clarify that? |
|
Test failed because of negatives price unit and tax amount on invoice line(in a refund invoice). Using this module it retrieve a wrong total amount because tax amount always had wrong sign. This change fixed this problem according to our tests. |
|
So it has to do with Tax lines changing their sign, that's why now you are only considering I'm sorry but this fix is still making little sense to me as a standalone change, is it possible to add a test showing what has been fixed? |
|
@SirTakobi I added a test trying to repoduce what l10n_it_fatturapa_in does with refunds with negative total. It look that it works properly |
|
@SirTakobi what do you think? |
| for line in self.line_ids: | ||
| digits = self.invoice_line_ids._fields["price_unit"]._digits | ||
| self.invoice_line_ids._fields["price_unit"]._digits = (16, 16) | ||
| for line in self.invoice_line_ids: |
There was a problem hiding this comment.
Thanks for the test, now I can check what is going wrong.
As you suggested, for some reason the sign of price_unit of the tax line is changed when calling https://github.com/odoo/odoo/blob/5a3a8d0b1fcc982f813ca77f744a0612abbe7b94/addons/account/models/account_move.py#L602; the method itself is not very clear to me, but the actual switch might be happening in https://github.com/odoo/odoo/blob/5a3a8d0b1fcc982f813ca77f744a0612abbe7b94/addons/account/models/account_move.py#L628.
Apparently the module is trying to manage adding multiple discounts on the tax lines but we can skip them, and we probably should also because multiple discounts can only be assigned on invoice lines (not tax lines) from the UI.
Another fix might be to only edit the lines that we have to: for instance skip the lines that have no aggregated_discount because for those lines the module is only writing the same price_unit and discount after calling super, that is not very useful
| invoice.with_context(check_move_validity=False)._recompute_dynamic_lines( | ||
| recompute_all_taxes=True | ||
| ) | ||
| self.assertEqual(invoice.move_type, "in_refund") |
There was a problem hiding this comment.
In my opinion, editing the invoice with check_move_validity=False is risky, and should be avoided as much as possible.
If really needed, the invoice should be balanced when you are done editing it.
I tried this locally and here the invoice is no more balanced: calling invoice._check_balanced() raises:
odoo.sql_db: bad query: UPDATE "account_move_line" SET "amount_currency"='0.00',"amount_residual"='23.00',"amount_residual_currency"='0.00',"balance"='23.00',"credit"='0.00',"debit"='23.00',"price_subtotal"='0.00',"price_total"='0.00',"price_unit"='0.00',"reconciled"=false,"tax_audit"='',"write_uid"=1,"write_date"=(now() at time zone 'UTC') WHERE id IN (242)
ERROR: new row for relation "account_move_line" violates check constraint "account_move_line_check_amount_currency_balance_sign"
DETAIL: Failing row contains (242, 83, Test Refund for Triple Discount, 2023-06-12, null, draft, 2, 1, 2, 14, 50049, 10, , 1.00, 0.00, 0.00, 23.00, 0.00, 23.00, 0.00, 0.00, 0.00, f, f, 2023-06-12, 2, null, null, null, null, null, null, null, null, null, 0.00, t, null, , 23.00, 0.00, null, null, null, null, f, t, 1, 2023-06-12 10:52:10.539377, 1, 2023-06-12 10:52:10.539377, 0.00, 0.00).
I know this is only the test and you are not using check_move_validity=False in the module's code but looks to me like you are checking assertions on something that is not possible (because usually you can't have an invoice that is not balanced), thus the assertions have little value.
There was a problem hiding this comment.
@SirTakobi Thanks for your review. I tried to add a test to replace behavior of l10n_it_fatturapa if there are some negative lines https://github.com/OCA/l10n-italy/blob/14.0/l10n_it_fatturapa_in/models/account.py#L321.
There was a problem hiding this comment.
@SirTakobi I made some changes and now the invoice is balanced. Could you please check if everything look good to you? Thanks
|
@SirTakobi is it good now or is some other change needed? |
|
There was a problem hiding this comment.
Please follow https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#71commit-message for the commits messages.
For instance:
Always put meaningful commit messages: commit messages should be self explanatory (long enough) including the name of the module that has been changed and the reason behind that change. Do not use single words like "bugfix" or "improvements".
account_invoice_triple_discount/tests/test_invoice_triple_discount.py
Outdated
Show resolved
Hide resolved
account_invoice_triple_discount/tests/test_invoice_triple_discount.py
Outdated
Show resolved
Hide resolved
| @@ -164,3 +183,26 @@ def test_04_discounts_decimals_tax(self): | |||
| invoice_form.save() | |||
|
|
|||
| self.assertEqual(invoice.amount_tax, 177.61) | |||
|
|
|||
| def test_05_refund_negative_taxes(self): | |||
There was a problem hiding this comment.
I tried this locally and this test is passing even without the changes to account_move.
When adding a fix, the included test should pass with the fix and break without it.
There was a problem hiding this comment.
@SirTakobi now I fixed this test and without our fix in _recompute_tax_lines it fails
| @@ -164,3 +183,26 @@ def test_04_discounts_decimals_tax(self): | |||
| invoice_form.save() | |||
|
|
|||
| self.assertEqual(invoice.amount_tax, 177.61) | |||
|
|
|||
| def test_05_refund_negative_taxes(self): | |||
account_invoice_triple_discount/tests/test_invoice_triple_discount.py
Outdated
Show resolved
Hide resolved
account_invoice_triple_discount/tests/test_invoice_triple_discount.py
Outdated
Show resolved
Hide resolved
| refund.with_context(check_move_validity=False)._recompute_dynamic_lines( | ||
| recompute_all_taxes=True, | ||
| ) | ||
| self.assertEqual(refund.move_type, "in_refund") |
There was a problem hiding this comment.
I tried this locally and here the refund is no more balanced: calling refund._check_balanced() raises:
odoo.sql_db: bad query: UPDATE "account_move_line" SET "amount_currency"='0.00',"amount_residual"='23.00',"amount_residual_currency"='0.00',"balance"='23.00',"credit"='0.00',"debit"='23.00',"price_subtotal"='0.00',"price_total"='0.00',"price_unit"='0.00',"reconciled"=false,"tax_audit"='',"write_uid"=1,"write_date"=(now() at time zone 'UTC') WHERE id IN (125)
ERROR: new row for relation "account_move_line" violates check constraint "account_move_line_check_amount_currency_balance_sign"
DETAIL: Failing row contains (125, 44, Test Refund for Triple Discount, 2023-08-07, null, draft, 2, 1, 2, 14, 50049, 10, , 1.00, 0.00, 0.00, 23.00, 0.00, 23.00, 0.00, 0.00, 0.00, f, f, 2023-08-07, 2, null, null, null, null, null, null, null, null, null, 0.00, t, null, , 23.00, 0.00, null, null, null, null, f, t, 1, 2023-08-07 08:48:13.137036, 1, 2023-08-07 08:48:13.137036, 0.00, 0.00).
so I have the same concerns of 2 months ago: #1442 (comment)
There was a problem hiding this comment.
I tried this locally and here the
refundis no more balanced: callingrefund._check_balanced()raises:odoo.sql_db: bad query: UPDATE "account_move_line" SET "amount_currency"='0.00',"amount_residual"='23.00',"amount_residual_currency"='0.00',"balance"='23.00',"credit"='0.00',"debit"='23.00',"price_subtotal"='0.00',"price_total"='0.00',"price_unit"='0.00',"reconciled"=false,"tax_audit"='',"write_uid"=1,"write_date"=(now() at time zone 'UTC') WHERE id IN (125) ERROR: new row for relation "account_move_line" violates check constraint "account_move_line_check_amount_currency_balance_sign" DETAIL: Failing row contains (125, 44, Test Refund for Triple Discount, 2023-08-07, null, draft, 2, 1, 2, 14, 50049, 10, , 1.00, 0.00, 0.00, 23.00, 0.00, 23.00, 0.00, 0.00, 0.00, f, f, 2023-08-07, 2, null, null, null, null, null, null, null, null, null, 0.00, t, null, , 23.00, 0.00, null, null, null, null, f, t, 1, 2023-08-07 08:48:13.137036, 1, 2023-08-07 08:48:13.137036, 0.00, 0.00).so I have the same concerns of 2 months ago: #1442 (comment)
@SirAionTech I don't understand what's wrong in lines recomputation at line 193...
If you duplicate lines 193-195 or manually launch double _recompute_dynamic_lines() from debug console, lines will be balanced and it will pass the test...
I don't know why we need 2 recomputations to balance taxes lines and I guess there are no issues in this method cause it's from Odoo standard accounting
| for line in self.invoice_line_ids | ||
| for line in self.line_ids |
There was a problem hiding this comment.
This check was on line_ids, why is it changed? Does not seem to affect the added test result
|
Please @eantones could you review this PR? |
|
@SirTakobi @odooNextev have you news? |
I just solved a couple of issues and I tried to relaunch tests cause in my local environment I wasn't able to reproduce tests fails in account_invoice_mass_sending. |
|
I propose to move here #1592 |
|
superseeded by #1592 |
We need this fix to avoid issues with
l10n_it_fatturapa_in(https://github.com/OCA/l10n-italy/tree/14.0/l10n_it_fatturapa_in).In this module there's a test that trying to import a refund with negative amounts and without this fix it fails cause tax sign it's not reversed unlike price unit.
So we analyzed how taxes are recomputed in 16.0 version of
account_invoice_triple_discountand we found something different that adapted and applied to 14.0 version should no longer get issues with negative amounts.