Skip to content
This repository has been archived by the owner on Apr 17, 2018. It is now read-only.

Commit

Permalink
Fixed element swapping in Collection
Browse files Browse the repository at this point in the history 
* Minor refactoring of specs to ensure Collection#[]= remains not a
  kicker.

[#955 state:resolved]
  • Loading branch information
@dkubb
dkubb committed Jul 9, 2009
1 parent 36b828e commit 8027d2f
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 82 deletions.
10 changes: 2 additions & 8 deletions lib/dm-core/associations/many_to_many.rb
View file
Expand Up @@ -315,24 +315,18 @@ def _create(safe, attributes)
# TODO: document
# @api private
def _save(safe)
resources = if loaded?
self
else
head + tail
end

# delete only intermediaries linked to the target orphans
unless @orphans.empty? || intermediaries(@orphans).send(safe ? :destroy : :destroy!)
return false
end

if via.respond_to?(:resource_for)
super
resources.all? { |resource| create_intermediary(safe, via => resource) }
loaded_entries.all? { |resource| create_intermediary(safe, via => resource) }
else
if intermediary = create_intermediary(safe)
inverse = via.inverse
resources.map { |resource| inverse.set(resource, intermediary) }
loaded_entries.map { |resource| inverse.set(resource, intermediary) }
end

super
Expand Down
34 changes: 19 additions & 15 deletions lib/dm-core/collection.rb
View file
Expand Up @@ -72,16 +72,10 @@ def model
def reload(query = nil)
query = query.nil? ? self.query.dup : self.query.merge(query)

resources = if loaded?
self
else
head + tail
end

# make sure the Identity Map contains all the existing resources
identity_map = repository.identity_map(model)

resources.each do |resource|
loaded_entries.each do |resource|
identity_map[resource.key] = resource
end

Expand Down Expand Up @@ -411,11 +405,15 @@ def slice!(*args)
#
# @api public
def []=(*args)
# orphan resources being replaced
orphan_resources(superclass_slice(*args[0..-2]))
orphans = Array(superclass_slice(*args[0..-2]))

# relate new resources
relate_resources(super)
resources = relate_resources(super)

# orphan resources that no longer exist in the collection elsewhere
orphan_resources(orphans - loaded_entries)

resources
end

alias splice []=
Expand Down Expand Up @@ -1032,6 +1030,16 @@ def lazy_load
self
end

# Loaded Resources in the collection
#
# @return [Array<Resource>]
# Resources in the collection
#
# @api private
def loaded_entries
loaded? ? self : head + tail
end

##
# Initializes a new Collection
#
Expand Down Expand Up @@ -1104,11 +1112,7 @@ def _update(dirty_attributes)
#
# @api private
def _save(safe)
resources = if loaded?
self
else
head + tail
end
resources = loaded_entries

# FIXME: remove this once the writer method on the child side
# is used to store the reference to the parent.
Expand Down
8 changes: 8 additions & 0 deletions spec/lib/collection_helpers.rb
View file
Expand Up @@ -5,6 +5,14 @@ def self.extended(base)
base.class_inheritable_accessor :loaded
base.loaded = false
end

def should_not_be_a_kicker
unless loaded
it 'should not be a kicker' do
@articles.should_not be_loaded
end
end
end
end
end
end
107 changes: 48 additions & 59 deletions spec/public/shared/collection_shared_spec.rb
View file
Expand Up @@ -161,13 +161,7 @@
@return = @resource = @articles.at(0)
end

# FIXME: this is spec order dependent, move this into a helper method
# and execute in the before :all block
unless loaded
it 'should not be a kicker' do
@articles.should_not be_loaded
end
end
should_not_be_a_kicker

it 'should return a Resource' do
@return.should be_kind_of(DataMapper::Resource)
Expand All @@ -187,13 +181,7 @@
@return = @resource = @articles.unshift(@other).at(0)
end

# FIXME: this is spec order dependent, move this into a helper method
# and execute in the before :all block
unless loaded
it 'should not be a kicker' do
@articles.should_not be_loaded
end
end
should_not_be_a_kicker

it 'should return a Resource' do
@return.should be_kind_of(DataMapper::Resource)
Expand All @@ -213,13 +201,7 @@
@return = @resource = @articles.at(-1)
end

# FIXME: this is spec order dependent, move this into a helper method
# and execute in the before :all block
unless loaded
it 'should not be a kicker' do
@articles.should_not be_loaded
end
end
should_not_be_a_kicker

it 'should return a Resource' do
@return.should be_kind_of(DataMapper::Resource)
Expand All @@ -239,13 +221,7 @@
@return = @resource = @articles.push(@other).at(-1)
end

# FIXME: this is spec order dependent, move this into a helper method
# and execute in the before :all block
unless loaded
it 'should not be a kicker' do
@articles.should_not be_loaded
end
end
should_not_be_a_kicker

it 'should return a Resource' do
@return.should be_kind_of(DataMapper::Resource)
Expand Down Expand Up @@ -619,10 +595,6 @@
@articles.should be_empty
end

it 'should be a kicker' do
@articles.should be_loaded
end

it 'should bypass validation' do
pending 'TODO: not sure how to best spec this'
end
Expand All @@ -648,10 +620,6 @@
@limited.should be_empty
end

it 'should be a kicker' do
@limited.should be_loaded
end

it 'should bypass validation' do
pending 'TODO: not sure how to best spec this'
end
Expand Down Expand Up @@ -1342,13 +1310,7 @@
@return = @articles.base_model
end

# FIXME: this is spec order dependent, move this into a helper method
# and execute in the before :all block
unless loaded
it 'should not be a kicker' do
@articles.should_not be_loaded
end
end
should_not_be_a_kicker

it 'should return expected object' do
@return.should == @article_model
Expand Down Expand Up @@ -2454,13 +2416,18 @@
before :all do
unless @skip
orphans = (1..10).map do |number|
@articles.create(:content => "Article #{number}")
@articles.pop # remove the article from the tail
articles = @articles.dup
articles.create(:content => "Article #{number}")
articles.pop # remove the article from the tail
end

@articles.unshift(*orphans.first(5))
@articles.concat(orphans.last(5))

unless loaded
@articles.should_not be_loaded
end

@copy = @articles.dup
@new = @article_model.new(:content => 'New Article')
end
Expand All @@ -2476,6 +2443,8 @@
end
end

should_not_be_a_kicker

it 'should return a Resource' do
@return.should be_kind_of(DataMapper::Resource)
end
Expand Down Expand Up @@ -2511,6 +2480,8 @@
end
end

should_not_be_a_kicker

it 'should return a Resource' do
@return.should be_kind_of(DataMapper::Resource)
end
Expand Down Expand Up @@ -2542,6 +2513,8 @@
end
end

should_not_be_a_kicker

it 'should return a Resource' do
@return.should be_kind_of(DataMapper::Resource)
end
Expand Down Expand Up @@ -2573,6 +2546,8 @@
end
end

should_not_be_a_kicker

it 'should return a Resource' do
@return.should be_kind_of(DataMapper::Resource)
end
Expand Down Expand Up @@ -2608,6 +2583,8 @@
end
end

should_not_be_a_kicker

it 'should return a Resource' do
@return.should be_kind_of(DataMapper::Resource)
end
Expand Down Expand Up @@ -2639,6 +2616,8 @@
end
end

should_not_be_a_kicker

it 'should return a Resource' do
@return.should be_kind_of(DataMapper::Resource)
end
Expand All @@ -2662,6 +2641,28 @@
end
end

describe '#[]=' do
describe 'when swapping resources' do
before :all do
rescue_if 'TODO', @skip do
@articles.create(:content => 'Another Article')

@entries = @articles.entries

@articles[0], @articles[1] = @articles[1], @articles[0]
end
end

it 'should include the Resource in the Collection' do
@articles.should == @entries.reverse
end

it 'should relate the Resource to the Collection' do
@articles.each { |resource| resource.collection.should equal(@articles) }
end
end
end

it { @articles.should respond_to(:unshift) }

describe '#unshift' do
Expand Down Expand Up @@ -2752,13 +2753,7 @@
@return = @articles.update!
end

# FIXME: this is spec order dependent, move this into a helper method
# and execute in the before :all block
unless loaded
it 'should not be a kicker' do
@articles.should_not be_loaded
end
end
should_not_be_a_kicker

it 'should return true' do
@return.should be_true
Expand Down Expand Up @@ -2836,13 +2831,7 @@
@return = @articles.update!(:title => nil)
end

# FIXME: this is spec order dependent, move this into a helper method
# and execute in the before :all block
unless loaded
it 'should not be a kicker' do
@articles.should_not be_loaded
end
end
should_not_be_a_kicker

it 'should return false' do
@return.should be_false
Expand Down

0 comments on commit 8027d2f

Please sign in to comment.