asp.net mvc post request + service layer- the best way to do this

Question

Lets say I wanted to make a post request to update the status of a house, ideally this data should be in some sort of service layer, typically this involves

1. validate user - are they still active or been kicked out by admin?
2. check the houseid - is the houseid/ record valid?
3. can the user see the house details?
4. update status to "open" or "closed"

In a real world/ complex domain - most views are very complex, we have to throw out maybe the number of houses in the area, how many comments on the house, the house details so on, maybe the number of outstanding tasks on the house...

In short - all the above code may be inside a service layer, however lets say an exception is thrown, a user cannot update the status on the house - now to populate the view you have to firstly get the house details (again), load up all other stuff that you just loaded up inside the service layer ALL of this inside the controller or another incovation to a service layer that loads up this data...

How can I ensure my domain model is protected by running validation and all the sorts without having to rewrite the same code multiple times...

This code is inside the action method, it could easily be inside a service layer...

//NOTE: _repo is a simple abstraction over linq to sql...

    [HttpGet]
    public ActionResult TaskDetail(int houseid, int taskid)
    {
        var loggedonuser = _repo.GetCurrentUser();

        var _house = _repo.Single<House>(x => x.HouseID == houseid && x.Handler == loggedonuser.CompanyID);

        if (_house == null)
            throw new NoAccessException();

        var summary = _house.ToSummaryDTO();

        var companies = _repo.All<Company>();
        var users = _repo.All<User>();

        var task = _repo.Single<HouseTask>
            (x => x.HouseID == _house.HouseID && x.TaskID == taskid && (x.CompanyID == loggedonuser.CompanyID));

        var dto = new TaskDTO
        {
            TaskID = task.TaskID,
            Title = task.Title,
            Description = task.Description,
            DateCreated = task.DateCreated,
            IsClosed = task.IsClosed,
            CompanyID = companies.Where(y => task.CompanyID == y.CompanyID).SingleOrDefault().Identifier,
        };

        if (task.DueDate.HasValue)
            dto.DueDate = task.DueDate.Value;

        var comments = _repo.All<HouseTaskComment>()
            .Where(x => x.TaskID == task.TaskID)
            .OrderByDescending(x => x.Timestamp)
            .Select(x => new TaskCommentDTO
            {
                Comment = x.Comment,
                Timestamp = x.Timestamp,
                CompanyID = companies.Where(y => x.CompanyID == y.CompanyID).SingleOrDefault().Identifier,
                UserID = users.Where(y => x.UserID == y.UserID).SingleOrDefault().Login,
                Type = EnumHelper.Convert<TaskCommentType>(x.Type)
            });

        dto.AllComments = comments;

        return View(new TaskViewModel
        {
            Summary = summary,
            TaskDetail = dto,
            NewComment = new TaskCommentDTO()
        });
    }

In short - get the house details for the summary, get the task detail (from the multiple available tasks) and also get the task comments. That is a simple view IMHO, nothing too complex.

At this point user can: Add comment, Close/ Open task - if they have permissions to do so (code was omitted for simplicity), set task due date, or even clear the due date for the task.

Now the UpdateTaskStatus - if it is not possible to update the status must return the above view, similar to comment, if you cant comment please return the detail view - comments may be closed.

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult TaskDueDate(int houseid, int taskid)
{

    var duedate = Request.Form["duedate"];
    var duetime = Request.Form["duetime"];
    try
    {
        if (ModelState.IsValid)
        {

            var newduedate = DateHelper.GoodDate(duedate, duetime);
            _service.SetTaskDueDate(houseid, taskid, newduedate);

            return RedirectToAction("TaskDetail");
        }
    }
    catch (RulesException ex)
    {
        ex.CopyTo(ModelState);
    }

    var loggedonuser = _repo.GetCurrentUser();

    var _house = _repo.Single<House>(x => x.InstructionID == houseid && x.HandlerID == loggedonuser.CompanyID);

    if (_house == null)
        throw new NoAccessException();

    var summary = _house.ToSummaryDTO();

    var companies = _repo.All<Company>();
    var users = _repo.All<User>();

    var task = _repo.Single<HouseTask>
        (x => x.InstructionID == _house.HouseID && x.CompanyID == loggedonuser.CompanyID && x.TaskID == taskid);

    var dto = new TaskDTO
    {
        TaskID = task.TaskID,
        Title = task.Title,
        Description = task.Description,
        DateCreated = task.DateCreated,
        IsClosed = task.IsClosed,
        CompanyID = companies.Where(y => task.CompanyID == y.CompanyID).SingleOrDefault().Identifier
    };

    if (task.DueDate.HasValue)
        dto.DueDate = task.DueDate.Value;

    var comments = _repo.All<HouseTaskComment>()
        .Where(x => x.TaskID == task.TaskID)
        .OrderByDescending(x => x.Timestamp)
        .Select(x => new TaskCommentDTO
        {
            Comment = x.Comment,
            Timestamp = x.Timestamp,
            CompanyID = companies.Where(y => x.CompanyID == y.CompanyID).SingleOrDefault().Identifier,
            UserID = users.Where(y => x.UserID == y.UserID).SingleOrDefault().Login
        });

    dto.AllComments = comments;

    return View("TaskDetail", new TaskViewModel
    {
        Summary = summary,
        TaskDetail = dto,
        NewComment = new TaskCommentDTO()
    });
}

I know the code above is badly structured but some advice on how to rectify it will be appreciated.

