tags:

views:

116

answers:

4

if i start off on a Detail page:

http:\\www.mysite.com\App\Detail

i have a controller action called Update which normally will call redirectToAction back to the detail page. but i have an error that is caught in validation and i need to return before redirect (to avoid losing all of my ModelState). Here is my controller code:

 public override ActionResult Update(Application entity)
    {
        base.Update(entity);
        if (!ModelState.IsValid)
        {
            return View("Detail", GetAppViewModel(entity.Id));
        }
      return RedirectToAction("Detail", new { id = entity.Id }) 

but now I see the view with the validation error messages (as i am using HTML.ValidationSummary() ) but the url looks like this:

http:\\www.mysite.com\App\Update

is there anyway i can avoid the URL from changing without some hack of putting modelstate into some temp variables? Is there a best practice here as the only examples i have seen have been putting ModelState in some tempdata between calling redirectToAction.

+1  A: 

The best practice you ask for is actually what you explained not to do: putting modelstate into tempdata. Tempdata is meant for it, that's why I would not call it a hack.

If this is to much repetitive code you could use the attribute modeldatatotempdata of MVCContrib. But the store is still TempData.

Malcolm Frexner
+3  A: 

As of ASP.NET MVC 2, there isn't any such API call that maintains the URL of the original action method when return View() is called from another action method.

Therefore as such, the recommended solution and a generally accepted convention in ASP.NET MVC is to have a corresponding, similarly named action method that only accepts a HTTP POST verb. So in your case, having another action method named Detail like so should solve your problem of having a different URL when validation fails.

[HttpPost]
public ActionResult Detail(Application entity)
{
    base.Update(entity);
    if (ModelState.IsValid)
    {
        //Save the entity here
    }
   return View("Detail", new { id = entity.Id });
}  

This solution is in line with ASP.NET MVC best practices and also avoids having to fiddle around with modestate and tempdate.

In addition, if you haven't explored this option already then client side validation in asp.net mvc might also provide for some solution with regards to your URL problem. I emphasize some since this approach won't work when javascript is disabled on the browser.

So, the best solution would be have an action method named Detail but accepting only HTTP POST verb.

Bikal Gurung
@Bikal Gurung - thanks . . i always thought this was a strange practice as the name of the action doesn't really represent what you are doing but i do agree that that would solve the issue i am asking about
ooo
you can fix that with [ActionName("Details")] public ActionResult WhatEverMethodName()Now that method will correspond to "/Details"
Carl Hörberg
"This solution is in line with ASP.NET MVC best practices..." - I would argue it's **not** in line with best practices, actually. Returning a view form a POST call, is directly in conflict with the PRG pattern, which is best practice for the entire web, not just ASP.NET MVC. There are lots of ways to comply with the PRG pattern in ASP.NET MVC, but the most common way is the already mentioned ModelState->TempData->ModelState approach. You can read more on PRG here: http://en.wikipedia.com/wiki/Post/Redirect/Get and a good implementation with attributes at http://tinyurl.com/39p54k8
Tomas Lycken
@Tomas - PRG pattern is not relevant here. The use case where PRG applies is to mostly prevent duplicate form submission. Quote from the same link you referred "...Post/Redirect/Get (PRG) is a common design pattern for web developers to help avoid certain duplicate form submissions ...". We are not looking to prevent duplicate form submission here just preserve the original URL. Even the problem PRG alludes to can be achieved simply by disabling 'Submit' button via javascript.Using ModelState->TempData->ModelState just to preserve original URL is not an intuitive solution and is an overkill
Bikal Gurung
@Bikal: But we wouldn't be using ModelState-TempData-ModelState *just* to preserve the original URL - preserving the original URL would be a welcome *side-effect of a more important concept*. And after all, taking well-tested, complete code from the internet and applying a couple of attributes isn't such a big cost for solving the problem at hand.
Tomas Lycken
@Bikal: On top of that, disabling the submit button still allows double-submitting on Enter, still allows double-submitting for users without javascript, and still doesn't solve the problem with the "To see the page again you must re-submit data" error message (http://blog.andreloker.de/post/2008/06/Post-Redirect-Get.aspx) that most browsers emmit on double-post. PRG solves a whole bunch of problems, and among them the problem the OP has asked for help with. *Not* implementing PRG is never good practice.
Tomas Lycken
@Tomas. I wouldn't use a sledge hammer to drive through nails. PRG addresses a use case where we can prevent duplicate form submissions in certain scenarios. Seems to me you are looking for a problem to solve a pattern use case rather than looking for a pattern to solve a problem. The big cost is not in "taking the code and putting it in our codebase" but in maintenance of that code. Surely, using PRG increases cost in terms of having to learn/understand the concept and maintain the codebase in the future. And for any software project, maintenance is more costly than initial development.
Bikal Gurung
@Bikal: Seriously, how expensive do you think maintenance of two small attributes, that both have full unit test coverage and that both perform small and comprehensible tasks depending on nothing but the MVC Framework itself, can be? Compared to maintenance of *any* other solution to this url problem, I assure you that it will not be more expensive. Your approach suggests naming the action method for saving data "Details" - what will the maintenance cost be for that?
Tomas Lycken
@Tomas. Read the question again. It asks not for naming convention but how to keep the original URL. And besides naming is always quite subjective and company specific. You might think sure enough if I want to edit I must name the action method 'Edit' but what if someone wants to amalgate 'Edit' and 'Details' together but the 'Details' concern overrides the 'Edit' concern since the users will mostly just want details but occasionally want an edit option. The latter use case is quite valid and thus the choice of name for action methods.
Bikal Gurung
@Bikal: In effect, what you're doing in your post is suggesting several anti-patterns to solve a simple problem. This problem can instead be solved in a way that - in a "by the way" kind of way - solves, or prevents, multiple other problems that might come up in the future, by implementing patterns that are recommended by many. You claim that using `TempData` in this way is bad practice - why? This is exactly the kind of work `TempData` exists for. I'm sorry, but your solution is one of the worst I've seen to this problem.
Tomas Lycken
@Tomas: You seem to be more guided by philosophical puritanism than applying a pragmatic solution to a simple problem. TempData has its uses as does PRG but not to this problem and no I have never said not to use PRG or tempdata ever. Use it but only when it justifies. Not indiscriminately. The way you are suggesting it seems you use "patterns" pretty much everywhere. Future this and future that is nothing more than speculation on your part. Like trying to predict the weather by putting your finger on the air. Maybe while applying patterns you might want to revisit a pattern called YAGNI.
Bikal Gurung
@Tomas: Maybe you could provide more original and better justification to why my approach is not as optimal as yours rather than just parotting "pattern".For me your approach is quite un-necessary. It adds code bloat, adds to maintenance cost and not a good use of PRG pattern. You could of course use it but it is nothing more than novelty and fanatical application of patterns not a justified and rational one.
Bikal Gurung
@Bikal: I think you need to read the question again. A short-version of the question is: "User posts with validation error. If I redirect, ModelState is lost. If I return View, URL is changed. What is best practice?" Since best practice in this case *is* to put ModelState in TempData, and since there is already an existing implementation of this solution, this is probably the easiest and best thing to do. The fact that this is in agreement with a pattern to avoid other problems is just a bonus; that means that the next question asked will not be how to get rid of the "Post again?" dialog.
Tomas Lycken
@Bikal: In response to your 2nd comment; First: why is this not a good use of the PRG pattern? It adresses the concerns of the OP, and solves the problem. Secondly: your approach suggests implementing several things that will be confusing in the future. Although it does preserve the original url, it also makes it confusing what that url does - and from the developer point of view, what action method does what. For example, if you collapse that controller to definitions in visual studio, you'll just have to *know* that the `Detail` action is where stuff is saved. The code won't tell you that.
Tomas Lycken
@Tomas: Quote from the wikipedia article you mentioned - "Post/Redirect/Get (PRG) is a common design pattern for web developers to help avoid certain duplicate form submissions and allow user agents to behave more intuitively with bookmarks and the refresh button.". Nowhere does it mention to use this pattern everywhere indiscriminately - ONLY when there is a requirement to prevent duplicate form submission. We do not have the problem of duplicate form submission here. There might be that need somewhere else but not here.
Bikal Gurung
@Tomas: My approach only suggests having another corresponding similarly named method name listening only to HTTP POST method. This avoids having to fiddle around with tempdata. This is my only assertion. Not this and that. With regards to naming the action methods you might want to put that across to the original author since he/she has better context and reasons for naming the action method as such. And as such I am not convinced yet application of PRG is needed here for reasons I mentioned already.
Bikal Gurung
+2  A: 

The problem here is actually caused by your implementation. This doesn't answer your question, but it describes where you've gone wrong in the first place.

If you want a page that is used to update or edit an item, the URL should reflect this. For example.

You visit http:\www.mysite.com\App\Detail and it displays some information about something. That is what the URL describes it is going to do. In your controller, the Detail() method would return the Detail.aspx view.

To edit an item, you would visit http:\www.mysite.com\App\Edit and change the information you wish to update, the form would post back to the same URL - you can handle this in the controller with these methods:

[HttpGet]
public ActionResult Edit() {
    MyModel model = new MyModel();
    ...
    return View(model);
}

[HttpPost]
public ActionResult Edit(MyModel model) {
    ...
    if (ModelState.IsValid) {
        // Save and redirect
        ...
        return RedirectToAction("Detail");
    }
    return View(model);
}

If you ever find yourself doing this...

return View("SomeView", model);

You are making your own life harder (as well as breaking the principles behind URLs).

If you want to re-use part of a view, make it a partial view and render it inside of the view that is named after the method on the controller.

I apologise that this potentially isn't very helpful, but you are falling into an MVC anti-pattern trap by displaying the same view from a differently named method.

Sohnee
+1  A: 

As @Malcolm sais, best practice is to put ModelState in TempData, but don't do it manually! If you'd do this manually in every controller action where it's relevant, you would introduce immense amounts of repeated code, and increase the maintenance cost vastly.

Instead, implement a couple of attributes that do the job for you. Kazi Manzur has an approach (scroll down to the end of the post) that has been widely spread, and Evan Nagle shows an implementation with tests that is essentially the same as Kazi's, but with different names. Since he also provides unit tests that make sure the attributes work, implementing them in your code will mean little or no maintenance cost. The only thing you'll have to keep track of is that the controller actions are decorated with the appropriate attributes, which can also be tested.

When you have the attributes in place, your controller might look something like this (I deliberately simplified, because I don't know the class you inherit from):

[HttpPost, PassState]
public ActionResult Update(EntityType entity)
{
    // Only update if the model is valid
    if (ModelState.IsValid) {
        base.Update(entity);
    }
    // Always redirect to Detail view.
    // Any errors will be passed along automagically, thanks to the attribute.
    return RedirectToAction("Detail", new { id = entity.Id });
}

[HttpGet, GetState]
public ActionResult Detail(int id)
{
    // Get stuff from the database and show the view
    // The model state, if there is any, will be imported by the attribute.
}

You mention that you feel putting ModelState in TempData feels like a "hack" - why? I agree with you that doing it with repeated code in every single controller action seems hacky, but that's not what we're doing here. In fact, this is exactly what TempData is for. And I don't think the above code looks hacky... do you?

Although there are solutions to this problem that might appear simpler, such as just renaming the action method to preserve the URL, I would strongly advise against that approach. It does solve this problem, but introduces a couple of others - for example, you'll still have no protection against double form submission, and you'll have pretty confusing action names (where a call to Detail actually changes stuff on the server).

Tomas Lycken