-
-
Notifications
You must be signed in to change notification settings - Fork 718
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
[17][MIG] stock_quant_manual_assign #2071
base: 17.0
Are you sure you want to change the base?
Conversation
/ocabot migration stock_quant_manual_assign |
@mav-adhoc Could you review #1973 instead ? |
@rousseldenis Hi! i can't review a PR in OCA but if you prefer i can close this PR. |
@mav-adhoc Ok, thanks. |
@mav-adhoc So, could you fix this ? |
* Better layout * Remove active_id dependency in some computed fields * Clean code * Refine constraint * Take into account if the current line is previously reserved before clicking on the button.
…ds + add security groups + hook
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: stock-logistics-warehouse-13.0/stock-logistics-warehouse-13.0-stock_quant_manual_assign Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-13-0/stock-logistics-warehouse-13-0-stock_quant_manual_assign/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: stock-logistics-warehouse-13.0/stock-logistics-warehouse-13.0-stock_quant_manual_assign Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-13-0/stock-logistics-warehouse-13-0-stock_quant_manual_assign/
97f7bc6
to
60d268e
Compare
@rousseldenis I'm working on this PR! Thanks! |
adbbb7a
to
7e289aa
Compare
@rousseldenis All tests approved! Could you review the PR 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.
Functional review, working ok!
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! Functional and code review.
ping @rousseldenis |
This PR has the |
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.
Functional review Ok !!
7e289aa
to
1915f9f
Compare
1915f9f
to
c67cdaf
Compare
c67cdaf
to
e5b0e65
Compare
@rousseldenis Suggested changes done! I think the PR is ready to be merged. |
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.
Thanks for the updates!
@@ -199,12 +195,12 @@ def _check_qty(self): | |||
for record in self.filtered("qty"): | |||
quant = record.quant_id | |||
move_lines = record.assign_wizard.move_id.move_line_ids.filtered( | |||
lambda ml: ( | |||
lambda ml, quant=quant: ( |
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.
Is there a reason for this change? Thought quant=quant
could be omitted.
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.
@yostashiro In fact no, in a lambda, each variable should be declared and not implicitly from external code.
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 don't see how it is an issue to omit it for this particular case, but I understand that if it's a matter of coding guideline.
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.
In fact it is good coding practice. Some linter would warn for it.
@antoniocanovas Can you review again please? Thanks! |
@rousseldenis I think the PR is ready to be merged! If you think i should make another change tell me! Thanks! |
@antoniocanovas Hi! Can you review the PR again please? Thanks! |
No description provided.