4

I'm working on a webapp for work, and I'm using standard CRUD style interactions. However, there are certain fields that I do not want the users updating, so I removed them from the view. However, if I don't explicitly set these fields, they're cleared when the model is updated in the database.

I'm concerned with what the proper method of populating the fields for my ViewModels is.

The rough idea I came up with was something like this:

My view model:

public class EditSoftwareTrackingViewModel 
{
    public EditSoftwareTrackingViewModel(SoftwareTracking model)
    {
        Id = model.Id;
        SoftwareId = model.SoftwareId;
        ComputerId = model.ComputerId;
        SoftwareActionId = model.SoftwareActionId;
        LastModified = model.LastModified;
        Computer = model.Computer;
        Software = model.Software;
        SoftwareAction = model.SoftwareAction;
    }
    public int Id { get; set; }
    [DisplayName("Software")]
    public int SoftwareId { get; set; }
    [DisplayName("Computer")]
    public int ComputerId { get; set; }
    [DisplayName("Software Action")]
    public int SoftwareActionId { get; set; }
    [DisplayName("Last Modified")]
    public DateTime? LastModified { get; set; }

    public virtual Computer Computer { get; set; }
    public virtual Software Software { get; set; }
    public virtual SoftwareAction SoftwareAction { get; set; }
}

My main model

[Table("asset.SoftwareTracking")]
public partial class SoftwareTracking
{
    public int Id { get; set; }
    [DisplayName("Software")]
    public int SoftwareId { get; set; }
    [DisplayName("Computer")]
    public int ComputerId { get; set; }
    [DisplayName("Date Entered")]
    public DateTime? EnteredDate { get; set; }
    [DisplayName("Software Action")]
    public int SoftwareActionId { get; set; }
    [DisplayName("Last Modified")]
    public DateTime? LastModified { get; set; }

    public virtual Computer Computer { get; set; }
    public virtual Software Software { get; set; }
    public virtual SoftwareAction SoftwareAction { get; set; }
}

And my controller using the view model

    public ActionResult Edit(int? id)
    {
        if (id == null)
        {
            return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
        }
        EditSoftwareTrackingViewModel softwaretracking = new EditSoftwareTrackingViewModel(db.SoftwareTrackings.Find(id));
        if (softwaretracking == null)
        {
            return HttpNotFound();
        }
        GeneratePageData(softwaretracking.Software.Id);
        return View(softwaretracking);
    }

    [HttpPost]
    [ValidateAntiForgeryToken]
    public ActionResult Edit(EditSoftwareTrackingViewModel softwaretracking)
    {
        if (ModelState.IsValid)
        {
            softwaretracking.LastModified = DateTime.Now;
            var softwareTrack = db.SoftwareTrackings.Find(softwaretracking.Id);
            softwareTrack = new SoftwareTracking
            {
                Computer = softwaretracking.Computer,
                ComputerId = softwaretracking.ComputerId,
                LastModified = softwaretracking.LastModified,
                Software = softwaretracking.Software,
                SoftwareAction = softwaretracking.SoftwareAction,
                SoftwareActionId = softwaretracking.SoftwareActionId,
                SoftwareId = softwaretracking.SoftwareId,
                EnteredDate = softwareTrack.EnteredDate
            };

            db.Entry(softwareTrack).State = EntityState.Modified;
            db.SaveChanges();
            return RedirectToAction("Index");
        }
        GeneratePageData(softwaretracking.Software.Id);
        return View(softwaretracking);
    }

Is there a better alternative? Or should I continue to create my view models in this manner?

EDIT

My business logic and view

    private void GeneratePageData(int? id = null)
    {

        ViewBag.Computers = new SelectList(db.Computers, "Id", "ComputerName");
        ViewBag.SoftwareActions = new SelectList(db.SoftwareActions, "Id", "ActionPerformed");

        var usedSoft = (from softTrack in db.SoftwareTrackings
                        where (softTrack.SoftwareActionId != 3)
                        select softTrack.Software);

        var softwareList = (from soft in db.Softwares
                            where (
                                ((from softTrack in db.SoftwareTrackings
                                  where (softTrack.SoftwareActionId != 3 && softTrack.SoftwareId == soft.Id)
                                  select softTrack.Software).Count() < soft.KeyQuantity)
                                && !(soft.AssetStatusId == 4 || soft.AssetStatusId == 5)
                                || soft.Id == id)
                            select soft).ToList();

        ViewBag.SoftwareList = softwareList.Select(t => new SelectListItem
        {
            Text = t.SoftwareIdNameFull,
            Value = t.Id.ToString()
        });

    }

And my view

@model Lighthouse_Asset_Manager.Models.EditSoftwareTrackingViewModel

@{
    ViewBag.Title = "Edit Software Install";
    Layout = "";
}

<div class="modal-header">
    <button type="button" class="close" data-dismiss="modal" aria-hidden="true">
        &times;
    </button>
    <h4 class="modal-title" id="myModalLabel">Edit Software Install</h4>
</div>
<div class="modal-body">

    @using (Html.BeginForm(null, null, FormMethod.Post, new { id = "computerForm" }))
    {
        @Html.AntiForgeryToken()
        @Html.HiddenFor(model => model.Id)

        <div class="form-horizontal">
            @Html.ValidationSummary(true)


            <div class="form-group">
                @Html.LabelFor(model => model.SoftwareId, "Software", new { @class = "control-label col-md-2" })
                <div class="col-md-10">
                    @Html.DropDownList("SoftwareId", (IEnumerable<SelectListItem>)ViewBag.SoftwareList, "-- Select --", new
                    {
                        @style = "width:100%",
                        @class = "select2"
                    })
                    @Html.ValidationMessageFor(model => model.SoftwareId)
                </div>
            </div>

            <div class="form-group">
                @Html.LabelFor(model => model.ComputerId, "Computer", new { @class = "control-label col-md-2" })
                <div class="col-md-10">
                    @Html.DropDownList("ComputerId", (IEnumerable<SelectListItem>)ViewBag.Computers, "-- Select --", new
                    {
                        @style = "width:100%",
                        @class = "select2"
                    })
                    @Html.ValidationMessageFor(model => model.ComputerId)
                </div>
            </div>

            <div class="form-group">
                @Html.LabelFor(model => model.SoftwareActionId, "Action", new { @class = "control-label col-md-2" })
                <div class="col-md-10">
                    @Html.DropDownList("SoftwareActionId", (IEnumerable<SelectListItem>)ViewBag.SoftwareActions, "-- Select --", new
                    {
                        @style = "width:100%",
                        @class = "form-control"
                    })
                    @Html.ValidationMessageFor(model => model.SoftwareActionId)
                </div>
            </div>


            <div class="form-actions no-color">
                <button type="submit" class="btn btn-primary btn-sm"><i class="fa fa-floppy-o"></i> Edit Install Record</button>
                <button type="button" class="btn btn-default" data-dismiss="modal">
                    Cancel
                </button>
            </div>
        </div>
    }
</div>
JD Davis
  • 3,517
  • 4
  • 28
  • 61
  • 1
    Using a view model is the correct approach, but in you POST method, you need to get the original `SoftwareTracking` object from the database and map your view model properties to it, then save the original. Sidenote: you do not (and should not) need to call `GeneratePageData(softwaretracking.Software.Id);` in the POST method (trying to reset the view model properties based on the original model is ignored anyway) –  Jan 17 '15 at 03:12
  • I updated my code above so it actually works, however, I'm not sure what else to do to it to make it cleaner, more efficient.. The reason I had the GeneratePageData is due to me filling the ViewBag with certain lists that are displayed within the view. Do these carry from the get method? – JD Davis Jan 17 '15 at 03:15
  • 2
    No, you need to repopulate any select lists before you return the view - if that's all its doing then its OK. Since you have a view model, you should be including the `SelectList` properties in the view model, not in `ViewBag`, and your view model contains a `LastModified ` property which seems unnecessay (just set the value in the data model in the controller when you post back and save - I'm assuming its not something that the user should be able to edit). You can also look at tools like [automapper](http://automapper.codeplex.com/) to make it a little easier and cleaner –  Jan 17 '15 at 03:23
  • I'm actually performing some very complex linq statements to create the select lists. I thought it was best practice to keep business logic out of the models? – JD Davis Jan 17 '15 at 03:25
  • 1
    I agree. I'm just suggesting that your view model contains the properties for the `SelectLists` and you still have a common private method to assign those properties that you call in the GET and (assuming you return the view) POST methods. I use something like `private void ConfigureEditModel(MyViewModel model) { model.MyFirstSelectList = someLinqQuery; }`. –  Jan 17 '15 at 03:30
  • @Jdsfighter He is saying that instead of setting `ViewBag.SomeList = complex_query` you have `softwaretrackingView.SomeList = complex_query` – wal Jan 17 '15 at 03:31
  • Thank you, that makes more sense. Also, after some quick digging. Google says that it's actually preferential to have your business logic in a model versus a controller. I guess I had them flipped in my head. I'm just really trying to get the feel for MVVC. It's a bit different than I'm used to. – JD Davis Jan 17 '15 at 03:33
  • Your view model also contains properties for `Computer`, `Software` and `SoftwareAction` but the fact you have separate `int` properties for `ComputerId` etc and `SelectLists` suggests you are creating dropdownlists to select the `Computer` etc, in which case there seems no need for the `Computer`, `Software` and `SoftwareAction` properties (would need to see the view to be sure) –  Jan 17 '15 at 03:36
  • I believe you may be right. I updated the original post with my business logic and the view respectively. – JD Davis Jan 17 '15 at 03:40
  • If it isn't sensitive information you could always use the hidden control otherwise I would recommend retrieving the record(s) from the database and setting them again (if you're doing concurrency check maybe you're already retrieving them so there's no problem there) – kondas Jan 17 '15 at 03:56
  • Other than some superfluous properties in the view model and a few enhancements you could make (including a mapping tool) you approach is generally good. I'll post an answer later with a few suggested improvements. –  Jan 17 '15 at 03:57

4 Answers4

4

You approach of using a view model is a good one. The answers to this question explains some of the benefits including preventing over-posting attacks, using view specific display and validation attributes and including view specific properties such as SelectLists. Tools such as automapper can make it easy to map between you data and view models and reduce the code in the controller. A few changes I would suggest to your view model. The LastModified, Computer, Software and SoftwareAction properties are not required (you not binding to these), and I would include the SelectList properties in the model rather than ViewBag

View model

public class EditSoftwareTrackingViewModel 
{
  public int Id { get; set; }
  [Display(Name="Software")]
  public int SoftwareId { get; set; }
  [Display(Name="Computer")]
  public int ComputerId { get; set; }
  [Display(Name="Software Action")]
  public int SoftwareActionId { get; set; }
  public SelectList Computers { get; set; }
  public SelectList SoftwareActions{ get; set; }
  public SelectList SoftwareList{ get; set; }
}

Then change the GeneratePageData() method to accept the view model

private void GeneratePageData(EditSoftwareTrackingViewModel model)
{
  model.Computers = new SelectList(db.Computers, "Id", "ComputerName");
  ....

and in the view (always preferable to use the strongly typed helpers)

@Html.DropDownListFor(m => m.SoftwareId, Model.SoftwareList, "-- Select --", new { @class = "select2" })

A few other things to note.

  • You should use the [Display(Name="..")] attribute (not [DisplayName(..)])
  • When you set the LastModified property, you should consider using UCT time.
  • The hidden input for the Id property is not required in the view (assuming your using the default {controller}/{action}/{id} route mapping) - its added to the route values and will be bound anyway
  • Unless you specifically want an id attribute for the form, you can just use @using(Html.BeginForm()) {
  • You do not need the second parameter in LabelFor() - it can be just Html.LabelFor(m => m.SoftwareId, new { @class = "control-label col-md-2" }) since you have specified it in the [Display] attribute

Finally, if you want to simplify your view further, you could consider custom EditorTemplates or html helpers as indicated in this answer which would allow you to replace

<div class="form-group">
  @Html.LabelFor(model => model.SoftwareId, new { @class = "control-label col-md-2" })
  <div class="col-md-10">
    @Html.DropDownListFor(m => m.SoftwareId, Model.SoftwareList, "-- Select --", new { @class = "select2" })
    @Html.ValidationMessageFor(model => model.SoftwareId)
  </div>
</div>

with (custom EditorTemplate)

@Html.EditorFor(m => m.SoftwareId, "BootstrapSelect", Model.SoftwareList)

or (custom HtmlHelper)

@Html.BootstrapDropDownFor(m => m.SoftwareId, Model.SoftwareList)
Community
  • 1
  • 1
  • Some amazing suggestions in there. Thank you very much for your input. – JD Davis Jan 17 '15 at 18:18
  • On a side-note, you mentioned setting the `LastModified` property from the model. How would one achieve this. I can't seem to wrap my head around it. – JD Davis Jan 17 '15 at 22:13
  • Assuming you don't want the user to edit it, then the only time you would just set its value (DateTime.Now) is in the controller immediately before saving (i.e map view model to data model, the set the last modifed date of the data model, then save the data model –  Jan 17 '15 at 22:35
  • That is how I currently handle it. I simply update the Model after mapping the ViewModel and publish it to the database. I have also updated all of my Models to now contain the SelectLists so that I simply load their data in the view, and no longer perform the logic from within the controller. – JD Davis Jan 17 '15 at 22:56
  • Yes I know, its just that previously you had the property in the view model, then updated the view model and then mapped it to the data model (the first 2 steps aren't really necessary :) –  Jan 17 '15 at 23:04
  • I started using AutoMapper and it's heavily simplifying the process. However, I'm beginning to realize the visual studio's built in scaffolding tools often do things in a somewhat odd manner and there's a lot of corrections I have to make to the generated code. – JD Davis Jan 17 '15 at 23:32
  • I don't use EF or scaffolding tools so can't really comment :( –  Jan 17 '15 at 23:36
2

You should use the AutoMapper to make the mapping between Model and ViewModel cleaner. Use this code to create the mapper first.

Mapper.CreateMap<SoftwareTracking, EditSoftwareTrackingViewModel>();
Mapper.CreateMap<EditSoftwareTrackingViewModel, SoftwareTracking>();

When you want to create a viewmodel from model, do this:

public ActionResult Edit(int? id)
{
    SoftwareTracking tracking = db.SoftwareTrackings.Find(id);
    EditSoftwareTrackingViewModel viewmodel = 
        Mapper.Map<SoftwareTracking, EditSoftwareTrackingViewModel>(tracking);
    return View(viewmodel);
}

When you want to populate the info from the viewmodel back to the model, do this

public ActionResult Edit(EditSoftwareTrackingViewModel vm)
{
    if (ModelState.IsValid)
    {
        vm.LastModified = DateTime.Now;
        var softwareTrack = db.SoftwareTrackings.Find(softwaretracking.Id);
        softwareTrack = 
           Mapper.Map<EditSoftwareTrackingViewModel, SoftwareTracking>(vm, softwareTrack);

        db.Entry(softwareTrack).State = EntityState.Modified;
        db.SaveChanges();
        return RedirectToAction("Index");
    }
Huy Hoang Pham
  • 4,107
  • 1
  • 16
  • 28
1

To patch update your model without loading the object from Db. Try Attach:

    [HttpPost]
    [ValidateAntiForgeryToken]
    public ActionResult Edit(EditSoftwareTrackingViewModel softwaretracking)
    {
        if (ModelState.IsValid)
        {
            var softwareTrack = new SoftwareTracking
            {
                 Computer = softwaretracking.Computer,
                 ComputerId = softwaretracking.ComputerId,
                 LastModified = softwaretracking.LastModified,
                 Software = softwaretracking.Software,
                 SoftwareAction = softwaretracking.SoftwareAction,
                 SoftwareActionId = softwaretracking.SoftwareActionId,
                 SoftwareId = softwaretracking.SoftwareId,
                 EnteredDate = softwareTrack.EnteredDate
            };
            db.SoftwareTrackings.Attach(softwareTrack);

            db.Entry(softwareTrack).Property(a => a.Computer).IsModified = true;
            db.Entry(softwareTrack).Property(a => a.ComputerId).IsModified = true;
            db.Entry(softwareTrack).Property(a => a.LastModified).IsModified = true;
            db.Entry(softwareTrack).Property(a => a.Computer).IsModified = true;
            db.Entry(softwareTrack).Property(a => a.Software).IsModified = true;
            db.Entry(softwareTrack).Property(a => a.SoftwareAction).IsModified = true;

            db.Entry(softwareTrack).Property(a => a.SoftwareActionId).IsModified = true;
            db.Entry(softwareTrack).Property(a => a.SoftwareId).IsModified = true;
            db.SaveChanges();
            return RedirectToAction("Index");
        }
        GeneratePageData(softwaretracking.Software.Id);
        return View(softwaretracking);
    }

Regarding the second question about whether to use ViewModel or just use the Model directly. This is really a matter of opinion, each approach has its pros and cons. I don't have strong opinion about this, i just want to point out these pros and cons for your consideration:

  • Using the model directly saves us from creating the viewModel, resulting in smaller source code and avoiding mapping logic but it would mix concerns. Because you use the same Model for your domain logic and for communcating with the client, any changes to the model may propagate up to the client if we don't take that into account.
  • Using the viewModel is a good way for separation of concerns but it would require more effort and mapping logic (maybe slow down the performance a bit). To apply ViewModel efficiently, I suggest using a mapper: https://github.com/AutoMapper/AutoMapper/wiki/Getting-started
Khanh TO
  • 48,509
  • 13
  • 99
  • 115
  • If I went this route, using a ViewModel would be superfluous as it would be unnecessary to use a ViewModel to eliminate fields. I could simply mark what fields I wanted to be changed. – JD Davis Jan 17 '15 at 03:24
  • @Jdsfighter: you must indicate to entity framework which fields you need to update as there is no way entity framework is aware of this because you update your data in another context. With your situation, you may need to convert ViewModel to your Model and mark which fields to update. Or you may use the Model directly if there are not so many mismatches between the 2 models. – Khanh TO Jan 17 '15 at 03:30
  • The models are identical, with only certain properties excluded from the ViewModel. – JD Davis Jan 17 '15 at 03:32
  • @Jdsfighter: if there are mismatches. We usually create a Mapper to map from ViewModel and Model so that we can reuse this mapping code. A good library for this is https://github.com/AutoMapper/AutoMapper/wiki/Getting-started – Khanh TO Jan 17 '15 at 03:35
  • @Jdsfighter: Usually, the ViewModel should be `flattened` from Model and does not contain complex domain structure. – Khanh TO Jan 17 '15 at 03:36
  • @Jdsfighter: In some cases, using a viewModel or not is a matter of opinion. I would go with either way. Each has its own pros and cons. – Khanh TO Jan 17 '15 at 04:02
1

This is the Model Class

[Table("CURRENCY")]
    public class CurrencyClass : ICurrency
    {
        private Int32 mCURRENCY_ID = default(Int32);
        [Key]
        public virtual Int32 CURRENCY_ID  
        {
            get { return mCURRENCY_ID; }
            set { mCURRENCY_ID = value; }
        }
        private string mCURRENCY_NAME = default(string); 
        public virtual string CURRENCY_NAME 
        { 
            get { return mCURRENCY_NAME;}
            set { mCURRENCY_NAME = value;}
        }
        private string mCURRENCY_DESC = default(string);
        public  virtual string CURRENCY_DESC 
        {
            get { return mCURRENCY_DESC; }
            set { mCURRENCY_DESC = value; }
        }
        private string mCURRENCY_SYMBOLE = default(string);
        public virtual string CURRENCY_SYMBOLE 
        {
            get { return mCURRENCY_SYMBOLE; }
            set { mCURRENCY_SYMBOLE = value; }
        }
        private Int32 mcreated_by = default(Int32);
        public virtual Int32 created_by 
        {
            get { return mcreated_by; }
            set { mcreated_by = value; } 
        }
        private DateTime mcreated_date = default(DateTime);
        public virtual DateTime created_date 
        {
            get { return mcreated_date; }
            set { mcreated_date = value; } 
        }
        private Int32 mmodified_by = default(Int32);
        public virtual Int32 modified_by 
        {
            get { return mmodified_by; }
            set { mmodified_by = value; } 
        }
        private DateTime mmodified_date = default(DateTime);
        public virtual DateTime modified_date 
        {
            get { return mmodified_date; }
            set { mmodified_date = value; }
        }
    }

This is the ViewModel

public class CurrencyViewModel
    {
        [Key]
        public Int32 CURRENCY_Id { get; set; }
        [Required(ErrorMessage="Currency Name is required")]
        public string CURRENCY_NAME { get; set; }
        [Required(ErrorMessage="Currency Description is required")]
        public string CURRENCY_DESC { get; set; }
        [Required(ErrorMessage = "Currency Symbole is Required")]
        public string CURRENCY_SYMBOLE { get; set; }
    }

This is the Action

[HttpPost]
        [ActionName("Create")]
        public ActionResult Create(CurrencyViewModel vm)
        {
            if (!ModelState.IsValid)
            {
                return View("Create");
            }   

            obj.CURRENCY_NAME = vm.CURRENCY_NAME;
            obj.CURRENCY_DESC = vm.CURRENCY_DESC;
            obj.CURRENCY_SYMBOLE = vm.CURRENCY_SYMBOLE;
            obj.created_by = 1;
            obj.created_date = DateTime.Now;
            obj.modified_by = 1;
            obj.modified_date = DateTime.Now;
            db.Currencies.Add(obj);
            db.SaveChanges();

            return RedirectToAction("Index");
        }
Durgpal Singh
  • 11,481
  • 4
  • 37
  • 49