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

[11.0][MIG] packaging_uom #469

Merged
merged 10 commits into from
Nov 2, 2018
Merged

[11.0][MIG] packaging_uom #469

merged 10 commits into from
Nov 2, 2018

Conversation

jhumfer
Copy link

@jhumfer jhumfer commented Aug 28, 2018

Laetitia Gangloff and others added 9 commits August 28, 2018 07:50
rename packaging_extended to packaging_uom
94bb315
purchase_packaging : compute price_unit with the new uom / packaging_…
0db2078 
…management : update readme
correct product creation with packaging
4fe4151
@lmignon
[MIG] packaging_uom, purchase_packaging, sale_packaging: Migrated to …
5c63707 
…10.0
@lmignon
[IMP] packaging_uom: declare 'inverse' method on qty
a8b82a5 
This change is required to make the code working with existing addons without breaking tets. By declaring an invers on qty, we are able to assign/create the required uom to the package based on the expected qty
@rousseldenis
[IMP] Adds the glue which fill in the product packaging in stock
d90e361 
When a sale order is confirmed and product packaging is filled in
sale order line(s), the information is propagated through
procurement orders and stock moves.
This is conditioned by a parameter on procurement rule.
@oca-transbot
OCA Transbot updated translations from Transifex
8766ef3
@rousseldenis
[10.0][PYLINT] packaging_uom, sale_packaging, purchase_packaging
fdad3c3
@oca-travis
[UPD] Update packaging_uom.pot
b90ba92
@jhumfer jhumfer mentioned this pull request Aug 28, 2018
Closed
46 tasks
@pedrobaeza pedrobaeza added this to the 11.0 milestone Aug 28, 2018
@jhumfer
Copy link
Author

jhumfer commented Aug 28, 2018

stock_inventory_discrepancy has caused the test error

@rousseldenis
Copy link
Sponsor Contributor

@jhumfer You can make a PR with the commit contained in #467 for 11.0 branch. We'll merge it fast

@jhumfer
Copy link
Author

jhumfer commented Aug 28, 2018

@rousseldenis done!, thanks #470

@pedrobaeza
Copy link
Member

You can rebase now

@jhumfer
Copy link
Author

jhumfer commented Aug 29, 2018

@pedrobaeza rebase done, thanks!

rousseldenis
rousseldenis requested changes Aug 29, 2018
View reviewed changes
Copy link
Sponsor 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

packaging_uom/models/product_packaging.py Outdated
@@ -10,10 +9,10 @@ class ProductPackaging(models.Model):

@api.model
def _default_uom_categ_domain_id(self):
uom_id = self.env.context.get("get_uom_categ_from_uom")
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

@jhumfer You delete this behaviour but you let the context in view. Could you explain why you changed this ?

Copy link
Author

Choose a reason for hiding this comment

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

With the change to version 11, the product.packaging model has a product.product associated instead of product.template, causing the "get_uom_categ_from_uom" context to stop working.
I have modified it in view as well.

rousseldenis
rousseldenis reviewed Aug 29, 2018
View reviewed changes
packaging_uom/views/product_packaging_views.xml Outdated
@@ -11,7 +11,7 @@
<page name="packaging" string="Packaging" attrs="{'invisible':[('type','=','service')]}" groups="product.group_stock_packaging" >
<group name="packaging" string="Packaging">
<field name="packaging_ids" string="Configurations"
context="{'get_uom_categ_from_uom': parent.uom_id, 'tree_view_ref':'product.product_packaging_tree_view_product', 'form_view_ref': 'product.product_packaging_form_view_without_product'}"/>
context="{'tree_view_ref':'product.product_packaging_tree_view_product', 'form_view_ref': 'product.product_packaging_form_view_without_product'}"/>
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

@jhumfer Ok you deleted it, but I think 'default_product_id' is not defined when creating packaging in existing product form, so when does the domain function '_default_uom_categ_domain_id' work ?

The default uom was correctly loaded before.

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Have you tested it in the interface?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I have tested it in the interface and it works correctly. This ensures that 'default_product_id' is defined when creating the package in the existing product form

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Just tested in the interface.

Ok through product form.

KO through the product packages menu (create from scratch). The domain should be correctly computed after having selected the product.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed from product packages menu

Copy link
Author

Choose a reason for hiding this comment

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

Is there a problem for the validation?

@rousseldenis rousseldenis mentioned this pull request Oct 1, 2018
Closed
@jhumfer
Copy link
Author

jhumfer commented Oct 22, 2018

Is there a problem for the validation?

@jhumfer jhumfer closed this Oct 22, 2018
@jhumfer
Copy link
Author

jhumfer commented Oct 22, 2018

Is there a problem with the validation?

@rousseldenis
Copy link
Sponsor Contributor

@jhumfer There were problems with Github. Is it normal that you closed this ?

4 similar comments
@rousseldenis
Copy link
Sponsor Contributor

@jhumfer There were problems with Github. Is it normal that you closed this ?

@rousseldenis
Copy link
Sponsor Contributor

@jhumfer There were problems with Github. Is it normal that you closed this ?

@rousseldenis
Copy link
Sponsor Contributor

@jhumfer There were problems with Github. Is it normal that you closed this ?

@rousseldenis
Copy link
Sponsor Contributor

@jhumfer There were problems with Github. Is it normal that you closed this ?

@rousseldenis
Copy link
Sponsor Contributor

@jhumfer

@jhumfer jhumfer reopened this Oct 22, 2018
@jhumfer
Copy link
Author

jhumfer commented Oct 22, 2018

@jhumfer There were problems with Github. Is it normal that you closed this ?

It has been a mistake. I have reopened it. Why is not it validated?

@rousseldenis
Copy link
Sponsor Contributor

What do you want to say by 'validation' ?

rousseldenis
rousseldenis approved these changes Oct 22, 2018
View reviewed changes
Copy link
Sponsor 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

@jhumfer
Copy link
Author

jhumfer commented Oct 22, 2018

What do you want to say by 'validation' ?

I have solved the issues from your review. Would you kindly accept the pull request?

@rousseldenis
Copy link
Sponsor Contributor

What do you want to say by 'validation' ?

I have solved the issues from your review. Would you kindly accept the pull request?

Done !

@rousseldenis
Copy link
Sponsor Contributor

@OCA/logistics-maintainers

@pedrobaeza
Copy link
Member

Can you clean a bit commit history, removing that ugly merge operation?

@jhumfer
Copy link
Author

jhumfer commented Oct 30, 2018

Can you clean a bit commit history, removing that ugly merge operation?

Done, is this enough?

@rousseldenis
Copy link
Sponsor Contributor

@jhumfer I think you made mistake to your branch. Commits are duplicated.

If you clean, I think you can squash all commits after '[MIG] packaging_uom: Migration to 11.0' into that one. If you think they have their own existence, keep them.

@jhumfer
Copy link
Author

jhumfer commented Oct 31, 2018

@jhumfer I think you made mistake to your branch. Commits are duplicated.

If you clean, I think you can squash all commits after '[MIG] packaging_uom: Migration to 11.0' into that one. If you think they have their own existence, keep them.

Done, thanks!

@rousseldenis
Copy link
Sponsor Contributor

@jhumfer Quite done. You left a modification in stock_inventory_discrepancy/tests/test_inventory_discrepancy.py. Remove that one.

@rousseldenis
Copy link
Sponsor Contributor

👍

@rousseldenis
Copy link
Sponsor Contributor

@Cedric-Pigeon @lmignon

lmignon
lmignon approved these changes Nov 2, 2018
View reviewed changes
Copy link
Sponsor Contributor

@lmignon lmignon 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 only

@rousseldenis rousseldenis merged commit 459a4fd into OCA:11.0 Nov 2, 2018
@andrp92
Copy link

andrp92 commented May 29, 2019

The packaging view, from the product.template model, is still not working properly. The get_uom_categ_from_uom context is not working.

@rousseldenis
Copy link
Sponsor Contributor

@andrp92 I've done complete migration of these modules to v12, you can see RFC there : #604

Please feel free to test them. I'll do the fix and backport to v11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmignon lmignon lmignon approved these changes

@rousseldenis rousseldenis rousseldenis approved these changes

Assignees
No one assigned
Labels
needs fixing needs review
Projects
None yet
Milestone
11.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jhumfer @rousseldenis @pedrobaeza @andrp92 @lmignon @oca-transbot @oca-travis