Add attachments support for notifications

[tatarincev]

@asvishnyakov commented on Fri Jun 09 2017

PR #645 related to this issue, please review

---

@eMazeika commented on Wed Apr 10 2019

It was never merged, right?

[j-mok]

Is development on this feature dead? We could really use it with SendGrid without patching the module manually.

[j-mok]

Can you please respond?

[Dan-BV]

Hello, @j-mok !

We are not planning to add attachments to notifications in the near future because we saw the following problems during the testing phase:

1. Attachments can affect the blocking of notifications in some ERP systems according to their security policies.
2. Attachments increase the final size of the notification, which may also be limited.
3. Attachments can have personal information.

Therefore, we recommend linking the notifications to attachments in e.g. assets. This way the user can access such attachments only after authorization.

[j-mok]

I understand your point of view and am ok with that. Still we need to have those attachments sent and so far we did this by inheriting from SendGridEmailNotificationMessageSender. The problem is that this class does not have good extension points. We need to override SendNotificationAsync() and copy all of the code from the base, while inserting a relatively small piece of attachment handling code in the middle. This is because SendNotificationAsync violates single responsibility rule - it both prepares and sends a message.

We're not happy with the current copy-and-paste approach and instead would like to use some kind of a pre-send hook that would allow us to inject attachments into the message just before invoking SendGrid client. Here's a general concept:

public class SendGridEmailNotificationMessageSender : INotificationMessageSender
{
    // ...
    
    public virtual async Task SendNotificationAsync(NotificationMessage message)
    {
        // ...

        try
        {
            var client = new SendGridClient(_sendGridOptions.ApiKey);
            var mailMsg = new SendGridMessage
            {
                From = fromAddress,
                Subject = emailNotificationMessage.Subject,
                HtmlContent = emailNotificationMessage.Body,
            };

            // ...

            BeforeMessageSend(message, mailMsg);

            var response = await client.SendEmailAsync(mailMsg);

            // ...
        }
        catch (SmtpException ex)
        {
            throw new SentNotificationException(ex);
        }
    }

    protected virtual void BeforeMessageSend(NotificationMessage message, SendGridMessage sendGridMessage)
    {
    }
}

This way we would extend only what we need to extend and avoid the hacky and brittle approach of base method duplication.