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

issue with primary key other than id #868

Merged
merged 7 commits into from
Sep 28, 2016
Merged
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/).

### Fixed

- [#868](https://github.com/airblade/paper_trail/pull/868)
Fix usage of find_by_id when primary key is not id, affecting reifying certain records.

## 5.2.2 (2016-09-08)

### Breaking Changes
Expand Down
3 changes: 2 additions & 1 deletion lib/paper_trail/reifier.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ def reify(version, options)
klass = version_reification_class(version, attrs)
# The `dup` option always returns a new object, otherwise we should
# attempt to look for the item outside of default scope(s).
if options[:dup] || (item_found = klass.unscoped.find_by_id(version.item_id)).nil?
find_cond = { klass.primary_key => version.item_id }
if options[:dup] || (item_found = klass.unscoped.where(find_cond).first).nil?
Copy link
Member

Choose a reason for hiding this comment

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

This part looks good to me.

model = klass.new
elsif options[:unversioned_attributes] == :nil
model = item_found
Expand Down
18 changes: 18 additions & 0 deletions spec/models/custom_primary_key_record_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
require "rails_helper"

describe CustomPrimaryKeyRecord, type: :model do
it { is_expected.to be_versioned }

describe "#versions" do
it "returns instances of CustomPrimaryKeyRecordVersion", versioning: true do
custom_primary_key_record = described_class.create!
custom_primary_key_record.update_attributes!(name: "bob")
version = custom_primary_key_record.versions.last
expect(version).to be_a(CustomPrimaryKeyRecordVersion)
version_from_db = CustomPrimaryKeyRecordVersion.last
expect(version_from_db.reify).to be_a(CustomPrimaryKeyRecord)
custom_primary_key_record.destroy
expect(CustomPrimaryKeyRecordVersion.last.reify).to be_a(CustomPrimaryKeyRecord)
end
end
end
13 changes: 13 additions & 0 deletions test/dummy/app/models/custom_primary_key_record.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
require "securerandom"
class CustomPrimaryKeyRecord < ActiveRecord::Base
self.primary_key = :uuid

has_paper_trail class_name: "CustomPrimaryKeyRecordVersion"
# this unusual default_scope is to test the case of the Version#item association
# not returning the item due to unmatched default_scope on the model.
default_scope -> { where(name: "custom_primary_key_record") }
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this default_scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is to prevent the record from being returned on version.item. this bug surfaces in the else path of https://github.com/airblade/paper_trail/blob/v5.2.2/lib/paper_trail/reifier.rb#L25-L40

version.item returns nil when the item is destroyed, and when a default scope on the model prevents the association from returning it. I added this default scope to test both of those cases.

Copy link
Member

Choose a reason for hiding this comment

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

ok that makes sense. Please add your explanation as a comment above the default_scope.

Also, please add a line in CHANGELOG.md and I think we'll be done here.


before_create do
self.uuid ||= SecureRandom.uuid
end
end
3 changes: 3 additions & 0 deletions test/dummy/app/versions/custom_primary_key_record_version.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class CustomPrimaryKeyRecordVersion < PaperTrail::Version
self.table_name = "custom_primary_key_record_versions"
end
18 changes: 18 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 @@ -271,6 +271,24 @@ def up
end
add_index :bar_habtms_foo_habtms, [:foo_habtm_id]
add_index :bar_habtms_foo_habtms, [:bar_habtm_id]

# custom_primary_key_records use a uuid column (string)
create_table :custom_primary_key_records, id: false, force: true do |t|
t.column :uuid, :string, primary_key: true
t.string :name
t.timestamps null: true
end

# and custom_primary_key_record_versions stores the uuid in item_id, a string
create_table :custom_primary_key_record_versions, force: true do |t|
t.string :item_type, null: false
t.string :item_id, null: false
t.string :event, null: false
t.string :whodunnit
t.text :object
t.datetime :created_at
end
add_index :custom_primary_key_record_versions, [:item_type, :item_id], name: "idx_cust_pk_item"
end

def down
Expand Down
18 changes: 18 additions & 0 deletions test/dummy/db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,24 @@
t.integer "quotation_id"
end

create_table "custom_primary_key_record_versions", force: :cascade do |t|
t.string "item_type", null: false
t.string "item_id", null: false
t.string "event", null: false
t.string "whodunnit"
t.text "object"
t.datetime "created_at"
end

add_index "custom_primary_key_record_versions", ["item_type", "item_id"], name: "idx_cust_pk_item"

create_table "custom_primary_key_records", id: false, force: :cascade do |t|
t.string "uuid"
t.string "name"
t.datetime "created_at"
t.datetime "updated_at"
end

create_table "customers", force: :cascade do |t|
t.string "name"
end
Expand Down