Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add touch on update configuration #329

Merged
merged 4 commits into from
Feb 13, 2019

Conversation

mgbatchelor
Copy link
Contributor

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
Copy link
Owner

brendon commented Oct 17, 2018

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
Copy link
Contributor Author

@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
Copy link
Owner

brendon commented Oct 17, 2018

Thanks @mgbatchelor :)

@mgbatchelor
Copy link
Contributor Author

@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 #327

@mgbatchelor mgbatchelor reopened this Oct 18, 2018
@mgbatchelor
Copy link
Contributor Author

Oops didn't mean to close/reopen 😊

@brendon
Copy link
Owner

brendon commented Oct 22, 2018

@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
Copy link
Contributor Author

@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
Copy link
Owner

brendon commented Oct 22, 2018

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
Copy link
Contributor Author

@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
Copy link
Owner

brendon commented Oct 22, 2018

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
Copy link
Contributor Author

@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
Copy link
Owner

brendon commented Oct 22, 2018

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
Copy link
Contributor Author

@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.

Allow for decrement_sequentially to update one record at a time, and also confirm to `touch_on_update` configuration. This change no longer calls Rails callbacks or validations when decrementing or incrementing the positions for each item when a reordering occurs.
@@ -22,6 +22,18 @@ def self.define_class_methods(caller_class, position_column)
@_quoted_position_column_with_table_name ||= "#{caller_class.quoted_table_name}.#{quoted_position_column}"
end

define_singleton_method :decrement_sequentially do
pluck(primary_key).each do |id|
where(primary_key => id).decrement_all
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be the case we just want to do self.class.where(primary_key => id).decrement_all instead of chaining off the existing query. thoughts @brendon?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure :) It'll make for a simpler query but makes the code read less simply. Let's leave it like this for now?

@brendon
Copy link
Owner

brendon commented Oct 23, 2018

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

@jdibiccari
Copy link

jdibiccari commented Jan 15, 2019

👋 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
Copy link
Contributor Author

bump @brendon

@brendon brendon merged commit 27f4050 into brendon:master Feb 13, 2019
@brendon
Copy link
Owner

brendon commented Feb 13, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants