Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MIG] stock_operating_unit: Migration to 17.0 #673

Merged
merged 67 commits into from
Aug 5, 2024

Conversation

jdidderen-nsi
Copy link
Contributor

@jdidderen-nsi jdidderen-nsi commented May 29, 2024

@AaronHForgeFlow
Copy link
Contributor

/ocabot migration stock_operating_unit

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone May 29, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request May 29, 2024
16 tasks
@jdidderen-nsi jdidderen-nsi force-pushed the 17.0-mig-stock_operating_unit branch from ac27757 to d50283c Compare May 29, 2024 13:33
@jdidderen-nsi
Copy link
Contributor Author

Open discussion for the constraint on locations : _check_required_operating_unit

In previous versions of the module, the operating unit was mandatory on locations (except customer and supplier ones). In the case of one of my projects, the customer wants to have a "central" warehouse common to all operating units. With the constraint, I have to define an operating unit, even if it's not coherent with the business need. 

Right now, I remove this part of the code from the constraint, but maybe there is another solution. 

What's the opinion on this?

@AaronHForgeFlow
Copy link
Contributor

I am not completely sure, but I think the condition is there mainly for correctly inventory accounting, so to ensure that the inventory value is owned by a specific operating unit. If that is the case maybe we can move the contraint to the stock_account_operating_unit module.

Another solution is to consider using hierarchy in operating units: #319 like if the parent OU represents the company. This approach is more complex because all the contraints definitions should be refactored.

Copy link

@nguyenminhchien nguyenminhchien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take this occasion to squash administrative commits (if any) with the previous commit for reducing commit noise.
Squash your 2 commits with [MIG] stock_operating_unit: Migration to 17.0
image

Thanks,

groups="operating_unit.group_multi_operating_unit"
readonly="state == 'draft'"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readonly when "state != 'draft'"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 👍

@jdidderen-nsi jdidderen-nsi force-pushed the 17.0-mig-stock_operating_unit branch from f28909c to d8322a7 Compare July 8, 2024 19:53
Copy link
Member

@yvaucher yvaucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yvaucher
Copy link
Member

yvaucher commented Aug 5, 2024

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 17.0-ocabot-merge-pr-673-by-yvaucher-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 466d4e1 into OCA:17.0 Aug 5, 2024
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.