Code review comment for lp:~therp-nl/server-env-tools/7.0-add_email_template_conditional_attachment

Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

I'd also prefer OpenERP's API, so +1 for safe_eval.

But more picking on this line: The '&' makes your code crash for an empty attachment domain ('[]') and adds no benefit I can see. Do I overlook some benefit?

Generally, I think it's safer to use osv.expression.normalize and osv.expression.combine where applicable.

Further: What purpose does conditional_attachment_ids on ir.attachment serve?

review: Needs Fixing (code review)

« Back to merge proposal