Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cleanup the SMS settings #14171

Merged
merged 1 commit into from
Aug 18, 2023
Merged

Cleanup the SMS settings #14171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/OrchardCore.Modules/OrchardCore.Sms/Drivers/SmsSettingsDisplayDriver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ namespace OrchardCore.Sms.Drivers;

public class SmsSettingsDisplayDriver : SectionDisplayDriver<ISite, SmsSettings>
{
public const string GroupId = "sms";

private readonly IHttpContextAccessor _httpContextAccessor;
private readonly IAuthorizationService _authorizationService;
private readonly IShellHost _shellHost;
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

public override async Task<IDisplayResult> UpdateAsync(SmsSettings settings, BuildEditorContext context)
{
var user = _httpContextAccessor.HttpContext?.User;

if (!context.GroupId.Equals(GroupId, StringComparison.OrdinalIgnoreCase)
if (!context.GroupId.Equals(SmsConstants.SettingsGroupId, StringComparison.OrdinalIgnoreCase)
|| !await _authorizationService.AuthorizeAsync(user, SmsPermissions.ManageSmsSettings))
{
return null;
Expand Down
4 changes: 2 additions & 2 deletions src/OrchardCore.Modules/OrchardCore.Sms/Drivers/TwilioSettingsDisplayDriver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,14 @@ public override async Task<IDisplayResult> EditAsync(TwilioSettings settings, Bu
model.AccountSID = settings.AccountSID;
model.HasAuthToken = !String.IsNullOrEmpty(settings.AuthToken);
}).Location("Content:5")
.OnGroup(SmsSettingsDisplayDriver.GroupId);
.OnGroup(SmsConstants.SettingsGroupId);
}

public override async Task<IDisplayResult> UpdateAsync(TwilioSettings settings, BuildEditorContext context)
{
var user = _httpContextAccessor.HttpContext?.User;

if (!context.GroupId.Equals(SmsSettingsDisplayDriver.GroupId, StringComparison.OrdinalIgnoreCase)
if (!context.GroupId.Equals(SmsConstants.SettingsGroupId, StringComparison.OrdinalIgnoreCase)
|| !await _authorizationService.AuthorizeAsync(user, SmsPermissions.ManageSmsSettings))
{
return null;
Expand Down
7 changes: 3 additions & 4 deletions ...Core.Modules/OrchardCore.Sms/AdminMenu.cs → ...e.Modules/OrchardCore.Sms/SmsAdminMenu.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,14 @@
using Microsoft.Extensions.Localization;
using OrchardCore.Navigation;
using OrchardCore.Sms;
using OrchardCore.Sms.Drivers;

namespace OrchardCore.Admin;

public class AdminMenu : INavigationProvider
public class SmsAdminMenu : INavigationProvider
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have kept it as AdminMenu and only changed the above namespace to OrchardCore.Sms.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is what we do when possible for Permissions, Migrations, AdminMenu and so on.

At least the namespace is wrong.

{
protected readonly IStringLocalizer S;

public AdminMenu(IStringLocalizer<AdminMenu> stringLocalizer)
public SmsAdminMenu(IStringLocalizer<SmsAdminMenu> stringLocalizer)
{
S = stringLocalizer;
}
Expand All @@ -29,7 +28,7 @@ public Task BuildNavigationAsync(string name, NavigationBuilder builder)
.Add(S["SMS"], S["SMS"].PrefixPosition(), sms => sms
.AddClass("sms")
.Id("sms")
.Action("Index", "Admin", new { area = "OrchardCore.Settings", groupId = SmsSettingsDisplayDriver.GroupId })
.Action("Index", "Admin", new { area = "OrchardCore.Settings", groupId = SmsConstants.SettingsGroupId })
.Permission(SmsPermissions.ManageSmsSettings)
.LocalNav()
)
Expand Down
2 changes: 1 addition & 1 deletion src/OrchardCore.Modules/OrchardCore.Sms/Startup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public override void ConfigureServices(IServiceCollection services)
}

services.AddScoped<IPermissionProvider, SmsPermissionProvider>();
services.AddScoped<INavigationProvider, AdminMenu>();
services.AddScoped<INavigationProvider, SmsAdminMenu>();
services.AddScoped<IDisplayDriver<ISite>, SmsSettingsDisplayDriver>();
}
}
Expand Down
15 changes: 7 additions & 8 deletions src/OrchardCore.Modules/OrchardCore.Sms/Views/SmsSettings.Edit.cshtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
<span asp-validation-for="DefaultProvider"></span>
</div>

<script at="Foot">
<script at="Foot" asp-name="sms-settings-providers">
document.addEventListener('DOMContentLoaded', () => {

let menu = document.getElementById('@Html.IdFor(m => m.DefaultProvider)');
Expand All @@ -19,13 +19,12 @@
menu.addEventListener('change', (e) => {

for (let i = 0; i < settingContainers.length; i++) {
settingContainers[i].classList.add('d-none');
}

let wrapper = document.getElementById('SmsSettingsContainer' + e.target.value);

if (wrapper) {
wrapper.classList.remove('d-none');
let container = settingContainers[i];
if (container.getAttribute('data-provider-name') == e.target.value) {
container.classList.remove('d-none');
} else {
container.classList.add('d-none');
}
}
});

Expand Down
23 changes: 1 addition & 22 deletions src/OrchardCore.Modules/OrchardCore.Sms/Views/TwilioSettings.Edit.cshtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

@model TwilioSettingsViewModel

<div class="sms-settings-container d-none" id="SmsSettingsContainer@(SmsConstants.TwilioServiceName)">
<div class="sms-settings-container d-none" data-provider-name="@(SmsConstants.TwilioServiceName)">
<h4>@T["Twilio Account Info"]</h4>
<div class="mb-3" asp-validation-class-for="PhoneNumber">
<label asp-for="PhoneNumber">@T["Phone Number"]</label>
Expand All @@ -28,24 +28,3 @@
}
</div>
</div>

<script at="Foot">
document.addEventListener('DOMContentLoaded', () => {

let menu = document.getElementById('@Html.IdFor(m => m.DefaultProvider)');
let settingContainers = document.getElementsByClassName('sms-settings-container');

menu.addEventListener('change', (e) => {

for (let i = 0; i < settingContainers.length; i++) {
settingContainers[i].classList.add('d-none');
}

let wrapper = document.getElementById('SmsSettingsContainer' + e.target.value);

if (wrapper) {
wrapper.classList.remove('d-none');
}
});
});
</script>
2 changes: 2 additions & 0 deletions src/OrchardCore/OrchardCore.Sms.Core/SmsConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,6 @@ public class SmsConstants
public const string TwilioServiceName = "Twilio";

public const string ConsoleServiceName = "Console";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

OrchardCore/src/OrchardCore/OrchardCore.Abstractions/Shell/Builders/Extensions/ServiceProviderExtensions.cs

Lines 16 to 22 in 2349676

/// <summary>
/// Instantiates a new object of the specified type, but with support for constructor injection.
/// </summary>
public static TResult CreateInstance<TResult>(this IServiceProvider provider, Type type) where TResult : class
{
return (TResult)ActivatorUtilities.CreateInstance(provider, type);
}

Or use directly ActivatorUtilities.CreateInstance().

Copy link
Member Author

@MikeAlhayek MikeAlhayek Aug 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

OrchardCore/src/OrchardCore/OrchardCore.Sms.Core/ServiceCollectionExtensions.cs

Line 48 in 2349676

options.Providers.Add(name, (sp) => sp.GetService<T>());

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you also have just before

    services.AddSingleton<T>();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a little typo, EnsureClientIsIsInitializedAsync() may need to be renamed to EnsureClientIsInitializedAsync().

Copy link
Member Author

@MikeAlhayek MikeAlhayek Aug 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public const string SettingsGroupId = "sms";
}