U4-7119 - New Content Type editor requires server side validation wired up

Created by Shannon Deminick 21 Sep 2015, 09:10:04 Updated by Mads Rasmussen 23 Oct 2015, 07:43:36

Relates to: U4-7134

Relates to: U4-7136

Relates to: U4-7251

Is required for: U4-7224

Subtask of: U4-112

Your report will have a greater chance of being addressed if you can give us clear steps to reproduce the issue, please answer the following questions in as much detail as possible: What did you do?

What did you expect to happen?

What actually happened?

3 Attachments

Comments

Shannon Deminick 14 Oct 2015, 15:53:06

Validation is wired up but we will need to fix styles or implementation on how to display the validation statements and highlight fields. We also want to keep validation somewhat consistent with the content editor experience.

Here's what validation will look like when things fail. There potentially could also be than one message displayed per field (i.e. alias is required and must match a pattern, group names are required and cannot be duplicated, etc...)

Some examples attached.

Some functionality that is missing with the 'navigation' buttons at the top is being able to indicate that there are validation issues on one of those navigation items. I've had a look at the code and because there's no direct DOM link between the nav items and the panel for their contents, we cannot use the DOM to check for errors there like we did with the val-tab directive. We will need to come up with something for that but I don't have time at the moment. For the content type editors, this isn't a huge problem because validation information doesn't really need to be displayed on the other tabs.


Shannon Deminick 15 Oct 2015, 13:44:28

PR is here: https://github.com/umbraco/Umbraco-CMS/pull/814


Shannon Deminick 15 Oct 2015, 13:50:43

For review of this task, testing needs to be done:

  • Check validation for content, media and member types
  • This includes entering blank values for required values, entering invalid aliases (i.e. starts with a number), entering duplicate aliases or group names
  • Also need to confirm validation for the creating property type's dialog - again, need to enter nulls, or invalid aliases, etc...

I know the styling will need to be fixed up but we will create a different task for that, in the meantime, this PR will need to get merged in quickly before there are too many other changes and merging becomes difficult.

There's been lots of changes to the code:

  • Saving is done using a ContentTypeSave model, instead of passing the whole display model back up to the server - this provides a lot more flexibility (future proof) and also reduces the upload size
  • Lots of code re-use so there's far less duplicated code in the c# controllers and mappers
  • I've tried to make minimal changes to the angular bits and pieces
  • I've fixed some of our older validation directives since they didn't work with dynamically applied attributes


Shannon Deminick 15 Oct 2015, 13:51:37

@madsrasmussen - I know we're supposed to 'pick' the ones to review but I think you'll be the best one to review this so you can see the end results... so we can fix the styling next sprint.


Mads Rasmussen 20 Oct 2015, 09:15:13

Cool. I have testet the changes. Most parts of the validation works great, but I have found some bugs.

'''Erros:''' *I get an error from "/Umbraco/BackOffice/Zbu/ModelsBuilderApi/GetModelsOutOfDateStatus” - but I guess it has nothing to do with this branch.

*I get an ysod error when trying to save a valid document type with 1 tab with 1 property and after this I am not able to save the document type at all. I have no problem saving a document type with 1 tab with no properties or 1 tab with 2 properties. I do not get this error in media types or member types.

*When saving a new member type a “Membership” tabs with a bunch of properties get automatically added to the member type. I this normal?

*When entering two of the same alias’s on one document type I get a ysod and no validation error. It works on media types and member types. Is it possible to mark all the duplicated alias’ and not just the first one?

'''UI improvements:''' Show notification error when save fails. As far as I can see this only works in other sections than the design section? If it is possible we should show these in the design section as well.


Shannon Deminick 20 Oct 2015, 13:37:24

I get an error from "/Umbraco/BackOffice/Zbu/ModelsBuilderApi/GetModelsOutOfDateStatus” - but I guess it has nothing to do with this branch.

This is fixed in a diff branch, so we don't need to worry about that.

I get an ysod error when trying to save a valid document type with 1 tab with 1 property and after this I am not able to save the document type at all. I have no problem saving a document type with 1 tab with no properties or 1 tab with 2 properties. I do not get this error in media types or member types.

@madsrasmussen I cannot replicate this issue. I've tried creating a new document type with 1 group + 1 property and it works. I've tried creating a new document type, then save, then creating 1 group + 1 property and it works. Tried creating a doc type, then creating 1 tab, then save, then creating 1 property type, then save and it works. Maybe I'm missing something ? or maybe you didn't have updated client files/cache?

