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

[ADD][14.0] procurement_auto_create_group_by_product #1821

Merged
merged 8 commits into from
May 22, 2024

Conversation

jbaudoux
Copy link
Contributor

@jbaudoux jbaudoux commented Aug 24, 2023

Pick by product by generating one picking per product on the procurement run.
Configurable on the rule.
A procurement group per product will be generated.

I refactored a bit procurement_create_group to make it hookable

The use case it to be able a pick by product instead of by SO. So if 5 SO are for the same product, you have only one move for the total quantity.

Initially we had configured one fixed procurement group on the rule and all moves where added to the same picking. Users where working on the move lines. This was causing performance issue each time a new move was inserted as there are lot of moves and odoo recomputes the check_entire_pack on all moves which is slow. This was also causing concurrent updates when 2 users working on different products were extracting & validating a move line from that same picking.
With this new module, we greatly increase the amount of pickings but we reduce a lot the recomputation when a new move is added as there should be only one move in the picking, and we reduce to the minimum the risk of concurrent updates (it remains only if 2 users work on the same product on 2 different locations).

cc @mt-software-de @LoisRForgeFlow @BernatPForgeFlow

@jbaudoux jbaudoux force-pushed the 14-procurement_create_group_by_product branch 7 times, most recently from f39cd29 to 6f9c4f0 Compare August 28, 2023 12:23
@jbaudoux jbaudoux marked this pull request as ready for review August 28, 2023 12:24
@jbaudoux jbaudoux force-pushed the 14-procurement_create_group_by_product branch from 6f9c4f0 to e19156a Compare August 28, 2023 12:26
@@ -13,13 +13,6 @@ def _get_rule(self, product_id, location_id, values):
rule = super()._get_rule(product_id, location_id, values)
# If there isn't a date planned in the values it means that this
# method has been called outside of a procurement process.
if (
rule
and not values.get("group_id")
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure we want to remove this check? This could have side-effects on existing installations no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Following your commit message it's what you want, but just to be sure about the possible regressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not find out why it was there. I hope it is safe to remove it @LoisRForgeFlow @JordiBForgeFlow
Otherwise it prevents the feature to work in a pull flow coming from a SO.

Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow Dec 12, 2023

Choose a reason for hiding this comment

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

Indeed, this is a bit risky. From our side, we have reviewed and we shouldn't be affected by the change as we never use it for SO rules. However, there might be other installations in which this can lead to a change in the behavior.

My 2 cents.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just in case, I think you can reduce the risk by putting the if condition in a method and doing super in the new module, removing the condition that is not wanted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don't see a breaking change, let's consider it a bug fix.
Note that I'm not changing anything in regards of the above comment regarding date planned.

The configuration of the rule should prevail. If it is configured to create a new procurement group, then it should do it. Independently of how the rule is run.

procurement_auto_create_group/models/stock_rule.py Outdated Show resolved Hide resolved
procurement_auto_create_group/models/stock_rule.py Outdated Show resolved Hide resolved
@jbaudoux jbaudoux force-pushed the 14-procurement_create_group_by_product branch 2 times, most recently from 11e7cc7 to d99d052 Compare September 1, 2023 17:16
@sebalix
Copy link
Contributor

sebalix commented Oct 9, 2023

@jbaudoux pre-commit to fix

Do not override rule1 during setup, renamed the 2 rules.
Regroup the creation of the procurement group in a method.
When the pull flow is coming from an SO, a procurement group is already
preset. When the option to auto create a procurement group, replace that
procurement group by a new one.
@jbaudoux jbaudoux force-pushed the 14-procurement_create_group_by_product branch from d99d052 to 41de956 Compare October 9, 2023 20:47
@jbaudoux
Copy link
Contributor Author

jbaudoux commented Oct 9, 2023

rebased

@jbaudoux jbaudoux force-pushed the 14-procurement_create_group_by_product branch from 41de956 to d2cec54 Compare October 9, 2023 20:56
Allow to merge moves having different date_deadline to pick everything
in one operation
@jbaudoux jbaudoux force-pushed the 14-procurement_create_group_by_product branch from d2cec54 to 252b9ad Compare October 9, 2023 21:44
@jbaudoux
Copy link
Contributor Author

ping @mt-software-de for review

@rousseldenis rousseldenis added this to the 14.0 milestone Nov 6, 2023
@jbaudoux
Copy link
Contributor Author

@mt-software-de @sebalix @TDu Need review

Copy link
Contributor

@mt-software-de mt-software-de left a comment

Choose a reason for hiding this comment

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

Minor remarks, to ensure only one group per product.

class ProcurementGroup(models.Model):
_inherit = "procurement.group"

product_id = fields.Many2one("product.product", index=True)
Copy link
Contributor

@mt-software-de mt-software-de Nov 28, 2023

Choose a reason for hiding this comment

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

Do we need a constraint? To ensure that there is only one group per product?

Comment on lines +14 to +15
if product.auto_create_procurement_group_ids:
return fields.first(product.auto_create_procurement_group_ids)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add a lock, to ensure there is only one group created in case of auto_by_product?

Suggested change
if product.auto_create_procurement_group_ids:
return fields.first(product.auto_create_procurement_group_ids)
if product.auto_create_procurement_group_ids:
return fields.first(product.auto_create_procurement_group_ids)
else:
# lock to ensure there is only one group created

Copy link
Contributor

Choose a reason for hiding this comment

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

@jbaudoux could you merge jbaudoux#1 ?

Concurent transaction could create multiple procurement group for the
same product.
The use of an adivsory lock will prevent this from happening.
…-add-lock

 [IMP] proc_auto_create_grp_by_product: add lock
@jbaudoux
Copy link
Contributor Author

jbaudoux commented Apr 5, 2024

@mt-software-de I have done this improvement 3213a30

@jbaudoux jbaudoux force-pushed the 14-procurement_create_group_by_product branch 6 times, most recently from 40e94f2 to c831049 Compare April 8, 2024 08:46
Copy link
Contributor

@mt-software-de mt-software-de left a comment

Choose a reason for hiding this comment

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

LGTM

@OCA-git-bot
Copy link
Contributor

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

Raise correct error to allow correct catching and retry
@jbaudoux jbaudoux force-pushed the 14-procurement_create_group_by_product branch from c920921 to 6572251 Compare May 8, 2024 09:38
@jbaudoux
Copy link
Contributor Author

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-1821-by-jbaudoux-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit c521e0b into OCA:14.0 May 22, 2024
5 checks passed
@OCA-git-bot
Copy link
Contributor

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

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

Successfully merging this pull request may close these issues.

8 participants