Add paper_trail.update_columns #1037

TylerRick · 2018-01-15T21:26:48Z

Add paper_trail.update_columns so you can record a version when using
update_columns (which skips all callbacks, including the after_update
callback).

Add paper_trail.update_columns so you can record a version when using update_columns (which skips all callbacks, including the after_update callback).

jaredbeck · 2018-01-16T22:04:55Z

Hi Tyler. I don't have time to review this right now, but a few quick thoughts:

Please don't change linter config
Can this be done without a method extraction, or was that necessary because of AbcSize cop? Generally, if you can put refactoring in a separate PR, it makes the review easier.

Sorry, that's not much of a review, but it's all I have time for right now.

argument so that we don't need an exception to Style/MethodCallWithoutArgsParentheses

@jaredbeck

record_update_columns, at @jaredbeck's request

TylerRick · 2018-01-17T00:48:47Z

Can this be done without a method extraction, or was that necessary because of AbcSize cop?

As you wish. I've added back the duplication. No, it was only necessary to allow reuse without duplication.

jaredbeck

This is close, nice work. Please also add an entry to the changelog under Unreleased -> Added.

jaredbeck · 2018-01-18T17:31:59Z

spec/models/widget_spec.rb

+      expect(widget.versions.count).to eq(2)
+      expect(widget.versions.last.event).to(eq("update"))
+      expect(widget.versions.last.changeset[:name]).to eq([nil, "Bugle"])
+      expect(widget.versions.last.created_at.to_i).to eq(Time.now.to_i)


We can't guarantee that this expectation will pass. The operation could take more than a second, or cross a second-boundary.

True... should I just use a Timecop.freeze to keep Time.now fixed? (That's what I did.)

Looks like there's at least one other expectation with the same problem:

expect(widget.paper_trail.version_at(Time.now)).to be_nil

jaredbeck · 2018-01-18T17:32:16Z

spec/models/widget_spec.rb

+      expect(widget.versions.count).to eq(1)
+      Timecop.travel 1.second.since # because MySQL lacks fractional seconds precision
+      widget.paper_trail.update_columns(name: "Bugle")
+      # widget.update_attributes(name: "Bugle")


Please delete all commented-out code.

jaredbeck · 2018-01-18T17:32:43Z

spec/models/widget_spec.rb

+      # widget.update_attributes(name: "Bugle")
+      expect(widget.versions.count).to eq(2)
+      expect(widget.versions.last.event).to(eq("update"))
+      expect(widget.versions.last.changeset[:name]).to eq([nil, "Bugle"])


Good, detailed test.

jaredbeck · 2018-01-18T17:34:38Z

spec/models/on/empty_array_spec.rb

+        widget = Widget.create
+        assert_equal 1, widget.versions.length
+        widget.paper_trail.update_columns(name: "Bugle")
+        assert_equal 2, widget.versions.length


Just FYI, there is a nice way to "expect a change" in RSpec. You don't have to use it, what you have is fine, just thought I'd share.

expect { widget.paper_trail.update_columns(name: "Bugle") }.to(change { widget.versions.length }).from(1).to(2)

jaredbeck · 2018-01-18T17:38:33Z

lib/paper_trail/record_trail.rb

+    end
+
+    def record_update_columns(changes)
+      if enabled?


I'd prefer a "guard clause" here.

return unless enabled?

jaredbeck · 2018-01-18T17:41:27Z

lib/paper_trail/record_trail.rb

+        versions_assoc = @record.send(@record.class.versions_association_name)
+        version = versions_assoc.create(data_for_update_columns(changes))
+        if version.errors.any?
+          log_version_errors(version, :update)


Nice job including details like this error logging, and the enabled? check. 👍

jaredbeck · 2018-01-18T17:46:16Z

lib/paper_trail/record_trail.rb

+        object: recordable_object,
+        whodunnit: PaperTrail.whodunnit
+      }
+      data[:created_at] = Time.now


Why do we have to set created_at? Won't AR do this?

I don't remember. I think it may have been necessary in an earlier version I had but it appears to work fine without it, so I'll remove it. Good catch.

jaredbeck · 2018-01-18T17:50:14Z

lib/paper_trail/record_trail.rb

+      changes = {}
+      attributes.each do |k, v|
+        changes[k] = [@record[k], v]
+      end


Let's add a comment somewhere here explaining that we are constructing a hash that is compatible with the hash returned by ActiveModel::Dirty#changes.

TylerRick · 2018-01-22T19:41:10Z

Thanks for the detailed review, @jaredbeck. I've pushed up another commit to fix the things you suggested.

- Use a guard in `record_update_columns` - Use Timecop.freeze so that we can guarantee that an expectation will pass - Add some comments

jaredbeck

Looks good. I think we can merge this as is, but maybe we should follow up with some documentation in the readme also, or else people will not know this feature exists.

jaredbeck · 2018-01-23T03:22:40Z

Merged. Thanks for your contribution, Tyler!

.. maybe we should follow up with some documentation in the readme also, or else people will not know this feature exists.

Please follow up with a docs PR, so other people can discover this feature. Thanks!

Jakanapes · 2018-01-25T14:10:59Z

This is excellent, we were just looking at how to solve this issue cleanly.

thejosk · 2018-02-12T17:31:16Z

@jaredbeck Can you please provide documentation for this?

jaredbeck · 2018-02-12T17:59:27Z

@jaredbeck Can you please provide documentation for this?

As I said above, contributions of documentation are welcome, thanks!

@jaredbeck

* Add paper_trail.update_columns Add paper_trail.update_columns so you can record a version when using update_columns (which skips all callbacks, including the after_update callback). * Change recordable_object_changes to not have a default for the changes argument so that we don't need an exception to Style/MethodCallWithoutArgsParentheses * Add back the duplication between record_update and record_update_columns, at @jaredbeck's request * - Add Changelog entry for `paper_trail.update_columns` - Use a guard in `record_update_columns` - Use Timecop.freeze so that we can guarantee that an expectation will pass - Add some comments

Add paper_trail.update_columns

18862c1

Add paper_trail.update_columns so you can record a version when using update_columns (which skips all callbacks, including the after_update callback).

TylerRick mentioned this pull request Jan 15, 2018

is there a way get papertrail to work when my app uses update_attributes_without_callbacks #456

Closed

TylerRick added 2 commits January 16, 2018 16:47

Change recordable_object_changes to not have a default for the changes

199f0a7

argument so that we don't need an exception to Style/MethodCallWithoutArgsParentheses

Add back the duplication between record_update and

1990fc4

record_update_columns, at @jaredbeck's request

jaredbeck requested changes Jan 18, 2018

View reviewed changes

TylerRick force-pushed the add_record_update_columns branch from dfb5983 to 8b8d9a4 Compare January 22, 2018 19:42

- Add Changelog entry for paper_trail.update_columns

3b9ee26

- Use a guard in `record_update_columns` - Use Timecop.freeze so that we can guarantee that an expectation will pass - Add some comments

TylerRick force-pushed the add_record_update_columns branch from 8b8d9a4 to 3b9ee26 Compare January 22, 2018 19:50

Merge branch 'master' into add_record_update_columns

85ad070

jaredbeck approved these changes Jan 23, 2018

View reviewed changes

jaredbeck merged commit 3960d7f into paper-trail-gem:master Jan 23, 2018

TylerRick deleted the add_record_update_columns branch February 15, 2018 17:15

iiwo mentioned this pull request Aug 1, 2022

paper_trail.update_columns is setting an incorrect Version#created_at value #1395

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add paper_trail.update_columns #1037

Add paper_trail.update_columns #1037

TylerRick commented Jan 15, 2018

jaredbeck commented Jan 16, 2018

TylerRick commented Jan 17, 2018 •

edited

jaredbeck left a comment

jaredbeck Jan 18, 2018

TylerRick Jan 22, 2018

jaredbeck Jan 18, 2018

jaredbeck Jan 18, 2018

jaredbeck Jan 18, 2018

jaredbeck Jan 18, 2018

jaredbeck Jan 18, 2018

jaredbeck Jan 18, 2018

TylerRick Jan 22, 2018

jaredbeck Jan 18, 2018

TylerRick commented Jan 22, 2018

jaredbeck left a comment

jaredbeck commented Jan 23, 2018

Jakanapes commented Jan 25, 2018

thejosk commented Feb 12, 2018

jaredbeck commented Feb 12, 2018

Add paper_trail.update_columns #1037

Add paper_trail.update_columns #1037

Conversation

TylerRick commented Jan 15, 2018

jaredbeck commented Jan 16, 2018

TylerRick commented Jan 17, 2018 • edited

jaredbeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerRick commented Jan 22, 2018

jaredbeck left a comment

Choose a reason for hiding this comment

jaredbeck commented Jan 23, 2018

Jakanapes commented Jan 25, 2018

thejosk commented Feb 12, 2018

jaredbeck commented Feb 12, 2018

TylerRick commented Jan 17, 2018 •

edited