Merge pull request #12 from coryschires/master

Merged in the counter caching code. Added docs for previous pull request concerning recording unique impressions

.gitignore

@@ -1,5 +1,4 @@
 Gemfile.lock
-/test_app/db/migrate/20*
 /test_app/db/schema.rb
 /pkg
 *~

README.md

@@ -5,15 +5,9 @@ impressionist
 
 A lightweight plugin that logs impressions per action or manually per model
 
+I would not call this a stable plugin yet, although I have been running it in prod with no problems.  Use at your own risk ;-)
 ------------------------------------------------------------------------------------------------------------------------------
 
-NOTE: If you are upgrading from a version prior to 0.4.0, you will need to run this migration after the upgrade:
-
-https://github.com/charlotte-ruby/impressionist/blob/master/upgrade_migrations/version_0_4_0.rb
-
-If you don't run this migration you will receive this error: Unknown attribute : referrer
-
-
 What does this thing do?
 ------------------------
 Logs an impression... and I use that term loosely.  It can log page impressions (technically action impressions), but it is not limited to that.
@@ -28,7 +22,7 @@ http://www.user-agents.org/allagents.xml
 
 Which versions of Rails and Ruby is this compatible with?
 ---------------------------------------------------------
-Rails 3.0.x and Ruby 1.9.2 (also tested on REE 1.8.7) - Sorry, but you need to upgrade if you are using Rails 2.  You know you want to anyways.. all the cool kids are doing it ;-)
+Rails 3.0.4 and Ruby 1.9.2 (also tested on REE 1.8.7) - Sorry, but you need to upgrade if you are using Rails 2.  You know you want to anyways.. all the cool kids are doing it ;-)
 
 Installation
 ------------
@@ -112,6 +106,38 @@ Usage
 
 Logging impressions for authenticated users happens automatically.  If you have a current_user helper or use @current_user in your before_filter to set your authenticated user, current_user.id will be written to the user_id field in the impressions table.
 
+Adding a counter cache
+----------------------
+Impressionist makes it easy to add a `counter_cache` column to your model. The most basic configuration looks like:
+
+  is_impressionable :counter_cache => true
+
+This will automatically increment the `impressions_count` column in the included model. Note: You'll need to add that column to your model. If you'd like specific a different column name, you can:
+
+  is_impressionable :counter_cache => { :column_name => :my_column }
+
+If you'd like to include only unique impressions in your count:
+
+  is_impressionable :counter_cache => { :column_name => :my_column, :unique => true }
+
+
+What if I only want to record unique impressions?
+-------------------------------------------------
+Maybe you only care about unique impressions and would like to eliminate unnecessary database calls. You can specify conditions for recording impressions in your controller:
+
+  # only record impression if the request has a unique combination of type, id, and session
+  impressionist :unique => [:impressionable_type, :impressionable_id, :session_hash]
+
+  # only record impression if the request has a unique combination of controller, action, and session
+  impressionist :unique => [:controller_name, :action_name, :session_hash]
+
+  # only record impression if session is unique
+  impressionist :unique => [:session_hash]
+
+Or you can use the `impressionist` method directly:
+
+  impressionist(impressionable, "some message", :unique => [:session_hash])
+
 
 Development Roadmap
 -------------------

app/models/impression.rb

@@ -1,3 +1,16 @@
 class Impression < ActiveRecord::Base
   belongs_to :impressionable, :polymorphic=>true
+
+  after_save :update_impressions_counter_cache
+
+  private
+
+  def update_impressions_counter_cache
+    impressionable_class = self.impressionable_type.constantize
+
+    if impressionable_class.counter_cache_options
+      resouce = impressionable_class.find(self.impressionable_id)
+      resouce.try(:update_counter_cache)
+    end
+  end
 end

app/models/impressionist/impressionable.rb

@@ -1,8 +1,31 @@
 module Impressionist
   module Impressionable
-    def is_impressionable
+
+    def self.included(base)
+      base.extend ClassMethods
+      base.send(:include, InstanceMethods)
+    end
+
+    module ClassMethods
+      attr_accessor :cache_options
+      @cache_options = nil
+
+      def is_impressionable(options={})
         has_many :impressions, :as=>:impressionable
-      include InstanceMethods
+        @cache_options = options[:counter_cache]
+      end
+
+      def counter_cache_options
+        if @cache_options
+          options = { :column_name => :impressions_count, :unique => false }
+          options.merge!(@cache_options) if @cache_options.is_a?(Hash)
+          options
+        end
+      end
+
+      def counter_caching?
+        counter_cache_options.present?
+      end
     end
 
     module InstanceMethods
@@ -19,6 +42,13 @@ module Impressionist
         imps.all.size
       end
 
+      def update_counter_cache
+        cache_options = self.class.counter_cache_options
+        column_name = cache_options[:column_name].to_sym
+        count = cache_options[:unique] ? impressionist_count(:filter => :ip_address) : impressionist_count
+        update_attribute(column_name, count)
+      end
+
       # OLD METHODS - DEPRECATE IN V0.5
       def impression_count(start_date=nil,end_date=Time.now)
         impressionist_count({:start_date=>start_date, :end_date=>end_date, :filter=>:all})
@@ -36,5 +66,6 @@ module Impressionist
         impressionist_count({:start_date=>start_date, :end_date=>end_date, :filter=> :session_hash})
       end
     end
+
   end
 end

lib/impressionist/engine.rb

@@ -4,7 +4,7 @@ require "rails"
 module Impressionist
   class Engine < Rails::Engine
     initializer 'impressionist.extend_ar' do |app|
-      ActiveRecord::Base.extend Impressionist::Impressionable
+      ActiveRecord::Base.send(:include, Impressionist::Impressionable)
     end
 
     initializer 'impressionist.controller' do

test_app/Gemfile.lock

@@ -1,5 +1,5 @@
 PATH
-  remote: /home/mio/prog/projects/impressionist
+  remote: /Users/coryschires/Desktop/impressionist
   specs:
     impressionist (0.4.0)
 
@@ -45,7 +45,7 @@ GEM
     autotest-standalone (4.5.8)
     bcrypt-ruby (3.0.1)
     builder (3.0.0)
-    capybara (1.1.1)
+    capybara (1.1.2)
       mime-types (>= 1.16)
       nokogiri (>= 1.3.3)
       rack (>= 1.0.0)
@@ -54,10 +54,10 @@ GEM
       xpath (~> 0.1.4)
     childprocess (0.2.2)
       ffi (~> 1.0.6)
-    cucumber (1.1.1)
+    cucumber (1.1.3)
       builder (>= 2.1.2)
       diff-lcs (>= 1.1.2)
-      gherkin (~> 2.6.0)
+      gherkin (~> 2.6.7)
       json (>= 1.4.6)
       term-ansicolor (>= 1.0.6)
     cucumber-rails (1.2.0)
@@ -65,12 +65,12 @@ GEM
       cucumber (>= 1.1.1)
       nokogiri (>= 1.5.0)
     daemons (1.0.10)
-    database_cleaner (0.6.7)
+    database_cleaner (0.7.0)
     diff-lcs (1.1.3)
     erubis (2.7.0)
-    ffi (1.0.9)
+    ffi (1.0.11)
     gem_plugin (0.2.3)
-    gherkin (2.6.2)
+    gherkin (2.6.8)
       json (>= 1.4.6)
     hike (1.2.1)
     i18n (0.6.0)
@@ -129,10 +129,10 @@ GEM
       activesupport (~> 3.0)
       railties (~> 3.0)
       rspec (~> 2.7.0)
-    rubyzip (0.9.4)
-    selenium-webdriver (2.10.0)
+    rubyzip (0.9.5)
+    selenium-webdriver (2.13.0)
       childprocess (>= 0.2.1)
-      ffi (= 1.0.9)
+      ffi (~> 1.0.9)
       json_pure
       rubyzip
     spork (0.8.5)

test_app/app/models/dummy.rb

@@ -0,0 +1,7 @@
+# We don't really care about this model. It's just being used to test the uniqueness controller
+# specs. Nevertheless, we need a model because the counter caching functionality expects it.
+#
+class Dummy < ActiveRecord::Base
+  self.abstract_class = true # doesn't need to be backed by an actual table
+  is_impressionable
+end

test_app/app/models/widget.rb

@@ -0,0 +1,3 @@
+class Widget < ActiveRecord::Base
+  is_impressionable :counter_cache => true
+end

test_app/db/migrate/20111127184039_create_widgets.rb

@@ -0,0 +1,15 @@
+class CreateWidgets < ActiveRecord::Migration
+  def self.up
+    create_table :widgets do |t|
+      t.string :name
+      t.integer :impressions_count
+
+      t.timestamps
+    end
+  end
+
+  def self.down
+    drop_table :widgets
+  end
+end
+

test_app/db/migrate/20111127193841_create_impressions_table.rb

