Code review comment for lp:~therp-nl/server-env-tools/7.0-add_email_template_conditional_attachment
Revision history for this message
| Graeme Gellatly (gdgellatly) wrote | # |
review:
Abstain
(code review, no test)
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
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.