Upgrade to Orchard Core 1.8 (OSOE-751)

[Piedone]

...once OrchardCMS/OrchardCore#14940 is done.

• Remove obsolete property in TenantSetupApiModel during the next major release (OSOE-751) Orchard-Core-API-Client#34 is part of this.
• Utilize IAsyncConfigureOptions OrchardCMS/OrchardCore#14759 and Startup Async Configure OrchardCMS/OrchardCore#14801 where we currently have sync-over-async.
• Perhaps we should do Add .form-label to the labels for proper rendering OrchardCMS/OrchardCore#14806 in our forms too?
• Add Ability to configure default browser (OSOE-733) UI-Testing-Toolbox#324 (comment) to the release notes of UI testing Toolbox.
• Add a note about the changed and app-wide setup for AI to its release notes.

Jira issue

[Piedone]

@domonkosgabor do you perhaps have something to add from the weekly news?

[domonkosgabor]

@domonkosgabor do you perhaps have something to add from the weekly news?

The documentation of the upcoming release summarizes the breaking changes and other changes so well: https://docs.orchardcore.net/en/latest/docs/releases/1.8.0/

Maybe I can add two minor things:

1. There are new extensions for IDisplayManager that we can utilize: https://orcharddojo.net/blog/use-shape-when-rendering-http-errors-to-allow-customization-from-the-ui-git-hg-mirror-is-running-on-orchard-core-this-week-in-orchard-01-12-2023
2. This post mentions the centrally defined media resources. But in the meantime, we have already added other resources to the manifest, so maybe we should check it out and use the named resources: https://orcharddojo.net/blog/introduce-a-new-navbar-shape-stimata-hotel-is-using-orchard-core-this-week-in-orchard-10-11-2023

[Piedone]

Thanks!

[Piedone]

Note this discussion about IAsyncConfigureOptions @Psichorex: OrchardCMS/OrchardCore#15125

[Psichorex]

Update: Onto the next problem
We have custom type such as PersonPage ExpressionContent ContentSetType etc.
When we navigate to their editor the following devtools exception occures:

And the preview button is broken.

The built in types and types created from Admin are working properly.
This $ is not defined is usually due to jQuery not being referenced before a script that uses it is called.
However from devtools all I see is jQuery is indeed included in both the problematic types and the built in types.
Also this has nothing to do with our newly added Decorator as I disabled the feature and the problem still occured.
I am currently trying to find out the cause of this.

[sarahelsaig]

PersonPage only contains PersonPart, whose editor doesn't even use Javascript at all. Same with ExpressionContent, it only contains stock OC content fields.

[Psichorex]

PersonPage only contains PersonPart, whose editor doesn't even use Javascript at all. Same with ExpressionContent, it only contains stock OC content fields.

@sarahelsaig But their preview button uses contentpreview-edit.js or something called like this.
I found the error in the meantime. So when these types are considered for some reason this contentpreview-edit.js is being registered prior to jquery.js being registered. Makes no sense to me what happens in the background that produces this.

Even more funny that I had to implement a ResourceManagerDecorator for different reasons and I was able from there to make sure that jQuery is always included first. At least this is a solution to the problem because the exception went away. Apart from this I think this is a bug in the OC 1.8 release but I am really not sure what I can call a bug in OC. It might be a feature.

Well I mean if it's a bug we would have had to make a temp solution for it anyways.

[sarahelsaig]

The preview button and contentpreview.edit.js are part of the OrchardCore.ContentPreview module so that should not be limited to our content items but any content item that can display a preview (may be inconsistent).

The module's resource configuration doesn't declare jQuery as a dependency of the "contentpreview-edit" resource, so it's up to the whims of the resource manager if it gets imported in the right order or not. Nor is a depends-on="jQuery" added to the script tag helper where it's used. This is an OC bug, I suggest opening an issue there.

A quick and dirty workaround is to override the ContentPreview.Button.cshtml with a copy and insert the depends-on attribute. But if you can add jQuery as a dependency to the ResourceDefinition of "contentpreview-edit" in your decorator, that's much better. Either way, please include a comment with a link to the OC issue.

[Psichorex]

OrchardCMS/OrchardCore#15181

[Piedone]

You can temporarily add an override for ContentPreview.Button.cshtml that explicitly requires jQuery to a suitable module until that's fixed.

[Psichorex]

I manually ordered the resources from the decorator so jquery always ends up first. It's more "pleasant" than an override.

[Psichorex]

Why did have thetheme-bootstrap-oc in ChartJs samples?

So the following error popped up after fixing the previous:
In the OC 1.8.2 branch The UITest TestChartJsSampleBehaviorAsync is broken because ResourceManager thinks that thetheme-bootstrap-oc is a required resource on that page (shown on the above picture) and we don't have that anymore and this is not due to the decorator. FATAL log error comes in as a required resource is not found.

I am asking this to collect data and not solution because I did not work with ChartJS before so I don't know if the required thetheme-bootstrap-oc is blatant in the first place or it is correct.

