[FW][FIX] website: fix cache for navbar active element #159361
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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]: odoo@b0a2a41 opw-3694651 opw-3750925 opw-3781668 X-original-commit: 8abbe4a
|
@Guillaume-gdi @rdeodoo this PR targets master and is the last of the forward-port chain. To merge the full chain, use
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port |
Guillaume-gdi
approved these changes
Mar 27, 2024
There was a problem hiding this comment.
I realize that there is an issue with this fix since the saas-16.4 fw port with odoo community. I will fix it with this other PR
@fw-bot r+
|
@Guillaume-gdi @rdeodoo linked pull request(s) odoo/enterprise#59467 not ready. Linked PRs are not staged until all of them are ready. |
robodoo
pushed a commit
that referenced
this pull request
Mar 28, 2024
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 #159361 X-original-commit: 8abbe4a Related: odoo/enterprise#59467 Signed-off-by: Romain Derie (rde) <rde@odoo.com> Signed-off-by: Guillaume Dieleman (gdi) <gdi@odoo.com>
robodoo
pushed a commit
that referenced
this pull request
Mar 28, 2024
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 #159361 X-original-commit: 8abbe4a Related: odoo/enterprise#59467 Signed-off-by: Romain Derie (rde) <rde@odoo.com> Signed-off-by: Guillaume Dieleman (gdi) <gdi@odoo.com>
robodoo
pushed a commit
that referenced
this pull request
Mar 28, 2024
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 #159361 X-original-commit: 8abbe4a Related: odoo/enterprise#59467 Signed-off-by: Romain Derie (rde) <rde@odoo.com> Signed-off-by: Guillaume Dieleman (gdi) <gdi@odoo.com>
Guillaume-gdi
deleted the
master-16.0-fix-cache-active-menu-item-gdi-0kRq-fw
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
you have 60,000 visible forum posts, you would end up with 60,000
different caches.
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:
=> At this point, the active menu element is correct
=> 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:
opw-3694651
opw-3750925
opw-3781668
Forward-Port-Of: #159191
Forward-Port-Of: #154358