Electronic Mail for Tryton

The Electronic mail module adds email storing capabilities to Tryton. Mailboxes have been implemented to which access can be controlled.

There is an related module which adds templating capabilities to this module and extends functionality. The template module is also available for review[1].

Sharoon Thomas
Openlabs Technologies & Consulting (P) LTD

[1] http://codereview.appspot.com/3224042/

[pheller, 2010-11-22 16:49:31]

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

http://codereview.appspot.com/3255042/diff/1/electronic_mail.py#newcode40
electronic_mail.py:40: super(Mailbox, self).__init__()
Do you plan to override the superclass __init__() ?  If not, this probably isn't
necessary.

http://codereview.appspot.com/3255042/diff/1/electronic_mail.py#newcode122
electronic_mail.py:122: super(ElectronicMail, self).__init__()
Do you plan to override the superclass __init__() ?  If not, this probably isn't
necessary.

[ced, 2010-11-22 16:57:22]

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

http://codereview.appspot.com/3255042/diff/1/electronic_mail.py#newcode39
electronic_mail.py:39: def __init__(self):
not used

http://codereview.appspot.com/3255042/diff/1/electronic_mail.py#newcode48
electronic_mail.py:48: _name = 'electronic_mail.mailbox-parent-mailbox'
mailbox-mailbox

http://codereview.appspot.com/3255042/diff/1/electronic_mail.py#newcode50
electronic_mail.py:50: mailbox_parent =
fields.Many2One('electronic_mail.mailbox', 'Parent',
simply parent

http://codereview.appspot.com/3255042/diff/1/electronic_mail.py#newcode52
electronic_mail.py:52: mailbox_child =
fields.Many2One('electronic_mail.mailbox', 'Child',
simply child

http://codereview.appspot.com/3255042/diff/1/electronic_mail.py#newcode101
electronic_mail.py:101: 'electronic_mail', 'Headers')
Not Tryton indent

http://codereview.appspot.com/3255042/diff/1/electronic_mail.py#newcode102
electronic_mail.py:102: digest = fields.Char('MD5 Digest')
size=32

http://codereview.appspot.com/3255042/diff/1/electronic_mail.py#newcode121
electronic_mail.py:121: def __init__ (self):
Not used

http://codereview.appspot.com/3255042/diff/1/electronic_mail.py#newcode185
electronic_mail.py:185: result[electronic_mail.id] = base64.encodestring(
Should be False if there is nothing

http://codereview.appspot.com/3255042/diff/1/electronic_mail.py#newcode195
electronic_mail.py:195: if data is False or data is None:
Should merge it with the attachment one.

http://codereview.appspot.com/3255042/diff/1/electronic_mail.py#newcode265
electronic_mail.py:265: :param mail: email object
Why not passing the email data?

http://codereview.appspot.com/3255042/diff/1/electronic_mail.py#newcode288
electronic_mail.py:288: 
We also require to have a method to retrieve original email

http://codereview.appspot.com/3255042/diff/1/electronic_mail.xml
File electronic_mail.xml (right):

http://codereview.appspot.com/3255042/diff/1/electronic_mail.xml#newcode16
electronic_mail.xml:16: ('add', ref('group_email_user')),
Not sure it is good to have "returns" inside eval.

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

http://codereview.appspot.com/3255042/diff/1/tests/__init__.py#newcode4
tests/__init__.py:4: from .test_electronic_mail import suite
. ?

http://codereview.appspot.com/3255042/diff/1/tests/test_electronic_mail.py
File tests/test_electronic_mail.py (right):

http://codereview.appspot.com/3255042/diff/1/tests/test_electronic_mail.py#ne...
tests/test_electronic_mail.py:38: self.groupsobj = POOL.get('res.group')
missing _

http://codereview.appspot.com/3255042/diff/1/tests/test_electronic_mail.py#ne...
tests/test_electronic_mail.py:45: [('name', '=', 'Electronic Mail User')])
Better to search with ir.model.data

http://codereview.appspot.com/3255042/diff/1/tests/test_electronic_mail.py#ne...
tests/test_electronic_mail.py:81: def test0010mailbox_security(self):
security -> read access right

http://codereview.appspot.com/3255042/diff/1/tests/test_electronic_mail.py#ne...
tests/test_electronic_mail.py:113: for user_id in expected_results.keys():
Why not using iteritems

http://codereview.appspot.com/3255042/diff/1/tests/test_electronic_mail.py#ne...
tests/test_electronic_mail.py:122: def test0020mail_create_security(self):
create access right

http://codereview.appspot.com/3255042/diff/1/tests/test_electronic_mail.py#ne...
tests/test_electronic_mail.py:175: message['From'] =
"pythonistas<@>openlabs.co.in"
You can perhaps use example.com which exists for such usage.

http://codereview.appspot.com/3255042/diff/1/tests/test_electronic_mail.py#ne...
tests/test_electronic_mail.py:178: http://www.python.org
missing ending /

[sharoonthomas, 2010-11-23 12:47:03]

Thanks for reviewing.

[ced, 2010-11-23 22:52:56]

http://codereview.appspot.com/3255042/diff/13001/__tryton__.py
File __tryton__.py (right):

http://codereview.appspot.com/3255042/diff/13001/__tryton__.py#newcode8
__tryton__.py:8: 'version': '1.8.0.1',
We leave 0.0.1 for developing version

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

http://codereview.appspot.com/3255042/diff/13001/__tryton__.py#newcode12
__tryton__.py:12: 'active': True,
there is no active in Tryton

http://codereview.appspot.com/3255042/diff/13001/electronic_mail.py
File electronic_mail.py (right):

http://codereview.appspot.com/3255042/diff/13001/electronic_mail.py#newcode182
electronic_mail.py:182: ) or False
I would prefer to get _get_email returning False

