Fixes/duplicate nil field

app/serializers/no_cms/blocks/active_record_serializer.rb

@@ -21,7 +21,7 @@ def read_single_field
 
       # If there was nothing in the cache then we try to get the object id and
       # find the object in the database
-      field_id = self.container.fields_info.symbolize_keys["#{field}_id".to_sym]
+      field_id = self.container.fields_info.symbolize_keys[id_field]
 
       # Hstore serializes every field as a string, so we have to turn it into an
       # integer
@@ -48,7 +48,7 @@ def read_multiple_field
       return values if values
 
       # If there was nothing in the cache then we try to get the object ids
-      field_ids = self.container.fields_info.symbolize_keys["#{field}_ids".to_sym]
+      field_ids = self.container.fields_info.symbolize_keys[id_field]
 
       # Hstore serializes every field as a string, so we have to turn "[1, 2]"
       # to an actual array of integers
@@ -58,7 +58,7 @@ def read_multiple_field
 
       # If there's any id we try to get them from the database
       if field_ids.blank?
-        []
+        field_config[:type].none
       else
         values = field_config[:type].where(id: field_ids)
         self.container.cached_objects[field.to_sym] = values
@@ -83,7 +83,7 @@ def write_single_field value
         # We save in the objects cache the new value
         self.container.cached_objects[field.to_sym] = value
         # and then we store the new id in the fields_info hash
-        self.container.fields_info["#{field}_id".to_sym] = value.nil? ? nil : value.id
+        self.container.fields_info[id_field] = value.nil? ? nil : value.id
       else
         raise ArgumentError.new "Hash, ActiveRecord or nil expected for #{field} attribute"
       end
@@ -123,8 +123,28 @@ def write_multiple_field values
         end
       end
 
-      self.container.fields_info["#{field}_ids".to_sym] = self.container.cached_objects[field.to_sym].map(&:id)
+      self.container.fields_info[id_field] = self.container.cached_objects[field.to_sym].map(&:id)
 
     end
+
+    ##
+    # Active Record duplicate behaviour.
+    #
+    # If the field value is empty in the original block we just leave it empty
+    # in this block. Otherwise we fall to the default behaviour.
+    #
+    # This is due to the bug that caused the creation of new objects from nil
+    # records in the original block.
+    def duplicate
+
+      # We look for the _id or _ids field and also check wether we have the
+      # object in the cached obects (in case one has been initialized in the
+      # original block but not yet saved)
+      if !self.container.fields_info[id_field].blank? ||
+        !self.container.cached_objects[field.to_sym].blank?
+        super
+      end
+    end
+
   end
 end

app/serializers/no_cms/blocks/active_resource_serializer.rb

@@ -22,7 +22,7 @@ def read_single_field
 
       # If there was nothing in the cache then we try to get the object id and
       # find the object in the database
-      field_id = self.container.fields_info.symbolize_keys["#{field}_id".to_sym]
+      field_id = self.container.fields_info.symbolize_keys[id_field]
 
       # Hstore serializes every field as a string, so we have to turn it into an
       # integer
@@ -49,7 +49,7 @@ def read_multiple_field
       return values if values
 
       # If there was nothing in the cache then we try to get the object ids
-      field_ids = self.container.fields_info.symbolize_keys["#{field}_ids".to_sym]
+      field_ids = self.container.fields_info.symbolize_keys[id_field]
 
       # Hstore serializes every field as a string, so we have to turn "[1, 2]"
       # to an actual array of integers
@@ -86,7 +86,7 @@ def write_single_field value
         # We save in the objects cache the new value
         self.container.cached_objects[field.to_sym] = value
         # and then we store the new id in the fields_info hash
-        self.container.fields_info["#{field}_id".to_sym] = value.nil? ? nil : value.id
+        self.container.fields_info[id_field] = value.nil? ? nil : value.id
       end
 
       value
@@ -125,45 +125,54 @@ def write_multiple_field values
         end
       end
 
-
-
-      self.container.fields_info["#{field}_ids".to_sym] = self.container.cached_objects[field.to_sym].map(&:id)
+      self.container.fields_info[id_field] = self.container.cached_objects[field.to_sym].map(&:id)
 
     end
 
     ##
-    # We need to override this method for active resource in order to
-    # not find and save AR fields in blocks. When the field type is an
-    # ActiveRecord, it doesn't matter, but with ActiveResource we have
-    # multiple errors in duplication time
+    # We need to override the link behaviour this method for active resource in
+    # order to not find and save AR fields in blocks. When the field type is an
+    # ActiveRecord, it doesn't matter, but with ActiveResource we have multiple
+    # errors in duplication time
+    #
+    # And we also control the behaviour when the field value is empty in the
+    # original block. We just leave it empty in this block. Otherwise we fall to
+    # the default behaviour.
+    #
+    # This is due to the bug that caused the creation of new objects from nil
+    # records in the original block.
     def duplicate
-      dupped_value = case field_config[:duplicate]
-        # When dupping we just dup the object and expect it has the right
-        # behaviour. If it's nil we save nil (you can't dup NilClass)
-        when :dup
-          field_value = read
-          field_value.nil? ? nil : field_value.dup
-        # When nullifying we return nil
-        when :nullify
-          nil
-        # when linking we return the same object
-        when :link
-          if field_config[:multiple]
-            field_to_read = "#{field}_ids".to_sym
-            field_ids = self.container.fields_info.symbolize_keys[field_to_read]
-            field_ids.map{|id| field_config[:type].new(id: id)}
-          else
-            field_to_read = "#{field}_id".to_sym
-            self.container.fields_info.symbolize_keys[field_to_read]
-          end
-      end
-      write dupped_value
 
