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.

Sign up for GitHub

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

Jump to bottom

12.0 Fix "stock_inventory_merge" #112

Open
wants to merge 3 commits into
base: 12.0
Choose a base branch
from
Open

12.0 Fix "stock_inventory_merge" #112

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de67259
[FIX] Break from a loop
azoellner Apr 12, 2021
89bf633
[FIX] Assign new "Complete With Zero" lines to current inventory
azoellner Apr 12, 2021
93ebd39
[FIX] Prevent "You cannot have two inventory adjustments..." error
azoellner Jun 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions stock_inventory_merge/models/stock_inventory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,11 @@ def complete_with_zero(self):
item
) == self._get_inventory_line_keys(product_line):
found = True
continue
break
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

indeed !

line_obj.create(product_line)

@api.multi
Expand Down Expand Up @@ -109,14 +109,18 @@ def action_merge_duplicated_line(self):
uom_obj.browse(default_uom_id),
)

# Update the first line with the sumed quantity
keeped_line = line_obj.browse(keeped_line_id)
keeped_line.write({"product_qty": sum_quantity})

# 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()`
Copy link
Member

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

Copy link
Author

@azoellner azoellner Jul 5, 2021 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Author

@azoellner azoellner Nov 7, 2021 •
edited
Loading

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,

  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().]

# which may trigger the "You cannot have two inventory
# adjustments..." exception.
keeped_line = line_obj.browse(keeped_line_id)
keeped_line.write({"product_qty": sum_quantity})

# Custom Section
@api.multi
def _get_duplicated_line_ids(self):
Expand Down