WIP 12.0 account invoice discount lines #1
Conversation
|
@StefanRijnhart you can ignore this for now. I didn't finish with the tests and did not check if the view was properly changed. |
|
Ready for review |
| self.assertEqual(len(invoice.invoice_line_ids), 1) | ||
| base_line = invoice.invoice_line_ids | ||
| self.assertTrue( | ||
| self.env.user.company_id.currency_id.is_zero( |
There was a problem hiding this comment.
In the version 8 tests the amount was 99 eeadbaa#diff-4a95f0cfd40b3af02d5c39b446330838R74
As far as I understand, this it should be 90. As 100 minus 10% discount is 90. Maybe I am missing something.
This assertion has been repeated in all of the tests below.
There was a problem hiding this comment.
I think the 90 euros is the net amount. The additional 9 euros is the 10% tax.
There was a problem hiding this comment.
OK than I don't have the tax in the demo setup. Should I change the test so we get tax on the line?
There was a problem hiding this comment.
Yes please! I added the tax to the test on purpose, to make sure that the discount lines mechanism does not mess up the tax amount.
There was a problem hiding this comment.
Tax was there I was writing it to a wrong field "invoice_line_tax_id" vs "invoice_line_tax_ids" now I get different numbers. Here for tax .1 total value is 90.09 - seams correct to me but not 99.0 like the original test.
After applying the discount line we have total value 90.10 because tax was applied on the 100 USD line.
Later on https://github.com/VanMoof/account-invoicing/pull/1/files#diff-4a95f0cfd40b3af02d5c39b446330838R106 the test fails because of a rounding is_zero() this is the difference 0.009999999999990905 the currency.rounding is 0.01. This is just wrong 0.009999999999990905 is less than 0.01.I guess 0.009999999999990905 is rounded up to 0.01 and the is_zero() fails.
Ill check the version 8.0 now to figure out how and why the old test passes.
Just a note testing with .1 percent maybe misleading because with higher values that is_zero() check will not work. I guess even in 8.0 - Ill check this theory.
There was a problem hiding this comment.
Update:
Test in the version 8 shows as follows
That is tax of 10%
What I think happens is meaning of the tax amount field that we set here https://github.com/VanMoof/account-invoicing/pull/1/files#diff-4a95f0cfd40b3af02d5c39b446330838R14 is changed. In 8 ".1" means 10% in 12 ".1" means exactly what is says 0.1 %
That explains part of the problem. If I adjust for the change I still get different amounts here https://github.com/VanMoof/account-invoicing/pull/1/files#diff-4a95f0cfd40b3af02d5c39b446330838R79 like in the picture above. Now I have 99.0 with one line and 100 with discount line. This makes more sense in the 12 to me as having an amount, applying discount and calculating tax on that sum will always be different than having full amount, applying tax and than subtracting the discount amount (shown in the first to pictures).
There was a problem hiding this comment.
Wow. Odoo is really trying to piss us off aren't they, with changed name fields and changed semantics of amount precisions. But great that you tracked these changes! As for the taxes I have made a suggestion above.
|
OD12-12 (we don't put the ticket number in the commit message so Ill start putting them here in the comments) |
| 'discount_real': 0, | ||
| }) | ||
| discount_line._onchange_product_id() | ||
| discount_line.invoice_line_tax_ids = False |
There was a problem hiding this comment.
You remove the tax from the line. But we need to have the same tax as on the original line (so that the tax amount and the tax administration does not change between the invoice with the discount on the original line and the invoice with a separate discount line). By copying the line, I retain that tax on the new line. The onchange method might reset the tax depending on the product settings. That is why I wipe it here: https://github.com/StefanRijnhart/account-invoicing/blob/8.0-account_invoice_discount_lines/account_invoice_discount_lines/models/account_invoice.py#L78. You're going to have to check how that works in the new API and if there is a chance that the tax is overridden.
… after adding the discount line
| 'discount_real': 0, | ||
| }) | ||
| discount_line._onchange_product_id() | ||
| discount_line.invoice_line_tax_ids = ( |
There was a problem hiding this comment.
A bit of good news: This solves our problems and ll tests match up now. The reason why I set it to flase in the firs place is this eeadbaa#diff-0141a136b71ad23276773845112b41dcR78. I saw in 8 that we do not write the value of the on_change() return, but this does not mean that there is no value at all in the tax field. At some point I thought negative value tax should solve my problem, but than I thought ERP nd negative value tax nooo.
|
Can you squash your commits so that I can check your changes? Did you check the note on https://vmbikes.atlassian.net/browse/OD12-12 about how this interacts with sale line's invoiced_qty? |
No I did not. I will have a look now. |
| <xpath expr="//field[@name='invoice_line_ids']/tree/field[@name='discount']" | ||
| position="before"> | ||
| <field name="discount" invisible="1"/> |
There was a problem hiding this comment.
How ever stupid this looks it works. I cannot find an explanation.
This will not work
<xpath expr="//field[@name='invoice_line_ids']/tree/field[@name='discount']"
position="after">
<field name="discount_display" string="Discount (%)"/>
</xpath>
<xpath expr="//field[@name='invoice_line_ids']/tree/field[@name='discount']"
position="attributes">
<attribute name="invisible">True</attribute>
</xpath>
There was a problem hiding this comment.
That's very strange indeed. Usually, having the same field in the view twice can lead to problems. How does it not work to set the attribute this way? What is the (undesired) effect?
|
Te bug that appears is when clicking into a field and changing the value,
and clicking out, immediately upon clicking out, the value flips back. Very
irritating.
The inverse function works. It just fires on write. In our case it does not
have the correct field value cashed on the view.
…On Thu, 4 Jun 2020, 08:09 Stefan Rijnhart (Opener), < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In account_invoice_discount_lines/views/account_invoice.xml
<#1 (comment)>
:
> + <xpath ***@***.******@***.***='discount']"
+ position="before">
+ <field name="discount" invisible="1"/>
That's very strange indeed. Usually, having the same field in the view
twice can lead to problems. How does it not work to set the attribute this
way? What is the (undesired) effect?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AK32JGDSI3J65QONAMNV6QTRU43C5ANCNFSM4NFCPH6A>
.
|
|
I will squash this in to Should I leave your PRs that come after mine and make them one commit. So we have 3 commits? |
|
No, you can squash all the commits after d75f1ea into a single migration commit. |
97fb472 to
a98cbff
Compare
Co-authored-by: Andrea Stirpe <a.stirpe@onestein.nl>
|
@astirpe Thanks for solving this mysterious warning all over the tests! |
|
Wow this is so old I forgot that I made this PR. What are we doing with this? Close it or merge? |
|
You can close the PR if you want but don't delete the branch as we are going to use this branch in production! |
|
I didn't see the repo. I'll leave it for nice overview. |
No description provided.