search

topcoder-platform / tc-notifications

4 stars 15 forks source link

Email templates support #59

Closed gondzo closed 6 years ago

gondzo commented 6 years ago

Two tasks are in scope of this issue:

Emails for all event types

Right now, email notifications are enabled only for post events (topic created/updated, post created/updated). The scope here is to add support for the remaining events. Full list of supported events is available at https://github.com/topcoder-platform/tc-notifications/blob/48d9a203992f1cb3cc20f587c71e44e8bb27d63e/connect/events-config.js#L48

There are two modes of sending email notifications - immediate (as it happens, one by one) or bundled. Both are handled in https://github.com/topcoder-platform/tc-notifications/blob/dev/connect/notificationServices/email.js and the logic for the other events should be added there as well. Note that constructing email logic will be changes in the next task (send raw html instead of data placeholders)

Individual notifications should always display projectId, project name, relevant user id, file name and url (only for File uploads), link name and url (only for New project links). Bundle notifications should contain the same data as individual notifications, but they should be grouped by projectId and grouped by notification type - the grouping can be reused from the frontend app - https://github.com/appirio-tech/connect-app/blob/e77f7d0a7daf4efbc1301f943fd1c6a8394f0211/src/routes/settings/routes/notifications/components/NotificationSettingsForm.jsx#L21 Also, multiple notifications of the same type should render just one notification. For example if we have two project.specificationModified events for the same project, we can display a notification like 'Project notification was updated by user1,user2' instead of two notifications 'Project notification was updated by user1' and 'Project notification was updated by user1'

Email templates support Our current approach is to have email templates in email service (in Sendgrid actually) and we send only placeholder values data to the email service, but templating engine in email service doesn't support loops or conditional expressions, so we're changing the behavior - tc-notifications should generate the email html content and send it as only placeholder value to the email service. Attached is the UI prototype for email templates. To get consistent email layout across all email clients, we need to inline all the css into html elements style tag. UI proto project has the css inliner support configured and it needs to be integrated into tc-notifications (see src/emails/one/one.template.html vs dist/one.template.html). Email service template has the following content

<!DOCTYPE html>
<head>

