Skip to content

Commit

Permalink
[FIX] website, test_website: fix cache for navbar active element
Browse files Browse the repository at this point in the history
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]: b0a2a41

opw-3694651
opw-3750925
opw-3781668

closes #159191

X-original-commit: 9c8c860
Related: odoo/enterprise#59399
Signed-off-by: Romain Derie (rde) <rde@odoo.com>
Signed-off-by: Guillaume Dieleman (gdi) <gdi@odoo.com>
  • Loading branch information
Guillaume-gdi committed Mar 26, 2024
1 parent 1b6a4ff commit 8abbe4a
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 2 deletions.
1 change: 1 addition & 0 deletions addons/test_website/tests/__init__.py
Expand Up @@ -8,6 +8,7 @@
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
Expand Down
45 changes: 45 additions & 0 deletions 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'].with_context(lang='en_US')._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")
8 changes: 8 additions & 0 deletions addons/website/models/website.py
Expand Up @@ -188,6 +188,14 @@ def _compute_menu(self):
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:
Expand Down
7 changes: 6 additions & 1 deletion addons/website/tools.py
Expand Up @@ -17,7 +17,7 @@
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,
):
Expand All @@ -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(),
Expand All @@ -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,
Expand All @@ -63,6 +66,8 @@ def MockRequest(
website=website,
render=lambda *a, **kw: '<MockResponse>',
)
if url_root is not None:
request.httprequest.url = werkzeug.urls.url_join(url_root, path)
if website:
request.website_routing = website.id

Expand Down
2 changes: 1 addition & 1 deletion addons/website/views/website_templates.xml
Expand Up @@ -155,7 +155,7 @@
<!-- website.is_public_user() is needed for menus having a page with
restricted visibility (only shown to logged in user): public users and
logged in users can't share the menu cache. -->
<attribute name="t-cache">xmlid,website,website.is_public_user()</attribute>
<attribute name="t-cache">None if website.is_menu_cache_disabled() else (xmlid,website,website.is_public_user())</attribute>
</xpath>

<xpath expr="//header" position="before">
Expand Down

0 comments on commit 8abbe4a

Please sign in to comment.