Azure Email Communication Services (Lombiq Technologies: OCORE-129)

[MikeAlhayek]

The following some of advantages for using this PR over #14749

• In this PR there is no breaking change and it is fully-backward compatible.
• Code structure that it open for extension and closed for modification.
• Fewer types being register directly into the IoC container.
• One sectioned UI to configure any available provider.
• One UI to test any configured provider.
• Adding new providers is fairly an easy task.
• We only release the shell of the tenant only when needed.

The following objects have been marked obsolete. They are kept for backward compatibility.

• ISmtpService
• SmtpService
• SmtpResult

@hishamco @Piedone @BenedekFarkas

Here is a quick demo of the UI

Fixes #14699.

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[Piedone]

I'm sure it's awesome, and I appreciate the effort, really, but after spending days reviewing #14749 and finally being OK with it, I don't think I have it in me to do it once again anytime soon. Especially that it seems that this is something from scratch, as opposed to starting from that branch.

I'd emphasize here too that this feature would be only usable for us if it doesn't need UI configuration, but everything can come from configuration providers. I'm not sure if that's the case.

[MikeAlhayek]

I'm sure it's awesome, and I appreciate the effort, really, but after spending days reviewing #14749 and finally being OK with it, I don't think I have it in me to do it once again anytime soon. Especially that it seems that this is something from scratch, as opposed to starting from that branch.

