-
-
Notifications
You must be signed in to change notification settings - Fork 715
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
[9.0] [MIG] stock_inventory_lockdown #282
[9.0] [MIG] stock_inventory_lockdown #282
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Code reviewed and tested on runbot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional and technical review 👍
Just to minor (not blocking) things:
- It would be good to update the images of the description, they are v8.
- Squash transifex commits.
c4b0515
to
8fd3d9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mreficent IMO we can improve the logic to check for locked locations.... and improve the performances. see my comments. What do you think about?
_inherit = 'stock.inventory' | ||
|
||
@api.model | ||
def _get_locations_open_inventories(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mreficent IMO if would be more efficient to allow to pass a location_ids parameter to this method and t use this paramaterd (if provided) into the domain for the search on inventories...
ex:
@api.model
def _get_locations_open_inventories(self, locations_ids = None):
inventory_domain = [('state', '=', 'confirm')]
if locations_ids:
inventory_domain.append(('location_id', 'child_of', location_ids))
inventories = self.search(inventory_domain)
location_ids = inventories.mapped('location_id')
location_domain = [
('location_id', 'child_of', location_ids.ids),
('usage', 'in', ['internal', 'transit'])]
if locations_ids:
location_domain.append(('location_id', 'child_of', location_ids))
return self.env['stock.location'].search(location_domain)
In this way we will filter the result to the locations for which we want to check if an inventory exist.
@api.multi | ||
def _check_inventory(self): | ||
"""Error if an inventory is being conducted here""" | ||
location_inventory_open_ids = self.env['stock.inventory'].sudo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mreficent With the change proposed for _get_locations_open_inventories
the method will be simplified
location_inventory_open_ids = self.env['stock.inventory'].sudo()._get_locations_open_inventories(self.ids)
if location_inventory_open_ids:
raise ValidationError(...)
This module is a feature extracted from stock_inventory_location. Ported to v8 and new API Adopted latest OCA standards Added tests
8fd3d9e
to
12a6ae3
Compare
@lmignon suggestion attended to 😊 |
""" | ||
if not self.env.context.get('bypass_lockdown', False): | ||
# Find the locked locations | ||
locked_location_ids = self.env[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mreficent This code should be also modified to take advantage of the new paramater on _get_locations_open_inventories
if 'location_id' in vals.keys():
locked_location_ids = self.env['stock.inventory']._get_locations_open_inventories(
self.env['stock.location'].browse(vals['location_id']) + self.mapped('location_id')
)
if locked_location_ids:
raise ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then the raise message will not have explicit locations 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mreficent
location_names = locked_location_ids.mapped('name')
raise ValidationError(
_('An inventory is being conducted at the following '
'location(s):\n%s') % "\n - ".join(location_names))
quant = super(StockQuant, self).create(vals) | ||
if not self.env.context.get('bypass_lockdown', False): | ||
locked_location_ids = self.env[ | ||
'stock.inventory']._get_locations_open_inventories() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mreficent same comment as above
cf016db
to
ee0a3eb
Compare
Lgtm code review only |
3f8a631
to
c5ad07e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Functional review
Standard migration.