[17.0][MIG] contract_sale_generation: Migration to 17.0

[javierjcf]

This PR migrates the module contract_sale_generation from 16.0 to 17.0.
Superseed #1087.

[javierjcf]

Hi,

I dont understand new codecov errors. It seems like is detecting lines of code not tested, but in my migration commit i only edit xml files. Is a simple migration... why is failing coverage if i did not add any python line.

Is mandatory to get this coverage in order to merge this PR?

[rousseldenis]

I dont understand new codecov errors. It seems like is detecting lines of code not tested, but in my migration commit i only edit xml files. Is a simple migration... why is failing coverage if i did not add any python line.

@javierjcf As for now (I'm not fond of :-) ), we start from blank base new branches. So, the module you migrate is not present. Coverage will do the full change tests. If you look at your PR, it adds the whole module and not only migration commit.

[rousseldenis]

/ocabot migration contract_sale_generation

[rousseldenis]

I dont understand new codecov errors. It seems like is detecting lines of code not tested, but in my migration commit i only edit xml files. Is a simple migration... why is failing coverage if i did not add any python line.

@javierjcf As for now (I'm not fond of :-) ), we start from blank base new branches. So, the module you migrate is not present. Coverage will do the full change tests. If you look at your PR, it adds the whole module and not only migration commit.

@javierjcf It seems tests are not running at all. Could you check locally (it can maybe be the accounting modules that causes problem)

[javierjcf]

Thanks @rousseldenis

Yes, tests weren't working, there are references in the test cases that are no longer valid. I will try to fix them throughout the week.

[javierjcf]

Hi, I need help with a test error.

AssertionError: can't write on invisible field 'quantity'

It doesn't occur in my local environment with a fresh database. However, I did face the same error when accessing contract_line_ids. That field was indeed invisible, and I had to explicitly set contract_form.line_recurrence = True. With that, the tests passed.

In CI, the error is triggered by the quantity field. These fields don't have any invisibility modifiers, except for the parent group, which depends on display_type. But if I add display_type to the test to make quantity writable, I get an error saying that display_type is invisible (which is true).

Could another module be interfering? In my local environment I only have contract_sale_generation and its dependencies installed, not the entire OCA...

I see that if I install the rest of the modules from this repository, I also get the same error locally.
Is it expected that if another module adds an invisibility modifier to a field, or to the part of the view where a field is located, it can cause the tests to fail?

Am I supposed to always run the tests with the other modules in the repository? Or with the whole OCA?

[javierjcf]

I found that module contract_variable_qty is doing:

<field name="quantity" position="attributes">
    <attribute name="invisible">"qty_type != 'fixed'"</attribute>
</field>

I don't think the solution is to worry about fields introduced by other modules, especially when the default is correctly set...
Even if I'm explicit by doing:

line_form.qty_type = 'fixed'

It still fails the same way.

How can I make the quantity field always visible from the test?

[javierjcf]

I was trying to understand why the field is invisible in the test, but it doesn’t seem to be.

The invisibility condition is set to False, yet the error is still being raised. How is this possible?

I was debugging and i see this:

The expresion '(display_type) or ("qty_type != \'fixed\'")' seems wrong. Its fault of this:

       <xpath expr="//field[@name='quantity']" position="attributes">
                <attribute name="required">"qty_type == 'fixed'"</attribute>
                <attribute name="invisible">"qty_type != 'fixed'"</attribute>
            </xpath>

Double quote makes not sense in the tag.

I will make a PR to fix that bug.

[javierjcf]

I tested in local with #1242 and test passed. To pass the test here we need #1243 i think

[ofonseca-pyming]

LGTM

[rousseldenis]

@javierjcf Don't do these changes. They are not needed. Thanks

[rousseldenis]

@javierjcf Could you check tests?

[javierjcf]

@javierjcf Could you check tests?

"I tried, but I don’t understand what’s failing (it was working before; only the runbiat part was wrong).

Analytic accounting has been tricky for me since the v16 changes. In any case, the prepare_sale function propagates the analytic account to the orders (which is now called group_id in the contract). I noticed it’s possible it isn't entering this method because it lacks a date_ref. I’ve updated the test to call the private version of the method, _recurring_create_sale, so I can pass the date_ref and ensure prepare_sale isn't skipped.

Even so, it still throws the same error.

In the v18 PR, I removed the line that checks for that—not a great solution. However, in a private project using the v18 module, the account is propagating correctly. I’m missing something regarding how the test is designed, but I don't have time this week to look into it in depth. If anyone knows why this is happening, I can change it immediately.

Otherwise, I’ll try to review it when I have more time available."

[rrebollo]

@javierjcf Please avoid using the "remove conflicting call" approach. I recommend reverting it in the v18 PR as well. I'm currently working on a migration that requires this addition. I'll investigate the failing test and share my findings here once I have them.

[javierjcf]

@rrebollo I tried to revert, but in v18 is a key errror. Not analytic_account_id on sale.order

I'm not sure about analytic plan/distribution and the right way to check it. I think test shoud new check analitic_distribution json in lines instead of sale.order level?? i'm not sure...

In v17 we still have a general analytic_account_id on sale.order, but makes not sense to me. In v18 this field is removed.

If we found a solution heare in v17 i will fix 18 PR as well

[javierjcf]

@rrebollo

I think I found the bug. The analytic account is being set at the contract header level in the common.py file. However, since the lines are created using Form() and group_id is a computed field, the value is lost. Furthermore, the lines in v17 do not have the analytic distribution in the view; perhaps it should be added...

Anyway, since I know the group_id field can be lost, I am re-setting it after using Form() to create the lines. (I don't understand why the test data setup mixes direct value assignment in cls with line creation using Form()).

This way, cls.contract preserves the analytic account that was initially established:

contract/contract_sale_generation/tests/common.py

Line 134 in d1475c0

cls.contract.group_id = cls.analytic_account.id

I don't use v17 or the analytic module myself, so I'm not entirely sure if this is the correct way to test it. It seems more like the new analytic_distribution widget should be at the line level (and visible too, otherwise we can't use Form() to assign a value to it) and then verify that the contract lines have the same analytic distribution as the generated sales order lines.

In v18, I think it will have to be this way since the analytic account field doesn't even exist at the sale.order level anymore.

[rrebollo]

@javierjcf To validate your approach—both for the tests and the overall migration—I recommend looking at merged addons in v17.0 branches of OCA repositories related to analytic workflow. A focused code search should be sufficient.

You can then compare their implementation with your approach. Thank you for all the effort you're putting into this. I haven't had a chance to review it today.

[javierjcf]

Hi @rrebollo, the functionality intended for this test (propagating the analytic account from the contract to the sale) is already being tested correctly. Regarding the analytic distribution at the contract line level, that logic is in the 'contract' module, which has already been merged. The base module handles this part correctly, although it doesn't seem to be tested at the line level.

Functionally, this module seems correct; the only issue was the test. While the test could be improved, I think it's fine as it is for the migration PR.

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