Add "Display Header" property to ListPartSettings

[MikeAlhayek]

Fix #12520

Summary of the changes

1. The "Edit [DisplayText]" is only visible if the user has permission to edit it. Contents support read-only support and the list should too.
2. The button were moved from the action-bar into their own bar (navigation-bar) and it's now visible on all parts of the container. "like if you add/edited contained item."
3. Added a new option to the ListPartSettings that would allow the user to show the container header across all pages like the navigation-bar does.

Below are some screenshots of the changes this PR introduces. To demonstrate this I created a content type called "Business" that has ListPart. The ListPartSettings contains "Business Location" and "Business Contact" as content types.

A new navigation buttons that appear on every page where PageListPart or ContainedPart are used for easier navigation

List Items view

Adding a new contained item "Business Location"

Editing a contained item "Business #11420

Editing the container it self "Edit Business"

New "ListPart Settings" to show the container's header

Here are screenshots with the Profile Header enabled

List Items view

Adding a new contained item "Business Location"

Editing a contained item "Business Location"

Editing the container it self "Edit Business"

The header is simple by default but you can override it using shape views.

Here is how one can override the view. In your theme, created a view Content-Business.Header.cshtml. My Custom view contains this code

@{
    var logos = Model.ContentItem.Content.Business?.Logo?.Paths?.ToObject<string[]>();
}
<div class="row">
    <div class="col-auto">
        @if (logos != null && logos.Length > 0)
        {
            <img src="~/media/@logos[0]" class="img-thumbnail" style="max-height: 15rem; max-width: 15rem;" />
        }
    </div>
    <div class="col">
        <h1 class="fs-5">@Model.ContentItem</h1>
        <p>@(Model.ContentItem.Content.ContentProfile?.About?.Text)</p>
    </div>
</div>

And the out come look like this

Before the PR

This is how "List Items" view looked with no buttons on the top

This is how "Edit Business" view looked with no buttons on the top

[MikeAlhayek]

@sebastienros @Skrypt I change this so it's a ListPart settings instead of using DisplayMode.

[MikeAlhayek]

@sebastienros I updated the header from menu to buttons as per your request

[jtkech]

Yes, we already can render any shape in any zone, and that's a good customization, but no sure we need it oob as we may often need different ones.

• Not sure about the navigation while editing an item, for example when we already have many fields and header Tabs, a customer may only want a smaller link to return back to the List.

• In the List I prefer to keep the navigation at the right but inline with the search box as in other pages.

• Maybe a little breaking for existing overrides as the nav buttons are moved to another shape.

• Note: In Lists what we are missing is filters and bulk actions.

As said in the review would be good to complete the ContainedPartIndex, here the main missing data are the Container ContentType and DisplayText, so in a given customization would be useful to retrieve them directly from the index table (could be also used to populate a cache) in place of loading the full item.

[MikeAlhayek]

@jtkech action bar is only available for hen you "list items". It's not available when editing the container or creating/editing a contained item. IMO, the great thing about adding the buttons on a new bar that is available while navigating around the container is that the user experience the same layout as he/she navigate around in the container. It's much better user experience and less confusing.

Additionally, it's a common use case that one user want to show a header for their container. So when the user navigates around the container, they always see a quick overview about the container itself. In the example I provided in the PR, the container is a "Business". A business contains "Locations" and "Contact" and could have many other types contained with in it. As you navigate around, it's extremely useful to get a small overview on the top of the business as new items being added/editing under it.

[jtkech]

action bar is only available for hen you "list items". It's not available when editing the container or creating/editing a contained item.

I'm reffering to the new ListPartNavigation shape (driven by both List and Contained drivers) that we see in all the above screenshots, and that renders the action buttons that were in ListPartDetailAdmin.

[MikeAlhayek]

Yes I understand. I am also saying that the navigation is an important change to improve user experience. With that change, the user will see that navigation bar across all any item that is related to the current container. So it's easy to navigate to any parts of the container.

[jtkech]

Okay, just saw the meeting where you discussed on this, in my above comment #12525 (comment) I was not talking about the Header I'm not against, that's okay as it is optional. Hmm maybe we could have find a way to not have to query twice the container (maybe currently for Latest and Published).

For the other points in #12525 (comment) I still think quite the same, particularly when we want to save space, we may prefer to provide a smaller link to go back to the list, keep the buttons inline the search box.

Also because for now it is a little breaking as the nav buttons are moved from an existing shape ListPartDetailAdmin to a new shape ListPartNavigation. Maybe an option to still use by default the same shape to render these buttons in the List, OR optionally add the new nav buttons everywhere.

But I don't want to block anything, in a future upgrade I may let the header hidden, easy to override the new ListPartNavigation to render nothing, and ListPartDetailAdmin to include again the nav buttons.

