openerp-community-reviewer team mailing list archive

[Graeme Gellatly <gdgellatly@xxxxxxxxx>]

Review: Abstain code review, no test

Hi I reviewed code only

All looks good and useful.  Only comments for improvement are

Line 215, I know it is extremely unlikely/impossible in current modules but for futureproofing consider 

if res_id and res:  

rather than just if res_id.

>From a usability standpoint it would be useful to have the reverse view. i.e. If you have an attachment you wish to attach to multiple templates being able to specify in the attachment rather than going in to each email template.  I think of the use case of say a new product launch or an important change which needs to go with many emails.  But that is new feature rather than complaint.

-- 
https://code.launchpad.net/~therp-nl/server-env-tools/7.0-add_email_template_conditional_attachment/+merge/202740
Your team Server Environment And Tools Core Editors is requested to review the proposed merge of lp:~therp-nl/server-env-tools/7.0-add_email_template_conditional_attachment into lp:server-env-tools.