FIX #92, Allow options to be passed to counter_cache unique, and clean up

app/models/impressionist/impressionable.rb

@@ -4,59 +4,43 @@ module Impressionist
 
     module ClassMethods
       attr_accessor :impressionist_cache_options
-      @impressionist_cache_options = nil
+
+      DEFAULT_CACHE = { :counter_cache => false, :column_name => :impressions_count, :unique => false }
 
       def impressionist_counter_cache_options
-        if @impressionist_cache_options
+        @impressionist_cache_options ||= {}
-          options = { :column_name => :impressions_count, :unique => false }
+        @impressionist_cache_options.reverse_merge!(DEFAULT_CACHE)
-          options.merge!(@impressionist_cache_options) if @impressionist_cache_options.is_a?(Hash)
-          options
-        end
       end
 
+      # true or false
       def impressionist_counter_caching?
-        impressionist_counter_cache_options.present?
+        impressionist_counter_cache_options[:counter_cache]
       end
 
       def counter_caching?
-        ::ActiveSupport::Deprecation.warn("#counter_caching? is deprecated; please use #impressionist_counter_caching? instead")
+          ::ActiveSupport::Deprecation.warn("#counter_caching? is deprecated; please use #impressionist_counter_caching? instead")
-        impressionist_counter_caching?
+          impressionist_counter_caching?
       end
+
     end
 
-    def impressionable?
+    # ------------------------------------------
-      true
+    # TODO: CLEAN UP, make it HUMAN readable
-    end
-
     def impressionist_count(options={})
       options.reverse_merge!(:filter=>:request_hash, :start_date=>nil, :end_date=>Time.now)
       imps = options[:start_date].blank? ? impressions : impressions.where("created_at>=? and created_at<=?",options[:start_date],options[:end_date])
       options[:filter] == :all ? imps.count : imps.count(options[:filter], :distinct => true)
     end
 
-    def update_impressionist_counter_cache
+    def update_impressionist_counter_cache 
-      cache_options = self.class.impressionist_counter_cache_options
+      slave = Impressionist::UpdateCounters.new(self)
-      column_name = cache_options[:column_name].to_sym
+      slave.update
-      count = cache_options[:unique] ? impressionist_count(:filter => :ip_address) : impressionist_count
+    end 
-      old_count = send(column_name) || 0
+
-      self.class.update_counters(id, column_name => (count - old_count))
+    def impressionable?
+      true
     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})
-    end
-
-    def unique_impression_count(start_date=nil,end_date=Time.now)
-      impressionist_count({:start_date=>start_date, :end_date=>end_date, :filter=> :request_hash})
-    end
-
-    def unique_impression_count_ip(start_date=nil,end_date=Time.now)
-      impressionist_count({:start_date=>start_date, :end_date=>end_date, :filter=> :ip_address})
-    end
-
-    def unique_impression_count_session(start_date=nil,end_date=Time.now)
-      impressionist_count({:start_date=>start_date, :end_date=>end_date, :filter=> :session_hash})
-    end
   end
+
 end

lib/impressionist.rb

@@ -1,4 +1,4 @@
-require 'impressionist/engine'
+require 'impressionist/load'
 
 module Impressionist
   # Define default ORM

lib/impressionist/counter_cache.rb

@@ -0,0 +1,83 @@
+module Impressionist
+  module CounterCache
+
+    attr_reader :impressionable_class, :entity
+
+    private
+      LOG_MESSAGE = "Can't find impressionable_type or impressionable_id. Will not update_counters!"
+
+      # if updatable returns true, it must be qualified to update_counters
+      # Therefore there's no need to validate again
+      # impressionable_class instance var is set when updatable? is called
+      def impressionable_counter_cache_updatable?
+        updatable? ? update_impression_cache : impressionist_log(LOG_MESSAGE)
+      end
+
+      def update_impression_cache
+        impressionable_find
+        impressionable_try
+      end
+
+      # asks imp_id whether it's present or not
+      # also expect imp_class to be true
+      # all should be true, so that it is updatable
+      def updatable?
+        @impressionable_class = impressionable_class_set
+        impressionable_valid?
+      end
+
+      # imps_type == nil, constantize returns Object
+      # Therefore it attemps to return false so it won't be updatable
+      # calls to_s otherwise it would try to constantize nil
+      # and it would raise an exeception..
+      def impressionable_class_set
+        _type_ = self.impressionable_type.to_s.constantize
+        ((_type_.to_s !~ /Object/) && _type_.impressionist_counter_caching? ? _type_ : false)
+      end
+
+      # Either true or false
+      # needs true to be updatable
+      def impressionable_valid?
+        (self.impressionable_id.present? && impressionable_class && impressionable_find)
+      end
+
+      # Logs to log file, expects a message to be passed
+      # default mode is ERROR
+      def impressionist_log(mode=:error, str)
+        Rails.logger.send(mode.to_s, str)
+      end
+
+      # read it out and LOUD
+      def impressionable_find
+        exeception_rescuer {
+          @entity = impressionable_class.find(self.impressionable_id)
+        }
+        @entity
+
+      end
+
+      # receives an entity(instance of a Model) and then tries to update
+      # counter_cache column
+      # entity is a impressionable_model
+      def impressionable_try
+        entity.try(:update_impressionist_counter_cache)
+      end
+
+      # Returns false, as it is only handling one exeception
+      # It would make updatable to fail thereafter it would not try
+      # to update cache_counter
+      def exeception_rescuer
+          begin
+            yield
+          rescue ActiveRecord::RecordNotFound
+            exeception_to_log
+            false
+          end
+      end
+
+      def exeception_to_log
+        impressionist_log("Couldn't find Widget with id=#{self.impressionable_id}")
+      end
+
+  end
+end

