Update scope_changed? for dirty deprecation warnings

[aaackerman]

Update scope_changed? method definer for dirty deprecation warnings for ActiveRecord versions 5.1 and greater. Warning seen here: https://github.com/rails/rails/blob/5-1-stable/activerecord/lib/active_record/attribute_methods/dirty.rb#L236.

Should there be any test updates for this?

[brendon]

Hi @aaackerman, thanks for this. I think we do need to test this change because the deprecation is in regards to this method being called in an after_ callback, but it might not always be called from a callback of that nature.

Would you mind quickly checking if scope_changed? is only ever used in after_ callbacks, and if not, write some tests to ensure it still functions properly for those cases also?

[aaackerman]

Thanks for the direction @brendon, and good call. I'll take a look and will update.

[aaackerman]

Hi @brendon. From what I see, scope_changed? is only used in a before_update callback. Because of this, we need to use changed_attribute_names_to_save instead of saved_changes.keys to make sure we’re checking the changes made before the update finishes. I started adding a test, but I’m not sure it adds much value as the tests below also ensure it works as expected within callbacks:

• ListWithCallbackTest#test_update_position_when_scope_changes
• ListTestWithDefault#test_update_position_when_scope_changes
• MultipleListsTest#test_check_scope_position
• ListTest#test_update_position_when_scope_changes

Do you think that’s enough coverage?

[brendon]

Hi @aaackerman, thanks for looking further into this. You're right, all the pathways to the scope_changed? methods only occur in before_ callbacks:

before_update :check_scope, unless: :act_as_list_no_update?
before_create "add_to_list_#{add_new_at}".to_sym, unless: :act_as_list_no_update?

And there is no public API apart from the scope_changed? method itself that can be called directly. It could be argued that that method should also be private but that's another topic.

I've not been able to find any documentation to say that the behaviour of changed has changed in before_ callbacks at all in 5.1. Can you point me to something? Could you also show me the deprecation message as you see it in your app?

[aaackerman]

The message I'm getting is: DEPRECATION WARNING: The behavior of 'change' inside of after callbacks will be changing in the next version of Rails. The new return value will reflect the behavior of calling the method after 'save' returned (e.g. the opposite of what it returns now). To maintain the current behavior, use 'saved_changes.keys' instead. (called from block (3 levels) in <top (required)> at <FILEPATH>

When I print out the caller I see the trace to scope changed.

gems/acts_as_list-0.9.11/lib/acts_as_list/active_record/acts/scope_method_definer.rb:20
gems/acts_as_list-0.9.11/lib/acts_as_list/active_record/acts/list.rb:444 in `internal_scope_changed?'
gems/acts_as_list-0.9.11/lib/acts_as_list/active_record/acts/list.rb:452 in `check_scope'

Being called in a before_ callback seems to be an bug in rails based on this issue thread https://github.com/rails/rails/issues/28594 and DHH's comment here https://github.com/rails/rails/commit/16ae3db5a5c6a08383b974ae6c96faac5b4a3c81. I agree that switching to the specific checks against pending changes and saved changes is a better practice. so it might be worth keeping this change even if the before callback bug is fixed. What do you think, @brendon ?

[brendon]

Ah I see, so there is a double-save going on. Is that in our code or in yours? If it's in ours then resolving that seems like the better path? If it's in yours, and unavoidable, then perhaps using the new methods for change detection would be better though it does make the code a bit more messy, and we have to be certain that these new methods return the correct results in before callbacks (that they detect the scope change correctly). :) Sorry about the delay, I was away on holiday :)

[aaackerman]

Sorry for the delay on this, I believe you're right that this is related to the double save issue on our end. I don't think it's necessary to fix here. Thanks for your patience and response.

[brendon]

Thanks @aaackerman :) That's not a problem. If you find out that there's something we can fix here, please don't hesitate to reopen the ticket :)