-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Azure Email Communication Services (Lombiq Technologies: OCORE-129) #14749
Conversation
@hishamco i know this isn't review-ready PR so I am not going to review it yet. However, I want to suggest something here so it can be more useful for everyone. I suggest that you add an email configuration that would allow the user to change the provide using UI. I did the same thing in the SMS module. For example, you can have multiple IEmailProviders, (ex, SMTP, Azure and any other the user wants to define.) In the UI the user can change the provider from a drop down menu. If the user select SMTP, then they'll see the current configuration we used to. If they select Azure, then we'll hide existing options and show the Azure configuration only. The benefit of this is that we can add other providers. For example, I have a provider called "SystemProvider" that I use in my projects. This allows all the tenant to utilize a pre-configured email service out of the box. They can change it to something else if they wish. The idea is to have an easy way to implement providers, and an Easy way to configure it per-tenant using UI. Again, a good example to follow is the SMS provider and how I implemented the drivers to allow for show/hide sections easily. Let me know if the idea is not clear or if you have questions. |
Thanks for the early feedback. According to the issue, the plan is to use Email through ACS and not to depend on the SMTP feature, the settings should be provided from |
@hishamco For example, in the Sms I created a generic SmsSettings provider which allow you to expose all the providers in a drop down menu and the user is able to hide/show the settings that are only related to the select provider Then I created a Twilio provider settings specific in another provider as you can see here Then here you can see how I register SMS providers "and can have as many providers as needed". In your case you can have OrchardCore/src/OrchardCore/OrchardCore.Sms.Core/ServiceCollectionExtensions.cs Lines 45 to 62 in 582653c
The null service can be as simple as this. In your case it would just implement a simple Here is a TwilioSmsProvider Using a simple factory, you can return the provider based on the options. Here is the factory that I implemented in the SMS OrchardCore/src/OrchardCore/OrchardCore.Sms.Core/ServiceCollectionExtensions.cs Lines 20 to 38 in 582653c
Then, you can just inject ISmsProvider into any class where you want to use ISmsPrivder and it'll return an instances based on the configured provider. |
src/OrchardCore.Modules/OrchardCore.Email/Controllers/AdminController.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should only need a single implementation of IEmailService (DefaultEmailService). This should resolve IEmailProvider based on site settings. The service will fire events "if needed" like sending, sent, failed... this service will call provider.SendAsync and the provider will know how to handle the message. If you don't want to fire events, you can drop the EmailService and just call the SendAsync on the provider directly. The settings can have a single provider to use. This will simplify the implementation and you'll likely to have less classes and more simple implementation.
@MikeAlhayek I reacted to almost all of your changes, I left some that we can revisit later. I don't want to change the old code as much as I can, because it requires a lot of testing. Let me try the last test by removing the "Order` |
@MikeAlhayek As I said the "Order" was for a reason before, now it's no longer needed, so can we merge this and then revisit the other related comments such as releasing the shell |
@hishamco can you please hold off merging this PR for a day? I am almost done with an implementation that utilizes provides as originally suggested. I think the concept is miss-understand and I think it is very valuable. I'll ping you once it is ready so you can compare the 2 PRs. |
@MikeAlhayek I'm sorry, but I definitely won't have time to review your PR this week. TBH I got lost in the refactoring process and I lack the necessary context at the moment to fully understand the extent of the changes. I'll point out again that I'd like to have this feature shipped as soon as practically possible (I really want to use it in Production!) - not sure how much is left to achieve that but a PR with so many comments probably warrants a live discussion to make reaching an agreement easier. If you agree and it's still useful, then let's do it. |
@BenedekFarkas I understand if you can't do a full review on PR #15254. Can you at least test it out? Follow the instruction here https://github.com/OrchardCMS/OrchardCore/blob/8e144c6412fe9c223d5954c89ff87ce181f38d84/src/docs/reference/modules/Email.Azure/README.md and see if enabling the Azure provider works as expected? Please provide feedback that are related to the other PR in the other PR not here |
This pull request has merge conflicts. Please resolve those before requesting a review. |
What's the status of this PR or it's replaced by #15254? |
#15254 supersedes this, so we can close. However, this was an enormous duplicated effort, so to prevent this in the future, I propose @MikeAlhayek (or anybody else) that the next time you think a PR needs to be significantly reworked then a) tell this early and b) try to start from the existing PR unless it's completely infeasible. |
I just lost one of my biggest PR 💣💣💣 :) |
Sorry you feel that way. But, you did not lose anything. Your code was utilized in the other PR. Team effort |
No problem, I'm still fighting for the longest PR ever #4466 five years ago :) |
I agree with Mike and thank you both for the effort! |
No problem but I prefer a second time to have any feedback in the early stages not after spending months and getting a final approval. Again I'm glad to have the best for OC, thanks Mike |
Fixes #14699