Skip to content

with_versioning broken in 3.0.0 #312

Closed
jacobat opened this Issue Jan 6, 2014 · 17 comments

3 participants

@jacobat
jacobat commented Jan 6, 2014

The introduction of the rspec integration in 3.0.0 has broken with_versioning as this spec demonstrates:

describe "with_versioning" do
  with_versioning do
    it "should version records" do
      PaperTrail.should be_enabled
    end
  end
end
@batter
Collaborator
batter commented Jan 6, 2014

Do you have a special with_versioning block from your test helper (such as shown in this example group on the README)? If so, you should remove it for 3.0.0 compatibility, because with_versioning is now provided to RSpec by the built-in RSpec helper (which was introduced in 3.0.0).

Please see this block in the README about the RSpec helper for more details on how it works.

@jacobat
jacobat commented Jan 6, 2014

Actually with_versioning seems to not be provided. I just tried setting up a Rails 4.0.2 app from scratch and created a spec matching the one in the example in the README and now running rspec I get:

/Users/jacob/papertraildemo/spec/with_options_spec.rb:4:in `block in <top (required)>': undefined method `with_versioning' for #<Class:0x007fde6bbbdba8> (NoMethodError)
        from /Users/jacob/.rbenv/versions/1.9.3-p448/gemsets/bundler/gems/rspec-core-2.14.5/lib/rspec/core/example_group.rb:246:in `module_eval'
        from /Users/jacob/.rbenv/versions/1.9.3-p448/gemsets/bundler/gems/rspec-core-2.14.5/lib/rspec/core/example_group.rb:246:in `subclass'
...

I've published a minimal Rails app as an example here: https://github.com/jacobat/papertrail_error

@batter
Collaborator
batter commented Jan 6, 2014

@jacobat - You are correct, the docs on the README are mis-stated. It appears you can only use the with_versioning block matcher within an it block on an RSpec test. I'm going to try to look into the DSL to see if there is some sort of an easy way to add custom blocks with context like that to the base set of blocks, but I'm not sure that it is possible. Hopefully I can find a way. Any suggestions you or anyone else has would be helpful.

You can also use the :versioning => true on your describe blocks as a replacement for this, and that definitely works.

@jacobat
jacobat commented Jan 7, 2014

It used to work. Before upgrading to 3.0 we had an around hook like:

around(:each) do |example|
  with_versioning do
    example.run
  end
end

This no longer works due to the way the RSpec integration is made (because it runs it's own before-each hook, that will disable PaperTrail for all examples).

I don't think it's great to hook into the RSpec tagging system with :versioning => true as it is very implicit. If you don't know PaperTrails RSpec integration, you're very likely to miss the semantics of it. I much prefer the old with_versioning style as it is much more explicit what's going on.

Actually I would prefer to to make the whole RSpec integration optional and disabled by default. I wouldn't mind having with_versioning available - maybe just by including a module into my spec_helper or something like that. This automagic integration is too much magic for my taste. But that's a personal preference of course.

@batter
Collaborator
batter commented Jan 8, 2014

@jacobat - you had a special with_versioning method included on your spec_helperlike the one on the README?

@jacobat
jacobat commented Jan 8, 2014

Yes, exactly.

@batter batter was assigned Jan 10, 2014
@batter
Collaborator
batter commented Feb 20, 2014

@jacobat - Stepped away from this for a while but trying to wrap this up. Can you share what your helper method looked like?

@jacobat
jacobat commented Feb 20, 2014

My helper method used to look like:

def with_versioning
  was_enabled = PaperTrail.enabled?
  PaperTrail.enabled = true
  begin
    yield
  ensure
    PaperTrail.enabled = was_enabled
  end
end
@batter
Collaborator
batter commented Feb 21, 2014

So I've considered some options. The before(:each) { PaperTrail.enabled = false } could be changed to a before(:all), and then the I thought helper would perform the way it used to as you describe, the only issue is that the with_versioning block will get triggered before the before(:all) block, and then PaperTrail.enabled? gets set to false by the before(:all) block right before the if statement it seems, like this:

# prior: PaperTrail.enabled? == true
with_versioning do # this block call sets `PaperTrail.enabled = true`, doesn't see `before(:all)` block
  # before(:all) block gets triggered here, sets `PaperTrail.enabled = false`
  it { PaperTrail.should be_enabled } # fails!
end

I figured the before(:all) block at the Rspec.configure level would set it to false before any other blocks get evaluated, but it seems to slip into order right before before(:each) blocks get executed. Now I'm really stumped on what to do. It might make sense for PaperTrail.config class to default the enabled value depending on what environment it's being run in (in RSpec, Cucumber, or when RACK_ENV or RAILS_ENV is test, default to false instead of true, but that seems like such a hack solution). I don't understand why the before(:all) block doesn't trigger at the beginning like the terminology would suggest.

@juanpastas

This is working for me:

it 'saves each change in record' do
  with_versioning do
    PaperTrail.should be_enabled
  end
end

But when I try with an actual record, it does not work:

      described_class.all.size.should == 0
      model = create(:model)
      described_class.all.size.should == 1
      model.versions.size.should == 0
      model.update_attribute :first, 'rty'
      model.reload.versions.size.should == 1   # Fails here
      model.versions.first.changeset.should == { :first => ['qwe', 'rty'] }
@batter
Collaborator
batter commented Mar 3, 2014

@juanpastas - this is within an 'it' block? What is the create statement on line 2 there? Is that using fixtures or a factory or something? There are a lot of things that could factor into why this is failing. Does it work if you use the :versioning => true metadata option?

@juanpastas

Yes, that's in an it block, I am using factories, I will try the versioning option, and let you know.

@juanpastas

Is working better now, but I am having this issue now:

it 'saves each change in record', :versioning => true do
    # with_versioning do
      PaperTrail.should be_enabled

      described_class.all.size.should == 0
      model = create(:model)
      described_class.all.size.should == 1
      model.versions.size.should == 1
      model.update_attribute :first, 'rty'
      # Not working in test env
      # pending 'Check https://github.com/airblade/paper_trail/issues/312'
      model.reload.versions.size.should == 2
      model.update_attribute :first, 'three'
      model.reload.versions.size.should == 3
      binding.pry
      model.versions.last.changes.should == { :first => ['rty', 'three'] }
    # end
  end
       expected: {:first=>["rty","three"]}
            got: nil (using ==)
@juanpastas

Also, see what I got from versions:

reservation.versions.map(&:changes)
=> [{}, {}, {}]
reservation.versions.map(&:changeset)
=> [nil, nil, nil]
@batter
Collaborator
batter commented Mar 4, 2014

@juanpastas - Do you have an object_changes column on your versions table? See the Diffing Versions section of the README. You can generate one with the install generator via: rails g paper_trail:install --with_changes.

@juanpastas
@batter batter added a commit that referenced this issue Mar 5, 2014
@batter batter Clarify install generator option for adding :object_changes column mi…
…gration to install in README; re #312 [ci skip]
d139722
@batter batter closed this in b75b009 Mar 11, 2014
@batter
Collaborator
batter commented Mar 14, 2014

@jacobat - Just released v3.0.1, which contains the fix for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.