[14.0][IMP] stock_reserve_rule: add full lot strategy #1834

geomer198 · 2023-09-08T23:50:42Z

New removal strategy is added.

aayartsev

Looks good

francesco-ooops

functional ok!

@OCA/logistics-maintainers good for merge?

OCA-git-bot · 2023-09-19T08:57:55Z

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

jbaudoux

Can you clarify what you try to achieve ? What are your real use cases ?

I have the impression that you try to minimize the amount of locations from which you reserve

stock_reserve_rule/models/stock_reserve_rule.py

elvise · 2023-09-19T10:30:34Z

Can you clarify what you try to achieve ? What are your real use cases ?

I have the impression that you try to minimize the amount of locations from which you reserve

Hi @jbaudoux thanks for review!
The final goal is to have a configuration on which lot to reserve.
For example, for some fabrics we only reserve lots that meet the reserved quantity without shipping several lots to reach the required quantity.

stock_reserve_rule/models/stock_reserve_rule.py

jbaudoux · 2023-09-19T11:22:53Z

@OCA/logistics-maintainers good for merge?

Please also drop a review on #1831 :)

stock_reserve_rule/models/stock_reserve_rule.py

elvise · 2023-10-01T14:49:31Z

@francesco-ooops @geomer198 looks tolerance doesn’t work :(

geomer198 · 2023-10-01T21:21:36Z

@francesco-ooops @geomer198 looks tolerance doesn’t work :(

Yes, I only added fields without functional.

stock_reserve_rule/models/stock_reserve_rule.py

elvise · 2023-10-16T19:51:33Z

@jbaudoux please take a look :)

geomer198 · 2023-11-11T17:03:58Z

@geomer198 Could you fix tests and pre-commit ?

Done, fixed it!

francesco-ooops · 2023-11-13T10:04:09Z

@jbaudoux feedback?

francesco-ooops · 2023-11-17T16:49:21Z

@jbaudoux gentle reminder!

jbaudoux

There are still issues to be addressed. Main concern is the group by location which does not seems inline with the intention of the module.
Also use float_compare and make a plain compare method.

geomer198 · 2023-11-18T18:15:19Z

There are still issues to be addressed. Main concern is the group by location which does not seems inline with the intention of the module. Also use float_compare and make a plain compare method.

Could you check my changes? Is now ok?

elvise · 2023-11-21T08:48:34Z

There are still issues to be addressed. Main concern is the group by location which does not seems inline with the intention of the module. Also use float_compare and make a plain compare method.

Could you check my changes? Is now ok?

@jbaudoux gentle reminder :)

jbaudoux · 2023-11-21T09:22:26Z

stock_reserve_rule/models/stock_reserve_rule.py

+            if computation == "percentage":
+                # need + rounding < product_qty <= need * (100 + tolerance) / 100
+                return (
+                    float_compare(need, product_qty, rounding) == -1


Why don't you accept exact value when you give a tolerance? It should be in the limit defined by the tolerance but if the quantity is exactly the need, it should also be valid, isn't it?

Suggested change

float_compare(need, product_qty, rounding) == -1

float_compare(need, product_qty, rounding) <= 0

Some unit tests on _compare_with_tolerance with real figures would be helpful. Just the call to the method without picking.
Tests with inside tolerance, at the lower limit of tolerance, at the upper limit of the tolerance, below tolerance, above tolerance

no tolerance

upper_limit, percentage

upper_limit, absolute

lower_limit, percentage

lower_limit, absolute

@elvise This still needs to be fixed or answered

@francesco-ooops @geomer198 please take care

@jbaudoux Done! Please check tests.

@jbaudoux Done! Please check tests.

@geomer198 can you add tests with decimals (qty)?

@jbaudoux Done! Please check tests.

@geomer198 can you add tests with decimals (qty)?

Yes) Done!

jbaudoux · 2023-11-21T09:44:39Z

stock_reserve_rule/models/stock_reserve_rule.py

+                    need = (
+                        yield fields.first(lot_quants).location_id,
+                        lot_quantity,
+                        need,
+                        lot,
+                        None,
+                    )


You have selected a lot and it can have multiple quants. I think you need to yield each quant one by one by looping on the quants (they should be already sorted by fifo/fefo) decreasing the need until need is 0 or all quants are consumed.
Can you add a test with this?
Example:

Loc1, Lot A, 10 units

Loc2, Lot A, 5 units
Need for 15 units. Lot A is selected

@jbaudoux Thank you for your solution! I updated function implementation. Now the strategy looks better!

