Tryton - Issues

 

Issue9111

Title Taxes are lost when using Modify Header Wizard
Priority bug Status resolved
Superseder Add contextual fields in on_change arguments, Ensure all contextual field are in the cache to use the cache
View: 9236, 9187
Nosy List ced, edbo, pokoli, reviewbot, roundup-bot
Type behavior Components account_asset, account_invoice, purchase, sale, sale_subscription
Assigned To pokoli Keywords review
Reviews 275031002
View: 275031002

Created on 2020-03-03.13:57:13 by pokoli, last changed by roundup-bot.

Messages
New changeset 6e82c234229e by Sergi Almacellas Abellana in branch 'default':
Add company as product context field
https://hg.tryton.org/tryton-env/rev/6e82c234229e
New changeset 86b81ab7f63a by Sergi Almacellas Abellana in branch 'default':
Add company as product context field
https://hg.tryton.org/modules/sale_subscription/rev/86b81ab7f63a
New changeset 1af59832ba84 by Sergi Almacellas Abellana in branch 'default':
Add company as product context field
https://hg.tryton.org/modules/sale/rev/1af59832ba84
New changeset 8a091dd118ff by Sergi Almacellas Abellana in branch 'default':
Add company as product context field
https://hg.tryton.org/modules/purchase/rev/8a091dd118ff
New changeset 6dc86cd483bd by Sergi Almacellas Abellana in branch 'default':
Add company as product context field
https://hg.tryton.org/modules/account_invoice/rev/6dc86cd483bd
New changeset c7fd1a45e241 by Sergi Almacellas Abellana in branch 'default':
Add company as product context field
https://hg.tryton.org/modules/account_asset/rev/c7fd1a45e241
msg57277 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2020-04-18.18:31:39
I can confirm that issue9236 makes the invoice module test pass.
msg57236 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2020-04-17.17:18:04
This is because the call to on_change_product depends implicitly of the company field to correctly instantiate the product field.
As we can not manage to keep on on_change's up to date with the proper depends, I created issue9236 to add automatically the depends.
review275031002 updated at https://codereview.tryton.org/275031002/#ps256971002
msg57180 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2020-04-15.17:20:35
Ok, I updated the review to add the company of the line. But when running the test suite I get the following errors: 

======================================================================
FAIL: /home/pokoli/projectes/nclone/trytond/trytond/modules/account_invoice/tests/scenario_invoice.rst
Doctest: scenario_invoice.rst
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib64/python3.8/doctest.py", line 2197, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for scenario_invoice.rst
  File "/home/pokoli/projectes/nclone/trytond/trytond/modules/account_invoice/tests/scenario_invoice.rst", line 0

----------------------------------------------------------------------
File "/home/pokoli/projectes/nclone/trytond/trytond/modules/account_invoice/tests/scenario_invoice.rst", line 153, in scenario_invoice.rst
Failed example:
    invoice.tax_amount
Expected:
    Decimal('20.00')
Got:
    Decimal('0.00')


======================================================================
FAIL: /home/pokoli/projectes/nclone/trytond/trytond/modules/account_invoice/tests/scenario_invoice_supplier.rst
Doctest: scenario_invoice_supplier.rst
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib64/python3.8/doctest.py", line 2197, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for scenario_invoice_supplier.rst
  File "/home/pokoli/projectes/nclone/trytond/trytond/modules/account_invoice/tests/scenario_invoice_supplier.rst", line 0

----------------------------------------------------------------------
File "/home/pokoli/projectes/nclone/trytond/trytond/modules/account_invoice/tests/scenario_invoice_supplier.rst", line 118, in scenario_invoice_supplier.rst
Failed example:
    invoice.tax_amount
Expected:
    Decimal('10.00')
Got:
    Decimal('0.00')


======================================================================
FAIL: /home/pokoli/projectes/nclone/trytond/trytond/modules/account_invoice/tests/scenario_credit_note.rst
Doctest: scenario_credit_note.rst
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib64/python3.8/doctest.py", line 2197, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for scenario_credit_note.rst
  File "/home/pokoli/projectes/nclone/trytond/trytond/modules/account_invoice/tests/scenario_credit_note.rst", line 0

----------------------------------------------------------------------
File "/home/pokoli/projectes/nclone/trytond/trytond/modules/account_invoice/tests/scenario_credit_note.rst", line 99, in scenario_credit_note.rst
Failed example:
    invoice.total_amount
Expected:
    Decimal('-220.00')
Got:
    Decimal('-200.00')


======================================================================
FAIL: /home/pokoli/projectes/nclone/trytond/trytond/modules/account_invoice/tests/scenario_invoice_alternate_currency.rst
Doctest: scenario_invoice_alternate_currency.rst
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib64/python3.8/doctest.py", line 2197, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for scenario_invoice_alternate_currency.rst
  File "/home/pokoli/projectes/nclone/trytond/trytond/modules/account_invoice/tests/scenario_invoice_alternate_currency.rst", line 0

----------------------------------------------------------------------
File "/home/pokoli/projectes/nclone/trytond/trytond/modules/account_invoice/tests/scenario_invoice_alternate_currency.rst", line 145, in scenario_invoice_alternate_currency.rst
Failed example:
    invoice.tax_amount
Expected:
    Decimal('40.00')
Got:
    Decimal('0.00')


All are related to not properly picking the right taxes for the invoice.
review275031002 updated at https://codereview.tryton.org/275031002/#ps289771002
msg57176 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2020-04-15.16:59:02
On 2020-04-15 16:21, Sergi Almacellas Abellana wrote:
> > For me, product on invoice line must also have the company in the context because it calls *_used properties which depend on company multivalue.
> > I ran the account_invoice tests with the product context having company set and I got no issue.
> 
> Did you have issue9187 patch applied? I'm not able to make it work.

Yes.

> Main problem on invoice is that there are two invoice fields. One for the line and other for the invoice. 

But only the company on invoice line is needed because it is required.

> If I use the one from the line the taxes are missing because when the product is instanciated the company is empty. So not sure how to solve this case.

I do not have this problem because there is a default_company on the
line which set it.
msg57175 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2020-04-15.16:21:41
> For me, product on invoice line must also have the company in the context because it calls *_used properties which depend on company multivalue.
> I ran the account_invoice tests with the product context having company set and I got no issue.

Did you have issue9187 patch applied? I'm not able to make it work.

Main problem on invoice is that there are two invoice fields. One for the line and other for the invoice. 

If I use the one from the line the taxes are missing because when the product is instanciated the company is empty. So not sure how to solve this case.
msg57017 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2020-04-11.15:13:00
For me, product on invoice line must also have the company in the context because it calls *_used properties which depend on company multivalue.
I ran the account_invoice tests with the product context having company set and I got no issue.
review275031002 updated at https://codereview.tryton.org/275031002/#ps299341002
msg56936 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2020-04-07.09:56:44
I've updated the review taking in account your comments. 

I've not modified the account_invoice module because it already has a company field on the line and adding company to context broke all test.
msg56699 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2020-03-27.15:53:06
Also sale_subscription service and account_asset product.
msg56698 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2020-03-27.15:49:05
The product on invoice should also have a company context set.
msg56697 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2020-03-27.15:48:02
Indeed it requires issue9187 to work correctly.
review275031002 updated at https://codereview.tryton.org/275031002/#ps286971002
msg56694 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2020-03-27.13:52:12
I've updated review275031002 with what is descrived on msg56647 but this does not seems to fix the issue.
msg56647 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2020-03-25.18:17:40
The context is not set because the value used is not in the depends and more over it can not be added in the depends because it is not a "real" field ('_parent_sale' see issue3901).
I think the proper way to fix that is to make such important context value use a direct field (computed). So in other word, add a 'company' field on sale line and purchase line.
And this should be done for all instances that are using the '*_used' attributes (so mainly product and party).
review275031002 updated at https://codereview.tryton.org/275031002/#ps289511002
msg56369 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2020-03-18.09:09:28
The problem is that line.product._context['company'] is None, so the issue is that the _parent_sale from [1] is empty, which causes that the product instance does not have the company set, and then taxes are empty.

