discarded? value on validations and callbacks

[kennyadsl]

I'm trying to understand an issue we have on Solidus: https://github.com/solidusio/solidus/pull/3202.

These are two specs that describe the behavior we had with discard 1.0.0:

  describe 'discarded? value on callbacks/validations' do
    with_model :Post, scope: :all do
      table do |t|
        t.datetime :discarded_at
        t.timestamps null: false
      end

      model do
        include Discard::Model
        validate :validate_something, if: :discarded?
        before_discard :do_before_discard, if: :discarded?

        def validate_something; end
        def do_before_discard; end
      end
    end

    let!(:post) { Post.create! }

    it "runs validations with if: :discarded?" do
      expect(post).to receive(:validate_something)

      expect(post.discard).to be true
      expect(post).to be_discarded
    end

    it "does not run callbacks with if: :discarded?" do
      expect(post).to_not receive(:do_before_discard)

      expect(post.discard).to be true
      expect(post).to be_discarded
    end
  end

and in 1.1.0 the validation spec fails with:

1) Discard::Model discarded? value on callbacks runs validations with if: :discarded?
     Failure/Error: expect(post).to receive(:validate_something)

       (#<Post id: 1, discarded_at: "2019-05-08 10:52:56", created_at: "2019-05-08 10:52:56", updated_at: "2019-05-08 10:52:56">).validate_something(*(any args))
           expected: 1 time with any arguments
           received: 0 times with any arguments
     # ./spec/discard/model_spec.rb:24:in `block (3 levels) in <top (required)>'

so the validations are no more called if you use if: :discarded?, differently from the previous version.

I think version 1.1.0 is more consistent with what happens for callbacks, like before_save, since in that case they are not called either.

Not sure if there is a specific reason behind that and I'm not sure if this could be considered a regression since this specific scenario is not documented anywhere.

[jarednorman]

I think this is an enhancement, but one that should have been considered a breaking change and warranted a major version bump. I'm leaning towards keeping this behaviour, possibly testing it, and regretting that I didn't realize that this change broke things.

I'll also leave this issue open so that I can either document and potential test this behaviour to avoid regressions or changes to it.