Merge pull request #338 from algolia/fix/changed-methods

Fix _changed? BC break and introduce algolia_dirty? method

lib/algoliasearch-rails.rb

@@ -739,19 +739,21 @@ def algolia_index_name(options = nil)
     end
 
     def algolia_must_reindex?(object)
+      # use +algolia_dirty?+ method if implemented
+      return object.send(:algolia_dirty?) if (object.respond_to?(:algolia_dirty?))
+      # Loop over each index to see if a attribute used in records has changed
       algolia_configurations.each do |options, settings|
         next if options[:slave] || options[:replica]
         return true if algolia_object_id_changed?(object, options)
         settings.get_attribute_names(object).each do |k|
-          changed_method = attribute_changed_method(k)
-          return true if !object.respond_to?(changed_method) || object.send(changed_method)
+          return true if algolia_attribute_changed?(object, k)
+          # return true if !object.respond_to?(changed_method) || object.send(changed_method)
         end
         [options[:if], options[:unless]].each do |condition|
           case condition
           when nil
           when String, Symbol
-            changed_method = attribute_changed_method(condition)
-            return true if !object.respond_to?(changed_method) || object.send(changed_method)
+            return true if algolia_attribute_changed?(object, condition)
           else
             # if the :if, :unless condition is a anything else,
             # we have no idea whether we should reindex or not
@@ -760,6 +762,7 @@ def algolia_must_reindex?(object)
           end
         end
       end
+      # By default, we don't reindex
       return false
     end
 
@@ -825,8 +828,8 @@ def algolia_object_id_of(o, options = nil)
     end
 
     def algolia_object_id_changed?(o, options = nil)
-      m = attribute_changed_method(algolia_object_id_method(options))
-      o.respond_to?(m) ? o.send(m) : false
+      changed = algolia_attribute_changed?(o, algolia_object_id_method(options))
+      changed.nil? ? false : changed
     end
 
     def algoliasearch_settings_changed?(prev, current)
@@ -920,13 +923,48 @@ def algolia_find_in_batches(batch_size, &block)
       end
     end
 
-    def attribute_changed_method(attr)
-      if defined?(::ActiveRecord) && ActiveRecord::VERSION::MAJOR >= 5 && ActiveRecord::VERSION::MINOR >= 1 ||
-          (defined?(::ActiveRecord) && ActiveRecord::VERSION::MAJOR > 5)
-        "will_save_change_to_#{attr}?"
-      else
-        "#{attr}_changed?"
+    def algolia_attribute_changed?(object, attr_name)
+      # if one of two method is implemented, we return its result
+      # true/false means whether it has changed or not
+      # +#{attr_name}_changed?+ always defined for automatic attributes but deprecated after Rails 5.2
+      # +will_save_change_to_#{attr_name}?+ should be use instead for Rails 5.2+, also defined for automatic attributes.
+      # If none of the method are defined, it's a dynamic attribute
+
+      method_name = "#{attr_name}_changed?"
+      if object.respond_to?(method_name)
+        # If +#{attr_name}_changed?+ respond we want to see if the method is user defined or if it's automatically
+        # defined by Rails.
+        # If it's user-defined, we call it.
+        # If it's automatic we check ActiveRecord version to see if this method is deprecated
+        # and try to call +will_save_change_to_#{attr_name}?+ instead.
+        # See: https://github.com/algolia/algoliasearch-rails/pull/338
+        # This feature is not compatible with Ruby 1.8
+        # In this case, we always call #{attr_name}_changed?
+        if Object.const_defined?(:RUBY_VERSION) && RUBY_VERSION.to_f < 1.9
+          return object.send(method_name)
+        end
+        unless automatic_changed_method?(object, method_name) && automatic_changed_method_deprecated?
+          return object.send(method_name)
+        end
       end
+
+      if object.respond_to?("will_save_change_to_#{attr_name}?")
+        return object.send("will_save_change_to_#{attr_name}?")
+      end
+
+      # Nil means we don't know if the attribute has changed
+      nil
+    end
+
+    def automatic_changed_method?(object, method_name)
+      raise ArgumentError.new("Method #{method_name} doesn't exist on #{object.class.name}") unless object.respond_to?(method_name)
+      file = object.method(method_name).source_location[0]
+      file.end_with?("active_model/attribute_methods.rb")
+    end
+
+    def automatic_changed_method_deprecated?
+      (defined?(::ActiveRecord) && ActiveRecord::VERSION::MAJOR >= 5 && ActiveRecord::VERSION::MINOR >= 1) ||
+          (defined?(::ActiveRecord) && ActiveRecord::VERSION::MAJOR > 5)
     end
   end
 

