mongoid / mongoid-history

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

Fix inheritance of history_trackable_options #225

Closed dblock closed 6 years ago

dblock commented 6 years ago

See BUGBUG below, looks wrong.

      context 'subclass' do
        before :each do
          # BUGBUG: if this is not prepared, it inherits the subclass settings
          MyModel.history_trackable_options

          class CustomTracker < MyModel
            field :key

            track_history on: :key, changes_method: :my_changes, track_create: true

            def my_changes
              changes.merge('key' => "Save history-#{key}")
            end
          end
        end

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

        it 'should not override in parent class' do
          expect(MyModel.history_trackable_options[:changes_method]).to eq :changes
          expect(CustomTracker.history_trackable_options[:changes_method]).to eq :my_changes
        end

        it 'should default to :changes' do
          m = MyModel.create!(modifier: user)
          expect(m).to receive(:changes).exactly(3).times.and_call_original
          expect(m).not_to receive(:my_changes)
          m.save!
        end
dblock commented 6 years ago

@jnfeinstein I think #227 may have fixed this, care to add the specs from this issue and check it out?

jnfeinstein commented 6 years ago

This is absolutely fixed by #227. Previously any child class that ran track_history would overwrite the parent class's options, so you could only have one configuration per class family.

I noticed that the reset! method will not accomplish a full reset since these classes are caching their options via @history_trackable_options ||= mongoid_history_options.prepared. I have a feeling that reset! is not widely used in production, you may want to only include it as a spec helper.

dblock commented 6 years ago

Looking forward to a PR! Makes total sense re: spec helper. Thanks.

jnfeinstein commented 6 years ago

Fixed per #229.