1

I am working on an online library using ASP.NET MVC. This is my view model for the library management page:

public class ManageViewModel
{
    public IPagedList<ManageBookViewModel> WholeInventory;
    public IPagedList<ManageBookViewModel> CurrentInventory;
    public bool OldInventoryIsShown { get; set; } = false;
}

In the corresponding view I have a checkbox for whether or not to show the old inventory and a local variable modelList, which I would like to set to Model.WholeInventory if the checkbox is checked and to Model.CurrentInventory otherwise. I use modelList to display a table with all the books and I would need its value to be reset every time I (un)check the checkbox in order for the list to be properly displayed. Is this possible? How would I go about doing this?

In my view I currently have:

<label class="switch">
    <input id="OldInventoryIsShown" name="OldInventoryIsShown" type="checkbox" />
    <span class="slider round"></span>
</label>
@{
    var modelList = Model.OldInventoryIsShown ? Model.WholeInventory : Model.CurrentInventory;
}      
@using (Html.BeginForm())
{
    <table id="bookInventory" class="table table-hover">
        <thead>
            <tr>
                <th>Author</th>
                <th>Title</th>
                ....
            </tr>
        </thead>
        @foreach (var entry in modelList)
        {
            <tr>
                <td>@Html.DisplayFor(modelItem => entry.Author)</td>
                <td>@Html.DisplayFor(modelItem => entry.Title)</td>
                ....
            </tr>
        }
    </table>

    <p>Page @(modelList.PageCount < modelList.PageNumber ? 0 : modelList.PageNumber) of @modelList.PageCount</p>    
    @Html.PagedListPager(modelList, page => Url.Action("Manage",  page }))
}

The controller action:

public ActionResult Manage(int? page)
{
    var wholeInventory = _bookService.GetBooksIncludingDisabled().Select(b => Mapper.Map<Book, ManageBookViewModel>(b));
    var currentInventory = _bookService.GetBooks().Select(b => Mapper.Map<Book, ManageBookViewModel>(b));
    int pageSize = 3;
    int pageNumber = page ?? 1;
    var model = new ManageViewModel
    {
        WholeInventory = wholeInventory.ToPagedList(pageNumber, pageSize),
        CurrentInventory = currentInventory.ToPagedList(pageNumber, pageSize)
    };
    return View(model);
}

Models:

Book.cs

public class Book
{
    public int BookId { get; set; }
    [Required]
    [MinLength(1)]
    public string Title { get; set; }
    [Required]
    [MinLength(1)]
    public string Author { get; set; }
    ....
    public bool IsDisabled { get; set; } = false;
    public virtual ICollection<UserBook> UserBooks { get; set; }
}

ManageBookViewModel.cs

public class ManageBookViewModel
{
    public int BookId { get; set; }
    [Required(AllowEmptyStrings = false, ErrorMessage = "Enter the book title")]
    public string Title { get; set; }
    [Required(AllowEmptyStrings = false, ErrorMessage = "Enter the book author.")]
    public string Author { get; set; }
    ....
    public bool IsDisabled { get; set; }
}
kanpeki
  • 435
  • 2
  • 6
  • 20
  • Why do you have 2 `IPagedList` properties? Since you need to populate this in the server, then all you need is one property (and assign its value based on the value of `OldInventoryIsShown`) –  Sep 11 '17 at 10:42
  • Share all the relevant parts from your view. It's not clear what you're using `modelList` for – adiga Sep 11 '17 at 10:45
  • @StephenMuecke Hi, initially I had other plans on how to display the book inventory and only had the two lists. Now I've added OldInventoryIsShown, but am not sure if it is a good idea to have this property int he model or just use a ViewBag property or some other kind of variable... WholeInventory includes the Current and Old Inventories. – kanpeki Sep 11 '17 at 10:59
  • But you only should have one collection property. This is no different form any other filtering using `PagedList` - you have a `
    ` that includes you `@Html.CheckBoxFor()` and makes GET to your controller method and you set the value of `IPagedList Inventory` based on the value of the checkbox (and you include the value of `OldInventoryIsShown` in the route parameters so its preserved when paging in the view)
    –  Sep 11 '17 at 11:04
  • You need to show the controller method as well –  Sep 11 '17 at 11:11
  • @StephenMuecke I was hoping to "toggle" between the two lists without calling the db eveytime to populate them, since I only have one admin at the moment and no changes woud have incurred to the lists in between applying/removing the filter). I do realize that this is limiting and not such a good idea from a design point of view.. – kanpeki Sep 11 '17 at 11:11
  • But that would not make much sense. You would need a `PagedListPager` for each collection. You would be degrading performance by generating far more html that needed and sending it to the client. If you toggle the visibility of the tables, the page numbers would be out and not make sense to a user. You will be far better of just making another call to the db. –  Sep 11 '17 at 11:15
  • But the fact that your using a view model and not the data model suggests your not taking advantage of server side paging anyway (unless you using the `StaticPageList`) –  Sep 11 '17 at 11:16
  • Looking at your controller, you hardly need to be worried about the 'extra' database call since your already making 100's of times as many calls as you need (you may as well not even use paging - your performance is so bad) –  Sep 11 '17 at 11:22
  • Can you show your models for both `Book` and `ManageBookViewModel` –  Sep 11 '17 at 11:24
  • @StephenMuecke I was trying to set modelList according to the checkbox being checked or not in order to avoid having two tables and toggling their visibility.. But I am not sure if it would be possible it like that.. My code is probably not affecting OldInventoryIsShown (not even in the frontend).. I couldn't use @Html.CheckBoxFor() because I have styling that depends on the :checked selector, which doesn't seem to work with the code generated by @Html.CheckBoxFor() – kanpeki Sep 11 '17 at 11:25
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/154133/discussion-between-stephen-muecke-and-kanpeki). –  Sep 11 '17 at 11:25

1 Answers1

2

Your ManageViewModel needs to include only one property for the paged list and it should be IPagedList<Book> (see explanation below)

public class ManageViewModel
{
    public IPagedList<Book> Inventory;
    [Display(Name = "Include old inventory")]
    public bool OldInventoryIsShown { get; set; }
    ... // any other search/filter properties
}

and your view needs to include the checkbox inside the <form> element, and the form should be making a GET to your controller method. Then you also need to include the current value of OldInventoryIsShown as a route value in the @Html.PagedListPager() method so that the current filter is retained when paging.

@model ManageViewModel
...
@using (Html.BeginForm("Manage", "ControllerName", FormMethod.Get))
{
    @Html.CheckBoxFor(m => m.OldInventoryIsShown)
    @Html.LabelFor(m => m.OldInventoryIsShown)
    ... // any other search/filter properties
    <input type="submit" value="search" />
}
<table id="bookInventory" class="table table-hover">
    ....
</table>
<p>Page @(modelList.PageCount < modelList.PageNumber ? 0 : modelList.PageNumber) of @modelList.PageCount</p>
@Html.PagedListPager(modelList, page => 
    Url.Action("Manage", new { page = page, oldInventoryIsShown = Model.OldInventoryIsShown })) // plus any other search/filter properties

Finally in the controller method you need a parameter for the value of the bool property an modify your query based on that value.

public ActionResult Manage(int? page, bool oldInventoryIsShown)
{
    int pageSize = 3;
    int pageNumber = page ?? 1;
    IQueryable<Book> inventory = db.Books;
    if (!oldInventoryIsShown)
    {
        inventory = inventory.Where(x => !x.IsDisabled);
    }
    ManageViewModel model = new ManageViewModel
    {
        Inventory = inventory.ToPagedList(pageNumber, pageSize),
        OldInventoryIsShown = oldInventoryIsShown
    };
    return View(model);
}

You current controller code is terribly inefficient. Lets assume your table has 10,000 Book records, and 5,000 of those are 'disabled' (archived). You current code first gets all 10,0000 records and adds them to memory. Then you map all then to a view model. Then you call another query to get another 5,0000 records (which are just duplicates of what you already have), which you add to memory and map to a view model. But all you want in the view is 3 records (the value of pageSize) so you have done thousands of times of extra unnecessary processing.

In your case, there is no need for a view model (although if you did need one, you would use the StaticPagedList methods - refer this answer for an example). Your query should be using your db context to generate an IQueryable<Book> so that only the results you need are returned from the database (internally the ToPagedList() method uses .Skip() and .Take() on IQueryable<T>)

  • Hi. Thank you for your answer. I would actually want the old inventory filter to be separate from search (the filter should be applied/removed when checking/unchecking the box, without the need to press a button). Is that possible? I tried a couple of things, but with no success. Example: extracted the checkbox outside the form, added id to the form containing search, on checkbox click called submit on the form in javascript. – kanpeki Sep 12 '17 at 11:51
  • Yes, of course, although in that case your would need javascript to handle the `.click()` event of the checkbox. But that is not normal expected behavior (and a poor UI) so it would be better to just have a link that you click (you would display either a (say) 'Show all' or 'Show current only' link depending on what is currently displayed) and that link would call the same `Manage` method –  Sep 12 '17 at 11:56
  • I've managed to make it work by adding an input of type submit with class hidden and an id, and triggering a click event on it whenever the checkbox was clicked. Initially I had added an id for the form and was calling submit on it, but that didn't work.. I'm still having trouble with the slider styling, but I've added another question for that. thank you :) – kanpeki Sep 14 '17 at 11:22
  • 1
    You do not really need your hidden input. You could just use `$('#yourCheckBoxId').click(function() { $(this).closest('form').submit(); })` - but triggering a submit on a form element is consider bad practice –  Sep 14 '17 at 11:25
  • Indeed, I could have done without the hidden field. I could have added the id to my already existing search button, though your method is more elegant, I suppose. The reason I insist on styling the checkbox as a slider is because I would expect something to happen right after switching on the slider, as opposed to a classic checkbox. But I will keep your recommendation in mind for the future. This is just a training project, so I am trying to build it in manner that seems appropriate to me. Might deploy it to Azure and submit it to some usability tests when complete :)) – kanpeki Sep 14 '17 at 12:01