diff --git a/addons/http_routing/models/ir_http.py b/addons/http_routing/models/ir_http.py index 94db3fedc60..650de9ad74d 100644 --- a/addons/http_routing/models/ir_http.py +++ b/addons/http_routing/models/ir_http.py @@ -104,8 +104,8 @@ _UNSLUG_RE = re.compile(r'(?:(\w{1,2}|\w[A-Za-z0-9-_]+?\w)-)?(-?\d+)(?=$|\/|#|\? def unslug(s): - """Extract slug and id from a string. - Always return un 2-tuple (str|None, int|None) + """ Extract slug and id from a string. + Always return a 2-tuple (str|None, int|None) """ m = _UNSLUG_RE.match(s) if not m: diff --git a/addons/website/models/website_menu.py b/addons/website/models/website_menu.py index af54eea1430..1db9d28609a 100644 --- a/addons/website/models/website_menu.py +++ b/addons/website/models/website_menu.py @@ -4,6 +4,8 @@ import werkzeug.exceptions import werkzeug.urls +from werkzeug.urls import url_parse + from odoo import api, fields, models from odoo.addons.http_routing.models.ir_http import unslug_url from odoo.http import request @@ -139,10 +141,29 @@ class Menu(models.Model): return url def _is_active(self): + """ To be considered active, a menu should either: + + - have its URL matching the request's URL and have no children + - or have a children menu URL matching the request's URL + + Matching an URL means, either: + + - be equal, eg ``/contact/on-site`` vs ``/contact/on-site`` + - be equal after unslug, eg ``/shop/1`` and ``/shop/my-super-product-1`` + + Note that saving a menu URL with an anchor or a query string is + considered a corner case, and the following applies: + + - anchor/fragment are ignored during the comparison (it would be + impossible to compare anyway as the client is not sending the anchor + to the server as per RFC) + - query string parameters should be the same to be considered equal, as + those could drasticaly alter a page result + """ if not request: return False - request_url = unslug_url(request.httprequest.path) + request_url = url_parse(request.httprequest.url) if not self.child_id: # Don't compare to `url` as it could be shadowed by the linked @@ -150,13 +171,17 @@ class Menu(models.Model): menu_url = self._clean_url() if not menu_url: return False - menu_url = unslug_url(menu_url) - if request_url == menu_url: + + menu_url = url_parse(menu_url) + if unslug_url(menu_url.path) == unslug_url(request_url.path) and menu_url.decode_query() == request_url.decode_query(): + if menu_url.netloc and menu_url.netloc != request_url.netloc: + # correct path but not correct domain + return False return True else: # Child match (dropdown menu), `self` is just a parent/container, # don't check its URL, consider only its children - if any(request_url == unslug_url(child.url) for child in self.child_id if child.url): + if any(child._is_active() for child in self.child_id): return True return False diff --git a/addons/website/tests/test_menu.py b/addons/website/tests/test_menu.py index 471e124aa5f..1ea07e8005e 100644 --- a/addons/website/tests/test_menu.py +++ b/addons/website/tests/test_menu.py @@ -2,6 +2,10 @@ import json +from unittest.mock import Mock, patch +from werkzeug.urls import url_parse + +from odoo.addons.website.tools import MockRequest from odoo.tests import common @@ -113,6 +117,116 @@ class TestMenu(common.TransactionCase): default_menu.child_id[0].unlink() self.assertEqual(total_menu_items - 1 - self.nb_website, Menu.search_count([]), "Deleting a default menu item should delete its 'copies' (same URL) from website's menu trees. In this case, the default child menu and its copies on website 1 and website 2") + def test_06_menu_active(self): + Menu = self.env['website.menu'] + website_1 = self.env['website'].browse(1) + menu = Menu.create({ + 'name': 'Page Specific menu', + 'url': '/contactus', + 'website_id': website_1.id, + }) + + def url_parse_mock(s): + if isinstance(s, Mock): + # We end up in this case when `url_parse` is actually called on + # `request.httprequest.url`. This is simulating as if we were + # really calling the `_is_active()` method from this endpoint + # url. + return url_parse(self.request_url_mock) + return url_parse(s) + + def test_full_case(a_menu): + """ This method is testing all the possible flows about URL + matching: + - Same domain: + - no qs & no anchor: + - Same path -> Active + - Not same path -> Not active + - qs: + - same qs + - Same path -> Active + - Not same path -> Not active + - not same qs -> Not active + - Anchor + - Same path -> Active + - Not same path -> Not active + - Not same domain: -> Not active + It should receives a URL with no query string and no anchor. + """ + url = a_menu.url + self.request_url_mock = 'http://localhost:8069' + url + with MockRequest(self.env, website=website_1), \ + patch('odoo.addons.website.models.website_menu.url_parse', new=url_parse_mock): + self.assertTrue(a_menu._is_active(), "Same path, no domain, no qs, should match") + a_menu.url = f'{url}#anchor' + self.assertTrue(a_menu._is_active(), "Same path, no domain, no qs, should match (anchor should be ignored)") + a_menu.url = f'{url}?qs=1' + self.assertFalse(a_menu._is_active(), "Same path, no domain, qs mismatch, should not match") + self.request_url_mock = f'http://localhost:8069{url}?qs=2' + self.assertFalse(a_menu._is_active(), "Same path, no domain, qs mismatch (not the same val), should not match") + self.request_url_mock = f'http://localhost:8069{url}?qs=1' + self.assertTrue(a_menu._is_active(), "Same path, no domain, qs match, should match") + a_menu.url = f'http://localhost.com:8069{url}' + self.request_url_mock = f'http://example.com:8069{url}' + self.assertFalse(a_menu._is_active(), "Same path, domain mismatch, should not match") + self.request_url_mock = f'http://localhost.com:8069{url}' + self.assertTrue(a_menu._is_active(), "Same path, same domain, should match") + + # First, test the full cases with a normal top menu (no child) + test_full_case(menu) + + # Create the following menu structure: + # - 2 menus without children: `/` and `#` + # - 2 menus with children: `/` and `#`, both with a `/contactus` child + # + # menu (/) menu2 (/) menu3 (#) menu4 (#) + # - submenu (/contactus) - submenu2 (/contactus) + menu.url = '/' + menu2 = menu.copy() + menu3 = menu.copy({'url': '#'}) + menu4 = menu3.copy() + submenu = Menu.create({ + 'name': 'Page Specific menu', + 'url': '/contactus', + 'website_id': website_1.id, + 'parent_id': menu.id + }) + submenu2 = submenu.copy({'parent_id': menu3.id}) + + # Second, test a nested menu configuration (simple URL, no qs/anchor) + self.request_url_mock = 'http://localhost:8069/' + with MockRequest(self.env, website=website_1), \ + patch('odoo.addons.website.models.website_menu.url_parse', new=url_parse_mock): + self.assertFalse(menu._is_active(), "Same path but it's a container menu, its URL shouldn't be considered") + self.assertTrue(menu2._is_active(), "Same path and no child -> Should be active") + self.assertFalse(menu3._is_active(), "Not same path + children") + # Anchor menus are a mistake (unless for container menu) (and + # shouldn't even be possible to create from frontend), the user + # forgot to add the path (the menu won't work on pages without the + # anchor) so the system will prefix it by `/` for the check. This + # will then become `/#` and since anchors are ignored for the check, + # this will match. + self.assertTrue(menu4._is_active(), "Should match, see comment in code") + self.assertFalse(submenu._is_active(), "Not same path (2)") + self.assertFalse(submenu2._is_active(), "Not same path (3)") + + self.request_url_mock = 'http://localhost:8069/contactus' + self.assertTrue(menu._is_active(), "A child is active (submenu)") + self.assertFalse(menu2._is_active(), "Not same path (4)") + self.assertTrue(menu3._is_active(), "A child is active (submenu2)") + self.assertFalse(menu4._is_active(), "Not same path (5)") + self.assertTrue(submenu._is_active(), "Same path") + self.assertTrue(submenu2._is_active(), "Same path (2)") + + # Third, do the same test as the first one but with a child menu, to + # ensure the behavior remains the same regardless if it is a top menu or + # a child menu + test_full_case(submenu) + # Fourth, do the same test again with a slugified URL, to be sure the + # anchor and query string are not messing with the slug url compare + submenu.url = '/sub/slug-3' + test_full_case(submenu) + class TestMenuHttp(common.HttpCase): def setUp(self):