-
-
Notifications
You must be signed in to change notification settings - Fork 723
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
[15.0][ADD] stock_inventory #1512
[15.0][ADD] stock_inventory #1512
Conversation
@DavidJForgeFlow Thanks for this. @pedrobaeza I think you could look at it following what we discussed on OCA days. |
Yeah, I see. David, please name the module |
efa3d6f
to
1a4afb0
Compare
1a4afb0
to
ffb8b68
Compare
All references changed. Also changed the model name to stock.inventory, is it ok like this? |
ffb8b68
to
73fd325
Compare
99e4ba0
to
79649c6
Compare
c223ffe
to
9ad7b96
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.
LGTM! 🔝 👍
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
("manual", "Manual Selection"), | ||
("category", "Product Category"), | ||
("one", "One Product"), | ||
("lot", "Lot Serial Number"), |
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.
String = "Lot" would be cleaner here?
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.
I would say Lot/Serial Number
to use Odoo naming
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.
@DavidJForgeFlow Maybe an 'owner' management too ?
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.
I agree with Lot/Serial Number
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.
@rousseldenis what do you mean with 'owner'? Add a responsible field?
The other changes are done, thanks!
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.
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.
Done, thank you!
|
||
lot_ids = fields.Many2many( | ||
"stock.production.lot", | ||
string="Lot/Serial Number", |
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.
String = "Lots" would be cleaner here?
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.
Edit: Lot/Serial Numbers
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.
Done thanks!
|
||
def action_state_to_in_progress(self): | ||
active_rec = self.env["stock.inventory"].search([("state", "=", "in_progress")]) | ||
if active_rec: |
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.
Should we extend this? Checking if there are counts with same product or same lots or same locations? Counting two locations at the same time should be possible?
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.
I don't think so because you can only do an Adjustment at a time, so repeated counts are "imposible" (unless something magic happens).
def action_state_to_done(self): | ||
self.state = "done" | ||
for quant in self.stock_quant_ids: | ||
quant.to_do = True |
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.
It is still a manual action to confirm the quant count?
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.
Yes, confirming a count is still manual.
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.
Ok, this is very confusing? Should we at least give a warning to the user that the count(s) still needs to be confirmed? Setting a group to done for me means that the count is confirmed.
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.
What if a quant is being mutated which is currently in a stock adjustment which is not done yet, should we not block this? Same like not allowing 2 counts? Overall I have a sense the module is confusing now. Why do we add a state to stock_inventory if nothing is being done / blocked or checked? If it is simply a group of quants, then we can create a group without a workflow?
self.state = "in_progress" | ||
if self.product_selection == "all": | ||
for location in self._origin.location_ids: | ||
self.stock_quant_ids = self.env["stock.quant"].search( |
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.
I would have put all domains in individual methods in order to allow override
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.
Done, thanks!
|
||
def action_state_to_done(self): | ||
self.state = "done" | ||
for quant in self.stock_quant_ids: |
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.
Better :
for quant in self.stock_quant_ids: | |
self.stock_quant_ids.update({"to_do": True}) |
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.
Done!
5d27993
to
9e89c1c
Compare
_order = "date desc, id desc" | ||
|
||
name = fields.Char( | ||
required=True, default="Inventory ", string="Inventory Reference" |
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.
required=True, default="Inventory ", string="Inventory Reference" | |
required=True, default="Inventory", string="Inventory Reference" |
("manual", "Manual Selection"), | ||
("category", "Product Category"), | ||
("one", "One Product"), | ||
("lot", "Lot Serial Number"), |
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.
|
||
def _get_all_quants(self, locations): | ||
return self.env["stock.quant"].search( | ||
[ |
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.
Maybe you could put domains retrievals in separate methods to easy possible inheritance?
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.
All the different type of Inventory Adjustments are computed in different methods, what do you mean with putting the domains retrievals in separate methods?
31e650e
to
9285897
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.
Works Good!
Just a minor thing that needs to be fixed:
- Create an inventory adjustment for one product only.
- Start the adjustment
- Change the quantity and click on Apply
- The stock move line is successfully linked
- Go to the stock.inventory again and then to the adjustments, remove the filter "Todo".
- Change the quantity again to a new one and click on apply.
- Issue: A new quant is successfully created but it is not linked to the inventory adjustment, that is, from the button Stock move lines I can only see one of the stock moves lines
9285897
to
d440ff7
Compare
Thank you Aaron, this is already fixed! The code is now better to inherit because of some change of how domains work, thanks to @GuillemCForgeFlow and @rousseldenis for the idea. Also, added the possiblilty of doing two Adjustments on two different Locations at the same time. |
Thanks for the work @DavidJForgeFlow 👍 ! |
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 LGTM!
@AaronHForgeFlow If you did a functional approval I am really curious about feedback on my questions. For me this module currently makes no sense. It is just a group of quants and could better be called stock_quant_group. |
Hi @CasVissers-360ERP It is called stock_inventory in order to respect what it was in standard in previous versions, otherwise the migration will get more complex. I guess the main discussion regards this comment:
I think that if the quant is altered while the inventory adjustment is done then there is no inconsistencies in the database in this case, because Odoo will internally handle this. It may be confusing for users that quants did a change while they were doing the adjustments. In that case maybe it is necessary to avoid the modification of the quant until the inventory adjustment is completed. But that can be done on a separate module I think. I think there was also a module for that in this repo. |
You only answered one of my concerns? My main concern: Why do we add a state to stock_inventory if nothing is being done / blocked or checked? In addition it is not an inventory / inventory count. An inventory count start which blocking the location, counting the stock, and adjusting the stock level accordingly. I miss the whole idea of creating a count in Odoo (group of counts). But if there is no logic in the group/inventory I feel like it will be very confusing for the end-user. I have a hard time understanding the benefits as well. If the goal of this module is to simply create a group of quants, just remove the state and rename the module. |
d440ff7
to
e9d627d
Compare
Hi @CasVissers-360ERP , I am just saying that the module works as my customer needs. They just need a document that records and keep a history of the changes on inventory. They are not worried about that restriction because the lifecycle of an inventory adjustment is too short for them and that kind of "issues" are not relevant. Nothing is blocked the quant in this module but it can be blocked on a module that depends on this one. Perhaps that feature can be included here, I am not against that. |
@AaronHForgeFlow Well maybe the module is not generic/mature enough to be on OCA module? |
@CasVissers-360ERP I think it is very generic module, it recovers the standard inventory adjustments feature form previous versions and that is proved to be generic enough. The only thing that may be missing is the locking on the quant that is being inspected. It is ok to me to implement that restriction on this module, but I will not block this PR because of that. Would you be fine with this PR if it implements that restriction? Those discussions are good for the quality of the module so I am glad you commented and I hope more people joins to the discussion here. |
e9d627d
to
73cf905
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.
Great work! Customers still need this functionality to be able to group inventories for individual products into single big document. That is much better for Warehouse manager to review it later
Not sure I understand why Odoo removed it.
@DavidJForgeFlow thank you for the work! Hope it will be merged soon into OCA. Approved from my side
73cf905
to
2c27d01
Compare
@AaronHForgeFlow I don't agree with this " it recovers the standard inventory adjustments " A big thing missing is: But since everybody else seems to approve this PR I think it's best for me to exit the discussion. |
[15.0][IMP] stock_inventory: add product selection and fixup
2c27d01
to
c4a3719
Compare
Obviously this can receive improvements and changes, but since I think it is a common requirement for customers coming from previous Odoo versions, let's at least cover the gap with this (I guess better this than nothing). Thanks to all participants! Do not hesitate to open PRs to include improvements and changes once this gets merged. /ocabot merge nobump |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at 34df53d. Thanks a lot for contributing to OCA. ❤️ |
This module allows to group Inventory Adjustments and have a group traceability (like before 15.0).
@ForgeFlow