12.0 Fix "stock_inventory_merge"

[azoellner]

This branch contains three small fixes for module "stock_inventory_merge" in version 12.0:

• break from a loop
  using break instead of continue
  ... which will continue the loop, which is unnecessary in this case
• assign new "Complete With Zero" lines to current inventory
  using the inventory.id of the single inventory in the for inventory in self loop
  ... instead of self.id, which will work only for singleton browse record sets
• prevent "You cannot have two inventory adjustments..." error
  by 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.

[codecov]

Codecov Report

Merging #112 (93ebd39) into 12.0 (1674aac) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             12.0     #112   +/-   ##
=======================================
  Coverage   72.31%   72.31%           
=======================================
  Files          66       66           
  Lines        1510     1510           
=======================================
  Hits         1092     1092           
  Misses        418      418           

Impacted Files	Coverage Δ
stock_inventory_merge/models/stock_inventory.py	88.76% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1674aac...93ebd39. Read the comment docs.

[legalsylvain]

LGTM. A question inline.

thanks for your contribution.

[legalsylvain]

makes sense.

[legalsylvain]

indeed !

[legalsylvain]

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 !)

[azoellner]

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.

[legalsylvain]

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.

[azoellner]

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,

1. create a new inventory (say with manually added products), and start it adding a product,
2. duplicate this inventory,
3. merge the two of them, so that you get an inventory with duplicate lines, and finally
4. 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().]