gl-dotnet-email changes for Cc, Bcc, ReplyTo, and attachments #33

Budsy · 2018-04-17T23:14:10Z

Edit: Addresses #14 and #15

Ok. This should be a little cleaner. My usage is mostly for the Smtp methods, but I added code to the InMemory and Sendgrid parts so that they would be still compatible with changes to the Smtp section.

Discussion: breaking code changes? Also, I'm not sure email address validation inside these classes is best. I prefer validation in higher level code. Anyway, these libraries will really be improved by allowing attachments and targeting addresses to Bcc and Cc and so on. So, see what you think.

Adds property AddressAs, holding enum AddressTarget value. Used as a flag for the sending methods to specify the respective address into the corresponding fields in the SmtpClient.

Adds a property to IEmailAddress: AddressTarget AddressAs { get; } . Used by the Sender method to insert EmailAddress objects into the appropriate SendEmail client fields.

Accommodates the attachments parameter. The param is specified as a default of null. This works, allowing the implementation to omit this, and it will default to null. Do not give the parameter an option of null in the implementation though, or it will always be null even if , and it will work.

Added attachment parameter, and IEnumerable<IEmailAddress> for 'to' addresses. For consistency I changed 'to' parameter name to 'recipients' since my implementation of the EmailAddress class and its interface IEmailAddress includes more general purpose usage, with enum flags for Bcc, Cc, ReplyTo, and so on.

Basically this has changes for compatibility with the IEmailProvider interface. The attachments are accommodated but not implemented. I added a method wrapper so existing code should still work with the provider.

Modifications for routing the new IEmailAddress objects to appropriate client fields (.To, .ReplyTo, etc). Also adds any attachment objects into the message object.

Basically just a compatibility change. The IEmailProvider interface wants the attachment parameter. I added it to the SendEmailAsync method for SendGrid, but it is not fully implemented. Also, since the interface sets a fallback to null, omitting it in calling code is allowed, and specifying attachments will have no effect. This might need some discussion. It is unclear to me whether Sendgrid discourages attachments anyway, so maybe it's better as is. Alternatively, the SendGrid methods could take up the attachment feature if desired.

Modified the SendEmail Action to illustrate usage for the new implementations.

Slight modifications so that the solution will hopefully compile without fuss. Attachments not used for test.

sandorfr · 2018-04-17T23:48:35Z

src/GeekLearning.Email/Internal/EmailSender.cs

+
+                    string trimmedEmail = recipient.Email.Trim();
+                    // not sure if it's the most friendly to validate in the sender method. Should be left to caller code, IMO 
+                    if (Regex.IsMatch(trimmedEmail, @"\A(?:[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?)\Z", RegexOptions.IgnoreCase).Equals(false))


I'm not comfortable with regex especially without a timeout in our codebase. I'm not sure this is the role of the library to validate input altogether. So I'm all in for dropping it entirely. @asiffermann you may have a different opinion.

Yes. And there was a throw in the original implementation when checking the domain part to conditional branch into the mocking sender insertion. Is there a way to check SMTP errors in the SmtpClient and then throw for failures there? Then at least the user code takes responsibility for address validation, but if that isn't done, at least the app will get alerted at the real point of failure.

sandorfr · 2018-04-17T23:54:04Z

