-
-
Notifications
You must be signed in to change notification settings - Fork 18
[18.0] [FIX] mass_mailing_custom_unsubscribe: Allow redirect to mailing list manager on unsubscribe #9
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: 18.0
Are you sure you want to change the base?
Conversation
| if ( | ||
| request.env["ir.config_parameter"] | ||
| .sudo() | ||
| .get_param("mass_mailing.mailing_lists_unsubscribe_redirect", "0") | ||
| == "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.
For booleans, it's simpler to just check if the param is there or not (and that's how it will work if you use it as a res.config.settings boolean config_parameter
| if ( | |
| request.env["ir.config_parameter"] | |
| .sudo() | |
| .get_param("mass_mailing.mailing_lists_unsubscribe_redirect", "0") | |
| == "1" | |
| ): | |
| if ( | |
| request.env["ir.config_parameter"] | |
| .sudo() | |
| .get_param("mass_mailing.mailing_lists_unsubscribe_redirect") | |
| ): |
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.
Actually you have odoo.tools.str2bool for this specific purpose.
| if ( | |
| request.env["ir.config_parameter"] | |
| .sudo() | |
| .get_param("mass_mailing.mailing_lists_unsubscribe_redirect", "0") | |
| == "1" | |
| ): | |
| if str2bool( | |
| request.env["ir.config_parameter"] | |
| .sudo() | |
| .get_param("mass_mailing.mailing_lists_unsubscribe_redirect", "0") | |
| ): |
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.
If you create the parameter but add no text to it, it will still be considered false, which goes against what the readme says.
I still suggest to use the str2bool approach. Otherwise, be more specific in the readme.
480f6c9 to
b6c8f24
Compare
… manager on unsubscribe When unsubscribing from a list, you cannot select a reason until you go to the mailing list manager. Now, you can choose with a system parameter to redirect directly to the mailing list manager and give the user in the first place an option to select a reason to opt out.
b6c8f24 to
03e0185
Compare
chienandalu
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 and functional review 👍
| if ( | ||
| request.env["ir.config_parameter"] | ||
| .sudo() | ||
| .get_param("mass_mailing.mailing_lists_unsubscribe_redirect", "0") | ||
| == "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.
If you create the parameter but add no text to it, it will still be considered false, which goes against what the readme says.
I still suggest to use the str2bool approach. Otherwise, be more specific in the readme.
| @@ -0,0 +1,4 @@ | |||
|
|
|||
| In order to redirect direct unsubscriptions to the Mailing Subscriptions form, | |||
| you need to create the system parameter `mass_mailing.mailing_lists_unsubscribe_manager_redirect`. | |||
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.
These are the docs I said above, that they are a bit misleading.
| you need to create the system parameter `mass_mailing.mailing_lists_unsubscribe_manager_redirect`. | |
| you need to create the system parameter `mass_mailing.mailing_lists_unsubscribe_manager_redirect` with value `1`. |
When unsubscribing from a list, you cannot select a reason until you go to the mailing list manager. Now, you can choose with a system parameter to redirect directly to the mailing list manager and give the user in the first place an option to select a reason to opt out.
https://www.loom.com/share/7801fec1afb24e1bb51c74b0c1fdbe1d
MT-12505 @moduon @yajo @EmilioPascual @chienandalu @rafaelbn please review if you want 😄