From 24d0c46080475176f0f0ebfb9bf7ccc0376a96f3 Mon Sep 17 00:00:00 2001
From: mio <georg@mittendorfer.com>
Date: Mon, 7 Nov 2011 20:39:14 +0100
Subject: [PATCH 1/5] added ~ files to gitignore

---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 31fda99..d4eb65e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,3 +2,4 @@ Gemfile.lock
 /test_app/db/migrate/20*
 /test_app/db/schema.rb
 /pkg
+*~

From 09bfa2f27eff55833d783f1705f189b8cd2fd880 Mon Sep 17 00:00:00 2001
From: mio <georg@mittendorfer.com>
Date: Mon, 7 Nov 2011 20:41:51 +0100
Subject: [PATCH 2/5] implemented first version for unique check on controller
 level.

---
 app/controllers/impressionist_controller.rb   |  82 +++++---
 test_app/Gemfile                              |   4 +-
 test_app/Gemfile.lock                         | 186 +++++++++---------
 .../app/controllers/widgets_controller.rb     |   3 +-
 test_app/config/database.yml                  |  25 +--
 test_app/spec/controllers/controller_spec.rb  |  22 +++
 .../unique_impression_controller_spec.rb      | 132 +++++++++++++
 .../rails_generators/rails_generators_spec.rb |   2 +
 test_app/spec/spec_helper.rb                  |   6 +
 9 files changed, 327 insertions(+), 135 deletions(-)
 create mode 100644 test_app/spec/controllers/unique_impression_controller_spec.rb

diff --git a/app/controllers/impressionist_controller.rb b/app/controllers/impressionist_controller.rb
index c7be098..4a13a8c 100644
--- a/app/controllers/impressionist_controller.rb
+++ b/app/controllers/impressionist_controller.rb
@@ -3,10 +3,10 @@ require 'digest/sha2'
 module ImpressionistController
   module ClassMethods
     def impressionist(opts={})
-      before_filter { |c| c.impressionist_subapp_filter opts[:actions] }
+      before_filter { |c| c.impressionist_subapp_filter(opts[:actions], opts[:unique])}
     end
   end
-  
+
   module InstanceMethods
     def self.included(base)
       base.before_filter :impressionist_app_filter
@@ -15,14 +15,7 @@ module ImpressionistController
     def impressionist(obj,message=nil)
       unless bypass
         if obj.respond_to?("impressionable?")
-          obj.impressions.create(:message=> message,
-                                 :request_hash=> @impressionist_hash,
-                                 :session_hash=> request.session_options[:id],
-                                 :ip_address=> request.remote_ip,
-                                 :user_id=> user_id,
-                                 :controller_name=>controller_name,
-                                 :action_name=> action_name,
-                                 :referrer=>request.referer)
+          obj.impressions.create(create_statement({:message => message}))
         else
           raise "#{obj.class.to_s} is not impressionable!"
         end
@@ -33,31 +26,76 @@ module ImpressionistController
       @impressionist_hash = Digest::SHA2.hexdigest(Time.now.to_f.to_s+rand(10000).to_s)
     end
 
-    def impressionist_subapp_filter(actions=nil)
+    def impressionist_subapp_filter(actions=nil, unique_opts=nil)
       unless bypass
         actions.collect!{|a|a.to_s} unless actions.blank?
-        if actions.blank? or actions.include?(action_name)
-          Impression.create(:controller_name=> controller_name,
-                            :action_name=> action_name,
-                            :user_id=> user_id,
-                            :request_hash=> @impressionist_hash,
-                            :session_hash=> request.session_options[:id],
-                            :ip_address=> request.remote_ip,
-                            :impressionable_type=> controller_name.singularize.camelize,
-                            :impressionable_id=> params[:id],
-                            :referrer=>request.referer)
+        if (actions.blank? || actions.include?(action_name)) && (unique_opts.blank? || is_unique(unique_opts))
+          if (!actions.blank? && !unique_opts.blank?)
+            logger.info "Restricted to actions #{actions.inspect} and uniqueness for #{unique_opts.inspect}"
+          end
+          Impression.create(create_statement(
+            :impressionable_type => controller_name.singularize.camelize,
+            :impressionable_id=> params[:id]
+          ))
         end
       end
     end
 
     private
+    
     def bypass
       Impressionist::Bots::WILD_CARDS.each do |wild_card|
         return true if request.user_agent and request.user_agent.downcase.include? wild_card
       end
       Impressionist::Bots::LIST.include? request.user_agent
     end
-
+    
+    def is_unique(unique_opts)
+      # FIXME think about uniqueness in relation to impressionable_id, impressionable_type and controller_name
+      # is controller name redundant? does the controller name always have to match?
+      default_statement = create_statement(
+        :impressionable_type => controller_name.singularize.camelize,
+        :impressionable_id=> params[:id]
+        )
+      statement = unique_opts.reduce({}) do |query, param|
+        query[param] = default_statement[param]
+        query
+      end
+      #logger.debug "Statement params: #{statement.inspect}."
+      # always use impressionable type?
+      statement[:impressionable_type] = controller_name.singularize.camelize
+      #statement[:impressionable_id] = params[:id]
+      return Impression.where(statement).size == 0
+    end
+    
+    # creates a statment hash that contains default values for creating an impression (without
+    # :impressionable_type and impressionable_id as they are not needed for creating via association).
+    def create_statement(query_params={})
+      query_params.reverse_merge!(
+        :controller_name => controller_name,
+        :action_name => action_name,
+        :user_id => user_id,
+        :request_hash => @impressionist_hash,
+        :session_hash => session_hash,
+        :ip_address => remote_ip,
+        :referrer => request.referer
+      )
+    end
+    
+    def session_hash
+      # # careful: request.session_options[:id] encoding in rspec test was ASCII-8BIT
+      # # that broke the database query for uniqueness. not sure how to solve this issue
+      # # seems to depend on app setup/config
+      # str = request.session_options[:id]
+      # # probably this isn't a fix: request.session_options[:id].encode("ISO-8859-1")
+      # logger.debug "Encoding: #{str.encoding.inspect}"
+      request.session_options[:id]
+    end
+    
+    def remote_ip
+      request.remote_ip
+    end
+    
     #use both @current_user and current_user helper
     def user_id
       user_id = @current_user ? @current_user.id : nil rescue nil
diff --git a/test_app/Gemfile b/test_app/Gemfile
index cfc3afe..9bdd5db 100644
--- a/test_app/Gemfile
+++ b/test_app/Gemfile
@@ -1,9 +1,9 @@
 source 'http://rubygems.org'
 
-gem 'rails', '3.1.0.rc1'
+gem 'rails', '3.1'
 gem 'sqlite3-ruby', :require => 'sqlite3'
 gem 'impressionist', :path=>"#{File.dirname(__FILE__)}/../"
-gem "pg"
+#gem "pg"
 
 group :development do
   gem 'ZenTest'
diff --git a/test_app/Gemfile.lock b/test_app/Gemfile.lock
index b5676b2..71c9c22 100644
--- a/test_app/Gemfile.lock
+++ b/test_app/Gemfile.lock
@@ -1,155 +1,156 @@
 PATH
-  remote: /rails_plugins/mine/impressionist
+  remote: /home/mio/prog/projects/impressionist
   specs:
-    impressionist (0.3.2)
+    impressionist (0.4.0)
 
 GEM
   remote: http://rubygems.org/
   specs:
