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

[12.0] [MIG] Product Brand #390

Merged
merged 25 commits into from
Jan 7, 2019
Merged

Conversation

ThomasBinsfeld
Copy link

Migration of product_attribute from 11.0 to 12.0.
#389

@ThomasBinsfeld ThomasBinsfeld force-pushed the 12-mig_product_brand_tbi branch 2 times, most recently from 203a5e7 to 6402016 Compare October 2, 2018 11:59
Copy link
Contributor

@acsonefho acsonefho left a comment

Choose a reason for hiding this comment

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

LGTM (about code)

Copy link
Member

@SimoRubi SimoRubi 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 and tested on runbot

product_brand/models/product_brand.py Outdated Show resolved Hide resolved
product_brand/readme/USAGE.rst 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.

Small Change

product_brand/__manifest__.py Show resolved Hide resolved
@pedrobaeza pedrobaeza added this to the 12.0 milestone Oct 11, 2018
@pedrobaeza
Copy link
Member

Please check Travis and runbot status

@OCA-git-bot OCA-git-bot mentioned this pull request Oct 11, 2018
24 tasks
@pedrobaeza
Copy link
Member

This also misses commits from #365

@pedrobaeza
Copy link
Member

@ThomasBinsfeld can you please review #364 and incorporate it here if everything is correct?

@pedrobaeza
Copy link
Member

#364 has been merged

bguillot and others added 18 commits October 25, 2018 09:31
* add smart button, move menu to a more visible position.
* show brand in product.template kanban and tree views.
* show brand in product variant kanban and tree views.
* add product_brand kanban view.
* update module's README and manifest file.
* Search and group by brand for both product.product and product.template.
* Convert model to new APIs.
* Refactor products_count computation using product_ids one2many field.
* Add public read access to product.brand (fixes 403 error on webshop for public user).
* Make brand name required.
Old form view was out of order: form blocks misaligned, because it was
not using Odoo 10 views styling and layout.
Lookup fails when the ID is formatted. The unformatted version of the
data is located under `raw_value`.
Axel-G and others added 6 commits October 25, 2018 09:31
Currently translated at 100,0% (21 of 21 strings)

Translation: product-attribute-11.0/product-attribute-11.0-product_brand
Translate-URL: https://translation.odoo-community.org/projects/product-attribute-11-0/product-attribute-11-0-product_brand/de/
… with other odoo apps. (OCA#364)

* Make the logo always the same width (64px).
* Remove the description (200 first caracters). Not relevant for a configuration model.
* Move the brand name and product count beside the image. This is the way it is displayed in partners and products kanban views.
@ThomasBinsfeld
Copy link
Author

@pedrobaeza All 11.0 commits are here now

@pedrobaeza
Copy link
Member

2 little pylint things:

product_brand/models/product_brand.py:31: [C8108(method-compute), ProductBrand] Name of compute method should start with "_compute_"
product_brand/reports/sale_report.py:15: [W0102(dangerous-default-value), SaleReport._query] Dangerous default value {} as argument

@pedrobaeza
Copy link
Member

@SimoRubi @nikul-serpentcs can you review again?

select_str += """
, sub.product_brand_id as product_brand_id
"""
return select_str
Copy link
Member

@bealdav bealdav Oct 28, 2018

Choose a reason for hiding this comment

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

can be summarized by

return super()._select() + ", sub.product_brand_id as product_brand_id"

Just typo, not blocking

The same below

Copy link
Author

Choose a reason for hiding this comment

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

@bealdav Thanks for your review. I don't think we should refactor code in migration PR if it's not required. Feel free to submit a refactoring in a PR after the migration merge.

Copy link
Member

Choose a reason for hiding this comment

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

OK. No problem

@SimoRubi
Copy link
Member

@ThomasBinsfeld please check runbot and Travis, everything is red

@ThomasBinsfeld ThomasBinsfeld force-pushed the 12-mig_product_brand_tbi branch 2 times, most recently from 1a51c33 to a97bd05 Compare October 29, 2018 09:36
Copy link
Member

@angelmoya angelmoya left a comment

Choose a reason for hiding this comment

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

👍

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.

Minor Change

def setUp(self):
super().setUp()
self.product = self.env.ref('product.product_product_4')
self.supplier = self.ref('base.res_partner_2')
Copy link
Member

Choose a reason for hiding this comment

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

@ThomasBinsfeld Here self.env.ref not self.ref

Copy link
Author

Choose a reason for hiding this comment

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

@nikul-serpentcs self.ref is a good way to get the id of the record and self.env.ref a good way to get the record itself. I won't change it since we only need the ID in this unit test. Please feel free to submit a refactoring PR after the migration merge.

Copy link
Member

@nikul-serpentcs nikul-serpentcs Dec 13, 2018

Choose a reason for hiding this comment

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

@ThomasBinsfeld In case, In future use supplier record then change the self.env.ref.
meanwhile, We change this(self.env.ref) and using self.supplier.id.

It's not a blocking issue but It's Good for us.

rather than, Code review LGTM.

Copy link
Author

Choose a reason for hiding this comment

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

@ThomasBinsfeld If you're review is non-blocking, you can update your review and add an approval. Thanks for the review.

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 Revie LGTM 👍

@youring
Copy link

youring commented Dec 19, 2018

Can pick #114, or add as an option?

@ThomasBinsfeld
Copy link
Author

ThomasBinsfeld commented Dec 19, 2018

@youring I don't plan to forward port improvements from previous versions in this migration. But feel free to open a pull request targeting my branch and I will review/merge it.

@ghost
Copy link

ghost commented Jan 7, 2019

Could this PR merged?

@pedrobaeza
Copy link
Member

Please squash migration commits together.

@ThomasBinsfeld
Copy link
Author

@pedrobaeza Done

@pedrobaeza pedrobaeza merged commit 8c31793 into OCA:12.0 Jan 7, 2019
@ThomasBinsfeld ThomasBinsfeld deleted the 12-mig_product_brand_tbi branch January 7, 2019 12:52
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.