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

Integrate versioning into AR touch method #1063

Merged
merged 3 commits into from
Mar 16, 2018
Merged
Show file tree
Hide file tree
Changes from 2 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,14 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/).

### Deprecated

- [#1063](https://github.com/airblade/paper_trail/pull/1063) - `touch_with_version` is deprecated in favor of using the original AR `touch` method
- [#1033](https://github.com/airblade/paper_trail/pull/1033) - Request variables
are now set using eg. `PaperTrail.request.whodunnit=` and the old way,
`PaperTrail.whodunnit=` is deprecated.

### Added

- [#1063](https://github.com/airblade/paper_trail/pull/1063) - AR `touch` method now creates a version if the `:on` option includes `touch`. By default it also includes the `touch` event now.
- [#1033](https://github.com/airblade/paper_trail/pull/1033) -
Set request variables temporarily using a block, eg.
`PaperTrail.request(whodunnit: 'Jared') do .. end`
Expand Down
21 changes: 16 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,6 @@ widget.paper_trail.previous_version
# Returns the widget (not a version) as it became next.
widget.paper_trail.next_version

# Generates a version for a `touch` event (`widget.touch` does NOT generate a
# version)
widget.paper_trail.touch_with_version

# Enable/disable PaperTrail, for Widget, for the current request (not all threads)
PaperTrail.request.disable_model(Widget)
PaperTrail.request.enable_model(Widget)
Expand Down Expand Up @@ -292,7 +288,7 @@ ignore `create` events:

```ruby
class Article < ActiveRecord::Base
has_paper_trail on: [:update, :destroy]
has_paper_trail on: [:update, :destroy, :touch]
end
```

Expand Down Expand Up @@ -337,6 +333,7 @@ class Article < ActiveRecord::Base
paper_trail.on_destroy # add destroy callback
paper_trail.on_update # etc.
paper_trail.on_create
paper_trail.on_touch
end
```

Expand Down Expand Up @@ -706,6 +703,20 @@ sql> delete from versions where created_at < 2010-06-01;
PaperTrail::Version.delete_all ['created_at < ?', 1.week.ago]
```

### 3.e. Trigger Version creation

At some point you may want to trigger a new version to be created. To do this we utilize the AR `touch` method.

```ruby
widget.touch
```

Note if you are using the `:on` option in your model you must specify the `touch` event also. For example:

```ruby
has_paper_trail on: [:update, :destroy, :touch]
```

## 4. Saving More Information About Versions

### 4.a. Finding Out Who Was Responsible For A Change
Expand Down
10 changes: 9 additions & 1 deletion lib/paper_trail/model_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,19 @@ def on_update
@model_class.paper_trail_options[:on] << :update
end

# Adds a callback that records a version after a "touch" event.
# @api public
def on_touch
@model_class.after_touch { |r|
r.paper_trail.record_update(force: true, in_after_callback: true)
}
end

# Set up `@model_class` for PaperTrail. Installs callbacks, associations,
# "class attributes", instance methods, and more.
# @api private
def setup(options = {})
options[:on] ||= %i[create update destroy]
options[:on] ||= %i[create update destroy touch]
options[:on] = Array(options[:on]) # Support single symbol
@model_class.send :include, ::PaperTrail::Model::InstanceMethods
setup_options(options)
Expand Down
16 changes: 11 additions & 5 deletions lib/paper_trail/record_trail.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ class RecordTrail
my_model_instance.paper_trail.whodunnit('John') is deprecated,
please use PaperTrail.request(whodunnit: 'John')
STR

DPR_TOUCH_WITH_VERSION = <<-STR.squish.freeze
my_model_instance.paper_trail.touch_with_version is deprecated,
please use my_model_instance.touch
STR

RAILS_GTE_5_1 = ::ActiveRecord.gem_version >= ::Gem::Version.new("5.1.0.beta1")

def initialize(record)
Expand Down Expand Up @@ -461,8 +467,9 @@ def source_version
# version records are inserted. It's unclear under which specific
# circumstances this technique should be adopted.
#
# @api public
# @deprecated
def touch_with_version(name = nil)
::ActiveSupport::Deprecation.warn(DPR_TOUCH_WITH_VERSION, caller(1))
Copy link
Member

Choose a reason for hiding this comment

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

Nice deprecation implementation

unless @record.persisted?
raise ::ActiveRecord::ActiveRecordError, "can not touch on a new record object"
end
Expand Down Expand Up @@ -563,17 +570,16 @@ def attribute_changed_in_latest_version?(attr_name)
# Rails 5.1 changed the API of `ActiveRecord::Dirty`. See
# https://github.com/airblade/paper_trail/pull/899
#
# Event can be any of the three (create, update, destroy).
# Event can be any of the following (create, update, destroy, touch).
Copy link
Member

Choose a reason for hiding this comment

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

"touch" is not an event. There are still only three events: create, update, and destroy. touch will cause the insertion of an "update" event. See record_update.

I think this will be confusing to many people, and I'm wondering how we can document it better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well should we make touch an event? I would simply add another optional argument to record_update(force:, in_after_callback:, event: "update") or add a record_touch method.

Otherwise I can simply add documentation in the readme next to the touch documentation stating that the event will be recorded as update

Copy link
Member

Choose a reason for hiding this comment

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

should we make touch an event?

I don't know.

Reasons against: A touch is simply an update. It updates timestamps. If we had a touch event, the lifecycle would be harder to understand. Instead of eg. create-update-update-destroy, it's eg. create-update-touch-touch-update-touch-destroy. The "middle part" of the lifecycle becomes heterogeneous. This heterogeneity would be a major breaking change.

Reasons for: Maintains a one-to-one correspondence between callbacks and events, which makes callbacks easier to understand I guess. It's not a big advantage.

So I guess I'm leaning against making touch an event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay then in that case, I simply updated the readme stating the event will be saved as update

#
# @api private
def attribute_in_previous_version(attr_name)
if RAILS_GTE_5_1
if @in_after_callback
@record.attribute_before_last_save(attr_name.to_s)
else
# Either we are doing a `touch_with_version` or `record_destroy`.
# Other events, like `record_create`, can only be done in an
# after-callback.
# We are performing a `record_destroy`. Other events,
# like `record_create`, can only be done in an after-callback.
@record.attribute_in_database(attr_name.to_s)
end
else
Expand Down
3 changes: 3 additions & 0 deletions spec/models/article_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,12 @@
describe "#touch_with_version" do
it "creates a version, ignoring the :only option" do
article = described_class.create

allow(::ActiveSupport::Deprecation).to receive(:warn)
expect { article.paper_trail.touch_with_version }.to change {
::PaperTrail::Version.count
}.by(+1)
expect(::ActiveSupport::Deprecation).to have_received(:warn).once
end
end
end
2 changes: 1 addition & 1 deletion spec/models/callback_modifier_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
context "when no callback-method used" do
it "sets paper_trail_options[:on] to [:create, :update, :destroy]" do
modifier = DefaultModifier.create!(some_content: FFaker::Lorem.sentence)
expect(modifier.paper_trail_options[:on]).to eq %i[create update destroy]
expect(modifier.paper_trail_options[:on]).to eq %i[create update destroy touch]
end

it "tracks destroy" do
Expand Down
4 changes: 4 additions & 0 deletions spec/models/not_on_update_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,21 @@
let!(:record) { described_class.create! }

it "creates a version, regardless" do
allow(::ActiveSupport::Deprecation).to receive(:warn)
expect { record.paper_trail.touch_with_version }.to change {
PaperTrail::Version.count
}.by(+1)
expect(::ActiveSupport::Deprecation).to have_received(:warn).once
end

it "increments the `:updated_at` timestamp" do
before = record.updated_at
allow(::ActiveSupport::Deprecation).to receive(:warn)
# Travel 1 second because MySQL lacks sub-second resolution
Timecop.travel(1) do
record.paper_trail.touch_with_version
end
expect(::ActiveSupport::Deprecation).to have_received(:warn).once
expect(record.updated_at).to be > before
end
end
Expand Down
2 changes: 2 additions & 0 deletions spec/models/on/empty_array_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ module On
describe "#touch_with_version" do
it "creates a version record" do
record = described_class.create(name: "Alice")
allow(::ActiveSupport::Deprecation).to receive(:warn)
record.paper_trail.touch_with_version
expect(::ActiveSupport::Deprecation).to have_received(:warn).once
expect(record.versions.length).to(eq(1))
v = record.versions.first
expect(v.event).to(eq("update"))
Expand Down
11 changes: 9 additions & 2 deletions spec/models/on/update_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@

module On
RSpec.describe Update, type: :model, versioning: true do
let(:record) { described_class.create(name: "Alice") }
Copy link
Member

Choose a reason for hiding this comment

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

Please do not use let. I believe it makes tests harder to understand, especially when it's too far away, lexically, from the test.

Choose a reason for hiding this comment

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

Reading this from 2020, and I completely agree. It can be useful for dynamic values to DRY up tests but if it's overused, it becomes a nightmare. Additionally, the let block is only executed once it is called so that alone can be confusing and have detrimental side effects with tests.


describe "#versions" do
it "only creates one version record, for the update event" do
record = described_class.create(name: "Alice")
record.update_attributes(name: "blah")
record.destroy
expect(record.versions.length).to(eq(1))
Expand All @@ -17,13 +18,19 @@ module On

context "#paper_trail_event" do
it "rembembers the custom event name" do
record = described_class.create(name: "Alice")
record.paper_trail_event = "banana"
record.update_attributes(name: "blah")
record.destroy
expect(record.versions.length).to(eq(1))
expect(record.versions.last.event).to(eq("banana"))
end
end

describe "#touch" do
it "does not create a version for the touch event" do
count = record.versions.count
expect(count).to eq(count)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Good test. Thanks for finding and using the existing models.

end
end
2 changes: 2 additions & 0 deletions spec/models/post_with_status_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@
expect(post.versions.count).to eq(1)
expect(post.status).to eq("draft")
Timecop.travel 1.second.since # because MySQL lacks fractional seconds precision
allow(::ActiveSupport::Deprecation).to receive(:warn)
post.paper_trail.touch_with_version
expect(::ActiveSupport::Deprecation).to have_received(:warn).once
expect(post.versions.count).to eq(2)
expect(post.versions.last[:object]).to include("status: 0")
expect(post.paper_trail.previous_version.status).to eq("draft")
Expand Down
20 changes: 19 additions & 1 deletion spec/models/widget_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -232,21 +232,39 @@

describe "#touch_with_version", versioning: true do
it "creates a version" do
allow(::ActiveSupport::Deprecation).to receive(:warn)
count = widget.versions.size
# Travel 1 second because MySQL lacks sub-second resolution
Timecop.travel(1) do
widget.paper_trail.touch_with_version
end
expect(widget.versions.size).to eq(count + 1)
expect(::ActiveSupport::Deprecation).to have_received(:warn).once
end

it "increments the `:updated_at` timestamp" do
allow(::ActiveSupport::Deprecation).to receive(:warn)
time_was = widget.updated_at
# Travel 1 second because MySQL lacks sub-second resolution
Timecop.travel(1) do
widget.paper_trail.touch_with_version
end
expect(widget.updated_at).to be > time_was
expect(::ActiveSupport::Deprecation).to have_received(:warn).once
end
end

describe "touch", versioning: true do
it "creates a version" do
expect { widget.touch }.to change {
widget.versions.count
}.by(+1)
end

it "does not create a version using without_versioning" do
count = widget.versions.count
widget.paper_trail.without_versioning do
expect(count).to eq(count)
end
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the implementation of this test. It's like saying expect(7).to eq(7), right? The local variable count is unchanging, right?

end
end

Expand Down
8 changes: 5 additions & 3 deletions spec/paper_trail/model_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,15 @@
context "and then updated without any changes" do
before { @widget.touch }

it "not have a new version" do
expect(@widget.versions.length).to(eq(1))
it "to have two previous versions" do
expect(@widget.versions.length).to(eq(2))
end
end

context "and then updated with changes" do
before { @widget.update_attributes(name: "Harry") }

it "have two previous versions" do
it "have three previous versions" do
expect(@widget.versions.length).to(eq(2))
end

Expand Down Expand Up @@ -381,7 +381,9 @@

context "given a symbol, specifying a method name" do
it "does not create a new version" do
allow(::ActiveSupport::Deprecation).to receive(:warn)
@widget.paper_trail.without_versioning(:touch_with_version)
expect(::ActiveSupport::Deprecation).to have_received(:warn).once
expect(@widget.versions.length).to(eq(@count))
expect(::PaperTrail.request.enabled_for_model?(Widget)).to eq(true)
end
Expand Down