Zach Dennis wrote: > On Sat, Mar 15, 2008 at 5:43 PM, Ben Mabey <[EMAIL PROTECTED]> wrote: > >> Zach Dennis wrote: >> > Can someone change this implementation and still have your tests pass, >> > but have the implementation be broken? If they can then yes it is >> > worth the 40 lines. >> > >> > Pat mentioned that he uses very skinny controllers and does >> > integration tests which go through the whole stack to make sure things >> > work. So if he doesn't test the controller#action's because they are >> > too simple he at least will have a failing integration test if someone >> > breaks the implementation. >> > >> > The problem I have with what Pat is doing is that it takes a lot of >> > discipline from the developer to not let the controller grow into a >> > cesspool of logic and interaction that shouldn't be there, but you put >> > it there because you aren't "testing" it directly. From what I've seen >> > most Rails apps have important interaction in the controller actions >> > and logic in the before filters. I would not personally take this >> > approach on customer paid for software. Pat may be more disciplined >> > then me though. =) >> > >> > >> >> I totally agree with this point. Using interaction-based testing really >> helps in forcing the logic down into the model. >> >> I'm curious about your comment: "From what I've seen most Rails apps >> >> have important interaction in the controller actions >> and logic in the before filters". What are you exactly referring to? I >> generally use before filters for authentication... How have you seen >> them abused? >> > > I've seen before filters be abused. Authentication is typical an > application level responsibility and it makes sense for the controller > to protect its resources. No qualms there. I have inherited code bases > which abused before filters for things like checking permissions for > business requirements. For example: > > class ProjectsController < ApplicationController > before_filter :login_required # i'm ok with this > before_filter :ensure_project_manager, :only => [:new, :create] > before_filter :ensure_project_write_access, :only => [:edit, :update] > # ... I've seen this filter list go on and on and on, based on > any possible persmission >
Ahh, yes... I have seen this before. In fact people have told me that they don't test there controllers because of such permutations that take too long to test fully (because they have to test it on EVERY controller with those set of filters). Thats when I tell them that if things are too hard to test that is probably a good sign the are doing something wrong. :) In the past I have always pushed this in the model. I have ofter though just let the controller call one simple predicate method on the model in a filter. So perhaps I am guilty of abusing filters as well... > I would rather introduce a Manager/Service object which is responsible > for accessing a project, and allow that do handle the responsibility > for knowing what permissions allow someone to create or edit a > project. > > Makes sense- I really like that idea. Having a service handle all of this makes a lot of sense the more I think about it. As I have developed rails apps I always keep my controllers extremely thin but certain models become too large, IMO. I've actually been reading DDD and have been trying to think of ways of creating more service objects to remove the extra responsibility I have added in my models. Thanks for pointing this one out to me. > I've been experimenting with using an approach similar in spirit to > what Pat is doing, but slightly different: > > class ApplicationController < ActionController::Base > rescue_from AccessDenied, ResourceNotFoundError do |exception| > redirect_to "/access_denied.html" > end > # ... > end > > class ProjectsController < ApplicationController > before_filter :login_required > > def new > @project = ProjectManager.new_project > end > > def create > ProjectManager.create_project params[:project] do |project_creation| > project_creation.success do |project| > flash[:notice] = "Project created successfully" > @project = project > end > > project_creation.failure do |project| > flash[:notice] = "Project creation failed" > @project = project > render :action => "new" > end > end > # ... > end > > I tend to like this for projects I've worked on where it doesn't make > sense to shove all of the responsibility onto a model because it > abstracts out the "Manager" who is in charge of ensuring permissions > and doing odds and ends things involved in the creation of a project > that the model should not be doing (like upload an associated picture > of the project manager or ensure memberships status). > Very nice... I will definitely steal that slick use of blocks ;) My one question about this implementation is how does the ProjectManager get access to the session or at least gets the information about the current user? Without that information you can't have any permissions logic in the manager... > In the above approach the application controller is setup to rescue > from certain application exceptions, and the controllers just do their > thing, and the managers which enforce the business logic will throw > the exceptions it a business rule has been violated. > > I have used a similar approach in apps in the past (by throwing an AccessDenied exception), but I have somewhat gone away from that. My hesitation was caused by me wondering if a violation of these permissions really deems an exception. Is having a user trying to get into an area where they don't have the needed rights *exceptional*? So seeing you use this same pattern makes me think that it probably is a good use of exceptions. Having the logic in a manger I think the only way to handle incorrect permissions cleanly would be an exception so I guess the argument is somehwat moot in this context. Thank so much for taking the time to answer my question. I always learn a great deal from this list so thank you for contributing so much to it. -Ben _______________________________________________ rspec-users mailing list rspec-users@rubyforge.org http://rubyforge.org/mailman/listinfo/rspec-users
