diff --git a/app/serializers/no_cms/blocks/active_record_serializer.rb b/app/serializers/no_cms/blocks/active_record_serializer.rb index 7acebda..ac14ba7 100644 --- a/app/serializers/no_cms/blocks/active_record_serializer.rb +++ b/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 diff --git a/app/serializers/no_cms/blocks/active_resource_serializer.rb b/app/serializers/no_cms/blocks/active_resource_serializer.rb index b24ad55..bacb44d 100644 --- a/app/serializers/no_cms/blocks/active_resource_serializer.rb +++ b/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 diff --git a/app/serializers/no_cms/blocks/base_multiple_serializer.rb b/app/serializers/no_cms/blocks/base_multiple_serializer.rb index 659a75e..89eedf1 100644 --- a/app/serializers/no_cms/blocks/base_multiple_serializer.rb +++ b/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. diff --git a/spec/dummy/app/models/country.rb b/spec/dummy/app/models/country.rb index 36eca73..e3590a6 100644 --- a/spec/dummy/app/models/country.rb +++ b/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 diff --git a/spec/dummy/app/models/news_item.rb b/spec/dummy/app/models/news_item.rb new file mode 100644 index 0000000..f15c752 --- /dev/null +++ b/spec/dummy/app/models/news_item.rb @@ -0,0 +1,2 @@ +class NewsItem < ActiveRecord::Base +end diff --git a/spec/dummy/db/migrate/20161004155830_create_news_items.rb b/spec/dummy/db/migrate/20161004155830_create_news_items.rb new file mode 100644 index 0000000..d4089db --- /dev/null +++ b/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 diff --git a/spec/models/no_cms/blocks/blocks/active_record_spec.rb b/spec/models/no_cms/blocks/blocks/active_record_spec.rb index 966aaf7..ca60dff 100644 --- a/spec/models/no_cms/blocks/blocks/active_record_spec.rb +++ b/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]) } diff --git a/spec/models/no_cms/blocks/duplicating_blocks_spec.rb b/spec/models/no_cms/blocks/duplicating_blocks_spec.rb index 52795db..5942f99 100644 --- a/spec/models/no_cms/blocks/duplicating_blocks_spec.rb +++ b/spec/models/no_cms/blocks/duplicating_blocks_spec.rb @@ -16,11 +16,13 @@ header: attributes_for(:test_image) end - let(:block_with_ar) do + let(:block_with_resource) do NoCms::Blocks::Block.create layout: 'with_country', title: block_title, country_id: 66, - countries_ids: [4, 87, 666] + countries_ids: [4, 87, 666], + dupped_country_id: 67, + dupped_countries_ids: [68, 69] end @@ -36,7 +38,9 @@ title: { type: :string, translated: false }, body: { type: :text, translated: false, duplicate: :nullify }, logo: { type: TestImage, translated: false, duplicate: :dup }, + headline: { type: NewsItem, translated: false, duplicate: :dup }, slides: { type: TestImage, translated: false, duplicate: :dup, multiple: true }, + optional_slides: { type: TestImage, translated: false, duplicate: :dup, multiple: true }, background: { type: TestImage, translated: false, duplicate: :nil }, header: { type: TestImage, translated: false, duplicate: :link } } @@ -46,34 +50,48 @@ fields: { title: { type: :string, translated: false }, country: { type: Country, translated: false, duplicate: :link }, - countries: { type: Country, translated: false, duplicate: :link, multiple: true } + countries: { type: Country, translated: false, duplicate: :link, multiple: true }, + dupped_country: { type: Country, translated: false, duplicate: :dup }, + dupped_countries: { type: Country, translated: false, duplicate: :dup, multiple: true }, + optional_country: { type: Country, translated: false, duplicate: :nullify }, + optional_countries: { type: Country, translated: false, duplicate: :nullify, multiple: true } } } } end end - before { dupped_block.save! } + let(:block_with_resource_dupped) do + block_with_resource.dup + end + + before(:each) do + dupped_block.save! + block_with_resource_dupped.save! + end + subject { NoCms::Blocks::Block.find dupped_block.id } context "duplicate with :link" do - before(:each) do - dupped.save! + + it "should save block with Active Resource field non multiple" do + expect(block_with_resource_dupped.country_id).to eq 66 end - let(:dupped) do - block_with_ar.dup + it "should save block with Active Resource field multiple" do + expect(block_with_resource_dupped.countries_ids).to eq block_with_resource.countries_ids end - it "should save block with Active Resource field non multiple" do - expect(dupped.country_id).to eq 66 + it "should link an Active Record field configured to be linked" do + expect(subject.header).to eq block.header end - it "should save block with Active Resource field multiple" do - expect(dupped.countries_ids).to eq [4, 87, 666] + it "should link an Active Resource field configured to be linked" do + expect(block_with_resource_dupped.country).to eq block_with_resource.country end + end it "should have the same layout" do @@ -84,25 +102,76 @@ expect(subject.title).to eq block_title end - it "should nullify the field configured to be nullified" do - expect(subject.body).to be_nil - end + context "duplicate with :nullify" do - it "should duplicate an Active Record field configured to be duplicated" do - expect(subject.logo).to_not be_new_record - expect(subject.logo).to_not eq block.logo - end - it "should duplicate a multiple Active Record field configured to be duplicated" do - expect(subject.slides).to be_a ActiveRecord::Relation - expect(subject.slides).to_not match_array block.slides - end + it "should nullify the field configured to be nullified" do + expect(subject.body).to be_nil + end + + it "should return a new object when an Active Record field is configured to nullify" do + expect(subject.background).to be_new_record + end + + it "should return a new object when an Active Resource field is configured to nullify" do + expect(block_with_resource_dupped.optional_country).to be_new_record + end - it "should return a new record when an AR field is configured to nullify" do - expect(subject.background).to be_new_record end - it "should link an Active Record field configured to be linked" do - expect(subject.header).to eq block.header + context "duplicate with :dup" do + + context "an ActiveRecord field" do + + it "should duplicate an Active Record field configured to be duplicated" do + expect(subject.logo).to_not be_new_record + expect(subject.logo).to_not eq block.logo + end + + it "should duplicate a multiple Active Record field configured to be duplicated" do + expect(subject.slides).to be_a ActiveRecord::Relation + expect(subject.slides).to_not match_array block.slides + end + + it "should not create an Active Record object when the original was nil" do + expect(subject.headline).to be_new_record + end + + it "should not create any Active Record object when the original was nil in a multiple field" do + expect(subject.optional_slides).to be_a ActiveRecord::Relation + expect(subject.optional_slides).to be_empty + end + + end + + context "an Active Resource field" do + + it "should duplicate an Active Resource field configured to be duplicated" do + expect(block_with_resource_dupped.dupped_country).to be_new_record + # WAIT!! ActiveResource behaviour with dup is not to create a new + # object, as it keeps the id, so the new object is exactly the same + # and :dup behaves just as :link + expect(block_with_resource_dupped.dupped_country).to eq block_with_resource.dupped_country + end + + it "should duplicate a multiple Active Resource field configured to be duplicated" do + expect(block_with_resource_dupped.dupped_countries).to be_a Array + # WAIT!! ActiveResource behaviour with dup is not to create a new + # object, as it keeps the id, so the new object is exactly the same + # and :dup behaves just as :link + expect(block_with_resource_dupped.dupped_countries).to match_array block_with_resource.dupped_countries + end + + it "should not create an Active Resource object when the original was nil" do + expect(block_with_resource_dupped.optional_country).to be_new_record + end + + it "should not create any Active Resource object when the original was nil in a multiple field" do + expect(block_with_resource_dupped.optional_countries).to be_a Array + expect(block_with_resource_dupped.optional_countries).to be_empty + end + + end + end end