Implement Email Provider

[meetmandeep]

Summary

Closes #3967

[valadas]

Left a couple of comments, also don't we also need the web.config changes in another file for upgrades ?

[bdukes]

Yes, see 06264b6 for the config needed to add a new provider on upgrades.

[valadas]

One more small copy-paste error 😁

[valadas]

Looks good to me, reviewers please do not merge until we decide into which version we get this in.

[mitchelsellers]

One thought on this, and I’m more than willing to get on a call for this also.

This provider solution is good, however we lack an extensibility to adjust settings for mail.

So, for example if someone were to create a SendGrid provider that needed to collect an API key and other values they would have to roll their own implementation.

I’m starting to believe that we need this provider to work a bit like the HtmlEditor and Authentication providers where a UI can be exposed. This would provide a true editor experience.

Thoughts?

[sleupold]

@mitchelsellers wouldn't a provider just need a Settings UI and store the values in HostSettings table (using proper prefix, e.g.)?

[valadas]

Yeah, that would be nice. now given that the email settings show up in the persona bar, if we do it like in the html editor provider than it would mean loading up that settings UI in webforms, which I think would be a step backwards.

I am thinking of 2 possible solutions:

1. We do load it up in a modal somehow (iframe or such) and we support the control just like any other module to support webforms, mvc or html5 in that modal.
2. We make the html5 module pattern work outside of a Dnn module (not sure how).

[mitchelsellers]

I’m adding this to the discussion for the approvers meeting on Tuesday.

For consistency we already have multiple providers that load a webforms view into the PersonaBar. Not saying that’s right, but it would be consistent with other documented processes. But we need to review as a whole

[bdukes]

FWIW, my vote is to accept this PR for 9.8.0, with or without a UI. If someone wants to create a SendGrid provider, it can come with its own UI or document how the SMTP settings map to SendGrid settings. Having a customizable in-place UI is nice to have, but not required, IMO.

[meetmandeep]

public abstract string SendMail(MailInfo mailInfo, SmtpInfo smtpInfo = null);

The signature on the SendMail basically requires just the MailInfo object and leaves the implementation to the method. We always add a generic in-place UI later but IMO it's not needed at this time. If anything, it restricts functionality. At best, it would allow a "Settings Config" at portal level. Right now the developer can route Emails based on priority via different Servers or Services.

If I were to create a MailProvider with load balancing option then the generic in-place UI is not sufficient.

I agree with @sleupold that developer should handle their own UI and store in HostSettings or elsewhere as needed.

[meetmandeep]

Please DO NOT merge this PR yet; we may want to create a generic Mapper for MailAttachment rather than using the System.Net.Mail class. I was mistaken that MailKit supports System.Net.Mail.Attachment.

I'm thinking a generic MailAttachment object with following overloads:
MailAttachment(string Filename)
MailAttachment(string Filename, Byte[] Attachment)
MailAttachment(string Filename, Byte[] Attachment, string ContentType ) //Mime Type

Thoughts?

[mitchelsellers]

I'm in 100% agreement that the developer should create their own UI, and not necessarily opposing this solution as-is for now, but it is an incomplete solution per how we handle other providers where additional settings and configuration are needed.

Authentication Providers expose a UI for the addition/management of all settings.
HTML Providers do the same thing

Not all email is sent with SMTP settings, in fact, some of the best ways to manage email aren't. A vendor can easily do something with this that is outside of everything else. But then we have confusion from a user perspective when they see "SMTP Settings" but they don't do anything or have any impact in their environment.

Again - not saying no, just saying that in addition to this we have some additional items to think about.

[mitchelsellers]

One other thought on this, after review, is the name of the actual provider implementation. The name "Core" is something that is not necessarily intuitive, or indicative of what is being done.

I'm thinking SmtpMailProvider or MailKitMailProvider or similar if we move this to an implementation with MailKit would be more descriptive.

[meetmandeep]

But then we have confusion from a user perspective when they see "SMTP Settings" but they don't do anything or have any impact in their environment.

I agree. Perhaps we can remove SMTP Settings when CoreMailProvider is not in use until we find a solution to in-place UI that is not webforms?

[mitchelsellers]

Regarding the object for Attachments, we have done similar in our .NET Core model that we use, so I think those could be good choices.

And that COULD be a good idea for the provider. But I'm thinking if we don't provide a UI option since the new provider COULD use it, that we almost just want to show a message "You are using a provider other than the default, the settings below may not apply."

[valadas]

Well, a solution like for the Authentication providers would not be bad for now:

Maybe we could move the smtp settings in there too. Now this opens another discussion. The "core" has a fallback from site settings to host settings, I guess the providers would also need something like that...

[mitchelsellers]

Discussing UI elements today in the approvers meeting, possibly adding a "Select Provider" option on the SMTP settings areas and a "SMTP Settings Required" or similar flag to the provider to show/hide SMTP configuration.

@donker is going to help look into this.

[meetmandeep]

Changes from @donker were merged in this PR.

@bdukes @valadas You may accept and merge this PR. The MailAttachment wrapper code is now part of the MailKit PR that will submit next week.

[mitchelsellers]

Changes were implemented as requested

[bdukes]

Ready to merge/rebase once build is done