FIX #97, and refactored

app/models/impressionist/impressionable.rb

@@ -12,7 +12,7 @@ module Impressionist
         @impressionist_cache_options.reverse_merge!(DEFAULT_CACHE)
       end
 
-      # true or false
+      # asks impressionable entity whether or not it is counter_caching
       def impressionist_counter_caching?
         impressionist_counter_cache_options[:counter_cache]
       end
@@ -22,7 +22,7 @@ module Impressionist
           impressionist_counter_caching?
       end
 
-    end
+    end # end of ClassMethods
 
     # ------------------------------------------
     # TODO: CLEAN UP, make it HUMAN readable

lib/impressionist/counter_cache.rb

@@ -1,55 +1,34 @@
+# Note
+# It is only updatable if
+# impressionist_id && impressionist_type(class) are present &&
+# impressionable_class(which is imp_type.constantize) is counter_caching
+# Defined like so
+# is_impressionable :counter_cache => true
+
 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)
+         updatable? && impressionable_try
       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
+      # asks imp_id && imp_class whether it's present or not
+      # 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)
+        (self.impressionable_id.present? && impressionable_class)
       end
 
-      # Logs to log file, expects a message to be passed
-
-      # default mode is ERROR
-      # ruby 1.8.7 support
-      def impressionist_log(str, mode=:error)
-        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)
@@ -58,10 +37,27 @@ module Impressionist
 
       end
 
+      # imps_type == nil, constantize returns Object
+      # 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
+      # Must be !~ Object and be counter_caching to be qualified as updatable
+      def impressionable_class_set
+        klass = self.impressionable_type.to_s.constantize
+        (klass.to_s !~ /Object/) && klass.impressionist_counter_caching? ? klass : false
+      end
+
+      # default mode is ERROR
+      # ruby 1.8.7 support
+      def impressionist_log(str, mode=:error)
+        Rails.logger.send(mode.to_s, str)
+      end
+
       # receives an entity(instance of a Model) and then tries to update
       # counter_cache column
-      # entity is a impressionable_model
+      # entity is a impressionable instance model
       def impressionable_try
+        impressionable_find
         entity.try(:update_impressionist_counter_cache)
       end
 

lib/impressionist/is_impressionable.rb

@@ -0,0 +1,23 @@
+module Impressionist
+  module IsImpressionable
+    extend ActiveSupport::Concern
+
+    module ClassMethods
+      def is_impressionable(options={})
+        define_association
+        @impressionist_cache_options = options
+
+        true
+      end
+
+      private
+
+      def define_association
+        has_many(:impressions,
+        :as => :impressionable,
+        :dependent => :destroy)
+      end
+    end
+  
+  end
+end

lib/impressionist/load.rb

@@ -1,5 +1,3 @@
-require 'impressionist/engine'
-
 require 'impressionist/setup_association'
 
 require 'impressionist/counter_cache'
@@ -7,3 +5,7 @@ require 'impressionist/counter_cache'
 require 'impressionist/update_counters'
 
 require 'impressionist/rails_toggle'
+
+require 'impressionist/is_impressionable'
+
+require 'impressionist/engine'

lib/impressionist/models/active_record/impression.rb

@@ -1,14 +1,12 @@
 # Responsability
-## See CounterCache TODO: extract it into a class
-# * be able to update_counters
-# * log an error if imps_id and imps_type can not be found
+# * logs 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
 
   include Impressionist::CounterCache
-  # sets belongs_to and attr_accessible
+
+  # sets belongs_to and attr_accessible depending on Rails version
   Impressionist::SetupAssociation.new(self).set
 
   after_save :impressionable_counter_cache_updatable?

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

@@ -1,26 +1,12 @@
-ActiveRecord::Base.send(:include, Impressionist::Impressionable)
-
 module Impressionist
+
   module Impressionable
-    extend ActiveSupport::Concern
 
-    module ClassMethods
-
-      def is_impressionable(options={})
-        define_association
-
-        @impressionist_cache_options = options
-      end
-
-private
-      def define_association
-        has_many(:impressions,
-        :as => :impressionable,
-        :dependent => :destroy)
+  # extends AS::Concern
+  include Impressionist::IsImpressionable
   end
 
 end
 
-  end
-
-end
+ActiveRecord::Base.
+send(:include, Impressionist::Impressionable)

lib/impressionist/models/mongoid/impressionist/impressionable.rb

@@ -1,31 +1,13 @@
-# TODO: Refactor this Entity
-# There's a lot of duplication
-Mongoid::Document.send(:include, Impressionist::Impressionable)
-
 module Impressionist
   module Impressionable
-    extend ActiveSupport::Concern
 
-    module ClassMethods
+  # extends AS::Concern
+  include Impressionist::IsImpressionable
 
-      def is_impressionable(options={})
-        define_association
-        @impressionist_cache_options = options
+    ## TODO: Make it readable
 
-        true
-      end
-
-        private
-          def define_association
-            has_many(:impressions,
-            :as => :impressionable,
-            :dependent => :destroy)
-          end
-
-    end
-
-    ##
-    # Overides active_record impressionist_count
+    # Overides impressionist_count in order to provied
+    # mongoid compability
     def impressionist_count(options={})
       options.reverse_merge!(:filter=>:request_hash, :start_date=>nil, :end_date=>Time.now)
       imps = options[:start_date].blank? ? impressions : impressions.between(created_at: options[:start_date]..options[:end_date])
@@ -34,5 +16,7 @@ module Impressionist
     end
 
   end
-
 end
+
+Mongoid::Document.
+send(:include, Impressionist::Impressionable)

lib/impressionist/update_counters.rb

@@ -1,61 +1,67 @@
+# Note
+# If impressionist_counter_cache_options[:counter_cache] is false(default)
+# it won't event run this class
 module Impressionist
+
   class UpdateCounters
-    attr_reader :receiver, :master
+    attr_reader :receiver, :klass
 
     def initialize(receiver)
       @receiver = receiver
-      @master = receiver.class
+      @klass = receiver.class
     end
 
     def update
-      result = (impressions_total - impressions_cached)
-
-      master.
+      klass.
       update_counters(id, column_name => result)
     end
 
     private
-      def id
-        receiver.id
+
+    def result
+      impressions_total - impressions_cached
     end
 
-      # if unique == true then uses it
-      # otherwise just count all impressions
-      # using filter: :all
+    # Count impressions based on unique_filter
+    # default is :ip_address when unique: true
     def impressions_total
-        receiver.impressionist_count filter(unique_filter)
+      receiver.impressionist_count filter
     end
 
-      # from a given db column
-      # default should be impressions_count
+    # Fetch impressions from a receiver's column
     def impressions_cached
       receiver.send(column_name) || 0
     end
 
+    def filter
+      {:filter => unique_filter}
+    end
+
+    # :filter gets assigned to :ip_address as default
+    # One could do
+    # is_impressionable :counter_cache => true,
+    # :unique => :any_other_filter
+    def unique_filter
+      Symbol === unique ?
+      unique :
+      :ip_address
+    end
+
+    def unique
+      cache_options[:unique]
+    end
+
     def column_name
-        cache_options[:column_name].to_sym
+      cache_options[:column_name].to_s
     end
 
     def cache_options
-        master. 
+      klass.
       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}
+    def id
+      receiver.id
     end
 
   end

tests/test_app/config/initializers/impression.rb

@@ -1,5 +1,8 @@
 # Use this hook to configure impressionist parameters
-Impressionist.setup do |config|
+#Impressionist.setup do |config|
   # Define ORM. Could be :active_record (default), :mongo_mapper or :mongoid
   # config.orm = :active_record
-end
+#end
+
+
+Impressionist.orm = :active_record

tests/test_app/db/migrate/20130719024021_create_impressions_table.rb

@@ -0,0 +1,30 @@
+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 :ip_address
+      t.string :session_hash
+      t.text :message
+      t.text :referrer
+      t.timestamps
+    end
+    add_index :impressions, [:impressionable_type, :message, :impressionable_id], :name => "impressionable_type_message_index", :unique => false, :length => {:message => 255 }
+    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
+    drop_table :impressions
+  end
+end