Any idea on how to fix it? 

I've updated review275031002 to reproduce the issue on the text scenario.

[1] https://hg.tryton.org/modules/sale/file/fbe18aba79f1/sale.py#l1004
msg56347 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2020-03-17.22:58:12
No it should call on_change_product because taxes and unit price (and maybe other) must be recomputed.
The context must be fixed. It is strange because the product Many2One has company set in the context.
Also if it happens for sale, it must happen also for purchase as it has the same design.
review275031002 updated at https://codereview.tryton.org/275031002/#ps267011003
msg56344 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2020-03-17.20:17:56
Here is review275031002 which fixes the issue for me by using compute_unit_price instead of on_change_product

The only drawback is that this method was introduced on 5.4 series so it can not be backported to 5.2 nor 5.0. 

We should decide what to do with this versions.
msg56343 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2020-03-17.20:09:54
I've debuged the code and the problem is that product instance does not have the proper company set on the context so the customer_taxes returns an empty list and this causes that the existing taxes are updated. 

Resetting the product on the lines gives the taxes back.

I'm wondering if it won't be better to just call compute_unit_price to just update the unit price. AFAIU unit_price is the only value that should be updated.
msg56320 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2020-03-17.18:50:50
There is no way to know if the price was edited or not. So the re-computation of the price should always happen. This could probably be better documented but this is for another issue.
Here the lost of taxes is weird because the wizard is only calling on_change_product so it means that with the new header the tax setup is different. Could you check that after the header modification, resetting the product set or not the taxes.
msg56166 (view) Author: [hidden] (edbo) Date: 2020-03-09.22:40:42
> The lines are updated because you can change the party so price list and taxes should be recomputed.
> Your unit_price change is the expected behaviour?

No, the expected behaviour is that the unit_price stays the same, because I changed it myself.

So the wizard needs to check which fields are changed and act accordingly. When no party is changed, do not update the unit_price. Also some extra checks can be done when the party is changed and the previous unit_price was manually added, it should stay. Otherwise it can be changed.
msg56165 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2020-03-09.21:15:40
The lines are updated because you can change the party so price list and taxes should be recomputed.

Your unit_price change is the expected behaviour? My problem is that taxes are the right ones for the new customer but they are lost.
msg56163 (view) Author: [hidden] (edbo) Date: 2020-03-09.20:55:54
I also came across this bug. It's even worse. When you change the 'unit_price' in the line form, it will be changed back to the 'list_price' of the product.

Because the wizard is about changing the "header", why are the sale lines involved in this? Is about VAT update?

Also, what I find weird is that the wizard is not available in state 'quotation'. Because most of the time you make changes to your quotation before the sale becomes order. But that's a different issue I think.
msg56029 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2020-03-03.13:57:12
Steps to reproduce:

1. Create a sale for any party
2. Add a line with a product that has an accouting category with customer taxes set.
3. Click the "Modify Header" button and the Modify Button of the opened wizard.

After the wizard is closed the taxes are removed from the product. 

I will expect that the taxes are set as they come from the product data. 

I've found it on 5.4 series but I imagine that trunk is also afected.
History
Date User Action Args
2020-04-25 09:20:07roundup-botsetmessages: + msg57463
2020-04-25 09:19:56roundup-botsetmessages: + msg57462
2020-04-25 09:19:46roundup-botsetmessages: + msg57461
2020-04-25 09:19:37roundup-botsetmessages: + msg57460
2020-04-25 09:19:32roundup-botsetmessages: + msg57459
2020-04-25 09:19:28roundup-botsetstatus: testing -> resolved
nosy: + roundup-bot
messages: + msg57458
2020-04-18 18:31:40pokolisetmessages: + msg57277
2020-04-17 17:18:04cedsetsuperseder: + Add contextual fields in on_change arguments
messages: + msg57236
2020-04-15 17:41:48reviewbotsetmessages: + msg57181
2020-04-15 17:20:36pokolisetmessages: + msg57180

Showing 10 items. Show all history (warning: this could be VERY long)