[14.0][IMP] product_state: move access to Sales + Inventory Manager

[aleuffre]

#1121

[OCA-git-bot]

Hi @emagdalenaC2i,
some modules you are maintaining are being modified, check this out!

[aleuffre]

Had to modify the tests of a dependent module, as a result.

[lfreeke]

Functional test 👍

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

[aleuffre]

@OCA/product-maintainers sorry for the ping, but could you please merge this?

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

[dreispt]

/ocabot merge patch

[OCA-git-bot]

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-1124-by-dreispt-bump-patch, awaiting test results.

[OCA-git-bot]

@dreispt your merge command was aborted due to failed check(s), which you can inspect on this commit of 14.0-ocabot-merge-pr-1124-by-dreispt-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

[dreispt]

Shouldn't this access be given to the Inventory Manager, rather than the Sales Manager?

[Kev-Roche]

Shouldn't this access be given to the Inventory Manager, rather than the Sales Manager?

Indeed, it's a good question. Maybe the answer is both because some products with service type are not in stock and some projects are more focused on sale than stock (and opposite).

[aleuffre]

Shouldn't this access be given to the Inventory Manager, rather than the Sales Manager?

Indeed, it's a good question. Maybe the answer is both because some products with service type are not in stock and some projects are more focused on sale than stock (and opposite).

That was my reasoning as well. I've added both.

[legalsylvain]

/ocabot merge patch

thanks for your contribution. Feel free to port this patch upstream, if necessary.

regards.

[OCA-git-bot]

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-1124-by-legalsylvain-bump-patch, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at b377db7. Thanks a lot for contributing to OCA. ❤️

[natuan9]

Hi @aleuffre , I noticed you moved access to Inventory Manager but didn’t add a dependency on the stock module. Doesn’t this cause any issues? I also see that the migration PRs don’t mention this either.
Currently, I’m migrating this module to version 18.0, and I’m uncertain whether I should add a dependency on the stock module. Could you clarify this for me?

[aleuffre]

Hi @aleuffre , I noticed you moved access to Inventory Manager but didn’t add a dependency on the stock module. Doesn’t this cause any issues? I also see that the migration PRs don’t mention this either. Currently, I’m migrating this module to version 18.0, and I’m uncertain whether I should add a dependency on the stock module. Could you clarify this for me?

I think you're correct. I thought there was an indirect dependency based on the module's existing dependencies, but it seems I was wrong (or maybe I just missed it at the time, I don't remember after almost a year). You should definitely add this fix when migrating. Thank you!

[simahawk]

Guys, this change is breaking the module. The dependency on stock has not been declared.
EDIT: I've just seen the msg by @natuan9, sorry for the repetition :)

Sounds like it was never tested in isolation (= install only this module).
Also is arguable that we have to add a full dep on stock just to use its group.
IMO it should be done like this:

• add a specific group in this module
• add an auto-installable module that depends on stock that add this group as implied on inv manager

BTW even the dep on sale could be ripped out as it's needed only for the menu...

[simahawk]

Hi @aleuffre , I noticed you moved access to Inventory Manager but didn’t add a dependency on the stock module. Doesn’t this cause any issues? I also see that the migration PRs don’t mention this either. Currently, I’m migrating this module to version 18.0, and I’m uncertain whether I should add a dependency on the stock module. Could you clarify this for me?

I think you're correct. I thought there was an indirect dependency based on the module's existing dependencies, but it seems I was wrong (or maybe I just missed it at the time, I don't remember after almost a year). You should definitely add this fix when migrating. Thank you!

I would proceed the other way around. We remove the need for the dep in v18 and you can backport the fix here 😉

[aleuffre]

Apologies again for the mistake on my part.

I don't have a strong opinion on how to fix it either way, in fact I like your solution best @simahawk , but I've definitely seen small modules blocked to avoid multiplying the number of modules in OCA...