openerp-community team mailing list archive

[Virgil Dupras <virgil.dupras@xxxxxxxxxxxxxxxxxxxx>]

Review: Approve

Not being familiar with the module and not being able to setup a local environment to verify that it actually works, I could only look at the diff and search for code smells. These are my remarks:

- Why not use Python's built-in csv module to produce the tsv in hr_expense_expense._create_csv_report()?
- There are lines which are commented out. That shouldn't be there.
- In hr_expense_expense._get_cur_account_manager(): that logic for getting a comma-separated list of emails is not idomatic and overly complicated. The idiomatic way to do that in Python is to have a list of emails to join into a string and call ','.join(email_list)
- Whole templates in PO files: It seems like a weird idea to have our "A new expense has been approved and is ready for export.." HTML templates translated through gettext. Maybe it's the best way we found, but I thought it worthy of mention.

Otherwise, the code looks fine to me.
-- 
https://code.launchpad.net/~savoirfairelinux-openerp/openerp-accountedge/6.1-initial-version/+merge/175949
Your team OpenERP Community is subscribed to branch lp:~savoirfairelinux-openerp/openerp-accountedge/6.1-initial-version.