Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

[Display(Order={i})] ignored when sorting validation summary messages and properties in @Html.Display() / @Html.Editor() object templates #964

Closed
dougbu opened this issue Aug 10, 2014 · 4 comments

Comments

@dougbu
Copy link
Member

dougbu commented Aug 10, 2014

This is a regression compared with MVC 5.2 and it's similar to #843, #844, #933, and #963. In this case however it's less than a functional gap; all the expected information is available. The visible symptoms are unpredictable ordering of validation errors and the Properties collection.

The current implementation of CachedModelMetadata<TPrototypeCache> does not override the Order property as it should. Value should be based on [Display] attributes in the model.

@dougbu
Copy link
Member Author

dougbu commented Aug 12, 2014

In addition ModelMetadata.Order is not currently used to sort the ModelMetadata.Properties enumeration. This means this collection does not have a predictable order, even when a user sets Order themselves.

@dougbu dougbu changed the title Set ModelMetadata.Order based on attributes [Display(Order={i})] ignored when sorting validation summary messages and properties in @Html.Display() / @Html.Editor() object templates Aug 13, 2014
@dougbu
Copy link
Member Author

dougbu commented Aug 13, 2014

Main fix here is to set ModelMetadata.Order based on [Display] attributes in the model.

MVC 5.2 does not provide a predictable property order when displaying / editing a complex object or filling in the @Html.ValidationSummary(). We could do a bit better here and sort by Order then by PropertyName.

dougbu added a commit that referenced this issue Aug 14, 2014
- supports useful property updates of `ModelMetadata` instances in the collection
- provide a controllable ordering using the `Order` value
- part of #964
dougbu added a commit that referenced this issue Aug 15, 2014
- supports useful property updates of `ModelMetadata` instances in the collection
- provide a controllable ordering using the `Order` value
- part of #964
dougbu added a commit that referenced this issue Aug 15, 2014
- supports useful property updates of `ModelMetadata` instances in the collection
- provide a controllable ordering using the `Order` value
- part of #964
dougbu added a commit that referenced this issue Aug 15, 2014
- supports useful property updates of `ModelMetadata` instances in the collection
- provide a controllable ordering using the `Order` value
- part of #964
@danroth27 danroth27 added this to the 6.0.0-rc1 milestone Sep 30, 2014
@yishaigalatzer yishaigalatzer modified the milestones: 6.0.0-rc1, 6.0.0-beta3 Jan 19, 2015
dougbu added a commit that referenced this issue Jan 27, 2015
…perty order

#964
- compute `ModelMetadata.Order` based on `[Display]` attribute
 - property affects e.g. `@Html.DisplayFor()` generation for complex objects
 - also affects order of messages in validation summary
- use `PropertyName` to make overall order predictable
- update existing tests to match new property order
- test new scenarios involving `ModelMetadata.Order`
 - per-property `ModelMetadata` and related tests
 - validation and `HtmlHelper` tests
 - add `HtmlHelperValidationSummaryTest` (which touches on #453)
- update ModelBinding functional test to show use of `[Display(Order = x)]`

nits:
- remove some trailing whitespace
- move more `NullDisplayText` bits into proper slots (just above `Order`)
- avoid `Assert.True()` & `Assert.False()`; split some assertions up
- add more assertions in tests involving `ModelStateDictionary.CanAddErrors`
- `""` -> `string.Empty` in affected test classes
- rename "DefaultEditorTemplatesTest~~s~~" class and file to follow guidelines
- rename "ModelBindingTest~~s~~" class and file to follow guidelines
dougbu added a commit that referenced this issue Jan 29, 2015
…perty order

#964
- compute `ModelMetadata.Order` based on `[Display]` attribute
 - property affects e.g. `@Html.DisplayFor()` generation for complex objects
 - also affects order of messages in validation summary
- use `PropertyName` to make overall order predictable
- update existing tests to match new property order
- test new scenarios involving `ModelMetadata.Order`
 - per-property `ModelMetadata` and related tests
 - validation and `HtmlHelper` tests
 - add `HtmlHelperValidationSummaryTest` (which touches on #453)
- update ModelBinding functional test to show use of `[Display(Order = x)]`

nits:
- remove some trailing whitespace
- move more `NullDisplayText` bits into proper slots (just above `Order`)
- avoid `Assert.True()` & `Assert.False()`; split some assertions up
- add more assertions in tests involving `ModelStateDictionary.CanAddErrors`
- `""` -> `string.Empty` in affected test classes
- rename "DefaultEditorTemplatesTest~~s~~" class and file to follow guidelines
- rename "ModelBindingTest~~s~~" class and file to follow guidelines
dougbu added a commit that referenced this issue Jan 29, 2015
- #964
- compute `ModelMetadata.Order` based on `[Display]` attribute
 - property affects e.g. `@Html.DisplayFor()` generation for complex objects
 - also affects order of messages in validation summaries
- test new scenarios involving `ModelMetadata.Order`
 - per-property `ModelMetadata` and related tests
 - validation and `HtmlHelper` tests
 - add `HtmlHelperValidationSummaryTest` (which touches on #453)
- update ModelBinding functional test to show use of `[Display(Order = x)]`

nits:
- move more `NullDisplayText` bits into proper slots (just above `Order`)
 - add doc comments for `ComputeNullDisplayText()`
- add more assertions in tests using `ModelStateDictionary.HasReachedMaxErrors`
- remove some trailing whitespace
- avoid `Assert.True()` & `Assert.False()`; split some assertions up
- `""` -> `string.Empty` in affected test classes
- rename "DefaultEditorTemplatesTest~~s~~" class and file to follow guidelines
- rename "ModelBindingTest~~s~~" class and file to follow guidelines

FYI #1888 covers a predictable (or even just stable) order in the UI
@dougbu
Copy link
Member Author

dougbu commented Jan 29, 2015

fixed in commit 8779caf

@dougbu dougbu closed this as completed Jan 29, 2015
@SteveHindmarsh
Copy link

Using the attribute [Display(Order=x)] only allows developers to fix this proplem if the summary is showing errors for a single entity. This does not fix the problem for other scenarious such as when you wish to display errors for a collection of entities. The problem is simply fixed in this respect by displaying the errors in which they were added to the ModelErrorCollection which it currently does not! Your cracking a nut with a hammer - seemingly?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants