[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

[Wodran14]

Any updates on this module?