Extending ISetupEventHandler with new events

[MikeAlhayek]

Fix #14170

@jtkech we discussed this in #14172.

This is the approach I had in mind. With the new ITenantSetupHandler, one can perform some setup logic after the tenant is setup.

Thoughts?

[MikeAlhayek]

@jtkech I am noticing an issue here that I can't seem to explain.

public class SelfTenantSetupHandler : TenantSetupHandlerBase
{
    private readonly INotifier _notifier;
    private readonly IHtmlLocalizer H;

    public TestSetup(
        INotifier notifier,
        IHtmlLocalizer<TestSetup> localizer)
    {
        _notifier = notifier;
        H = localizer;
    }

    public override async Task SuccessAsync()
    {
        await _notifier.SuccessAsync(H["You successfully have setup your website!"]);
    }
}

When the tenant is done setting up, we set set it's status to running and then reload the site. So SuccessAsync() is called in a new scope "after the site is reloaded". I am expecting that the above SelfTenantSetupHandler to show show the notification message. But it does not! Why would it not show the notification here? It's like the message is added to the current HttpContext which gets lost when we start a new scope?

Either way, the new ITenantSetupHandler allow us to perform logic when the tenant is setup successfully or failed. This should fix #14170. Also, this approach will allow us to fire a background job if needed by implementing ITenantSetupHandler.

[jtkech]

Yes INotifier is a scoped service so what you enlist in a new local inner scope is lost when returning back to the controller scope.

---

Yes I closed my PR to make a pause around this, waiting for after the incoming release and because I have some works in late, but I'm not against so let's restart the discussion ;)

Maybe we could have kept the existing ISetupEventHandler and just extended it, yes by making a SetupEventHandlerBase class so that custom implementations would have just to inherit from SetupEventHandlerBase in place of ISetupEventHandler to migrate.

I like the method names, if we use Failed() we could then use Succeeded(), hmm I think we could keep the "Setup" term, so SetupAsync() in place of SettingUpAsync(), anyway at this point the setup is almost done, only handlers may add errors. And maybe in this order.

Task SetupAsync(SetupContext context);
Task FailedAsync(SetupContext context);
Task SucceededAsync();

Just saw that if we failed to init the data store we alreay have enlisted an error, I think we are missing the following check also before reloading the shell.

        if (context.Errors.Any())
        {
            // Here do we need to call the failed event?
            return null;
        }

Or maybe handlers would have to check if there is already an error if we don't add the above check, but yes existing handlers don't have a direct access to the setup context, maybe there is a good reason.

Not fully sure we need a Faulted() event, what can we expect to do consistently if the tenant is not setup correctly, maybe here we are not at the right level to manage failed setups, we may need a host level service and / or handlers to manage failed and successful setups, but not fully sure ;)

I'm stopping, I will think about it and review it more in depth when I will have time.

[MikeAlhayek]

Maybe we could have kept the existing ISetupEventHandler and just extended it, yes by making a SetupEventHandlerBase class so that custom implementations would have just to inherit from SetupEventHandlerBase in place of ISetupEventHandler to migrate.

That would be a breaking change which is I am trying to avoid. Other people may already be implementing ISetupEventHandler directly in their project.

Not sure I follow you on the FailedAsync(). We currently have a way to report error which cancels the setup. But we don't have a way of knowing if the setup failed. Checking the errors does in the handler will not give us accurate status since the error could be called on the next handler.

I'll rename events as per your suggestions. Also check the condition that returns null.

[jtkech]

That would be a breaking change which is I am trying to avoid. Other people may already be implementing ISetupEventHandler directly in their project.

Yes I know that's why in my other PR (whose goal was not the same) I let it as is and only added a new handler. Here I thought about what would be better at the end, sometimes we can introduce intentionally a well documented breaking change, but as said I was not fully sure, and yes here maybe not worth to do.

Hmm, maybe a compromise would be to keep ISetupEventHandler as it is, this one being to collaborate to the setup itself, only done locally, then add a new interface, whatever the name, but to only handle the 2 new tenant setup events, the success event and now the failed event that you added.

Otherwise why not, maybe I'm wrong, only thinkings on the fly, as said would need to focus on it a little more.

You already described a little what you may do in a setup succeeded event, can you describe what you may do in a setup failed event?

[MikeAlhayek]

@jtkech FailedAsync could be used for logging or communication. For example, create support ticket in a help desk to alert that a self setup is failing the customer may need help. Or maybe one want to send an email to the SaaS support.

I went ahead and did some cleanup in the SetupService. I move shell state alteration out of the SetupInternalAsync so it is easier to understand the tenant status changes in one place. Also, I moved reporting success or error to the SetupAsync this way the code is much cleaner and we remove redundant code.

Let me know if you see any issues with the presented PR.

[jtkech]

Okay,

Looks like you did too much changes and are doing things in some places that may not be correct.

I will review and comment it asap.

[MikeAlhayek]

@jtkech I reverted some changes here and tested it. Everything seems to be still working as expected. I'll await your reply to my comments before making the other changes. I also changed where we fire the new events based on your recommendation.

[jtkech]

Okay, can't right know but will review asap

[MikeAlhayek]

@jtkech I think I addressed all you feedback. Let me know if I missed anything.

[jtkech]

Okay, looks good, will review tomorrow.

[sebastienros]

@MikeAlhayek will test default interfance implementations

[MikeAlhayek]

@jtkech Made all the requested changed.... Last changes? hopefully...

[jtkech]

Yes, sorry for being too strict sometimes, maybe because it's too hot in France ;)

[jtkech]

In the meantime I will approve

But as a last idea the default implementation of SetupAsync() could be to call Setup() so that we don't need to call the obsolete Setup() from the SetupService and only use the rule pragma here, all in one place.

Something like this, quickly written so may need to be tweaked.

Task SetupAsync(SetupContext context) =>
    Setup(context.Properties, (key, message) => context.Errors[key] = message );

[MikeAlhayek]

Yes, sorry for being too strict sometimes, maybe because it's too hot in France ;)

Weather in Romans is 29c, weather in Las Vegas 37c :) no problem.

[jtkech]

I think you missed my last comment

Did you tried my last suggestion here #14184 (comment)

Yes, now 29c because at midnight ;)