Add touch on update configuration

[mgbatchelor]

In some cases, there is not a want to change the updated_at timestamp. In my use case, I have an old record being deleted that has a low position, when that record is deleted the callback is triggered and all other items in the scope are repositioned, with an updated timestamp.

In this case the behavior is confusing. There may be a case where using move_to_top or move_to_bottom should reorder and update timestamps, but the use case I care about most is when a record is destroyed.

I could scope this setting to specify that it only has affect when sequential_updates is not enabled, because it only affects the decrement_all call here: https://github.com/mgbatchelor/acts_as_list/blob/1db1772efa716ebda76f366a5b9e86fe02cb1e9c/lib/acts_as_list/active_record/acts/list.rb#L335

Please advise. Thanks

[brendon]

Hi @mgbatchelor, thanks for this. I've had a look at the code and it makes sense. Could you elaborate on your last paragraph as I didn't quite get what you mean there.

There are failing tests but it's more to do with the need to pin a couple of gems (rake and public_suffix) to specific versions for old versions of ruby. If you'd like to do that as part of this PR that'd be helpful :D

[mgbatchelor]

@brendon all I was saying is that if sequential_updates is enabled, then it will still update the timestamps because its using decrement! which uses update_attribute. I don't think this is a problem, maybe I'll try to add a description that this feature is only compatible if not using sequential_updates

I'll take a look at the tests and see what I can do

[brendon]

Thanks @mgbatchelor :)

[mgbatchelor]

@brendon updated to lockdown rake version for ruby_19 - I don't understand why TravisCI works sometimes, and not others. I'm not sure where the public_suffix error disappeared to either...

However, I'm a fan of https://github.com/swanandp/acts_as_list/pull/327

[mgbatchelor]

Oops didn't mean to close/reopen 😊

[brendon]

@mgbatchelor thanks for that :) I think we shouldn't be inconsistent, so perhaps we should be using update_all for sequential updates too? It's probably also faster. What do you think?

[mgbatchelor]

@brendon calling decrement! https://apidock.com/rails/ActiveRecord/Base/decrement%21 calls update_attribute https://api.rubyonrails.org/classes/ActiveRecord/Persistence.html#method-i-update_attribute which just skips validations.

I would be concerned making this change and making sure its comparable with all versions of ActiveRecord. A quick search at the current versions, it would be possible by calling decrement(position_column).save(validate: false, touch: false) or something like that.

I could try give that a go, and if its possible in other versions

[brendon]

From what I could read, touch: false is Rails 5 only. Could you do:

where(id: id).update_attributes position:, 'position - 1'

inside the loop. Maybe just pluck the id's instead of creating all those objects and loop over the id's instead?

[mgbatchelor]

@brendon sure I can make the sequential_updates more efficient, however, I don't think I can make the functionality consistent with the touch_to_update: false configuration

[brendon]

I think it should be possible :) Why don't we extract to a method called decrement_sequentially (like we already do with decrement_all and then within there assemble our update line and call off to update_all_with_touch?

decrement_sequentially can be called for each id in the plucked id's. What do you think? :)

[mgbatchelor]

@brendon

Maybe just pluck the id's instead of creating all those objects and loop over the id's instead?

If we pluck the IDs and then update one record at a time with a raw SQL statement using update_all_with_touch, we would have the instance to have AR callbacks fired - which currently happens in decrement!(...).

If you think the callbacks should be removed when sequential_updates: true, then thats a different discussion. We would need to see if the intent of sequential_updates still works from #246

The SQL and refactor you're describing isn't the problem, and I agree that would be much cleaner and faster.

[brendon]

Thanks @mgbatchelor, I think the execution of callbacks in this situation is probably an undocumented side effect. I had a look through #246 and couldn't find any explicit mention of preference for callbacks happening. Indeed, before this, callbacks weren't executed because we were just executing raw sql through update_all. I think we can safely move back to that situation for sequential updates.

At the end of the day it's mostly a matter of consistency.

[mgbatchelor]

@brendon totally agree!

I'll push up some additional commits later - and confirm the intent of #246 was properly tested so that doesn't get broken.

[brendon]

sigh the test suite :) I've restarted the one timeout'd build to see if it passes :)

[jdibiccari]

:wave: I'm excited to see that this is in the works! I ran into an issue recently where I needed to order a list of records by updated_at. However, since we are using acts_as_list, whenever a record is deleted all of the records whose positions are updated have their timestamps updated as well. This leads to very confusing behavior 😄 Any idea when this might be in a place to merge?

[mgbatchelor]

bump @brendon

[brendon]

Sorry @mgbatchelor! It was holiday time in the Southern Hemisphere when this came in and I've only just gotten around to tending all my open source commitments :)

Thanks for your work on this. I'm merging it now.