-    ZenTest (4.5.0)
-    actionmailer (3.1.0.rc1)
-      actionpack (= 3.1.0.rc1)
+    ZenTest (4.6.2)
+    actionmailer (3.1.0)
+      actionpack (= 3.1.0)
       mail (~> 2.3.0)
-    actionpack (3.1.0.rc1)
-      activemodel (= 3.1.0.rc1)
-      activesupport (= 3.1.0.rc1)
+    actionpack (3.1.0)
+      activemodel (= 3.1.0)
+      activesupport (= 3.1.0)
       builder (~> 3.0.0)
       erubis (~> 2.7.0)
-      i18n (~> 0.6.0beta1)
-      rack (~> 1.3.0.beta2)
-      rack-cache (~> 1.0.1)
-      rack-mount (~> 0.8.1)
-      rack-test (~> 0.6.0)
-      sprockets (~> 2.0.0.beta.5)
-      tzinfo (~> 0.3.27)
-    activemodel (3.1.0.rc1)
-      activesupport (= 3.1.0.rc1)
-      bcrypt-ruby (~> 2.1.4)
+      i18n (~> 0.6)
+      rack (~> 1.3.2)
+      rack-cache (~> 1.0.3)
+      rack-mount (~> 0.8.2)
+      rack-test (~> 0.6.1)
+      sprockets (~> 2.0.0)
+    activemodel (3.1.0)
+      activesupport (= 3.1.0)
+      bcrypt-ruby (~> 3.0.0)
       builder (~> 3.0.0)
-      i18n (~> 0.6.0beta1)
-    activerecord (3.1.0.rc1)
-      activemodel (= 3.1.0.rc1)
-      activesupport (= 3.1.0.rc1)
-      arel (~> 2.1.1)
-      tzinfo (~> 0.3.27)
-    activeresource (3.1.0.rc1)
-      activemodel (= 3.1.0.rc1)
-      activesupport (= 3.1.0.rc1)
-    activesupport (3.1.0.rc1)
+      i18n (~> 0.6)
+    activerecord (3.1.0)
+      activemodel (= 3.1.0)
+      activesupport (= 3.1.0)
+      arel (~> 2.2.1)
+      tzinfo (~> 0.3.29)
+    activeresource (3.1.0)
+      activemodel (= 3.1.0)
+      activesupport (= 3.1.0)
+    activesupport (3.1.0)
       multi_json (~> 1.0)
-    arel (2.1.1)
+    addressable (2.2.6)
+    arel (2.2.1)
     autotest (4.4.6)
       ZenTest (>= 4.4.1)
-    autotest-notification (2.3.1)
-      autotest (~> 4.3)
-    bcrypt-ruby (2.1.4)
+    autotest-notification (2.3.3)
+      autotest-standalone (~> 4.5)
+    autotest-standalone (4.5.8)
+    bcrypt-ruby (3.0.1)
     builder (3.0.0)
-    capybara (1.0.0.beta1)
+    capybara (1.1.1)
       mime-types (>= 1.16)
       nokogiri (>= 1.3.3)
       rack (>= 1.0.0)
       rack-test (>= 0.5.4)
-      selenium-webdriver (>= 0.0.27)
+      selenium-webdriver (~> 2.0)
       xpath (~> 0.1.4)
-    childprocess (0.1.9)
+    childprocess (0.2.2)
       ffi (~> 1.0.6)
-    configuration (1.2.0)
-    cucumber (0.10.3)
+    cucumber (1.1.1)
       builder (>= 2.1.2)
       diff-lcs (>= 1.1.2)
-      gherkin (>= 2.3.8)
+      gherkin (~> 2.6.0)
       json (>= 1.4.6)
-      term-ansicolor (>= 1.0.5)
-    cucumber-rails (0.5.1)
-      capybara (>= 1.0.0.beta1)
-      cucumber (>= 0.10.3)
-      nokogiri (>= 1.4.4)
-      rack-test (>= 0.5.7)
+      term-ansicolor (>= 1.0.6)
+    cucumber-rails (1.2.0)
+      capybara (>= 1.1.1)
+      cucumber (>= 1.1.1)
+      nokogiri (>= 1.5.0)
     daemons (1.0.10)
     database_cleaner (0.6.7)
-    diff-lcs (1.1.2)
+    diff-lcs (1.1.3)
     erubis (2.7.0)
     ffi (1.0.9)
     gem_plugin (0.2.3)
-    gherkin (2.3.10)
+    gherkin (2.6.2)
       json (>= 1.4.6)
-    hike (1.0.0)
+    hike (1.2.1)
     i18n (0.6.0)
-    json (1.5.1)
-    json_pure (1.5.1)
-    launchy (0.4.0)
-      configuration (>= 0.0.5)
-      rake (>= 0.8.1)
+    json (1.6.1)
+    json_pure (1.6.1)
+    launchy (2.0.5)
+      addressable (~> 2.2.6)
     mail (2.3.0)
       i18n (>= 0.4.0)
       mime-types (~> 1.16)
       treetop (~> 1.4.8)
-    mime-types (1.16)
+    mime-types (1.17.2)
     mongrel (1.2.0.pre2)
       daemons (~> 1.0.10)
       gem_plugin (~> 0.2.3)
     multi_json (1.0.3)
-    nokogiri (1.4.4)
-    pg (0.11.0)
-    polyglot (0.3.1)
-    rack (1.3.0)
-    rack-cache (1.0.2)
+    nokogiri (1.5.0)
+    polyglot (0.3.3)
+    rack (1.3.5)
+    rack-cache (1.0.3)
       rack (>= 0.4)
-    rack-mount (0.8.1)
+    rack-mount (0.8.3)
       rack (>= 1.0.0)
     rack-ssl (1.3.2)
       rack
-    rack-test (0.6.0)
+    rack-test (0.6.1)
       rack (>= 1.0)
-    rails (3.1.0.rc1)
-      actionmailer (= 3.1.0.rc1)
-      actionpack (= 3.1.0.rc1)
-      activerecord (= 3.1.0.rc1)
-      activeresource (= 3.1.0.rc1)
-      activesupport (= 3.1.0.rc1)
+    rails (3.1.0)
+      actionmailer (= 3.1.0)
+      actionpack (= 3.1.0)
+      activerecord (= 3.1.0)
+      activeresource (= 3.1.0)
+      activesupport (= 3.1.0)
       bundler (~> 1.0)
-      railties (= 3.1.0.rc1)
-    railties (3.1.0.rc1)
-      actionpack (= 3.1.0.rc1)
-      activesupport (= 3.1.0.rc1)
+      railties (= 3.1.0)
+    railties (3.1.0)
+      actionpack (= 3.1.0)
+      activesupport (= 3.1.0)
       rack-ssl (~> 1.3.2)
       rake (>= 0.8.7)
+      rdoc (~> 3.4)
       thor (~> 0.14.6)
-    rake (0.9.1)
-    rspec (2.6.0)
-      rspec-core (~> 2.6.0)
-      rspec-expectations (~> 2.6.0)
-      rspec-mocks (~> 2.6.0)
-    rspec-core (2.6.3)
-    rspec-expectations (2.6.0)
+    rake (0.9.2.2)
+    rdoc (3.11)
+      json (~> 1.4)
+    rspec (2.7.0)
+      rspec-core (~> 2.7.0)
+      rspec-expectations (~> 2.7.0)
+      rspec-mocks (~> 2.7.0)
+    rspec-core (2.7.1)
+    rspec-expectations (2.7.0)
       diff-lcs (~> 1.1.2)
