Use after_save instead of after_commit for clear_scope_changed callback

[Flixt]

The problem is, when running multiple updates of the same objects within the same transaction, only firing clear_scope_changed after the transaction commit can lead to duplicate positions.

Without the change the test included in this PR fails with (run with ruby -I test test/test_list.rb --name test_multiple_updates_within_transaction):

Run options: --name test_multiple_updates_within_transaction --seed 61388

# Running:

F

Finished in 0.025951s, 38.5342 runs/s, 269.7391 assertions/s.

  1) Failure:
MultiUpdateTest#test_multiple_updates_within_transaction [test/test_list.rb:668]:
Expected: [1, 2]
  Actual: [1, 1]

1 runs, 7 assertions, 1 failures, 0 errors, 0 skips

Actually I have no idea if changing this callback from after_commit to after_save` is the "correct" way to fix this.

[brendon]

Hi @Flixt, sorry for the delay. Summer holidays here :)

At the very least it should be after_save_commit. This could potentially be quite a large change if people are depending on callback order. Fix it up and let's see how it tests out for everything else. For better backward compatibility you could use:

after_commit :clear_scope_changed, on: [ :create, :update ]

[Flixt]

Hi @brendon, no worries.

Fix it up and let's see how it tests out for everything else.

Using after_save_commit will not fix the problem. clear_scope_change would still only be called after the surrounding transaction is committed. I tried it with the test included in this PR and the test did not pass. All other tests passed, but I think the different between after_save_commit and after_commit is only that after_save_commit is not firing after a destroy is committed. So for our case of clearing a scope change it should not really make a difference.

This could potentially be quite a large change if people are depending on callback order.

I agree, but still there seems to be a bug triggered by exactly that line.

[brendon]

Sorry! I was reading the change backward! :D Let's see how it tests out across the suite :)

[brendon]

Looking good :) Would you mind adding a quick changelog line to the commit and we should be all good :)

[Flixt]

I added the line to the changelog. Not sure about if you'd want this change to be marked as a "breaking change".

[brendon]

Thanks @Flixt, that's a helpful fix :)

[brendon]

Are you happy to use the master branch for a while and let me know if you notice any issues? Then I can release a version.

[Flixt]

Yup, we will use the fix from our fork for a while.

[brendon]

I've released 1.1.0 :)