In OC 1.8.2 the following happens:

[sarahelsaig]

Why not search for that "thetheme-bootstrap-oc" string in the code base first? It's here:
https://github.com/Lombiq/Orchard-Chart.js/blob/f314e6262328274311a0eb5c9d94008e96cbb349/Lombiq.ChartJs.Samples/Views/_Layout.cshtml#L10

And here is the commit where it originated:
Lombiq/Orchard-Chart.js@a0e82a3#diff-8b380456dc41bb9509408f2f227dc615beac595681f2bda8835bc50e249e7b95R9

Looks like the sample just wants an empty page to display the charts in.

[Piedone]

Reordering resource from the decorator is a bad idea, don't do that. The dependency order is complex, and can be determined by a number of factors, you can't just say that jQuery is always at the top.

[Psichorex]

Why not search for that "thetheme-bootstrap-oc" string in the code base first? It's here: https://github.com/Lombiq/Orchard-Chart.js/blob/f314e6262328274311a0eb5c9d94008e96cbb349/Lombiq.ChartJs.Samples/Views/_Layout.cshtml#L10

And here is the commit where it originated: Lombiq/Orchard-Chart.js@a0e82a3#diff-8b380456dc41bb9509408f2f227dc615beac595681f2bda8835bc50e249e7b95R9

Looks like the sample just wants an empty page to display the charts in.

I did a search and nothing popped up. I use visual studio and used ctrl+shift+f and searched the file name. It found nothing.

Reordering resource from the decorator is a bad idea, don't do that. The dependency order is complex, and can be determined by a number of factors, you can't just say that jQuery is always at the top.

I did that because from where it worked Jquery was always in the first places. Also I did remove the error and seemingly nothing else popped up.

[Piedone]

You're testing a tiny fraction of the dependency combinations that are possible in OSOCE, let alone for every solution using our projects. You can't make sure it'll work in every case.

[Psichorex]

Seems like this did not solve the problem.
According to ShapeTracing the shape has been overriden but it still fails to find jQuery although it was set as a dependency in the cshtml.

[Psichorex]

You're testing a tiny fraction of the dependency combinations that are possible in OSOCE, let alone for every solution using our projects. You can't make sure it'll work in every case.

I created an OrderBy that is purely bringing jQuery to the first place and keeps every single other item in it's original place.
This is what actually happens on the Content Item editors that are working:
jQuer is first.

[Psichorex]

We have a test failing due to HtmlValidation as there is a role="button" in the html code.
I found that and it's in the TheTheme theme so I must remove the warning manually.

https://github.com/OrchardCMS/OrchardCore/blob/f843b91d479e222474a067573fcb5022814af6ed/src/OrchardCore.Themes/TheTheme/Views/ToggleTheme.cshtml#L2

[Piedone]

Again, no, the dependency order is not so simple. Features can break if you bring jQuery to the top. Don't do that. Include it from a shape override. It'll work for sure and we only need it until the next OC upgrade (do document this there).

[Psichorex]

Again, no, the dependency order is not so simple. Features can break if you bring jQuery to the top. Don't do that. Include it from a shape override. It'll work for sure and we only need it until the next OC upgrade (do document this there).

I did try to use a shape override and also included a comment about that here above #638 (comment)

It doesn't want to use the new ContentPreview_Button shape no matter what I do.
All I know is that I should copy the .cshtml to one of our modules that is enabled and reference the original OC module as a dependency. I did that in Lombiq.HE and also from Lombiq.BaseTheme and neither way worked.

From ShapeTracing it was saying that it is coming from HE but the Shape did not include the changes I made to the local shape to make sure it's ours.

[Psichorex]

Well for some reason it works from Lombiq.VueJs

[Piedone]

When is this causing an issue, to begin with? If in UI testing then you can exclude the URL from monkey testing or ignore the error because this is an OC bug.

[Psichorex]

It is causing errors in UITesting on Content Item Edit pages when the Preview button is present but not everytime.
It also made an error in a Test that was using TheBlogTheme and it occured there aswell so it is indeed an OC bug.
I think we should settle with this shape override now that it finally works because of the multiple places it causes errors.

[Piedone]

Putting it into our Vue module is random though.

Exclude that error from that UI test's assertion then, we have a lot of examples of that.

[Psichorex]

When using primary constructor stringLocalizer T will be problematic due to the analysers.

SO we either still declare a field named T or we remove the rule that warns us to start with lowerCase which is not a good idea imo or we supress.

What do you think?

[Piedone]

What's the analyzer violation?

[Psichorex]

That members of the primary constructor should start with a lower-case letter. https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1313.md
But I used fields to keep to the IStringLocalizer T convention.
So:

This way we don't need supresses and we can keep to the T naming convention.
Note this is only needed when we use the stringLocalizer which we usually name T.

[Piedone]

A suppression for this all the time is definitely not good, neither is disabling the parameter naming rule. Please bring this up in Teams for the whole team to be able to chime in, because we'll need a new convention, then; my vote would be on using _t (and _h for IHtmlLocalizer).