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

[MIG]Migrated shop_by_brand module in v9. #103

Merged
merged 10 commits into from
Apr 11, 2016
Merged

[MIG]Migrated shop_by_brand module in v9. #103

merged 10 commits into from
Apr 11, 2016

Conversation

JayVora-SerpentCS
Copy link
Sponsor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 38.516% when pulling 5464667 on JayVora-SerpentCS:9.0 into d6c3886 on OCA:9.0.

@pedrobaeza pedrobaeza mentioned this pull request Mar 31, 2016
17 tasks
.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/113/50
For further information, please visit:
Copy link
Member

Choose a reason for hiding this comment

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

Don't put this.

@pedrobaeza
Copy link
Member

Please fix PYLINT errors.

@ghost
Copy link

ghost commented Mar 31, 2016

@pedrobaeza Thanks for the updates.

Will do this changes asap.

Can you please help me to resolve the rst-syntax-error in travis ?

Regards.

@ghost
Copy link

ghost commented Mar 31, 2016

@pedrobaeza Can you please help me to fix the travis test regarding "Uses of a deprecated module openerp.osv" ?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 41.216% when pulling c89cd10 on JayVora-SerpentCS:9.0 into d6c3886 on OCA:9.0.

from openerp.http import request


class WebSite(orm.Model):
class WebSite(models.Model):
_inherit = 'website'

def sale_product_domain(self, cr, uid, ids, context=None):
Copy link
Member

Choose a reason for hiding this comment

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

Make this in new API (with @api.multi signature)

@pedrobaeza
Copy link
Member

Sorry for not answering before. I have been all day on travel.

I see that you have already figure out how to resolve the Travis problems.

@ghost
Copy link

ghost commented Apr 1, 2016

@pedrobaeza

Hope you are doing well.

Thanks for the updates.

I have updated the code as per your suggestion.

Regards.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 41.216% when pulling 759c4b6 on JayVora-SerpentCS:9.0 into d6c3886 on OCA:9.0.

@ghost
Copy link

ghost commented Apr 1, 2016

@pedrobaeza Can we proceed for merge ?

@pedrobaeza
Copy link
Member

You have my 👍, but we can't merge yet. We need at least one review more.

@atchuthan
Copy link
Member

Code review 👍 (It would be better to squash all the commits!!)

@ghost
Copy link

ghost commented Apr 2, 2016

@atchuthan Thanks for the review.

@yajo @rafaelbn Can you please review this PR ?

Thanks.

@@ -33,21 +37,35 @@ Based on this configuration, you will see the menuitem shop by brand under shop
It will show all the brands and once you select that brand it will show product's which
is related to this brand.

The blog here explains the HOWTO :

Choose a reason for hiding this comment

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

I remember a comment about it: should not we rather have the content of the blog in here? (Or at least a summary)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please put here the content of the blog post. We want all the documentation auto-contained.

Copy link

Choose a reason for hiding this comment

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

@pedrobaeza @elicoidal I will update readme with blog content.

Copy link

Choose a reason for hiding this comment

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

@pedrobaeza @elicoidal Actually summary of is already added in the description part of the readme file.

By going through the blog, I think we can add road map in readme file ?

What are your suggestions ?

Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Can't you make a summary of the post blog in the Usage section?

Copy link
Sponsor Author

Choose a reason for hiding this comment

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

@pedrobaeza @elicoidal Changed the usage section.

@elicoidal
Copy link

👍

domain.append(
('product_brand_id', '=', request.env.context['brand_id']))
('product_brand_id', '=', request.context['brand_id']))
Copy link
Member

Choose a reason for hiding this comment

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

When you are inside a model, you should use self.env.context. request.context is mainly for controllers.

Copy link

Choose a reason for hiding this comment

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

@yajo Thanks for the review, I will update the code soon.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 35.69% when pulling 6368d8c on JayVora-SerpentCS:9.0 into d6c3886 on OCA:9.0.


To use this module, you need to:

- Create Product brand, to create product brand
- Install the module website_sale_product_brand.
Copy link
Member

Choose a reason for hiding this comment

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

This goes in Installation section, but it's obvious, so you can omit it.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 35.69% when pulling 8cad4b9 on JayVora-SerpentCS:9.0 into d6c3886 on OCA:9.0.

@pedrobaeza
Copy link
Member

👍

@pedrobaeza pedrobaeza merged commit 3939a82 into OCA:9.0 Apr 11, 2016
**post)
result.qcontext['brand'] = brand
return result
context = dict(request.env.context)
Copy link
Member

Choose a reason for hiding this comment

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

we could use with_context() to update context values
self = self.with_context(brand_id= int(brand))

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.

None yet

6 participants