Add option of supplying Reply-To header to MailDispatcher.SendMail()

[ajbeaven]

Since there could be many properties of the MimeMessage that someone may want to change I wondered whether alternatively, you could expose the MimeMessage somehow... perhaps by adding an overload that takes a delegate?

public async Task SendMail<T>(
            T data,
            MailboxAddress[] to,
            Action<MimeMessage> transformMessage) where T : MailBase 
{
    // ...
    transformMessage(message);
}

Let me know if you want to go that route instead.

[corstian]

Thanks for your PR! Regarding exposing the MimeMessage I'd love @synercoder's input. We're trying to maintain a boundary between delivery related properties (which can be set on the SendMail method), and mail composition using the template. There are however many ambiguous properties for which there are valid use cases to set from either of them, and currently I'm not sure how to deal with these.

Having an action to set properties on the MimeMessage, which should override properties set from the template seems like a pretty clean option. If we're doing this it would also make sense to add an Action<MimeMessage> to the MailBase.

As for adding the Reply-To header, I'm happy to merge this PR, but would rather first explore further possibilities with the MimeMessage.

[synercoder]

@corstian I do like the idea you suggested about the action on the mailbase. This would allow pre-sending change of the mime-message.

But just spitballing here, how about instead we add an overload with the Action, in that case you can either call the SendMail (with the reply to added, is a good idea) or call the method with the action. In which case you do need to set the to, from etc all yourself:

public Task SendMail<T>(
    T data,
    MailboxAddress[] to,
    MailboxAddress[]? cc = default,
    MailboxAddress[]? bcc = default,
    MailboxAddress? from = default,
    MailboxAddress[]? replyTo = default) where T : MailBase
{
    from ??= _options.DefaultFromAddress ?? throw new ArgumentException(nameof(from), $"Either the parameter {nameof(from)} must be set or the {nameof(_options.DefaultFromAddress)} must be set.");
    if (to.Length == 0)
        throw new ArgumentException(nameof(to), $"There must be atleast one mail address in the {nameof(to)} parameter.");

    Action<T, MimeMessage> messageTransform = (d, m) =>
    {
        m.From.Add(from);
        m.To.AddRange(to);
        if (cc != default && cc.Any())
            m.Cc.AddRange(cc);
        if (bcc != default && bcc.Any())
            m.Bcc.AddRange(bcc);
        if (replyTo != default && replyTo.Any())
            m.ReplyTo.AddRange(replyTo);
    };

    return SendMail(data, messageTransform);
}

public async Task SendMail<T>(
    T data,
    Action<T, MimeMessage> transformMessage) where T : MailBase
{
    var message = await _renderModelToMimeMessage(data);

    transformMessage?.Invoke(data, message);

    if (message.To.Count == 0)
        throw new ArgumentException($"The {nameof(message.To)} parameter must be set in the {nameof(transformMessage)} action.");
    if (message.From.Count == 0)
        throw new ArgumentException($"The {nameof(message.From)} parameter must be set in the {nameof(transformMessage)} action.");

    using var scope = _scopeFactory.CreateScope();
    var mailSender = scope.ServiceProvider.GetRequiredService<IMailSender>();
    await mailSender.SendMail(message);
}

[ajbeaven]

Yeah, this is pretty much what I envisioned. Personally I wouldn't bother with adding replyTo argument to the existing method if you were happy to add the overload.

[synercoder]

@corstian @ajbeaven how about the current state of the PR? I also added a sync and async of the overload (Func<T, MimeMessage, Task> & Action<T, MimeMessage>)

[corstian]

I think that the Action<T, MimeMessage> does not make sense in this situation. Given both T and MimeMessage are mutable, people might think they can still modify data on T, which is not the case, because T has already been used on the template, and will not impact anything any further. When using the SendMail method, one already has access of all the data which is available in T outside of the delegate.

I'd also add an Action<MimeMessage> to the MailBase in order to enable setting several recurring headers from the template instead from the SendMail method (Setting the Unsubscribe-Link would be one of those things I'd do from the template). This way the workflow would be:

1. Render the template (which includes use of BodyBuilder)
2. Apply post-render transformations on the MimeMessage
3. Apply pre-send transformations on the MimeMessage

This last one would be the transformation as applied on the SendMail method.

Does this make sense?

Ps. I like the overload using the traditional method signature and the optional Action<MimeMessage> signature. This definitely helps keeping the behaviour of these methods predictable :).

[synercoder]

Not directly to me, the point of the double parameters, yes, but are post-render and pre-send not almost at the same time?

And why enable doing the same thing from multiple places, I would either: add it to the mailbase, or add it to the pre-send transformation, but not both.

[corstian]

You're right that they are almost being executed at the same time.

My current mental model dictates that the template is being used for data which is static, or can be computed based on the model, while properties set on the SendMail method can change from call to call, such as destination, subject and headers. While it is possible to put these properties within the model, and set everything from within the template, I think it'd be more convenient to have these two possibilities.

For me it's mostly about not doing things in multiple locations if I can do them in a single location.

[synercoder]

So:

mailbase.MimeTransform => For example set X-Language header or something

mailDispatcher.SendMail(transform) => Set to/from/bcc/cc/reply to etc

Is my understanding correct?

[corstian]

Yep, I think it would boil down to that.

[synercoder]

Ill right, Ill try to take a stab at it this evening, too see if I can update the PR with what I guess would do.

[synercoder]

@ajbeaven After talking with @corstian on Skype over some ideas, we will code something either tonight or tomorrow, stay tuned because this might be a slighty bigger change (it will enable your use-case).

[ajbeaven]

Thanks all, much appreciated. No rush on my end.

[synercoder]

Closing this one as the request is currently being implemented in #11