Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get the images as images in the email notifications #350

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Get the images as images in the email notifications #350

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f66e4f4
Fix https://github.com/unee-t/frontend/issues/342
Jul 3, 2018
62cace0
The spacing between [!attachment] and url is actually a CR. Account f…
Jul 4, 2018
018aa0b
Merge remote-tracking branch 'upstream/master' into attachment
Nov 7, 2018
142553c
Make attachment link more flexible regarding whitespace
Nov 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion imports/email-templates/case-new-message.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,18 @@
import url from 'url'
import { optOutHtml, optOutText } from './components/helpers'
import { matchWidth } from '../util/cloudinary-transformations'

function linkAttachment (message) {
const msgParts = message.split(' ')
if (msgParts.length > 1 && msgParts[0] === '[!attachment]') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use https://github.com/unee-t/frontend/blob/master/imports/util/matchers.js#L3 instead of writing this check on you own

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

https://github.com/kaihendry/frontend/blob/f66e4f42373df558aba48ae577d178d45ec470b8/imports/ui/case/case-details.jsx#L384

Copy link
Contributor

@nbiton nbiton Jul 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will re-try tomorrow.

const attachmentUrl = msgParts[1]
if (attachmentUrl.match(/\.(jpeg|jpg|gif|png)$/) != null) {
const thumbUrl = matchWidth(attachmentUrl, 500)
return `<a href=${thumbUrl}><img alt="Attachment image" src=${thumbUrl}></a>`
}
}
return message
}

export default (assignee, notificationId, settingType, caseTitle, caseId, userId, message) => ({
subject: `New message on case "${caseTitle}"`,
Expand All @@ -9,7 +22,7 @@ export default (assignee, notificationId, settingType, caseTitle, caseId, userId

<p>New message by ${userId.profile.name}:</p>

<p>${message}</p>
<p>${linkAttachment(message)}</p>

<p>Please follow <a href='${url.resolve(process.env.ROOT_URL, `/case/${caseId}`)}'>${url.resolve(process.env.ROOT_URL, `/case/${caseId}`)}</a> to participate.</p>

Expand Down