I can confirm that issue9236 makes the invoice module test pass.

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.

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.

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.

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

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.

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.

Also sale_subscription service and account_asset product.

The product on invoice should also have a company context set.

Indeed it requires issue9187 to work correctly.

I've updated review275031002 with what is descrived on msg56647 but this does not seems to fix the issue.

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

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

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.

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.

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.

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.

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

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.

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.

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.