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. _______________________________________________ Mailing list: https://launchpad.net/~openerp-community Post to : [email protected] Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
