Incorrect model value within foreach in my view

Question

I have a table where I keep a list of Roles, which has three columns: Name and two buttons Edit and Delete. When users click on Edit, it takes them to another form where they can edit the name of the role. If they click on Delete, they get a modal dialog asking them for verification before they delete the role.

My issue is the following: if I click on the Edit button, it detects the right Role and the correct role name shows up in the new form that appears. However, if I click on Delete, it always grabs the first Role of the Model list (that is, the first role of the table), no matter which one I clicked on. Why?

Here's my view:

<table class="table table-striped table-hover ">
    <thead>
        <tr>
            <th>@Resources.Role</th>
            <th></th>
            <th></th>
        </tr>
    </thead>
    <tbody>
        @foreach (var role in Model)
        {
            <tr>
                <td>
                    @role.Name
                </td>
                <td>
                    @using (Html.BeginForm("RoleEdit", "Admin", new { ReturnUrl = ViewBag.ReturnUrl }, FormMethod.Get, new { @class = "form-horizontal", role = "form" }))
                    {
                        @Html.AntiForgeryToken()
                        @Html.HiddenFor(m => m.Where(r => r.Id.Equals(role.Id)).FirstOrDefault().Name)
                        <input type="submit" value="@Resources.Edit" class="btn btn-default btn-sm" />
                    }
                </td>
                <td>
                    <input type="submit" value="@Resources.Delete" class="btn btn-default btn-sm" data-toggle="modal" data-target="#confirm-delete" />

                    <div class="modal fade" id="confirm-delete" tabindex="-1" role="dialog" aria-labelledby="myModalLabel" aria-hidden="true">
                        <div class="modal-dialog">
                            <div class="modal-content">
                                <div class="modal-header">
                                    @Resources.DeleteRole
                                </div>
                                <div class="modal-body">
                                    @Resources.AreYouSureYouWantToDelete
                                    <hr />
                                    @role.Name
                                </div>
                                <div class="modal-footer">
                                    @using (Html.BeginForm("RoleDelete", "Admin", new { ReturnUrl = ViewBag.ReturnUrl }, FormMethod.Post, new { @class = "form-horizontal", role = "form" }))
                                    {
                                        @Html.AntiForgeryToken()
                                        @Html.HiddenFor(m => m.Where(r => r.Id.Equals(role.Id)).FirstOrDefault().Name)
                                        <button type="button" class="btn btn-default" data-dismiss="modal">@Resources.Cancel</button>
                                        <input type="submit" value="@Resources.Delete" class="btn btn-danger btn-ok" />
                                    }
                                </div>
                            </div>
                        </div>
                    </div>
                </td>
            </tr>
        }
    </tbody>
</table>

Why do you have a form to redirect to an edit method? It should just be a link - e.g. `@Html.ActionLink("Edit", "RoleEdit", "Admin", new { id = role.ID })` — , Oct 06 '15 at 10:11
And you creating invalid html because `id="confirm-delete"` is generating duplicate `id` attributes and I assume is the cause of you problem. But you need to show your script. — , Oct 06 '15 at 10:13
I'm using a form so that I can use a button. Is there a way I could use a button with ActionLink? And I'm not using any script. — erictrigo, Oct 06 '15 at 10:16
And of course your using a script - the modal doesn't just appear by magic :) — , Oct 06 '15 at 10:19
Well, if I'm using one I didn't add it, so I assume something that got created with the Visual Studio project template. Regarding the action link, I got it to look like a button, but I can't figure out how to make the actions work, which I do with the current setup (I have two actions, one for Get and other for Post to which I pass a RoleEditViewModel to both of them). — erictrigo, Oct 06 '15 at 10:32
Your GET should definitely not have a parameter for `RoleEditViewModel`. It should be `public ActionResult Edit(int id)`. The link will pass the role ID value to the method. Inside the method you get the Role based on the ID and pass it to the view. for example `var role = db.Roles.Where(r => r.ID == ID).FirstOrDefault(); var model = new RoleEditViewModel();` and set properties of the view model properties based on the data model properties, then `return View(model);` — , Oct 06 '15 at 10:39
I think better you make delete button type as type="button" instead of type="submit" and on click event of button you can write like $(this).closest("form").submit(); — Ravi, Oct 06 '15 at 10:47
@StephenMuecke I added the following `@Html.ActionLink(Resources.Edit, "RoleEdit", "Admin", null, new { ID = role.Id, @class = "btn btn-default btn-sm" })` and my action signature is `[HttpGet] [Authorize(Roles = "Admin")] public ActionResult RoleEdit(string ID)`, but "ID" parameter is always `null`. What am I missing? Notice that `role.Id` is actually a `string`. — erictrigo, Oct 06 '15 at 10:51
Your using the wrong overload of `@Html.ActionLink()` - your adding an `id` attribute, not a route value - it should be `@Html.ActionLink(Resources.Edit, "RoleEdit", "Admin", new { ID = role.Id }, new { @class = "btn btn-default btn-sm" })` — , Oct 06 '15 at 10:55
@StephenMuecke Thanks, that did it. Could you briefly explain to me why is this a better practice than using a form and passing the ViewModel for both Actions (GET and POST) like I was doing? — erictrigo, Oct 06 '15 at 11:19
To long to go into detail, but a GET is for getting data which is what you initially want to do - i.e get the model so you can edit it. And you should always be able to navigate to it by typing it in the address bar (although that may result in displaying an error if the user does not have the permission to edit it). A POST is for modifying data, which is what you do when you submit the form - you update the database with the modified data. — , Oct 06 '15 at 11:24
And a GET method should not generally have a model as a parameter. It (1) will create a really ugly query string (2) you could easily exceed the query string limit and throw an exception (3) if the model contains properties which are complex objects (other models) or collections then binding will fail — , Oct 06 '15 at 11:26
In your case you were just passing a single property (i.e the value of the hidden input) to the GET method so it was never really a problem but its just an unusual way of doing it. — , Oct 06 '15 at 11:29
@StephenMuecke Thanks for clarifying a lot of stuff for me, based on what you said I got to make a few changes to my code and the way I've been doing things. I still don't know exactly what's wrong with the Delete, or modal though, could you elaborate on what's happening? If you do it in an answer I'll give it to you. — erictrigo, Oct 06 '15 at 11:35
I would first start by removing the `id="confirm-delete"` from the `
` and see what happens. — , Oct 06 '15 at 11:36
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/91481/discussion-between-antrim-and-stephen-muecke). — erictrigo, Oct 06 '15 at 11:37

score 1 · Accepted Answer · answered Oct 06 '15 at 22:31

Your issue is that you are creating duplicate id attributes in your foreach loop (which is invalid html) in the following line of code

The submit button above it is target the element with that id

<input type="submit" ... data-target="#confirm-delete" />

which will only ever return the first element with id="confirm-delete"

One way to solve this if to ensure your elements have unique id attributes, for example

@{ int counter = 0; }
@foreach (var role in Model)
{
  var id = string.Format("confirm-delete-{0}", ++counter);
  var target = string.Format("#{0}", id);
  ....
  <input type="submit" ... data-target="@target" />
  <div class="modal fade" id="@id" ...>

However this design is generating a separate dialog in each iteration and generating a lot of extra unnecessary html. It would be be better to have one dialog and handle the .submit() event to display the dialog and cancel the submit if it returns false. You have not indicated which jquery plugin your using for the dialog, so that will need to be the subject of a separate question.

Side notes:

You can simplify your view by using a link for the redirecting to the Edit() method rather than using a form element

@Html.ActionLink("Edit", "RoleEdit", "Admin", new { ID = role.Id }, new { @class = "btn btn-default btn-sm" })

and int the 'Delete' form, you can add the Id as a route value and omit the hidden input. Note also that your deleting data so it should be a POST, not a GET

@using (Html.BeginForm("RoleEdit", "Admin", new { ReturnUrl = ViewBag.ReturnUrl, id = item.Id }, FormMethod.Post, new { @class = "form-horizontal", role = "form" }))
{
    @Html.AntiForgeryToken()
    <input type="submit" value="@Resources.Edit" class="btn btn-default btn-sm" />
}

however a better way to handle the 'Delete' would be to post the value using ajax and return a result indicating success or otherwise, and if successful, remove the corresponding item form the DOM. This allows the users to stay on the same page and continue to delete other items.

Incorrect model value within foreach in my view

1 Answers1