[REF] sale_order_type: Split off stock dependent part

[hbrunn]

as sale_order_type can also be very useful in contexts where we don't use stock, I propose to split the module into a base part without stock and one that autoinstalls when you have stock. This should also make upgrades for current users work smoothly.

[legalsylvain]

design seems ok, but such refactoring is it allowed on stable ? you are moving data from an installed module to a uninstalled one. people that don't have the new module available on the system will lose the data on production. Don't you think ?

[hbrunn]

Odoo only drops columns after all upgrades are done, and by that time the _stock module is already installed and has registered the same column names. You can try with -u sale_order_type after pulling this change.

[legalsylvain]

"and by that time the _stock module is already installed" : that's the point : maybe not.

I mean, if you install modules by git clone you'll have the new glue module available in the same time and so it will be installed by dependency, but if you installed original module by other way (by downloading on appstore, or by pip), the new module will not be available, letting production servers in a unstable state.
I personally deploy by git (gitaggregate) but maybe @sbidoul could confirm (or not) what I'm saying, regarding pip installation.

[sbidoul]

You can help ensure the glue module is available in addons path for pip users, by declaring it in the install_requires key of of the main module' setup.py like this:

import setuptools

setuptools.setup(
    setup_requires=['setuptools-odoo'],
    odoo_addon=True,
    install_requires=[
        "odoo13-addon-sale_order_type_stock",
    ],
)

This way, when users pip install odoo13-addon-sale_order_type they also automatically get the glue module (in addition to other dependencies declared in the manifest, which are injected by setuptools-odoo.

Otherwise I'm personally fine with such changes in OCA as we have no "stable policy" and we rather try to follow semver.

I'd bump the major version of sale_order_type though and it might be worth a HISTORY.rst entry.

[hbrunn]

thanks @sbidoul, I'll add that. This will however cause pip users to always get the second module, not only when updating from the pre split version, right? As I don't use it myself I'm fine with that, but if there's a simple way to add this in case we're updating, and not if not, I'll be happy to implement that too. Or don't we care this in installed on the python level, but not by Odoo? Then there's nothing to do. History is a good one, will add that too.

[sbidoul]

This will however cause pip users to always get the second module, not only when updating from the pre split version, right?

No, since the setup.py update will be merged together with the split ? Not sure I understand what you mean.

[hbrunn]

if I say pip install sale_order_type with your proposed change, I'll always also get sale_order_type_stock on the filesystem, right? For new installations we don't need this (but it also doesn't hurt), that's why I asked. But well, it's a non-issue basically and I'll just add what you said.

[JakubWiselka]

Hi @hbrunn, could you please update your branch as there are new conflicts. Thanks in advance!

[hbrunn]

there were some serious changes, I'll need to check how to integrate them with this PR

[hbrunn]

@sbidoul thanks for your input, did as you proposed in the end

@JakubWiselka done with rebasing, please review critically to be sure I didn't miss any of the improvements

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[rafaelbn]

Same need #2679

[pedrobaeza]

Why this change?

[hbrunn]

made it simpler to test with a db with multiple companies, but I don't mind reverting this

[yajo]

Looks good, although I have a couple of questions.

[yajo]

I find it weird that you're removing the check_company=True across fields like this one.

I guess it was doing nothing because this model doesn't have a company_id field, but in that case it seems more sensible to add that field, don't you think?

[hbrunn]

I find that weird too, don't remember my reasoning back then if there was any, will re-add

[yajo]

I think this should be named sale_stock_order_type, as its core base module is sale_stock. However it's a minor detail, non-blocking.

[hbrunn]

hmm, I see it as a derivate of sale_order_type, and also envision possible other addons like sale_order_type_ecommerce or similar. What do other reviewers think?

[pedrobaeza]

OK about sale_stock_order_type.

[hbrunn]

but then this doesn't even show up when I search for sale_order_type, and they also don't sort together. But if you two insist and nobody else takes my side, will do

[yajo]

You can add a CONTEXT.rst file in sale_order_type to point to this other module. That will help no matter what's the final name.

In any case, I was just suggesting. If you feel more comfortable with the current name, for me it's OK too. 😉

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.