</head>
<body style="background-color: #EBEBEB; font-family: Helvetica, sans-serif; margin: 0 auto !important;">
    <!-- container begings -->
    <table class="container" style="background-color: #EBEBEB; border-collapse: collapse; color: #151516; font-family: Helvetica, sans-serif; margin: 0 auto !important; padding: 0; width: 100%;">
        {{contents}}
    </table>
    <!-- container ends -->
    <!-- footer container begins -->
    <table class="footer-container" style="background-color: #EBEBEB; border-collapse: collapse; color: #151516; font-family: Helvetica, sans-serif; margin: 0 auto !important; padding: 0; width: 100%;">
        <tr class="empty-21" style="height: 20px; margin: 0; padding: 0;">
            <td style="margin: 0; padding: 0;">
                <table style="border-collapse: collapse; height: 20px; margin: 0; padding: 0;">
                    <tr style="margin: 0; padding: 0;"><td style="margin: 0; padding: 0;"></td></tr>
                </table>  
            </td>
        </tr>
        <!-- footer beings -->
        <tr class="footer" style="margin: 0; padding: 0;">
            <td class="footer-space" style="margin: 0; padding: 5px !important;"></td>
            <td class="footer-content" align="center" style="margin: 0; padding: 0; vertical-align: middle;">
                <table style="border-collapse: collapse; margin: 0; padding: 0; width: 400px;">
                    <tr style="margin: 0; padding: 0;"><td style="color: #808080; font-size: 11px; line-height: 15px; margin: 0; padding: 0; text-align: center; width: 400px;">Connect is the world’s first project management crowdsorcing platform utilizing the power of Topcoder communities</td></tr>
                    <tr class="empty-10" style="height: 10px; margin: 0; padding: 0;"><td style="color: #808080; font-size: 11px; line-height: 15px; margin: 0; padding: 0; text-align: center; width: 400px;"></td></tr>
                    <tr style="margin: 0; padding: 0;"><td style="color: #808080; font-size: 11px; line-height: 15px; margin: 0; padding: 0; text-align: center; width: 400px;">201 S Capitol Ave #1100</td></tr>
                    <tr style="margin: 0; padding: 0;"><td style="color: #808080; font-size: 11px; line-height: 15px; margin: 0; padding: 0; text-align: center; width: 400px;">Indianapolis, IN 46225 United States</td></tr>
                    <tr class="empty-10" style="height: 10px; margin: 0; padding: 0;"><td style="color: #808080; font-size: 11px; line-height: 15px; margin: 0; padding: 0; text-align: center; width: 400px;"></td></tr>
                    <tr style="margin: 0; padding: 0;"><td class="html-entities" style="color: #808080; font-size: 11px; line-height: 15px; margin: 0; padding: 0; text-align: center; width: 400px;">&#9679;&#9679;&#9679;</td></tr>
                    <tr class="empty-10" style="height: 10px; margin: 0; padding: 0;"><td style="color: #808080; font-size: 11px; line-height: 15px; margin: 0; padding: 0; text-align: center; width: 400px;"></td></tr>
                    <tr style="margin: 0; padding: 0;">
                        <td style="color: #808080; font-size: 11px; line-height: 15px; margin: 0; padding: 0; text-align: center; width: 400px;">You are receiving this email based on your Connect account preferences. To change which emails you receive from Connect, go to your <a href="#" style="color: #006DEA; font-size: 11px; line-height: 15px; text-align: center; text-decoration: none;">Notification Settings.</a>
                    </td></tr>
                </table>
            </td>
            <td class="footer-space" style="margin: 0; padding: 5px !important;"></td>
        </tr>
        <!-- footer ends -->
        <tr class="empty-20" style="height: 20px; margin: 0; padding: 0;">
            <td style="margin: 0; padding: 0;">
                <table style="border-collapse: collapse; height: 20px; margin: 0; padding: 0;">
                    <tr style="margin: 0; padding: 0;"><td style="margin: 0; padding: 0;"></td></tr>
                </table>  
            </td>
        </tr>
    </table>
    <!-- footer container ends -->
</body>

In tc notifications, we need to build only the content inside "body table.container", inline the css and send that as the contents placeholder value

gondzo commented 6 years ago

Note: we can use notifications.action.email.connect.project.specificationModified for all emails at the moment (this event has the above sendgrid template).

architectt1 commented 6 years ago

@gondzo @vikasrohit About this:

UI proto project has the css inliner support configured and it needs to be integrated into tc-notifications (see src/emails/one/one.template.html vs dist/one.template.html)

I'm not sure what repo is that, would you mind providing a link to it? Thanks.

gondzo commented 6 years ago

i forgot to attach it. here it is

email-connect (1).zip

architectt1 commented 6 years ago

Should we use a single email template with logic in it to be able to display the aforementioned data for each event type?

architectt1 commented 6 years ago

@gondzo @vikasrohit I wanted to confirm with you the approach I was thinking of using:

Do you think this is a good approach?

gondzo commented 6 years ago

Is it easier to add the html for all notifications first and then inline all the css - at build time we don't know all the contents of the email?

gondzo commented 6 years ago

Maybe we could use handlebars templates for each event type - they should be easy to maintain in the future (just a suggestion)?

architectt1 commented 6 years ago

Is it easier to add the html for all notifications first and then inline all the css

If you mean on runtime, I think think would present a bit of a performance issue. I understand the style doesn't change, so we should be able to inline that on build time. The template would only contain the part we need to insert in the {{contents}} as said in

In tc notifications, we need to build only the content inside "body table.container", inline the css and send that as the contents placeholder value

Regarding:

at build time we don't know all the contents of the email?

No, at build time we build the template with all the inline styling. The result would be the template with inline css that when variables get replaced on runtime, that will be used as {{contents}}.

Does that make sense/do you see a disadvantage in this approach?

architectt1 commented 6 years ago

Maybe we could use handlebars templates for each event type - they should be easy to maintain in the future (just a suggestion)?

I'm a bit more used to mustache, although I'm open to suggestions. Do you see an advantage of handlebars over using mustache?

gondzo commented 6 years ago

OK, build time inlining is fine then. Mustache vs handlebars - it's up to you, both will serve the purpose here

architectt1 commented 6 years ago

I have some questions:

Bundle notifications should contain the same data as individual notifications, but they should be grouped by projectId and grouped by notification type

Please confirm my understanding of this is correct: on a period basis (current logic of bundled notifications in this project), we should send a single email with a section for each project, and in each of those sections, a section for each group of notification type as defined in here.

So for example, if we have scheduled notifications of types

for a given user and project, then we would need to send an email with two sections for that project, one with the first two notifications (and a New posts and replies section title), and another one for the third one (with a Project status changes title).

Regarding this:

Also, multiple notifications of the same type should render just one notification

This makes sense, although the grouping might need further insight into what data is associated to the specific notifications types, depending on what is needed to be displayed. For example, a post notification could have an associated message, and we might want to include that into the email. But if we have two posts for the same project, and we want to group them, we would loose the ability to display this information. That is an observation, and the overall question I'd like an answer for is, given multiple notifications of the same type, should we just include the description of the event (in your example, Project notification was updated), and the list of users that triggered that event?

gondzo commented 6 years ago

You're right on all of those. We can keep the last requirement out of scope for now and implement it later on (single notification for same event types)

On Sun, Jul 22, 2018, 18:05 architectt1 notifications@github.com wrote:

I have some questions:

Bundle notifications should contain the same data as individual notifications, but they should be grouped by projectId and grouped by notification type

Please confirm my understanding of this is correct: on a period basis (current logic of bundled notifications in this project), we should send a single email with a section for each project, and in each of those sections, a section for each group of notification type as defined in here https://github.com/appirio-tech/connect-app/blob/e77f7d0a7daf4efbc1301f943fd1c6a8394f0211/src/routes/settings/routes/notifications/components/NotificationSettingsForm.jsx#L21 .

So for example, if we have scheduled notifications of types

  • 'notifications.connect.project.topic.created'
  • 'notifications.connect.project.topic.deleted'
  • 'notifications.connect.project.created'

for a given user and project, then we would need to send an email with two sections for that project, one with the first two notifications (and a New posts and replies section title), and another one for the third one (with a Project status changes title).

Regarding this:

Also, multiple notifications of the same type should render just one notification

This makes sense, although the grouping might need further insight into what data is associated to the specific notifications types, depending on what is needed to be displayed. For example, a post notification could have an associated message, and we might want to include that into the email. But if we have two posts for the same project, and we want to group them, we would loose the ability to display this information. That is an observation, and the overall question I'd like an answer for is, given multiple notifications of the same type, should we just include the description of the event (in your example, Project notification was updated), and the list of users that triggered that event?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/topcoder-platform/tc-notifications/issues/59#issuecomment-406877827, or mute the thread https://github.com/notifications/unsubscribe-auth/AC6QfiNg8IWSkEEmEfEeCh7cKuheQWlyks5uJKKtgaJpZM4VYQmN .

vikasrohit commented 6 years ago

We can keep the last requirement out of scope for now and implement it later on (single notification for same event types)

I think we already handling this by showing the count of such events against that event here.

architectt1 commented 6 years ago

@gondzo @vikasrohit I closed my previous PR and created a new one with just the solicited changes. That should make reviewing easier.

vikasrohit commented 6 years ago

Thanks @architectt1 for the updated PR.

vikasrohit commented 6 years ago

@gondzo we can close this one now, right?

gondzo commented 6 years ago

yes