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

Email Notification Customization #518

Open
wants to merge 102 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@SnorlaxYum
Copy link
Contributor

SnorlaxYum commented Dec 29, 2018

Mail Format Choose

Available options: plain, html, multipart. Default set to plain, so you don't need to config this in the mail block of the isso server conf if you mean to use plain text:

[mail]
...
format = plain

To use multipart, set the option to multipart in the mail block of the isso server conf:

[mail]
...
format = multipart

To use html, set the option to html in the mail block of the isso server conf:

[mail]
...
format = html

Title Customization

To customize the mail title, set like the following options in the mail block of the isso server conf:

[mail]
...
title_admin = {replier} commented on {title}
title_user = {receiver}, {replier} replied your comments on {title}

Result:

Mail Language

[mail]
...
language = en

#518 (comment)

Mail Customization

All three formats support customization, it helps to read the dafault configuration for details (isso/templates/comment.html for format = html, isso/templates/comment.plain for format = plain).

To use the feature, set the following options like this in the mail block of the isso server conf:

[mail]
...
template = /the/path/to/the/directory/or/a-single-template

If you set it to the path to a directory,
for format = html it will check if admin.html and user.html in that directory are available.
(For format = plain it will check admin.plain and user.plain;
For format = multipart, it will check admin.html and user.html for html part, admin.plain and user.plain for plain part)
The name of the file should be self-explaining. user.html and user.plain are used for reply notifications to the subcribed users, admin.html and admin.plain are used for mail notifications to the admin.
If one of these is missing then in the corresponding case the template will fall back to the default.
The log will tell you what happened when the error occurs. The template will be rendered by Jinja2.

Alternatively, you could just directly edit the default mail template without setting the option template.

Default template path for format = plain (Different if you set a different mail language using the option language):
When mail language is not set using language or set via language = en: isso/templates/comment.plain
When it's set any other language than en using language, take language = ja for example: isso/templates/comment_ja.plain

Default template path for format = html (Different if you set a different mail language using the option language):
When mail language is not set using language or set via language = en: isso/templates/comment.html
When it's set any other language than en using language, take language = ja for example: isso/templates/comment_ja.html

Default template path for format = multipart:
html part: the same as Default template path for format = html (listed above)
plain part: the same as Default template path for format = plain (listed above)

Templating Examples: #518 (comment)

HTML Styling suggestion: Using inline css as much as u can, cause webmail service like gmail will block any kind of css on the web other than the inline css. In imap <style> tag works in mail. Don't leave out the <html> out as this will add to spam score to your mail.

Format choose suggestion: Don't use format = html, the mail will have a higher spam score for having only html part. If you wanna use html, use format = multipart instead.

