#311 Don't insert invalid ActiveRecord objects (#312)

* #311 Don't insert invalid ActiveRecord objects.
* Add insert_at!

lib/acts_as_list/active_record/acts/list.rb

@@ -69,6 +69,10 @@ def insert_at(position = acts_as_list_top)
           insert_at_position(position)
         end
 
+        def insert_at!(position = acts_as_list_top)
+          insert_at_position(position, true)
+        end
+
         # Swap positions with the next lower item, if one exists.
         def move_lower
           return unless lower_item
@@ -208,9 +212,9 @@ def default_position?
         end
 
         # Sets the new position and saves it
-        def set_list_position(new_position)
+        def set_list_position(new_position, raise_exception_if_save_fails=false)
           write_attribute position_column, new_position
-          save(validate: false)
+          raise_exception_if_save_fails ? save! : save
         end
 
         private
@@ -394,8 +398,8 @@ def shuffle_positions_on_intermediate_items(old_position, new_position, avoid_id
           end
         end
 
-        def insert_at_position(position)
-          return set_list_position(position) if new_record?
+        def insert_at_position(position, raise_exception_if_save_fails=false)
+          return set_list_position(position, raise_exception_if_save_fails) if new_record?
           with_lock do
             if in_list?
               old_position = send(position_column).to_i
@@ -404,12 +408,12 @@ def insert_at_position(position)
               # gap is required to leave room for position increments
               # positive number will be valid with unique not null check (>= 0) db constraint
               temporary_position = bottom_position_in_list + 2
-              set_list_position(temporary_position)
+              set_list_position(temporary_position, raise_exception_if_save_fails)
               shuffle_positions_on_intermediate_items(old_position, position, id)
             else
               increment_positions_on_lower_items(position)
             end
-            set_list_position(position)
+            set_list_position(position, raise_exception_if_save_fails)
           end
         end
 

test/shared_list.rb

@@ -293,8 +293,22 @@ def test_before_create_callback_adds_to_given_position
 
     def test_non_persisted_records_dont_get_lock_called
       new = ListMixin.new(parent_id: 5)
-
       new.destroy
     end
+
+    def test_invalid_records_dont_get_inserted
+      new = ListMixinError.new(parent_id: 5, state: nil)
+      assert !new.valid?
+      new.insert_at(1)
+      assert !new.persisted?
+    end
+
+    def test_invalid_records_raise_error_with_insert_at!
+      new = ListMixinError.new(parent_id: 5, state: nil)
+      assert !new.valid?
+      assert_raises ActiveRecord::RecordInvalid do
+        new.insert_at!(1)
+      end
+    end
   end
 end

test/test_list.rb

@@ -74,6 +74,14 @@ class ListMixinSub2 < ListMixin
   end
 end
 
+class ListMixinError < ListMixin
+  if rails_3
+    validates :state, presence: true
+  else
+    validates_presence_of :state
+  end
+end
+
 class ListWithStringScopeMixin < Mixin
   acts_as_list column: "pos", scope: 'parent_id = #{parent_id}'
 end