From 09bfa2f27eff55833d783f1705f189b8cd2fd880 Mon Sep 17 00:00:00 2001 From: mio Date: Mon, 7 Nov 2011 20:41:51 +0100 Subject: [PATCH] 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