-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[4258][FIX] template_content_swapper #5
Conversation
@@ -16,7 +16,8 @@ def _render_template(self, template, values=None): | |||
result_str = str(result) | |||
lang_code = "en_US" | |||
request = values.get("request") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request = values.get("request") |
fd138f5
to
37f7227
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scope of _render_template()
is not clear to me when mega menu is involved. Is it processed on the mega menu only? If that is the case, is it still relevant for content swapping at all?
@yostashiro Our scope for content swapping is for all views and reports. So, _render_template() is called every time the view is rendered. |
I checked the log when I created the mega menu. It seems like the _render_templated is called twice and first one is in cache and there is no values. The first view is
|
@AungKoKoLin1997 My question was more from the business perspective. Mega menu is totally customizable IIUC, and if that is the case, would there be any point doing the content swapping at all? If there was no point, I thought one way of handling this case was to just return result if there there was no value. Not saying that this should be the conclusion. |
@yostashiro I got the point. When I quick check for other options in website and I see mega menu is only the case. So, I will take your suggestion. |
2c93e2e
to
05284ed
Compare
elif re.search(r'data-oe-lang="([^"]+)"', result_str): | ||
# For reports | ||
lang_match = re.search(r'data-oe-lang="([^"]+)"', result_str) | ||
if lang_match: | ||
lang_code = lang_match.group(1) | ||
lang_code = lang_match.group(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lang_match = re.search(r'data-oe-lang="([^"]+)"', result_str)
if lang_match:
# For reports
lang_code = lang_match.group(1)
lang_match = re.search(r'data-oe-lang="([^"]+)"', result_str) | ||
if lang_match: | ||
# For reports | ||
lang_match = re.search(r'data-oe-lang="([^"]+)"', result_str) | ||
if lang_match: | ||
lang_code = lang_match.group(1) | ||
lang_code = lang_match.group(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to correct many times...
else:
lang_match = re.search(r'data-oe-lang="([^"]+)"', result_str)
if lang_match:
# For reports
lang_code = lang_match.group(1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functioanl and Code review.
Could you please update the OCA PR.
4258