mongoid / mongoid-history

Multi-user non-linear history tracking, auditing, undo, redo for mongoid.
https://rubygems.org/gems/mongoid-history
MIT License
392 stars 129 forks source link

Changing collection breaks history tracking #226

Closed joel-nyansa closed 6 years ago

joel-nyansa commented 6 years ago
    describe 'changing collection' do
      before :each do
        class Fish
          include Mongoid::Document
          include Mongoid::History::Trackable

          track_history on: [:species], modifier_field_optional: true
          store_in collection: :animals

          field :species
        end
      end

      after :each do
        Object.send(:remove_const, :Fish)
      end

      it 'should track history' do
        Fish.new.save!
      end
    end

=>

Mongoid::History
  track
    collections
      should track history (FAILED - 1)

Failures:

  1) Mongoid::History track collections should track history
     Failure/Error: @history_trackable_options ||= Mongoid::History.trackable_class_options[trackable_scope].prepared

     NoMethodError:
       undefined method `prepared' for nil:NilClass
     # ./lib/mongoid/history/trackable.rb:531:in `history_trackable_options'
     # ./lib/mongoid/history/trackable.rb:34:in `history_trackable_options'
     # ./lib/mongoid/history/trackable.rb:286:in `increment_current_version'
     # ./lib/mongoid/history/trackable.rb:299:in `track_history_for_action'
     # ./lib/mongoid/history/trackable.rb:255:in `track_create'

It looks like a result of this line which hashes the configuration by collection name. I tried adjusting it to hash by class name and search for any ancestor with a configuration (for inheritance) but it broke a bunch of tests.

dblock commented 6 years ago

And if you move store_in above that track_history does it change anything? Either way I think it's a bug and the scope should include the target collection. Try PRing a full spec and maybe a fix, I can try helping with that too.

jnfeinstein commented 6 years ago

@dblock yes, that fixes it. I can PR a fix that substitutes class name as the key instead of collection, but the tests wont pass. The other solution I was thinking is to store the options on the class itself but I think that messes with inheritance.

The end goal is to have a set-and-forget module that can be included anywhere, and ideally everywhere to track every change.

dblock commented 6 years ago

I think storing options on the class itself is the way to go, and it can use class level attributes that are copied on inheritance but become local to the child class when modified. I am not saying it's easy, but you'll be my hero if you can make it work :)

jnfeinstein commented 6 years ago

download

It's going to break some tests. I'll code it up and report back.

dblock commented 6 years ago

Closed via https://github.com/mongoid/mongoid-history/pull/227