http://codereview.appspot.com/3255042/diff/13001/electronic_mail.py#newcode274
electronic_mail.py:274: ) or False,
I think "or False" is not required

http://codereview.appspot.com/3255042/diff/13001/electronic_mail.py#newcode274
electronic_mail.py:274: ) or False,
Why not put this code out of the {}, it will be more readable

http://codereview.appspot.com/3255042/diff/13001/electronic_mail.xml
File electronic_mail.xml (right):

http://codereview.appspot.com/3255042/diff/13001/electronic_mail.xml#newcode214
electronic_mail.xml:214: ('name', '=', 'user'),
Same remarks than with eval

http://codereview.appspot.com/3255042/diff/13001/tests/test_electronic_mail.py
File tests/test_electronic_mail.py (right):

http://codereview.appspot.com/3255042/diff/13001/tests/test_electronic_mail.p...
tests/test_electronic_mail.py:45: user_group_xml_ids =
self.ir_model_data_obj.search(
There is a method on ir.model.data: get_id

[sharoonthomas, 2011-03-07 21:28:37]

The required changes are done. Module will follow

http://codereview.appspot.com/3255042/diff/13001/__tryton__.py
File __tryton__.py (right):

http://codereview.appspot.com/3255042/diff/13001/__tryton__.py#newcode8
__tryton__.py:8: 'version': '1.8.0.1',
The problem is you cant test setup.py when the version initials are wrong
because it will search for dependencies which fail. Do you think setup.py shuld
be altered to handle this case?

On 2010/11/23 22:52:56, ced wrote:
> We leave 0.0.1 for developing version

http://codereview.appspot.com/3255042/diff/13001/__tryton__.py#newcode11
__tryton__.py:11: 'website': 'http://openlabs.co.in',
On 2010/11/23 22:52:56, ced wrote:
> missing ending /

Done.

http://codereview.appspot.com/3255042/diff/13001/__tryton__.py#newcode12
__tryton__.py:12: 'active': True,
On 2010/11/23 22:52:56, ced wrote:
> there is no active in Tryton

Done.

http://codereview.appspot.com/3255042/diff/13001/electronic_mail.py
File electronic_mail.py (right):

http://codereview.appspot.com/3255042/diff/13001/electronic_mail.py#newcode182
electronic_mail.py:182: ) or False
The problem is _get_email is reused by the function fields for Text part and
HTML part in electronicmail_template

If i make it false here then additional False whecking will be needed at that
level

Also encodestring will not accept a bool arg

On 2010/11/23 22:52:56, ced wrote:
> I would prefer to get _get_email returning False

http://codereview.appspot.com/3255042/diff/13001/electronic_mail.py#newcode274
electronic_mail.py:274: ) or False,
On 2010/11/23 22:52:56, ced wrote:
> Why not put this code out of the {}, it will be more readable

