From 1781041f13b66dfe91b3230794ffb6b2001d86c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Theys?= Date: Mon, 2 Sep 2019 11:43:04 +0000 Subject: [PATCH] [IMP] website, website_event, website_slides: add rel=canonical tag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The canonical tag is important for SEO, indeed it prevents search engines from indexing duplicate content. Reasoning ========= The choice has been made to create the canonical tag automatically depending on the request path, ignoring the query string, and manually prefixing the appropriate domain and language code. Indeed creating it manually for each resource would create a lot of code and potential mistakes. It is more dangerous to do it the generic way, but after investigation it appears that it is an acceptable trade-off since the vast majority of our routes are well built and already ready for this: - using query string only for minor features that do not change the main content - having the models, the ids, the pager and other important features in the path Override ======== It is still possible to override the default behavior by passing `canonical_params` manually to the view or to the different methods. This is done for `/event` because the only way to display Past Events is to add `date=old`. Languages ========= Fix an issue where it was possible for a bot to be on the URL without language code but to use a language that is not the default language. Adapt hreflang, because it: - must only be present on canonical pages - must always lead to canonical pages - should not be set if there is no alternate language Misc ==== task-1958075 closes #12532 Inspired by OCA module `website_canonical_url` courtesy of Jairo Llopis. closes odoo/odoo#35852 Signed-off-by: Jérémy Kersten (jke) Co-authored-by: Jairo Llopis Co-authored-by: Sébastien Theys --- addons/http_routing/models/ir_http.py | 24 +++-- addons/website/models/website.py | 102 ++++++++++++++---- addons/website/tests/test_base_url.py | 51 ++++++--- addons/website/views/website_templates.xml | 4 +- addons/website_event/controllers/main.py | 11 +- .../website_event/tests/test_event_website.py | 9 ++ .../website_slides_templates_homepage.xml | 2 +- 7 files changed, 154 insertions(+), 49 deletions(-) diff --git a/addons/http_routing/models/ir_http.py b/addons/http_routing/models/ir_http.py index c9d08f3c754..8ed7527232c 100644 --- a/addons/http_routing/models/ir_http.py +++ b/addons/http_routing/models/ir_http.py @@ -418,14 +418,16 @@ class IrHttp(models.AbstractModel): nearest_lang = not func and cls.get_nearest_lang(request.env['res.lang']._lang_get_code(path[1])) url_lang = nearest_lang and path[1] - # if lang in url but not the displayed or default language --> change or remove - # or no lang in url, and lang to dispay not the default language --> add lang - # and not a POST request - # and not a bot or bot but default lang in url - if ((url_lang and (url_lang != request.lang.url_code or url_lang == default_lg_id.url_code)) - or (not url_lang and request.is_frontend_multilang and request.lang != default_lg_id) - and request.httprequest.method != 'POST') \ - and (not is_a_bot or (url_lang and url_lang == default_lg_id.url_code)): + # The default lang should never be in the URL, and a wrong lang + # should never be in the URL. + wrong_url_lang = url_lang and (url_lang != request.lang.url_code or url_lang == default_lg_id.url_code) + # The lang is missing from the URL if multi lang is enabled for + # the route and the current lang is not the default lang. + # POST requests are excluded from this condition. + missing_url_lang = not url_lang and request.is_frontend_multilang and request.lang != default_lg_id and request.httprequest.method != 'POST' + # Bots should never be redirected when the lang is missing + # because it is the only way for them to index the default lang. + if wrong_url_lang or (missing_url_lang and not is_a_bot): if url_lang: path.pop(1) if request.lang != default_lg_id: @@ -440,6 +442,12 @@ class IrHttp(models.AbstractModel): path.pop(1) routing_error = None return cls.reroute('/'.join(path) or '/') + elif missing_url_lang and is_a_bot: + # Ensure that if the URL without lang is not redirected, the + # current lang is indeed the default lang, because it is the + # lang that bots should index in that case. + request.lang = default_lg_id + request.context = dict(request.context, lang=default_lg_id.code) if request.lang == default_lg_id: context = dict(request.context) diff --git a/addons/website/models/website.py b/addons/website/models/website.py index 29398c35872..338cdba591b 100644 --- a/addons/website/models/website.py +++ b/addons/website/models/website.py @@ -7,7 +7,9 @@ import logging import hashlib import re + from werkzeug import urls +from werkzeug.datastructures import OrderedMultiDict from werkzeug.exceptions import NotFound from odoo import api, fields, models, tools @@ -437,37 +439,42 @@ class Website(models.Model): # Languages # ---------------------------------------------------------- - def get_alternate_languages(self, req=None): + def _get_alternate_languages(self, canonical_params): + self.ensure_one() + + if not self._is_canonical_url(canonical_params=canonical_params): + # no hreflang on non-canonical pages + return [] + + languages = self.language_ids + if len(languages) <= 1: + # no hreflang if no alternate language + return [] + langs = [] - if req is None: - req = request.httprequest - default = self.get_current_website().default_lang_id.url_code shorts = [] - def get_url_localized(router, lang): - arguments = dict(request.endpoint_arguments) - for key, val in list(arguments.items()): - if isinstance(val, models.BaseModel): - arguments[key] = val.with_context(lang=lang) - return router.build(request.endpoint, arguments) - - router = request.httprequest.app.get_db_router(request.db).bind('') - for lg in self.language_ids: - lg_path = ('/' + lg.url_code) if lg.url_code != default else '' + for lg in languages: lg_codes = lg.code.split('_') - shorts.append(lg_codes[0]) - uri = get_url_localized(router, lg.url_code) if request.endpoint else request.httprequest.path - if req.query_string: - uri += u'?' + req.query_string.decode('utf-8') - lang = { + short = lg_codes[0] + shorts.append(short) + langs.append({ 'hreflang': ('-'.join(lg_codes)).lower(), - 'short': lg_codes[0], - 'href': req.url_root[0:-1] + lg_path + uri, - } - langs.append(lang) + 'short': short, + 'href': self._get_canonical_url_localized(lang=lg, canonical_params=canonical_params), + }) + + # if there is only one region for a language, use only the language code for lang in langs: if shorts.count(lang['short']) == 1: lang['hreflang'] = lang['short'] + + # add the default + langs.append({ + 'hreflang': 'x-default', + 'href': self._get_canonical_url_localized(lang=self.default_lang_id, canonical_params=canonical_params), + }) + return langs # ---------------------------------------------------------- @@ -827,6 +834,55 @@ class Website(models.Model): self.ensure_one() return self._get_http_domain() or super(BaseModel, self).get_base_url() + def _get_canonical_url_localized(self, lang, canonical_params): + """Returns the canonical URL for the current request with translatable + elements appropriately translated in `lang`. + + If `request.endpoint` is not true, returns the current `path` instead. + + `url_quote_plus` is applied on the returned path. + """ + self.ensure_one() + if request.endpoint: + router = request.httprequest.app.get_db_router(request.db).bind('') + arguments = dict(request.endpoint_arguments) + for key, val in list(arguments.items()): + if isinstance(val, models.BaseModel): + if val.env.context.get('lang') != lang.url_code: + arguments[key] = val.with_context(lang=lang.url_code) + path = router.build(request.endpoint, arguments) + else: + # The build method returns a quoted URL so convert in this case for consistency. + path = urls.url_quote_plus(request.httprequest.path, safe='/') + lang_path = ('/' + lang.url_code) if lang != self.default_lang_id else '' + canonical_query_string = '?%s' % urls.url_encode(canonical_params) if canonical_params else '' + return self.get_base_url() + lang_path + path + canonical_query_string + + def _get_canonical_url(self, canonical_params): + """Returns the canonical URL for the current request.""" + self.ensure_one() + return self._get_canonical_url_localized(lang=request.lang, canonical_params=canonical_params) + + def _is_canonical_url(self, canonical_params): + """Returns whether the current request URL is canonical.""" + self.ensure_one() + # Compare OrderedMultiDict because the order is important, there must be + # only one canonical and not params permutations. + params = request.httprequest.args + canonical_params = canonical_params or OrderedMultiDict() + if params != canonical_params: + return False + # Compare URL at the first rerouting iteration (if available) because + # it's the one with the language in the path. + # It is important to also test the domain of the current URL. + current_url = request.httprequest.url_root[:-1] + (hasattr(request, 'rerouting') and request.rerouting[0] or request.httprequest.path) + canonical_url = self._get_canonical_url_localized(lang=request.lang, canonical_params=None) + # A request path with quotable characters (such as ",") is never + # canonical because request.httprequest.base_url is always unquoted, + # and canonical url is always quoted, so it is never possible to tell + # if the current URL is indeed canonical or not. + return current_url == canonical_url + class BaseModel(models.AbstractModel): _inherit = 'base' diff --git a/addons/website/tests/test_base_url.py b/addons/website/tests/test_base_url.py index 421174d0c65..18d296f38e9 100644 --- a/addons/website/tests/test_base_url.py +++ b/addons/website/tests/test_base_url.py @@ -1,22 +1,40 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. -import odoo +from lxml.html import document_fromstring + import odoo.tests -@odoo.tests.tagged('-at_install', 'post_install') -class TestBaseUrl(odoo.tests.HttpCase): - def test_base_url(self): - ICP = self.env['ir.config_parameter'] - Website = self.env['website'] +class TestUrlCommon(odoo.tests.HttpCase): + def setUp(self): + super(TestUrlCommon, self).setUp() + self.domain = 'http://' + odoo.tests.HOST + self.website = self.env['website'].create({ + 'name': 'test base url', + 'domain': self.domain, + }) + lang_fr = self.env.ref('base.lang_fr') + lang_fr.write({'active': True}) + self.website.language_ids = self.env.ref('base.lang_en') + lang_fr + self.website.default_lang_id = self.env.ref('base.lang_en') + + def _assertCanonical(self, url, canonical_url): + res = self.url_open(url) + canonical_link = document_fromstring(res.content).xpath("/html/head/link[@rel='canonical']") + self.assertEqual(len(canonical_link), 1) + self.assertEqual(canonical_link[0].attrib["href"], canonical_url) + + +@odoo.tests.tagged('-at_install', 'post_install') +class TestBaseUrl(TestUrlCommon): + def test_01_base_url(self): + ICP = self.env['ir.config_parameter'] icp_base_url = ICP.sudo().get_param('web.base.url') - domain = 'https://www.domain.jke' - website = Website.create({'name': 'test base url', 'domain': domain}) # Test URL is correct for the website itself when the domain is set - self.assertEqual(website.get_base_url(), domain) + self.assertEqual(self.website.get_base_url(), self.domain) # Test URL is correct for a model without website_id without_website_id = self.env['ir.attachment'].create({'name': 'test base url'}) @@ -30,12 +48,19 @@ class TestBaseUrl(odoo.tests.HttpCase): self.assertEqual(with_website_id.get_base_url(), icp_base_url) # ...when the website is correctly set - with_website_id.website_id = website - self.assertEqual(with_website_id.get_base_url(), domain) + with_website_id.website_id = self.website + self.assertEqual(with_website_id.get_base_url(), self.domain) # ...when the set website doesn't have a domain - website.domain = False + self.website.domain = False self.assertEqual(with_website_id.get_base_url(), icp_base_url) # Test URL is correct for the website itself when no domain is set - self.assertEqual(website.get_base_url(), icp_base_url) + self.assertEqual(self.website.get_base_url(), icp_base_url) + + def test_02_canonical_url(self): + self._assertCanonical('/', self.domain + '/') + self._assertCanonical('/?debug=1', self.domain + '/') + self._assertCanonical('/a-page', self.domain + '/a-page') + self._assertCanonical('/en_US', self.domain + '/') + self._assertCanonical('/fr_FR', self.domain + '/fr/') diff --git a/addons/website/views/website_templates.xml b/addons/website/views/website_templates.xml index 373b8d524db..83c68dfb959 100644 --- a/addons/website/views/website_templates.xml +++ b/addons/website/views/website_templates.xml @@ -228,10 +228,12 @@ - + + + diff --git a/addons/website_event/controllers/main.py b/addons/website_event/controllers/main.py index edd01ebbe19..6ee8b01a42b 100644 --- a/addons/website_event/controllers/main.py +++ b/addons/website_event/controllers/main.py @@ -3,7 +3,7 @@ import babel.dates import re import werkzeug -import json +from werkzeug.datastructures import OrderedMultiDict from datetime import datetime, timedelta from dateutil.relativedelta import relativedelta @@ -28,6 +28,7 @@ class WebsiteEventController(http.Controller): searches.setdefault('type', 'all') searches.setdefault('country', 'all') + website = request.website def sdn(date): return fields.Datetime.to_string(date.replace(hour=23, minute=59, second=59)) @@ -63,7 +64,7 @@ class WebsiteEventController(http.Controller): ] # search domains - domain_search = {'website_specific': request.website.website_domain()} + domain_search = {'website_specific': website.website_domain()} current_date = None current_type = None current_country = None @@ -110,7 +111,7 @@ class WebsiteEventController(http.Controller): step = 10 # Number of events per page event_count = Event.search_count(dom_without("none")) - pager = request.website.pager( + pager = website.pager( url="/event", url_args={'date': searches.get('date'), 'type': searches.get('type'), 'country': searches.get('country')}, total=event_count, @@ -139,6 +140,10 @@ class WebsiteEventController(http.Controller): 'search_path': "?%s" % werkzeug.url_encode(searches), } + if searches['date'] == 'old': + # the only way to display this content is to set date=old so it must be canonical + values['canonical_params'] = OrderedMultiDict([('date', 'old')]) + return request.render("website_event.index", values) @http.route(['''/event//page/'''], type='http', auth="public", website=True, sitemap=False) diff --git a/addons/website_event/tests/test_event_website.py b/addons/website_event/tests/test_event_website.py index bc8e9b18a1e..b01c975bb71 100644 --- a/addons/website_event/tests/test_event_website.py +++ b/addons/website_event/tests/test_event_website.py @@ -2,6 +2,8 @@ from datetime import datetime, timedelta from odoo import fields from odoo.addons.event.tests.common import TestEventCommon +from odoo.addons.website.tests.test_base_url import TestUrlCommon +import odoo.tests class TestEventWebsiteHelper(TestEventCommon): @@ -35,3 +37,10 @@ class TestEventWebsite(TestEventWebsiteHelper): self.assertFalse(self.event_0.menu_id) self.event_0.website_menu = True self._assert_website_menus(self.event_0) + + +@odoo.tests.tagged('-at_install', 'post_install') +class TestUrlCanonical(TestUrlCommon): + def test_01_canonical_url(self): + self._assertCanonical('/event?date=all', self.domain + '/event') + self._assertCanonical('/event?date=old', self.domain + '/event?date=old') diff --git a/addons/website_slides/views/website_slides_templates_homepage.xml b/addons/website_slides/views/website_slides_templates_homepage.xml index af2f8bf9346..33f84388453 100644 --- a/addons/website_slides/views/website_slides_templates_homepage.xml +++ b/addons/website_slides/views/website_slides_templates_homepage.xml @@ -238,7 +238,7 @@ t-esc="tag_group.name"/>
-