Skip to content

Commit

Permalink
An idea to combat model pollution
Browse files Browse the repository at this point in the history
Problem
-------

`has_paper_trail` adds too many methods to the ActiveRecord model.

> If I'm counting correctly, installing the paper_trail gem adds 36 instance
> methods and 10 class methods to all of your active record models. Of those
> 46, 13 have a prefix, either "pt_" or "paper_trail_". I don't know what the
> best way to deal with this is. Ideally, we'd add far fewer methods to
> people's models. If we were able to add fewer methods to models, then I
> wouldn't mind prefixing all of them.
> #703

Solution
--------

Add only two methods to the AR model.

1. An instance method `#paper_trail`
2. A class method `.paper_trail`

The instance method would return a `RecordTrail` and the class method would
return a `ClassTrail`. Those names are totally up for debate.

Advantages
----------

- Plain ruby, easy to understand
- Adding new methods to e.g. the `RecordTrail` does not add any methods to
  the AR model.
- Methods privacy is more strongly enforced.
- Enables isolated testing of e.g. `RecordTrail`; it can be constructed with a
  mock AR instance.

Disadvantages
-------------

- Two new classes, though they are simple.
  • Loading branch information
jaredbeck committed Feb 23, 2016
1 parent 7fd529d commit 8b3a04c
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 15 deletions.
2 changes: 1 addition & 1 deletion .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ Metrics/MethodLength:
# Offense count: 2
# Configuration parameters: CountComments.
Metrics/ModuleLength:
Max: 281
Max: 287

# Offense count: 6
Metrics/PerceivedComplexity:
Expand Down
22 changes: 14 additions & 8 deletions lib/paper_trail/has_paper_trail.rb
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,16 @@ def paper_trail_version_class
# Wrap the following methods in a module so we can include them only in the
# ActiveRecord models that declare `has_paper_trail`.
module InstanceMethods
# Returns true if this instance is the current, live one;
# returns false if this instance came from a previous version.
def paper_trail
::PaperTrail::RecordTrail.new(self)
end

def live?
source_version.nil?
::ActiveSupport::Deprecation.warn(
"Deprecated: use record.paper_trail.live? instead of record.live?",
caller(1)
)
paper_trail.live?
end

# Returns who put the object into its current state.
Expand All @@ -200,6 +206,10 @@ def clear_rolled_back_versions
send(self.class.versions_association_name).reload
end

def source_version
send self.class.version_association_name
end

# Returns the object (not a Version) as it was at the given timestamp.
def version_at(timestamp, reify_options={})
# Because a version stores how its object looked *before* the change,
Expand Down Expand Up @@ -300,10 +310,6 @@ def will_record_after_update?
on.nil? || on.include?(:update)
end

def source_version
send self.class.version_association_name
end

def record_create
if paper_trail_switched_on?
data = {
Expand Down Expand Up @@ -399,7 +405,7 @@ def clear_version_instance!
# Invoked via callback when a user attempts to persist a reified
# `Version`.
def reset_timestamp_attrs_for_update_if_needed!
return if self.live?
return if self.paper_trail.live?
timestamp_attributes_for_update_in_model.each do |column|
# ActiveRecord 4.2 deprecated `reset_column!` in favor of
# `restore_column!`.
Expand Down
14 changes: 14 additions & 0 deletions lib/paper_trail/record_trail.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
module PaperTrail
# Represents the "paper trail" for a single record.
class RecordTrail
def initialize(record)
@record = record
end

# Returns true if this instance is the current, live one;
# returns false if this instance came from a previous version.
def live?
@record.source_version.nil?
end
end
end
6 changes: 3 additions & 3 deletions spec/models/widget_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,11 @@

subject { widget.versions.last.reify }

it { expect(subject).not_to be_live }
it { expect(subject.paper_trail).not_to be_live }

it "should clear the `versions_association_name` virtual attribute" do
subject.save!
expect(subject).to be_live
expect(subject.paper_trail).to be_live
end

it "corresponding version should use the widget updated_at" do
Expand Down Expand Up @@ -133,7 +133,7 @@
before { PaperTrail.whodunnit = orig_name }

context "accessed from live model instance" do
specify { expect(widget).to be_live }
specify { expect(widget.paper_trail).to be_live }

it "should return the originator for the model at a given state" do
expect(widget.paper_trail_originator).to eq(orig_name)
Expand Down
8 changes: 5 additions & 3 deletions test/unit/model_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase
end

should 'be live' do
assert @widget.live?
assert @widget.paper_trail.live?
end

context 'which is then created' do
Expand All @@ -262,7 +262,7 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase
end

should 'be live' do
assert @widget.live?
assert @widget.paper_trail.live?
end

should "use the widget `updated_at` as the version's `created_at`" do
Expand Down Expand Up @@ -317,7 +317,9 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase
end

should 'have versions that are not live' do
assert @widget.versions.map(&:reify).compact.all? { |w| !w.live? }
assert @widget.versions.map(&:reify).compact.all? { |w|
!w.paper_trail.live?
}
end

should 'have stored changes' do
Expand Down

0 comments on commit 8b3a04c

Please sign in to comment.