From 034af8d9873e4c9fa051d6a326a69435dd11e06e Mon Sep 17 00:00:00 2001 From: Antonio C Nalesso Moreira Date: Sat, 6 Jul 2013 03:14:25 +0100 Subject: [PATCH] FIX #92, Allow options to be passed to counter_cache unique, and clean up --- app/models/impressionist/impressionable.rb | 54 +++++------- lib/impressionist.rb | 2 +- lib/impressionist/counter_cache.rb | 83 +++++++++++++++++++ lib/impressionist/load.rb | 7 ++ .../models/active_record/impression.rb | 26 ++---- .../impressionist/impressionable.rb | 18 +++- lib/impressionist/set_up_association.rb | 14 ++++ lib/impressionist/update_counters.rb | 63 ++++++++++++++ test_app/Gemfile | 4 +- test_app/app/models/widget.rb | 2 +- .../migrate/20111127184039_create_widgets.rb | 2 +- test_app/spec/controllers/controller_spec.rb | 2 +- test_app/spec/models/counter_caching_spec.rb | 19 +++-- test_app/spec/models/model_spec.rb | 34 +------- 14 files changed, 228 insertions(+), 102 deletions(-) create mode 100644 lib/impressionist/counter_cache.rb create mode 100644 lib/impressionist/load.rb create mode 100644 lib/impressionist/set_up_association.rb create mode 100644 lib/impressionist/update_counters.rb diff --git a/app/models/impressionist/impressionable.rb b/app/models/impressionist/impressionable.rb index b416305..9385646 100644 --- a/app/models/impressionist/impressionable.rb +++ b/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 - options = { :column_name => :impressions_count, :unique => false } - options.merge!(@impressionist_cache_options) if @impressionist_cache_options.is_a?(Hash) - options - end + @impressionist_cache_options ||= {} + @impressionist_cache_options.reverse_merge!(DEFAULT_CACHE) 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") - impressionist_counter_caching? + ::ActiveSupport::Deprecation.warn("#counter_caching? is deprecated; please use #impressionist_counter_caching? instead") + impressionist_counter_caching? end + end - def impressionable? - true - end - + # ------------------------------------------ + # TODO: CLEAN UP, make it HUMAN readable 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 - cache_options = self.class.impressionist_counter_cache_options - column_name = cache_options[:column_name].to_sym - count = cache_options[:unique] ? impressionist_count(:filter => :ip_address) : impressionist_count - old_count = send(column_name) || 0 - self.class.update_counters(id, column_name => (count - old_count)) + def update_impressionist_counter_cache + slave = Impressionist::UpdateCounters.new(self) + slave.update + end + + 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 diff --git a/lib/impressionist.rb b/lib/impressionist.rb index 8e4c7a5..6623ac1 100644 --- a/lib/impressionist.rb +++ b/lib/impressionist.rb @@ -1,4 +1,4 @@ -require 'impressionist/engine' +require 'impressionist/load' module Impressionist # Define default ORM diff --git a/lib/impressionist/counter_cache.rb b/lib/impressionist/counter_cache.rb new file mode 100644 index 0000000..4a7e78f --- /dev/null +++ b/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 diff --git a/lib/impressionist/load.rb b/lib/impressionist/load.rb new file mode 100644 index 0000000..dfeac3a --- /dev/null +++ b/lib/impressionist/load.rb @@ -0,0 +1,7 @@ +require "impressionist/engine" + +require "impressionist/set_up_association" + +require "impressionist/counter_cache" + +require "impressionist/update_counters" diff --git a/lib/impressionist/models/active_record/impression.rb b/lib/impressionist/models/active_record/impression.rb index f211b98..abf6267 100644 --- a/lib/impressionist/models/active_record/impression.rb +++ b/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, - :controller_name, :action_name, :view_name, :request_hash, :ip_address, - :session_hash, :message, :referrer + include Impressionist::CounterCache + include Impressionist::SetUpAssociation - 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 diff --git a/lib/impressionist/models/active_record/impressionist/impressionable.rb b/lib/impressionist/models/active_record/impressionist/impressionable.rb index 4db6979..d2226ad 100644 --- a/lib/impressionist/models/active_record/impressionist/impressionable.rb +++ b/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 - @impressionist_cache_options = options[:counter_cache] + define_association + 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 diff --git a/lib/impressionist/set_up_association.rb b/lib/impressionist/set_up_association.rb new file mode 100644 index 0000000..9aee85d --- /dev/null +++ b/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 diff --git a/lib/impressionist/update_counters.rb b/lib/impressionist/update_counters.rb new file mode 100644 index 0000000..3b4a445 --- /dev/null +++ b/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 diff --git a/test_app/Gemfile b/test_app/Gemfile index 9c0e90c..65a819a 100644 --- a/test_app/Gemfile +++ b/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' diff --git a/test_app/app/models/widget.rb b/test_app/app/models/widget.rb index 51bab18..0d3e23e 100644 --- a/test_app/app/models/widget.rb +++ b/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 diff --git a/test_app/db/migrate/20111127184039_create_widgets.rb b/test_app/db/migrate/20111127184039_create_widgets.rb index 5cb1fe2..3305dbc 100644 --- a/test_app/db/migrate/20111127184039_create_widgets.rb +++ b/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 diff --git a/test_app/spec/controllers/controller_spec.rb b/test_app/spec/controllers/controller_spec.rb index 7f1307a..aa06c29 100644 --- a/test_app/spec/controllers/controller_spec.rb +++ b/test_app/spec/controllers/controller_spec.rb @@ -131,4 +131,4 @@ describe DummyController do get "index" Impression.all.size.should eq 12 end -end \ No newline at end of file +end diff --git a/test_app/spec/models/counter_caching_spec.rb b/test_app/spec/models/counter_caching_spec.rb index 2f9636d..936f347 100644 --- a/test_app/spec/models/counter_caching_spec.rb +++ b/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 { - @widget.impressions.create(:request_hash => 'abcd1234') - @widget.reload - }.should change(@widget, :impressions_count).from(0).to(1) + expect { + widget.impressions.create(:request_hash => 'abcd1234') + widget.reload + }.to change(widget, :impressions_count).from(0).to(1) end it "should not update the timestamp on the impressable" do - lambda { - @widget.impressions.create(:request_hash => 'abcd1234') - @widget.reload - }.should_not change(@widget, :updated_at) + expect { + widget.impressions.create(:request_hash => 'abcd1234') + widget.reload + }.to_not change(widget, :updated_at) end end diff --git a/test_app/spec/models/model_spec.rb b/test_app/spec/models/model_spec.rb index 5460166..d0c21e2 100644 --- a/test_app/spec/models/model_spec.rb +++ b/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