Get the images as images in the email notifications #350
Conversation
kaihendry
commented
Jul 3, 2018
kaihendry
commented
|
|
||
| function linkAttachment (message) { | ||
| const msgParts = message.split(' ') | ||
| if (msgParts.length > 1 && msgParts[0] === '[!attachment]') { |
There was a problem hiding this comment.
You should use https://github.com/unee-t/frontend/blob/master/imports/util/matchers.js#L3 instead of writing this check on you own
There was a problem hiding this comment.
I didn't actually understand your code. attachmentTextMatcher is a test to see if it's off that type? Never seen a base64 type.
And then there is still the bit where the second part if properly checked since it can be truncated and put into a template, unlike here:
There was a problem hiding this comment.
The base64 type is for attachments while they're being uploaded...
I brought this up because you just shouldn't reinvent the matching and parsing of the attachment url IMO.
The second part? You mean checking for a file extension that matches an image?
We should have only image type attachments for now, but if you think this improves the robustness of the check, I think you should still use attachmentTextMatcher but modify it so other places in the code can benefit from it as well 😄
There was a problem hiding this comment.
Ok, will re-try tomorrow.
|
@kaihendry what's missing so we can merge this? |
|
@nbiton approval since the attachment markup is not properly defined. |
franck-boullier
changed the title
|
Issue is legacy support for the defns with carriage return. Will it work? |
|
Struggling to test this locally, so please don't merge quite yet. |
|
@kaihendry any idea on what's missing so we can merge this? |