Plain Text Customization Suggestion: remember to tidy your text and remove every unnecessary spaces besides the comment you want (things like {#...#} won't be rendered), 'cause they will be rendered into the final mail.

Fallback Template: #518 (comment)

@SnorlaxYum

This comment has been minimized.

Copy link
Contributor Author

SnorlaxYum commented Dec 29, 2018

  • [Easy] Add a author name variable without email so the Sim<email>'s URL could be Sim's URL Now the author variable only shows the name, email address is used by the email variable if available
  • [Easy] Add The Config To Documentation
  • [Easy] Add them to the config file so WARNING: no such option won't pop.

SnorlaxYum added some commits Dec 30, 2018

Add the ability to add the replier's website URL to the reply notific…
…ation. To be consistent with isso default, the default reply notification format won't include the replier's website URL in both cases.
Add the ability to add the replier's website URL to the reply notific…
…ation. To be consistent with isso default, the default reply notification format won't include the replier's website URL in both cases.
@SnorlaxYum

This comment has been minimized.

Copy link
Contributor Author

SnorlaxYum commented Dec 30, 2018

Fallback Default when u don't set template

Note: I have these settings for title so title is not the fallback default.

[mail]
...
title_admin = {replier} commented on {title}
title_user = {receiver}, {replier} replied your comments on {title}
...

For the default mail format plain (Also the default plain part for multipart)


Should be the same as the current isso default.

For another format html (Also the default html part for multipart)

Reply Notification when replier has an URL:
Reply Notification
Reply Notification when replier doesn't submit an URL:
Reply Notification
The comment published without moderation and user's url:

The comment directly published with user's url:

The comment waiting for approval without user's url:

The comment waiting for approval with user's url:

@SnorlaxYum

This comment has been minimized.

Copy link
Contributor Author

SnorlaxYum commented Dec 30, 2018

  • [Easy]Title Customization
@matolivier

This comment has been minimized.

Copy link

matolivier commented Dec 30, 2018

Hi, why not adding the user website url as default behaviour in Isso and adding international translation for the text in current code?

@SnorlaxYum

This comment has been minimized.

Copy link
Contributor Author

SnorlaxYum commented Dec 30, 2018

Hi, why not adding the user website url as default behaviour in Isso and adding international translation for the text in current code?

I didn't change the default setting 'cause this may conflict with some users who are already satisfied with the default setting.
I'll consider the translation after doing this:

  • [Easy] Add ability to add parent comment URL to the notification url.
@SnorlaxYum

This comment has been minimized.

Copy link
Contributor Author

SnorlaxYum commented Dec 30, 2018

Templating Examples

Example 1: Single template

Setting in the server configuration:

[mail]
format=multipart
title_admin = {replier} commented on {title}
title_user = {receiver}, {replier} replied your comments on {title}
template = /home/sim/extemp.plain

It's how /home/sim/extemp.plain look like:

{% if part == "html" %}
<html>
<style>
.button {
  background-color: #555; 
  border: none;
  color: white;
  padding: 15px 32px;
  margin:32px;
  text-align: center;
  text-decoration: none;
  display: inline-block;
  font-size: 16px;
}
</style>

<p>{{author}}{% if email %} &lt;<a href="mailto:{{email}}">{{ email }}</a>&gt;{% endif %} wrote:</p>

<p>{{comment}}</p>

<p>{% if website %}{{author}}'s URL: <a href="{{website}}">{{website}}</a><br>{% endif %}
{% if admin %}IP address: {{ip}}{% endif %}</p>

<hr>
<p>
{% if admin %}
    <a href="{{com_link}}" class="button">See It</a>
    <a href="{{del_link}}" class="button">Delete</a>
    {% if mode == 2 %}
        <a href="{{act_link}}" class="button">Activate</a>
    {% endif %}
{% else %}
    <a href="{{parent_link}}" class="button">Your Comment</a>
    <a href="{{com_link}}" class="button">See It</a>
    <a href="{{unsubscribe}}" class="button">Unsubscribe</a>
{% endif %}
</p>
</html>
{% else %}{{author}} wrote:

{{comment}}

{% if email %}{{author}}'s Email: {{ email }}
{% endif %}{% if admin %}{% if website %}{{author}}'s URL: {{website}}
{% endif %}IP address: {{ip}}
{% else %}Your original comment: {{parent_link}}
{% endif %}Link to the comment: {{com_link}}

---
{% if admin %}Delete comment: {{del_link}}
{% if mode == 2 %}Activate comment: {{act_link}}
{% endif %}
{% else %}Unsubcribe from this conversation: {{unsubscribe}}
{% endif %}
{% endif %}

Result:

Admin notification (Html part):

Admin notification (Plain part):

Reply notification (Html part):

Reply notification (Plain part):

Example 2: Seperate htmls for admin and user

Setting in the server configuration:

[mail]
format = multipart
title_admin = {replier} commented on {title}
title_user = {receiver}, {replier} replied your comments on {title}
template = /home/sim/isso_temp

Then inside /home/sim/isso_temp:

admin.html:

{% extends "base.html" %}
{% block css %}a:hover{background-color: blue} {% endblock %}

{% block body_att %} style="background-image:url(http://hdqwalls.com/download/1/small-memory-lp.jpg);color:white"{% endblock %}

{% block info %}
<p>{{author}}{% if email %} &lt;<a style="color:#fff" href="mailto:{{email}}">{{ email }}</a>&gt;{% endif %} wrote:</p>

<p>{{comment}}</p>

<p>{% if website %}{{author}}'s URL: <a style="color:#fff" href="{{website}}">{{website}}</a><br>{% endif %}
IP address: {{ip}}
</p>
{% endblock %}

{% block button %}
	<a style="
  border: 1px dotted #fff;
  color: white;
  padding: 15px 32px;
  margin:5px;
  text-align: center;
  text-decoration: none;
  display: inline-block;" href="{{com_link}}" class="button">See It</a>
	<a style="
  border: 1px dotted #fff;
  color: white;
  padding: 15px 32px;
  margin:5px;
  text-align: center;
  text-decoration: none;
  display: inline-block;" href="{{del_link}}" class="button">Delete</a>
	{% if mode == 2 %}
		<a style=" 
  border: 1px dotted #fff;
  color: white;
  padding: 15px 32px;
  margin:5px;
  text-align: center;
  text-decoration: none;
  display: inline-block;" href="{{act_link}}" class="button">Activate</a>
	{% endif %}
{% endblock %}

user.html:

{% extends "base.html" %}
{% block css %}a:hover{background-color: red} {% endblock %}

{% block body_att %} style="background-image:url(http://hdqwalls.com/download/1/spiderman-into-the-spider-verse-new-china-poster-qa.jpg);color:white"{% endblock %}

{% block info %}

<p>{{author}}{% if email %} &lt;<a style="color:#fff" href="mailto:{{email}}">{{ email }}</a>&gt;{% endif %} replied to you:</p>

<p>{{comment}}</p>

{% if website %}<p>{{author}}'s URL: <a style="color:#fff" href="{{website}}">{{website}}</a><br></p>{% endif %}

{% endblock %}

{% block button %}
	<a style="
  border: 1px dotted #fff;
  color: white;
  padding: 15px 32px;
  margin:5px;
  text-align: center;
  text-decoration: none;
  display: inline-block;" href="{{parent_link}}" class="button">Your Comment</a>
	<a style="
  border: 1px dotted #fff;
  color: white;
  padding: 15px 32px;
  margin:5px;
  text-align: center;
  text-decoration: none;
  display: inline-block;" href="{{com_link}}" class="button">See It</a>
	<a style="
  border: 1px dotted #fff;
  color: white;
  padding: 15px 32px;
  margin:5px;
  text-align: center;
  text-decoration: none;
  display: inline-block;" href="{{unsubscribe}}" class="button">Unsubscribe</a>
{% endblock %}

base.html:

<html>
<style>
{% block css %}{% endblock %}
</style>
<body{% block body_att %}{% endblock %}>
<div style="background:rgba(0,0,0,0.5);color:#fff; text-align:center">
<h1><a style="color:white" href="{{thread_link}}">{{thread_title}}</a></h1>
{% block info %}{% endblock %}
<hr>
<div>
{% block button %}
{% endblock %}
</div>
</div>
</body>
</html>

The html part result (Srry but I'm tired of screenshoting, so two pics r enough for me):


The plain part will be the same as the plain default since isso couldn't find user.plain and admin.plain in the set path, have a look at #518 (comment)

The two example just shows you how to tango with it. The templating can be very customizable to your needs, depending on how you use block(for two-file situation, oh, in this situation you can also attain the best customizability by using two templates that do not depend on the same template) or if(for single-file situation).

@SnorlaxYum

This comment has been minimized.

Copy link
Contributor Author

SnorlaxYum commented Dec 30, 2018

To do:

  • Translation Ability

@matolivier What's the language u need? Or u do the translation of ur language for the fallback mail format?

@matolivier

This comment has been minimized.

Copy link

matolivier commented Dec 30, 2018

@SnorlaxYum, I apologize for my raw answer earlier which I should have expanded. Indeed the fact to change the text thanks to your work is an advantage, however the idea of translation is having the emails text ready to use for anyone instead of having to translate manually in conf. ;)

I would need french translation (I can provide the text).

@SnorlaxYum

This comment has been minimized.

Copy link
Contributor Author

SnorlaxYum commented Dec 30, 2018

Now it's possible to translate.
Just set language in [mail] section could do the trick.

language = zh

This will translate the default template and Anonymous:
Admin notification(Plain Part):

Admin notification(Html Part):

Reply notification(Plain Part):

Reply notification(Html Part):

Supported language: #518 (comment)

@SnorlaxYum

This comment has been minimized.

Copy link
Contributor Author

SnorlaxYum commented Dec 30, 2018

The checklist of the languages isso script has already got:

  • bg
  • cs
  • da
  • de
  • el
  • en
  • es
  • eo
  • fa
  • fi
  • fr
  • hr
  • hu
  • it
  • sv
  • nl
  • vi
  • ru
  • zh
  • zh_CN
  • zh_TW

The one whose checkbox is already ticked is now supported by the language option.

is the title of the thread. Available tags: `{title}`, `{receiver}`,
`{replier}`

admin_format_urluser_moderate

This comment has been minimized.

@jelmer

jelmer Dec 30, 2018

Collaborator

Can we perhaps use a real templating system for the e-mails, e.g. jinja2 ? That way, we don't end up with a plethora of configuration variables, one for each combination of tags that is available.

This comment has been minimized.

@SnorlaxYum

SnorlaxYum Dec 30, 2018

Author Contributor

Okay, I'll try looking into that. I also deem that plain text is not really flexible.

SnorlaxYum added some commits Jan 5, 2019

@SnorlaxYum SnorlaxYum force-pushed the SnorlaxYum:notification branch 3 times, most recently from 9695a6f to 21211d6 Jan 7, 2019

@SnorlaxYum

This comment has been minimized.

Copy link
Contributor Author

SnorlaxYum commented Jan 9, 2019

Any feedback? @jelmer

@jelmer

This comment has been minimized.

Copy link
Collaborator

jelmer commented Jan 18, 2019

Sorry for the slow reply. I think this is much better, but can you please remove most of the configuration options here? It's adding significant complexity to the code and without a clear benefit (i.e. the single digit percentage of users that want to modify the templates can just edit them).

@SnorlaxYum

This comment has been minimized.

Copy link
Contributor Author

SnorlaxYum commented Jan 19, 2019

Sorry for the slow reply. I think this is much better, but can you please remove most of the configuration options here? It's adding significant complexity to the code and without a clear benefit (i.e. the single digit percentage of users that want to modify the templates can just edit them).

This is what I just added to the top of this request:

Alternatively, you could just directly edit the default mail template without setting the option mail_template.

Default template path for mail_format = plain (Different if you set a different mail language using the option mail_language):
When mail language is not set using mail_language or set via mail_language = en: isso/templates/comment.plain
When it's set any other language than en using mail_language, take mail_language = ja for example: isso/templates/comment_ja.plain

Default template path for mail_format = html (Different if you set a different mail language using the option mail_language):
When mail language is not set using mail_language or set via mail_language = en: isso/templates/comment.html
When it's set any other language than en using mail_language, take mail_language = ja for example: isso/templates/comment_ja.html

Altogether, I think I've added these options:

  • mail_format
  • mail_title_admin
  • mail_title_user
  • mail_template
  • mail_language
  • anonymous and anonymous_xx for various mail_language (which I didn't mention in the doc for the sake of complexity)

There's no need in deleting the first 4 options.
mail_template seems a little optional (However it adds convenience if you update via git pull and forget to put the edited file in .gitignore, the custom path u set won't be affected if the path is out of the source directory).
anonymous is okay for me, adding to customizability even when another language is used.

Actually the six options can be optional. They all have fallback values. Maybe add another section for Mail Customization and include the six options?

@jelmer

This comment has been minimized.

Copy link
Collaborator

jelmer commented Jan 19, 2019

The issue I have with it is that it adds a lot of extra code paths that we'll need to keep working in the future. It's very easy for one of these code paths to regress since isso doesn't have a lot of unit tests - as we saw with 0.12.0.

If you insist on keeping these variables, then can you please:

  • add unit tests for each one of them
  • move them to a different section than "smtp"; perhaps "mail" or "notifications" ?
@SnorlaxYum

This comment has been minimized.

Copy link
Contributor Author

SnorlaxYum commented Jan 26, 2019

The issue I have with it is that it adds a lot of extra code paths that we'll need to keep working in the future. It's very easy for one of these code paths to regress since isso doesn't have a lot of unit tests - as we saw with 0.12.0.

If you insist on keeping these variables, then can you please:

* add unit tests for each one of them

* move them to a different section than "smtp"; perhaps "mail" or "notifications" ?

Sorry for my late reply. Just got back to this pull request and add one format multipart, 'cause a mail will have higher spam score in spamassassin for having only html part. multipart is the perfect solution for that.

I have moved the options to a new section [mail], and am wondering the unit tests you say. I guess you mean something like this in def __init__ of class SMTP:

    # test SMTP connectivity
    try:
        with SMTPConnection(self.conf):
            logger.info("connected to SMTP server")
    except (socket.error, smtplib.SMTPException):
        logger.exception("unable to connect to SMTP server")

In def format of class SMTP, I actually write something for debuging like this:

    if self.mail_lang != "en":
        com_ori = os.path.join(temp_path, "comment_%s.%s" % (self.mail_lang, part))
        try:
            jinjaenv.get_template(com_ori)
        except jinja2_exceptions.TemplateSyntaxError as err:
            logger.warn("[mail] Wrong format. %s"%err)
            logger.warn("[mail] Default template fell back to the one for en.")
        except jinja2_exceptions.TemplateNotFound:
            logger.warn("[mail] No default template for such language: %s. Default template fell back to the one for en." % self.mail_lang)
        except Exception as err:
            logger.warn("[mail] Some error about jinja2. %s"  % type(err))
            for er in err.args:
                logger.warn(      "%s" % er)
            logger.warn("[mail] Default template fell back to the one for en.")
        else:
            com_ori_admin = com_ori_user = os.path.basename(com_ori)

Or you mean something like test_*.py in isso/tests/?

@jelmer

This comment has been minimized.

Copy link
Collaborator

jelmer commented Jan 28, 2019

Or you mean something like test_*.py in isso/tests/?

Yep, those are the unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment