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.

Sign up for GitHub

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

Jump to bottom

[16.0][IMP] stock_average_daily_sale: support multi-warehouses #336

Merged
merged 8 commits into from
Sep 24, 2024
Merged

[16.0][IMP] stock_average_daily_sale: support multi-warehouses #336

merged 8 commits into from
Sep 24, 2024

Conversation

twalter-c2c
Copy link
Contributor

@twalter-c2c twalter-c2c commented Aug 28, 2024 •
edited
Loading

  1. Dependencies to "abc profile" has been removed as consumptions are per warehouse, independently of the many profile you could have on a warehouse.
  2. Major changes in the configuration: added company field, added form view, allowed excluding or not the weekends for calculating values of the report.
  3. 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.

@twalter-c2c twalter-c2c force-pushed the 16.0-stock_average_daily_sale branch 3 times, most recently from 31bd2c0 to 21524fd Compare August 28, 2024 11:53
@simahawk
Copy link

@jbaudoux ciaoooooooo ping

jbaudoux
jbaudoux requested changes Sep 3, 2024
View reviewed changes
Copy link
Contributor

@jbaudoux jbaudoux left a 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/__manifest__.py Outdated Show resolved Hide resolved
stock_average_daily_sale/data/ir_cron.xml Outdated Show resolved Hide resolved
stock_average_daily_sale/models/stock_average_daily_sale.py Outdated Show resolved Hide resolved
stock_average_daily_sale/models/stock_average_daily_sale_config.py 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
stock_average_daily_sale/models/stock_average_daily_sale_config.py Outdated Show resolved Hide resolved
stock_average_daily_sale/static/description/index.html Outdated Show resolved Hide resolved
stock_average_daily_sale/views/stock_average_daily_sale.xml Outdated Show resolved Hide resolved
@jbaudoux jbaudoux changed the title [IMP] Modify config, remove dependencies, fix WH root location [16.0][IMP] stock_average_daily_sale: support multi-warehouses Sep 3, 2024
@jbaudoux
Copy link
Contributor

jbaudoux commented Sep 3, 2024

Can you also prefix your commits first line message by the module name stock_average_daily_sale: ...

@jbaudoux
Copy link
Contributor

jbaudoux commented Sep 3, 2024

cc @rousseldenis

@rousseldenis rousseldenis added this to the 16.0 milestone Sep 3, 2024
@twalter-c2c twalter-c2c force-pushed the 16.0-stock_average_daily_sale branch 3 times, most recently from f023e26 to a483927 Compare September 6, 2024 09:12
jbaudoux
jbaudoux reviewed Sep 12, 2024
View reviewed changes
Copy link
Contributor

@jbaudoux jbaudoux left a 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?

@twalter-c2c
Copy link
Contributor Author

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.

@twalter-c2c
stock_average_daily_sale: Remove product_abc_classification dependency
00f35aa 
Remove product_abc_classification dependency.
This module is not essential for the current module to function properly.
@twalter-c2c
stock_average_daily_sale: Change default access rights
e966ad9 
Only stock_user can see config and report; stock_manager can create and delete config entries
@twalter-c2c
stock_average_daily_sale: Set default root location on WH
7e36ea6 
Set default average_daily_sale_root_location on WH create.
Default stock location of the new WH is used for this purpose
@twalter-c2c
stock_average_daily_sale: Allow to include/exclude weekends
693370b 
Allow to decide if weekends should be included or excluded
in average daily sale calculation.
@twalter-c2c
stock_average_daily_sale: Take production location into account
296bc61 
Take also production location as dest location for daily usage calculation
@twalter-c2c
stock_average_daily_sale: Config adjustment - add from view and compa…
448443f 
…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
henrybackman
henrybackman approved these changes Sep 13, 2024
View reviewed changes
Copy link

@henrybackman henrybackman left a comment

Choose a reason for hiding this comment

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

Technical review only

@simahawk
Copy link

@rousseldenis good for you?

@simahawk
Copy link

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-336-by-simahawk-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Sep 23, 2024
@OCA-git-bot
Merge PR #336 into 16.0
5a10b94 
Signed-off-by simahawk
@OCA-git-bot
Copy link
Contributor

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

@sebalix
Copy link

sebalix commented Sep 24, 2024

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-336-by-sebalix-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 401eddd into OCA:16.0 Sep 24, 2024
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@twalter-c2c twalter-c2c mentioned this pull request Sep 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbaudoux jbaudoux jbaudoux approved these changes

@henrybackman henrybackman henrybackman approved these changes

@rousseldenis rousseldenis rousseldenis approved these changes

Assignees
No one assigned
Labels
approved enhancement merged 🎉 needs review
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
7 participants
@twalter-c2c @simahawk @jbaudoux @OCA-git-bot @sebalix @henrybackman @rousseldenis