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

[11.0][ADD] purchase_order_secondary_unit: New module to allow buy in secondary units #601

Merged

Conversation

sergio-teruel
Copy link
Contributor

This PR depends of OCA/product-attribute#398
cc @Tecnativa

Copy link
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.

Thank you @sergio-teruel Some little comments

@lmignon
Copy link
Contributor

lmignon commented Oct 26, 2018

@sergio-teruel Can you also check travis. Some dependencies are missing

@sergio-teruel sergio-teruel force-pushed the 11.0-PR_purchase_order_secondary_unit branch from e3ccbca to c7ee647 Compare October 26, 2018 07:09
@sergio-teruel
Copy link
Contributor Author

@lmignon Thanks!!. Changes done. The tests will fails until the OCA/product-attribute#398 has not been merged.

@sergio-teruel sergio-teruel force-pushed the 11.0-PR_purchase_order_secondary_unit branch from c7ee647 to 7f3ccfd Compare October 26, 2018 08:38
Copy link
Member

@tarteo tarteo left a comment

Choose a reason for hiding this comment

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

Code LGTM. Couldn't test it, Runbot is not working.

purchase_order_secondary_unit/__manifest__.py Show resolved Hide resolved
Copy link
Member

@tarteo tarteo left a comment

Choose a reason for hiding this comment

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

Tested the functionality and it works fine:

Can you make the labels consistent
image

@sergio-teruel
Copy link
Contributor Author

@tarteo Are you referring to changing the name form "Secondary uom" to "Secondary product unit of measure"?.. I think that this name is very long.... 😲

@tarteo
Copy link
Member

tarteo commented Nov 6, 2018

@sergio-teruel Yes, just so it's consistent with the other label.

@GSLabIt
Copy link

GSLabIt commented Nov 6, 2018

@tarteo seems not a big issue, secondary uom sound good as well, isn't it?

@GSLabIt
Copy link

GSLabIt commented Nov 14, 2018

Ready to merge?

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.

Minor comment. LGTM

purchase_order_secondary_unit/models/purchase_order.py Outdated Show resolved Hide resolved
Copy link
Member

@nikul-serpentcs nikul-serpentcs left a comment

Choose a reason for hiding this comment

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

Improve Code

@sergio-teruel sergio-teruel force-pushed the 11.0-PR_purchase_order_secondary_unit branch from 7f3ccfd to 9bdc268 Compare November 16, 2018 07:54
Copy link
Member

@nikul-serpentcs nikul-serpentcs 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 LGTM 👍

@pedrobaeza pedrobaeza added this to the 11.0 milestone Dec 5, 2018
@pedrobaeza pedrobaeza force-pushed the 11.0-PR_purchase_order_secondary_unit branch from 9bdc268 to 454e4c8 Compare December 5, 2018 11:44
@pedrobaeza pedrobaeza merged commit 88bcbea into OCA:11.0 Dec 5, 2018
@pedrobaeza pedrobaeza deleted the 11.0-PR_purchase_order_secondary_unit branch December 5, 2018 12:35
@rousseldenis
Copy link
Contributor

rousseldenis commented Dec 5, 2018

@pedrobaeza @sergio-teruel Did this PR not depending on OCA/product-attribute#398 as announced ?

@pedrobaeza
Copy link
Member

Well, not technically, but for coherence for not seeing 2 times the same field. Sorry for not checking before. Can we merge the other already?

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

Successfully merging this pull request may close these issues.

7 participants