-    rspec-mocks (2.6.0)
-    rspec-rails (2.6.1)
+    rspec-mocks (2.7.0)
+    rspec-rails (2.7.0)
       actionpack (~> 3.0)
       activesupport (~> 3.0)
       railties (~> 3.0)
-      rspec (~> 2.6.0)
+      rspec (~> 2.7.0)
     rubyzip (0.9.4)
-    selenium-webdriver (0.2.1)
-      childprocess (>= 0.1.7)
-      ffi (>= 1.0.7)
+    selenium-webdriver (2.10.0)
+      childprocess (>= 0.2.1)
+      ffi (= 1.0.9)
       json_pure
       rubyzip
     spork (0.8.5)
-    sprockets (2.0.0.beta.9)
-      hike (~> 1.0)
+    sprockets (2.0.3)
+      hike (~> 1.2)
       rack (~> 1.0)
       tilt (~> 1.1, != 1.3.0)
-    sqlite3 (1.3.3)
+    sqlite3 (1.3.4)
     sqlite3-ruby (1.3.3)
       sqlite3 (>= 1.3.3)
-    systemu (2.2.0)
-    term-ansicolor (1.0.5)
+    systemu (2.4.1)
+    term-ansicolor (1.0.7)
     thor (0.14.6)
-    tilt (1.3.2)
-    treetop (1.4.9)
+    tilt (1.3.3)
+    treetop (1.4.10)
+      polyglot
       polyglot (>= 0.3.1)
-    tzinfo (0.3.27)
+    tzinfo (0.3.31)
     xpath (0.1.4)
       nokogiri (~> 1.3)
 
@@ -167,8 +168,7 @@ DEPENDENCIES
   impressionist!
   launchy
   mongrel (= 1.2.0.pre2)
-  pg
-  rails (= 3.1.0.rc1)
+  rails (= 3.1)
   rspec
   rspec-rails
   spork
diff --git a/test_app/app/controllers/widgets_controller.rb b/test_app/app/controllers/widgets_controller.rb
index 880ba5a..83c1261 100644
--- a/test_app/app/controllers/widgets_controller.rb
+++ b/test_app/app/controllers/widgets_controller.rb
@@ -1,5 +1,5 @@
 class WidgetsController < ApplicationController
-  impressionist :actions=>[:show,:index]
+  impressionist :actions=>[:show,:index], :unique => [:action_name, :impressionable_id]
   def show
   end
   
@@ -8,4 +8,5 @@ class WidgetsController < ApplicationController
   
   def new
   end
+  
 end
\ No newline at end of file
diff --git a/test_app/config/database.yml b/test_app/config/database.yml
index bccd3e9..077168a 100644
--- a/test_app/config/database.yml
+++ b/test_app/config/database.yml
@@ -6,28 +6,19 @@ development:
   pool: 5
   timeout: 5000
 
-# Warning: The database defined as "test" will be erased and
-# re-generated from your development database when you run "rake".
-# Do not set this db to the same as development or production.
-#test: &test
-#  adapter: sqlite3
-#  database: db/test.sqlite3
-#  pool: 5
-#  timeout: 5000
-
-test:
+test: &test
   adapter: sqlite3
   database: db/test.sqlite3
   pool: 5
   timeout: 5000
 
-pg_test:
-  adapter: postgresql
-  database: impressionist_test
-  username: johnmcaliley
-  password: 
-  host: localhost
-  encoding: UTF8
+#pg_test:
+#  adapter: postgresql
+#  database: impressionist_test
+#  username: johnmcaliley
+# password: 
+#  host: localhost
+#  encoding: UTF8
 
 production:
   adapter: sqlite3
diff --git a/test_app/spec/controllers/controller_spec.rb b/test_app/spec/controllers/controller_spec.rb
index c303671..da9f32c 100644
--- a/test_app/spec/controllers/controller_spec.rb
+++ b/test_app/spec/controllers/controller_spec.rb
@@ -78,6 +78,28 @@ describe WidgetsController do
     Impression.all.size.should eq 13
   end
   
+  it "should log unique impressions at the per action" do
+    get "show", :id=> 1
+    Impression.all.size.should eq 12
+    get "show", :id=> 2
+    Impression.all.size.should eq 13
+    get "show", :id => 2
+    Impression.all.size.should eq 13
+    get "index"
+    Impression.all.size.should eq 14
+  end
+  
+  it "should log unique impressions only once per id" do
+    get "show", :id=> 1
+    Impression.all.size.should eq 12
+    get "show", :id=> 2
+    Impression.all.size.should eq 13
+    get "show", :id => 2
+    Impression.all.size.should eq 13
+    get "index"
+    Impression.all.size.should eq 14
+  end
+  
   it "should not log impression when user-agent is in wildcard list" do
     request.stub!(:user_agent).and_return('somebot')
     get "show", :id=> 1
diff --git a/test_app/spec/controllers/unique_impression_controller_spec.rb b/test_app/spec/controllers/unique_impression_controller_spec.rb
new file mode 100644
index 0000000..f4abde6
--- /dev/null
+++ b/test_app/spec/controllers/unique_impression_controller_spec.rb
@@ -0,0 +1,132 @@
+require 'spec_helper.rb'
+
+# we use the posts controller as it uses the impressionsist module. any such controller would do.
+describe PostsController do
+    
+  before do
+    @impression_count = Impression.all.size
+  end
+  
+  it "should ignore uniqueness if not requested" do
+    controller.impressionist_subapp_filter(nil, nil)
+    controller.impressionist_subapp_filter(nil, nil)
+    Impression.should have(@impression_count + 2).records
+  end
+  
+  it "should recognize session uniqueness" do
+    # the following line was necessary as session hash returned a binary string (ASCII-8BIT encoded)
+    # not sure how to 'fix' this. setup/config issue?
+    controller.stub!(:session_hash).and_return(request.session_options[:id].encode("ISO-8859-1"))
+    controller.impressionist_subapp_filter(nil, [:session_hash])
+    controller.impressionist_subapp_filter(nil, [:session_hash])
+    Impression.should have(@impression_count + 1).records
+  end
+  
+  it "should recognize ip uniqueness" do
+    controller.stub!(:action_name).and_return("test_action")
+    controller.impressionist_subapp_filter(nil, [:ip_address])
+    controller.impressionist_subapp_filter(nil, [:ip_address])
+    Impression.should have(@impression_count + 1).records
+  end
+  
+  it "should recognize request uniqueness" do
+    controller.impressionist_subapp_filter(nil, [:request_hash])
+    controller.impressionist_subapp_filter(nil, [:request_hash])
+    Impression.should have(@impression_count + 1).records
+  end
+  
+  it "should recognize action uniqueness" do
+    controller.stub!(:action_name).and_return("test_action")
+    controller.impressionist_subapp_filter(nil, [:action_name])
+    controller.impressionist_subapp_filter(nil, [:action_name])
+    Impression.should have(@impression_count + 1).records
+  end
+  
+  it "should recognize controller uniqueness" do
+    controller.stub!(:controller_name).and_return("test_controller")
+    controller.impressionist_subapp_filter(nil, [:controller_name])
+    controller.impressionist_subapp_filter(nil, [:controller_name])
+    Impression.should have(@impression_count + 1).records
+  end
+  
+   it "should recognize user uniqueness" do
+    controller.stub!(:user_id).and_return(1)
+    controller.impressionist_subapp_filter(nil, [:user_id])
+    controller.impressionist_subapp_filter(nil, [:user_id])
+    Impression.should have(@impression_count + 1).records
+  end
+  
+  it "should recognize referrer uniqueness" do
+    controller.stub!(:referrer).and_return("http://somehost.someurl.somdomain/some/path")
+    controller.impressionist_subapp_filter(nil, [:referrer])
+    controller.impressionist_subapp_filter(nil, [:referrer])
+    Impression.should have(@impression_count + 1).records
+  end
+  
+  # extra redundant test for important controller and action combination.
+  it "should recognize difference in controller and action" do
+    controller.stub!(:controller_name).and_return("test_controller")
+    controller.stub!(:action_name).and_return("test_action")
+    controller.impressionist_subapp_filter(nil, [:controller_name, :action_name])
+    Impression.should have(@impression_count + 1).records
+    controller.stub!(:action_name).and_return("another_action")
+    controller.impressionist_subapp_filter(nil, [:controller_name, :action_name])
+    Impression.should have(@impression_count + 2).records
+    controller.stub!(:controller_name).and_return("another_controller")
+    controller.impressionist_subapp_filter(nil, [:controller_name, :action_name])
+    Impression.should have(@impression_count + 3).records
+  end
+  
+  it "should recognize difference in action" do
+    controller.stub!(:action_name).and_return("test_action")
+    controller.impressionist_subapp_filter(nil, [:action_name])
+    Impression.should have(@impression_count + 1).records
+    controller.stub!(:action_name).and_return("another_action")
+    controller.impressionist_subapp_filter(nil, [:action_name])
+    Impression.should have(@impression_count + 2).records
+  end
+  
+  it "should recognize difference in controller" do
+    controller.stub!(:controller_name).and_return("test_controller")
+    controller.impressionist_subapp_filter(nil, [:controller_name])
+    Impression.should have(@impression_count + 1).records
+    controller.stub!(:controller_name).and_return("another_controller")
+    controller.impressionist_subapp_filter(nil, [:controller_name])
+    Impression.should have(@impression_count + 2).records
+  end
+  
+  it "should recognize difference in session" do
+    controller.stub!(:session_hash).and_return(request.session_options[:id].encode("ISO-8859-1"))
+    controller.impressionist_subapp_filter(nil, [:session_hash])
+    Impression.should have(@impression_count + 1).records
+    controller.stub!(:session_hash).and_return("anothersessionhash")
+    controller.impressionist_subapp_filter(nil, [:session_hash])
+    Impression.should have(@impression_count + 2).records
+  end
+  
+  it "should recognize combined uniqueness" do
+    controller.stub!(:action_name).and_return("test_action")
+    controller.impressionist_subapp_filter(nil, [:ip_address, :request_hash, :action_name])
+    controller.impressionist_subapp_filter(nil, [:request_hash, :ip_address, :action_name])
+    controller.impressionist_subapp_filter(nil, [:request_hash, :action_name])
+    controller.impressionist_subapp_filter(nil, [:ip_address, :action_name])
+    controller.impressionist_subapp_filter(nil, [:ip_address, :request_hash])
+    controller.impressionist_subapp_filter(nil, [:action_name])
+    controller.impressionist_subapp_filter(nil, [:ip_address])
+    controller.impressionist_subapp_filter(nil, [:request_hash])
+    Impression.should have(@impression_count + 1).records
+  end
+  
+  it "should recognize combined non-uniqueness" do
+    controller.stub!(:action_name).and_return(nil)
+    controller.impressionist_subapp_filter(nil, [:ip_address, :action_name])
+    controller.stub!(:action_name).and_return("test_action")
+    controller.impressionist_subapp_filter(nil, [:ip_address, :action_name])
+    controller.stub!(:action_name).and_return("another_action")
+    controller.impressionist_subapp_filter(nil, [:ip_address, :action_name])
+    Impression.should have(@impression_count + 3).records
+  end
+    
+end
+
+
diff --git a/test_app/spec/rails_generators/rails_generators_spec.rb b/test_app/spec/rails_generators/rails_generators_spec.rb
index 7059b22..38ffbff 100644
--- a/test_app/spec/rails_generators/rails_generators_spec.rb
+++ b/test_app/spec/rails_generators/rails_generators_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 require 'systemu'
 
+# FIXME this test might break the others if run before them
+#
 describe Impressionist do
   fixtures :articles,:impressions,:posts
   it "should delete existing migration and generate the migration file" do
diff --git a/test_app/spec/spec_helper.rb b/test_app/spec/spec_helper.rb
index 9b8b02c..f24601c 100644
--- a/test_app/spec/spec_helper.rb
+++ b/test_app/spec/spec_helper.rb
@@ -24,4 +24,10 @@ RSpec.configure do |config|
   # examples within a transaction, remove the following line or assign false
   # instead of true.
   config.use_transactional_fixtures = true
+  
+  # make the rails logger usable in the tests as logger.xxx "..."
+  def logger
+    Rails.logger
+  end
+  
 end

From e63e835936d9772286360a2c8e760adc7209055e Mon Sep 17 00:00:00 2001
From: mio <georg@mittendorfer.com>
Date: Wed, 9 Nov 2011 18:10:35 +0100
Subject: [PATCH 3/5] added draft for impressionist(...) uniqueness

---
 app/controllers/impressionist_controller.rb   | 78 +++++++++----------
 .../unique_impression_controller_spec.rb      |  3 +-
 2 files changed, 41 insertions(+), 40 deletions(-)

diff --git a/app/controllers/impressionist_controller.rb b/app/controllers/impressionist_controller.rb
index 4a13a8c..1f0be8f 100644
--- a/app/controllers/impressionist_controller.rb
+++ b/app/controllers/impressionist_controller.rb
@@ -12,11 +12,14 @@ module ImpressionistController
       base.before_filter :impressionist_app_filter
     end
 
-    def impressionist(obj,message=nil)
+    def impressionist(obj,message=nil,opts={})
       unless bypass
         if obj.respond_to?("impressionable?")
-          obj.impressions.create(create_statement({:message => message}))
+          if unique_instance?(obj, opts[:unique])
+            obj.impressions.create(associative_create_statement({:message => message}))
+          end
         else
+          # we could create an impression anyway. for classes, too. why not?
           raise "#{obj.class.to_s} is not impressionable!"
         end
       end
@@ -26,17 +29,11 @@ module ImpressionistController
       @impressionist_hash = Digest::SHA2.hexdigest(Time.now.to_f.to_s+rand(10000).to_s)
     end
 
-    def impressionist_subapp_filter(actions=nil, unique_opts=nil)
+    def impressionist_subapp_filter(actions=nil,unique_opts=nil)
       unless bypass
         actions.collect!{|a|a.to_s} unless actions.blank?
-        if (actions.blank? || actions.include?(action_name)) && (unique_opts.blank? || is_unique(unique_opts))
-          if (!actions.blank? && !unique_opts.blank?)
-            logger.info "Restricted to actions #{actions.inspect} and uniqueness for #{unique_opts.inspect}"
-          end
-          Impression.create(create_statement(
-            :impressionable_type => controller_name.singularize.camelize,
-            :impressionable_id=> params[:id]
-          ))
+        if (actions.blank? || actions.include?(action_name)) && unique?(unique_opts)
+          Impression.create(direct_create_statement)
         end
       end
     end
@@ -50,52 +47,55 @@ module ImpressionistController
       Impressionist::Bots::LIST.include? request.user_agent
     end
     
