Code review comment for lp:~openerp-dev/openobject-addons/trunk-account_check_writing_jam

Hello,

Some more remarks about the updated code.

account_voucher.py:
- lint remark: your imports should separate standard lib imports from openerp imports (and each category should be ordered), like
from lxml import etree
// intended void line
from openerp.addons.account_check_writing.amount_to_text_en import amount_to_text
from openerp.osv import osv, fields
- _make_journal docstring: seacrh -> search :)
- _get_currency: in account_voucher/account_voucher.py, it already returns self.pool.get('res.users').browse(cr, uid, uid, context=context).company_id.currency_id.id, the same line that you added in account_check_writing. Your override does not seem necessary to me.
- _get_amount_in_word:
-- do not lowerize currency.id.name; instead, in amount_to_text do a currency.upper() == 'USD' (I think uppercase is more standard)
-- the following code is not correct:
language = 'en'
if context.get('lang'):
    language = context.get('lang')[0:2]
it fails if context is not defined. Please add an "if context is None: context = {}" at the beginning of the function. You could also simplify the line using the 'get' default value: language = context.get('lang', 'en')[0:2]
- I think you could improve the columns name to ease readability / understandability:
-- rename allow_check to allow_check_writing, because it is a related field and having the same name ease the code readability
-- rename check_done into 'printed', not necessary to add a prefix, and if the column meaning is to tell that the check has been printed, then the variable name should be printed
-- minor remark: put 'journal_id' column definition above 'allow_check_writing' to have a consistent columns order
-- I saw you added and then removed check_number being a "new field" based on number, using oldname, as I asked you in the first review. Why? Has the number field any meaning when having account_check_writing installed ? Should'nt check_number be number; basically ? Do you have any use of the two fields at the same time ?
- onchange_amount:
-- same remark about language = context.get(...)
-- same remark about currency.name.lower()
- onchange_journal: shouldn't the reset of currency_id also be applied if vals is void ?
- fields_view_get: if context == None -> if context is None
- print_check:
-- same remark about if context and None
-- the management of ids is not clean.
--- you browse on ids[0]
--- in value, you add a protection about ids: for example 'id': ids and ids[0] or False
---> at the beginning of the method, if ids is void, return {}, the set check_id to ids[0] and use this variable through the whole method

account.py
- Why are you using SUPERUSER_ID when calling create_check_sequence? If you have some access rights issues, shouldn't they be solved by regular access rights ?

check_print.py
- N, STARS, LINEKEYS -> should be class variables with a trailing underscore, not global variables (call to N will therefore become self._N)
- if defaults == None -> if defaults is None
- You added docstrings, it is a good practice. However could you also tell the type of the variables ? For example, :param list lines: a list of my.object.lines to be added on the check to print

res_company.py
- set the help at next line, your code is too long in just one line
- some lint issues
- defaults: lambda *a: 'top' -> just 'top', not necessary to use a lambda here

The views seem correct, but I did not go into details. I saw some indentation issues, just be careful with indentation.

Best regards,