[16.0][IMP][REF] stock_average_daily_sale: refactor#403
[16.0][IMP][REF] stock_average_daily_sale: refactor#403OCA-git-bot merged 2 commits intoOCA:16.0from
Conversation
jbaudoux
force-pushed
the
16-stock_average_daily_sale-refactor
branch
12 times, most recently
from
4b01770 to
037c974
Compare
|
Tests are red due to network issue |
|
For review @rousseldenis @twalter-c2c @lmignon |
| <field name="standard_deviation_exclude_factor">3</field> | ||
| <field name="safety_factor">0.3</field> | ||
| <field name="number_days_qty_in_stock">2</field> | ||
| <field name="exclude_weekends">1</field> |
There was a problem hiding this comment.
| <field name="exclude_weekends">1</field> | |
| <field name="exclude_weekends">0</field> |
Maybe we can include weekends in demo data to be consistent with the change merged in v14 #355.
There was a problem hiding this comment.
I don't rely much on demo data anymore in the tests. So doesn't really matter
| <field name="warehouse_id" /> | ||
| <field name="location_id" /> | ||
| <field name="product_id" /> | ||
| <field name="abc_classification_level" /> | ||
| <field name="sale_ok" /> | ||
| <field name="date_from" /> | ||
| <field name="date_to" /> | ||
| </group> | ||
| <group> | ||
| <field name="nbr_sales" /> | ||
| <field name="average_qty_by_sale" /> | ||
| <field name="average_daily_sales_count" /> | ||
| <field name="max_daily_qty" /> | ||
| <field name="average_daily_qty" /> | ||
| <field name="daily_standard_deviation" /> | ||
| <field name="safety" /> | ||
| <field name="recommended_qty" /> |
There was a problem hiding this comment.
I suggested a different order of columns in #401 . Hard to say which one is better. To be discussed.
There was a problem hiding this comment.
Ah thanks for pointing out. I'll add the optional here. Seems a nice addition.
For the order of columns, it's a matter of taste... I tried to be logical. Anyway it's a detail
There was a problem hiding this comment.
I did this
d5add51
based on your suggestion
| SElECT * FROM cfg | ||
| WHERE cfg.abc_classification_level = COALESCE(pt.abc_storage, 'c') | ||
| AND sl_src.parent_path ilike concat(cfg.location_parent_path, '%%') | ||
| AND sl_dest.parent_path not ilike concat(cfg.location_parent_path, '%%') |
There was a problem hiding this comment.
I wonder if we want to take internal transfers into account. Which we do now, am I right?
There was a problem hiding this comment.
Yes it's on purpose to be able to monitor a picking zone instead of stock to customer
twalter-c2c
left a comment
twalter-c2c
left a comment
There was a problem hiding this comment.
Great job! I dropped few minor comments.
- Allow to set a location from which consumption are computed - Properly compute the average daily quantity - Add max daily consumption metric - Remove stock level, this is not relevant for this report - Drop spike exclusion as it was counter-productive the way it was used - Moved safety and recommended quantity as computed field so that it can be extended - Added multi-company rules
jbaudoux
force-pushed
the
16-stock_average_daily_sale-refactor
branch
from
037c974 to
d5add51
Compare
jbaudoux
force-pushed
the
16-stock_average_daily_sale-refactor
branch
from
d5add51 to
08e8abe
Compare
lmignon
left a comment
lmignon
left a comment
There was a problem hiding this comment.
The readability is highly improved in addition to all the improvements. Thank you @jbaudoux . LGTM (Code review).
As a side note, I'm a little alarmed by the deletion of the exclusion of peaks, which was intended to ensure that the average does not take into account “exceptional events”, which should not be taken into account when talking about averages. (we'll see..)
|
This PR has the |
|
/ocabot merge major |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
Congratulations, your PR was merged at 709c973. Thanks a lot for contributing to OCA. ❤️ |
5 participants
performance: more than 1000 lines computed per second