-
Notifications
You must be signed in to change notification settings - Fork 901
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
@@ -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)) | ||
unless @record.persisted? | ||
raise ::ActiveRecord::ActiveRecordError, "can not touch on a new record object" | ||
end | ||
|
@@ -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). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. I think this will be confusing to many people, and I'm wondering how we can document it better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well should we make Otherwise I can simply add documentation in the readme next to the touch documentation stating that the event will be recorded as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't know. Reasons against: A 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
# | ||
# @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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,9 +5,10 @@ | |
|
||
module On | ||
RSpec.describe Update, type: :model, versioning: true do | ||
let(:record) { described_class.create(name: "Alice") } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please do not use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
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)) | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good test. Thanks for finding and using the existing models. |
||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
end | ||
end | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice deprecation implementation