-    def is_unique(unique_opts)
-      # FIXME think about uniqueness in relation to impressionable_id, impressionable_type and controller_name
-      # is controller name redundant? does the controller name always have to match?
-      default_statement = create_statement(
-        :impressionable_type => controller_name.singularize.camelize,
-        :impressionable_id=> params[:id]
-        )
-      statement = unique_opts.reduce({}) do |query, param|
-        query[param] = default_statement[param]
-        query
-      end
-      #logger.debug "Statement params: #{statement.inspect}."
-      # always use impressionable type?
-      statement[:impressionable_type] = controller_name.singularize.camelize
-      #statement[:impressionable_id] = params[:id]
-      return Impression.where(statement).size == 0
+    def unique_instance?(impressionable, unique_opts)
+      return unique_opts.blank? || impressionable.impressions.where(unique_query(unique_ops)).size == 0
     end
     
-    # creates a statment hash that contains default values for creating an impression (without
-    # :impressionable_type and impressionable_id as they are not needed for creating via association).
-    def create_statement(query_params={})
+    def unique?(unique_opts)
+      return unique_opts.blank? || Impression.where(unique_query(unique_opts)).size == 0
+    end
+    
+    # creates the query to check for uniqueness
+    def unique_query(unique_opts)
+      full_statement = direct_create_statement
+      # reduce the full statement to the params we need for the specified unique options
+      unique_opts.reduce({}) do |query, param|
+        query[param] = full_statement[param]
+        query
+      end
+    end
+    
+    # creates a statment hash that contains default values for creating an impression via an AR relation.
+    def associative_create_statement(query_params={})
       query_params.reverse_merge!(
         :controller_name => controller_name,
         :action_name => action_name,
         :user_id => user_id,
         :request_hash => @impressionist_hash,
         :session_hash => session_hash,
-        :ip_address => remote_ip,
+        :ip_address => request.remote_ip,
         :referrer => request.referer
-      )
+        )
+    end
+    
+    # creates a statment hash that contains default values for creating an impression.
+    def direct_create_statement(query_params={})
+      query_params.reverse_merge!(
+        :impressionable_type => controller_name.singularize.camelize,
+        :impressionable_id=> params[:id]
+        )
+      associative_create_statement(query_params)
     end
     
     def session_hash
       # # careful: request.session_options[:id] encoding in rspec test was ASCII-8BIT
-      # # that broke the database query for uniqueness. not sure how to solve this issue
-      # # seems to depend on app setup/config
+      # # that broke the database query for uniqueness. not sure if this is a testing only issue.
       # str = request.session_options[:id]
-      # # probably this isn't a fix: request.session_options[:id].encode("ISO-8859-1")
       # logger.debug "Encoding: #{str.encoding.inspect}"
+      # # request.session_options[:id].encode("ISO-8859-1")
       request.session_options[:id]
     end
     
-    def remote_ip
-      request.remote_ip
-    end
-    
     #use both @current_user and current_user helper
     def user_id
       user_id = @current_user ? @current_user.id : nil rescue nil
diff --git a/test_app/spec/controllers/unique_impression_controller_spec.rb b/test_app/spec/controllers/unique_impression_controller_spec.rb
index f4abde6..82710ca 100644
--- a/test_app/spec/controllers/unique_impression_controller_spec.rb
+++ b/test_app/spec/controllers/unique_impression_controller_spec.rb
@@ -57,7 +57,8 @@ describe PostsController do
   end
   
   it "should recognize referrer uniqueness" do
-    controller.stub!(:referrer).and_return("http://somehost.someurl.somdomain/some/path")
+    @request.env['HTTP_REFERER'] = 'http://somehost.someurl.somdomain/some/path'
+    #controller.stub!(:referer).and_return("http://somehost.someurl.somdomain/some/path")
     controller.impressionist_subapp_filter(nil, [:referrer])
     controller.impressionist_subapp_filter(nil, [:referrer])
     Impression.should have(@impression_count + 1).records

From 132f1f74ec1ac9782d13d978085458482fdbbdc8 Mon Sep 17 00:00:00 2001
From: mio <georg@mittendorfer.com>
Date: Thu, 10 Nov 2011 14:14:51 +0100
Subject: [PATCH 4/5] fixed typo

---
 app/controllers/impressionist_controller.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/controllers/impressionist_controller.rb b/app/controllers/impressionist_controller.rb
index 1f0be8f..1b74eb2 100644
--- a/app/controllers/impressionist_controller.rb
+++ b/app/controllers/impressionist_controller.rb
@@ -48,7 +48,7 @@ module ImpressionistController
     end
     
     def unique_instance?(impressionable, unique_opts)
-      return unique_opts.blank? || impressionable.impressions.where(unique_query(unique_ops)).size == 0
+      return unique_opts.blank? || impressionable.impressions.where(unique_query(unique_opts)).size == 0
     end
     
     def unique?(unique_opts)

From 5d77a1a2a08144654855b4f688ac1d06912d99aa Mon Sep 17 00:00:00 2001
From: mio <georg@mittendorfer.com>
Date: Thu, 10 Nov 2011 14:16:57 +0100
Subject: [PATCH 5/5] added tests for uniqueness feature

---
 test_app/app/controllers/dummy_controller.rb  |   6 +
 .../app/controllers/widgets_controller.rb     |   3 +-
 test_app/spec/controllers/controller_spec.rb  |  49 +--
 .../impressionist_uniqueness_spec.rb          | 312 ++++++++++++++++++
 .../unique_impression_controller_spec.rb      | 133 --------
 5 files changed, 347 insertions(+), 156 deletions(-)
 create mode 100644 test_app/app/controllers/dummy_controller.rb
 create mode 100644 test_app/spec/controllers/impressionist_uniqueness_spec.rb
 delete mode 100644 test_app/spec/controllers/unique_impression_controller_spec.rb

diff --git a/test_app/app/controllers/dummy_controller.rb b/test_app/app/controllers/dummy_controller.rb
new file mode 100644
index 0000000..f487fd1
--- /dev/null
+++ b/test_app/app/controllers/dummy_controller.rb
@@ -0,0 +1,6 @@
+# This controller imports the impressionist module to make the modules methods available for testing
+class DummyController < ActionController::Base
+
+  impressionist
+  
+end
diff --git a/test_app/app/controllers/widgets_controller.rb b/test_app/app/controllers/widgets_controller.rb
index 83c1261..3a7c5dd 100644
--- a/test_app/app/controllers/widgets_controller.rb
+++ b/test_app/app/controllers/widgets_controller.rb
@@ -1,5 +1,6 @@
 class WidgetsController < ApplicationController
-  impressionist :actions=>[:show,:index], :unique => [:action_name, :impressionable_id]
+  impressionist :actions=>[:show,:index], :unique => [:controller_name,:action_name,:impressionable_id]
+  
   def show
   end
   
diff --git a/test_app/spec/controllers/controller_spec.rb b/test_app/spec/controllers/controller_spec.rb
index da9f32c..f52665d 100644
--- a/test_app/spec/controllers/controller_spec.rb
+++ b/test_app/spec/controllers/controller_spec.rb
@@ -78,28 +78,6 @@ describe WidgetsController do
     Impression.all.size.should eq 13
   end
   
