From 88b016fdc407e318c43c96df9b582853512f04fa Mon Sep 17 00:00:00 2001 From: Guillaume-gdi Date: Fri, 16 Feb 2024 12:11:38 +0100 Subject: [PATCH] [FIX] website, test_website: fix cache for navbar active element Since [this other commit], the style indicating the active nav item stopped working if the user added record pages to the navbar. This was due to the cache system not being invalidated when switching from one record page to another. This commit fixes the issue by disabling the cache for the navbar if there is a record page in the menu. Other potential solutions were considered but ultimately rejected: 1. Using the record as a t-cache key. However, this would mean that if you have 60,000 visible forum posts, you would end up with 60,000 different caches. 2. Activate the correct element using JavaScript. This would lead to a duplicate logic in the JavaScript and the Python code, and it would introduces a slight lag to add the active class on the correct nav item (due to the time it takes to load and execute the JavaScript). The chosen solution is the best compromise, as it maintains the cache for most cases (website menu without records page links in it), nothing change with this commit. For problematic cases (record pages in the website menu), this commit disables the cache, which is a reasonable trade-off. Steps to reproduce the bug fixed by this commit: - Edit a website's menu - Add a link to a product page (e.g., customizable-desk) - Add a link to another product (e.g., chair-floor-protection) - Save the menu - Click on the menu link to go to customizable-desk => At this point, the active menu element is correct - Click on the menu link to go to chair-floor-protection => The active menu element does not update This commit fixes the issue (a update of the website module is needed) and adds a test to prevent regressions. Notes: - To see the issue locally, remove the --dev xml or --dev all arguments. - The same issue was occurring with other record pages (blog posts, ..). - We will introduce back the groups on menu and benefit from this new method to also disable the menu cache if one of the menu is linked to a group. See task-3800830 [this other commit]: https://github.com/odoo/odoo/commit/b0a2a41d78292cb8b9e53788d40c6dc5915a466d opw-3694651 opw-3750925 opw-3781668 closes odoo/odoo#159006 X-original-commit: 43576cd424b6d0fc7da01142b5e6550e371ad1ff Related: odoo/enterprise#59321 Signed-off-by: Romain Derie (rde) Signed-off-by: Guillaume Dieleman (gdi) --- addons/test_website/tests/__init__.py | 1 + addons/test_website/tests/test_menu.py | 45 ++++++++++++++++++++++ addons/website/models/website.py | 8 ++++ addons/website/tools.py | 7 +++- addons/website/views/website_templates.xml | 2 +- 5 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 addons/test_website/tests/test_menu.py diff --git a/addons/test_website/tests/__init__.py b/addons/test_website/tests/__init__.py index 58eb63a6e22..e6b0379eb0c 100644 --- a/addons/test_website/tests/__init__.py +++ b/addons/test_website/tests/__init__.py @@ -8,6 +8,7 @@ from . import test_fuzzy from . import test_image_upload_progress from . import test_is_multilang from . import test_media +from . import test_menu from . import test_multi_company from . import test_page_manager from . import test_page diff --git a/addons/test_website/tests/test_menu.py b/addons/test_website/tests/test_menu.py new file mode 100644 index 00000000000..bba560dbc0f --- /dev/null +++ b/addons/test_website/tests/test_menu.py @@ -0,0 +1,45 @@ +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from lxml import html + +from odoo.addons.website.tools import MockRequest +from odoo.tests import tagged, HttpCase + + +@tagged('post_install', '-at_install') +class TestWebsiteMenu(HttpCase): + + def test_menu_active_element(self): + records = self.env['test.model'].create([{ + 'name': "Record 1", + 'is_published': True, + }, { + 'name': "Record 2", + 'is_published': True, + }]) + + controller_url = '/test_website/model_item/' + website = self.env['website'].browse(1) + + self.env['website.menu'].create([{ + 'name': records[0].name, + 'url': f"{controller_url}{records[0].id}", + 'parent_id': website.menu_id.id, + 'website_id': website.id, + 'sequence': 10, + }, { + 'name': records[1].name, + 'url': f"{controller_url}{records[1].id}", + 'parent_id': website.menu_id.id, + 'website_id': website.id, + 'sequence': 20, + }]) + for record in records: + record_url = f"{controller_url}{record.id}" + with MockRequest(self.env, website=website, url_root='', path=record_url): + tree = html.fromstring(self.env['ir.qweb']._render('test_website.model_item', { + 'record': record, + 'main_object': record, + })) + menu_link_el = tree.xpath(".//*[@id='top_menu']//a[@href='%s' and contains(@class, 'active')]" % record_url) + self.assertEqual(len(menu_link_el), 1, "The menu link related to the current record should be active") diff --git a/addons/website/models/website.py b/addons/website/models/website.py index 775654203f6..e4a07f98dfe 100644 --- a/addons/website/models/website.py +++ b/addons/website/models/website.py @@ -189,6 +189,14 @@ class Website(models.Model): def _get_menu_ids(self): return self.env['website.menu'].search([('website_id', '=', self.id)]).ids + @tools.ormcache('self.env.uid', 'self.id') + def is_menu_cache_disabled(self): + """ + Checks if the website menu contains a record like url. + :return: True if the menu contains a record like url + """ + return any(self.env['website.menu'].browse(self._get_menu_ids()).filtered(lambda menu: re.search(r"[/](([^/=?&]+-)?[0-9]+)([/]|$)", menu.url))) + @api.model_create_multi def create(self, vals_list): for vals in vals_list: diff --git a/addons/website/tools.py b/addons/website/tools.py index debfb113134..4dda141e59d 100644 --- a/addons/website/tools.py +++ b/addons/website/tools.py @@ -17,7 +17,7 @@ from odoo.tools.misc import hmac, DotDict, frozendict def MockRequest( env, *, path='/mockrequest', routing=True, multilang=True, context=frozendict(), cookies=frozendict(), country_code=None, - website=None, remote_addr=HOST, environ_base=None, + website=None, remote_addr=HOST, environ_base=None, url_root=None, # website_sale sale_order_id=None, website_sale_current_pl=None, ): @@ -41,6 +41,8 @@ def MockRequest( cookies=cookies, referrer='', remote_addr=remote_addr, + url_root=url_root, + args=[], ), type='http', future_response=odoo.http.FutureResponse(), @@ -51,6 +53,7 @@ def MockRequest( geoip={'country_code': country_code}, sale_order_id=sale_order_id, website_sale_current_pl=website_sale_current_pl, + context={'lang': ''}, ), geoip=odoo.http.GeoIP('127.0.0.1'), db=env.registry.db_name, @@ -63,6 +66,8 @@ def MockRequest( website=website, render=lambda *a, **kw: '', ) + if url_root is not None: + request.httprequest.url = werkzeug.urls.url_join(url_root, path) if website: request.website_routing = website.id diff --git a/addons/website/views/website_templates.xml b/addons/website/views/website_templates.xml index 20105434ec1..014859bd5d9 100644 --- a/addons/website/views/website_templates.xml +++ b/addons/website/views/website_templates.xml @@ -161,7 +161,7 @@ - xmlid,website,website.is_public_user() + None if website.is_menu_cache_disabled() else (xmlid,website,website.is_public_user())