src/GeekLearning.Email/IEmailAddress.cs

    public interface IEmailAddress
    {
        string Email { get; }

        string DisplayName { get; }
+
+        AddressTarget AddressAs { get;  } 


IEmailAddress is meant to be a simple interface consumer can implement on their on types (or through a facade) so they can use such objects directly.

I think AddressTarget should be an argument for IEmailSender methods overloads, not a part of this interface.

I understand your idea about IEmailAddress, just defining the conventional "name [email protected]" definition. So, what might be an elegant way to link the email user with their 'AddressTarget' attributes? I think I was guessing your thinking when I made the AddressAs property optional. It can be left out, and will default to a 'To:' address insertion. But to avoid the original params implementation (which gets awkward if you want to define 5-10 or more recipients!), and keep the original simplicity of the IEmailAddress contract, maybe there has to have as inputs some kind of User structure, in which the IEmailAddress is itself a property? or overload the class methods? It would be nice to avoid too much of that.

sandorfr · 2018-04-17T23:55:25Z

src/GeekLearning.Email/IEmailProvider.cs

    public interface IEmailProvider
    {
-        Task SendEmailAsync(IEmailAddress from, IEnumerable<IEmailAddress> recipients, string subject, string bodyText, string bodyHtml);
+        Task SendEmailAsync(


IEmailProvider should probably use AddressTarget

My idea was to be able to define a complete email transaction in one call. You might want to send to two (or more) different addresses, Cc: another, and Bcc: yet another, and also have a ReplyTo: for the message that is different that the From: address. (some hosting services required the From address to match the authentication user name, which might not be the same as the 'sender' of the immediate message). Using an attribute parameter for the email address implies a single target transaction. Slower if the app wants to send to several targets.

sandorfr · 2018-04-17T23:56:04Z

src/GeekLearning.Email/IEmailSender.cs

@@ -4,12 +4,12 @@

    public interface IEmailSender


there should be additional overloads using AddressTarget

That's a thought. But there needs to be a solid way to link an AddressTarget to an email address. And how does it look if we have multiple email addresses in the call?

sandorfr · 2018-04-17T23:56:26Z

src/GeekLearning.Email/Internal/EmailAddress.cs

        }

        public string Email { get; set; }

        public string DisplayName { get; set; }
+
+        public AddressTarget AddressAs { get; set; }


we don't want that.

sandorfr · 2018-04-17T23:59:45Z

Also, I'm not sure email address validation inside these classes is best. I prefer validation in higher level code.

I am in total agreement here.

Breaking code changes

The next version is breaking because of the SendGrid changes. So I'm 100% comfortable with introducing more of them. If we want this to get better we will have to anyway.

sandorfr · 2018-04-18T09:35:09Z

Ok, now I see our misunderstanding.

I overlooked AddressTarget itself (which is why I was finding its naming weird). The Api I have in mind is to chenge from:

Task SendEmailAsync(IEmailAddress from, IEnumerable<IEmailAddress> recipients, string subject, string bodyText, string bodyHtml);

to

Task SendEmailAsync(IEmailAddress from, IAudience audience, string subject, string bodyText, string bodyHtml);

public interface IAudience {
   IEnumerable<IEmailAddress> To {get;}
   IEnumerable<IEmailAddress> Cc {get;}
   IEnumerable<IEmailAddress> Bcc {get;}
   IEnumerable<IEmailAddress> ReplyTo{get;}
}

We could imagine offering an AudienceBuilder.

sandorfr · 2018-04-18T09:39:30Z

src/GeekLearning.Email/IEmailSender.cs

@@ -4,12 +4,12 @@

    public interface IEmailSender
    {
-        Task SendEmailAsync(string subject, string message, params IEmailAddress[] to);


We should not remove the params commodity.

sandorfr · 2018-04-18T09:42:02Z

Otherwise to keep your current implementation. I'd be more inclined to use decorators to attach the AddressTarget to an existing IEmailAddress instance. With some extensions methods. It could give something like:

await sender.SendEmailAsync("subject", "body", firstEmail.ToCc(), secondEmail.ToReplyTo());

Extension methods for IEmailAddress for decorators .ToCc(), .ToBcc(), and .ReplyTo() . Attach to an IEmailAddress instance, optionally, to specify to the SendMailxxx() methods to which properties of the sendMail client to install an email address.

Attachments now can be set as parameters in overloaded methods. IEmailAddress objects now are params at the ends of method call parameter lists.

Interface for IEmailAddress restored to original, simple form.

Overloaded methods now specified in the interface. Allows attachments in separate methods. IEmailAddress objects now params .

Uses optional decorators on IEmailAddress objects. Plus, email message attachments.

Conforms to changes.

Updated SendMail action method to show usage of sending mail with an attachment, and a second send demonstrating adding a Cc 'to' address, using the optional decorators.

Budsy · 2018-04-19T21:42:07Z

Commits to the develop branch for 4/19/2018 include revisions to restore the Send... methods to use param array elements of IEmailAddress instead of IEnumerable objects. Restored IEmailAddress interface to original, simple form. I also modified the methods and interfaces so that attachments can be added to as a parameter in calls when desired. The sample project's SendEmail() action method updated to demo examples of usage. Decorators can be added to the '(IEmailAddress) to' addresses to make them Cc, Bcc, or ReplyTo if desired, otherwise an address in the 'to' param list defaults to a 'To' address. The AddressTarget enum is simplified and is only used internally.

sandorfr · 2018-04-20T00:46:54Z

I like this much better :) I'll wait for @asiffermann feedback

Budsy

This is obsolete now because of later commit revisions.

Budsy added 10 commits April 17, 2018 14:24

Update EmailAddress.cs

2ae944b

Adds property AddressAs, holding enum AddressTarget value. Used as a flag for the sending methods to specify the respective address into the corresponding fields in the SmtpClient.

Update EmailSender.cs

6a9a2fc

Update IEmailAddress.cs

28b5117

Adds a property to IEmailAddress: AddressTarget AddressAs { get; } . Used by the Sender method to insert EmailAddress objects into the appropriate SendEmail client fields.

Update InMemoryEmailProvider.cs

35d7331

Basically this has changes for compatibility with the IEmailProvider interface. The attachments are accommodated but not implemented. I added a method wrapper so existing code should still work with the provider.

Update SmtpEmailProvider.cs

36618ae

Modifications for routing the new IEmailAddress objects to appropriate client fields (.To, .ReplyTo, etc). Also adds any attachment objects into the message object.

Update HomeController.cs

d9198ab

Modified the SendEmail Action to illustrate usage for the new implementations.

Update SendTemplatedTest.cs

2a1cf73

Slight modifications so that the solution will hopefully compile without fuss. Attachments not used for test.

sandorfr requested changes Apr 17, 2018

View reviewed changes

sandorfr requested a review from asiffermann April 18, 2018 00:00

sandorfr reviewed Apr 18, 2018

View reviewed changes

Budsy added 7 commits April 19, 2018 13:38

Update EmailAddress.cs

9635f2a

Extension methods for IEmailAddress for decorators .ToCc(), .ToBcc(), and .ReplyTo() . Attach to an IEmailAddress instance, optionally, to specify to the SendMailxxx() methods to which properties of the sendMail client to install an email address.

Update EmailSender.cs

19bbf69

Attachments now can be set as parameters in overloaded methods. IEmailAddress objects now are params at the ends of method call parameter lists.

Update IEmailAddress.cs

3d2e989

Interface for IEmailAddress restored to original, simple form.

Update IEmailSender.cs

b5dbc89

Overloaded methods now specified in the interface. Allows attachments in separate methods. IEmailAddress objects now params .

Update SmtpEmailProvider.cs

2e4b9e1

Uses optional decorators on IEmailAddress objects. Plus, email message attachments.

Update SendTemplatedTest.cs

90d4f8d

Conforms to changes.

Update HomeController.cs

2493e33

Updated SendMail action method to show usage of sending mail with an attachment, and a second send demonstrating adding a Cc 'to' address, using the optional decorators.

sandorfr assigned asiffermann Apr 20, 2018

Budsy commented May 21, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gl-dotnet-email changes for Cc, Bcc, ReplyTo, and attachments #33

gl-dotnet-email changes for Cc, Bcc, ReplyTo, and attachments #33

Budsy commented Apr 17, 2018 •

edited by sandorfr

Loading

sandorfr Apr 17, 2018

Budsy Apr 18, 2018

sandorfr Apr 17, 2018 •

edited

Loading

Budsy Apr 18, 2018

sandorfr Apr 17, 2018

Budsy Apr 18, 2018

sandorfr Apr 17, 2018

Budsy Apr 18, 2018

sandorfr Apr 17, 2018

sandorfr commented Apr 17, 2018

sandorfr commented Apr 18, 2018 •

edited

Loading

sandorfr Apr 18, 2018

sandorfr commented Apr 18, 2018

Budsy commented Apr 19, 2018

sandorfr commented Apr 20, 2018

Budsy left a comment

gl-dotnet-email changes for Cc, Bcc, ReplyTo, and attachments #33

Are you sure you want to change the base?

gl-dotnet-email changes for Cc, Bcc, ReplyTo, and attachments #33

Conversation

Budsy commented Apr 17, 2018 • edited by sandorfr Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandorfr Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandorfr commented Apr 17, 2018

sandorfr commented Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

sandorfr commented Apr 18, 2018

Budsy commented Apr 19, 2018

sandorfr commented Apr 20, 2018

Budsy left a comment

Choose a reason for hiding this comment

Budsy commented Apr 17, 2018 •

edited by sandorfr

Loading

sandorfr Apr 17, 2018 •

edited

Loading

sandorfr commented Apr 18, 2018 •

edited

Loading