-
-
Notifications
You must be signed in to change notification settings - Fork 233
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
[16.0][IMP] stock_average_daily_sale: support multi-warehouses #336
[16.0][IMP] stock_average_daily_sale: support multi-warehouses #336
Conversation
twalter-c2c
commented
Aug 28, 2024
•
edited
Loading
edited
- Dependencies to "abc profile" has been removed as consumptions are per warehouse, independently of the many profile you could have on a warehouse.
- Major changes in the configuration: added company field, added form view, allowed excluding or not the weekends for calculating values of the report.
- Changes in the query (which calculates report values) logic: exclude weekends only if this option is chosen in the config, include production location as destination location.
31bd2c0
to
21524fd
Compare
@jbaudoux ciaoooooooo ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some remarks
stock_average_daily_sale/models/stock_average_daily_sale_config.py
Outdated
Show resolved
Hide resolved
stock_average_daily_sale/models/stock_average_daily_sale_config.py
Outdated
Show resolved
Hide resolved
stock_average_daily_sale/models/stock_average_daily_sale_config.py
Outdated
Show resolved
Hide resolved
stock_average_daily_sale/models/stock_average_daily_sale_config.py
Outdated
Show resolved
Hide resolved
Can you also prefix your commits first line message by the module name |
f023e26
to
a483927
Compare
a483927
to
b432934
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks.
- Can you reword your commits to prefix first line message by the module name? stock_average_daily_sale: ...
- Can you add the unique constraint on the config?
Thank you for the review. Yes, I was just about to add the prefix to commit messages, forgot about it while pushing the last changes. I will also add a constraint then, thank you for the suggestion. |
Remove product_abc_classification dependency. This module is not essential for the current module to function properly.
Only stock_user can see config and report; stock_manager can create and delete config entries
Set default average_daily_sale_root_location on WH create. Default stock location of the new WH is used for this purpose
Allow to decide if weekends should be included or excluded in average daily sale calculation.
Take also production location as dest location for daily usage calculation
b432934
to
77152f7
Compare
stock_average_daily_sale/models/stock_average_daily_sale_config.py
Outdated
Show resolved
Hide resolved
…ny_id stock_average_daily_sale: - add form view for config - add company_id to config - allow editing ABC classification level and make sure the level is unique per warehouse
77152f7
to
448443f
Compare
6abf060
to
9f066bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technical review only
@rousseldenis good for you? |
/ocabot merge nobump |
This PR looks fantastic, let's merge it! |
@simahawk your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-336-by-simahawk-bump-nobump. 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. |
/ocabot merge nobump |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at 3e88cbf. Thanks a lot for contributing to OCA. ❤️ |