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.

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

Cleanup ISmsService #15142

Merged
merged 2 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ public class AdminController : Controller
private readonly INotifier _notifier;
private readonly IAuthorizationService _authorizationService;
private readonly ISmsProviderResolver _smsProviderResolver;
private readonly ISmsService _smsService;

protected readonly IHtmlLocalizer H;
protected readonly IStringLocalizer S;
Expand All @@ -27,7 +26,6 @@ public AdminController(
IOptions<SmsProviderOptions> smsProviderOptions,
IPhoneFormatValidator phoneFormatValidator,
ISmsProviderResolver smsProviderResolver,
ISmsService smsService,
INotifier notifier,
IAuthorizationService authorizationService,
IHtmlLocalizer<AdminController> htmlLocalizer,
Expand All @@ -36,7 +34,6 @@ public AdminController(
_smsProviderOptions = smsProviderOptions.Value;
_phoneFormatValidator = phoneFormatValidator;
_smsProviderResolver = smsProviderResolver;
_smsService = smsService;
_notifier = notifier;
_authorizationService = authorizationService;
H = htmlLocalizer;
Expand Down Expand Up @@ -72,19 +69,19 @@ public async Task<IActionResult> Test(SmsTestViewModel model)

if (provider is null)
{
ModelState.AddModelError(nameof(model.PhoneNumber), S["Please select a valid provider."]);
ModelState.AddModelError(nameof(model.Provider), S["Please select a valid provider."]);
}
else if (!_phoneFormatValidator.IsValid(model.PhoneNumber))
{
ModelState.AddModelError(nameof(model.PhoneNumber), S["Please provide a valid phone number."]);
}
else
{
var result = await _smsService.SendAsync(new SmsMessage()
var result = await provider.SendAsync(new SmsMessage()
{
To = model.PhoneNumber,
Body = S["This is a test SMS message."]
}, provider);
});

if (result.Succeeded)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ namespace OrchardCore.Sms;
public interface ISmsProviderResolver
{
/// <summary>
/// Gets the SMS provider for the given name.
/// When null is null or empty, it gets the default SMS provider.
/// Gets the SMS provider for the technical given name.
/// When null or empty string is provided, it returns the default SMS provider.
/// </summary>
/// <param name="name">The key of the SMS provider</param>
/// <returns>Instance ISmsProvider or null when no service found.</returns>
Expand Down
6 changes: 2 additions & 4 deletions src/OrchardCore/OrchardCore.Sms.Abstractions/ISmsService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,9 @@ namespace OrchardCore.Sms;
public interface ISmsService
{
/// <summary>
/// Send the given message.
/// Send the given message using the default provider.
/// </summary>
/// <param name="message">The message to send.</param>
/// <param name="provider">An SMS Provider to use. When null, we sent using the default provider.</param>
/// <returns>SmsResult object.</returns>
Task<SmsResult> SendAsync(SmsMessage message, ISmsProvider provider = null);

Task<SmsResult> SendAsync(SmsMessage message);
}
7 changes: 5 additions & 2 deletions src/OrchardCore/OrchardCore.Sms.Abstractions/SmsResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ public class SmsResult
/// <summary>
/// Returns an <see cref="SmsResult"/> indicating a successful SMS operation.
/// </summary>
public static SmsResult Success { get; } = new() { Succeeded = true };
public readonly static SmsResult Success = new()
{
Succeeded = true
};

/// <summary>
/// An <see cref="IEnumerable{LocalizedString}"/> containing an errors that occurred during the SMS operation.
Expand All @@ -33,6 +36,6 @@ public static SmsResult Failed(params LocalizedString[] errors)
=> new()
{
Succeeded = false,
Errors = errors
Errors = errors ?? [],
};
}
9 changes: 5 additions & 4 deletions src/OrchardCore/OrchardCore.Sms.Core/Services/SmsService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ namespace OrchardCore.Sms.Services;
public class SmsService : ISmsService
{
private readonly ISmsProviderResolver _smsProviderResolver;
private ISmsProvider _provider;
Comment on lines 8 to +9
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private readonly ISmsProviderResolver _smsProviderResolver;
private ISmsProvider _provider;
private ISmsProvider _provider;
private readonly ISmsProviderResolver _smsProviderResolver;

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 usually leave the static variables top, then readonly then the writable privates. It's done that way everywhere

Copy link
Member

Choose a reason for hiding this comment

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

You're right


protected readonly IStringLocalizer S;

Expand All @@ -17,15 +18,15 @@ public SmsService(
S = stringLocalizer;
}

public async Task<SmsResult> SendAsync(SmsMessage message, ISmsProvider provider = null)
public async Task<SmsResult> SendAsync(SmsMessage message)
{
provider ??= await _smsProviderResolver.GetAsync();
_provider ??= await _smsProviderResolver.GetAsync();

if (provider == null)
if (_provider is null)
{
return SmsResult.Failed(S["SMS settings must be configured before an SMS message can be sent."]);
}

return await provider.SendAsync(message);
return await _provider.SendAsync(message);
}
}