#495 Added RecipeExtraParams to SetupContext for simplifying Custom Setup. NOT MERGE WIP #624

jersiovic · 2017-03-19T15:03:42Z

#495 issue asks for a setup wizard.
This PR replaces current setup view per a setup shape. In this way an user can override default setup shape with his own customized Setup shape adding the setup shape on a custom module enabled by default at startup.cs of the root project with services.AddOrchardCms("ModuleOrThemeOverridingSetupShape");.

This PR also changes SetupController and SetupService to help to be reused with other shapes that provides extras parameters for a setup recipe. For doing that it adds to SetupContext a property called RecipeExtraParams of type IDictionary<string, StringValues> where our custom controller will put Request.Form keys and values.

sebastienros · 2017-03-20T23:43:45Z

src/Orchard.Cms.Web/Modules/Orchard.Setup/Services/SetupContext.cs

@@ -12,6 +12,7 @@ public class SetupContext
        public string DatabaseProvider { get; set; }
        public string DatabaseConnectionString { get; set; }
        public string DatabaseTablePrefix { get; set; }
+        public IDictionary<string, string> RecipeExtraParams { get; set; }


Why a dictionary, didn't the anonymous object work?

Well, in the case anyone want to provide its own custom controller you are right, the same could be done with an anonymous.
But this morning I was thinking we can change current setup controller to act in a generic way passing all the key/values available in form posted to RecipeExtraParams (except those named as the properties in SetupContext).
Doing that in many cases we won't need to have our own controller. So, to have a custom setup we would need only a new view. That connot be donde with an anonymous type as far as I know.
Next step would be we could add a custom shape to our theme that would replace custom setup form, then in many cases we won't need an extra module to have a custom setup.

Well, in the case anyone want to provide its own custom controller you are right, the same could be done with an anonymous.
But this morning I was thinking we can change current setup controller to act in a generic way passing all the key/values available in form posted to RecipeExtraParams (except those named as the properties in SetupContext).
Doing that in many cases we won't need to have our own controller. So, to have a custom setup we would need only a new view. That cannoy be done with an anonymous type as far as I know.
Next step would be we could add a custom shape to our theme that would replace custom setup form, then in many cases we won't need an extra module to have a custom setup.

Well, in the case anyone want to provide its own custom controller you are right, the same could be done with an anonymous.
But this morning I was thinking we can change current setup controller to act in a generic way passing all the key/values available in form posted to RecipeExtraParams (except those named as the properties in SetupContext).
Doing that in many cases we won't need to have our own controller. So, to have a custom setup we would need only a new view. That cannot be done with an anonymous type as far as I know.
Next step would be we could add a custom shape to our theme that would replace custom setup form, then in many cases we won't need an extra module to have a custom setup.

jersiovic · 2017-04-12T19:01:35Z

@sebastienros on last commit I've added a setup shape and passed to the recipe parameters all the key values in RequestForm, in that way anyone can override default setup shape for passing extra parameters to the recipe reusing existing controller.
For use cases where it is ok to perform validation of extra parameters only in client side it is an useful feature.

jersiovic · 2017-04-12T19:04:26Z

src/OrchardCore.Modules/Orchard.Setup/Views/Setup.cshtml

+}
+@{
+    var setupViewModel = Model.SetupViewModel as SetupViewModel;
+    AddPrefixToModelState(nameof(SetupViewModel));


How shapes model is dynamic I need to rename ModelState keys to be able of continuing using asp-for attributes for validating posted data

Instead of this hack do you agree if I change calls to AddModelError within Controller for adding nameof(SetupViewModel) as Prefix

You can have strongly typed Shapes in OrchardCore. Look at the drivers from the CMS module. If you can't find it I will grab an example for you.

jersiovic · 2017-04-12T19:08:15Z

src/OrchardCore.Modules/Orchard.Setup/Services/SetupContext.cs

@@ -12,7 +13,7 @@ public class SetupContext
        public string DatabaseProvider { get; set; }
        public string DatabaseConnectionString { get; set; }
        public string DatabaseTablePrefix { get; set; }
-        public IDictionary<string, string> RecipeExtraParams { get; set; }
+        public IDictionary<string, StringValues> RecipeExtraParams { get; set; }


Request.Form stores data on StringValues that is why I replaced string per StringValues for not losing input data.

jersiovic · 2017-04-12T19:12:46Z

A future improvement for this feature could be to add a shape alternate for the setup with the name of the recipe selected. When the user changes the recipe the reload would be forced to load any specific shape for the selected recipe.

jersiovic · 2017-04-12T23:15:15Z

Checks based on travis build fails on mac, but error doesn't seems to be related with nothing in this PR

sebastienros · 2017-04-12T23:29:37Z

The mac build on travis is always failing, independent from your PR

jersiovic · 2017-04-16T08:15:13Z

src/OrchardCore/OrchardCore.Cms/ServiceExtensions.cs


 namespace Microsoft.Extensions.DependencyInjection
 {
    public static class ServiceExtensions
    {
-        public static IServiceCollection AddOrchardCms(this IServiceCollection services)
+        public static IServiceCollection AddOrchardCms(this IServiceCollection services, params string[] extraDefaultFeatureIds)


This allows to set featureIds to be loaded on startup of the root project.
For example if you provide an alternate Setup shape in your SafeMode theme that replaces default shape you should initialize Orchard calling services.AddOrchardCms("SafeMode"); on Startup

jersiovic · 2017-04-17T06:14:25Z

Please can you give me feedback related with this PR? Do you prefer I remove the hack done in setup shape for validation? What would be best solution in your opinión? Do we need to agree on design of this feature before merging it? Do you want a demo?

sebastienros

I took some time to look at the changes and the goals here. It's a good start I think. I can't see any better way than providing a custom shape for now in terms of extensibility. And i assume it doesn't force the final application to have theming, it's just for setup.

Have you used this branch already and checked it worked as expected?

sebastienros · 2017-04-17T15:26:35Z

src/OrchardCore.Modules/Orchard.Setup/Views/Setup.cshtml

+}
+@{
+    var setupViewModel = Model.SetupViewModel as SetupViewModel;
+    AddPrefixToModelState(nameof(SetupViewModel));


You can have strongly typed Shapes in OrchardCore. Look at the drivers from the CMS module. If you can't find it I will grab an example for you.

sebastienros · 2017-04-17T15:31:20Z

src/OrchardCore.Modules/Orchard.Setup/_ViewImports.cshtml

@@ -0,0 +1,2 @@
+@inherits Orchard.DisplayManagement.Razor.RazorPage<TModel>


You should keep this file in the Views folder (preparing for the future).

sebastienros · 2017-04-17T15:32:03Z

src/OrchardCore/Orchard.Recipes.Implementations/ParametersMethodProvider.cs

@@ -18,7 +19,7 @@ public ParametersMethodProvider(object environment)
                Name = "parameters",
                Method = serviceprovider => (Func<string, object>) (name =>
                {
-                    return environmentObject[name].Value<string>();
+                    return environmentObject[name].Value<StringValues>();


Keep strings.

sebastienros · 2017-04-17T15:33:44Z

src/OrchardCore.Modules/Orchard.Setup/Services/SetupService.cs

-                        DatabaseConnectionString = context.DatabaseConnectionString,
-                        DatabaseTablePrefix = context.DatabaseTablePrefix
-                    }); 
+                    var recipeParams = context.RecipeExtraParams ?? new Dictionary<string,StringValues>();


Use strings only. Or even better, objects. But definitely not StringValues

sebastienros · 2017-04-17T15:42:19Z

src/OrchardCore.Modules/Orchard.Setup/Controllers/SetupController.cs

@@ -128,7 +129,8 @@ public class SetupController : Controller
                AdminEmail = model.Email,
                AdminPassword = model.Password,
                Errors = new Dictionary<string, string>(),
-                Recipe = selectedRecipe
+                Recipe = selectedRecipe,
+                RecipeExtraParams = Request.Form.ToDictionary(keyValue => keyValue.Key, keyValue => keyValue.Value)


I am not sure it's a good idea to just take everything coming from the Form. Looks like a potential security issue to me.

There might not be a better way though, I will have to think about it.

sebastienros · 2017-04-17T15:47:54Z

Right now the setup recipes have to be in the Setup module. Maybe we should make this extensible and have the current recipes come from a CMS module. Something using DI could then resolve the IFile that give access to a list of recipes. This module would be part of the Default features.

jersiovic · 2017-04-17T16:06:57Z

Thank you for the review.
Till now I was using my own setup recipe stored in my custom module and it was detected without problems. I will check that it doesn't work as it did before when I rebase our branch o top of master.

jersiovic · 2017-04-17T17:02:06Z

Confirmed, setup recipes in other modules are loaded automatically by Orchard.

jersiovic · 2017-04-17T17:15:52Z

@sebastienros said:

I took some time to look at the changes and the goals here. It's a good start I think. I can't see any better way than providing a custom shape for now in terms of extensibility. And i assume it doesn't force the final application to have theming, it's just for setup.

Yes, it is only for setup. You only need to override setup shape, not anything else.

Have you used this branch already and checked it worked as expected?

I tested it partially, I checked valid values were provided to the recipe.
But after hearing that you agree with this PR I'm going to test it in our web api project based on Orchard 2 to be sure everything fits properly. Till now in that project we had modified Orchard.Setup module view and controller to have a custom setup, so it will be a good test.

jersiovic · 2017-04-17T20:49:40Z

I'm able to override setup shape only if I put the custom setup shape in SafeMode theme and initializin OrchardCms with SafeMode module: services.AddOrchardCms("SafeMode");. But if I use a regular module it doesn't work. Is there a way of overriding a shape invoked in another module from a module that is not a theme?

jersiovic · 2017-04-18T20:41:29Z

src/OrchardCore/Orchard.Recipes.Implementations/Services/RecipeExecutor.cs

@@ -259,7 +259,7 @@ private void EvaluateJsonTree(IScriptingManager scriptingManager, JToken node)
                    // Evaluate the expression while the result is another expression
                    while (value.StartsWith("[") && value.EndsWith("]"))
                    {
-                        value = value.Trim('[', ']');
+                        value = value.Substring(1, value.Length - 2);


@sebastienros I replaced Trim because it caused a bug on expressions like this: "[js:parameters('extraSettings.Code')[0]]" because it removes last two brackets instead of only ones causing an exception later when the expression is parsed.

jersiovic · 2017-04-20T21:52:41Z

I close this PR cause I didn't take into account that for returning posted data in postback when controller finds an error I need do things not very clean for binding those data on the setup shape controls. So the solution will be a bit hacky. It would be interesting me or other tries a less easy but more elegant approach based on #716

jersiovic changed the title ~~#495 Added RecipeExtraParams to SetupContext for simplify Custom Setup~~ #495 Added RecipeExtraParams to SetupContext for simplifying Custom Setup Mar 19, 2017

sebastienros reviewed Mar 20, 2017

View reviewed changes

sebastienros force-pushed the master branch 2 times, most recently from 37f0cda to 081a307 Compare April 6, 2017 02:35

jersiovic added 2 commits April 12, 2017 20:55

Added RecipeExtraParams to SetupContext

19d39ff

Added setup shape for customizing it without changing controller

34c7691

jersiovic force-pushed the #495customSetup branch from b8429e0 to 34c7691 Compare April 12, 2017 18:56

jersiovic commented Apr 12, 2017

View reviewed changes

Little fixes

f5f383c

Added extraDefaultFeatureIds param to AddOrchardCms extension

5940a41

jersiovic force-pushed the #495customSetup branch from bdf62b1 to 5940a41 Compare April 16, 2017 08:09

jersiovic commented Apr 16, 2017

View reviewed changes

sebastienros reviewed Apr 17, 2017

View reviewed changes

jersiovic force-pushed the #495customSetup branch from ae6ff89 to f027e16 Compare April 18, 2017 16:25

StringValues to Object and other fixes

0451d58

jersiovic force-pushed the #495customSetup branch from f027e16 to 0451d58 Compare April 18, 2017 16:26

Changes after testing feature

8c0e16d

jersiovic commented Apr 18, 2017

View reviewed changes

Merge branch 'master' into #495customSetup

aee93b9

Fix in ParametersMethodProvider

770b25f

jersiovic changed the title ~~#495 Added RecipeExtraParams to SetupContext for simplifying Custom Setup~~ #495 Added RecipeExtraParams to SetupContext for simplifying Custom Setup. NOT MERGE WIP Apr 19, 2017

jersiovic closed this Apr 20, 2017

Jetski5822 deleted the #495customSetup branch June 20, 2017 07:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#495 Added RecipeExtraParams to SetupContext for simplifying Custom Setup. NOT MERGE WIP #624

#495 Added RecipeExtraParams to SetupContext for simplifying Custom Setup. NOT MERGE WIP #624

jersiovic commented Mar 19, 2017 •

edited

Loading

sebastienros Mar 20, 2017

jersiovic Mar 21, 2017

jersiovic Mar 21, 2017

jersiovic Mar 21, 2017

jersiovic commented Apr 12, 2017 •

edited

Loading

jersiovic Apr 12, 2017 •

edited

Loading

jersiovic Apr 12, 2017

sebastienros Apr 17, 2017

jersiovic Apr 12, 2017

jersiovic commented Apr 12, 2017

jersiovic commented Apr 12, 2017

sebastienros commented Apr 12, 2017

jersiovic Apr 16, 2017

jersiovic commented Apr 17, 2017

sebastienros left a comment

sebastienros Apr 17, 2017

sebastienros Apr 17, 2017

sebastienros Apr 17, 2017

sebastienros Apr 17, 2017

sebastienros Apr 17, 2017

sebastienros Apr 17, 2017

sebastienros commented Apr 17, 2017

jersiovic commented Apr 17, 2017

jersiovic commented Apr 17, 2017

jersiovic commented Apr 17, 2017 •

edited

Loading

jersiovic commented Apr 17, 2017

jersiovic Apr 18, 2017

jersiovic commented Apr 20, 2017

		@@ -0,0 +1,2 @@
		@inherits Orchard.DisplayManagement.Razor.RazorPage<TModel>

#495 Added RecipeExtraParams to SetupContext for simplifying Custom Setup. NOT MERGE WIP #624

#495 Added RecipeExtraParams to SetupContext for simplifying Custom Setup. NOT MERGE WIP #624

Conversation

jersiovic commented Mar 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jersiovic commented Apr 12, 2017 • edited Loading

jersiovic Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jersiovic commented Apr 12, 2017

jersiovic commented Apr 12, 2017

sebastienros commented Apr 12, 2017

Choose a reason for hiding this comment

jersiovic commented Apr 17, 2017

sebastienros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienros commented Apr 17, 2017

jersiovic commented Apr 17, 2017

jersiovic commented Apr 17, 2017

jersiovic commented Apr 17, 2017 • edited Loading

jersiovic commented Apr 17, 2017

Choose a reason for hiding this comment

jersiovic commented Apr 20, 2017

jersiovic commented Mar 19, 2017 •

edited

Loading

jersiovic commented Apr 12, 2017 •

edited

Loading

jersiovic Apr 12, 2017 •

edited

Loading

jersiovic commented Apr 17, 2017 •

edited

Loading