-  it "should log unique impressions at the per action" do
-    get "show", :id=> 1
-    Impression.all.size.should eq 12
-    get "show", :id=> 2
-    Impression.all.size.should eq 13
-    get "show", :id => 2
-    Impression.all.size.should eq 13
-    get "index"
-    Impression.all.size.should eq 14
-  end
-  
-  it "should log unique impressions only once per id" do
-    get "show", :id=> 1
-    Impression.all.size.should eq 12
-    get "show", :id=> 2
-    Impression.all.size.should eq 13
-    get "show", :id => 2
-    Impression.all.size.should eq 13
-    get "index"
-    Impression.all.size.should eq 14
-  end
-  
   it "should not log impression when user-agent is in wildcard list" do
     request.stub!(:user_agent).and_return('somebot')
     get "show", :id=> 1
@@ -111,5 +89,32 @@ describe WidgetsController do
     get "show", :id=> 1
     Impression.all.size.should eq 11    
   end
+  
+  describe "impressionist unique options" do
+    
+    it "should log unique impressions at the per action level" do
+      get "show", :id=> 1
+      Impression.all.size.should eq 12
+      get "show", :id=> 2
+      Impression.all.size.should eq 13
+      get "show", :id => 2
+      Impression.all.size.should eq 13
+      get "index"
+      Impression.all.size.should eq 14
+    end
+    
+    it "should log unique impressions only once per id" do
+      get "show", :id=> 1
+      Impression.all.size.should eq 12
+      get "show", :id=> 2
+      Impression.all.size.should eq 13
+      get "show", :id => 2
+      Impression.all.size.should eq 13
+      get "index"
+      Impression.all.size.should eq 14
+    end
+  
+  end
+  
 end
   
