Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(79)

Issue 3255042: Electronic Mail for Tryton

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 11 months ago by sharoonthomas
Modified:
9 years, 7 months ago
Reviewers:
bch, pheller, yangoon, ced, yangoon1
Visibility:
Public.

Description

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/

Patch Set 1 #

Total comments: 22

Patch Set 2 : Changes from CR of ced and pheller #

Patch Set 3 : Updated mailbox to avoid recursion #

Patch Set 4 : Minor Refactoring #

Total comments: 15

Patch Set 5 : Changes from last review #

Total comments: 22
Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -54 lines) Patch
M __tryton__.py View 1 2 3 4 1 chunk +1 line, -2 lines 0 comments Download
M electronic_mail.py View 1 2 3 4 2 chunks +3 lines, -3 lines 11 comments Download
M electronic_mail.xml View 1 2 3 4 3 chunks +11 lines, -34 lines 2 comments Download
M tests/test_electronic_mail.py View 1 2 3 4 5 chunks +9 lines, -15 lines 9 comments Download

Messages

Total messages: 8
pheller
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 ...
9 years, 11 months ago (2010-11-22 16:49:31 UTC) #1
ced
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' ...
9 years, 11 months ago (2010-11-22 16:57:22 UTC) #2
sharoonthomas
Thanks for reviewing.
9 years, 11 months ago (2010-11-23 12:47:03 UTC) #3
ced
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 ...
9 years, 11 months ago (2010-11-23 22:52:56 UTC) #4
yangoon1
9 years, 11 months ago (2010-11-24 12:15:52 UTC) #5
yangoon
9 years, 10 months ago (2010-12-31 12:38:01 UTC) #6
sharoonthomas
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': ...
9 years, 7 months ago (2011-03-07 21:28:37 UTC) #7
ced
9 years, 7 months ago (2011-03-23 10:42:16 UTC) #8
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b