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.

Sign up for GitHub

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

Jump to bottom

[14.0][IMP]Account_invoice_triple_discount: For in invoice_line_ids and not in line_ids #1442

Closed
wants to merge 9 commits into from
Closed

[14.0][IMP]Account_invoice_triple_discount: For in invoice_line_ids and not in line_ids #1442

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
18e8df2
[IMP]For in invoice line and not in line_ids
matteonext Apr 17, 2023
e90c53f
[IMP]Add tests related to avoid updates in confilct with l10n-italy m…
matteonext Jun 7, 2023
d25a872
[IMP]Pre commit stuff
matteonext Jun 7, 2023
69c4fdf
[IMP]Fix invoice balance
matteonext Jul 27, 2023
c6e7391
[FIX] Fix _recompute_tax_lines to avoid issues with negative tax line…
stenext Aug 3, 2023
7fc2600
[FIX] Fix taxes assigment in tests
stenext Aug 3, 2023
9a26232
[FIX] Removed unuseful comment in tests
stenext Aug 4, 2023
10df6db
[FIX] Fix test refund_negative_taxes to justify improvement in _recom…
stenext Aug 4, 2023
43cf80b
[FIX] Changed _recompute_tax_lines doc according to method changes
stenext Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions account_invoice_triple_discount/models/account_move.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,19 @@ def _recompute_tax_lines(self, **kwargs):
restored after the original process is done
"""
old_values_by_line_id = {}
digits = self.line_ids._fields["price_unit"]._digits
self.line_ids._fields["price_unit"]._digits = (16, 16)
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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

aggregated_discount = line._compute_aggregated_discount(line.discount)
old_values_by_line_id[line.id] = {
"price_unit": line.price_unit,
"discount": line.discount,
}
price_unit = line.price_unit * (1 - aggregated_discount / 100)
line.update({"price_unit": price_unit, "discount": 0})
self.line_ids._fields["price_unit"]._digits = digits
self.invoice_line_ids._fields["price_unit"]._digits = digits
res = super(AccountMove, self)._recompute_tax_lines(**kwargs)
for line in self.line_ids:
for line in self.invoice_line_ids:
if line.id not in old_values_by_line_id:
continue
line.update(old_values_by_line_id[line.id])
Expand Down
42 changes: 42 additions & 0 deletions account_invoice_triple_discount/tests/test_invoice_triple_discount.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,25 @@ def setUpClass(cls):
)
cls.sale_journal = cls.Journal.search([("type", "=", "sale")], limit=1)

@classmethod
def _create_refund(cls, tax=False, date=False, in_type=False):
odooNextev marked this conversation as resolved.
Show resolved Hide resolved
refund_form = Form(
cls.env["account.move"].with_context(default_move_type="in_refund")
)
# refund_form.partner_id = partner
odooNextev marked this conversation as resolved.
Show resolved Hide resolved
refund_form.name = "Test Refund for Triple Discount"

with refund_form.invoice_line_ids.new() as refund_line:
refund_line.quantity = 1.00
refund_line.name = "test refund line"
refund_line.price_unit = 100.00
if tax:
refund_line.tax_ids.clear()
refund_line.tax_ids.add(tax)

refund = refund_form.save()
return refund

def create_simple_invoice(self, amount):
invoice_form = Form(
self.AccountMove.with_context(
Expand Down Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SirTakobi now I fixed this test and without our fix in _recompute_tax_lines it fails

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

"""
Tests refund negative taxes
"""
invoice = self._create_refund(0)
invoice_form = Form(invoice)

with invoice_form.invoice_line_ids.edit(0) as line_form:
line_form.name = "Negative amounts"
line_form.quantity = 10
line_form.price_unit = -2.00
invoice_form.save()
for line in invoice.line_ids:
line.with_context(check_move_validity=False).update(
{"price_unit": -line.price_unit}
)
invoice.with_context(check_move_validity=False)._recompute_dynamic_lines(
recompute_all_taxes=True,
)
invoice._check_balanced()
self.assertEqual(invoice.move_type, "in_refund")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@odooNextev odooNextev Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SirTakobi I made some changes and now the invoice is balanced. Could you please check if everything look good to you? Thanks

self.assertEqual(round(invoice.amount_total, 2), 23.0)
Loading