PerplexDigital / Perplex.ContentBlocks

Block based content editor for Umbraco
MIT License
31 stars 15 forks source link

Umbraco 8.7+ validation issues #44

Closed AndersBrohus closed 3 years ago

AndersBrohus commented 3 years ago

Hi I'm not quite sure where to create this issue but i guess it's here :)

After the latest update to Umbraco, we are getting the "Property has error" message, if a proptery is required but not filled. :)

image

Number 1 Is the error we are getting but number 2 is a field that is set to "Required" :)

PerplexDaniel commented 3 years ago

Yeah I also noticed some issues with validation starting in v8.7. They have revamped it quite a bit for their BlockList so the server-side code now returns something else when we call NestedContent's validation method in v8.7 compared to earlier versions, and client-side the validation messages are also gone unfortunately.

I was investigating this earlier but have not finished that. Will resume it soon and hopefully I can release a preview version on NuGet so you guys can verify whether it works or not :)

PerplexDaniel commented 3 years ago

I also believe there were some issues with the new validation when the site is set to multilingual, then you would only get a single error message like the one in your screenshot rather than at the correct property. This occurs in default Nested Content in v8.7 if I recall correctly. Not sure if that has been fixed already in some patch release. Which version of Umbraco are you guys running?

AndersBrohus commented 3 years ago

Thanks for continuously investigating this issue :)

We are running 8.9 on Cloud, on a non multilingual site :)

AndersBrohus commented 3 years ago

Maybe this could also be why we are experiencing the problem? https://github.com/umbraco/Umbraco-CMS/issues/8963

PerplexDaniel commented 3 years ago

Yeah the missing * is caused by Nested Content, we do not actually render the property label + editor that is all NestedContent. I would imagine 8.9 should have fixed that, as Sebastiaan points out a fix has been merged to 8.7.1 in the issue you link.

I'll have a quick look at this today, hopefully we can get the validation messages appear at the correct property or at least highlight the Content Block that contains the validation error like earlier.

I believe I already had a way to show the correct validation messages but they would all simply appear where your screenshot has "Property has error". That's at least a bit of an improvement over this. I'll update this at the end of the day.

PerplexDaniel commented 3 years ago

Well due to the fairly significant validation changes in Umbraco 8.7 it is probably easier to re-target ContentBlocks to 8.7+ only so I can handle it better on the server. Now I'm missing a reference to a ComplexEditorValidationResult class that was introduced in 8.7 and not available in earlier versions.

Anyway, I did some testing to make it work while still targeting 8.1. There is a way to get the validation messages to appear, although at the moment still grouped at the beginning of the content block. Nicest way would be if it just works again with Nested Content but that will take some more testing.

Attached is a NuGet I build of this small experiment, you could try that if you have some time. It should at least improve on the current situation. Looks something like this. errorMsg

You can test it with the attached NuGet files: Perplex.ContentBlocks.1.6.3-validation87-experiment.1.zip

I'll have to look at this some more at a later time because in 8.7 the best solution would of course display the errors properly at the exact properties where they occur.

AndersBrohus commented 3 years ago

We are going live with this project in this week, so i would prefer not to try the experiment on this project :)

But it looks good, i'll see if i can get some time to play with the experiment on a side project :)

PerplexDaniel commented 3 years ago

So I did some more experimenting and I believe I have made the new complex validation work now for 8.7+. This is still a bit of an early build and subject to change. But if you have time, please try it out locally with your installation. The error message should show up at the property itself.

You can get it from NuGet, version 1.7.0-alpha.1, make sure to tick "Include prerelease" to find it.

It will still (for now) say "This property is invalid" at the top outside of ContentBlocks, that is simply indicating that the whole property ContentBlocks has a validation error somewhere. Likewise for that message at the top of 1 block. Those can probably be removed either through CSS or JS, I'll have a look at that later.

It will now look something like this: complex

PerplexDaniel commented 3 years ago

@AndersBrohus

We are going live with this project in this week, so i would prefer not to try the experiment on this project :)

I understand you are not going to put any wild "experiments" into that project then ;-) Although this new version I just uploaded is less of an experiment and should work quite well already. Regardless, there is no rush. Just wanted to put this out there so if other people see some validation issues they can also try this alpha release to see if it improves things.

I do notice some issues in NestedContent as well though, messages not showing up in multilingual setups I witnessed myself now (only a "This property is invalid") and indeed no * at required properties. Also, I wasn't able to save/publish again with an invalid content picker property, even after setting a new value... This was all in a Nested Content only property, so not inside ContentBlocks. I will probably file some bug reports at Umbraco's GitHub for those :)

AndersBrohus commented 3 years ago

I just had a call with the client and they would like us to test it because it's an error which is impacting them quite alot :)

Is it possible for you to make an zip file i can install in Umbraco? Because i can't install it as NuGet inside the .Web project, since it would like to install alot of dependencies again which is already installed :)

PerplexDaniel commented 3 years ago

Sure, attached is the Umbraco package 👍 Perplex.ContentBlocks_1.7.0-alpha.1.zip

Please be aware of this issue in NestedContent that prevents Save/Publish when the Content Picker is invalid :( Will probably also affect your content picker inside a Content Block.

AndersBrohus commented 3 years ago

Thanks alot @PerplexDaniel :)

Yes, we have seen that :( Hopefully someone in the community is able to fix it soon, if not HQ does it them self :)

AndersBrohus commented 3 years ago

It looks like it's working very well @PerplexDaniel :) Then we just have to wait on the issue in NestedContent :)

stefankip commented 3 years ago

We're running into this as well and am going to switch to the pre-release to see if that's better for us. Going live in a few days, what could go wrong? 🤪

PerplexDaniel commented 3 years ago

I have released 1.7.0 final today which includes those fixes and some additional improvements. It is unfortunate there are some bugs in Umbraco with respect to complex validation + nested content + multilingual so not everything works perfectly. Regardless this is a big improvement over 1.6. Closing this issue now.

If you run into any new validation issues feel free to re open or make a new issue.

AndersBrohus commented 3 years ago

Thanks alot @PerplexDaniel ! :D