1

I am currently in the process of getting accustomed to MVC, having come from ASP.Net.

So far I have found ways to achieve what I want to do, but with this one I am getting a "This cannot be the easiest way" moment.

Scenario: I am migrating a quoting application to MVC that has an existing database so my model classes are auto-generated. I have created a viewmodel class for each controller action that needs to display data to the user.

The edit quote viewmodel looks like this:

public class QuoteEdit_ViewModel
{
    public SelectList DelLocations { get; set; }

    public int QuoteID { get; set; }

    public string QuoteNo { get; set; }
    public string EnquiryNo { get; set; }
    public string SalesPerson { get; set; }
    public string Exceptions { get; set; }
    public string CreatedBy { get; set; }
    public string ModifiedBy { get; set; }

    [Required]
    [Display(Name = "Equipment Overview")]
    public string EquipmentOverview { get; set; }

    public DateTime Created { get; set; }
    public DateTime Modified { get; set; }

    [Required]
    public int? Validity { get; set; }

    [Required]
    [Display(Name = "Minimum Delivery Weeks")]
    public int? DeliveryMin { get; set; }

    [Required]
    [Display(Name = "Maximum Delivery Weeks")]
    public int? DeliveryMax { get; set; }

    [Required]
    [Display(Name = "Delivery Location")]
    public int? DelLocationID { get; set; }

    public List<Constants.QPT> PackTypes { get; set; }
    public List<Constants.QE> Equipments { get; set; }
    public List<Constants.QEEx> Extras { get; set; }
}

The lists at the bottom contain equipment lines etc that are junction tables in the database.

Currently I can edit this and post the data back to the database and it works perfectly.

The part that seems messy is the following, specifically the part after the if:

 public ActionResult Save(QuoteEdit_ViewModel VM)
    {
        Quote a = DAL.DB.Quotes.Where(x => x.QuoteID == VM.QuoteID).Single();
        TryUpdateModel(a);

        if (ModelState.IsValid)
        {
            DAL.DB.SaveChanges();
            return RedirectToAction("Dashboard", "Home");
        }

        VM.DelLocations = DAL.GetDeliveryLocationDropdown();

        var QData = DAL.GetQuoteEditVM(VM.QuoteID);
        VM.QuoteNo = QData.QuoteNo;
        VM.EnquiryNo = QData.EnquiryNo;
        VM.SalesPerson = QData.SalesPerson;
        VM.PackTypes = QData.PackTypes;
        VM.Equipments = QData.Equipments;
        VM.Extras = QData.Extras;
        VM.Created = QData.Created;
        VM.CreatedBy = QData.CreatedBy;
        VM.Modified = QData.Modified;
        VM.ModifiedBy = QData.ModifiedBy;

        return View("Edit", VM);
    }

Currently I need to reload the entire viewmodel and repopulate any fields that were not bound in the view as their values are lost during the POST.

I have read in other posts that you can use hiddenfor, but can this be used for Lists as well?

Also is this the correct way to approach this or am I completely missing the point of MVC?

ThatChris
  • 752
  • 1
  • 4
  • 18

1 Answers1

0

Do not use HiddenFor. You're going about the correct way. The only change I would make is factoring out your common code into another method(s) that both the GET and POST actions can utilize.

private void PopulateQuoteEditViewModel(QuoteEdit_ViewModel model)
{
    mode.DelLocations = DAL.GetDeliveryLocationDropdown();

    var QData = DAL.GetQuoteEditVM(model.QuoteID);
    model.QuoteNo = QData.QuoteNo;
    model.EnquiryNo = QData.EnquiryNo;
    model.SalesPerson = QData.SalesPerson;
    model.PackTypes = QData.PackTypes;
    model.Equipments = QData.Equipments;
    model.Extras = QData.Extras;
    model.Created = QData.Created;
    model.CreatedBy = QData.CreatedBy;
    model.Modified = QData.Modified;
    model.ModifiedBy = QData.ModifiedBy;
}

Then:

public ActionResult QuoteEdit()
{
    var model = new QuoteEdit_ViewModel();

    PopulateQuoteEditViewModel(model);
    return View(model);
}

[HttpPost]
public ActionResult QuoteEdit(QuoteEdit_ViewModel model)
{
    if (ModelState.IsValid)
    {
        ...
    }

    PopulateQuoteEditViewModel(model);
    return View(model);
}

Other Comments

  1. Don't use TryUpdateModel. It's not meant to be used the way you are here. The correct approach is to map over the posted values from your view model. You can either do this manually, or utilize a library like AutoMapper. Either way, you don't want to just willy-nilly overwrite anything on your database entity based on raw posted data as you're doing.

  2. You should never post the ID of the entity you're editing, but rather rely on obtaining it from the URL via a route param. If the URL is changed to a different ID, you are literally editing a different thing, and you can add object-level permissions and such to control who can edit what. However, the ID that's posted can be manipulated, and if you aren't careful (as you aren't being here), then a user can tamper with the ID to mess with objects they potentially shouldn't be editing.

Chris Pratt
  • 232,153
  • 36
  • 385
  • 444
  • Thanks for the feedback. I will look into factoring the code and also read up on AutoMapper and route parameters. – ThatChris Nov 23 '16 at 04:53