spec/integration_spec.rb

@@ -83,6 +83,12 @@
     t.boolean :premium
     t.boolean :released
   end
+  create_table :ebooks do |t|
+    t.string :name
+    t.string :author
+    t.boolean :premium
+    t.boolean :released
+  end
   create_table :disabled_booleans do |t|
     t.string :name
   end
@@ -155,6 +161,7 @@ class Camera < Product
 
 class Color < ActiveRecord::Base
   include AlgoliaSearch
+  attr_accessor :not_indexed
 
   algoliasearch :synchronous => true, :index_name => safe_index_name("Color"), :per_environment => true do
     attributesToIndex [:name]
@@ -166,6 +173,14 @@ class Color < ActiveRecord::Base
 
     # we're using all attributes of the Color class + the _tag "extra" attribute
   end
+
+  def hex_changed?
+    false
+  end
+
+  def will_save_change_to_short_name?
+    false
+  end
 end
 
 class DisabledBoolean < ActiveRecord::Base
@@ -372,6 +387,22 @@ def public?
   end
 end
 
+class Ebook < ActiveRecord::Base
+  include AlgoliaSearch
+  attr_accessor :current_time, :published_at
+
+  algoliasearch :synchronous => true, :index_name => safe_index_name("eBooks")do
+    attributesToIndex [:name]
+  end
+
+  def algolia_dirty?
+    return true if self.published_at.nil? || self.current_time.nil?
+    # Consider dirty if published date is in the past
+    # This doesn't make so much business sense but it's easy to test.
+    self.published_at < self.current_time
+  end
+end
+
 class EncodedString < ActiveRecord::Base
   include AlgoliaSearch
 
@@ -532,6 +563,56 @@ class SerializedObject < ActiveRecord::Base
 
 end
 
+describe 'Change detection' do
+
+  it "should detect attribute changes" do
+    color = Color.new :name => "dark-blue", :short_name => "blue"
+
+    Color.algolia_must_reindex?(color).should == true
+    color.save
+    Color.algolia_must_reindex?(color).should == false
+
+    color.hex = 123456
+    Color.algolia_must_reindex?(color).should == false
+
+    color.not_indexed = "strstr"
+    Color.algolia_must_reindex?(color).should == false
+    color.name = "red"
+    Color.algolia_must_reindex?(color).should == true
+
+    color.delete
+  end
+
+  it "should detect change with algolia_dirty? method" do
+    ebook = Ebook.new :name => "My life", :author => "Myself", :premium => false, :released => true
+
+    Ebook.algolia_must_reindex?(ebook).should == true # Because it's defined in algolia_dirty? method
+    ebook.current_time = 10
+    ebook.published_at = 8
+    Ebook.algolia_must_reindex?(ebook).should == true
+    ebook.published_at = 12
+    Ebook.algolia_must_reindex?(ebook).should == false
+  end
+
+  it "should know if the _changed? method is user-defined", :skip => Object.const_defined?(:RUBY_VERSION) && RUBY_VERSION.to_f < 1.9 do
+    color = Color.new :name => "dark-blue", :short_name => "blue"
+
+    expect { Color.send(:automatic_changed_method?, color, :something_that_doesnt_exist) }.to raise_error(ArgumentError)
+
+    Color.send(:automatic_changed_method?, color, :name_changed?).should == true
+    Color.send(:automatic_changed_method?, color, :hex_changed?).should == false
+
+    Color.send(:automatic_changed_method?, color, :will_save_change_to_short_name?).should == false
+
+    if Color.send(:automatic_changed_method_deprecated?)
+      Color.send(:automatic_changed_method?, color, :will_save_change_to_name?).should == true
+      Color.send(:automatic_changed_method?, color, :will_save_change_to_hex?).should == true
+    end
+
+  end
+
+end
+
 describe 'Namespaced::Model' do
   it "should have an index name without :: hierarchy" do
     Namespaced::Model.index_name.should == "Namespaced_Model"