-
Notifications
You must be signed in to change notification settings - Fork 21
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
12.0 Fix "stock_inventory_merge" #112
base: 12.0
Are you sure you want to change the base?
Conversation
When clicking on button "Complete With Zero" in an inventory form view, the created inventory lines have to be added to the current inventory.
The duplicate lines need to be deleted *before* writing the total quantity on the kept line, as the write method checks for duplicates, which may trigger that "You cannot have two inventory adjustments..." exception.
Codecov Report
@@ Coverage Diff @@
## 12.0 #112 +/- ##
=======================================
Coverage 72.31% 72.31%
=======================================
Files 66 66
Lines 1510 1510
=======================================
Hits 1092 1092
Misses 418 418
Continue to review full report at Codecov.
|
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. A question inline.
thanks for your contribution.
@@ -74,11 +74,11 @@ def complete_with_zero(self): | |||
item | |||
) == self._get_inventory_line_keys(product_line): | |||
found = True | |||
continue | |||
break |
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.
makes sense.
if not found: | ||
# Add the line, if inventory line was not found | ||
product_line["product_qty"] = 0 | ||
product_line["inventory_id"] = self.id | ||
product_line["inventory_id"] = inventory.id |
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.
indeed !
# Delete all the other lines | ||
line_ids.remove(keeped_line_id) | ||
line_obj.browse(line_ids).unlink() | ||
|
||
# Update the first line with the sumed quantity | ||
# Note: This has to be done *after* deleting the other lines, | ||
# because the `write()` calls the `_check_no_duplicate_line()` |
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 should not fails, due to that https://github.com/grap/grap-odoo-incubator/blob/12.0/stock_inventory_merge/models/stock_inventory_line.py#L25
could you explain in which case it is failing. i don't face any problem (and I have a lot of duplicates !)
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.
The issue here is that if the update keeped_line.write(...)
comes before the removal line_obj.browse(line_ids).unlink()
, then the duplicates are still there during the update, and the Odoo core method write()
will do a check for duplicates and thus -- under certain conditions -- raise the exception:
https://github.com/OCA/OCB/blob/a082f6d9ae716561a40df61a5f77e1c089a97945/addons/stock/models/stock_inventory.py#L405-L409
Note that there the condition used in the method _check_no_duplicate_line()
for actual checking does not simply compare the product_id
, but a bit more, including a check for inventory_id.state
being 'confirm'
.
So, as long as the inventory is not yet confirmed, you should be fine. I'm not sure if the inventory being already confirmed is "too artificial" a condition, and usually won't occur in practice.
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.
Hi,
Finally have time to finish the review. sorry for the delay !
Well, I think is "too artificial" as you say. ;-)
the inventory with duplicates can only be in a non confirm state, as the function action_validate
check if there are duplicates, and prevent validation :
See : https://github.com/grap/grap-odoo-incubator/blob/12.0/stock_inventory_merge/models/stock_inventory.py#L37
So I see the commit 93ebd39 as unnecessary.
Or maybe I missed something. If yes, could you write a test that is failing without this change ?
If not, could you consider remove this commit, and we can merge the PR for the 2 others great commits.
kind regards.
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.
Hi,
sorry, I was a bit confused here myself, especially about when this failing check happens.
First of all, state 'confirm'
is the "In Progess" state, which is certainly not "too artificial", but rather the expected state for a started, but not yet validated (a.k.a. done or finalized) inventory. (The state 'done'
= "Validated" would be "too artificial" for the merging of inventories, in the sense that such done inventories should never be merged anymore. But this case is not what I had in mind, anyway.)
Now, the analysis below is no longer valid, as that issue has already been fixed with commit eb02a51, where the flag do_not_check_duplicates=True
gets added to the context also in method write()
of model "stock.inventory.line". I decided leave it here for documentation purposes.
However, the way the issue had been solved, by changing of the search domain to [('product_id', '=', False)]
, seems like a dirty hack to me. This domain will yield no matching records, because the product_id
is a required field. Wouldn't it be better in terms of code quality and readability to explicitly suppress the check properly (in module "stock_inventory_merge" at the right place in model "stock.inventory.line"):
def _check_no_duplicate_line(self):
if self.env.context.get("do_not_check_duplicates", False):
return
return super(StockInventoryLine, self)._check_no_duplicate_line()
rather than accomplish the effect indirectly in this obscure way by overwriting search()
.
Or am I missing something here?
If this is fine, I would the revert commit 93ebd39 and then add the suggested code change for overwriting _check_no_duplicate_line()
and removing the overwriting of search()
.
[Here follows the obsolete analysis:]
Module "stock_inventory_merge" provides a flag do_not_check_duplicates=True
in the context when creating "stock.inventory.line" objects.
Now, the check _check_no_duplicate_line()
, which is done in core module "stock" when creating or writing a "stock.inventory.line" record, does a search()
for duplicate lines. However, module "stock_inventory_merge" overwrites the search domain when this context flag do_not_check_duplicates
is True
-- to the simple domain [('product_id', '=', False)]
, which will yield no matching records, because the product_id
is a required field.
This essentially means that when the module "stock_inventory_merge" is installed, the check for duplicate lines is "suppressed" when creating or writing "stock.inventory.line" records.
So, the actual merging of the inventories should be fine.
However, the method action_merge_duplicated_line()
we are talking about here is executed when merging duplicate lines of the merged inventory via button "Merge Duplicates".
And here when clicking on that button the exception "You cannot have two inventory adjustments..." will be raised if there are multiple lines with the same product, location etc.
And of course this is just the situation when the button should be clicked.
So, if the accumulated quantity is written to the line to be kept (keeped_line
) before removing the duplicate lines, this write()
will execute the check _check_no_duplicate_line()
, this time without the context flag do_not_check_duplicates
. And thus the exception is raised.
In other words, the method action_merge_duplicated_line()
for button "Merge Duplicates" should never work when it is supposed to do (i.e., when there actually are duplicate lines in the inventory).
For a test,
- create a new inventory (say with manually added products), and start it adding a product,
- duplicate this inventory,
- merge the two of them, so that you get an inventory with duplicate lines, and finally
- in the merged inventory, do merge the duplicate lines with button "Merge Duplicates", which (with the old code) should raise the exception.
Summarized, I think this commit 93ebd39 is actually necessary.
Alternatively, one could provide the flag do_not_check_duplicates=True
in the context when writing the accumulated quantity to the kept line, i.e.,
keeped_line.with_context(do_not_check_duplicates=True).write({"product_qty": sum_quantity})
[And this is what makes the analysis obsolete, as that context flag gets already added in write()
.]
This branch contains three small fixes for module "stock_inventory_merge" in version 12.0:
using
break
instead ofcontinue
... which will continue the loop, which is unnecessary in this case
using the
inventory.id
of the single inventory in thefor inventory in self
loop... instead of
self.id
, which will work only for singleton browse record setsby switching the order of updating the kept line and removing all its other duplicates
... In action
action_merge_duplicated_line()
of button "Merge Duplicates", the duplicate lines need to be deleted before writing the total quantity on the kept line, as the write method checks for duplicates, which may trigger that "You cannot have two inventory adjustments..." exception; see Odoo core https://github.com/odoo/odoo/blob/6b24df0e037d64be0d4e48045e47215059bffddb/addons/stock/models/stock_inventory.py#L406. Note that ironically this button method is intended to merge duplicate lines, for the purpose of preventing that exception.