When saving a new member type a “Membership” tabs with a bunch of properties get automatically added to the member type. I this normal?

Yup totally normal.

When entering two of the same alias’s on one document type I get a ysod and no validation error. It works on media types and member types.

All fixed - perhaps this was actually the issue with the other ysod issue.

Is it possible to mark all the duplicated alias’ and not just the first one?

We 'can' but it's not really wired up to work that way. When a field is highlighted it is actually marked as invalid. This means that to re-validate it, it needs to become dirty so if the user changes one alias, they would also have to change the other one in order to resubmit the form. If we really want to fix this later we can explore this but to do that we'd have to create some sort of custom client side validator that would handle this scenario. I think for now, let's just leave it as the first one is highlighted (or we can do last) and if we want to change it we can create another task. Sound ok ? This same concept would be true for duplicate group names too.

UI improvements: Show notification error when save fails. As far as I can see this only works in other sections than the design section? If it is possible we should show these in the design section as well.

@madsrasmussen Do you mean that (for example), if we enter duplicate aliases on the 'Design' section, than navigate to the 'List View' section, then save, that we need to indicate that the 'Design' section has errors? If that is what you mean then I had made notes on this somewhere. This currently works in the content editor and the tabs are highlighted that have errors. The way that the button navigation works in the content type editor are different because they use booleans to toggle views and there's not really a nice way to inspect the DOM for the active view to see if there are validation error classes applied (this is how the tab highlighting worked). I know there are a few different ways we can achieve that but we'll need to save that for the U4-7224 Update UI to support validation changes in new Content Type Editor task. Sound good ? http://issues.umbraco.org/issue/U4-7224


Mads Rasmussen 20 Oct 2015, 18:40:36

@Shandem Arg.. I really should have given the errors numbers or letters.

'''1. model builder error''' Cool. No worries then.

'''2. ysod error with one group and one property.''' I can still reproduce this one. What I do: Add one tab - name it “Content". Add property - name et “Test". Add editor - choose “Date". Save. I get this error.

Failed to save data for content type id 1055

Mapping types: WhereSelectListIterator2 -> IEnumerable1 System.Linq.Enumerable+WhereSelectListIterator2[[System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089],[Umbraco.Core.Models.ITemplate, Umbraco.Core, Version=1.0.5771.35456, Culture=neutral, PublicKeyToken=null]] -> System.Collections.Generic.IEnumerable1[[Umbraco.Core.Models.ITemplate, Umbraco.Core, Version=1.0.5771.35456, Culture=neutral, PublicKeyToken=null]] Destination path: IContentType.AllowedTemplates.AllowedTemplates Source value: System.Linq.Enumerable+WhereSelectListIterator`2[System.String,Umbraco.Core.Models.ITemplate

'''3. Membership group''' Cool. I got a bit surprised but I guess there is not much we can do about it right now.

'''4. ysod on alias validation''' Looks good to me too.

'''5. UI improvements''' I am aware of the challenges we have for the section buttons and that they do not work as the tabs in the content sections. That is okay for now. What I tried to explains was. If you are on the design section and removes an alias on a property to create an error and save. You will get a required text next to the alias. That is good. If you do the same thing and go to one of the other sections - you will get a red notification in the bottom, telling you that an alias is missing. I wan’t the same bottom notification in the design section as well :)

Let’s sync tomorrow, so we can get this wrapped up.

I hope it all makes sense. Beautiful work so far.


Shannon Deminick 21 Oct 2015, 15:56:51

@madsrasmussen I have fixed up the validation bits and pieces if you can review. This includes changes to the umb-editor-sub-views so that each view is just hidden when the section is changed. Seems to work well. See rev: 73fb761c4d0995bcab487d7a35989839837dddf3

I still cannot reproduce #2 strangely even though I saw it on your computer. We can do a hangout tomorrow and i'll show you. might be something else strange going on.


Shannon Deminick 22 Oct 2015, 13:11:33

Ok, have pushed another fix to fix that issue... it definitely saves now ;)

So next up is for you to have a look at the code i changed for the ng-repeat for the views in that PR and if all is good you can accept the PR :)


Mads Rasmussen 23 Oct 2015, 07:43:19

@Shandem it all looks good!


Priority: Task - Pri 1

Type: Task

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 7.4.0

Sprint: Sprint 1

Story Points:

Cycle: