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

Enable control over the order of AR callbacks #614

Merged
merged 7 commits into from
Oct 10, 2015
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ None

### Added

- Added callback-methods `paper_trail_update` `paper_trail_create` `paper_trail_destroy`
instead of has_paper_trail
[#593](https://github.com/airblade/paper_trail/pull/607)
Copy link
Member

Choose a reason for hiding this comment

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

  • Should read paper_trail_on_update.
  • PR number in link should be 614, right?

- Added `unversioned_attributes` option to `reify`.
[#579](https://github.com/airblade/paper_trail/pull/579)

Expand Down
21 changes: 21 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,27 @@ a.versions.size # 3
a.versions.last.event # 'update'
```

### Controlling the Order of AR Callbacks

You can also use the corresponding callback-methods seperately instead of using
the :on option. If you choose to use the callback-methods, PaperTrail will only
track the according events - so `paper_trail_on_create` is basically the same as
`has_paper_trail :on => :create`.
Copy link
Member

Choose a reason for hiding this comment

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

I'd put a heading like "Controlling the Order of AR Callbacks" above this paragraph, as (for me) that's the point of this new feature.

Copy link
Author

Choose a reason for hiding this comment

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

Added. Since I am not an english native-speaker, every suggestion to my readme addition is much appreciated.


```ruby
class Article < ActiveRecord::Base
has_paper_trail :on => []
paper_trail_on_destroy
paper_trail_on_update
paper_trail_on_create
end
```

The `paper_trail_on_destroy` method can be configured to be called `:before` or `:after` the
destroy event. This can be usefull if you are using a third party tool that alters the
destroy method (for example paranoia). If you do not pass an argument, it will default
to after_destroy.

## Choosing When To Save New Versions

You can choose the conditions when to add new versions with the `if` and
Expand Down
68 changes: 54 additions & 14 deletions lib/paper_trail/has_paper_trail.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,18 @@ module ClassMethods
# column if it exists. Default is true
#
def has_paper_trail(options = {})
options[:on] ||= [:create, :update, :destroy]

# Wrap the :on option in an array if necessary. This allows a single
# symbol to be passed in.
options[:on] = Array(options[:on])

setup_model_for_paper_trail(options)

setup_callbacks_from_options options[:on]
end

def setup_model_for_paper_trail(options = {})
# Lazily include the instance methods so we don't clutter up
# any more ActiveRecord models than we have to.
send :include, InstanceMethods
Expand All @@ -60,6 +72,7 @@ def has_paper_trail(options = {})
self.version_class_name = options[:class_name] || 'PaperTrail::Version'

class_attribute :paper_trail_options

self.paper_trail_options = options.dup

[:ignore, :skip, :only].each do |k|
Expand Down Expand Up @@ -87,26 +100,53 @@ def has_paper_trail(options = {})
:order => self.paper_trail_version_class.timestamp_sort_order
end

options[:on] ||= [:create, :update, :destroy]

# Wrap the :on option in an array if necessary. This allows a single
# symbol to be passed in.
options_on = Array(options[:on])

after_create :record_create, :if => :save_version? if options_on.include?(:create)
if options_on.include?(:update)
before_save :reset_timestamp_attrs_for_update_if_needed!, :on => :update
after_update :record_update, :if => :save_version?
after_update :clear_version_instance!
end
after_destroy :record_destroy, :if => :save_version? if options_on.include?(:destroy)

# Reset the transaction id when the transaction is closed.
after_commit :reset_transaction_id
after_rollback :reset_transaction_id
after_rollback :clear_rolled_back_versions
end

def setup_callbacks_from_options(options_on = [])
options_on.each do |option|
send "paper_trail_on_#{option}"
end
end

# Record version before or after "destroy" event
def paper_trail_on_destroy(recording_order = 'after')
unless %w[after before].include?(recording_order.to_s)
fail ArgumentError, 'recording order can only be "after" or "before"'
Copy link
Member

Choose a reason for hiding this comment

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

Nice use of fail here, and I appreciate the validation of arguments.

end

send "#{recording_order}_destroy",
:record_destroy,
:if => :save_version?

return if paper_trail_options[:on].include?(:destroy)
paper_trail_options[:on] << :destroy
end

# Record version after "update" event
def paper_trail_on_update
before_save :reset_timestamp_attrs_for_update_if_needed!,
:on => :update
after_update :record_update,
:if => :save_version?
after_update :clear_version_instance!

return if paper_trail_options[:on].include?(:update)
paper_trail_options[:on] << :update
end

# Record version after "create" event
def paper_trail_on_create
after_create :record_create,
:if => :save_version?

return if paper_trail_options[:on].include?(:create)
paper_trail_options[:on] << :create
end

# Switches PaperTrail off for this class.
def paper_trail_off!
PaperTrail.enabled_for_model(self, false)
Expand Down
17 changes: 17 additions & 0 deletions spec/models/animal_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,22 @@
expect(dog).to be_instance_of(Dog)
end
end

context 'with callback-methods' do
context 'when only has_paper_trail set in super class' do
let(:callback_cat) { Cat.create(:name => 'Markus') }

it 'trails all events' do
callback_cat.update_attributes(:name => 'Billie')
callback_cat.destroy
expect(callback_cat.versions.collect(&:event)).to eq %w(create update destroy)
end

it 'does not break reify' do
callback_cat.destroy
expect { callback_cat.versions.last.reify }.not_to raise_error
end
end
end
end
end
97 changes: 97 additions & 0 deletions spec/models/callback_modifier_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
require 'rails_helper'
require 'support/callback_modifier'

describe CallbackModifier, :type => :model do
with_versioning do
describe 'callback-methods', :versioning => true do
describe 'paper_trail_on_destroy' do
it 'should add :destroy to paper_trail_options[:on]' do
modifier = NoArgDestroyModifier.create!(:some_content => Faker::Lorem.sentence)
expect(modifier.paper_trail_options[:on]).to eq [:destroy]
end

context 'when :before' do
it 'should create the version before destroy' do
modifier = BeforeDestroyModifier.create!(:some_content => Faker::Lorem.sentence)
modifier.test_destroy
expect(modifier.versions.last.reify).not_to be_flagged_deleted
end
end

context 'when :after' do
it 'should create the version after destroy' do
modifier = AfterDestroyModifier.create!(:some_content => Faker::Lorem.sentence)
modifier.test_destroy
expect(modifier.versions.last.reify).to be_flagged_deleted
end
end

context 'when no argument' do
it 'should default to after destroy' do
modifier = NoArgDestroyModifier.create!(:some_content => Faker::Lorem.sentence)
modifier.test_destroy
expect(modifier.versions.last.reify).to be_flagged_deleted
end
end
end

describe 'paper_trail_on_update' do
it 'should add :update to paper_trail_options[:on]' do
modifier = UpdateModifier.create!(:some_content => Faker::Lorem.sentence)
expect(modifier.paper_trail_options[:on]).to eq [:update]
end

it 'should create a version' do
modifier = UpdateModifier.create!(:some_content => Faker::Lorem.sentence)
modifier.update_attributes! :some_content => 'modified'
expect(modifier.versions.last.event).to eq 'update'
end
end

describe 'paper_trail_on_create' do
it 'should add :create to paper_trail_options[:on]' do
modifier = CreateModifier.create!(:some_content => Faker::Lorem.sentence)
expect(modifier.paper_trail_options[:on]).to eq [:create]
end

it 'should create a version' do
modifier = CreateModifier.create!(:some_content => Faker::Lorem.sentence)
expect(modifier.versions.last.event).to eq 'create'
end
end

context 'when no callback-method used' do
it 'should set paper_trail_options[:on] to [:create, :update, :destroy]' do
modifier = DefaultModifier.create!(:some_content => Faker::Lorem.sentence)
expect(modifier.paper_trail_options[:on]).to eq [:create, :update, :destroy]
end

it 'should default to track destroy' do
modifier = DefaultModifier.create!(:some_content => Faker::Lorem.sentence)
modifier.destroy
expect(modifier.versions.last.event).to eq 'destroy'
end

it 'should default to track update' do
modifier = DefaultModifier.create!(:some_content => Faker::Lorem.sentence)
modifier.update_attributes! :some_content => 'modified'
expect(modifier.versions.last.event).to eq 'update'
end

it 'should default to track create' do
modifier = DefaultModifier.create!(:some_content => Faker::Lorem.sentence)
expect(modifier.versions.last.event).to eq 'create'
end
end

context 'when only one callback-method' do
it 'does only track the corresponding event' do
modifier = CreateModifier.create!(:some_content => Faker::Lorem.sentence)
modifier.update_attributes!(:some_content => 'modified')
modifier.test_destroy
expect(modifier.versions.collect(&:event)).to eq ['create']
end
end
end
end
end
28 changes: 28 additions & 0 deletions spec/support/callback_modifier.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
class BeforeDestroyModifier < CallbackModifier
has_paper_trail :on => []
paper_trail_on_destroy :before
end

class AfterDestroyModifier < CallbackModifier
has_paper_trail :on => []
paper_trail_on_destroy :after
end

class NoArgDestroyModifier < CallbackModifier
has_paper_trail :on => []
paper_trail_on_destroy
end

class UpdateModifier < CallbackModifier
has_paper_trail :on => []
paper_trail_on_update
end

class CreateModifier < CallbackModifier
has_paper_trail :on => []
paper_trail_on_create
end

class DefaultModifier < CallbackModifier
has_paper_trail
end
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 you choose to put these classes in spec/support instead of in test/dummy/app/models like their parent callback_modifier.rb?

Copy link
Author

Choose a reason for hiding this comment

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

Because for me they are more like helper-classes wrapped around the model, it didn't feel right to put them there with all the "normal" activerecord models.

Copy link
Member

Choose a reason for hiding this comment

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

I see your point, but I'd still rather keep all the test models in one place.

16 changes: 16 additions & 0 deletions test/dummy/app/models/callback_modifier.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class CallbackModifier < ActiveRecord::Base
has_paper_trail :on => []

def test_destroy
transaction do
run_callbacks(:destroy) do
self.deleted = true
save!
end
end
end

def flagged_deleted?
deleted?
end
end
7 changes: 7 additions & 0 deletions test/dummy/db/migrate/20110208155312_set_up_test_tables.rb
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,11 @@ def self.up
t.boolean :scoped, :default => true
end

create_table :callback_modifiers, :force => true do |t|
t.string :some_content
t.boolean :deleted, :default => false
end

create_table :chapters, :force => true do |t|
t.string :name
end
Expand Down Expand Up @@ -277,5 +282,7 @@ def self.down
remove_index :version_associations, :column => [:version_id]
remove_index :version_associations, :name => 'index_version_associations_on_foreign_key'
drop_table :version_associations
drop_table :filter_modifier
Copy link
Member

Choose a reason for hiding this comment

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

I think this line is probably an oversight. We can remove it, right?

Copy link
Author

Choose a reason for hiding this comment

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

Ohhh.. Yeah that one is very old. No one will miss that line ;)

Copy link
Member

Choose a reason for hiding this comment

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

Please don't forget to delete this line.

drop_table :callback_modifiers
end
end
5 changes: 5 additions & 0 deletions test/dummy/db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@
t.boolean "scoped", default: true
end

create_table "callback_modifiers", force: :cascade do |t|
t.string "some_content"
t.boolean "deleted", default: false
end

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