[18.0][MIG]contract_sale_generation: Migration to 18.0

[javierjcf]

Depends on:

• [18.0][MIG] contract_sale #1138

Migration from v17 with changes of this PR #1234 over v17.

[pedrobaeza]

It seems this is bringing more changes apart from the module contract_sale_generation. Have you done the git am properly?

[javierjcf]

I did it only to take commits of contract_sale_generation from my PR branch:
git format-patch --keep-subject --stdout origin/18.0..Comunitea/17.0-mig-contract_sale_generation -- contract_sale_generation | git am -3 --keep

But then i realize i need contract_sale dependency on PR #1138... and i did only:

git remote add acsone https://github.com/acsone/contract.git
git fetch acsone
git rebase acsone/18.0-mig_contract_sale-sbj

I guess I should also run format-patch against the branch from PR #1138, and only apply the commits from the contract_sale module?
Is the order important in this case?

I'm more used to migrating modules that already exist in the official OCA branch.
In this case, since I need to combine two unmerged PRs, I'm not entirely sure about the correct order in which to apply them.

[pedrobaeza]

For a non merged dependency, check https://github.com/OCA/maintainer-tools/wiki/Use-temporary-reference%28s%29-to-another-pull-request%28s%29

[javierjcf]

Hi @pedrobaeza

One question that comes to mind after reviewing the procedure is:

Since this is for v18, my understanding is that by simply editing the pyproject.toml file within the module, I'm telling the test environment that it needs that module, which means this PR can be independent.

What confuses me is having a local branch that is missing the dependency (which it would actually need in order to be installed), but then not committing anything other than the module being migrated.

In summary, my final question is: when a PR has a dependency on one or more other PRs, should the code from those dependencies be included in this PR's branch or not?.

It seems it should be independient, but i'm not sure at all.

[pedrobaeza]

OCA CI works with pip installable modules, so putting that test_requirements.txt, activate a pip installation over a GitHub pull request, not requiring the files in the traditional way.

[javierjcf]

Hi @pedrobaeza

To avoid close the PR i try to fix the branch with: git reset --hard origin/18.0 and doing the git am from my PR #1234 branch.
Then i followed the note about dependencies.

I hope is well done now.

[pedrobaeza]

Yes, excellent. For a veteran like you, these are peanuts, hehe!

/ocabot migration contract_sale_generation

[rousseldenis]

Code review

[AtaponDew]

It looks good to me, approve.

[javierjcf]

Hello,

I applied the corrections from @acsonefho and removed the test-requirements.txt (since contract_sale has already been merged). I did a rebase, and now the failure is happening in the sale_project module — I think because the project is not being propagated correctly.

What’s curious is that on a demo database, with this module and sale_project installed, the tests actually pass.

I thought something might be missing because I removed the check for the analytic_account_id field in this module’s test (it no longer exists, I believe it’s now handled with analytic_distribution). But this module doesn’t actually interact with the project or the analytic account…

The fact that the tests pass on a demo database is confusing me.

Any ideas?

[NICO-SOLUTIONS]

Just wanted to ask whats the status here?
@javierjcf
After a quick look, it seems that the test test_contract_autoconfirm fails because Odoo’s sale_project module is triggered when confirming the Sale Order. Since the product uses service tracking, sale_project requires a project to create the task – but no project is provided in the test data.

[javierjcf]

Just wanted to ask whats the status here? @javierjcf After a quick look, it seems that the test test_contract_autoconfirm fails because Odoo’s sale_project module is triggered when confirming the Sale Order. Since the product uses service tracking, sale_project requires a project to create the task – but no project is provided in the test data.

I'm using this module in production, it works as expected, and in my local enviroment, with sale_project installed (clean db, but no with all contract modules installed) there is no error in the tests...

I dont know if the problem belongs to this module (maybe not doing well with the analytic account/distribution) or is in the oca CI demo data...

[NICO-SOLUTIONS]

Just wanted to ask whats the status here? @javierjcf After a quick look, it seems that the test test_contract_autoconfirm fails because Odoo’s sale_project module is triggered when confirming the Sale Order. Since the product uses service tracking, sale_project requires a project to create the task – but no project is provided in the test data.

I'm using this module in production, it works as expected, and in my local enviroment, with sale_project installed (clean db, but no with all contract modules installed) there is no error in the tests...