I understand. However, I would not have spent the time to re-implement that PR if I did it's not worth it. At the end of the day, we want to keep the code in the framework clean and extensible. I highlighted the advantages on this PR so that way we can compare the advantage/disadvantage of each approach and bring into main the better one. Even if you don't want to review the code, I would review the argument regardless how long that other PR took. (Both provider you what you need which is ACS so it's a win)

I you can to spend 10 mins on this PR, please read the docs included in the PR to understand some of the changes. Also, if you can, check out the branch and test out the ACS provider and make sure you can quickly perform a test. You can use the test UI to ensure that the service is working.

Also, don't forget that the code in the other PR has breaking changes and is not extensible as this one provides.

I'd emphasize here too that this feature would be only usable for us if it doesn't need UI configuration, but everything can come from configuration providers. I'm not sure if that's the case.

There is an option to PreventUIConnectionChange in the appsettings that would prevent the user from being able to configure the connection string form the UI which is the scenario you would use.

[Piedone]

I'll get back to you in the next days.

[BenedekFarkas]

I did some functional testing. I didn't compare the user flow with main, so not everything here might be down to the changes in this branch.
Starting point is the Agency recipe and then enabled OrchardCore.Email.Azure.

1. ✔️Without adding app settings for OrchardCore.Email.Azure, there are no providers to select under "/Admin/Settings/email" and "/Admin/Email/Test" shows the "No Email providers are available. Enable at least one Email provider feature." error, even though the SMTP-provider should still be available at this point.
2. ✔️Since the UI exists to override those settings, IMO it should be available when there are no associated app settings, so it can be provided from scratch too.
3. ✔️ PreventUIConnectionChange: I'd rather call this AllowAdminSettingsOverride or something like that and flip the conditions that use it - at least I think it's easier to read/use such code when they don't need to be negated. Should be true by default (as per the previous point).
4. Both providers have their own checkbox to enable them, but the Azure provider still appears in the provider selection list, even when "Enable Azure Provider" is false. IMO we shouldn't have these checkboxes at all and this should be just governed by having feature enabled or not (but that would require the SMTP provider to be moved into a separate feature).
5. ✔️Following the user flow from the previous item, I can't save the provider selection due to a validation error in the SMTP-provider's editor:
   
6. ✔️When I try to modify Azure mail settings (while there's no provider selected on the first tab and "Enable SMTP Provider" is also false), I get these validation errors:
   
   The DefaultSender field is required. is probably coming from the SMTP-tab.
7. ✔️If I set "Enable Azure Provider" to false and try with the SMTP-setting instead, "Sender email address" is not persisted (there are no validation errors, but the value is gone after reload).
   EDIT: 5-6-7 moved to a new thread.
8. ✔️Otherwise (if I just go to the email test page directly without doing anything else), sending test emails through Azure works like a charm!!!

[MikeAlhayek]

Without adding app settings for OrchardCore.Email.Azure, there are no providers to select under "/Admin/Settings/email" and "/Admin/Email/Test" shows the "No Email providers are available. Enable at least one Email provider feature." error, even though the SMTP-provider should still be available at this point.

Are you saying that we should enable the SMTP provider by default? But, what if it is not configured? The ISmtpService will be available to prevent breaking change. I guess we could check and see if the SmtpSettings has all the required values and enable it by default. I'll look into this.

Additionally, OrchardCore.Email.Azure is a feature that you would have to configure in order to use it. You can add a connection string via the appsettings.json or the environment variable if you want all your tenants to use the same default configuration. But, also the user can configure the connection via UI "if you did not prevent this behavior from the appsettings".

Since the UI exists to override those settings, IMO it should be available when there are no associated app settings, so it can be provided from scratch too.

I am not sure what you are referring to here. When there are no appsettings provided, the user is able to configure the provider from UI.

PreventUIConnectionChange: I'd rather call this AllowAdminSettingsOverride or something like that and flip the conditions that use it - at least I think it's easier to read/use such code when they don't need to be negated. Should be true by default (as per the previous point).

I think false as default is better. PreventAdminSettingsOverride but I can change it bit an issue.

Both providers have their own checkbox to enable them, but the Azure provider still appears in the provider selection list, even when "Enable Azure Provider" is false. IMO we shouldn't have these checkboxes at all and this should be just governed by having feature enabled or not (but that would require the SMTP provider to be moved into a separate feature).

If the checkbox is not checked, it should not be enabled. Maybe it's a bug. I'll look.

The idea of providers is that you don't need to depend on features. Also, what if you enable a feature but never configure the provider? You must configure a provider before you use it. Enabling a feature is not enough to get a provider to work unless you configure it. That's the nature of providers.

Following the user flow from the previous item, I can't save the provider selection due to a validation error in the SMTP-provider's editor:

Yes if you enable the SMTP provider, you need to configure it properly. There is another issue open to highlight the tabs when a tab contain an error. That is something we can address in a separate PR which is related on how currently tabs behave.

When I try to modify Azure mail settings (while there's no provider selected on the first tab and "Enable SMTP Provider" is also false), I get these validation errors:

I'll look

If I set "Enable Azure Provider" to false and try with the SMTP-setting instead, "Sender email address" is not persisted (there are no validation errors, but the value is gone after reload).

I'll look. You should see this error "The selected provider is invalid or no longer enabled."

I'll update you once I address the bugs you mentioned.

[hishamco]

Maybe I come late, just a quick thing why I SHOULD enable the provider if the feature already enable? I presume enabling the feature indicates that it should works without any further settings

[MikeAlhayek]

@BenedekFarkas I addressed most if not all your feedback. Please try it out again.

Now, if the provider is configured previously (like SMTP settings already exists), we'll auto enable the the provider. The same goes to configuration provided by the appsettings/environment variables. If you pre-configure a provider from file, we'll auto enable the provider.

Can you confirm that you are able to test the Azure email provider again using the test UI to make sure it sends messages as we are expecting?

Maybe I come late, just a quick thing why I SHOULD enable the provider if the feature already enable? I presume enabling the feature indicates that it should works without any further settings

@hishamco in the case of Azure Email, if you provide settings in the appsettings, then enabling the feature will auto enable the provider for you. But, if you enable the feature without any configuration, then you'll need to enable the provider by configuring it using the UI. Without configuration a provider is useless as it'll always fail.

[Piedone]

Looks good! @BenedekFarkas @hishamco if you'd like, please review it. I'm unsubscribing from this PR now, so if you want to involve me, please @mention me. Otherwise, I'm good to merge this and needn't be involved anymore.

[hishamco]

The thing that still surprised me was how I can enable/disable an email provider after I enable its' feature, this is confusing from the user perspective. I already mentioned that a long time ago, but I'm not sure what you agreed with @Piedone

[Piedone]

I don't have anything else to add on that topic.

[hishamco]

Seems you need to merge only :) but that's a crucial question from the beginning

[MikeAlhayek]

The thing that still surprised me was how I can enable/disable an email provider after I enable its' feature, this is confusing from the user perspective. I already mentioned that a long time ago, but I'm not sure what you agreed with

@hishamco each feature compose multiple provider. If you provide settings using the configuration provider, you won't need to worry about the UI at all. But, if you want to provider tenant level settings, then you enable the tenant level providers.

Think of it as a provider for configuration provider level. and another provider for admin settings. You can have one enabled or both at the same time.

[hishamco]

@hishamco each feature compose multiple provider

How, I presume each feature enables certain type of email provider

[MikeAlhayek]

@BenedekFarkas any additional feedback? I would like to merge this PR soon unless you have additional requests

[hishamco]

This is a binary-breaking change!!

[MikeAlhayek]

Only if you are depending on SmtpService directly. Usually ISmtpService is injected and used. So if someone in inheriting from SmtpService directly, they would have a compile time error. So it is okay.

[BenedekFarkas]

@BenedekFarkas any additional feedback? I would like to merge this PR soon unless you have additional requests

I'll do some final functional testing later today!

[BenedekFarkas]

Works great, thanks! One minor thing I noticed is related to the default provider - repro steps:

1. I have both the SMTP and Azure Email features enabled and their settings set up.
2. Select Azure as the default provider.
3. Disable the Azure Email feature.
4. Email sending (from a workflow) fails.

I believe in this case the provider selection should automatically fall back to SMTP (in generic terms, another enabled provider, if there is one).
BTW if I navigate to the email settings screen, then it looks like that the SMTP provider is selected as default (because there's no other value in the dropdown), but due to the above, it's not actually selected.

[MikeAlhayek]

@BenedekFarkas good catch. That issue is now fixed. Let me know if you see anything else or if you approve.

[BenedekFarkas]

Awesome! Can't wait to start using it for our sites!