Fix the User TimeZoneService

[MikeAlhayek]

Fix #16932

[hishamco]

FYI I just wrote a comment here to allow @KeithRaven to chime again on his PR after there are a few comments later

I suggest closing this and letting Keith finish his PR unless he doesn't have time. I hope you understand what I mean @MikeAlhayek

[MikeAlhayek]

@hishamco, I am looking at that PR now. That PR does not fix the problem this PR fixes.

[hishamco]

I never mind creating another PR, at least we need to know the status of the previous PR, and why it's closed. Could the author responds to the feedback

[KeithRaven]

I never mind creating another PR, at least we need to know the status of the previous PR, and why it's closed. Could the author responds to the feedback

Sorry I was going to sort out #15796 however it was mentioned that another issue needed resolving first #15796 (comment).

This PR looks to be covering the same thing and more though, so makes sense to keep it in a single change set, so happy to leave the other closed.

[MikeAlhayek]

@deanmarcussen I believe this will fix your issue. However, we @sebastienros and I have two different opinions on the implementation. We need your opinion to determine the final implementation before we merge.

@sebastienros suggest that we remove both IUserTimeZoneService and UserTimeZoneService. Instead, place the same code in the UserTimeZoneSelector. So that the UserTimeZoneSelector will be responsible of caching the timezone for the current user and we have one less service to deal with. #16932 (comment)

Personally, I think it is okay to keep IUserTimeZoneService and UserTimeZoneService to allow others to reuse this service instead of having to couple that logic in the UserTimeZoneSelector. This will also allow us to get the timeZoneId of any user not just the current user. Also, this won't be a breaking change and can be done in 2.1 instead of waiting until 3.0 "unless we want to leave IUserTimeZoneService as a useless service marked obsolete.

Either approaches are valid for me with no strong opinion. The question here, would you keep or remove both IUserTimeZoneService and UserTimeZoneService?

If we decide to remove IUserTimeZoneService, the UserTimeZoneSelector will look like this:

public class UserTimeZoneSelector : UserEventHandlerBase, ITimeZoneSelector
{
    private const string CacheKey = "UserTimeZone/";
    private const string EmptyTimeZone = "NoTimeZoneFound";

    private static readonly DistributedCacheEntryOptions _slidingExpiration = new()
    {
        SlidingExpiration = TimeSpan.FromHours(1),
    };

    private readonly IHttpContextAccessor _httpContextAccessor;
    private readonly IClock _clock;
    private readonly IDistributedCache _distributedCache;
    private readonly UserManager<IUser> _userManager;

    public UserTimeZoneSelector(
        IHttpContextAccessor httpContextAccessor,
        IClock clock,
        IDistributedCache distributedCache,
        UserManager<IUser> userManager)
    {
        _httpContextAccessor = httpContextAccessor;
        _clock = clock;
        _distributedCache = distributedCache;
        _userManager = userManager;
    }

    public Task<TimeZoneSelectorResult> GetTimeZoneAsync()
    {
        var result = !_httpContextAccessor.HttpContext.User.Identity.IsAuthenticated
            ? null
            : new TimeZoneSelectorResult
            {
                Priority = 100,
                TimeZoneId = async () =>
                {
                    var timeZone = await GetAsync();

                    return timeZone?.TimeZoneId;
                },
            };

        return Task.FromResult(result);
    }

    public override Task DeletedAsync(UserDeleteContext context)
        => ForgetCacheAsync(context.User.UserName);

    public override Task UpdatedAsync(UserUpdateContext context)
        => ForgetCacheAsync(context.User.UserName);

    public override Task DisabledAsync(UserContext context)
        => ForgetCacheAsync(context.User.UserName);

    private async ValueTask<ITimeZone> GetAsync()
    {
        var currentTimeZoneId = await GetTimeZoneIdAsync();

        if (string.IsNullOrEmpty(currentTimeZoneId))
        {
            return null;
        }

        return _clock.GetTimeZone(currentTimeZoneId);
    }

    private async ValueTask<string> GetTimeZoneIdAsync()
    {
        var principal = _httpContextAccessor.HttpContext.User;

        var userName = principal.Identity.Name;

        var key = GetCacheKey(userName);

        var timeZoneId = await _distributedCache.GetStringAsync(key);

        // The timeZone is not cached yet, resolve it and store the value.
        if (string.IsNullOrEmpty(timeZoneId))
        {
            // At this point, we know the timeZoneId is not cached for the given userName.
            // Retrieve the user and cache the timeZoneId.
            var user = await _userManager.GetUserAsync(principal);

            if (user is User u)
            {
                timeZoneId = u.As<UserTimeZone>()?.TimeZoneId;
            }

            // We store a placeholder string to indicate that there is no specific value for this user.
            // This approach ensures compatibility with distributed cache implementations that may not support null values.
            // Caching this placeholder helps avoid redundant queries for this user on each request when no time zone is set.
            if (string.IsNullOrEmpty(timeZoneId))
            {
                timeZoneId = EmptyTimeZone;
            }

            await _distributedCache.SetStringAsync(key, timeZoneId, _slidingExpiration);
        }

        // If TimeZoneId matches the placeholder value, we return null instead of the placeholder itself.
        if (timeZoneId == EmptyTimeZone)
        {
            return null;
        }

        return timeZoneId;
    }

    private Task ForgetCacheAsync(string userName)
    {
        var key = GetCacheKey(userName);

        return _distributedCache.RemoveAsync(key);
    }

    private static string GetCacheKey(string userName)
        => CacheKey + userName;
}

[deanmarcussen]

I don't have a massively strong opinion either way.

We used the service because it was there, and largely because we needed to be able to differentiate between user time, site time, and utc, particularly being able to provide those values to javascript.

Because we work in a very time distributed world, we need to present to users 2-3 date times to users, to save sanity.
For example, we have sites in EU data centers which will run jobs at specific times, but they are often supported by users in the US, so being able to present what time a job will run at in both those timezones is a valid use case.

So realistically the timezone selector option doesn't work as ideally for us, as its results don't specify exactly what that timezone refers to. I can register it manually, and run it manually.

Mostly I've opened tickets about this service, to improve them for the community, and if the service goes away, then we will just adopt it internally, and manage it outselves.

I personally think it's a useful service, and concept (anyone implementing it themselves will probably forget to cache it, and drop their performance down, without knowing why)

[MikeAlhayek]

@sebastienros it sounds to me that @deanmarcussen is also leaning toward keeping the service. Unless you have a strong opinion on removing the service and move it into the time zone selector, I suggest we leave it.

[sebastienros]

Cool then @MikeAlhayek you can merge.

I wondered if we should keep IUser but in the end the service can always load it if it needs it.