A spec for a filter

privacy_filter.rb

class PrivacyFilter
  def self.filter(controller)
    [:first_article?,
     :authenticate_administrator!,
     :authenticate_user!
    ].find do |m|
      controller.send(m)
    end
  end
end

privacy_filter_spec.rb

require "app/models/privacy_filter"

describe PrivacyFilter do

  let(:controller) do
    double(:first_article? => false,
           :authenticate_administrator! => false,
           :authenticate_user! => false)
  end

  it "allows access to the root article" do
    controller.should_receive(:first_article?).and_return(true)
    PrivacyFilter.filter(controller).should be_true
  end

  it "allows access for administrators" do
    controller.should_receive(:authenticate_administrator!).and_return(true)
    PrivacyFilter.filter(controller).should be_true
  end

  it "allows access to users" do
    controller.should_receive(:authenticate_user!).and_return(true)
    PrivacyFilter.filter(controller).should be_true
  end

  it "denies all others" do
    PrivacyFilter.filter(controller).should be_false
  end
end

But, yeah, this is kid of weird. I'm not a big fan of setting the instance variable there and calling the others. I would probably pass the controller to it.

I'm terribly torn between passing controller everywhere, which is ugly, and setting @vars in a class, which is ugly. Ugh. Effing Rails...

https://gist.github.com/1464967#file_privacy_filter.rb
I think this highlights how weird the code is. When you pass the controller around, the names of the methods start to be strange. Do you need those to be methods?

Yep. I dont think so. Check out the latest version.

As always, you start with something, you let it evolve, you end up somewhere...

Why not instantiate it?

PrivacyFilter.new(controller).filter

and set the controller instance var in intitialize?

It always seems strange to me to set state in a singleton object, unless I meant for that state to be used later on.. "stashing" the object to save a few keystrokes seems like it makes it a bit less clear what is happening. What about something like this:

class PrivacyFilter
  class << self 
    def filter(controller)
      new(controller).allow?
    end
  end

  def initialize(controller)
    @controller = controller
  end

  def allow?
    first_article? or administrator? or user?
  end

private

  def first_article?
    @controller.params[:id] == 1  # or Hash#fetch if params[:id] is always expected
  end

  def administrator?
    @controller.authenticate_administrator!
  end

  def user?
    @controller.authenticate_user!
  end

end

@svenfuchs Rails :/

before_filter PrivacyFilter

Hmm, now that I see this here it might be overkill ;) All well, just another pov to consider.

... what dkubb said :)

@dkubb I wouldn't even make it a singleton, if the Rails API wasn't that self.filter is how this stuff gets called. I ended up moving the first_article? method into the controller, so that I just straight-up delegate, which seems to be the right way to do it.

do the !bang methods raise errors?

also, is it completely unacceptable to use a one liner? there's not a lot of inherent complexity to what this filter does right now, so is there a great need to add cognitive complexity to the code?

@dennyabraham I don't think so. They're just standard Devise stuff. I think the bang is because they can redirect.

I'm torn between what's more readable, a one liner, or this.

Now that I'm thinking about it, that might mean that this won't even work, as a user will hit the admin filter, which will redirect to admin login...

I should really be using CanCan for this, I guess.

Late to the party, but I really want this to be AccessPolicy.has_access?(user, article). (Raptor will let you write exactly this and specify it directly in a route... some day. garybernhardt/raptor@3e1b48e ;)

Yeah, that seems like the right API. Rails makes it awkward.

For the app, I just ended up using cancan, which is the Right Way anywaay...