-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
[FIX] mrp: prevent byproduct missing record error #92728
[FIX] mrp: prevent byproduct missing record error #92728
Conversation
328743e
to
5fb3826
Compare
f02d44f
to
12a62be
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.
Fix seems fine except for my one comment and ideally you should add a test as well since byproducts don't have many tests and they seem to be quite buggy.
Also your commit message should be improved:
- There is no need to write what code change did since it is already in your code change.
- It would be better to say your fix is a modified version of the similar bugfix: c1768d0
- You don't really explain why the issue was occurring so its hard to understand your solution (see the similar bugfix for its explanation of why the issue is occurring)
- It should be "missing record error" instead of "missing error"
- rename your commit to something more specific and remove 2nd reference to mrp since its already there:
[FIX] mrp: prevent byproduct missing record error
addons/mrp/models/mrp_production.py
Outdated
if 'move_byproduct_ids' in vals and 'move_finished_ids' not in vals: | ||
vals['move_finished_ids'] = vals.get('move_finished_ids', []) + vals['move_byproduct_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.
If condition looks for 'move_finished_ids' not in vals
, then won't vals.get('move_finished_ids', [])
always be []
?
4a0792a
to
561484c
Compare
@ticodoo |
Hello, I believe you can combine your new test with Also:
|
Before this commit, there was a bug with MO by-product moves: - With a newly created MO, confirm MO and after manually add `Produced` quantity in By-Products and change quantity producing in MO and save the record then it raises the missing record error. same issues was fixed here odoo@c1768d0 but forgot to fix same in write method. so this commit is fix same but in write method TaskID - 2799848
561484c
to
41f4b0c
Compare
@ticodoo |
LGTM, thanks for your work! @robodoo r+ |
Before this commit, there was a bug with MO by-product moves:
same issues was fixed here c1768d0
but forgot to fix same in write method.
so this commit is fix same but in write method
TaskID - 2799848
PR - #92728