7

Say you have the following object:

public class Address
{
  public String Line1 { get; set; }
  public String Line2 { get; set; }
  public String City { get; set; }
  public String State { get; set; }
  public String ZipCode { get; set; }

  public Address()
  {
  }
}

public class Contact
{
  public String FirstName { get; set; }
  public String LastName { get; set; }
  public String Telephone { get; set; }

  public Address BillingAddress { get; set; }
  public List<Address> ShippingAddresses { get; set; }

  public Contact()
  {
    // Assume anything that _could_ be null wouldn't be. I'm excluding
    // most "typical" error checking just to keep the examples simple
    this.BillingAddress = new Address();
    this.ShippingAddresses = new List<Address>();
  }
}

Assume the properties are decorated with [Required], [Display] and other attributes.

Then my controller (simplified for the sake of demo):

public ActionResult Edit(String id)
{
  Contact contact = ContactManager.FindByID(id);
  return View(model: contact);
}

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Edit(Contact contact)
{
  if (ModelState.IsValid) //always fails
  {
    ContactManager.Save(contact);
    return RedirectToAction("Saved");
  }
  return View(model: contact);
}

I keep seeing demos on editing an object like this in MVC, but they are constantly breaking out collections of an object in to their own form (e.g. Edit the contact, then edit a specific address off of a contact). I, on the other hand, am trying to editing all this information within the same page and am having no success. For example:

@model Contact

// simplified for brevity
@using (Html.BeginForm())
{
  @Html.LabelFor(x => x.FirstName): @Html.EditorFor(x => x.FirstName)
  @Html.LabelFor(x => x.LastName): @Html.EditorFor(x => x.LastName)
  @Html.LabelFor(x => x.Telephone): @Html.EditorFor(x => x.Telephone)

  <div>
    @Html.LabelFor(x => x.BillingAddress.Line1): @Html.EditorFor(x => x.BillingAddress.Line1)
    @Html.LabelFor(x => x.BillingAddress.Line2): @Html.EditorFor(x => x.BillingAddress.Line2)
    @Html.LabelFor(x => x.BillingAddress.City): @Html.EditorFor(x => x.BillingAddress.City)
    @Html.LabelFor(x => x.BillingAddress.State): @Html.EditorFor(x => x.BillingAddress.State)
    @Html.LabelFor(x => x.BillingAddress.ZipCode): @Html.EditorFor(x => x.BillingAddress.ZipCode)
  </div>

  <div>
  @foreach (Address addr in Model.ShippingAddresses)
  {
    <div>
      @Html.LabelFor(x => addr.Line1): @Html.EditorFor(x => addr.Line1)
      @Html.LabelFor(x => addr.Line2): @Html.EditorFor(x => addr.Line2)
      @Html.LabelFor(x => addr.City): @Html.EditorFor(x => addr.City)
      @Html.LabelFor(x => addr.State): @Html.EditorFor(x => addr.State)
      @Html.LabelFor(x => addr.ZipCode): @Html.EditorFor(x => addr.ZipCode)
    </div>
  }
  </div>
}

The problem I keep running in to is that the ModelState.IsValid never passes when I go to save the information back. Is there a trick to doing this, or is it just outside the realm of MVC? I would like to take an object like Contact and dump all the information on one page for editing, and have it re-save successfully, but I can't seem to get it to work. (My next step is to tie in ajax so you could dynamically add/remove "ShipingAddresses" on that page, but I need the save to work first--K.I.S.S)

Problems:

  • ModelState.IsValid is almost always false
  • Form elements for collection items frequently have the same names, so in this demo every Line1 in the ShippingAddresses collection dumps to the page as name="addr_Line1" instead of something like ShippingAddresses[0]_Line1 like I'd expect.
Brad Christie
  • 100,477
  • 16
  • 156
  • 200

1 Answers1

10

I guess ModelState.IsValid is false because you didn't populate the shipping addresses collection properly. This happened because you didn't use proper names for your input fields. Take a look at the following article to understand better what format does the model binder expects your input fields to be named in order to be able to reconstruct values back.

The reason your code is not generating correct input names is because you used this foreach loop in which the view lost track of the navigation context.

So try like this:

@for (var i = 0; i < Model.ShippingAddresses.Count; i++)
{
    <div>
        @Html.LabelFor(x => x.ShippingAddresses[i].Line1): 
        @Html.EditorFor(x => x.ShippingAddresses[i].Line1)

        @Html.LabelFor(x => x.ShippingAddresses[i].Line2): 
        @Html.EditorFor(x => x.ShippingAddresses[i].Line2)

        @Html.LabelFor(x => x.ShippingAddresses[i].City): 
        @Html.EditorFor(x => x.ShippingAddresses[i].City)

        @Html.LabelFor(x => x.ShippingAddresses[i].State): 
        @Html.EditorFor(x => x.ShippingAddresses[i].State)

        @Html.LabelFor(x => x.ShippingAddresses[i].ZipCode): 
        @Html.EditorFor(x => x.ShippingAddresses[i].ZipCode)
    </div>    
}

Remark: notice that I have also replaced your duplicate labels with an EditorFor call to effectively generate input fields for those fields.

or even better using editor templates like this:

@model Contact

@using (Html.BeginForm())
{
    @Html.LabelFor(x => x.FirstName): 
    @Html.EditorFor(x => x.FirstName)

    @Html.LabelFor(x => x.LastName): 
    @Html.EditorFor(x => x.LastName)

    @Html.LabelFor(x => x.Telephone): 
    @Html.EditorFor(x => x.Telephone)

    @Html.EditorFor(x => x.BillingAddress)

    @Html.EditorFor(x => x.ShippingAddresses)
}

and then define a custom editor template which will automatically be rendered for each element of the ShippingAddresses collection (~/Views/Shared/EditorTemplates/Address.cshtml):

@model Address
<div>
    @Html.LabelFor(x => x.Line1): 
    @Html.EditorFor(x => x.Line1)

    @Html.LabelFor(x => x.Line2): 
    @Html.EditorFor(x => x.Line2)

    @Html.LabelFor(x => x.City): 
    @Html.EditorFor(x => x.City)

    @Html.LabelFor(x => x.State): 
    @Html.EditorFor(x => x.State)

    @Html.LabelFor(x => x.ZipCode): 
    @Html.EditorFor(x => x.ZipCode)
</div>

Now you should no longer worry about getting wrong input names. And not only this but you are reusing the address editor template for both your billing address and the collection of shipping addresses. It makes your views DRYier.

Darin Dimitrov
  • 1,023,142
  • 271
  • 3,287
  • 2,928
  • See, this is something I don't quite fully understand about MVC and the templating yet; If I want the elements to have "sensible" names (i.e. not all be named the same thing) the lamba plays a big part in that? – Brad Christie Jan 27 '12 at 20:15
  • @BradChristie, absolutely, it's crucial. But if you follow the following simple rule you will always get it right: if you find yourself writing a loop inside a view (`for` or `foreach`) you are doing it wrong. An alarm should ring immediately. So stop and replace this loop by a template (editor if you are presenting input fields and display if you are displaying data in a readonly manner). – Darin Dimitrov Jan 27 '12 at 20:20
  • Follow-up: Say I'm not using a list, and using a "Addresses" custom collection. Will templates still allow me this flexibility? e.g. `MyPage.cshtml`=>`@Html.EditorFor(x => x.Addresses)` -- `Addresses.cshtml`=>`
    for(i ...){ @Html.EditorFor(model => model[i])` -- (Then `Address.cshtml` template as you describe?)
    – Brad Christie Jan 27 '12 at 20:22
  • 1
    @BradChristie, why are you still showing for loops? I said no to loops. Templates work with absolutely any collections (custom or not), and not only collection, any types. So if we suppose that on your view model you have a property `IEnumerable FooBars { get; set; }` then inside your main view you don't write loops (remember this), you simply write `@Html.EditorFor(x => x.FooBars)` and then you define a custom editor template in `~/Views/Shared/EditorTemplates/MyElement.cshtml` which will automatically be rendered for each element of this collection... – Darin Dimitrov Jan 27 '12 at 20:25
  • ... The location and name of the template is important. It should be located either in `~/Views/Shared/EditorTemplates` or in `~/Views/XXX/EditorTemplates` where XXX is the name of your current controller. In the first case you are defining a template which could be reused along your entire application whereas in the second case you are defining a template which will be used only for views belonging to the XXX controller. The name of the template is also important. It must be the name of the type of the collection element. In this example it should be `MyElement.cshtml` because you have `IEnum – Darin Dimitrov Jan 27 '12 at 20:27
  • Didn't see your comment edit about loops until after I replied. However, what's the best method to make sure that that enumerable is wrapped in a container then? e.g. For argument's sake anytime there's an `IEnumerable
    ` I'd like it wrapped with a `
    @Html.EditorFor(x => x.FooBars
    `? Sticking with DRY, I would want that extracted from my Contact.cshtml template wouldn't I?
    – Brad Christie Jan 27 '12 at 20:28
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/7113/discussion-between-brad-christie-and-darin-dimitrov) – Brad Christie Jan 27 '12 at 20:29
  • @BradChristie, in this case you could do the following: `@Html.EditorFor(x => x.FooBars, "FooBars")` and then define `~/Views/Shared/EditorTemplates/FooBars.cshtml` template which this time will be strongly typed to `IEnumerable` instead of `MyElement` (because you passed a second argument to the EditorFor helper) in which you put the following code: `@model IEnumerable
    @Html.EditorForModel()
    `. And then of course you define your `~/Views/Shared/EditorTemplates/MyElement.cshtml` template.
    – Darin Dimitrov Jan 27 '12 at 20:31
  • I apologize for hitting you up with so many questions, I just am trying to further understand MVC. I've seen your posts in MVC-related questions and value your feedback. I've been doing great with getting things going up until I started getting in to objects with nested objects/collections thereof. (Templates have also been pretty straight-forward until, again, collections come in to play). – Brad Christie Jan 27 '12 at 20:33
  • @BradChristie, I would be happy to try and explain you any further questions you might have about templates. Don't hesitate to post them on SO and also you could drop me emails. I don't promise that I will respond immediately but will do my best. – Darin Dimitrov Jan 27 '12 at 20:35
  • I'm going to take what you've explained and try to apply it. Once again, I really appreciate your patience, expertise and time. I moved over to MVC (dabbling) about 2 months ago and so far like it, I just don't have the complete work-flow mapped out in my mind yet (and how the parts play together). But, given these comments, I think I have a more firm understanding of what I was doing wrong and which direction to head. Many thanks. – Brad Christie Jan 27 '12 at 20:39
  • This is what I meant by the constructor comment :) But I couldn't remember the details – Chris S Jan 27 '12 at 20:49
  • Please check out my [similar issue](http://stackoverflow.com/q/16423073/75500 "Submitting complex form in ASP.NET MVC") (check the update on the bottom). – Shimmy Weitzhandler May 07 '13 at 16:36