Add support for inline attachments in SmtpMailer

johnwc commented 1 month ago

Enhanced the Attachment class by adding a ContentId property to support inline content. Updated SmtpMailer to handle attachments with non-empty ContentId as linked resources, generating RFC-compliant ContentId using MimeKit.Utils.MimeUtils.GenerateMessageId(). Modified the HTML body to reference the correct ContentId. Attachments with empty or whitespace ContentId are added as regular attachments.

Thank you for your contribution! Before submitting this PR, please consider:

If you are fixing something other than a typo, please always create an issue first. Otherwise, your PR will probably be rejected.
You have added unit tests which (a) pass and (b) sufficiently cover your changes

Based on feature request #376

johnwc commented 3 weeks ago

@jamesmh do you have time to review this so we can get this update released?

jamesmh commented 3 weeks ago

Hey, thanks for this! I'm super busy FYI but I'll try to check things out asap :)

johnwc commented 2 weeks ago

Thanks again for this! Have a few minor things that I think make sense. We'll also need tests + documentation that I can add I think we should add a method on Attachment:
public Attachment WithContentId()
{
    this.ContentId = MimeKit.Utils.MimeUtils.GenerateMessageId();
    return this;
}         
Then, when adding an attachment the user can generate the id once, and we don't need to do the regex search + generate another id, etc.

So the usage would be something similar to this:
var attachment = new Attachment
{
    Bytes = new byte[] { },
    Name = "test_image.png"
}
.WithContentId();

UserModel user = new UserModel()
{
    Email = "FromUserModel@test.com",
    Name = "Coravel Test Person",
    ImgContentId = attachment.ContentId
};

string message = await this._mailer.RenderAsync(
    new NewUserViewMail(user)
        .Attach(attachment));
Or using an HTML template, same idea. The content id is generated once - and we'll add this to the docs as the way to to generate the content id.

This works ok when there a few embedded images, but in some of our razor templates we have 10 or 15 embedded images, maybe more. I would loath having to use an array everywhere we have images embedded. I like the idea of the fluid method that generates the ContentId, in the cases where it can be used like your example. There's also the idea of making a razor helper/extension that can be called in the template that returns the content id of the image based on the name they gave when adding the image. This way, we don't have to worry about if the array of images changed at any time, and developers later looking at the code know exactly what it is referring to in the html.

@model My.Namespace.PageModel
@using Coravel.Mailer.Mail.Razor

<html>
     <h3>Hello, @Model.FirstName</h3>
     <img src="cid:@Html.AttachmentContentId("mobile")" /> Cell: @Model.Cell
     <img src="cid:@Html.AttachmentContentId("office")" /> Office: @Model.Office
     <img src="cid:@Html.AttachmentContentId("email")" /> Cell: @Model.Email
</html>

jamesmh commented 1 week ago

Curious how you are embedding images? The cid that your attachments have need to be unique. Where / when are you setting that if not via the attachment?

johnwc commented 1 week ago

Curious how you are embedding images? The cid that your attachments have need to be unique. Where / when are you setting that if not via the attachment?

The content id is unique for each attachment. I don't follow your confusion you have with it. Can you elaborate more?

jamesmh commented 1 week ago

You said

This works ok when there a few embedded images, but in some of our razor templates we have 10 or 15 embedded images, maybe more. I would loath having to use an array everywhere we have images embedded...

I'm not sure what an alternative approach to using an array would be? Wouldn't you have to still set a unique value for each cid anyways?

You said "I like the idea of the fluid method that generates the ContentId, in the cases where it can be used like your example" -what are the cases not like my example where the original approach you had would seem better?

Thanks!

johnwc commented 1 week ago

what are the cases not like my example where the original approach you had would seem better?

When using embedded images in an email template that has several images throughout the template. (Think icons scattered throughout the email. Email, phone, office, house icon, external link icon, etc...) Having to keep track of what array index goes with what attachment, that would be a really big pain. As well as another developer coming in months later to update the template, inserts an image at the beginning(or wherever they want) of the array, now causing all the embedded images to be inaccurate. The new developer not realizing what they did, now eats up time trying to figure out how they broke it. Whereas if there was a razor helper function, like in the example I gave; we would look up the attachment based on the attachment's name and return the content id for it @Html.AttachmentContentId("mobile").

Also, don't want to have to be forced to have 40 additional property fields in the model to hold every image's content id.

jamesmh / coravel

Add support for inline attachments in SmtpMailer #413