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

Possibility to preserve skipped attributes on reify #579

Merged
merged 1 commit into from Aug 3, 2015
Merged

Possibility to preserve skipped attributes on reify #579

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 18 additions & 11 deletions lib/paper_trail/version_concern.rb
View file
Open in desktop
Expand Up @@ -154,14 +154,18 @@ def object_changes_col_is_json?
# they were "at the time", if they are also being versioned by PaperTrail.
#
# Options:
# :has_one set to `true` to also reify has_one associations. Default is `false`.
# :has_many set to `true` to also reify has_many and has_many :through associations.
# Default is `false`.
# :mark_for_destruction set to `true` to mark the has_one/has_many associations that did not exist in the
# reified version for destruction, instead of remove them. Default is `false`.
# This option is handy for people who want to persist the reified version.
# :dup `false` default behavior
# `true` it always create a new object instance. It is useful for comparing two versions of the same object
# :has_one set to `true` to also reify has_one associations. Default is `false`.
# :has_many set to `true` to also reify has_many and has_many :through associations.
# Default is `false`.
# :mark_for_destruction set to `true` to mark the has_one/has_many associations that did not exist in the
# reified version for destruction, instead of remove them. Default is `false`.
# This option is handy for people who want to persist the reified version.
# :dup `false` default behavior
# `true` it always create a new object instance. It is useful for comparing two versions of the same object
# :unversioned_attributes `:nil` - (default) attributes undefined in version record
# are set to nil in reified record
# `:preserve` - attributes undefined in version record are
# not modified
def reify(options = {})
return nil if object.nil?

Expand All @@ -170,7 +174,8 @@ def reify(options = {})
:version_at => created_at,
:mark_for_destruction => false,
:has_one => false,
:has_many => false
:has_many => false,
:unversioned_attributes => :nil
)

attrs = self.class.object_col_is_json? ? object : PaperTrail.serializer.load(object)
Expand All @@ -191,7 +196,9 @@ def reify(options = {})
if options[:dup] != true && item
model = item
# Look for attributes that exist in the model and not in this version. These attributes should be set to nil.
(model.attribute_names - attrs.keys).each { |k| attrs[k] = nil }
if options[:unversioned_attributes] == :nil
(model.attribute_names - attrs.keys).each { |k| attrs[k] = nil }
end
else
inheritance_column_name = item_type.constantize.inheritance_column
class_name = attrs[inheritance_column_name].blank? ? item_type : attrs[inheritance_column_name]
Expand All @@ -200,7 +207,7 @@ def reify(options = {})
# to look for the item outside of default scope(s)
if options[:dup] || (_item = klass.unscoped.find_by_id(item_id)).nil?
model = klass.new
else
elsif options[:unversioned_attributes] == :nil
model = _item
# Look for attributes that exist in the model and not in this version. These attributes should be set to nil.
(model.attribute_names - attrs.keys).each { |k| attrs[k] = nil }
Expand Down
47 changes: 38 additions & 9 deletions spec/models/skipper_spec.rb
View file
Open in desktop
@@ -1,16 +1,45 @@
require 'rails_helper'

describe Skipper, :type => :model do
describe "#update_attributes!", :versioning => true do
context "updating a skipped attribute" do
let(:t1) { Time.zone.local(2015, 7, 15, 20, 34, 0) }
let(:t2) { Time.zone.local(2015, 7, 15, 20, 34, 30) }
with_versioning do
it { is_expected.to be_versioned }

it "should not create a version" do
skipper = Skipper.create!(:another_timestamp => t1)
expect {
skipper.update_attributes!(:another_timestamp => t2)
}.to_not change { skipper.versions.length }
describe "#update_attributes!", :versioning => true do
context "updating a skipped attribute" do
let(:t1) { Time.zone.local(2015, 7, 15, 20, 34, 0) }
let(:t2) { Time.zone.local(2015, 7, 15, 20, 34, 30) }

it "should not create a version" do
skipper = Skipper.create!(:another_timestamp => t1)
expect {
skipper.update_attributes!(:another_timestamp => t2)
}.to_not change { skipper.versions.length }
end
end
end

describe "reify" do
context "reifying a with a skipped attribute" do
let(:t1) { Time.zone.local(2015, 7, 15, 20, 34, 0) }
let(:t2) { Time.zone.local(2015, 7, 15, 20, 34, 30) }

context "without preserve (default)" do
it "should have no timestamp" do
skipper = Skipper.create!(:another_timestamp => t1)
skipper.update_attributes!(:another_timestamp => t2, :name => "Foobar")
skipper = skipper.versions.last.reify
expect(skipper.another_timestamp).to be(nil)
end
end

context "with preserve" do
it "should preserve its timestamp" do
skipper = Skipper.create!(:another_timestamp => t1)
skipper.update_attributes!(:another_timestamp => t2, :name => "Foobar")
skipper = skipper.versions.last.reify(:unversioned_attributes => :preserve)
expect(skipper.another_timestamp).to eq(t2)
end
end
end
end
end
Expand Down
1 change: 1 addition & 0 deletions test/dummy/db/migrate/20110208155312_set_up_test_tables.rb
View file
Open in desktop
@@ -1,6 +1,7 @@
class SetUpTestTables < ActiveRecord::Migration
def self.up
create_table :skippers, :force => true do |t|
t.string :name
t.datetime :another_timestamp
t.timestamps :null => true
end
Expand Down
88 changes: 63 additions & 25 deletions test/dummy/db/schema.rb
View file
Open in desktop
Expand Up @@ -13,77 +13,108 @@

