[17.0][MIG] product_company_default: Migration to 17.0

[peluko00]

Module migrated to version 17.0

cc https://github.com/APSL 163062
@miquelalzanillas @javierobcn @mpascuall @BernatObrador @ppyczko please review

• [Issue] Migration to version 17.0 #1446

[BernatObrador]

LGTM, tested in runboat

[rousseldenis]

/ocabot migration product_company_default

[rousseldenis]

@AungKoKoLin1997

[rousseldenis]

This is not what the module does. Is it normal ?

Moreover, using the config parameter with a company that is not enabled on user that creates the product can leads to UX problems.

[yostashiro]

I think the current description is not accurate. Should be "the current active one" instead of "user's company."

Moreover, using the config parameter with a company that is not enabled on user that creates the product can leads to UX problems.

@rousseldenis I'm not getting this comment. The config parameter doesn't depend on the company.

[yostashiro]

(Aung Ko Ko Lin is on leave for a while.)

[yostashiro]

I think the current description is not accurate. Should be "the current active one" instead of "user's company."

Moreover, using the config parameter with a company that is not enabled on user that creates the product can leads to UX problems.

@rousseldenis I'm not getting this comment. The config parameter doesn't depend on the company.

[rousseldenis]

@rousseldenis I'm not getting this comment. The config parameter doesn't depend on the company.

Of course. But:

• Configure the default company to 1
• The user is defined with company 2 and only that one.
• The user try to create a product.

What will happen as this will set the company 1 for this new product ?

[yostashiro]

@rousseldenis The config param just enables the feature. It's not setting the default company.

[rousseldenis]

@rousseldenis The config param just enables the feature. It's not setting the default company.

Ooops, I'm tired 😅

[pedrobaeza]

There's no sense in having a global config parameter for disabling the feature. If you don't want the feature, just don't install the module.

If it's a trick for testing together with other modules, just do something like:

    company_id = fields.Many2one(default=lambda self: self._default_company_id())

    @api.model
    def _default_company_id(self):
        context = self.env.context
        if (
            config["test_enable"]
            and not context.get("test_product_company_default")
        ):
            return False
        return self.env.company

[rousseldenis]

There's no sense in having a global config parameter for disabling the feature. If you don't want the feature, just don't install the module.

If it's a trick for testing together with other modules, just do something like:

    company_id = fields.Many2one(default=lambda self: self._default_company_id())

    @api.model
    def _default_company_id(self):
        context = self.env.context
        if (
            config["test_enable"]
            and not context.get("test_product_company_default")
        ):
            return False
        return self.env.company

Don"t agree at all. It's anti-programatic to rely on tests in business code.

[pedrobaeza]

That's your opinion, and I already know it, but it's just a line and avoid incompatibilities in the tests that lead to split CIs. And more in this case, where it's not a module incompatibility. Just test issues due to incorrect company in the products created for testing purposes.

Anyway, I was just doing a hypothesis for that config parameter, which I don't find sense if not for this goal.

[rousseldenis]

That's your opinion, and I already know it, but it's just a line and avoid incompatibilities in the tests that lead to split CIs. And more in this case, where it's not a module incompatibility. Just test issues due to incorrect company in the products created for testing purposes.

It's not just about tests.

It decouples also the module deployment and feature activation. That makes sense.

[pedrobaeza]

I don't agree on that. You can deploy the module, but don't install it. You have the same control changing the system parameter and installing the module. Both can be UI activations.

[yostashiro]

Putting the debate on the validity of the config param (and the minor suggestion on the README) aside, the PR looks fine as far as the migration is concerned.

[sebalix]

LG.

About the pro/con of system parameter it's not blocking the migration here.

[sebalix]

Should we make use of odoo.tools.str2bool to support other trui-sh values?

[pedrobaeza]

I think the debate about the technique is still relevant, as you prefer to add ir_config.parameter data, plus code to read it (with the str2bool possible problem) + demo data to deactivate the parameter for tests using demo data... vs adding 3 lines of code. I don't think that's better. And even Odoo is planning to deactivate demo data in master.

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