Add IUserTimeZoneService to make it easier to mock UserTimeZoneService

[hishamco]

Fixes #16613

[MikeAlhayek]

Additionally, add this change to the list of breaking changes in 2.0.

[hishamco]

Renaming the methods will be a breaking changes

[MikeAlhayek]

Renaming the methods will be a breaking changes

Introducing the interface is already a breaking change :)

[hishamco]

Introducing the interface is already a breaking change :)

It doesn't break anything, anyhow I agree with your suggestions, @deanmarcussen if you don't have any objection I will react to @MikeAlhayek changes

[MikeAlhayek]

It doesn't break anything

@hishamco You are no longer registering the service directly. So now anyone that is injecting that service directly will be broken.

you can instead do this to avoid this breaking change

services.AddScoped<UserTimeZoneService>();
services.AddScoped<IUserTimeZoneService>(sp => sp.GetSetvice<UserTimeZoneService>());

Then add [Obsolete] method over the 3 methods that are not part of the interface so this way people have transition path. In 3.0, we can then just removed the obsolete methods.

Either way, an interface should be little more generic. So since you are introducing an interface, I rather have a cleaner contract

[hishamco]

While 2.0.0 is coming soon, no problem with renaming and introducing a new interface, I will update the PR ASAP

[sebastienros]

Please wait, I want to understand more about the issue first

Whatever

[MikeAlhayek]

@hishamco we triaged this today. @sebastienros @gvkries and myself agreed with my change request. Please make the requested changed and ping me again.

[hishamco]

@MikeAlhayek does this mean any service we have should have an interface for extensibility?

[MikeAlhayek]

I don't think we should just blindly start creating interfaces because we want to abstract everything. Ideally, this kind of service will be internal so they are not exposed.

But, when we decide we need an interface, preferable we would add a reusable interface without including any implementation info. For example, in this case we want UserTimeZone not CurrentUserTimeZone.

So the interface would look either

public interface IUserTimeZone
{
    ValueTask<ITimeZone> GetAsync(IUser user);

    ValueTask<ITimeZone> UpdateAsync(IUser user);
}

or

public interface ICurrentUserTimeZone
{
    ValueTask<ITimeZone> GetAsync();

    ValueTask<ITimeZone> UpdateAsync();
}

This way we do not repeat the context. In this case, I think we should use IUserTimeZone and keep it generic.

[MikeAlhayek]

@hishamco not exactly

[MikeAlhayek]

@hishamco are you planning to finalize this soon as part of 2.0 or should we push this out to 3.0?

[hishamco]

@MikeAlhayek I just removed the extension methods, for now, we are ready to ship this

[MikeAlhayek]

@hishamco

[hishamco]

@MikeAlhayek please check the changes before suggest code change, seems the build is broken

[MikeAlhayek]

@hishamco feel free to merge if you good with my last changs.