Include header images from template messages as attachments on preview message #1262
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1262 +/- ##
==========================================
+ Coverage 81.85% 81.87% +0.01%
==========================================
Files 278 278
Lines 17588 17592 +4
==========================================
+ Hits 14397 14403 +6
+ Misses 2610 2609 -1
+ Partials 581 580 -1 ☔ View full report in Codecov by Sentry. |
flows/actions/testdata/send_msg.json
Outdated
| "name": "cat_fact" | ||
| }, | ||
| "template_variables": [ | ||
| "http://example.com/cat2.jpg", |
There was a problem hiding this comment.
@norkans7 @ericnewcomer wondering if this shouldn't be "image/jpeg:http://example.com/cat2.jpg" so that these image variables are more consistent with how we do msg media in general.. and then the onus is on courier to transform it like it would a regular attachment.
There was a problem hiding this comment.
I adjusted courier side for that in nyaruka/courier#752
There was a problem hiding this comment.
Ok I've updated this PR to assume that variable values look like attachments
flows/actions/send_msg.go
Outdated
| variable := variables[index] | ||
| if variable.Type == "text" { | ||
| previewContent = strings.ReplaceAll(previewContent, fmt.Sprintf("{{%s}}", key), variable.Value) | ||
| } else if variable.Type == "image" && variable.Value != "" { |
There was a problem hiding this comment.
What about videos or documents? which are the other type that can be supported
There was a problem hiding this comment.
Yeah good point we should make this work for all header types since we're doing this work
| } | ||
|
|
||
| variables[i] = &flows.TemplatingVariable{Type: v.Type(), Value: value} |
There was a problem hiding this comment.
I thought about having this ignore attachments that don't match the header type, e.g. ignore video/mp4:http://... if variable is type image... but the empty values will cause a send failure anyway and I think it will be more obvious to the user what the problem is if we just try to send the wrong attachment type and let WhatsApp complain about it.
There was a problem hiding this comment.
I avoided the mismatch by using the attachment type in https://github.com/nyaruka/courier/pull/752/files should I keep using the parameter type instead?
Of course both approaches would fail to send if the parameter type is not the same as the one on the template
There was a problem hiding this comment.
I think your change is fine.. in the case that someone updates their template but doesn't update the flow using it, we'll send a param of the wrong type to whatsapp, and I assume they'll send back a fairly clear error message.
No description provided.