Skip to content

11.0 mig website_logo - #470

Merged
pedrobaeza merged 22 commits into
OCA:11.0from
ernestotejeda:11.0-mig-website_logo
Jun 20, 2018
Merged

pedrobaeza merged 22 commits into
OCA:11.0from
ernestotejeda:11.0-mig-website_logo

Conversation

@ernestotejeda

@ernestotejeda ernestotejeda commented Jun 4, 2018

Copy link
Copy Markdown
Member

Comment thread website_logo/__manifest__.py Outdated
'views/website_templates.xml',
],
'qweb': [
],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid empty key.

Comment thread website_logo/__manifest__.py Outdated
"LasLabs, "
"Tecnativa, "
"Odoo Community Association (OCA)",
'website': 'http://www.agilebg.com',

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Replace website with oca website URL.

Comment thread website_logo/models/res_config.py Outdated

logo = fields.Binary(
string="Website logo", related="website_id.logo",
help="This field holds the logo for this website, showed in header."

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

no need to repeat params on related field

'/website_logo',
'/website_logo.png',
], type='http', auth="none", cors="*")
def website_logo(self, dbname=None, **kw):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

question: do we really need this whole machinery just to get the logo?
Why can't we simply get current website from request and grab logo value?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We can use current website method and obtain the current website, but this method, if it not find a website that has the current domain, return the firs website un database. But, with the current code, if it not find a website tha has the current domain, it try to find a website that has the 'localhost' domain. That is a differece. If it doesn't matter, we can use the method.

'/website_logo.png',
], type='http', auth="none", cors="*")
def website_logo(self, dbname=None, **kw):
imgname = 'website_nologo.png'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nologo image seems empty, am I wrong? Should we have a no logo image or the default odoo one?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is the text placed in Usage section of README.rst, it was written before the migration:

This addon supports multi-website. If no logo is defined, a transparent image is
placed, allowing to have no logo in one website, even if "Show logo" option is
set.

Name, favicon and logo of your website
</div>
</xpath>
<xpath expr="//div[@id='domain_setting']//field[@name='favicon']/.." position="after">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

shouldn't we state that png format is required?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's not requiredf AFAIK

@rafaelbn

rafaelbn commented Jun 5, 2018

Copy link
Copy Markdown
Member

cc @Tecnativa

@rafaelbn rafaelbn added this to the 11.0 milestone Jun 5, 2018
@pedrobaeza pedrobaeza mentioned this pull request Jun 5, 2018
38 tasks
@JGarcia-Panach

Copy link
Copy Markdown

Tested funcionally 👍

@JGarcia-Panach JGarcia-Panach left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Functional review 👍

@rafaelbn rafaelbn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Functionaly tested 👍

Please @chienandalu @simahawk @MeetKD could you make here last review in technical staff ? Thank you!

@yajo yajo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lol, I expected to see something more, but code is OK besides some unneeded whitespace and comments. It would be great if you can clean those.

Comment thread website_logo/__init__.py Outdated
@@ -1,4 +1,3 @@
# -*- coding: utf-8 -*-

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔥

Comment thread website_logo/controllers/__init__.py Outdated
@@ -1,3 +1,2 @@
# -*- coding: utf-8 -*-

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔥

Comment thread website_logo/controllers/main.py Outdated
@@ -1,8 +1,8 @@
# -*- coding: utf-8 -*-

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔥

Comment thread website_logo/models/__init__.py Outdated
@@ -1,4 +1,3 @@
# -*- coding: utf-8 -*-

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔥

Comment thread website_logo/models/website.py Outdated
@@ -1,4 +1,3 @@
# -*- coding: utf-8 -*-

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔥

Comment thread website_logo/tests/__init__.py Outdated
@@ -1,3 +1,2 @@
# -*- coding: utf-8 -*-

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔥

Comment thread website_logo/tests/test_logo.py Outdated
self.cont_obj = Website()
self.rec_id = self.env.ref('base.main_company')
self.image_val = 'Test'.encode('base64')
# self.image_val = 'Test'.encode('base64')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔥

Comment thread website_logo/views/res_config_view.xml Outdated
<field name="logo" widget="image"/>
</div>
</xpath>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔥

@pedrobaeza

Copy link
Copy Markdown
Member

@yajo is a pyromaniacal 😝

lasley and others added 7 commits June 13, 2018 19:42
* [FIX] website_logo:  show logo by domain

Use domain field to match a logo by website instead of the name field of the website
model.

* [FIX] website_logo update readme

The logo is now configured from 'Website admin' instead of 'Company settings'.
@ernestotejeda
ernestotejeda force-pushed the 11.0-mig-website_logo branch from a0273c3 to ec2156b Compare June 13, 2018 23:45
@pedrobaeza
pedrobaeza merged commit 5103505 into OCA:11.0 Jun 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.