lib/impressionist/load.rb

@@ -0,0 +1,7 @@
+require "impressionist/engine"
+
+require "impressionist/set_up_association"
+
+require "impressionist/counter_cache"
+
+require "impressionist/update_counters"

lib/impressionist/models/active_record/impression.rb

@@ -1,21 +1,13 @@
+# Responsability
+# * be able to update_counters
+# * log an error if imps_id and imps_type can not be found
+# asks updatable? whether it may or may not be updated
+# FIX exeception raising when no imps_id is found
+
 class Impression < ActiveRecord::Base
-  attr_accessible :impressionable_type, :impressionable_id, :user_id,
+  include Impressionist::CounterCache
-  :controller_name, :action_name, :view_name, :request_hash, :ip_address,
+  include Impressionist::SetUpAssociation 
-  :session_hash, :message, :referrer
 
-  belongs_to :impressionable, :polymorphic=>true
+  after_save :impressionable_counter_cache_updatable?
 
-  after_save :update_impressions_counter_cache
-
-  private
-
-  def update_impressions_counter_cache
-    if self.impressionable_type && self.impressionable_id
-      impressionable_class = self.impressionable_type.constantize
-      if impressionable_class.impressionist_counter_cache_options
-        resouce = impressionable_class.find(self.impressionable_id)
-        resouce.try(:update_impressionist_counter_cache)
-      end
-    end
-  end
 end

lib/impressionist/models/active_record/impressionist/impressionable.rb

@@ -2,13 +2,27 @@ ActiveRecord::Base.send(:include, Impressionist::Impressionable)
 
 module Impressionist
   module Impressionable
+
     extend ActiveSupport::Concern
 
     module ClassMethods
       def is_impressionable(options={})
-        has_many :impressions, :as => :impressionable, :dependent => :destroy
+        define_association
-        @impressionist_cache_options = options[:counter_cache]
+        imp_cache_options_set(options)
       end
+
+      def define_association
+        has_many(:impressions,
+        :as => :impressionable,
+        :dependent => :destroy)
+      end
+
+      def imp_cache_options_set(options)
+        @impressionist_cache_options = options
+      end
+
     end
+
   end
+
 end

lib/impressionist/set_up_association.rb

@@ -0,0 +1,14 @@
+module Impressionist
+  module SetUpAssociation
+
+    def self.included(base)
+      base.attr_accessible(:impressionable_type,:impressionable_id,
+      :user_id,:controller_name,:action_name,:view_name,:request_hash,
+      :ip_address,:session_hash,:message,:referrer)
+
+      base.belongs_to(:impressionable, :polymorphic => true)
+    end
+
+  end
+
+end

lib/impressionist/update_counters.rb

@@ -0,0 +1,63 @@
+module Impressionist
+  class UpdateCounters
+    attr_reader :receiver, :master
+
+    def initialize(receiver)
+      @receiver = receiver
+      @master = receiver.class
+    end
+
+    def update
+      result = (impressions_total - impressions_cached)
+
+      master.
+      update_counters(id, column_name => result)
+    end
+
+    private
+      def id
+        receiver.id
+      end
+
+      # if unique == true then uses it
+      # otherwise just count all impressions
+      # using filter: :all
+      def impressions_total
+        receiver.impressionist_count filter(unique_filter)
+      end
+
+      # from a given db column
+      # default should be impressions_count
+      def impressions_cached
+        receiver.send(column_name) || 0
+      end
+
+      def column_name
+        cache_options[:column_name].to_sym
+      end
+
+      def cache_options
+        master. 
+        impressionist_counter_cache_options
+      end 
+
+      # default is ip_address if what_is_unique is TRUE or FALSE 
+      def unique_filter
+       what_is_unique? ? :ip_address : cache_options[:unique]
+      end
+
+      # Either true or false
+      # :filter gets assigned to :ip_address as default
+      # One could do
+      # is_impressionable :counter_cache => true, :unique => :any_other_colum
+      def what_is_unique?
+        cache_options[:unique].to_s =~ /true|false/
+      end
+
+      def filter(filter_will_be)
+        {:filter => filter_will_be.to_sym}
+      end
+
+  end
+
+end

