-
-
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
[16.0][IMP] stock_inventory: various fixes #2017
Conversation
1404c71
to
aba201a
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.
We found that the problem is generated by this other module https://github.com/OCA/stock-logistics-warehouse/blob/16.0/stock_inventory_preparation_filter/views/stock_inventory_view.xml. We'll fix after merging this one. |
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.
👍
self.env["stock.inventory"].browse( | ||
self.env.context.get("active_id") | ||
).refresh_stock_quant_ids() | ||
return res |
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.
Can't we simply add context default values and add related fields in the tree view? Like I did in the previous PR. Also you should use model_create_multi.
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, I've changed the model_create_multi, however I don't understand exactly what you refer by context default values. You mean creating the quant with default values automatically?
If you agree, we could merge this PR with the alignment changes and you can create a new PR with your changes added correctly. If not please tell me exaclty what do you mean please. 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.
@DavidJForgeFlow I think this is what it is about:
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've added the default_to_do as stock_inventory_ids is no longer in quants. I think we need to review if this fields is still needed, IMO it's not longer needed.
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.
do you agree @benwillig ?
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'm not a fan of this implementation. I wasn't a fan of what I did based on your previous algorithm but it was cleaner in my opinion. I should take time to analyze this and also include the rest of my PR, because here the merge is incomplete so I have things to to that was not planned.
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 Based on this response I think it is best to split this PR, between the strict fwport from v15 and the adjustments done by @benwillig in which we could continue this discussion.
I would like to have v15 and v16 aligned to unlock the migration of v16 cycle count.
313531a
to
b950600
Compare
@DavidJForgeFlow #2029 is merged, can you rebase this one? |
b950600
to
b381744
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.
Code and functional review. LGTM!
/ocabot merge minor |
@LoisRForgeFlow The merge process could not start, because command
|
/ocabot merge minor |
@LoisRForgeFlow The merge process could not start, because command
|
/ocabot merge minor |
1 similar comment
/ocabot merge minor |
What a great day to merge this nice PR. Let's do it! |
This PR looks fantastic, let's merge it! |
@LoisRForgeFlow your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-2017-by-LoisRForgeFlow-bump-minor. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
- fixed the way to link the move to the inventory, loading all move lines linked to a product or a lot for a given location was not right - fixed computed methods - do not use ValidationError if not inside a python constrains - use states field attribute to avoid duplicating visibility condition - missing ensure_one - added cancel state to match with old odoo inventory
b381744
to
9acd4d9
Compare
Rebased after issues with tests got fixed in #2087 |
/ocabot merge minor |
@LoisRForgeFlow The merge process could not start, because command
|
/ocabot merge minor |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at 748b564. Thanks a lot for contributing to OCA. ❤️ |
Forward port of #1991 #2010.
Also adds some improvements from #1935 and supersede it.
From the last PR some changes are colliding with the ones applied in 15.0. I've merged the first two commits (be31060 and d4f3808) and the last two were removed due the conflict with the 15.0 changes.
The changes removed are the ones affecting to stock.quant, as the versions have changed a lot and some options (like creating quants that did not exist earlier) are now covered. Please take a look and if you miss any change I beg you for creating a new PR adding them!
@benwillig @LoisRForgeFlow