Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Corrected method call to :loaded! in HasManyAssociation (fixes #383) #384

Merged
merged 1 commit into from
Apr 4, 2014
+15 −5
Merged

Corrected method call to :loaded! in HasManyAssociation (fixes #383) #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/active_fedora/associations/has_many_association.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def count_records
# If there's nothing in the database and @target has no new records
# we are certain the current target is an empty array. This is a
# documented side-effect of the method that may avoid an extra SELECT.
@target ||= [] and loaded if count == 0
@target ||= [] and loaded! if count == 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should and be && here? I'm not suggesting anything is broken, but using and near assignment operators can cause unexpected issues due to the difference in precedence between it and &&. (&& is higher precedence than ||= and and is lower than ||=.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay. It was intended to be a copy of this:
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/has_many_association.rb#L69

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.


return count
end
Expand Down
4 changes: 2 additions & 2 deletions spec/integration/associations_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,9 @@ class Publisher < ActiveFedora::Base

it "should let you shift onto the association" do
@library.new_record?.should be_true
@library.books.size == 0
@library.books.size.should == 0
@library.books.should == []
@library.book_ids.should ==[]
@library.book_ids.should == []
@library.books << @book
@library.books.should == [@book]
@library.book_ids.should ==[@book.pid]
Expand Down
14 changes: 12 additions & 2 deletions spec/integration/has_many_associations_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'spec_helper'

describe "Looking up collection members" do
describe "Collection members" do
before :all do
class Library < ActiveFedora::Base
has_many :books
Expand All @@ -14,7 +14,17 @@ class Book < ActiveFedora::Base
Object.send(:remove_const, :Book)
Object.send(:remove_const, :Library)
end
describe "of has_many" do
describe "size of has_many" do
let(:library) { Library.create }
context "when no members" do
it "should cache the count" do
expect(library.books).not_to be_loaded
expect(library.books.size).to eq(0)
expect(library.books).to be_loaded
end
end
end
describe "looking up has_many" do
let(:book) { Book.create }
let(:library) { Library.create() }
before do
Expand Down