\ No newline at end of file
diff --git a/test_app/spec/controllers/impressionist_uniqueness_spec.rb b/test_app/spec/controllers/impressionist_uniqueness_spec.rb
new file mode 100644
index 0000000..6c8c337
--- /dev/null
+++ b/test_app/spec/controllers/impressionist_uniqueness_spec.rb
@@ -0,0 +1,312 @@
+require "spec_helper.rb"
+
+# we use the posts controller as it uses the impressionsist module. any such controller would do.
+describe DummyController do
+  
+  before do
+      @impression_count = Impression.all.size
+    end
+  
+  describe "impressionist filter uniqueness" do  
+  
+    it "should ignore uniqueness if not requested" do
+      controller.impressionist_subapp_filter(nil, nil)
+      controller.impressionist_subapp_filter(nil, nil)
+      Impression.should have(@impression_count + 2).records
+    end
+    
+    it "should recognize unique session" do
+      # the following line was necessary as session hash returned a binary string (ASCII-8BIT encoded)
+      controller.stub!(:session_hash).and_return(request.session_options[:id].encode("ISO-8859-1"))
+      controller.impressionist_subapp_filter(nil, [:session_hash])
+      controller.impressionist_subapp_filter(nil, [:session_hash])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+    it "should recognize unique ip" do
+      controller.request.stub!(:remote_ip).and_return("1.2.3.4")
+      controller.impressionist_subapp_filter(nil, [:ip_address])
+      controller.impressionist_subapp_filter(nil, [:ip_address])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+    it "should recognize unique request" do
+      controller.impressionist_subapp_filter(nil, [:request_hash])
+      controller.impressionist_subapp_filter(nil, [:request_hash])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+    it "should recognize unique action" do
+      controller.stub!(:action_name).and_return("test_action")
+      controller.impressionist_subapp_filter(nil, [:action_name])
+      controller.impressionist_subapp_filter(nil, [:action_name])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+    it "should recognize unique controller" do
+      controller.stub!(:controller_name).and_return("test_controller")
+      controller.impressionist_subapp_filter(nil, [:controller_name])
+      controller.impressionist_subapp_filter(nil, [:controller_name])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+     it "should recognize unique user" do
+      controller.stub!(:user_id).and_return(42)
+      controller.impressionist_subapp_filter(nil, [:user_id])
+      controller.impressionist_subapp_filter(nil, [:user_id])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+    it "should recognize unique referer" do
+      controller.request.stub!(:referer).and_return("http://foo/bar")
+      controller.impressionist_subapp_filter(nil, [:referrer])
+      controller.impressionist_subapp_filter(nil, [:referrer])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+    it "should recognize unique id" do
+      controller.stub!(:params).and_return({:id => "666"}) # for correct impressionable id in filter
+      controller.impressionist_subapp_filter(nil, [:impressionable_id])
+      controller.impressionist_subapp_filter(nil, [:impressionable_id])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+    # extra redundant test for important controller and action combination.
+    it "should recognize different controller and action" do
+      controller.stub!(:controller_name).and_return("test_controller")
+      controller.stub!(:action_name).and_return("test_action")
+      controller.impressionist_subapp_filter(nil, [:controller_name, :action_name])
+      controller.impressionist_subapp_filter(nil, [:controller_name, :action_name])
+      Impression.should have(@impression_count + 1).records
+      controller.stub!(:action_name).and_return("another_action")
+      controller.impressionist_subapp_filter(nil, [:controller_name, :action_name])
+      controller.impressionist_subapp_filter(nil, [:controller_name, :action_name])
+      Impression.should have(@impression_count + 2).records
+      controller.stub!(:controller_name).and_return("another_controller")
+      controller.impressionist_subapp_filter(nil, [:controller_name, :action_name])
+      controller.impressionist_subapp_filter(nil, [:controller_name, :action_name])
+      Impression.should have(@impression_count + 3).records
+    end
+    
+    it "should recognize different action" do
+      controller.stub!(:action_name).and_return("test_action")
+      controller.impressionist_subapp_filter(nil, [:action_name])
+      controller.impressionist_subapp_filter(nil, [:action_name])
+      Impression.should have(@impression_count + 1).records
+      controller.stub!(:action_name).and_return("another_action")
+      controller.impressionist_subapp_filter(nil, [:action_name])
+      controller.impressionist_subapp_filter(nil, [:action_name])
+      Impression.should have(@impression_count + 2).records
+    end
+    
+    it "should recognize different controller" do
+      controller.stub!(:controller_name).and_return("test_controller")
+      controller.impressionist_subapp_filter(nil, [:controller_name])
+      controller.impressionist_subapp_filter(nil, [:controller_name])
+      Impression.should have(@impression_count + 1).records
+      controller.stub!(:controller_name).and_return("another_controller")
+      controller.impressionist_subapp_filter(nil, [:controller_name])
+      controller.impressionist_subapp_filter(nil, [:controller_name])
+      Impression.should have(@impression_count + 2).records
+    end
+    
+    it "should recognize different session" do
+      controller.stub!(:session_hash).and_return("foo")
+      controller.impressionist_subapp_filter(nil, [:session_hash])
+      controller.impressionist_subapp_filter(nil, [:session_hash])
+      Impression.should have(@impression_count + 1).records
+      controller.stub!(:session_hash).and_return("bar")
+      controller.impressionist_subapp_filter(nil, [:session_hash])
+      controller.impressionist_subapp_filter(nil, [:session_hash])
+      Impression.should have(@impression_count + 2).records
+    end
+    
+    it "should recognize different ip" do
+      controller.request.stub!(:remote_ip).and_return("1.2.3.4")
+      controller.impressionist_subapp_filter(nil, [:ip_address])
+      controller.impressionist_subapp_filter(nil, [:ip_address])
+      Impression.should have(@impression_count + 1).records
+      controller.request.stub!(:remote_ip).and_return("5.6.7.8")
+      controller.impressionist_subapp_filter(nil, [:ip_address])
+      controller.impressionist_subapp_filter(nil, [:ip_address])
+      Impression.should have(@impression_count + 2).records
+    end
+    
+    it "should recognize different referer" do
+      controller.request.stub!(:referer).and_return("http://foo/bar")
+      controller.impressionist_subapp_filter(nil, [:referrer])
+      controller.impressionist_subapp_filter(nil, [:referrer])
+      Impression.should have(@impression_count + 1).records
+      controller.request.stub!(:referer).and_return("http://bar/fo")
+      controller.impressionist_subapp_filter(nil, [:referrer])
+      controller.impressionist_subapp_filter(nil, [:referrer])
+      Impression.should have(@impression_count + 2).records
+    end
+    
+    it "should recognize different id" do
+      controller.stub!(:params).and_return({:id => "666"}) # for correct impressionable id in filter
+      controller.impressionist_subapp_filter(nil, [:impressionable_type, :impressionable_id])
+      controller.impressionist_subapp_filter(nil, [:impressionable_type, :impressionable_id])
+      controller.stub!(:params).and_return({:id => "42"}) # for correct impressionable id in filter
+      controller.impressionist_subapp_filter(nil, [:impressionable_type, :impressionable_id])
+      controller.impressionist_subapp_filter(nil, [:impressionable_type, :impressionable_id])
+      Impression.should have(@impression_count + 2).records
+    end
+    
+    it "should recognize combined uniqueness" do
+      controller.stub!(:action_name).and_return("test_action")
+      controller.impressionist_subapp_filter(nil, [:ip_address, :request_hash, :action_name])
+      controller.impressionist_subapp_filter(nil, [:request_hash, :ip_address, :action_name])
+      controller.impressionist_subapp_filter(nil, [:request_hash, :action_name])
+      controller.impressionist_subapp_filter(nil, [:ip_address, :action_name])
+      controller.impressionist_subapp_filter(nil, [:ip_address, :request_hash])
+      controller.impressionist_subapp_filter(nil, [:action_name])
+      controller.impressionist_subapp_filter(nil, [:ip_address])
+      controller.impressionist_subapp_filter(nil, [:request_hash])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+    it "should recognize combined non-uniqueness" do
+      controller.stub!(:action_name).and_return(nil)
+      controller.impressionist_subapp_filter(nil, [:ip_address, :action_name])
+      controller.stub!(:action_name).and_return("test_action")
+      controller.impressionist_subapp_filter(nil, [:ip_address, :action_name])
+      controller.stub!(:action_name).and_return("another_action")
+      controller.impressionist_subapp_filter(nil, [:ip_address, :action_name])
+      Impression.should have(@impression_count + 3).records
+    end     
+    
+  end
+  
+  describe "impressionist method uniqueness for impressionables" do  
+    
+    # in this test we reuse the post model. might break if model changes.
+    
+    it "should ignore uniqueness if not requested" do
+      impressionable = Post.create
+      controller.impressionist impressionable
+      controller.impressionist impressionable
+      Impression.should have(@impression_count + 2).records
+    end
+    
+    it "should recognize unique session" do
+      # the following line was necessary as session hash returned a binary string (ASCII-8BIT encoded)
+      controller.stub!(:session_hash).and_return(request.session_options[:id].encode("ISO-8859-1"))
+      impressionable = Post.create
+      controller.impressionist(impressionable, nil, :unique => [:session_hash])
+      controller.impressionist(impressionable, nil, :unique => [:session_hash])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+    it "should recognize unique ip" do
+      controller.request.stub!(:remote_ip).and_return("1.2.3.4")
+      impressionable = Post.create
+      controller.impressionist(impressionable, nil, :unique => [:ip_address])
+      controller.impressionist(impressionable, nil, :unique => [:ip_address])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+    it "should recognize unique request" do
+      impressionable = Post.create
+      controller.impressionist(impressionable, nil, :unique => [:request_hash])
+      controller.impressionist(impressionable, nil, :unique => [:request_hash])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+     it "should recognize unique user" do
+      controller.stub!(:user_id).and_return(666)
+      impressionable = Post.create
+      controller.impressionist(impressionable, nil, :unique => [:user_id])
+      controller.impressionist(impressionable, nil, :unique => [:user_id])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+    it "should recognize unique referer" do
+      controller.request.stub!(:referer).and_return("http://foo/bar")
+      impressionable = Post.create
+      controller.impressionist(impressionable, nil, :unique => [:referrer])
+      controller.impressionist(impressionable, nil, :unique => [:referrer])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+    it "should recognize different session" do
+      impressionable = Post.create
+      controller.stub!(:session_hash).and_return("foo")
+      controller.impressionist(impressionable, nil, :unique => [:session_hash])
+      controller.impressionist(impressionable, nil, :unique => [:session_hash])
+      Impression.should have(@impression_count + 1).records
+      controller.stub!(:session_hash).and_return("bar")
+      controller.impressionist(impressionable, nil, :unique => [:session_hash])
+      controller.impressionist(impressionable, nil, :unique => [:session_hash])
+      Impression.should have(@impression_count + 2).records
+    end
+    
+    it "should recognize different ip" do
+      controller.request.stub!(:remote_ip).and_return("1.2.3.4")
+      impressionable = Post.create
+      controller.impressionist(impressionable, nil, :unique => [:ip_address])
+      controller.impressionist(impressionable, nil, :unique => [:ip_address])
+      Impression.should have(@impression_count + 1).records
+      controller.request.stub!(:remote_ip).and_return("5.6.7.8")
+      controller.impressionist(impressionable, nil, :unique => [:ip_address])
+      controller.impressionist(impressionable, nil, :unique => [:ip_address])
+      Impression.should have(@impression_count + 2).records
+    end
+    
+    it "should recognize different user" do
+      impressionable = Post.create
+      controller.stub!(:user_id).and_return(666)
+      controller.impressionist(impressionable, nil, :unique => [:user_id])
+      controller.impressionist(impressionable, nil, :unique => [:user_id])
+      Impression.should have(@impression_count + 1).records
+      controller.stub!(:user_id).and_return(42)
+      controller.impressionist(impressionable, nil, :unique => [:user_id])
+      controller.impressionist(impressionable, nil, :unique => [:user_id])
+      Impression.should have(@impression_count + 2).records
+    end
+    
+    it "should recognize combined uniqueness" do
+      impressionable = Post.create
+      controller.stub!(:session_hash).and_return("foo")
+      controller.impressionist(impressionable, nil, :unique => [:ip_address, :request_hash, :session_hash])
+      controller.impressionist(impressionable, nil, :unique => [:request_hash, :ip_address, :session_hash])
+      controller.impressionist(impressionable, nil, :unique => [:request_hash, :session_hash])
+      controller.impressionist(impressionable, nil, :unique => [:ip_address, :session_hash])
+      controller.impressionist(impressionable, nil, :unique => [:ip_address, :request_hash])
+      controller.impressionist(impressionable, nil, :unique => [:session_hash])
+      controller.impressionist(impressionable, nil, :unique => [:ip_address])
+      controller.impressionist(impressionable, nil, :unique => [:request_hash])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+    it "should recognize combined non-uniqueness" do
+      impressionable = Post.create
+      controller.stub!(:session_hash).and_return(nil)
+      controller.impressionist(impressionable, nil, :unique => [:ip_address, :session_hash])
+      controller.stub!(:session_hash).and_return("foo")
+      controller.impressionist(impressionable, nil, :unique => [:ip_address, :session_hash])
+      controller.stub!(:session_hash).and_return("bar")
+      controller.impressionist(impressionable, nil, :unique => [:ip_address, :session_hash])
+      Impression.should have(@impression_count + 3).records
+    end
+    
+  end
+  
+  describe "impressionist filter and method uniqueness" do
+  
+    it "should recognize uniqueness" do
+      impressionable = Post.create
+      controller.stub!(:controller_name).and_return("posts") # for correct impressionable type in filter
+      controller.stub!(:params).and_return({:id => impressionable.id.to_s}) # for correct impressionable id in filter
+      controller.stub!(:session_hash).and_return("foo")
+      controller.request.stub!(:remote_ip).and_return("1.2.3.4")
+      # order of the following methods is important for the test!
+      controller.impressionist_subapp_filter(nil, [:ip_address, :request_hash, :session_hash])
+      controller.impressionist(impressionable, nil, :unique => [:ip_address, :request_hash, :session_hash])
+      Impression.should have(@impression_count + 1).records
+    end
+    
+  end
+  
+end
+
diff --git a/test_app/spec/controllers/unique_impression_controller_spec.rb b/test_app/spec/controllers/unique_impression_controller_spec.rb
deleted file mode 100644
index 82710ca..0000000
--- a/test_app/spec/controllers/unique_impression_controller_spec.rb
+++ /dev/null
@@ -1,133 +0,0 @@
-require 'spec_helper.rb'
-
-# we use the posts controller as it uses the impressionsist module. any such controller would do.
-describe PostsController do
-    
-  before do
-    @impression_count = Impression.all.size
-  end
-  
-  it "should ignore uniqueness if not requested" do
-    controller.impressionist_subapp_filter(nil, nil)
-    controller.impressionist_subapp_filter(nil, nil)
-    Impression.should have(@impression_count + 2).records
-  end
-  
-  it "should recognize session uniqueness" do
-    # the following line was necessary as session hash returned a binary string (ASCII-8BIT encoded)
-    # not sure how to 'fix' this. setup/config issue?
-    controller.stub!(:session_hash).and_return(request.session_options[:id].encode("ISO-8859-1"))
-    controller.impressionist_subapp_filter(nil, [:session_hash])
-    controller.impressionist_subapp_filter(nil, [:session_hash])
-    Impression.should have(@impression_count + 1).records
-  end
-  
-  it "should recognize ip uniqueness" do
-    controller.stub!(:action_name).and_return("test_action")
-    controller.impressionist_subapp_filter(nil, [:ip_address])
-    controller.impressionist_subapp_filter(nil, [:ip_address])
-    Impression.should have(@impression_count + 1).records
-  end
-  
-  it "should recognize request uniqueness" do
-    controller.impressionist_subapp_filter(nil, [:request_hash])
-    controller.impressionist_subapp_filter(nil, [:request_hash])
-    Impression.should have(@impression_count + 1).records
-  end
-  
-  it "should recognize action uniqueness" do
-    controller.stub!(:action_name).and_return("test_action")
-    controller.impressionist_subapp_filter(nil, [:action_name])
-    controller.impressionist_subapp_filter(nil, [:action_name])
-    Impression.should have(@impression_count + 1).records
-  end
-  
-  it "should recognize controller uniqueness" do
-    controller.stub!(:controller_name).and_return("test_controller")
-    controller.impressionist_subapp_filter(nil, [:controller_name])
-    controller.impressionist_subapp_filter(nil, [:controller_name])
-    Impression.should have(@impression_count + 1).records
-  end
-  
-   it "should recognize user uniqueness" do
-    controller.stub!(:user_id).and_return(1)
-    controller.impressionist_subapp_filter(nil, [:user_id])
-    controller.impressionist_subapp_filter(nil, [:user_id])
-    Impression.should have(@impression_count + 1).records
-  end
-  
-  it "should recognize referrer uniqueness" do
-    @request.env['HTTP_REFERER'] = 'http://somehost.someurl.somdomain/some/path'
-    #controller.stub!(:referer).and_return("http://somehost.someurl.somdomain/some/path")
-    controller.impressionist_subapp_filter(nil, [:referrer])
-    controller.impressionist_subapp_filter(nil, [:referrer])
-    Impression.should have(@impression_count + 1).records
-  end
-  
-  # extra redundant test for important controller and action combination.
-  it "should recognize difference in controller and action" do
-    controller.stub!(:controller_name).and_return("test_controller")
-    controller.stub!(:action_name).and_return("test_action")
-    controller.impressionist_subapp_filter(nil, [:controller_name, :action_name])
-    Impression.should have(@impression_count + 1).records
-    controller.stub!(:action_name).and_return("another_action")
-    controller.impressionist_subapp_filter(nil, [:controller_name, :action_name])
-    Impression.should have(@impression_count + 2).records
-    controller.stub!(:controller_name).and_return("another_controller")
-    controller.impressionist_subapp_filter(nil, [:controller_name, :action_name])
-    Impression.should have(@impression_count + 3).records
-  end
-  
-  it "should recognize difference in action" do
-    controller.stub!(:action_name).and_return("test_action")
-    controller.impressionist_subapp_filter(nil, [:action_name])
-    Impression.should have(@impression_count + 1).records
-    controller.stub!(:action_name).and_return("another_action")
-    controller.impressionist_subapp_filter(nil, [:action_name])
-    Impression.should have(@impression_count + 2).records
-  end
-  
-  it "should recognize difference in controller" do
-    controller.stub!(:controller_name).and_return("test_controller")
-    controller.impressionist_subapp_filter(nil, [:controller_name])
-    Impression.should have(@impression_count + 1).records
-    controller.stub!(:controller_name).and_return("another_controller")
-    controller.impressionist_subapp_filter(nil, [:controller_name])
-    Impression.should have(@impression_count + 2).records
-  end
-  
-  it "should recognize difference in session" do
-    controller.stub!(:session_hash).and_return(request.session_options[:id].encode("ISO-8859-1"))
-    controller.impressionist_subapp_filter(nil, [:session_hash])
-    Impression.should have(@impression_count + 1).records
-    controller.stub!(:session_hash).and_return("anothersessionhash")
-    controller.impressionist_subapp_filter(nil, [:session_hash])
-    Impression.should have(@impression_count + 2).records
-  end
-  
-  it "should recognize combined uniqueness" do
-    controller.stub!(:action_name).and_return("test_action")
-    controller.impressionist_subapp_filter(nil, [:ip_address, :request_hash, :action_name])
-    controller.impressionist_subapp_filter(nil, [:request_hash, :ip_address, :action_name])
-    controller.impressionist_subapp_filter(nil, [:request_hash, :action_name])
-    controller.impressionist_subapp_filter(nil, [:ip_address, :action_name])
-    controller.impressionist_subapp_filter(nil, [:ip_address, :request_hash])
-    controller.impressionist_subapp_filter(nil, [:action_name])
-    controller.impressionist_subapp_filter(nil, [:ip_address])
-    controller.impressionist_subapp_filter(nil, [:request_hash])
-    Impression.should have(@impression_count + 1).records
-  end
-  
-  it "should recognize combined non-uniqueness" do
-    controller.stub!(:action_name).and_return(nil)
-    controller.impressionist_subapp_filter(nil, [:ip_address, :action_name])
-    controller.stub!(:action_name).and_return("test_action")
-    controller.impressionist_subapp_filter(nil, [:ip_address, :action_name])
-    controller.stub!(:action_name).and_return("another_action")
-    controller.impressionist_subapp_filter(nil, [:ip_address, :action_name])
-    Impression.should have(@impression_count + 3).records
-  end
-    
-end
-
-