OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.44k stars 2.4k forks source link

Add HtmlPrefix parameter to IDisplayManager<TModel> #11612

Closed MikeAlhayek closed 2 years ago

MikeAlhayek commented 2 years ago

Is your feature request related to a problem? Please describe.

I have a need to create a function that would create users in bulk from excel file. Now, that I have a collection of data (user data), I need a way to tell drivers the prefix that should be use when building the ShapeResult. For example, the UserDisplayDriver tries to bind the data from the request to EditUserViewModel. However, if the request has data that are prefixed with Users[0].User_ the binding engine would not know how to bind the data in the request to the viewmodel.

Describe the solution you'd like

Change the IDisplayManager to the following

    public interface IDisplayManager<TModel>
    {
        Task<IShape> BuildDisplayAsync(TModel model, IUpdateModel updater, string displayType = "", string groupId = "", string htmlPrefix = "");
        Task<IShape> BuildEditorAsync(TModel model, IUpdateModel updater, bool isNew, string groupId = "", string htmlPrefix = "");
        Task<IShape> UpdateEditorAsync(TModel model, IUpdateModel updater, bool isNew, string groupId = "", string htmlPrefix = "");
    }

Now with that change, during the post request on creating the user I can do something like this

for(int i = 0; i < model.Record.Length; i++)
{
    var user = new User();
    var prefix = $"{nameof(model.Record)}[{i}]"; // prefix would be "Record[0]"

    var shape = await _userDisplayManager.UpdateEditorAsync(user, updater: _updateModelAccessor.ModelUpdater, isNew: true, htmlPrefix = prefix);

    await _userService.CreateUserAsync(user, null, ModelState.AddModelError);
}

If this is an acceptable approach I can submit a PR

ns8482e commented 2 years ago

IMO - Prefix is what driver author decides to keep to differentiate different inputs - You may want to dynamically build prefix inside driver if your driver needs to have specific prefix.

You may need implement your driver similar to FlowPart driver

MikeAlhayek commented 2 years ago

I agreed that the driver should be defining the prefix. Adding prefix from the display manager would only concatenate to that prefix. so in the above example, the Prefix would be Record[0]. then the driver's own prefix. This would only be added if one explicitly added it.

ns8482e commented 2 years ago

As DisplayManager creates root shape and it should start with empty prefix.

You can always change prefix by defining your own driver

MikeAlhayek commented 2 years ago

As DisplayManager creates root shape and it should start with empty prefix.

You can always change prefix by defining your own driver

In the above example, why would I have to define a new display driver just because the Prefix value would be different? The logic in the driver relay won't change. IMO, adding the ability to specify prefix won't change the default behavior but would give the consumer the flexibility to control the behavior when needed.

ns8482e commented 2 years ago

IMO if prefix is needed by parent shape for it’s child shapes then it should not be child shape’s display manager’s responsibility

sebastienros commented 2 years ago

Same as @ns8482e, if FlowPart and BagPart can already do it, why not create a custom driver that defines the prefix for its inner content?

But if the change is simple and not risky (just an extensibility) then please provide a PR

MikeAlhayek commented 2 years ago

Same as @ns8482e, if FlowPart and BagPart can already do it, why not create a custom driver that defines the prefix for its inner content?

But if the change is simple and not risky (just an extensibility) then please provide a PR

I think the idea here is not having to create drivers with the same logic as existing once just because the prefix is not the same. If you think about the use case I listed above, the logic to create a user won't really change. The only thing that changes here is where the request is coming from. So I read the user data from excel file, then render a the data in a grid view. When the user submit's the data, the logic in the UpdateAsync of existing of the drivers won't change but the binding prefix will. I just think this would make it easier for one to reuse drivers only by controlling the prefix. It won't change anything that currently exists, it just extends existing behavior to allow for covering more use-case. I'll try to sent a PR request

ns8482e commented 2 years ago

@CrestApps it’s not question about just extending behavior It’s like encapsulation, you don’t want to expose the prefix outside of root shape created by display manager

if you need for some reason you can always achieve such behavior by creating and registering display driver responsible for it

MikeAlhayek commented 2 years ago

@ns8482e we have string htmlFieldPrefix = "" in IContentItemDisplayManager any reason why it's acceptable there but not acceptable on IDisplayManager<TModel>. These two manager do the same exact work except that IContentItemDisplayManager is concrete to ContentItem.

ns8482e commented 2 years ago

As I said earlier - it’s my opinion

even I don’t like to have html prefix as param in IContentItemDisplayManager as it adds inconsistency between display and edit