Done.

http://codereview.appspot.com/3255042/diff/13001/electronic_mail.xml
File electronic_mail.xml (right):

http://codereview.appspot.com/3255042/diff/13001/electronic_mail.xml#newcode214
electronic_mail.xml:214: ('name', '=', 'user'),
On 2010/11/23 22:52:56, ced wrote:
> Same remarks than with eval

Done.

http://codereview.appspot.com/3255042/diff/13001/tests/test_electronic_mail.py
File tests/test_electronic_mail.py (right):

http://codereview.appspot.com/3255042/diff/13001/tests/test_electronic_mail.p...
tests/test_electronic_mail.py:45: user_group_xml_ids =
self.ir_model_data_obj.search(
On 2010/11/23 22:52:56, ced wrote:
> There is a method on ir.model.data: get_id

Done.

[ced, 2011-03-23 10:42:16]

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.py
File electronic_mail.py (right):

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.py#newcode8
electronic_mail.py:8: from sys import getsizeof
only in 2.6

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.py#newcode33
electronic_mail.py:33: subscribed = fields.Boolean('Subscribed')
Could add a default method

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.py#newcode160
electronic_mail.py:160: value = u''
Should be a str not a unicode

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.py#newcode167
electronic_mail.py:167: 'email', filename[0:2], filename)
You should also use a 2 folders depth to store email like in attachement

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.py#newcode169
electronic_mail.py:169: with open(filename, 'r') as file_p:
You should read in binary mode

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.py#newcode201
electronic_mail.py:201: # Filename <DIRECTORY>/<DIGEST>
2 digest levels

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.py#newcode207
electronic_mail.py:207: with open(filename, 'w') as file_p:
Must write in binary mode

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.py#newcode223
electronic_mail.py:223: 'WHERE digest = %s AND collision !=0 '
space after !=

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.py#newcode240
electronic_mail.py:240: with open(filename, 'w') as file_p:
binary mode

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.py#newcode265
electronic_mail.py:265: mktime(parsedate(mail.get('date'))))
Will it be more readable with an if statement

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.py#newcode303
electronic_mail.py:303: 'electronic_mail':mail_id,
space after :

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.xml
File electronic_mail.xml (right):

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.xml#newcode4
electronic_mail.xml:4: <tryton>
XML file are indented with 4 spaces

http://codereview.appspot.com/3255042/diff/26001/electronic_mail.xml#newcode24
electronic_mail.xml:24: <field name="name" select="1"/>
indent also in CDATA

http://codereview.appspot.com/3255042/diff/26001/tests/test_electronic_mail.py
File tests/test_electronic_mail.py (right):

http://codereview.appspot.com/3255042/diff/26001/tests/test_electronic_mail.p...
tests/test_electronic_mail.py:57: def create_users(self, no_of_sets=1):
I don't understand because every users are the same

http://codereview.appspot.com/3255042/diff/26001/tests/test_electronic_mail.p...
tests/test_electronic_mail.py:65: created_users = [ ]
no space inside []

http://codereview.appspot.com/3255042/diff/26001/tests/test_electronic_mail.p...
tests/test_electronic_mail.py:68: tuple([
Don't understand

http://codereview.appspot.com/3255042/diff/26001/tests/test_electronic_mail.p...
tests/test_electronic_mail.py:90: {
Indent ???

http://codereview.appspot.com/3255042/diff/26001/tests/test_electronic_mail.p...
tests/test_electronic_mail.py:111: USER: 2
,

http://codereview.appspot.com/3255042/diff/26001/tests/test_electronic_mail.p...
tests/test_electronic_mail.py:135: user_o, user_r, user_w =
self.create_users(no_of_sets=1)[0]
safer: (user_o, user_r, user_w), = ...

http://codereview.appspot.com/3255042/diff/26001/tests/test_electronic_mail.p...
tests/test_electronic_mail.py:150: 'mailbox': mailbox
add ,

http://codereview.appspot.com/3255042/diff/26001/tests/test_electronic_mail.p...
tests/test_electronic_mail.py:151: },))
remove ,

http://codereview.appspot.com/3255042/diff/26001/tests/test_electronic_mail.p...
tests/test_electronic_mail.py:219: )
strange indent