Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v8 - Fixes multiple issues with variant validation #4955

Merged
merged 33 commits into from
Apr 4, 2019

Conversation

Shazwazza
Copy link
Contributor

@Shazwazza Shazwazza commented Mar 12, 2019

Fixes #4884

This PR fixes/updates several things:

  • Previously on the server, we would only validate critical values for persistence if the user was creating new content/media - but this makes no sense! We can't actually persist anything if critical data (i.e. The Name is empty. If that were attempted an exception would be thrown. The UI should prevent that from happening but the c# should always validate that, so this is fixed
  • Previously if you tried to save + publish a new document that had validation errors for any mandatory language, the error response you would receive would not indicate that you didn't actually publish anything, now it does
  • ContentService.SaveAndPublish has been fixed, previously if the user had only selected 2 out of 3 cultures to publish and the other culture had invalid/empty properties, the ContentService.SaveAndPublish would still validate all 3 cultures! This meant that the user would get back some strange warnings and nothing would actually be persisted.
  • Previous angular updates had code in there to manually validate if variant names were empty, there was quite a bit of code to achieve this in the publish.html and directives/components/content/edit.controller.js files but it is unnecessary since the server deals with the name validation and returns validation responses accordingly. So this code has been reverted. It also means that by continuing to use the server validation for empty names that the user is still able to persist any variant they have selected but get a validation error about another variant that couldn't be persisted.
  • There was issues with server validation for Invariant properties on a variant content item. a) The validation messages were never wired up to the invariant property b) The validation messages for the invariant property were never associated with the default culture. This meant if you tried to publish the default culture but an invariant property had server side validation problems, the item would not be persisted but there would be no UI indication to the user of what went wrong.
  • ContentService.SaveAndPublish would not actually save if there was validation problems
  • When using the string "*" to denote a culture and the content item was invariant, it's name wasn't actually validated
  • When trying to publish a non-default culture, the invariant properties weren't validated even if there was no published version at all for the content. Invariant properties need to be validated for the default language, or if a non-default language is publishing and there is no published version
  • ... and more!

To see the changed files without whitespace changes, use this: https://github.com/umbraco/Umbraco-CMS/pull/4955/files?w=1

I have renamed a couple classes to clean things up a little, added some tests and notes.

Testing ... well there's a lot of combinations to test here so please use your imagination. Pro tip #1:
Create 2 mandatory langs, a variant doc type with 2 text properties that both validate as mandatory and as a Number and make one of those properties invariant.
This will give you a nice testing ground for server side validation with mandatory requirements, then test creating new content, updating content, with valid/invalid combinations. Pro tip #2: If you navigate to a language, enter a value for it's name and then clear that value it will be marked as dirty, then navigate to another language and go to save/publish, it will still allow you to try to publish the other language even though the name is null. Server side validation will take care of this, but its another combination to test.

Then be sure that invariant content validation still works as expected too.

@@ -365,7 +365,7 @@
action: action,
variants: _.map(displayModel.variants, function(v) {
return {
name: v.name,
name: v.name ? v.name : "", //if its null/empty,we must pass up an empty string else we get json converter errors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shazwazza I think this also can be written as name: v.name || ""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjarnef ah of course, i always forget about that operator in JS thanks!

@Shazwazza Shazwazza changed the title WIP - Fixes how critical data is validated v8 - Fixes multiple issues with variant validation Mar 14, 2019
@ghost ghost assigned robert-cpl Mar 19, 2019
robert-cpl and others added 8 commits March 20, 2019 10:20
…dated on the default lang or if the item is not published
# Conflicts:
#	src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml
…ures since it wouldn't actually save the content if it was invalid - we always must save even if data is invalid, this was an oversight with the rush to release with these last minute APIs
@zpqrtbnk zpqrtbnk changed the base branch from dev-v8 to v8/dev March 30, 2019 10:25
@ghost ghost assigned zpqrtbnk Apr 1, 2019
@Shazwazza Shazwazza merged commit 984c31f into v8/dev Apr 4, 2019
@Shazwazza Shazwazza deleted the temp8-server-content-validation-fixes-4884 branch April 4, 2019 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When publishing content with no Name, I want to see serverside validation, not an error
4 participants