-
-
Notifications
You must be signed in to change notification settings - Fork 357
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
Add touch on update configuration #329
Conversation
…uch every record that was reordered.
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 |
@brendon all I was saying is that if sequential_updates is enabled, then it will still update the timestamps because its using I'll take a look at the tests and see what I can do |
Thanks @mgbatchelor :) |
Ruby 2.0.0 required in rake as of 12.3.0 ruby/rake@4135948 https://github.com/ruby/rake/blob/master/History.rdoc#1230
Oops didn't mean to close/reopen 😊 |
@mgbatchelor thanks for that :) I think we shouldn't be inconsistent, so perhaps we should be using |
@brendon calling 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 I could try give that a go, and if its possible in other versions |
From what I could read,
inside the loop. Maybe just pluck the |
@brendon sure I can make the sequential_updates more efficient, however, I don't think I can make the functionality consistent with the |
I think it should be possible :) Why don't we extract to a method called
|
If we pluck the IDs and then update one record at a time with a raw SQL statement using If you think the callbacks should be removed when The SQL and refactor you're describing isn't the problem, and I agree that would be much cleaner and faster. |
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 At the end of the day it's mostly a matter of consistency. |
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
sigh the test suite :) I've restarted the one timeout'd build to see if it passes :) |
👋 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 |
bump @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. |
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
ormove_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 thedecrement_all
call here: https://github.com/mgbatchelor/acts_as_list/blob/1db1772efa716ebda76f366a5b9e86fe02cb1e9c/lib/acts_as_list/active_record/acts/list.rb#L335Please advise. Thanks