[IMP] website: fallback on odoo social by default in all cases

[rdeodoo]

There is a mechanism in place that will use the Odoo social media URLs as default social media URL for the websites.

It's done this way:

• base.main_company company has its social fields populated in XML in social_media module
• website depend of social_media
• website social fields have default value set to self.env.ref('base.main_company').social_xxx in its fields:

social_twitter = fields.Char('Twitter Account', default=_default_social_twitter)

But if one does empty those fields on the company, it won't bootstrap those values on website creation.
Maybe some other flows are possible to lead to those empty fields.

Seems like a low effort to be 100% sure we don't have empty fields by default.

[robodoo]

Pull request status dashboard.

[rdeodoo]

@JKE-be Seems like the only flow is when one is emptying manually the main company social fields then creating a website.
Deleting the default company is probably not possible due to sql constraint etc, so it's probably the only flow

[JKE-be]

Not fan to store in db some odoo url that could change in the future.
Imo make more sense to do it in python, e.g. in def social() controller as fallback, no ?

but really not my war.

[rdeodoo]

It's already hardcoded in the company fields with social_media module, but also stored in almost all websites since it's copied from the company (which is only False if manually emptied by user)

But indeed, it seems like an even better idea 👍
I'll change to something like this

    @http.route(['/website/social/<string:social>'], type='http', auth="public", website=True, sitemap=False)
    def social(self, social, **kwargs):
+        default_values = {
+            'facebook': 'https://www.facebook.com/Odoo',
+            'github': 'https://github.com/odoo',
+            'linkedin': 'https://www.linkedin.com/company/odoo',
+            'youtube': 'https://www.youtube.com/user/OpenERPonlin',
+            'instagram': 'https://www.instagram.com/explore/tags/odoo/',
+            # etc ...
+        }
-        url = getattr(request.website, 'social_%s' % social, False)
+        url = getattr(request.website, 'social_%s' % social, False) or default_values.get(social)
        if not url:
            raise werkzeug.exceptions.NotFound()
        return request.redirect(url, local=False)

[rdeodoo]

This is the diff to switch from default field values at website creation vs fallback when accessing the route.

Both seems ok.. Advantage now is that it can be changed easily and not stored, but disadvantage is that if someone empty the field, it will redirect on Odoo social media instead of 404, but in all cases people should not leave social link to a media that they empty so it's not a big deal imo..

[rdeodoo]

As discussed on Discord, a possible good solution would have been to fallback on https://odoo.com/website/social/<social> but since tiktok and instagram are not set on Odoo.com (and can't be set as UI rely on this not being set in custo to not show icons), it's not a possible solution at the end.

[rdeodoo]

@robodoo delegate=@bso-odoo

Could you have a final review and/or merge here?

[bso-odoo]

In the commit message:

• "depend of" => "depends on" ?
• "have default value set" => "have their default value set" ?
• "should not let" => "should not leave" ?
• "multiple time" => "multiple times"

Is this related to a task ?
Is this not a fix that avoids accidental 404 errors (rather than an imp) ?

[rdeodoo]

Fixed typo.

Is this related to a task ?

No, it's just a feedback from the internal team

Is this not a fix that avoids accidental 404 errors (rather than an imp) ?

I think it's a bit in between the fix and the imp, I'd say it tends more in the imp side tho, since there is no real bug but just inconvenience.

I also added a test

[bso-odoo]

Can't we avoid the security warning with something like:

field_name = f'social_{social}'
url = field_name in request.website and request.website[field_name] or self._get_social_default_values(social)

[rdeodoo]

We should not try to avoid security warning just to avoid security warning, it seems more nicely written with getattr on top of being less diff, right?

[bso-odoo]

Well, it's not "hack a way to avoid being detected", IMO it's applying their suggested alternative.
But let's ask for an override to minimize the diff. @odoo/rd-security Edit: the getattr call existed with the same parameters before the change.

[rdeodoo]

Note that we could also just decide to not protect the route and do request.website[f'social_{social}'], but since it was like that initially and just a stable quick seo imp, I wouldn't do it

[xmo-odoo]