I dont know if the problem belongs to this module (maybe not doing well with the analytic account/distribution) or is in the oca CI demo data...

@javierjcf
Haven’t tested it in detail yet... For me, (for now) it looks like the CI tests fail because the demo/test data do not provide a project for the service-tracked product in the test. The issue affects only test/demo data; in production, with full data, it is possible that module works correctly.
I guess adding a project in the test method or setUp() should fix it, which should be the goal here.

[javierjcf]

I finally managed to reproduce the error locally. Until now, I was only installing contract_sale_generation and sale_project on a new database (with demo data), and there was no error.
I’m suspecting the sale_timesheet module, because it overwrites the demo product ID that fails:

<record id="product.product_product_1" model="product.product">
    <field name="service_type">timesheet</field>
    <field name="service_tracking">task_global_project</field>
</record>

If i install also sale_timesheet then i got the same error as in OCA CI

I don’t have much control over the new analytic accounting part (now it’s a widget with JSON format), but I suspect that in the tests I’m not doing anything with it, and maybe I should, since the analytic account (now analytic distribution) used to be linked to the projects.

I’d also like to ask if anyone knows the best way to reproduce OCA’s CI locally… or at least to know which modules are usually installed. It seems like all Odoo addons and all the modules from the given repository. Are the rest of the modules from other repositories included as well?

I also have another question: why do many modules use generic demo data, which may not be installed, instead of declaring their own demo data?

For example, this tests make a mixure of own demo data and odoo demo data

cls.pricelist = cls.env["product.pricelist"].create(
            {"name": "pricelist for contract test"}
        )
        cls.partner = cls.env["res.partner"].create(
            {
                "name": "partner test contract",
                "property_product_pricelist": cls.pricelist.id,
                "email": "demo@demo.com",
            }
        )
        cls.product_1 = cls.env.ref("product.product_product_1")

Pricelist and partner are own demo data, but not the product...

In a production database I can’t run these tests, because they require Odoo’s demo data with id=product_product_1. But if the product to test were generated as part of the module’s own test data, and not Odoo’s demo data, then the test could be run on any database. I suppose there’s some reason it’s done this way, but I can’t figure it out.

[javierjcf]

When testing on a demo database, even after adding the analytic distribution and the group_id field (which would be the analytic account), the error still occurs:

The thing is, in my opinion, a generic demo product (product.product_product_1) should not be overwritten

If the sale_timesheet module is not installed, the demo product “Virtual Interior Design” remains normal. But with this module, it’s marked as needing to generate a task, yet no project is associated with it. (You can do it at product level or sale level)

I’m not sure if it’s the responsibility of the demo data in this test to provide a project. If that’s the case, then it should be with the analytic_distribution, right? I tried adding the field to the line, but no luck.

An improvement for me, from my inexperience with tests, would be to use a demo product specific to the module, not Odoo’s generic product_product_1, which several modules overwrite (l10n_in, point_of_sale, sale, and sale_timesheet).

I modified the demo product from product.product_product_1 to product.product_product_2, and as I suspected, the tests now pass. but i'm confused about the use of demo data to run tests.

I don’t have much experience developing tests, so maybe I’m missing something… but wouldn’t it be better to never use generic demo data (since it seems they can be overwritten by other modules, making them inconsistent), and instead have each module be responsible for defining its own demo data?

[NICO-SOLUTIONS]

I might be wrong, but I think it’s safer not to rely on generic demo data, since other modules can change it. Creating the needed records directly in the test seems more reliable and isolated to me — isn’t that the right approach?

[javierjcf]

Hi @acsonefho , I did the requested changes, and solve the test error. I think this is ready to merge?

[acsonefho]

Thank you @javierjcf for your contribution and this good work! 🙏 I hope it'll be merge quickly 💪

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

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

[javierjcf]

There was an issue with the analytic account test in v17. In v18, the account_analytic_id field no longer exists in sale.order.

The group_id field set in the contract test data (which represents the analytic account) was lost due to the use of Form() when creating lines. (See discussion in #1234) However, it can be recovered so that the test—which verifies that generated sales orders carry that analytic account—matches.

In v18, however, the analytic account field was removed from the header of sale order an the code seems does nothing with this group_id field, so i removed this check from the test in this versión.