stock_reserve_rule/readme/DESCRIPTION.rst

jbaudoux · 2023-11-21T10:05:25Z

There are still issues to be addressed. Main concern is the group by location which does not seems inline with the intention of the module. Also use float_compare and make a plain compare method.

Could you check my changes? Is now ok?

Thanks for the work. The code is so much cleaner, easy to read and simple :)

geomer198 · 2023-11-21T20:05:30Z

@jbaudoux Please check my changes.

elvise · 2023-11-23T08:11:06Z

@jbaudoux gentle reminder :)

elvise · 2023-11-26T10:42:23Z

@jbaudoux gentle reminder :)

jbaudoux · 2023-11-26T22:29:14Z

stock_reserve_rule/models/stock_reserve_rule.py

+                for lot_quant in lot_quants:
+                    product_qty = sum(lot_quant.mapped("quantity"))
+                    lot_quantity = sum(lot_quant.mapped("quantity")) - sum(
+                        lot_quant.mapped("reserved_quantity")
+                    )
+                    if (
+                        lot
+                        and self._compare_with_tolerance(need, product_qty, rounding)
+                        and lot_quantity > 0
+                    ):
+                        need = (
+                            yield lot_quant.location_id,
+                            lot_quantity,
+                            need,
+                            lot,
+                            None,
+                        )  # noqa


What about lots that are partially reserved? I think you want to skip them.

product_qty can be computed outside loop.

Suggested change

for lot_quant in lot_quants:

product_qty = sum(lot_quant.mapped("quantity"))

lot_quantity = sum(lot_quant.mapped("quantity")) - sum(

lot_quant.mapped("reserved_quantity")

)

if (

lot

and self._compare_with_tolerance(need, product_qty, rounding)

and lot_quantity > 0

):

need = (

yield lot_quant.location_id,

lot_quantity,

need,

lot,

None,

) # noqa

if not lot:

continue

if any(quant.reserved_quantity in lot_quants):

# skip lots that are partially reserved

continue

product_qty = sum(lot_quants.mapped("quantity"))

if not self._compare_with_tolerance(need, product_qty, rounding):

continue

for lot_quant in lot_quants:

lot_quantity = lot_quant.quantity - lot_quant.reserved_quantity

need = yield (

lot_quant.location_id,

lot_quantity,

need,

lot,

None,

) # noqa

@jbaudoux Could you check my proposed version of the function?

@jbaudoux Your code has an error.

if any(quant.reserved_quantity for quant in lot_quants):

@geomer198 was this addressed?

@geomer198 was this addressed?

Yes, I added this code.

jbaudoux · 2023-11-27T13:44:27Z

stock_reserve_rule/models/stock_reserve_rule.py

+            for lot, lot_quants in quants.filtered(
+                lambda quant: quant.product_id.id == product.id
+                and quant.lot_id
+                and not quant.reserved_quantity


You cannot exclude quant with reserved quantities before the group by. If the lot is on 2 locations and has a reservation only on one location, you will not end-up with the right result

@jbaudoux I fixed it. I hope now is better.

Now we lost the exclusion. See my proposed solution above

Now we lost the exclusion. See my proposed solution above

I didn't understand this line:

if any(quant.reserved_quantity in lot_quants): # skip lots that are partially reserved continue

In this function only quants variable. And why float must be contain in quant's record set?

elvise · 2023-11-28T13:08:31Z

@jbaudoux good now ? :)

jbaudoux · 2023-11-30T09:36:02Z

@jbaudoux good now ? :)

@elvise @francesco-ooops
Please write a list of tests instead of keeping asking me if this is now good. Write them in English and ask the developer to implement them. Also run your tests manually. You will then have your answer.

Write unit tests for the tolerance. See #1834 (comment)
Write unit test for a lot on multiple locations
Write unit test for anything that has been discussed above and for which you could not tell yourself if it is now good

github-actions · 2024-04-28T12:27:17Z

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.

geomer198 marked this pull request as draft September 8, 2023 23:50

geomer198 marked this pull request as ready for review September 11, 2023 10:12

geomer198 force-pushed the 14.0-t2780-stock_reserve_rule-imp branch from 3e791e2 to edd4fdc Compare September 15, 2023 11:05

aayartsev approved these changes Sep 19, 2023

View reviewed changes

francesco-ooops approved these changes Sep 19, 2023

View reviewed changes

OCA-git-bot added approved ready to merge labels Sep 19, 2023

jbaudoux requested changes Sep 19, 2023

View reviewed changes