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.

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][MIG] stock_quant_manual_assign #1707

Merged
merged 39 commits into from
May 18, 2023

Conversation

yankinmax
Copy link
Contributor

@yankinmax yankinmax commented May 16, 2023

Open this PR to fast-track questions and comments asked on original migration PR created by @alexis-via here:

fanha99 and others added 30 commits October 29, 2022 20:36
* 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.
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/
There are cases where auto-filling of qty_done of stock move line is not desirable.
e.g. you assign quants manually for some of the moves in a picking and not the others,
in such case you need to go over all the moves in the picking to either remove qty_done
or fill it in to proceed with the validation of the entire moves. Auto-fill behavior is
also troublesome when this function is used in a manufacturing order. i.e. having
qty_done of the component move live messes up the outcome of the production.
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: stock-logistics-warehouse-14.0/stock-logistics-warehouse-14.0-stock_quant_manual_assign
Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-14-0/stock-logistics-warehouse-14-0-stock_quant_manual_assign/
Currently translated at 100.0% (31 of 31 strings)

Translation: stock-logistics-warehouse-14.0/stock-logistics-warehouse-14.0-stock_quant_manual_assign
Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-14-0/stock-logistics-warehouse-14-0-stock_quant_manual_assign/fr/
@jcadhoc
Copy link
Contributor

jcadhoc commented May 16, 2023

Hello @yankinmax !

I`m tryng to use this module in v16 but i have the following error: https://watch.screencastify.com/v/GBE9JageLOKSYH3EOqGX

Can you check it please? In the build is not working.

@yankinmax
Copy link
Contributor Author

hey @jcadhoc I'll check, thanks

@jcadhoc
Copy link
Contributor

jcadhoc commented May 16, 2023

@yankinmax Thank you!!

@yankinmax yankinmax force-pushed the 16.0-mig-stock_quant_manual_assign branch 4 times, most recently from 18a9a34 to dd4d5da Compare May 16, 2023 15:37
@@ -128,7 +139,6 @@ class AssignManualQuantsLines(models.TransientModel):
assign_wizard = fields.Many2one(
comodel_name="assign.manual.quants",
string="Move",
required=True,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't need here required as it forces an error when _onchange_selected is played
the field assign_wizard will be anyway propagated with the value, because one2many creation is handled

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 be required, as we should assure DB integrity on one2many tables, so any other problem is because something is incorrect in other place. Maybe the problem is to have such field in the child model definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pedrobaeza thanks for quick response on this
Actually, I can't reproduce the problem locally with demo database.
What I definitely checked is the fact assign_wizard is filled with the value when wizard is created.
But, it's empty before assign_quants (Confirm button) is called, so it forces an error on runboat build.
Locally it's not the case and works properly.
Then, my guess to fix the issue with Command.create, just refactors the one2many creation.
So, I have no idea for the moment what can cause a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jcadhoc I've checked different implementations of one2many lines for a wizard.
The code is definitely ok.
IDK why there is an issue with the assign_wizard field being checked for required when onchange selected called.
It should be checked on creation.

Choose a reason for hiding this comment

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

@yankinmax I tested the version merged in 16.0 and got the same error:
#1736

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BT-jmichaud I've tried functional testing several times on real database and demo database. Locally I don't have such issue. On the runbot it exists. I have no idea why. If you know where the problem comes from please open a PR to fix that.
The way one2many field on the wizard is filled with data is ok.

@yankinmax
Copy link
Contributor Author

@jcadhoc issue is fixed, you can check and pls submit your approve in case it's ok

Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

Code review.

@yankinmax Could you improve your commit messages including module name in them ([IMP] stock_quant_manual_assign: (...)) ? Thanks

yankinmax added 2 commits May 17, 2023 11:09
when using an uom on move line which is different from product uom
new field introduced in v16 reserved_uom_qty
@yankinmax yankinmax force-pushed the 16.0-mig-stock_quant_manual_assign branch 2 times, most recently from ff3be6a to 4f241de Compare May 17, 2023 09:23
@yankinmax yankinmax force-pushed the 16.0-mig-stock_quant_manual_assign branch from 4f241de to 17c899e Compare May 17, 2023 09:41
Copy link

@SilvioC2C SilvioC2C left a comment

Choose a reason for hiding this comment

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

LGTM overall

Copy link
Contributor

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

LG

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1707-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 69d6e89 into OCA:16.0 May 18, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at d88d5fe. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.