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

[14.0][FIX] stock_request_purchase: Define the correct quantity of allocations to be created from purchase order lines #2186

Open
wants to merge 1 commit into
base: 14.0
Choose a base branch
from

Conversation

victoralmau
Copy link
Member

@victoralmau victoralmau commented Nov 5, 2024

Define the correct quantity of allocations to be created from purchase order lines

Example use case:

  • Create stock request for a product with quantity 10
  • Confirm purchase order
  • Change purchase order line to quantity 12
  • "In progress" quantity of the stock request must be 12 (not 22=10+12)

@Tecnativa TT51567

@victoralmau victoralmau marked this pull request as draft November 5, 2024 15:37
@victoralmau victoralmau marked this pull request as ready for review November 5, 2024 15:46
@victoralmau
Copy link
Member Author

Ping @pedrobaeza @LoisRForgeFlow

@@ -37,7 +37,8 @@ def _prepare_stock_moves(self, picking):
0,
{
"stock_request_id": request.id,
"requested_product_uom_qty": request.product_qty,
"requested_product_uom_qty": re["product_uom_qty"]
/ len(self.stock_request_ids),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm quite doubtful on the len()

Copy link
Member Author

Choose a reason for hiding this comment

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

Example use case (https://github.com/OCA/stock-logistics-warehouse/blob/14.0/stock_request_purchase/tests/test_stock_request_purchase.py#L138):

  • Create a stock request 1 with quantity 5
  • Create a stock request 2 with quantity 5
  • Confirm stock request 1: A purchase order line is generated with quantity 5
  • Confirm stock request 2: The purchase order line is modified with quantity 10
  • The purchase order is confirmed.
  • 2 allocations will be generated with quantity 5 each.

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, this should be the diff qty (with the main comment example, 12 - 10 = 2).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! I will encourage you to add a third stock request 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to set the appropriate (proportional) quantity for each allocation + extra test.

@pedrobaeza pedrobaeza added this to the 14.0 milestone Nov 5, 2024
@victoralmau victoralmau force-pushed the 14.0-fix-stock_request_purchase-TT51567 branch from 4ede3a7 to c6a84c8 Compare November 6, 2024 07:51
@rousseldenis
Copy link
Contributor

@JordiBForgeFlow @LoisRForgeFlow Any thought on this ?

Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

Hi!

I think the PR description does not really match what is being changed in the code.

Also functionally reproducing the case, the behavior is a bit different. Instead of ""In progress" quantity of the stock request must be 10" the in progress qty is 12. Which in fact makes more sense for me, even if the request is for 10 units if your PO is finally for 12 and there is no other request to allocate those extra units, those 2 units are added to the in progress qty.

At the same time, looking at the code I see a different problem being solved, and it is the allocation of quantities when the same PO line or stock moves is serving several stock requests. In such case, it was indeed correct but I think the proposed algorithm is not the best way. I put an example:

SR1 for 7 units
SR2 for 5 units
both confirmed and combined in the same RfQ.
Confirm, receipt for 12 units. At this point qty in progress is fine in both SR.
Go and add one more unit to the PO from 12 to 13. Automatically the receipts gets updated to 13 units to. Result:

image

image

So the extra unit is spread among the SR. I don't like too much this outcome, it is true that the precision rounding of the units in Odoo is 0.01 and that has an effect. However, maybe allocating one by one until full and them allocating all the excess to the last one might be a more pragmatic approach and less confusing. Not 100% sure of this, so feel free to argument against.

I could be doing more tests (like changing the uom rounding, or reducing the qty instead of increasing, etc) but I would ask you to first clarify the scope of this fix. Maybe a step for second request missing in your description?

EDIT: I just saw it is actually described in the discussion above #2186 (comment). Sorry about that, just add it to the description so anyone else that is a fast reader like me does not miss it :)

There is for sure a bug here, so thanks for tackling it!

@LoisRForgeFlow
Copy link
Contributor

BTW, I tried my same example above, but instead of adding one unit, reducing it, and the qty in progres remained unchanged in both SR (5 and 7 units respectively) even when the receipt it is now for 11 units.

…ons to be created from purchase order lines

Example use case:
- Create stock request for a product with quantity 10
- Confirm purchase order
- Change purchase order line to quantity 12
- "In progress" quantity of the stock request must be 12 (not 22=10+12)

TT51567
@victoralmau victoralmau force-pushed the 14.0-fix-stock_request_purchase-TT51567 branch from c6a84c8 to c9c1675 Compare November 6, 2024 13:48
@victoralmau
Copy link
Member Author

Some comments:

  • Sorry for what was indicated in the description (already corrected), the expected value is 12.
  • Corrected regarding possible rounding and added an extra use case.
  • The use case would be covered to increase/decrease the amount. Important to remember that odoo does not cover the use case of reducing the quantity on a purchase order until v15 (if I remember correctly), i.e. it is not reduced on the picking.
  • The previous error I think is clear, if the quantity was modified (although for now in v14 it is not possible to reduce by the odoo core) the quantity in progress was increased, now, the correct quantity is defined proportionally.

Do you need anything else?

@LoisRForgeFlow
Copy link
Contributor

Some comments:

  • Sorry for what was indicated in the description (already corrected), the expected value is 12.

Thanks! 👍

  • Corrected regarding possible rounding and added an extra use case.
  • The use case would be covered to increase/decrease the amount. Important to remember that odoo does not cover the use case of reducing the quantity on a purchase order until v15 (if I remember correctly), i.e. it is not reduced on the picking.
  • The previous error I think is clear, if the quantity was modified (although for now in v14 it is not possible to reduce by the odoo core) the quantity in progress was increased, now, the correct quantity is defined proportionally.

Sorry, I was not specific, I reduced the qty in the receipt manually by unlocking it. The qty in progress is still unchanged.

Do you need anything else?

What do you think of my previous proposal/suggestion: "maybe allocating one by one until full and them allocating all the excess to the last one might be a more pragmatic approach and less confusing". I mean what you propose is not technically incorrect, but it is a bit more confusing. Eg:

PO 14 units and 2 SR for 4 and 8 units. I would rather have qty in progress 4 and 10, than 4.66 and 9.33. I see the proportional allocation a bit confusing. What do you think?

@victoralmau
Copy link
Member Author

Sorry, I was not specific, I reduced the qty in the receipt manually by unlocking it. The qty in progress is still unchanged.

But that is not enough, changing the stock move demand will not change anything because it will not call the _prepare_stock_moves() method of purchase.order.line.

What do you think of my previous proposal/suggestion: "maybe allocating one by one until full and them allocating all the excess to the last one might be a more pragmatic approach and less confusing". I mean what you propose is not technically incorrect, but it is a bit more confusing. Eg:

PO 14 units and 2 SR for 4 and 8 units. I would rather have qty in progress 4 and 10, than 4.66 and 9.33. I see the proportional allocation a bit confusing. What do you think?

I think that although it may seem more readable it is totally incorrect, if for example we have 11 and 1 it would become 11 and 2. Personally I think that all this problem happens due to linking several requests to the same purchase order line (something that can be avoided with procurement_purchase_no_grouping).

@LoisRForgeFlow
Copy link
Contributor

Sorry, I was not specific, I reduced the qty in the receipt manually by unlocking it. The qty in progress is still unchanged.

But that is not enough, changing the stock move demand will not change anything because it will not call the _prepare_stock_moves() method of purchase.order.line.

Fair enough, out of the scope of this PR.

What do you think of my previous proposal/suggestion: "maybe allocating one by one until full and them allocating all the excess to the last one might be a more pragmatic approach and less confusing". I mean what you propose is not technically incorrect, but it is a bit more confusing. Eg:
PO 14 units and 2 SR for 4 and 8 units. I would rather have qty in progress 4 and 10, than 4.66 and 9.33. I see the proportional allocation a bit confusing. What do you think?

I think that although it may seem more readable it is totally incorrect, if for example we have 11 and 1 it would become 11 and 2. Personally I think that all this problem happens due to linking several requests to the same purchase order line (something that can be avoided with procurement_purchase_no_grouping).

You are right about the procurement_purchase_no_grouping. However since we are dealing with this case here, we need to discuss it even if it is quite uncommon or avoidable, at the end of the day it is the reason that made you open this PR, isn't it? Back to discussion, I still like it more 11 and 2 than 11.91666 and 1.08333.

@pedrobaeza @rousseldenis thoughts? opinions?

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

Successfully merging this pull request may close these issues.

4 participants