Generic Site settings Deployment Step

[agriffard]

Fixes #5325, #5558

/cc @sebastienros @deanmarcussen As explained during today's meeting.

[agriffard]

Just implemented it in OrchardCore.Admin with AdminSetting for the moment.

Please tell me if it is the correct way to do it:

    [RequireFeatures("OrchardCore.Deployment")]
    public class DeploymentStartup : StartupBase
    {
        public override void ConfigureServices(IServiceCollection services)
        {
            var serviceProvider = services.BuildServiceProvider();
            var S = serviceProvider.GetService<IStringLocalizer<Startup>>();
            services.AddSingleton<IDeploymentStepFactory>(s => new GenericSiteSettingsDeploymentStepFactory(nameof(AdminSettings), S["Admin settings"], S["Exports the admin site settings."]));
        }
    }

[agriffard]

Add Step:

Edit Step:

Deployment plan:

[deanmarcussen]

@agriffard good start :)

We're going to have to move it to generics, i.e. GenericDriver<AdminSiteSettings> or the drivers are going to be fired multiple times for each registration. Same with the stepfactory.

And it's going to have to go into a Core project to sort out referencing.

This is easier to explain over skype. I should be free later on if you want?

[deanmarcussen]

        public override void ConfigureServices(IServiceCollection services)
        {
            services.AddSingleton<IDeploymentStepFactory>(sp => {
           var S = sp.GetService<IStringLocalizer<Startup>>();
 
return new GenericSiteSettingsDeploymentStepFactory(nameof(AdminSettings), S["Admin settings"], S["Exports the admin site settings."])
});
        }

For infos. But also will have to be generics, and I think in this case we can get away with out a custom factory, just a generic class, but would have to check

[agriffard]

Added OrchardCore.Settings.Core with generic SiteSettingsDeploymentStep, source and driver.
Renamed the existing SiteSettingsDeploymentStep in OrchardCore.Settings into GeneralSiteSettingsDeploymentStep.

[deanmarcussen]

@agriffard should be updated and working now.

For infos, @sebastienros , this had to be made to use generics, not for the step factory, but for the DisplayDriver which needs to be a specific type, in order to only fire for that specific type.

It doesn't make me happy to use generics, but it is the right solution in this case, I think

Additionally we can't have a simple extension method to register these, because of the string localizer, which I believe needs to visibly show a variable var S = ... in the Startup class where we want to register these localized strings.

so

services.AddSiteSettingsPropertyDeployment<AdminSettings>(() => S["Admin settings"));

would (probably) not be found by the extractor
and

services.AddSiteSettingsPropertyDeployment<AdminSettings>((sp) => {
  var S = sp.GetRequiredService<IStringLocalizer<Startup>>();
  return S["Admin settings");
},
... (for the `Description`) 
... etc
);

is to wordy for an extension method to be useful, I figure

[deanmarcussen]

We need to test here we don't get double encoding. Should be ok, but just put an accent in one of the localized values, and check the page source after to make sure the encoding is correct

[deanmarcussen]

Tested is all good

[deanmarcussen]

this needs to go in a separate startup with RequiresFeatures just for the default login settings

[agriffard]

ok, please check.

[agriffard]

[deanmarcussen]

Merged dev, and added the Taxonomy Filters settings too

[agriffard]

Do we keep the GraphQL settings?

[deanmarcussen]

@agriffard I have evaluated and updated your list on #5558

Some of the ones you have done here, have their own recipe step, so cannot be included in a generic design.

Others have secrets, so also must be custom.

GraphQL is not a ISite setting, it's configured via IOptions so was just not named as per the convention originally.

[deanmarcussen]

@agriffard I think it's good - but I've helped write some of it, so I would think that ;)

Can we demo tomorrow or review Thursday? I would like to be sure we're doing the right thing with the generics, and the localization

[deanmarcussen]

Remove this reference

[deanmarcussen]

Remove this reference

[deanmarcussen]

same

[deanmarcussen]

same

[deanmarcussen]

same

[deanmarcussen]

same

[deanmarcussen]

same

[deanmarcussen]

Tested is all good

[deanmarcussen]

We should find the property if it exists already, so it is only one step that is being added.
I will do this, while I am checking the registration with both attributes (i.e. Feature and RequiresFeature works ok)

[deanmarcussen]

@agriffard have changed it to look for an existings Settings property rather than creating a new one every time.

output changes from

  "steps": [
    {
      "name": "Settings", <-- first Settings step
      "AdminSettings": {
        "DisplayMenuFilter": false
      }
    },
    {
      "name": "Settings", <-- second Settings step
      "LoginSettings": {
        "UseSiteTheme": false,
        "UseExternalProviderIfOnlyOneDefined": false,
        "DisableLocalLogin": false,
        "UseScriptToSyncRoles": false,
        "SyncRolesScript": null
      }
    }
  ]

  "steps": [
    {
      "name": "Settings", <-- Single Settings step
      "AdminSettings": {
        "DisplayMenuFilter": false
      },
      "LoginSettings": {
        "UseSiteTheme": false,
        "UseExternalProviderIfOnlyOneDefined": false,
        "DisableLocalLogin": false,
        "UseScriptToSyncRoles": false,
        "SyncRolesScript": null
      },
      "HttpsSettings": {
        "EnableStrictTransportSecurity": false,
        "RequireHttps": false,
        "RequireHttpsPermanent": false,
        "SslPort": null
      }
    }
  ]

An optimization, and makes it clearer that all the steps are contained in one

If you are happy with the change let me know, and I think we can merge it.

I've checked the Feature and RequiresFeature are working fine together. i.e. if you disable deployment but still have registration the startup doesn't fire.

[agriffard]

Great

[agriffard]

I prefixed the Factory key with the name instead of suffixing it :
private static readonly string GenericTypeKey = typeof(TModel).Name + "_SiteSettingsPropertyDeploymentStep";

Thanks to this, the steps will be sorted a bit more alphabetically in the modal.

[deanmarcussen]

I prefixed the Factory key with the name instead of suffixing it
Thanks to this, the steps will be sorted a bit more alphabetically in the modal.

Cool, can you do the same in the

        public SiteSettingsPropertyDeploymentStep()
        {
            Name = "SiteSettingsPropertyDeploymentStep_" + typeof(TModel).Name;
        }

so the match I have a vague memory this is important ;)