@@ -0,0 +1,37 @@
+class CreateImpressionsTable < ActiveRecord::Migration
+  def self.up
+    create_table :impressions, :force => true do |t|
+      t.string :impressionable_type
+      t.integer :impressionable_id
+      t.integer :user_id
+      t.string :controller_name
+      t.string :action_name
+      t.string :view_name
+      t.string :request_hash
+      t.string :session_hash
+      t.string :ip_address
+      t.string :message
+      t.string :referrer
+      t.timestamps
+    end
+    add_index :impressions, [:impressionable_type, :impressionable_id, :request_hash], :name => "poly_request_index", :unique => false
+    add_index :impressions, [:impressionable_type, :impressionable_id, :ip_address], :name => "poly_ip_index", :unique => false
+    add_index :impressions, [:impressionable_type, :impressionable_id, :session_hash], :name => "poly_session_index", :unique => false    
+    add_index :impressions, [:controller_name,:action_name,:request_hash], :name => "controlleraction_request_index", :unique => false
+    add_index :impressions, [:controller_name,:action_name,:ip_address], :name => "controlleraction_ip_index", :unique => false
+    add_index :impressions, [:controller_name,:action_name,:session_hash], :name => "controlleraction_session_index", :unique => false        
+    add_index :impressions, :user_id
+  end
+
+  def self.down
+    remove_index :impressions, :name => :poly_request_index
+    remove_index :impressions, :name => :poly_ip_index
+    remove_index :impressions, :name => :poly_session_index        
+    remove_index :impressions, :name => :controlleraction_request_index
+    remove_index :impressions, :name => :controlleraction_ip_index
+    remove_index :impressions, :name => :controlleraction_session_index        
+    remove_index :impressions, :user_id
+    
+    drop_table :impressions
+  end
+end

test_app/spec/controllers/controller_spec.rb

@@ -1,7 +1,7 @@
 require 'spec_helper.rb'
 
 describe ArticlesController do
-  fixtures :articles,:impressions,:posts
+  fixtures :articles,:impressions,:posts,:widgets
   render_views
 
   it "should make the impressionable_hash available" do
@@ -69,6 +69,12 @@ describe PostsController do
 end
 
 describe WidgetsController do
+
+  before(:each) do
+    @widget = Widget.find(1)
+    Widget.stub(:find).and_return(@widget)
+  end
+
   it "should log impression at the per action level" do
     get "show", :id=> 1
     Impression.all.size.should eq 12
@@ -117,4 +123,3 @@ describe WidgetsController do
   end
   
 end
-  

test_app/spec/controllers/impressionist_uniqueness_spec.rb

@@ -44,7 +44,7 @@ describe DummyController do
     end
 
     it "should recognize unique controller" do
-      controller.stub!(:controller_name).and_return("test_controller")
+      controller.stub!(:controller_name).and_return("post")
       controller.impressionist_subapp_filter(nil, [:controller_name])
       controller.impressionist_subapp_filter(nil, [:controller_name])
       Impression.should have(@impression_count + 1).records
@@ -73,7 +73,7 @@ describe DummyController do
 
     # 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!(:controller_name).and_return("post")
       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])
@@ -82,7 +82,7 @@ describe DummyController do
       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.stub!(:controller_name).and_return("article")
       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
@@ -100,11 +100,11 @@ describe DummyController do
     end
 
     it "should recognize different controller" do
-      controller.stub!(:controller_name).and_return("test_controller")
+      controller.stub!(:controller_name).and_return("post")
       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.stub!(:controller_name).and_return("article")
       controller.impressionist_subapp_filter(nil, [:controller_name])
       controller.impressionist_subapp_filter(nil, [:controller_name])
       Impression.should have(@impression_count + 2).records

test_app/spec/fixtures/widgets.yml

@@ -0,0 +1,4 @@
+one:
+  id: 1
+  name: A Widget
+  impressions_count: 0

test_app/spec/models/counter_caching_spec.rb

@@ -0,0 +1,30 @@
+require 'spec_helper'
+
+describe Impression do
+  fixtures :widgets
+
+  before(:each) do
+    @widget = Widget.find(1)
+    Impression.destroy_all
+  end
+
+  describe "self#counter_caching?" do
+    it "should know when counter caching is enabled" do
+      Widget.should be_counter_caching
+    end
+
+    it "should know when counter caching is disabled" do
+      Article.should_not be_counter_caching
+    end
+  end
+
+  describe "#update_counter_cache" do
+    it "should update the counter cache column to reflect the correct number of impressions" do
+      lambda {
+         Impression.create(:impressionable_type => @widget.class.name, :impressionable_id => @widget.id)
+         @widget.reload
+       }.should change(@widget, :impressions_count).from(0).to(1)
+    end
+  end
+
+end

test_app/spec/models/model_spec.rb

@@ -49,6 +49,8 @@ describe Impression do
     @article.impressionist_count(:filter=>:session_hash).should eq 7
   end
   
+  
+  
   #OLD COUNT METHODS.  DEPRECATE SOON
   it "should return the impression count with no date range specified" do
     @article.impression_count.should eq 11