I'd really rather this was switched to a field access, this is not a case where the getattr is actually needed / warranted in any way, it's just a bit more convenient. You could try to lobby rd-framework for a get method on BaseModel for this sort of situations tho.

Note that if you absolutely don't want to change this you can just add the fallback as a separate assignment.

[rdeodoo]

it's just a bit more convenient.

It's not about convenience, it's about the controller not returning a 500 error page if requested with a non existing social network. Maybe overkill, but seems like a very short / easy win? It would be easy to just turn it into a simple field access and let it crashes if unknown field, which is probably fine, but now that it was coded like that for ages.. Is it really worth changing?

Note that if you absolutely don't want to change this you can just add the fallback as a separate assignment.

Do you mean 👇 ?

-       url = getattr(request.website, 'social_%s' % social, False) or self._get_social_default_urls().get(social)
+       url = getattr(request.website, 'social_%s' % social, False)
+       if not url:
+           url = self._get_social_default_urls().get(social)
        if not url:
            raise werkzeug.exceptions.NotFound()
        return request.redirect(url, local=False)

Just to bypass the required security CI?
It seems to make the code worst just for the sake of bypassing it, isn't it?

Since the code was already there before (I am just adding a fallback), and since it's safe¹, it's worth keeping it as is? Less diff, easier to read, less verbose

I'll apply the diff you suggest if you don't want to override the CI (or if there is a real security issue), no problem tho 🤷

Footnotes

1. Well, one could access a social_x field on website which is not related to our social network set of fields, but if it's returning the info (can read it), it means there is other ways to get it too, and seems very unlikely ↩

[rdeodoo]

@odoo/rd-security Would you be able to validate the CI or bounce on the current discussion if you really want to?

As:

• XMO is on timoff
• there is actually no security issue here?
• the diff of this pr is just adding a fallback on top of the discussed code (so not really related)
• I don't want to bypass the security ci by writing (IMHO) code uglier than it could be

It would be nice if it can be approved

[xmo-odoo]

I'd really rather this was switched to a field access, this is not a case where the getattr is actually needed / warranted in any way, it's just a bit more convenient. You could try to lobby rd-framework for a get method on BaseModel for this sort of situations tho.

Note that if you absolutely don't want to change this you can just add the fallback as a separate assignment.

[mart-e]

I don't want to bypass the security ci by writing (IMHO) code uglier than it could be

The suggestion is not uglier, nor less readable, it’s just two different ways to write the same things but yes I can override if you prefer this way

@robodoo override=ci/security

and let’s remove this getattr in master please :)

I find it a bit odd to fallback on odoo social media btw, it’s incorrect for everybody except us 🤷
Why not just an empty string?

[rdeodoo]

I find it a bit odd to fallback on odoo social media btw, it’s incorrect for everybody except us 🤷

It's actually already the case for most website, as it's what is hardcoded in social_media module on the res.company(1,) in the xml data.
But some people are emptying this, then forget to update manually the website ones, and SEO tools / audits are returning that as an issue, because it means that the social network link will return a 404 page.
In the dom, the social links are stuff like /website/social/facebook which return the actual social network link by reading it on website.
It's done that way because we can't use any Qweb instruction in DOM in website pages / content, or it's not editable anymore.

Note that for those who empty those fields, they should also remove those media link/icons from the website footer (not only the footer), which most do.

[rdeodoo]

Note that if social_media was not hardcoding those URL itself on the company, and if by default website would not bootstrap on those company values, I would not have done this quick "fix".
It seems to just be tying everything together, avoiding complains from clients about those errors on SEO audits.

But if anyone think it's a bad idea, we could just close this PR, it's also a valid possibility.

About the behavior existing outside of this PR (social_media company + default website behavior), I think it might be a bit problematic for visitor to be redirected to Odoo social on client DBs as they could think it's the social media of the DB they are visiting. A bit like Odoo phone number which we removed from the source code for that exact reason.

[JKE-be]

quand il y a un doute, il n'y a pas de doute...

404 vs backlink to odoo.com (from probably demo website), One isn't better than the other.