-
-
Notifications
You must be signed in to change notification settings - Fork 699
[16.0][FIX] email_template_qweb: use language-aware env for rendering QWeb #1784
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
base: 16.0
Are you sure you want to change the base?
Conversation
thomaspaulb
left a comment
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 approved, just the commit message should change to the OCA standard:
[FIX] email_template_qweb: use language-aware env for rendering QWeb
More detailed information... (maybe can leave out in this case, summary is clear)
| "body_html" | ||
| ] = self_with_lang._render_template_postprocess( | ||
| {res_id: body_html} | ||
| )[ |
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.
Bit overkill to spread [res_id] over three lines.
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.
It looks that way due to the pylint but the res_id used is from the for loop.
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.
Maybe I was not clear, I just meant that lines 48 - 50 could be on one line, making it all a bit more compact and readable.
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.
updated the readability on the lines affected
NL66278
left a comment
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.
👍 One minor remark but for the rest LGTM
c36282d to
4ffe344
Compare
|
This PR has the |
|
@OCA/social-maintainers This is ready for merge |
|
@KKamaa Please remove the Draft: status of this PR, as that might stop people from merging. |
NL66278
left a comment
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.
👍 Already looked good. Now even better ;-)
fix bug where in email_template_qweb module: the QWeb renderer (ir.qweb) is instantiated once from the original environment (self.env) before the code switches into the per-language context: