MailKit conversion by MattMofDoom · Pull Request #78 · datalust/seq-app-htmlemail

MattMofDoom · 2021-08-09T05:59:36Z

This is the initial MailKit conversion that we discussed in #77 (I removed the SSL validation suppression option that I'd implemented).

I noted that we can make use of async methods for MailKit so I've updated EmailPlus to use onasync, and provided opportunity to handle unsuccessful emails in future (eg. the fallback mailhost idea I mentioned) by implementing a MailResult class.

I used a couple of idea from FluentEmail wrt an 'Options' class (SmtpOptions) and left a couple of unused properties that would likely be used for some of the ideas from #77.

I also ported over the $EventUri template that we added to Seq.App.Opsgenie as a nicety, and updated Handlebars.NET to the latest build (from v2.0.8 to v2.0.9).

As I was doing this I could envisage how we would go about implementing a number of the ideas from my list, but as agreed, I started with MailKit 😁

Cheers,

Matt

nblumhardt

Looks great; SmtpOptions is a nice improvement. Had some thoughts and queries inline. Cheers!

nblumhardt · 2021-08-10T04:47:22Z

-            var client = new SmtpClient(Host, Port ?? 25) {EnableSsl = EnableSsl ?? false};
-            if (!string.IsNullOrWhiteSpace(Username))
-                client.Credentials = new NetworkCredential(Username, Password);
+            var result = await _mailGateway.Send(_client, _options, new MimeMessage(new List<InternetAddress> {MailboxAddress.Parse(From)},


Since result is unused, we'll no longer get errors logged to the app's diagnostic stream when sending fails. Perhaps until we've looked into the fallback server idea, we should just keep throwing exceptions on failure, here?

Agree. I was planning to log errors as part of this but didn't implement a throw in the interim

Updated - if !result.Success, we throw result.Errors.

nblumhardt · 2021-08-10T04:47:44Z

-                client.Credentials = new NetworkCredential(Username, Password);
+            var result = await _mailGateway.Send(_client, _options, new MimeMessage(new List<InternetAddress> {MailboxAddress.Parse(From)},
+                new List<InternetAddress> {MailboxAddress.Parse(to)}, subject,
+                (new BodyBuilder() {HtmlBody = body}).ToMessageBody()));


Looks like we no longer set credentials for the outgoing mail server

@nblumhardt We do, but it's in the SmtpOptions that are set during OnAttached.

It's set on _options, but never passed to the client.

Well, crap. Will fix.

Missing code found down the back of the sofa

nblumhardt · 2021-08-10T04:48:30Z

-                { "$ServerUri",           host.BaseUri }
+                { "$ServerUri",           host.BaseUri },
+                // Note, this will only be valid when events are streamed directly to the app, and not when the app is sending an alert notification.
+                { "$EventUri",            string.Concat(host.BaseUri, "#/events?filter=@Id%20%3D%20'", evt.Id, "'&amp;show=expanded") } 


nblumhardt · 2021-08-10T04:53:13Z

+        public bool UseSsl { get; set; } = false;
+        public bool RequiresAuthentication { get; set; } = false;
+        public string PreferredEncoding { get; set; } = string.Empty;
+        public SecureSocketOptions? SocketOptions { get; set; }


SocketOptions and UseSsl overlap, here. Perhaps we knock out UseSsl, make SocketOptions non-nullable, and set it to SslOnConnect when using SSL, or StartTlsWhenAvailable otherwise? (This is how MailKit interprets Boolean useSsl behind the scenes.

@nblumhardt I think you're right. I'll take a look.

SocketOptions is now the only way to fly

nblumhardt · 2021-08-10T04:54:06Z

+        public string Password { get; set; } = string.Empty;
+        public bool UseSsl { get; set; } = false;
+        public bool RequiresAuthentication { get; set; } = false;
+        public string PreferredEncoding { get; set; } = string.Empty;


To avoid confusion and an accumulation of unimplemented bits and pieces, it's probably best to remove the unused properties here until we've worked through the features they are used by

Removed the unused bitses

nblumhardt · 2021-08-10T04:54:28Z


        [Fact]
-        public void ToAddressesAreTemplated()
+        public async void ToAddressesAreTemplated()


Should return async Task here, or xUnit can't track completion

Now returning async Task.

nblumhardt · 2021-08-10T21:37:35Z

-            client.Send(message);
+            try
+            {
+                await client.ConnectAsync(options.Server, options.Port, options.UseSsl);


Since we connect and disconnect here, any reason not to just create and release a new SmtpClient each time?

Yeah agree. MailKit initialises quite quickly with minimal overhead and is Async to boot, so that shouldn't be a problem. There are some cases where it's more efficient to hold a single instance of a client (not MailKit) to reduce initialisation overhead, but this was a change before I really got deep into the conversion and proved unnecessary.

Now implementing SmtpClient each time. This is now redundant in EmailApp and have removed it from IMailGateway/DirectMailGateway/etc

nblumhardt · 2021-08-10T21:38:02Z

        readonly ConcurrentDictionary<uint, DateTime> _lastSeen = new ConcurrentDictionary<uint, DateTime>();
        readonly Lazy<Template> _bodyTemplate, _subjectTemplate, _toAddressesTemplate;
+        private SmtpClient _client = new SmtpClient();
+        private SmtpOptions _options = new SmtpOptions();


Missed these - should be readonly, no explicit private

Done - no need for OnAttached now.

MattMofDoom · 2021-08-11T23:42:40Z

@nblumhardt Might need a little bit more work to ensure that the to address parse doesn't fail for comma-separated strings.

I was working through mailhost fallbacks and possibly DNS delivery so will fix as part of that.

…, set SmtpOptions to use Lazy

MattMofDoom · 2021-08-12T02:19:13Z

@nblumhardt This will need a bit of battle testing to make sure fallbacks and DNS delivery work as expected, but I'm quietly optimistic.

MattMofDoom · 2021-08-12T04:45:16Z

@nblumhardt Okay, I've now successfully run through unit tests for delivering via mailhost and delivering via DNS, and made corrections based on the result. I left the test cases that I used commented out for your benefit, although obviously they'll be removed.

In any case - the code is working for both scenarios 😀

MattMofDoom · 2021-08-13T05:30:38Z

@nblumhardt Reviewed the rest of the ideas from #77 and pulled in the following enhancements;

Priority added, with optional priority mapping ... this is the only property mapping that seems to make sense per list below
CC, BCC, ReplyTo added as optional envelope options
Added optional plan text body template - if not specified, it won't be used
Email logging added for sent and error scenarios ... this is probably the most useful debug logging that could be added

Rejected or parked:

Mapping to/cc/bcc properties rejected, since we can use Handlebars to pull in properties.
Parking attachment of event json - we have an ability to link EventUri within the Handlebars template
Parking event tags since they don't fit an SMTP scenario
Validation of email addresses parked - we do parse them to MailKit internet addresses so this may fit the bill

This should make Email Plus more fully featured without overburdening it with unnecessary drudge.

nblumhardt · 2021-08-13T05:34:43Z

Thanks Matt! Just a heads up that we're deep in some feature work this week and may not emerge for a few more days yet - will get some 👀 back on this ASAP 😅

RE how we shape up PRs - it's often much faster to discuss/review/merge small PRs one at a time; adding features to one large PR can stall the process sometimes.

If you're able to force push the older commit with just the mailkit conversion, but send the remaining commits in a follow-up PR, that would be optimal. Can help to figure out a plan next week, either way.

Have a great weekend!

MattMofDoom · 2021-08-13T05:39:12Z

@nblumhardt Fair enough on all points 😊 Will see what I can do.

Have a great weekend yourselves!

MattMofDoom · 2021-08-13T05:46:49Z

Closing PR in favour of #79 ... will send the remaining commits in a follow-up PR as discussed.

MattMofDoom and others added 5 commits July 25, 2021 12:50

Add SSL validation suppression option

222aa53

Merge branch 'datalust:dev' into dev

1654098

Remove SSL validation suppression option as discussed in datalust#77

8a455fd

Update Handlebars.net

f6d004c

Install MailKit and implement, add $EventUri to template

d4927b1

nblumhardt reviewed Aug 10, 2021

View reviewed changes

Update to reflect comments in datalust#78

8580120

MattMofDoom added 3 commits August 12, 2021 12:04

Implement mail host fallback and DNS delivery, fix To address parsing…

19da9c3

…, set SmtpOptions to use Lazy

Remove extant code from TryDeliver

189f988

Improve DnsMailResult logic

1a45cc3

MattMofDoom added 3 commits August 12, 2021 12:23

Retain lastError if not superseded

bffb82a

Always track list of exceptions for logging

26261af

Corrections from running mail host and DNS test cases

6c3f9f3

MattMofDoom added 2 commits August 13, 2021 14:17

Add email priority and priority property mapping

e695402

Add cc, bcc, and plain text body

abf8d25

MattMofDoom mentioned this pull request Aug 13, 2021

MailKit conversion redux #79

Merged

MattMofDoom closed this Aug 13, 2021

MattMofDoom mentioned this pull request Aug 22, 2021

Envelope enhancements #83

Open

Conversation

MattMofDoom commented Aug 9, 2021

Uh oh!

nblumhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattMofDoom commented Aug 11, 2021

Uh oh!

MattMofDoom commented Aug 12, 2021

Uh oh!

MattMofDoom commented Aug 12, 2021

Uh oh!

MattMofDoom commented Aug 13, 2021

Uh oh!

nblumhardt commented Aug 13, 2021

Uh oh!

MattMofDoom commented Aug 13, 2021

Uh oh!

MattMofDoom commented Aug 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants