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.

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

fix issue in #reify_has_many_through #596

Merged
merged 1 commit into from
Sep 16, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion lib/paper_trail/version_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,14 @@ def reify_has_many_through(associations, model, options = {})
associations.each do |assoc|
next unless assoc.klass.paper_trail_enabled_for_model?
through_collection = model.send(assoc.options[:through])
collection_keys = through_collection.map { |through_model| through_model.send(assoc.foreign_key) }

# if the association is a has_many association again, then call reify_has_manys for each through_collection
if !assoc.source_reflection.belongs_to? && through_collection.present?
through_collection.each { |through_model| reify_has_manys(through_model,options) }
next
end

collection_keys = through_collection.map { |through_model| through_model.send(assoc.association_foreign_key)}
Copy link
Member

Choose a reason for hiding this comment

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

If I revert the change to this one line, by calling assoc.foreign_key instead of assoc.association_foreign_key, all the tests still pass. So, is it necessary to change this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the example of Document, Sections and paragraphs:
The through_model is a object of Section.

What we are trying to do is get the list of all the paragraphs associated to the section by calling:
section.paragraph_ids

If we do Section.reflect_on_association(:paragraphs).foreign_key, it returns 'section_id' and if we do Section.reflect_on_association(:paragraphs).association_foreign_key, it returns 'paragraph_id'. So we should be using 'association_foreign_key' instead of 'foreign_key'.

This was not an issue so far because in a 'belongs_to' association assoc.association_foreign_key and assoc.foreign_key are the same.

Even if you replace assoc.association_foreign_key with assoc.foreign_key it works because there is no chance of 'has_many' associations to hit the line because we are calling 'return' on line:424

Copy link
Member

Choose a reason for hiding this comment

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

That all makes sense, thanks.


version_id_subquery = assoc.klass.paper_trail_version_class.
select("MIN(id)").
Expand Down
9 changes: 9 additions & 0 deletions test/dummy/app/models/chapter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
class Chapter < ActiveRecord::Base
has_many :sections, :dependent => :destroy
has_many :paragraphs, :through => :sections

has_many :quotations, :dependent => :destroy
has_many :citations, :through => :quotations

has_paper_trail
end
5 changes: 5 additions & 0 deletions test/dummy/app/models/citation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class Citation < ActiveRecord::Base
belongs_to :quotation

has_paper_trail
end
5 changes: 5 additions & 0 deletions test/dummy/app/models/paragraph.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class Paragraph < ActiveRecord::Base
belongs_to :section

has_paper_trail
end
5 changes: 5 additions & 0 deletions test/dummy/app/models/quotation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class Quotation < ActiveRecord::Base
belongs_to :chapter
has_many :citations, :dependent => :destroy
has_paper_trail
end
6 changes: 6 additions & 0 deletions test/dummy/app/models/section.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class Section < ActiveRecord::Base
belongs_to :chapter
has_many :paragraphs, :dependent => :destroy

has_paper_trail
end
27 changes: 27 additions & 0 deletions test/dummy/db/migrate/20110208155312_set_up_test_tables.rb
Original file line number Diff line number Diff line change
Expand Up @@ -211,9 +211,33 @@ def self.up
t.string :name
t.boolean :scoped, :default => true
end

create_table :chapters, :force => true do |t|
t.string :name
end

create_table :sections, :force => true do |t|
t.integer :chapter_id
t.string :name
end

create_table :paragraphs, :force => true do |t|
t.integer :section_id
t.string :name
end

create_table :quotations, :force => true do |t|
t.integer :chapter_id
end

create_table :citations, :force => true do |t|
t.integer :quotation_id
end
end

def self.down
drop_table :citations
drop_table :quotations
drop_table :animals
drop_table :skippers
drop_table :not_on_updates
Expand Down Expand Up @@ -247,6 +271,9 @@ def self.down
drop_table :line_items
drop_table :fruits
drop_table :boolits
drop_table :chapters
drop_table :sections
drop_table :paragraphs
remove_index :version_associations, :column => [:version_id]
remove_index :version_associations, :name => 'index_version_associations_on_foreign_key'
drop_table :version_associations
Expand Down
165 changes: 165 additions & 0 deletions test/unit/model_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1902,4 +1902,169 @@ class HasPaperTrailModelTransactionalTest < ActiveSupport::TestCase
end
end
end

context "Models with nested has_many through relationships" do
setup { @chapter = Chapter.create(:name => 'ch_1') }

context "before any associations are created" do
setup do
@chapter.update_attributes(:name => "ch_2")
@ch_1 = @chapter.versions.last.reify(:has_many => true)
end

should "reify the record when reify is called" do
assert_equal "ch_1", @ch_1.name
end
end

context "after the first has_many through relationship is created" do
setup do
@chapter.update_attributes :name => "ch_2"
Timecop.travel 1.second.since
@chapter.sections.create :name => "section 1"
Timecop.travel 1.second.since
@chapter.sections.first.update_attributes :name => "section 2"
Timecop.travel 1.second.since
@chapter.update_attributes :name => "ch_3"
Timecop.travel 1.second.since
@chapter.sections.first.update_attributes :name => "section 3"
end

context "when reify is called" do
setup do
@chapter_2 = @chapter.versions.last.reify(:has_many => true)
end

should "show the value of the base record as it was before" do
assert_equal "ch_2", @chapter_2.name
end

should "show the value of the associated record as it was before the base record was updated" do
assert_equal ['section 2'], @chapter_2.sections.map(&:name)
end

context "to the version before the relationship was created" do
setup { @chapter_1 = @chapter.versions.second.reify(:has_many => true) }

should "not return any associated records" do
assert_equal 0, @chapter_1.sections.size
end
end

context "to the version before the associated record has been destroyed" do
setup do
@chapter.update_attributes :name => 'ch_3'
Timecop.travel 1.second.since
@chapter.sections.destroy_all
Timecop.travel 1.second.since
@chapter_3 = @chapter.versions.last.reify(:has_many => true)
end

should "return the associated record" do
assert_equal ['section 2'], @chapter_3.sections.map(&:name)
end
end

context "to the version after the associated record has been destroyed" do
setup do
@chapter.sections.destroy_all
Timecop.travel 1.second.since
@chapter.update_attributes :name => 'ch_4'
Timecop.travel 1.second.since
@chapter_4 = @chapter.versions.last.reify(:has_many => true)
end

should "return the associated record" do
assert_equal 0, @chapter_4.sections.size
end
end
end

context "after the nested has_many through relationship is created" do
setup do
@section = @chapter.sections.first
@paragraph = @section.paragraphs.create :name => 'para1'
end

context "reify the associations" do
setup do
Timecop.travel 1.second.since
@initial_section_name = @section.name
@initial_paragraph_name = @paragraph.name
@chapter.update_attributes :name => 'ch_5'
Timecop.travel 1.second.since
@paragraph.update_attributes :name => 'para3'
Timecop.travel 1.second.since
@chapter_4 = @chapter.versions.last.reify(:has_many => true)
end

should "to the version before the change was made" do
assert_equal [@initial_section_name], @chapter_4.sections.map(&:name)
assert_equal [@initial_paragraph_name], @chapter_4.sections.first.paragraphs.map(&:name)
end
end

context "and the first has_many through relationship is destroyed" do
setup do
@section.destroy
Timecop.travel 1.second.since
@chapter.update_attributes(:name => 'chapter 6')
Timecop.travel 1.second.since
@chapter_before = @chapter.versions.last.reify(:has_many => true)
end

should "reify should not return any associated models" do
assert_equal 0, @chapter_before.sections.size
assert_equal 0, @chapter_before.paragraphs.size
end
end

context "reified to the version before the nested has_many through relationship is destroyed" do
setup do
Timecop.travel 1.second.since
@initial_paragraph_name = @section.paragraphs.first.name
@chapter.update_attributes(:name => 'chapter 6')
Timecop.travel 1.second.since
@paragraph.destroy
@chapter_before = @chapter.versions.last.reify(:has_many => true)
end

should "restore the associated has_many relationship" do
assert_equal [@initial_paragraph_name], @chapter_before.sections.first.paragraphs.map(&:name)
end
end

context "reified to the version after the nested has_many through relationship is destroyed" do
setup do
@paragraph.destroy
Timecop.travel 1.second.since
@chapter.update_attributes(:name => 'chapter 6')
@chapter_before = @chapter.versions.last.reify(:has_many => true)
end

should "restore the associated has_many relationship" do
assert_equal [], @chapter_before.sections.first.paragraphs
end
end
end
end

context "a chapter with one paragraph and one citation" do
should "reify paragraphs and citations" do
chapter = Chapter.create(:name => 'Chapter One')
section = Section.create(:name => 'Section One', :chapter => chapter)
paragraph = Paragraph.create(:name => 'Paragraph One', :section => section)
quotation = Quotation.create(:chapter => chapter)
citation = Citation.create(:quotation => quotation)
Timecop.travel 1.second.since
chapter.update_attributes(:name => 'Chapter One, Vers. Two')
assert_equal 2, chapter.versions.count
paragraph.destroy
citation.destroy
reified = chapter.versions[1].reify(:has_many => true)
assert_equal [paragraph], reified.sections.first.paragraphs
assert_equal [citation], reified.quotations.first.citations
end
end
end
end