[IMP] website: ignore anchor but not qs when comparing menu URL
- Ignore anchors, those are not sent to the server anyway, no way to compare even if we wanted to - Ensure query string (qs) are the same to be considered equals On top of that, it also fixes the case when the user inserted an absolute URL instead of a relative one, it will now match. task-3096367 opw-3091427 closes odoo/odoo#107782 Signed-off-by: Quentin Smetz (qsm) <qsm@odoo.com>
This commit is contained in:
Romain Derie
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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):
|
||||
|
||||
Reference in New Issue
Block a user