Add ISmsService and support multiple SMS Providers

[MikeAlhayek]

We improved the UI settings by to grouping each provider settings into it's own tab to simply have a one place to configure all settings

Also, adding a way to test out SMS providers.

[hishamco]

Lol, I planned to that but you was fast, please refer to my email PR :)

[MikeAlhayek]

@hishamco I saw that now. Good approach. I think there is a bug along the lines when trying to resolve dependencies for some reason. I have to find time to resolve them. What I had before does the same thing as the keyed services. I just need to know how to get a list of a dictionary with the keyed service along with their key. They figure out the issue with the dependency injection.

[hishamco]

I just need to know how to get a list of a dictionary with the keyed service along with their key

Why you need to do that, you can get the service(s) by its key

[MikeAlhayek]

Why you need to do that, you can get the service(s) by its key

We need to create a menu with key/name pair in the settings so that the user can select which provider to use from the UI (driver). Then we store the key in the site settings. The SmsService will use the provider based on the configured value.

[MikeAlhayek]

@jtkech I am noticing some odd behavior when registering keyed services.

As you can see I am registering keyed service like this

    public static IServiceCollection AddTwilioSmsProvider(this IServiceCollection services)
        => services.AddKeyedScoped<ISmsProvider, TwilioSmsProvider>(TwilioSmsProvider.TechnicalName);

    public static IServiceCollection AddLogSmsProvider(this IServiceCollection services)
        => services.AddKeyedScoped<ISmsProvider, LogSmsProvider>(LogSmsProvider.TechnicalName);

But I am getting an exception in the Elasticsearch model!

Are we using a custom service provider that does not support keyed services? Any idea what is missing here?

[hishamco]

Please check my OC.Email.Azure PR I resolve this issue at shell level

[hishamco]

That's the question I forgot to ask you yesterday if you already test the code or not

[jtkech]

Are we using a custom service provider that does not support keyed services?

We build the shell service provider ourselves but not a custom one.

To support keyed services we need to do some changes, before checking the ImplementationType of a service descriptor we now need to check IsKeyedService and then KeyedImplementationType, otherwise it may throw, for example

    public Type? ImplementationType
    {
        get
        {
            if (IsKeyedService)
            {
                ThrowKeyedDescriptor();
            }
            return _implementationType;
        }
    }

[hishamco]

@jtkech I already resolve the same issue by checking the service in GetImplementationType() method, please refer to my email using ACS pr

[MikeAlhayek]

@hishamco please submit a PR with the ServiceProvider fix so we can take that soon.

[jtkech]

@hishamco

Just took a look, yes it "fixes" the issue but it is incomplete, need to also check KeyedImplementationType , KeyedImplementationInstance and so on.

Should be inspired by https://github.com/dotnet/runtime/blob/2158031420214184d22a29029ff24420472fcaa0/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceDescriptor.cs#L292 and adapted to our usage for example when we clone a container.

If we want to support keyed services I will need to do a separated PR.

[MikeAlhayek]

@jtkech are you free to submit a PR for this? If you do, ping me and I'll get it tested in the morning.

[jtkech]

Can't right now but I will do asap.

[hishamco]

@hishamco please submit a PR with the ServiceProvider fix so we can take that soon.

Sure, I will do it now

[hishamco]

I suggest splitting the settings for each SMS provider on a separate page because it becomes from different module

[MikeAlhayek]

I suggest splitting the settings for each SMS provider on a separate page because it becomes from different module

@hishamco from a user perspective, it is much easier to visit one page where you can configure all the related options. So here one place can be used to configure all the SMS providers and set the default provider.

These settings can be injected from different modules or features but should render on the same page. This way you don't have to confuse the user with so much instruction. All the instructions that you need is to direct them to the settings page.

Also, with this approach feature that needs to depends on SMS "for example 2FA - SMS", will only need to add dependency on the OC.Sms module not specify implementation. If you need to create a new provider in a separate module, all you need to do is depend on OC.Sms module. This is important structure to provide in OC. The idea is to expose a facade in one module "main module" and then have same or other modules compose providers.

[hishamco]

Seems you merged this so quick, I just planned to revise it tonight :(

[MikeAlhayek]

This PR has been open for couple months. But, if you think there is a need for modification, feel free to propose a PR

[hishamco]

Usually in OC we need one approval other than the creator to ensure everything is as expected, so there is no rush :)

This PR has been open for couple months.

FYI I have a few PRs opened for years :)