Refactor extension.

lib/refinery/page_images/extension.rb

@@ -1,58 +1,45 @@
 module Refinery
   module PageImages
     module Extension
-      def self.included(base)
-        base.extend(ClassMethods)
-      end
-
-      module ClassMethods
-        def has_many_page_images
-          has_many :image_pages, :as => :page, :order => 'position ASC'
-          has_many :images, :through => :image_pages, :order => 'position ASC'
-          # accepts_nested_attributes_for MUST come before def images_attributes=
-          # this is because images_attributes= overrides accepts_nested_attributes_for.
-
-          accepts_nested_attributes_for :images, :allow_destroy => false
-          module_eval do ## need to do it this way because of the way accepts_nested_attributes_for deletes an already defined images_attributes
-            def images_attributes=(data)
-              ids_to_delete = data.map{|i, d| d['image_page_id']}.compact
-              self.image_pages.each do |image_page|
-                if ids_to_delete.index(image_page.id.to_s).nil?
-                  # Image has been removed, we must delete it
-                  self.image_pages.delete(image_page)
-                  image_page.destroy
-                end
+      def has_many_page_images
+        has_many :image_pages, :as => :page, :order => 'position ASC'
+        has_many :images, :through => :image_pages, :order => 'position ASC'
+        # accepts_nested_attributes_for MUST come before def images_attributes=
+        # this is because images_attributes= overrides accepts_nested_attributes_for.
+
+        accepts_nested_attributes_for :images, :allow_destroy => false
+
+        # need to do it this way because of the way accepts_nested_attributes_for
+        # deletes an already defined images_attributes
+        module_eval do
+          def images_attributes=(data)
+            ids_to_keep = data.map{|i, d| d['image_page_id']}.compact
+            self.image_pages.where(
+              Refinery::ImagePage.arel_table[:id].not_in(ids_to_keep)
+            ).destroy_all
+
+            data.each do |i, image_data|
+              image_page_id, image_id, caption =
+                image_data.values_at('image_page_id', 'id', 'caption')
+
+              next if image_id.blank?
+
+              image_page = if image_page_id.present?
+                self.image_pages.find(image_page_id)
+              else
+                self.image_pages.build(:image_id => image_id)
               end
 
-              (0..(data.length-1)).each do |i|
-                unless (image_data = data[i.to_s]).nil? or image_data['id'].blank?
-                  image_page = if image_data['image_page_id'].present?
-                    self.image_pages.find(image_data['image_page_id'])
-                  else
-                    self.image_pages.new(:image_id => image_data['id'].to_i)
-                  end
-                  image_page.position = i
-                  # Add caption if supported
-                  image_page.caption = image_data['caption'] if Refinery::PageImages.config.captions
-
-                  self.image_pages << image_page if image_data['image_page_id'].blank?
-                  image_page.save
-                end
-              end
+              image_page.position = i
+              image_page.caption = caption if Refinery::PageImages.config.captions
+              image_page.save
             end
           end
+        end
 
-          include Refinery::PageImages::Extension::InstanceMethods
+        include Refinery::PageImages::Extension::InstanceMethods
 
-          # this wasn't working for me so I've commented it out for now
-          # if ActiveModel::MassAssignmentSecurity::WhiteList === active_authorizer
-          #   attr_accessible :images_attributes
-          # else
-          #   # to prevent a future call to attr_accessible
-          #   self._accessible_attributes = accessible_attributes + [:images_attributes]
-          # end
-          attr_accessible :images_attributes
-        end
+        attr_accessible :images_attributes
       end
 
       module InstanceMethods
@@ -69,4 +56,4 @@ def image_page_id_for_image_index(index)
   end
 end
 
-ActiveRecord::Base.send(:include, Refinery::PageImages::Extension)
+ActiveRecord::Base.send(:extend, Refinery::PageImages::Extension)

spec/models/refinery/page_spec.rb

@@ -2,19 +2,58 @@
 
 module Refinery
   describe Page do
-    it "should have an image" do
-      p = Factory(:page_with_image)
-      p.images.count.should == 1
+    it "can have images added" do
+      page = Factory(:page)
+      page.images.count.should eq(0)
+
+      page.images << Factory(:image)
+      page.images.count.should eq(1)
     end
 
-    it "should accept images_attributes=" do
-      page = Factory(:page)
-      image = Factory(:image)
+    describe "#images_attributes=" do
+      it "adds images" do
+        page = Factory(:page)
+        image = Factory(:image)
+
+        page.images.count.should == 0
+        page.update_attributes({:images_attributes => {"0" => {"id" => image.id}}})
+
+        page.images.count.should == 1
+      end
+
+      it "deletes images" do
+        page = Factory(:page)
+        images = [Factory(:image), Factory(:image)]
+        page.images = images
+
+        page.update_attributes(:images_attributes => {
+          "0" => {
+            "id" => images.first.id.to_s,
+            "image_page_id" => page.image_pages.first.id,
+          },
+        })
+
+        page.images.should eq([images.first])
+      end
+
+      it "reorders images" do
+        page = Factory(:page)
+        images = [Factory(:image), Factory(:image)]
+        page.images = images
 
-      page.images.count.should == 0
-      page.update_attributes({:images_attributes => {"0" => {"id" => image.id}}})
+        page.update_attributes(:images_attributes => {
+          "0" => {
+            "id" => images.second.id,
+            "image_page_id" => page.image_pages.second.id,
+          },
+          "1" => {
+            "id" => images.first.id,
+            "image_page_id" => page.image_pages.first.id,
+          },
+        })
 
-      page.images.count.should == 1
+        page.images.should eq([images.second, images.first])
+      end
     end
   end
 end