Should this be a first citizen of ActiveRecord?

[arthurnn]

cc @rafaelfranca

I keep having to include this gem everytime I use after_commit callbacks. I found that a lot of folks, also, don't know that dirty attributes won't be there during after_commit.

So I wonder if this should be integrated with the dirty attributes module in AR.

Thanks for the gem Dylan

[dylanahsmith]

Yes, I think this should just be a part of active record. Ideally, changed_attributes would return the same thing as transaction_changed_attributes does in an after_commit callback.

[rafaelfranca]

You mean like saved_changes added in rails 5.1?

[rafaelfranca]

Oh, I see now. Yeah, it seems fine to be as a new method, but I don't think we should change an existing behavior. @sgrif may have a good option on this

[sgrif]

Yeah, having changed_attributes behave differently would be bad. I'm actually not sure what behavior this gem adds that isn't already covered by saved_changes

[dylanahsmith]

If there are multiple saves in a transaction, then saved_changes will only return the changes from the last save, which can cause changes to be missed when using an after_commit callback.

When I say "ideally" I mean excluding backwards compatibility concerns. If we agreed that was the ideal, then we could get there by adding the new method and deprecating using changed_attributes in an after_commit callback so that we could transition usage away from the behaviour that we wish to change before changing it. This way we could make it easy to do the right thing.

[dylanahsmith]

Here are the test failures I get from using saved_changes, which shows cases which it doesn't cover.

# Running:

..F...FFF

Finished in 0.067143s, 134.0423 runs/s, 148.9359 assertions/s.

  1) Failure:
TransactionChangesTest#test_transaction_changes_for_touch [test/transaction_changes_test.rb:87]:
--- expected
+++ actual
@@ -1 +1 @@
-[2017-08-28 18:49:24 UTC, 2017-08-28 18:49:25 UTC]
+[2017-08-28 18:49:25 UTC, 2017-08-28 14:49:24 -0400]


  2) Failure:
TransactionChangesTest#test_transaction_changes_for_double_save [test/transaction_changes_test.rb:34]:
Expected: ["Dylan", "Dillon"]
  Actual: nil

  3) Failure:
TransactionChangesTest#test_transaction_changes_after_reload_unsaved [test/transaction_changes_test.rb:66]:
Expected: ["Dylan", "Dillon"]
  Actual: nil

  4) Failure:
TransactionChangesTest#test_transaction_changes_after_reload_saved [test/transaction_changes_test.rb:54]:
Expected: ["Dylan", "Dillon"]
  Actual: nil

9 runs, 10 assertions, 4 failures, 0 errors, 0 skips

[vincentwoo]

Just ran into a nasty bug related to multiple saves in a transaction messing with previous_changes in an after_commit callback. I really think the behavior of this gem should be the default in AR.

[artemave]

Ended up here after a long debugging journey. Same problem as this gem is solving - thanks a lot! - but also it looks like similar problem occurs with after_save and saved_changes_to_*? (multiple updates within one transaction; callbacks are all run in the end and all have the same changes from the last update).