12.0 mig mail debrand #321
Conversation
There's no commits preservation, as the module doesn't serve as is on v9.
There was a problem hiding this comment.
The change of strategy is not going to work as noupdate="1" records are not updated on secondary modules, so you should stick to python hack. It would be also good that you generate the README directly in this PR for saving one extra commit on merge and the lack of verification of the full composition.
|
About regenerating the README, you only need to install maintainer-tools in your laptop and run the command inside module folder: but if not, at least don't remove entire README and let the previous one for reducing the diffs (one removing the text and another adding it the other way). |
gdgellatly
force-pushed
the
12.0-mig-mail_debrand
branch
4 times, most recently
from
9f53dc0
to
8e8bc13
Compare
gdgellatly
force-pushed
the
12.0-mig-mail_debrand
branch
from
8e8bc13
to
0b84d92
Compare
|
Odoo changed templates inside modules and this module is not working for many templates. Example: https://github.com/odoo/odoo/blob/11.0/addons/auth_signup/data/auth_signup_data.xml#L131 Python hack will not work either unless better regex patterns are written. @gdgellatly @pedrobaeza @lreficent |
|
Just make a PR for those templates to add. They aren't inherited so no real
problem.
…On Tue, 11 Dec 2018, 10:48 PM Levent Karakaş, ***@***.***> wrote:
Odoo changed templates inside modules and this module is not working for
many templates.
Example:
https://github.com/odoo/odoo/blob/11.0/addons/auth_signup/data/auth_signup_data.xml#L131
becomes
https://github.com/odoo/odoo/blob/12.0/addons/auth_signup/data/auth_signup_data.xml#L279
Python hack will not work either unless better regex patterns are written.
@gdgellatly <https://github.com/gdgellatly> @pedrobaeza
<https://github.com/pedrobaeza> @lreficent <https://github.com/lreficent>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#321 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AE23kOa6L487JR5wBEtcAXZWuaBde03wks5u339pgaJpZM4YR7SI>
.
|
|
Actually that won't work. Will need a separate module for each module. Can
you give a definitive number of modules affected?
…On Tue, 11 Dec 2018, 11:01 PM Graeme Gellatly, ***@***.***> wrote:
Just make a PR for those templates to add. They aren't inherited so no
real problem.
On Tue, 11 Dec 2018, 10:48 PM Levent Karakaş, ***@***.***>
wrote:
> Odoo changed templates inside modules and this module is not working for
> many templates.
>
> Example:
>
>
> https://github.com/odoo/odoo/blob/11.0/addons/auth_signup/data/auth_signup_data.xml#L131
> becomes
>
> https://github.com/odoo/odoo/blob/12.0/addons/auth_signup/data/auth_signup_data.xml#L279
>
> Python hack will not work either unless better regex patterns are written.
>
> @gdgellatly <https://github.com/gdgellatly> @pedrobaeza
> <https://github.com/pedrobaeza> @lreficent <https://github.com/lreficent>
>
> —
> You are receiving this because you were mentioned.
>
>
> Reply to this email directly, view it on GitHub
> <#321 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AE23kOa6L487JR5wBEtcAXZWuaBde03wks5u339pgaJpZM4YR7SI>
> .
>
|
|
Well, this module shouldn't depend on those modules, so better to improve regexp for them. |
|
@pedrobaeza I took a quick look, its not too bad. The first xml replacement needs to stay. While the rest could all be covered by 3 or 4 fairly straightforward regex, I think a better solution is an etree xpath based element replacement in code. Just traverse to the node and remove and continue to each ancestor if it is empty. Some of these are in td's, some in tr's some in tables, some in div's. Just easier to xpath to the node containing the text, remove, check parent, if now empty recursively remove. There must be some point in the rendering that the full document is available for parsing. |
|
Yes, but once rendered, not by XML. |
|
Are you sure this module works? I'm getting mad trying with sending a quotation by email end the "Powered by Odoo" is not removed. the xpath view extension of the mail.mail_notification_paynow looks correct however it looks like the inherit is not applied. For instance if I change something in the mail.mail_notification_paynow template, I see it change in the email, but that last |
|
They did work just fine, but I must admit I didn't retest after being made
to change view priority from review. But anyway for a different reason we
need to go back to a python based approach because these brandings are not
just in mail, but about 7 other modules incl sale.
…On Wed, 26 Dec 2018, 4:38 PM Raphaël Valyi, ***@***.***> wrote:
Are you sure this module works? I'm getting mad trying with sending a
quotation by email end the "Powered by Odoo" is not removed. the xpath view
extension of the mail.mail_notification_paynow looks correct however it
looks like the inherit is not applied. For instance if I change something
in the mail.mail_notification_paynow template, I see it change in the
email, but that last where the branding stands is never removed...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#321 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AE23kHbPSQcJ8yYwNUk6jcFTSJaqbtf_ks5u8u8bgaJpZM4YR7SI>
.
|
|
I told about the change on the strategy about Python code, and now my fears come true... |
|
@pedrobaeza Well looking at it, actually its working absolutely identically to v11.0 (in fact better as it at least takes care of all mail cases) so it is nothing to do with strategy change or anything like that. It just so happens that the v11.0 module also didn't remove the powered by messages from templates either inside or outside of mail module. So picking on this PR and "strategy change" without checking is just a red herring, so you can be less fearful now. The functionality never existed to be ported. Now if we agree we do want to remove these Powered by messages, I have written the xml parsing to do so, the question is do we want the change? EDIT: I should not, there is a significant performance tradeoff here. Every email will require parsing and at least 1 xpath, whereas in the past only 1 single template was ever checked. |
|
@gdgellatly when I migrated this module in v10, I took the approach of replacing text in the mail rendering because of these things, and that's why I talked about from the very first moment. I still don't understand why this can't be done the same way: https://github.com/OCA/social/blob/10.0/mail_debrand/models/mail_template.py |
|
@pedrobaeza But you didn't. Except for one template. And that was before they were changed to qweb. And definitely the powered by messages were never removed. Now it is 8 templates all slightly differently formatted in 2 differing formats. Parsing xml is the right way for the html based templates, replacement is right for the qweb ones. I did a POC, works nicely. POC is at #344 |
|
@pedrobaeza I just went back and tested based off your v11. It doesn't work that way anymore for anything qweb based. It seems it now renders the template without the overarching qweb, and then adds them in during later based on user preferences. |
|
But mine is not v11, but v10. v11 version was done by Eficent. |
|
I am experiencing the same thing. I find no way to override those views.
|
12.0 mig mail debrand
Complete rewrite as mail.templates are now QWeb templates so simply overwrite using inheritance.
The main change is all default mail notifications are debranded, not just the main one.