1. I am leaving all read only code inside the actions, because each view could be different, I dont want a service layer interfering here
2. I want to "protect" my updates/ edits and hold this inside my service layer or core project (sepeparate c# class lib) or even Domain layer, how would I write that code- handle validation, (which is what I do inside the service call), perform the actual save?

I have heard of CommandHandler approach, is this a good approach? Ideally I want to hold my validating + persistance using a simple approach inside my domain not controller action....

Answer 1

Maybe you're over-thinking this a bit

As I understand your process this is the way it should be done

1. User validation (whether it's still a valid user) is done on user login and not in process later because unregistered (or kicked out users) should not be able to access your app. In case your users can get kicked out in the middle of their work you can solve this by creating an action filter and put it on either:

   • all your controller classes (not actions)

   • have a base controller class and put filter on it

2. Your steps 2,3 and 4 are actually related because

   • first of all you must get house data (including permission)

   • if house ID is invalid you won't get anything back from DB

   • if you did get data back then check permissions

   • set status accordingly when allowed

An addition note

With complex domain models it's usually not necessary to have complex views. You probably just have more of them and have a more complex navigation to aid usability. Users would also be happy(er) to work with simple views than complex ones.

Model validation

I would keep static validation as Asp.net MVC implements - using data annotations on your POCO objects layer - because it's build in and would cut down on your code (less lines less = smaller bug surface) and it will make it more robust because you won't accidentally forget to validate some stuff.

But your dynamic validation (related to particular user/company permission over an entity instance) would best be kept inside your service layer. If any violation occurs I would throw custom exceptions and propagate them into model state (similar to what you're doing - based on my understanding of your code - with RulesException class).

To avoid tedious (and repetitious code) of try/catch code blocks you can just write a custom exception filter that would propagate values to model state and return whatever view is required to display model state errors. Implementation of such exception filter is completely on you. Then just put it on your parent controller (since you're not using MVC 3 which has global filters) and forget about it. This will also cut down repetitious code.

If you put dynamic model validation into service layer you will have to pass user required details to it because it relies on them.

User fetching

I can see you're reading user data from data store each time, which is not desired. Most of the time you probably just need user ID (and obviously also company ID). This small bit of data could be persisted in a different way (usually in a secure cookie but you may decide on your own strategy) so you save some time. Invalidating this cached data is rather trivial because a user probably is only able to change their own data so you can invalidate at that point and reread. You can bundle it with additional data that you frequently need (maybe user display name or login name). But that it. Most of the time your code will use user ID.

Model claim/permission validation

Your model is not just validating whether model entity data is correct, but you need to validate claims (or permissions) that is whether a particular user has change entity permission (depending on their company). You said you can't put it at filter level, but as much as I can see your problem you can. If you persist your user data (as stated previously), you have all the info you need. You have user data, you have your model and based on the action you also know static claim related to that action.

[RequirePermission(Permission.UpdateTaskDueDate)]
public ActionResult TaskDueDate(...) { ... }

if action filter can't be done that simple, you can also provide it a custom validator class that implements a certain interface (so filter can call any) and parameter name that should be validated. This will make it possible to validate custom classes against user/company data. It would also allow you to validate more than just one permission or even same/different permissions using different validators.

[RequirePermission(Permission.UpdateTaskDueDate, typeof(TaskValidator), "paramName")]
public ActionResult TaskDueDate(...) { ... }

This validator's Validate method would take permission enumeration value and validate what's required of it.

There's also another possibility. You can have a custom attribute (or more of them) on your entity classes that define their custom validator(s), so filter would still only take permission type that should be validated. This will also make it easy to validate multiple objects on a single action. Just provide permission and filter would check action parameters, their types and declared custom validators.

Having custom validators that your filter barely calls also keeps your validation on service layer and not in presentation. So it's up to your domain logic how those are being validated which keep UI independent of the underlying code. In case anything would change only your input and output types should match but all UI layer code should practically stay as it is.

Reading request data

Instead of reading your data using Request.Form["duedate"] you should rather just put those as action parameters. They'll get populated by MVC for you. Because in your code:

try
{
    if (ModelState.IsValid)
    {

        var newduedate = DateHelper.GoodDate(duedate, duetime);
        _service.SetTaskDueDate(houseid, taskid, newduedate);

        return RedirectToAction("TaskDetail");
    }
}
catch (RulesException ex)
{
    ex.CopyTo(ModelState);
}

if statement is completely redundant. Your integer parameters will always be valid (they're not set as nullable) within action body. Whether your DateHelper.GoodDate checks the other couple statically, they can better be contained in a custom class (with TaskId as well) and you can put data annotation on it. And model validation would actually have the ability to be invalid (so if statement will make sense).

But instead of repeating the same code in your TaskDueDate action you could just have it done this way:

[HttpPost]
[ValidateAntiForgeryToken]
[RequirePermission(Permission.UpdateTaskDueDate)]
public ActionResult TaskDueDate(int houseid, TaskDue taskDueData)
{
    if (!this.ModelState.IsValid)
    {
        // don't repeat code and just call another action within this controller
        return this.TaskDetail(houseid, taskDueData.TaskId);
    }
    _service.SetTaskDueDate(houseid, taskid, newduedate);
    return RedirectToAction("TaskDetail");
}

Because you're doing the same thing in both actions. Nobody said you can't call other actions within actions. They all return ActionResult anyway.

I think we both agree that this action is much more simplified.

Action method code

I wouldn't put all those _repo calls inside a controller action because that's service domain code. All you'd have to do is provide parameters and call into service layer to provide your objects that views consume.

Epilogue

I hope I answered at least some of your questions/problems/issues because it's quite hard for me to decipher what the actual problem is (or better said what exactly are you asking). You've written your question in a rather confusing way hence not getting answeres on your question. People don't get what your problem is.

Anyway. I tried to refactor your code and explain how I'd do it if I had to implement dynamic validation.

Answer 2

Can you inherit from the standard FaultException and add a HouseDetails property to if of type HouseDTO?