-      # We need to clear cached objects when duplicate block because when it saves
-      # it tries to save the cached objects. And we don't want this functionality when dup
-      # in active record, because there is nothing to save
+      # We look for the _id or _ids field and also check wether we have the
+      # object in the cached obects (in case one has been initialized in the
+      # original block but not yet saved)
+      if self.container.fields_info[id_field] ||
+        self.container.cached_objects[field.to_sym]
 
-      self.container.cached_objects.clear
+
+        if field_config[:duplicate] == :link
+          dupped_value = if field_config[:multiple]
+
+              # when linking we use the exact same objects if we already have
+              # them in the cached objects
+              if self.container.cached_objects[field.to_sym]
+                self.container.cached_objects[field.to_sym]
+              else
+                # Id we don't have them we save API calls by just creating
+                # persisted active resource objects with the right id
+                field_ids = self.container.fields_info.symbolize_keys[id_field]
+                field_ids.map{|id| field_config[:type].new({ id: id }, true )}
+              end
+            else
+              self.container.fields_info[id_field]
+            end
+
+          write dupped_value
+
+        else
+          super
+        end
+      end
     end
   end
 end

app/serializers/no_cms/blocks/base_multiple_serializer.rb

@@ -9,6 +9,9 @@ module NoCms::Blocks
   # the read and write methods.
   class BaseMultipleSerializer < BaseSerializer
 
+    def id_field
+      field_config[:multiple] ? "#{field}_ids".to_sym : "#{field}_id".to_sym
+    end
 
     ##
     # This method is the "read" implementation for the serializer.

spec/dummy/app/models/country.rb

@@ -1,3 +1,21 @@
 class Country < ActiveResource::Base
   self.site = 'http://localhost:3000'
+
+  def self.find id
+    self.new id: id, name: Faker::Lorem.name
+  end
+
+  def self.build
+    self.new name: nil
+  end
+
+  def self.find_all ids
+    ids.map{|id| find id }
+  end
+
+  def save
+    self.persisted = true
+    true
+  end
+
 end

spec/dummy/app/models/news_item.rb

@@ -0,0 +1,2 @@
+class NewsItem < ActiveRecord::Base
+end

spec/dummy/db/migrate/20161004155830_create_news_items.rb

@@ -0,0 +1,9 @@
+class CreateNewsItems < ActiveRecord::Migration
+  def change
+    create_table :news_items do |t|
+      t.string :title
+
+      t.timestamps null: false
+    end
+  end
+end

spec/models/no_cms/blocks/blocks/active_record_spec.rb

@@ -14,7 +14,8 @@
               fields: {
                 caption: :string,
                 logo: TestImage,
-                slides: { type: TestImage, multiple: true }
+                slides: { type: TestImage, multiple: true },
+                optional_slides: { type: TestImage, multiple: true }
               }
             }
           }
@@ -44,31 +45,20 @@
         expect{subject.slide_ids}.to_not raise_error
       end
 
-      it("should return objects") do
-        expect(subject.logo).to be_a(TestImage)
-      end
-
-      it("should return arrays of objects") do
-        expect(subject.slides).to be_a(ActiveRecord::Relation)
-        expect(subject.slides.first).to be_a(TestImage)
-      end
-
-      it("should return objects with the right value") do
-        expect(subject.logo.name).to eq image_attributes[:name]
-      end
+      context "concerning single active record fields" do
 
-      it("should return objects with the right value") do
-        expect(subject.logo.name).to eq image_attributes[:name]
-        expect(subject.slides.first.name).to eq slide_attributes_1[:name]
-        expect(subject.slides.second.name).to eq slide_attributes_2[:name]
-      end
+        it("should return objects") do
+          expect(subject.logo).to be_a(TestImage)
+        end
 
-      it("should save related objects") do
-        expect(TestImage.first).to_not be_nil
-      end
+        it("should return objects with the right value") do
+          expect(subject.logo.name).to eq image_attributes[:name]
+        end
 
+        it("should save related objects") do
+          expect(TestImage.first).to_not be_nil
+        end
 
-      context "concerning single active record fields" do
 
         context "when related objects are modified outside" do
 
@@ -150,6 +140,22 @@
 
       context "concerning multiple active record fields" do
 
+        it("should return arrays of objects") do
+          expect(subject.slides).to be_a(ActiveRecord::Relation)
+          expect(subject.slides.first).to be_a(TestImage)
+        end
+
+        it("should return an empty relation when no objects are related") do
+          expect(subject.optional_slides).to be_a(ActiveRecord::Relation)
+          expect(subject.optional_slides).to be_empty
+        end
+
+        it("should return objects with the right value") do
+          expect(subject.slides.first.name).to eq slide_attributes_1[:name]
+          expect(subject.slides.second.name).to eq slide_attributes_2[:name]
+        end
+
+
         context "when related objects are modified outside" do
 
           let(:slide) { TestImage.find_by(name: slide_attributes_1[:name]) }