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