Fixes for discard 1.1.0

[kennyadsl]

Description

This PR fixes a bug with the last version of discard. I think the bug was more related to how we were using discard than to the discard update itself.

TLDR;

discarded? was returning true, even when the record updated to deleted_at wasn't yet persisted. This behavior seems to be changed (probably correctly?) so this validation

validate :validate_no_amount_used, if: :discarded?

was never performed on the first discard call, which is exactly what we need.

This PR changes that way to validate the record into a before_discard callback (similar to what we have with the after_destroy), throwing abort to interrupt the callback chain.

Asking for a @jarednorman review as well, since he's the new discard maintainer. 🎉

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message

[kennyadsl]

@jarednorman do you think we need to update the discard dependency along with this change? I'm not sure the old discard version is compatible with this new code. Let me know!

[jarednorman]

In reviewing the discard gem, I'm not sure what caused this change. The only change I see to how discarded? works since v1.0.0 is this one, which doesn't seem like it should change the behaviour.

[ericsaupe]

LGTM assuming it is backwards compatible. If not we'll need to update the discard version in the gemspec

[kennyadsl]

I've just run specs with discard 1.0.0 and they pass so I think it's backward compatible. 🎉

Investigation that leads me to open jhawthorn/discard/issues/51

I've also tried to run `master` specs with discard `1.0.0` and they pass. Next attempt done is using previous specs and discard `1.1.0`, which had the failure that originated this PR. I changed in discard locally:

run_callbacks(:discard) do
  update_attribute(self.class.discard_column, Time.current)
end

to:

with_transaction_returning_status do
  run_callbacks(:discard) do
    self[self.class.discard_column] = Time.current
    save
  end
end

and specs pass, so I assumed the problem is here. I then changed that block into:

run_callbacks(:discard) do
  self[self.class.discard_column] = Time.current
  save
end

removing with_transaction_returning_status and they still pass, so I assume the problem is with the with_transaction_returning_status change.

From with_transaction_returning_status API reference, it:

Executes method within a transaction and captures its return value as a status flag. If the status is true the transaction is committed, otherwise a ROLLBACK is issued. In any case the status flag is returned.

I think that removing this method somehow changes the state the record is when it's time to execute validations, but I'm not sure if it's an issue worth spending too much time on so I've opened this PR on discard.

I'd say to wait to see if we receive some feedback on that issue otherwise, we can safely merge this.

[jarednorman]

Now that I understand the change that caused this to break, I'm in favour of this fix. Thanks for addressing this! 🤘🏻