Cleanup the SMS settings #14171
Cleanup the SMS settings #14171
Conversation
| @@ -55,14 +53,14 @@ public override async Task<IDisplayResult> EditAsync(SmsSettings settings, Build | |||
| .Select(provider => new SelectListItem(provider, provider)) | |||
| .ToArray(); | |||
| }).Location("Content:1") | |||
| .OnGroup(GroupId); | |||
| .OnGroup(SmsConstants.SettingsGroupId); | |||
There was a problem hiding this comment.
Only for info as this PR is merged.
Not sure that's the best way to define constants.
Here could have been a const defined in the setiings => SmsSettings.GroupId.
Also I would have defined the constants and SmsSettings in the Sms.Abstractions.
I saw a SmsSettingsConfigurator, why not using the Configuration term as we usualy do?
In TwilioSmsProvider I saw EnsureClientIsIsInitializedAsync(), does it need to be thread safe / atomic?
There was a problem hiding this comment.
I rather have it is constants because it is a value that should be used by all implementation. Implementation don't need to do a hard reference to the SMS module. I know you can simply type "sms". But I thought it would be better to have a reusable constant.
The twilio does not need to be atomic I just did it this way so I don't have to initialize the client multiple times is the same request.
There was a problem hiding this comment.
I'm not against constants but how they are defined, for example I prefer SmsSettings.GroupId, SmsProvider.Twilio and so on.
Just saw that EnsureClientIsIsInitializedAsync() updates different fields of the twilio provider which is a singleton.
|
|
||
| namespace OrchardCore.Admin; | ||
|
|
||
| public class AdminMenu : INavigationProvider | ||
| public class SmsAdminMenu : INavigationProvider |
There was a problem hiding this comment.
We could have kept it as AdminMenu and only changed the above namespace to OrchardCore.Sms.
There was a problem hiding this comment.
Yes the namespace should have been changed. But having SmsAdminMenu is better to me than AdminMenu. There are many AdminMenu classes in the project and makes it harder to search for a file.
There was a problem hiding this comment.
Yes, this is what we do when possible for Permissions, Migrations, AdminMenu and so on.
At least the namespace is wrong.
| @@ -5,4 +5,6 @@ public class SmsConstants | |||
| public const string TwilioServiceName = "Twilio"; | |||
|
|
|||
| public const string ConsoleServiceName = "Console"; | |||
There was a problem hiding this comment.
Maybe another static class and fields so that we can use something like that SmsProvider.Twilio, SmsProvider.Console. Or maybe the sms providers should provide their names, then get the right one when resolving ISmsProvider, hmm but still need to register all providers in the DI and also resolve them.
Or keep the names in the options but don't register providers in the DI, only enlist types in place of resolution delegates, then activate the right one when resolving ISmsProvider, this by using our service provider extension.
Or use directly ActivatorUtilities.CreateInstance().
There was a problem hiding this comment.
If you look at the way they providers are registers, we register a delegate not an instance. So the provider is not initialized until it is needed based on the DefaultProvider
There was a problem hiding this comment.
But you also have just before
services.AddSingleton<T>();
There was a problem hiding this comment.
Also a little typo, EnsureClientIsIsInitializedAsync() may need to be renamed to EnsureClientIsInitializedAsync().
There was a problem hiding this comment.
Yea but isn't that definitions the type in the IoC it does not resolve it. It's is resolved at run time. Isn't it?
I noticed the typo. I'll clean it up in the morning
There was a problem hiding this comment.
Nothing urgent, need to leave too, will see tomorrow.
Yes, only resolved on demand, just to not add too much descriptors to lookup, but here only a few ones are added, for info at some point we decreased scoped services in favor of singletons, but still good to decrease both.
I think that the twilio initialization need to be thread safe.
No description provided.