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

Possibility to preserve skipped attributes on reify #579

Merged

Conversation

McRip
Copy link

@McRip McRip commented Jul 31, 2015

Hello, i added the possibility to preserve skipped attributes on reify instead of setting them all to nil.
I added an option to the reify method to enable this behaviour. A Test is attached to this PR.

Thanks for merging.

@McRip
Copy link
Author

McRip commented Jul 31, 2015

I actually have no clue why this fails:

  1. JsonVersion Methods Class #where_object_changes valid arguments should be able to locate versions according to their object_changes contents
    Failure/Error: expect(fruit.versions.where_object_changes(:color => color)).to eq(fruit.versions[1..2])
   expected: [#<JsonVersion id: 5, item_type: "Fruit", item_id: 2, event: "update", whodunnit: nil, object: {"id"=>2, "name"=>"pomegranate", "color"=>nil}, object_changes: {"name"=>["pomegranate", "coconut"], "color"=>[nil, "navy"]}, created_at: "2015-07-31 09:23:59">, #<JsonVersion id: 6, item_type: "Fruit", item_id: 2, event: "update", whodunnit: nil, object: {"id"=>2, "name"=>"coconut", "color"=>"navy"}, object_changes: {"name"=>["coconut", "orange"]}, created_at: "2015-07-31 09:23:59">]
        got: #<ActiveRecord::AssociationRelation [#<JsonVersion id: 5, item_type: "Fruit", item_id: 2, event: "update", whodunnit: nil, object: {"id"=>2, "name"=>"pomegranate", "color"=>nil}, object_changes: {"name"=>["pomegranate", "coconut"], "color"=>[nil, "navy"]}, created_at: "2015-07-31 09:23:59">]>

   (compared using ==)

   Diff:
   @@ -1,4 +1,4 @@
   -[#<JsonVersion:0x575ddcc9
   +[#<JsonVersion:0x288bd61a
      id: 5,
      item_type: "Fruit",
      item_id: 2,
   @@ -6,14 +6,5 @@
      whodunnit: nil,
      object: {"id"=>2, "name"=>"pomegranate", "color"=>nil},
      object_changes: {"name"=>["pomegranate", "coconut"], "color"=>[nil, "navy"]},
   -  created_at: Fri, 31 Jul 2015 09:23:59 UTC +00:00>,
   - #<JsonVersion:0x379f2abf
   -  id: 6,
   -  item_type: "Fruit",
   -  item_id: 2,
   -  event: "update",
   -  whodunnit: nil,
   -  object: {"id"=>2, "name"=>"coconut", "color"=>"navy"},
   -  object_changes: {"name"=>["coconut", "orange"]},
      created_at: Fri, 31 Jul 2015 09:23:59 UTC +00:00>]
 # ./spec/models/json_version_spec.rb:67:in `(root)'

@jaredbeck
Copy link
Member

Don't worry about the JsonVersion test, it's flakey. I ran it again and it passed.

@jaredbeck
Copy link
Member

Can you squash this into one commit, please?

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.

@jaredbeck
Copy link
Member

This seems like a useful feature, and it doesn't increase the complexity of reify too much.

Also, I like this as an option to reify, as opposed to e.g. model-specific config.

As far as the API is concerned, you have:

+    # :preserve             `false` default behaviour
+    #                       `true` preserve attributes of reified object instead of setting 
+    #                       unknown attributes to nil

but I think I'd prefer something like:

+    # :unversioned_attribute    `:nil` - (default) attributes undefined in version record 
+    #                           are set to nil in reified record
+    #                           `:preserve` - attributes undefined in version record are 
+    #                           not modified

My thinking is, this allows for a third option in the future.

@jaredbeck jaredbeck added this to the 4.1.0 milestone Aug 1, 2015
@McRip McRip force-pushed the preserve_skipped_attributes_on_reify branch from 52b8e00 to 4db87df Compare August 3, 2015 09:01
@McRip
Copy link
Author

McRip commented Aug 3, 2015

Hello Jared,
thanks for your feedback. I changed the syntax and merged all the commits into one.

jaredbeck added a commit that referenced this pull request Aug 3, 2015
Possibility to preserve skipped attributes on reify
@jaredbeck jaredbeck merged commit 239db43 into paper-trail-gem:master Aug 3, 2015
@jaredbeck
Copy link
Member

Merged. Thanks Moritz!

jaredbeck added a commit that referenced this pull request Aug 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants