Tenant Workflows: Disable and Enable Tasks

[agriffard]

Implement Disable and Enable Workflow activities.

React to @jtkech review about #2450

[jtkech]

@agriffard

Thanks for taking into account my comments, yes somewhere it is related to #4164 and i saw it when i merged dev to update my PR. Then i was blocked by some questions about some points i'm not sure of, but now that's ok because through this PR we will be able to work on it separately.

I will merge #4164 where i only fixed the building (a method that is now async), you will have some conflicts but you will be able to rebase this PR on dev by just ignoring my changes on the related files.

I will do a 1st review soon and as said i will ask you some questions about some points i'm not sure of.

• The 1st main question is do we need to take into account preconfigured values? If so we would have to use _shellSettingsManager.CreateDefaultSettings() that takes into account the current configuration (recently i said that new ShellSettings() do the same but this is not true). Then we would have to check if a provided value is not null before overriding a shell settings value.

  Note: The ApiController uses CreateDefaultSettings() but doesn't check the provided values. So maybe intended that in this case all necessary values need to be provided and will override any preconfigured values. And maybe idem for WF tenant tasks.

  But anyway i think it is better to use CreateDefaultSettings().

To be continued

Update i merged #4164 so that you can rebase, sorry for the conflicts, just ignore my changes that was only to fix the building.

[jtkech]

You can remove this line, this is done by UpdateShellSettingsAsync() internally.

Note: Just a 1st review before merging #4164 on dev and rebase your PR

[jtkech]

idem, you can remove this line

Then, if this task can be run separately, not just after a create tenant task, maybe after having done ShellHost.TryGetSettings(), maybe good to do ShellSettingsManager.LoadSettings(settings.Name), i will let you know.

Here i asked me that, if for a given tenant name, no settings are found (or if no tenant name provided), why not just returning Failed. Will see when i will ask other questions, only simple questions not so much ;)

[jtkech]

Sorry i didn't have any time to work on it, i will do it tomorrow.

I thought about some changes, e.g better to not use Task.WhenAll() to evaluate expressions that may have liquid syntax because the evaluation would use scoped liquid handlers concurently.

Let me know if i can commit some changes in your branch directly or in a separate PR targetting you PR.

[jtkech]

I updated your PR. First part just for infos

• I took a look on the SetupController, the tenants AdminController and ApiController. Things are done differently because of the context but i think that they would need to be harmonized.

• E.g when creating a tenant through the admin we display a form including pre-config values and let the user change them or not, then on post we can use the model provided values. But in the ApiController i would have check the provided values before overriding pre-config ones.

• In the SetupController we do the opposite, we 1st check some pre-config values (or set when the tenant has been created) and if null use the model provided values, but here we know that if a value is pre-configured we didn't show the related fields, so the model can't provide them.

• But in the ApiController setup action, maybe better to 1st check the model provided values and if not null use them to override the pre-configured ones.

• In the SetupController the secret token is checked, but with the ApiController, on creating a tenant we provide a secret token, but on setup we don't check it.

• In the 'ApiController' we use a regular expression to check the tenant name.

And so on, so maybe we could harmonize this through another PR, i don't want to block this one.

---

So here i forgot the above

• I didn't do anything about authorizations and secret tokens.

• I didn't change anything on the UI and didn't add any UI validations.

• I just added some checks in the task activities.

• Then, for a given provided value, if not null i use it otherwise i don't override an eventual pre-configured one (or set e.g through a previous create tenant task or through the admin).

• I didn't try it ;)

[sebastienros]

Is there any permission check? And could these tasks be in a custom feature, such that not any user could use them by default?

[jtkech]

As a reminder

As i remember i updated this PR to make it consistent with what is done in the Tenants Admin / Api controllers but didn't have the time to try it. Maybe someone just need to solve the conflicts and try it, at least it would be better than what is currently in the dev branch because of the following.

• As i remember, currently in the dev branch, when creating / setup a tenant, we evaluate expressions (that involve liquid stuff) in parallel (by using Task.WhenAll) which is not good.

• When creating a tenant, at the beginning we don't create a default shell settings based on the configuration, as it is done in the Admin / Api controllers.

• When creating / setup a tenant we use SaveSettingsAsync() + GetOrCreateShellContextAsync() in place of using UpdateShellSettingsAsync() that reload configs / settings, as it is done in the Admin / Api controllers. Note: With my shells branch it will also let the tenant being lazily built.

[agriffard]

Conflicts fixed.

[jtkech]

Cool

[jtkech]

Fixes #5711

I will aprove it
But do we need to wait for someone to try it?
did somebody already try it?

If needed i'll try to take the time to try it this w-end