test_app/Gemfile

@@ -1,6 +1,6 @@
 source 'https://rubygems.org'
 
-gem 'rails', '3.2.2'
+gem 'rails', '3.2.12'
 
 gem 'impressionist', :path => '../'
 
@@ -41,7 +41,7 @@ group :test do
   gem 'systemu'
 end
 
-#gem 'jquery-rails'
+gem 'jquery-rails'
 
 # To use ActiveModel has_secure_password
 # gem 'bcrypt-ruby', '~> 3.0.0'

test_app/app/models/widget.rb

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

test_app/db/migrate/20111127184039_create_widgets.rb

@@ -2,7 +2,7 @@ class CreateWidgets < ActiveRecord::Migration
   def self.up
     create_table :widgets do |t|
       t.string :name
-      t.integer :impressions_count
+      t.integer :impressions_count, :default => 0
 
       t.timestamps
     end

test_app/spec/controllers/controller_spec.rb

@@ -131,4 +131,4 @@ describe DummyController do
     get "index"
     Impression.all.size.should eq 12
   end
-end
+end

test_app/spec/models/counter_caching_spec.rb

@@ -3,8 +3,9 @@ require 'spec_helper'
 describe Impression do
   fixtures :widgets
 
+  let(:widget) { Widget.find(1) }
+
   before(:each) do
-    @widget = Widget.find(1)
     Impression.destroy_all
   end
 
@@ -33,17 +34,17 @@ describe Impression do
 
   describe "#update_impressionist_counter_cache" do
     it "should update the counter cache column to reflect the correct number of impressions" do
-      lambda {
+      expect {
-         @widget.impressions.create(:request_hash => 'abcd1234')
+         widget.impressions.create(:request_hash => 'abcd1234')
-         @widget.reload
+         widget.reload
-       }.should change(@widget, :impressions_count).from(0).to(1)
+       }.to change(widget, :impressions_count).from(0).to(1)
     end
 
     it "should not update the timestamp on the impressable" do
-      lambda {
+      expect {
-         @widget.impressions.create(:request_hash => 'abcd1234')
+         widget.impressions.create(:request_hash => 'abcd1234')
-         @widget.reload
+         widget.reload
-       }.should_not change(@widget, :updated_at)
+       }.to_not change(widget, :updated_at)
     end
   end
 

test_app/spec/models/model_spec.rb

@@ -51,7 +51,7 @@ describe Impression do
 
   # tests :dependent => :destroy
   it "should delete impressions on deletion of impressionable" do
-    impressions_count = Impression.all.size
+    #impressions_count = Impression.all.size
     a = Article.create
     i = a.impressions.create
     a.destroy
@@ -59,36 +59,4 @@ describe Impression do
     i.destroyed?.should be_true
   end
 
-  #OLD COUNT METHODS.  DEPRECATE SOON
-  it "should return the impression count with no date range specified" do
-    @article.impression_count.should eq 11
-  end
-
-  it "should return unique impression count with no date range specified" do
-    @article.unique_impression_count.should eq 9
-  end
-
-  it "should return impression count with only start date specified" do
-    @article.impression_count("2011-01-01").should eq 8
-  end
-
-  it "should return impression count with whole date range specified" do
-    @article.impression_count("2011-01-01","2011-01-02").should eq 7
-  end
-
-  it "should return unique impression count with only start date specified" do
-    @article.unique_impression_count("2011-01-01").should eq 7
-  end
-
-  it "should return unique impression count with date range specified" do
-    @article.unique_impression_count("2011-01-01","2011-01-02").should eq 7
-  end
-
-  it "should return unique impression count using ip address (which in turn eliminates duplicate request_hashes)" do
-    @article.unique_impression_count_ip.should eq 8
-  end
-
-  it "should return unique impression count using session_hash (which in turn eliminates duplicate request_hashes)" do
-    @article.unique_impression_count_session.should eq 7
-  end
 end