Email Template for Tryton

[udono, 2010-11-23 16:57:25]

http://codereview.appspot.com/3224042/diff/1/template.py
File template.py (right):

http://codereview.appspot.com/3224042/diff/1/template.py#newcode28
template.py:28: #: is infact the source record to generate an electronic mail
This I do not understand. For me the template is the base record, which is
extended by real email. What is the goal with this design?

http://codereview.appspot.com/3224042/diff/1/template.py#newcode35
template.py:35: # Safely, the other fields are firectly from electronic_mail
itself
_s_afely ... _d_irectly

[ced, 2010-11-23 23:20:38]

http://codereview.appspot.com/3224042/diff/1/__init__.py
File __init__.py (right):

http://codereview.appspot.com/3224042/diff/1/__init__.py#newcode6
__init__.py:6: from .template import *
I'm not sure .import is compatible with 2.5

http://codereview.appspot.com/3224042/diff/1/__tryton__.py
File __tryton__.py (right):

http://codereview.appspot.com/3224042/diff/1/__tryton__.py#newcode7
__tryton__.py:7: 'description': '''Electronic mail template''',
The description is not very descriptive

http://codereview.appspot.com/3224042/diff/1/__tryton__.py#newcode11
__tryton__.py:11: 'website': 'http://openlabs.co.in',
missing ending /

http://codereview.appspot.com/3224042/diff/1/__tryton__.py#newcode12
__tryton__.py:12: 'active': True,
There is no active in Tryton

http://codereview.appspot.com/3224042/diff/1/electronic_mail.py
File electronic_mail.py (right):

http://codereview.appspot.com/3224042/diff/1/electronic_mail.py#newcode26
electronic_mail.py:26: result[name] = defaultdict(unicode).fromkeys(ids)
default value should be False

http://codereview.appspot.com/3224042/diff/1/template.py
File template.py (right):

http://codereview.appspot.com/3224042/diff/1/template.py#newcode37
template.py:37: 'Language Code', help='Expression to find the ISO langauge
code')
language

http://codereview.appspot.com/3224042/diff/1/template.py#newcode39
template.py:39: html = fields.Text('HTML Body')
html and plain will not come from electronic_mail ?

http://codereview.appspot.com/3224042/diff/1/template.py#newcode57
template.py:57: def get_engines(self):
Why not just let other modules inherit this method and append its own engine?

http://codereview.appspot.com/3224042/diff/1/template.py#newcode57
template.py:57: def get_engines(self):
By the way, I'm not sure we must have many engine.
Just choose one good (and if possible Genshi because already used)

http://codereview.appspot.com/3224042/diff/1/template.py#newcode102
template.py:102: return safe_eval(expression, {'self': record})
I would put directly record in context.

http://codereview.appspot.com/3224042/diff/1/template.py#newcode133
template.py:133: for field_name in simple_fields.keys():
You could use iteritems

http://codereview.appspot.com/3224042/diff/1/template.py#newcode164
template.py:164: message.attach(MIMEText(html, 'html'))
Should not test if plain and/or html exist before attaching them?

http://codereview.appspot.com/3224042/diff/1/template.py#newcode195
template.py:195: # The boolean for direct print in the tuple is useless for
emails
But I think it will be better to keep the current tuple, we could add later or
with a module new value that could be useful.

http://codereview.appspot.com/3224042/diff/1/template.py#newcode198
template.py:198: def render_and_save(self, template_id, record_ids):
Why saving and not sending?

http://codereview.appspot.com/3224042/diff/1/template.py#newcode209
template.py:209: print email_message.as_string()
print statement

http://codereview.appspot.com/3224042/diff/1/template.py#newcode218
template.py:218: The process involves identifying the tempalte which needs
template

http://codereview.appspot.com/3224042/diff/1/template.xml
File template.xml (right):

http://codereview.appspot.com/3224042/diff/1/template.xml#newcode120
template.xml:120: 
It will be good to have access right by groups

http://codereview.appspot.com/3224042/diff/1/trigger.py
File trigger.py (right):

http://codereview.appspot.com/3224042/diff/1/trigger.py#newcode9
trigger.py:9: class Trigger(ModelSQL, ModelView):
I don't find the default_ method elegant.
I think it will be more user-friendly and consistent, if you inherits from
trigger and create an "ir.trigger.email" (per example). Then you can control
that it will call the right method on the right model.

[sharoonthomas, 2011-03-07 21:40:02]

http://codereview.appspot.com/3224042/diff/1/template.py
File template.py (right):

http://codereview.appspot.com/3224042/diff/1/template.py#newcode28
template.py:28: #: is infact the source record to generate an electronic mail
Ref to our discussion for the records
http://www.tryton.org/~irclog/2010-11-23.log.html

On 2010/11/23 16:57:25, udono wrote:
> This I do not understand. For me the template is the base record, which is
> extended by real email. What is the goal with this design?

http://codereview.appspot.com/3224042/diff/1/template.py#newcode35
template.py:35: # Safely, the other fields are firectly from electronic_mail
itself
On 2010/11/23 16:57:25, udono wrote:
> _s_afely ... _d_irectly

Done.

http://codereview.appspot.com/3224042/diff/1/template.py#newcode37
template.py:37: 'Language Code', help='Expression to find the ISO langauge
code')
On 2010/11/23 23:20:38, ced wrote:
> language

Done.

http://codereview.appspot.com/3224042/diff/1/template.py#newcode39
template.py:39: html = fields.Text('HTML Body')
The HTML and PLAIN are function fields in electronic_mail which will just read
from the copy of original mail stored in FS. These are the fields to store the
template in the database. 

Does this need change? Could you suggest

On 2010/11/23 23:20:38, ced wrote:
> html and plain will not come from electronic_mail ?