-
-
Notifications
You must be signed in to change notification settings - Fork 661
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
[MIG][10.0] product_brand #191
Conversation
Hey @loxamir, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
I Confirm I have signed the odoo CLA and send it by email |
[FIX] Author field meet limitation for type character varying(128)
Set sale as depence instead of product
everything is green now 😀 |
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.
Thank you for your port. There are only a few things missing (see comments).
Also you can replace 'openerp' in imports on models file with 'odoo'
@@ -207,7 +207,7 @@ | |||
name="Product Brands" | |||
id="menu_product_brand" | |||
action="action_product_brand" | |||
parent="base.menu_product"/> | |||
parent="sale.prod_config_main"/> | |||
|
|||
</data> | |||
</openerp> |
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.
s/openerp/odoo/
@@ -207,7 +207,7 @@ | |||
name="Product Brands" | |||
id="menu_product_brand" | |||
action="action_product_brand" | |||
parent="base.menu_product"/> | |||
parent="sale.prod_config_main"/> | |||
|
|||
</data> |
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.
can be removed
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.
what can be removed?
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.
<data>
tag
'category': 'Product', | ||
'summary': "Product Brand Manager", | ||
'author': 'NetAndCo, Akretion, Prisnet Telecommunications SA' | ||
', MONK Software, Odoo Community Association (OCA)' | ||
', SerpentCS Pvt. Ltd.', | ||
'license': 'AGPL-3', | ||
'depends': ['product'], | ||
'depends': [ | ||
'sale', |
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.
Are you sure it is correct? Aren't you inheriting views from product.template and product.template class?
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.
sale
depends on procurement
which depends on product
, so it should be 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.
But why do you need so high-level dependency?
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.
He uses it for the menu positioning, he uses sale.prod_config_main
as parent menu.
I'm not sure if there is a good lower-level menu to use as reference for positioning, do you have any proposals?
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 just depend on sale because of the menu item to keep it in the same place as older versions, if somebody have a better place, I will be glad to position the menu there and change that dependency
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.
OK, I see there's no better option here and I agree that brands are very related with sales.
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.
@loxamir Some things could be improved, but nothing blocking, IMO better to have it this way for now that not having it in v10.
The 'sale' dependency is strong, but I don't think there are a lot of people that would be interested in this module without the sale module installed
It would be great to improve the UI of the form view, fixing the smart button alignment:
@loxamir, please attend the comments and I'll merge the PR. Thanks for your contribution! |
@loxamir - do you plan on continuing with this PR? |
I make my contribution, what should I do now? |
You need to correct the comments we have made. |
I have made all the changes requested in comments except UI improvement which I let for anyone else do that |
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 @loxamir - would you mind adding the view improvement note to the roadmap so we don't lose it going forward?
And please squash your commits together |
@lasley how can I do that? |
@loxamir - There is a RoadMap section in the Readme, just add a bullet so we don't lose the bug note:
The squashing Pedro refers to is like this. We can do it for you too, but it's good for you to know how IMO. Squashing makes for way better commit histories, and works great in personal projects too. |
@@ -35,7 +35,7 @@ For further information, please visit: | |||
Known issues / Roadmap | |||
====================== | |||
|
|||
* Add a field with brands assiciated to a Customer or Supplier on | |||
* Add a field with brands associated to a Customer or Supplier on | |||
the Customers/Suppliers Form View. |
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.
Add the bullet here 😄
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.
done, thanks @lasley
FIX typo Update __manifest__.py [FIX] Author field meet limitation for type character varying(128) [FIX] Wrong depence Set sale as depence instead of product s/openerp/odoo Removed <data> tag Add view fix to roadmap
…sasa/product-attribute into migrate_product_brand_to_v10
I have made the squash, what more should I do? |
HI @loxamir - doesn't look like the squash went through, that should have reduced the commit count to 1 - we're still at 10. Not a big deal though, we can just squash on merge. Thanks for the submission, I look forward to seeing you around OCA! I don't have write access in this repo, but someone that does will squash & merge this soon. |
OK, squashing on merge then |
thanks @lasley @pedrobaeza and all other that help me make this contribution 👍 |
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
…when this method it's call from website (OCA#191)
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migration to 10.0
Migrate product_brand to V10