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][ADD] stock_available_exclude_location: add new module #2153

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

Conversation

WesleyOliveira98
Copy link

@WesleyOliveira98 WesleyOliveira98 commented Sep 9, 2024

Ticket: HT00837

This module allow defining excluded locations for product availability.

Select the excluded locations in Inventory Settings:
image

Now the quantity in these locations is excluded of product availability
image

Even when we have quantities of this product in the locations we marked to exclude
image

cc: @marcelsavegnago @kaynnan @douglascstd

@WesleyOliveira98 WesleyOliveira98 force-pushed the 14.0-add-stock_available_exclude_location branch 2 times, most recently from af5cbf4 to f0c802d Compare September 9, 2024 19:34
@WesleyOliveira98 WesleyOliveira98 marked this pull request as ready for review September 9, 2024 19:42
Copy link
Member

@douglascstd douglascstd left a comment

Choose a reason for hiding this comment

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

[FUNCTIONAL REVIEW]

The test is conducted to check if the configuration is applied to the child locations; if so, those locations are excluded.

image
image
image

Is this the expected behavior? Wouldn't it be better to exclude all child locations from the selected location? Alternatively, could there be two fields:

  • Unique
  • Parent and Children

@WesleyOliveira98 WesleyOliveira98 force-pushed the 14.0-add-stock_available_exclude_location branch from f0c802d to c5bd70a Compare September 10, 2024 14:26
@WesleyOliveira98
Copy link
Author

Wouldn't it be better to exclude all child locations from the selected location?

Makes sense, we add the module stock_location_children in depends and add all the children the excluded location have

Copy link
Contributor

@kaynnan kaynnan left a comment

Choose a reason for hiding this comment

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

LGTM

Code Review

@WesleyOliveira98 WesleyOliveira98 force-pushed the 14.0-add-stock_available_exclude_location branch 2 times, most recently from a368b3d to 670ad36 Compare September 10, 2024 15:05
@WesleyOliveira98 WesleyOliveira98 force-pushed the 14.0-add-stock_available_exclude_location branch from 670ad36 to 1c665c6 Compare September 10, 2024 17:20
@rousseldenis
Copy link
Contributor

This looks great.

@WesleyOliveira98 I'm just wondering if those excluded locations should be excluded at warehouse level, why not defining those locations outside the warehouse view location ?

@WesleyOliveira98
Copy link
Author

I'm just wondering if those excluded locations should be excluded at warehouse level, why not defining those locations outside the warehouse view location ?

@rousseldenis We thought about defining it in the warehouse as it was the model that made the most sense for defining location rules, where do you suggest we place this configuration?

@rousseldenis
Copy link
Contributor

I'm just wondering if those excluded locations should be excluded at warehouse level, why not defining those locations outside the warehouse view location ?

@rousseldenis We thought about defining it in the warehouse as it was the model that made the most sense for defining location rules, where do you suggest we place this configuration?

No, I wanted to say, from a functional point of view about locations that should be excluded from available quantities.
If you define them outside the warehouse view location, they will be excluded from available quantities. But maybe you have a use case that does not correspond to that.

@WesleyOliveira98 WesleyOliveira98 force-pushed the 14.0-add-stock_available_exclude_location branch from 1c665c6 to cad63b0 Compare September 11, 2024 20:16
@WesleyOliveira98 WesleyOliveira98 force-pushed the 14.0-add-stock_available_exclude_location branch from cad63b0 to 7f1bf7d Compare September 11, 2024 20:29
@WesleyOliveira98
Copy link
Author

@rousseldenis I change the configuration of the excluded locations to inventory settings based on the company:

image

Could you review it please?

cc: @marcelsavegnago @kaynnan @douglascstd

@rousseldenis
Copy link
Contributor

Ok, but you didn't answer my question 😅

@WesleyOliveira98
Copy link
Author

No, I wanted to say, from a functional point of view about locations that should be excluded from available quantities.
If you define them outside the warehouse view location, they will be excluded from available quantities. But maybe you have a use case that does not correspond to that.

Sure, this way it is much easier to deal with this specific problem of not counting the quantities of a location, but we have some cases such as not counting a specific shelf in stock, or excluding pre-production and post -production when using MRP with 3 steps, and using the stock_available_base_exclude_location module we just need to define where it will be configured and pass this as a context to the product, that's why we are proposing this module, it will be very useful for some of our customers

@WesleyOliveira98
Copy link
Author

ping @OCA/stock-weighing-maintainers @rousseldenis @marcelsavegnago @kaynnan @douglascstd

@rousseldenis rousseldenis added this to the 14.0 milestone Sep 24, 2024
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.

4 participants