Editing and saving complex objects

Question

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.

Homogenous

You can shorten [AcceptVerbs(HttpVerbs.Post)] to [HttpPost] – Reta

Does Contact have a default constructor? – Reta

@ChrisS: Yes, they all have empty constructors for the sake of argument. – Homogenous

Answer 1

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.

Modestomodesty

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? – Homogenous

@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). – Modestomodesty

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=><div>for(i ...){ @Html.EditorFor(model => model[i]) -- (Then Address.cshtml template as you describe?) – Homogenous

@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<MyElement> 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... – Modestomodesty

... 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 – Modestomodesty

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<Address> I'd like it wrapped with a <div class="addresses">@Html.EditorFor(x => x.FooBars</div>? Sticking with DRY, I would want that extracted from my Contact.cshtml template wouldn't I? – Homogenous

let us continue this discussion in chat – Homogenous

@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<MyElement> instead of MyElement (because you passed a second argument to the EditorFor helper) in which you put the following code: @model IEnumerable<MyElement> <div class="addresses">@Html.EditorForModel()</div>. And then of course you define your ~/Views/Shared/EditorTemplates/MyElement.cshtml template. – Modestomodesty

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). – Homogenous

@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. – Modestomodesty

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. – Homogenous

This is what I meant by the constructor comment :) But I couldn't remember the details – Reta

Please check out my similar issue (check the update on the bottom). – Farm