Note: Also as said in the review would be good to complete the ContainedPartIndex.

[MikeAlhayek]

@jtkech

Hmm maybe we could have find a way to not have to query twice the container (maybe currently for Latest and Published).
I am not sure we can avoid the second query when "unless we add the owner of the container to ContainedListPart". Note that today we don't do any kid of permission check to see if the the current use can edit the container or not. We just assume that if you can view the container and it's items you also can edit the container which is wrong in my opinion. [Check out the current code] (

OrchardCore/src/OrchardCore.Modules/OrchardCore.Lists/Views/ListPartDetailAdmin.cshtml

Lines 74 to 76 in a01d914

	<a class="btn btn-primary" edit-for="@Model.ListPart.ContentItem" asp-route-returnUrl="@FullRequestPath">
	@T["Edit {0}", @Model.ListPart.ContentItem.DisplayText]
	</a>

).

If we store the owner name in the contained part, then we can create a dummy content item with the same owner and check to see if the user can edit the container. At the same time, even if we decided to store the owner in the containedPart, that would be a breaking change since old data will not have the owner name. The owner is needed to determine if the user can edit their own containers. However, we could check for owner name first, and if there is no owner name we can execute the second query to get info about the container. Updated Another note here is that we only run the second query when we are editing or creating a new contained item not when we display the ListPart or editing ListPart since ListPart has the container content item.

Also because for now it is a little breaking as the nav buttons are moved from an existing shape ListPartDetailAdmin to a new shape ListPartNavigation. Maybe an option to still use by default the same shape to render these buttons in the List, OR optionally add the new nav buttons everywhere.

This would only impact users that are overriding the ListPartDetailAdmin shape "very rare case I would say."Even if they do, they just see the buttons listed twice. I would not call that a breaking change.

For the other points in #12525 (comment) I still think quite the same, particularly when we want to save space, we may prefer to provide a smaller link to go back to the list, keep the buttons inline the search box.

I am not against the idea of adding "Back" button when returnUrl is provided. But this isn't as good as having all the buttons visible on every contained page. For example, if you are editing a contained item and decided you want to create a different container item instead, then you can do that with a single click. Whereas previously, you would have to go back and then create a new one "2 clicks". I just believe what I did is far more user friendly than having the user to go back and fourth.

IMP, for a ListPart to be user friendly, it should give the user virtual look as if you are on the same page while navigating around anything to do with Container and it's contained items. Otherwise, ListPart is not more than OC.Contents with just a restriction on what content item to create.

[jtkech]

@MikeAlhayek Okay, I will fork your branch, better to have the code locally to try it, and then review it when I will have time but as soon as possible.

[jtkech]

@MikeAlhayek

Okay the code looks good to me, we can also plan to update the ContainedPartIndex in a future PR if that bothers you, otherwise I will talk too much ;)

What I suggest is to have another settings to render or not the navigation buttons, as we have to render the header or not. Then if this new setting is false (by default) ListPartNavigation would not be driven, and then ListPartDetailAdmin would also check this option to render or not these buttons as before.

Also the new ListPartHeader shape could be also driven by ListPartDisplayDriver, as you also drive it in ContainedPartDisplayDriver, so that we don't need to enlist a new ListPartHeaderDisplayDriver.

Not sure about the Header display type, as I remember we already have a display mode named Header, but I don't remember all details around this, maybe HeaderAdmin as we already have SummaryAdmin.

[MikeAlhayek]

@jtkech no bother at all. I'll update the index tomorrow. I personally don't see the value of adding the DisplayText/contentVersion in the index since it's already indexed in the ContentItemIndex but I am sure that I am missing something. Would you want a bool column for latest and another for published? Or, do you want a single column with the version?

I'll also look so see if I can drop the added display driver as you suggested and change the Header to HeaderAdmin.

Thanks for digging into this one! I'll ping you again once I update it

Updated
I actually do not like the idea of adding another settings for showing the new buttons or not. I think ListPart without these buttons is really missing. Unless you really want me too, I would rather not add that settings.

[MikeAlhayek]

@jtkech all requested changed were made and re-tested.
I have also renamed ListPartNavigation to ListPartNavigationAdmin and ListPartHeader to ListPartHeaderAdmin. Lastly, used HeaderAdmin as the display mode for the header instead of Header

[jtkech]

@MikeAlhayek Okay cool will review it tomorrow.

I actually do not like the idea of adding another settings for showing the new buttons or not.

Okay, I will think about it.

Yes it was to save space by default, for me in the List the search box is now too large, and while editing an item usualy I just add a smaller link (with the name of the container) to return back to the List, like the cancel button does but targetting the Title zone at the top.

That's funny, we already have an admin setting to save space by putting the Title in the NavbarTop.

Anyway only a minor problem, here maybe better to just do what others prefer.

[MikeAlhayek]

@jtkech I made the agreed on changes. Let me know if you see something else that needs to be done.

[jtkech]

I didn't agree, just suggested to remove the changes related to ContainedPartIndex as we are not in sync on this part and it may need more work (see below), and not required for this PR.

That means if you have index that has columns A, B, C yet only column B and C appear in the query, then that index won't even be used.

Yes there are a lot of articles about column order, index sizes, splitting in lighter ones based on planned queries, like in the life nobody really know but there are many gurus, I even saw indexes not used at the beginning but then used later on by the sql optimizer. The compromise we found, tried with query plans and profilers, and then used as a convention is to use as possible only one index including all columns which covers most of the cases including queries we don't know by advance.

I would rather keep it how I have it until we have a use case for such a large index or proofed that all these columns are needed.

Because of the above and it's not a large index, the light Published and Latest may be often used in the front end, also for an Admin filter to save a join on ContentItemIndex. Idem for DisplayText but which is longer so we can hesitate. If we do a direct .QueryIndex() (not a joined query), not currently used for this index table but may be useful, YesSql select all columns so we even need the Id column.

Anyway having columns in the Index Table that are not in the IDX_ doesn't follow the convention.

---

There is another issue with ContainedPartIndex, it indexes all versions including soft deleted items, so if you publish many times an item, in the List if you filter by Owned by me you see all of them, and marked as Draft which is wrong. On my side I make all child types non Versionable, but If you delete items the inactive versions are still there and if you try to edit one of them you get a 404.

So in the Owned by me filter we may need to add (Published || Latest), but sufficient I think that the index provider returns null if neither Published nor Latest, as we do most of the time in new providers.

[MikeAlhayek]

@jtkech I would follow the existing convention and just add all column to the index. Even though, I do not agree that is a good idea.

When the index is too small, doing a table scan may be cheaper for SQL server than analyzing and using the index. This may be the reason why no index is used in some cases. I think in OC, we should only index the queries that we are using. And yes, not all columns have to appear in an index for the query to perform best. I work with large data and saw many scenarios like this. If someone is using OC or any other platform, and the data become large, they would run the profiler on the server side to find bad queries that are often repeated and manually index when needed. I think by default only ids should be indexed since these column we use is join or in where statements. Keep in mind that most things does need index in SQL to perform. Index are good when the data is large “we should not assume data will always be large”. Also, remember tat indexes are not cheap as rebuilding indexes comes at a cost.

I'll also add the published or latest in the index provider in the morning.

Let me know if you want more changes before then.

[jtkech]

Okay cool, yes we read many contradictory articles like this which led us to this compromise.

[MikeAlhayek]

@jtkech all requested changes were made. let me know if you have any additional changes.

[jtkech]

Okay cool, will review it asap, this night or tomorrow

[jtkech]

Before reviewing tomorrow, for the .Owner filter in ContainerService

OrchardCore/src/OrchardCore.Modules/OrchardCore.Lists/Services/ContainerService.cs

Lines 371 to 377 in 3dacb34

	case ContentsStatus.Owner:
	var currentUserName = _httpContextAccessor.HttpContext?.User?.FindFirstValue(ClaimTypes.NameIdentifier);
	if (currentUserName != null)
	{
	query.With<ContentItemIndex>(i => i.Owner == currentUserName);
	}

I suggest to use the following for existing installations to filter the soft deleted items that are already there, even if now we don't index them anymore.

query.With<ContentItemIndex>(i => i.Owner == currentUserName && (i.Published || i.Latest));

[MikeAlhayek]

query.With<ContentItemIndex>(i => i.Owner == currentUserName && (i.Published || i.Latest));

Updated

[jtkech]

Okay LGTM, I will approve.

I only suggest to apply everywhere the same following ordering, in the index provider and migrations, even in the Create() method that will only be run on fresh installations.

// When used in the IDX_
Id
DocumentId

// These 2 contentItem Ids can be swapped as you want.
ContentItemId
ListContentItemId
DisplayText

// These 2 data can be swapped as you want.
Order
ListContentType

// These 2 contentItem states can be swapped as you want.
Published
Latest

[MikeAlhayek]

@jtkech I reordered the index to match the properties order. Please don't forget to approve this PR.

[jtkech]

Sorry, maybe I was not clear, here a suggested ordering

Id
DocumentId
ContentItemId         // <= Renamed from ContainedContentItemId.
ListContentItemId
DisplayText
Order
ListContentType
Published
Latest

You can also rename ContainedContentItemId to ContentItemId.

Not so crucial so anyway I will approve tomorrow

But in the meantime if you can apply this ordering in the index provider and migrations.

[MikeAlhayek]

@jtkech updated.

[hishamco]

Looks good