ActiveRecord::Schema.define(version: 20110208155312) do

create_table "animals", force: true do |t|
create_table "animals", force: :cascade do |t|
t.string "name"
t.string "species"
end

create_table "articles", force: true do |t|
create_table "articles", force: :cascade do |t|
t.string "title"
t.string "content"
t.string "abstract"
t.string "file_upload"
end

create_table "authorships", force: true do |t|
create_table "authorships", force: :cascade do |t|
t.integer "book_id"
t.integer "person_id"
end

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

add_index "banana_versions", ["item_type", "item_id"], name: "index_banana_versions_on_item_type_and_item_id"

create_table "bananas", force: :cascade do |t|
t.datetime "created_at"
t.datetime "updated_at"
end

create_table "books", force: :cascade do |t|
t.string "title"
end

create_table "customers", force: true do |t|
create_table "boolits", force: :cascade do |t|
t.string "name"
t.boolean "scoped", default: true
end

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

create_table "documents", force: true do |t|
create_table "documents", force: :cascade do |t|
t.string "name"
end

create_table "editors", force: true do |t|
create_table "editors", force: :cascade do |t|
t.string "name"
end

create_table "editorships", force: true do |t|
create_table "editorships", force: :cascade do |t|
t.integer "book_id"
t.integer "editor_id"
end

create_table "fluxors", force: true do |t|
create_table "fluxors", force: :cascade do |t|
t.integer "widget_id"
t.string "name"
end

create_table "gadgets", force: true do |t|
create_table "fruits", force: :cascade do |t|
t.string "name"
t.string "color"
end

create_table "gadgets", force: :cascade do |t|
t.string "name"
t.string "brand"
t.datetime "created_at"
t.datetime "updated_at"
end

create_table "legacy_widgets", force: true do |t|
create_table "legacy_widgets", force: :cascade do |t|
t.string "name"
t.integer "version"
end

create_table "line_items", force: true do |t|
create_table "line_items", force: :cascade do |t|
t.integer "order_id"
t.string "product"
end

create_table "orders", force: true do |t|
create_table "not_on_updates", force: :cascade do |t|
t.datetime "created_at"
t.datetime "updated_at"
end

create_table "orders", force: :cascade do |t|
t.integer "customer_id"
t.string "order_date"
end

create_table "people", force: true do |t|
create_table "people", force: :cascade do |t|
t.string "name"
t.string "time_zone"
end

create_table "post_versions", force: true do |t|
create_table "post_versions", force: :cascade do |t|
t.string "item_type", null: false
t.integer "item_id", null: false
t.string "event", null: false
Expand All @@ -96,31 +127,38 @@

add_index "post_versions", ["item_type", "item_id"], name: "index_post_versions_on_item_type_and_item_id"

create_table "post_with_statuses", force: true do |t|
create_table "post_with_statuses", force: :cascade do |t|
t.integer "status"
end

create_table "posts", force: true do |t|
create_table "posts", force: :cascade do |t|
t.string "title"
t.string "content"
end

create_table "songs", force: true do |t|
create_table "skippers", force: :cascade do |t|
t.string "name"
t.datetime "another_timestamp"
t.datetime "created_at"
t.datetime "updated_at"
end

create_table "songs", force: :cascade do |t|
t.integer "length"
end

create_table "things", force: true do |t|
create_table "things", force: :cascade do |t|
t.string "name"
end

create_table "translations", force: true do |t|
create_table "translations", force: :cascade do |t|
Copy link
Member

Choose a reason for hiding this comment

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

Why did the force option change from true to :cascade on every table? Is that the new recommended value, or is it something you did intentionally? From the docs:

:force
Set to true to drop the table before creating it. Set to :cascade to drop 
dependent objects as well. Defaults to false.

It seems like with force: true the order of table creation wouldn't matter, but with :cascade it would, so true seems preferable to me.

Copy link
Author

Choose a reason for hiding this comment

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

It was changed during db:migrate. This has changed since Rails 4.2. The release notes say:

SchemaDumper uses force: :cascade on create_table. This makes it possible to reload a schema when foreign keys are in place.

t.string "headline"
t.string "content"
t.string "language_code"
t.string "type"
end

create_table "version_associations", force: true do |t|
create_table "version_associations", force: :cascade do |t|
t.integer "version_id"
t.string "foreign_key_name", null: false
t.integer "foreign_key_id"
Expand All @@ -129,7 +167,7 @@
add_index "version_associations", ["foreign_key_name", "foreign_key_id"], name: "index_version_associations_on_foreign_key"
add_index "version_associations", ["version_id"], name: "index_version_associations_on_version_id"

create_table "versions", force: true do |t|
create_table "versions", force: :cascade do |t|
t.string "item_type", null: false
t.integer "item_id", null: false
t.string "event", null: false
Expand All @@ -149,13 +187,13 @@

add_index "versions", ["item_type", "item_id"], name: "index_versions_on_item_type_and_item_id"

create_table "whatchamajiggers", force: true do |t|
create_table "whatchamajiggers", force: :cascade do |t|
t.string "owner_type"
t.integer "owner_id"
t.string "name"
end

create_table "widgets", force: true do |t|
create_table "widgets", force: :cascade do |t|
t.string "name"
t.text "a_text"
t.integer "an_integer"
Expand All @@ -171,7 +209,7 @@
t.datetime "updated_at"
end

create_table "wotsits", force: true do |t|
create_table "wotsits", force: :cascade do |t|
t.integer "widget_id"
t.string "name"
t.datetime "created_at"
Expand Down