bulk update through act_as_list_no_update #171

randoum · 2015-09-29T07:37:42Z

To newcomers : The conversation for this PR continues here #244

Allow to perform bulk update by disabling act_as_list callbacks

ActiveRecord::Base.act_as_list_no_update do
  TodoItem.find(5).update position: 17
  TodoItem.find(8).update position: 11
end

inspired from http://api.rubyonrails.org/classes/ActiveRecord/NoTouching/ClassMethods.html

houndci-bot · 2015-09-29T07:37:48Z

lib/acts_as_list/active_record/acts/list/disabled.rb

+        end
+
+        def act_as_list_run_callbacks? # :nodoc:
+          ! Disabled.applied_to?(self.class)


Do not leave space between ! and its argument.

randoum · 2015-10-03T05:35:17Z

@swanandp the CI error comes from bundle

Please install the sqlite3 adapter: gem install activerecord-sqlite3-adapter (sqlite3 is not part of the bundle. Add it to Gemfile.)

Shall I do something about that?

swanandp · 2015-10-04T06:49:06Z

I think that calls for a separate PR. Go for it!

On Sat, Oct 3, 2015 at 11:05 AM, randoum notifications@github.com wrote:

@swanandp https://github.com/swanandp the CI error comes from bundle

Please install the sqlite3 adapter: gem install
activerecord-sqlite3-adapter (sqlite3 is not part of the bundle. Add it
to Gemfile.)

Shall I do something about that?

—
Reply to this email directly or view it on GitHub
#171 (comment)
.

randoum · 2015-10-04T07:39:42Z

Ok, noted. Let's focus on this one first if you don't mind. What's your feedback?

randoum · 2015-10-07T14:22:54Z

@swanandp any feedback? Thanks.

swanandp · 2015-10-07T15:13:38Z

Sorry, got held up in other things, will come back to this shortly.

randoum · 2015-10-07T15:22:05Z

Ok sure.

randoum · 2015-11-15T09:57:23Z

@swanandp any feedback? Thanks.

fabn · 2015-12-16T16:56:57Z

@randoum tests have been fixed in master. Could you please rebase and fix hound issues as well?

Thanks.

houndci-bot · 2015-12-27T06:42:09Z

test/shared_array_scope_list.rb


      ArrayScopeListMixin.where(id: 2).first.remove_from_list
      ArrayScopeListMixin.where(id: 2).first.destroy

-      assert_equal [1, 3, 4], ArrayScopeListMixin.where(parent_id: 5, parent_type: 'ParentClass').order('pos').map(&:id)
+      assert_equal [1, 3, 4], ArrayScopeListMixin.where(parent_id: 5, parent_type: "ParentClass").order("pos").map(&:id)


Line is too long. [120/80]

nickpoorman · 2016-12-05T20:42:13Z

I could really use this right now... 😞 I'm updating thousands of items in a list and it's wreaking havoc on my database.

brendon · 2016-12-05T20:47:46Z

Hi @nickpoorman, sorry to hear that. As a workaround you could use update_columns to circumvent all the callback logic. Or if you feel like chipping in on this PR please do :) It's at a bit of an impasse at the moment.

@wikyd, now that @swanandp has had his say, would you like to push this through along his recommendations?

wikyd · 2016-12-05T22:39:00Z

I would like to help, but the proposed changes does not work with our use case. We have a Javascript front-end that allows users to re-order and remove items from the list. Then, we take this set of changes and update our items in a batch use Rails params -> model magic. Turning off callbacks for a specific model does not help in that case.

brendon · 2016-12-05T23:01:04Z

@wikyd, I've always found this to be the most troubling part of using acts_as_list. Unless you use the relative move methods or set one position at a time, the callbacks can get in the way. I often use this relation extension:

module Sortable
  def sort(array_of_ids)
    array_of_ids.each.with_index(1) do |id, index|
      where(:id => id).update_all(:position => index)
    end
  end
end

As a way to mass-assign positions passed in as a list of id's. It can be limited if the list of id's isn't all the id's in the scope though.

nickpoorman · 2016-12-05T23:02:44Z

@wikyd - I'm actually doing the same thing. We have a bulk upsert endpoint that does a create/destroy/update all in one. For this route, I had to disable paper trail also, as it was creating massive amounts of rows in our versions table. They seemed to have solved this "disable" issue this way: https://github.com/airblade/paper_trail/blob/05dd9fb40848c2c8e7079eac7fccffb54720db9e/lib/paper_trail/record_trail.rb#L425

ie.

@widget.paper_trail.without_versioning do
  @widget.update_attributes :name => 'Ford'
end

...and it works quite well.

brendon · 2016-12-05T23:06:22Z

@nickpoorman, they're using some kind of class variable to track the disabled state? I'd assume that wasn't thread safe.

nickpoorman · 2016-12-05T23:09:06Z

@brendon - I believe it is thread safe: paper-trail-gem/paper_trail#328

nickpoorman · 2016-12-05T23:36:40Z

@brendon - I see what they did. They used https://github.com/steveklabnik/request_store to store the state. https://github.com/airblade/paper_trail/blob/8b9ce72f2c449becb3d75b094b9b07fb64b6e908/lib/paper_trail.rb#L62

brendon · 2016-12-06T01:10:36Z

@nickpoorman, ah yes, I use that in my applications too. It's handy, but may be brittle to add as a dependency in our case? Will there be any downsides say in the console?

brendon · 2016-12-06T01:11:00Z

I noticed ancestry has a similar method though I've not investigated how they do theirs.

nickpoorman · 2016-12-06T13:32:31Z

@brendon, as far as I can tell request_store is a wrapper for Thread.current with some middleware to clean up after requests so I see no reason why it wouldn't work in the console.

brendon · 2016-12-06T20:24:58Z

Yes I suppose if the store variable is set and unset cleanly after the block executes then that would be fine.

Here's how ancestry is doing it: https://github.com/stefankroes/ancestry/blob/master/lib/ancestry/instance_methods.rb#L296

It's an instance method but is quite spread out in terms of its application in the code.

@swanandp, do you have any more thoughts given the latest comments here?

swanandp · 2016-12-07T05:41:39Z

@brendon Definitely need to do the blocking on object level. Let's avoid class or thread level variables unless we have to, and we don't have to here.

brendon · 2016-12-07T07:59:19Z

Thanks @swanandp. @wikyd, @randoum given this direction, would one of you like to refactor this so that it uses an instance variable and also if possible includes just checks for this variable on the callback declarations rather than inline in the code?

randoum · 2016-12-07T08:10:46Z

Personally I have zero availability.
Just a note about request store. It is used in several gems for quite some times and seems to be maturely thread safe.

brendon · 2016-12-07T08:15:39Z

Thanks @randoum, that's all good. I have no doubts about the quality of RequestStore but as @swanandp says in this case, it's not necessary to use a thread level variable so I'd rather not add an unnecessary dependency. Thanks for your input on this PR to date. :)

swanandp · 2016-12-07T10:33:55Z

That's exactly right. I've used RequestStore in the past quite effectively, but I don't think it's needed here.

…

On Wed, 7 Dec 2016 at 1:45 PM, Brendon Muir ***@***.***> wrote: Thanks @randoum <https://github.com/randoum>, that's all good. I have no doubts about the quality of RequestStore but as @swanandp <https://github.com/swanandp> says in this case, it's not necessary to use a thread level variable so I'd rather not add an unnecessary dependency. Thanks for your input on this PR to date. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#171 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFjGLOthRgDpAVLGjX2sOPC5PYReqJ0ks5rFmsrgaJpZM4GFe7p> .

# The first commit's message is: fix setting position when previous position was nil (#230) * set position properly if starting position is nil * update tests to work with rails 3.2 Closes #109 # This is the 2nd commit message: Show items with same position in higher and lower items (#231) * Show items with same position in higher and lower items * Remove spacing, use update_column instead of update_columns # This is the 3rd commit message: Version 0.8.2 # This is the 4th commit message: Updated Changelog 0.8.2 # This is the 5th commit message: Extract modules (#229) * Convert .acts_as_list to use keyword arguments * Refactoring .acts_as_list: remove configuration variable * Refactoring: use ActiveSupport::Inflector.foreign_key instead of ad-hoc * Refactoring: extract #idify * Refactoring: extract ScopeDefiner module * Refactoring: extract TopOfListDefiner module * Refactoring: extract ColumnDefiner module * Refactoring: extract AddNewAtDefiner module * Refactoring: extract UpdatePositonDefiner module * Refactoring: remove redundant #class_eval call * Refactoring: rename *Definer modules to *MethodDefiner * Refactoring: extract CallbackDefiner module * Refactoring: remove keyword arguments to support Ruby 1.9 * Add a missing dot in comments * Refactoring: merge UpdatePositonMethodDefiner into ColumnMethodDefiner * Refactoring: extract AuxMethodDefiner module # This is the 6th commit message: Add travis config for testing against multiple databases (#236) * Add travis-CI config for tests with postgres / mysql / sqlite # This is the 7th commit message: Use timecop to freeze time for testing # This is the 8th commit message: Compare updated_at with TimeWithZone # This is the 9th commit message: Freeze time at Time.current # This is the 10th commit message: Cache the current time # This is the 11th commit message: Fix non regular sequence movement (#237) # This is the 12th commit message: Be explicit about ordering when mapping :pos in tests (#241) # This is the 13th commit message: Improve load method (#240) * Refactor loading sequence * Fix failing tests # This is the 14th commit message: Fixed tests to prevent warning: too many arguments for format string (#242) # This is the 15th commit message: README update: Adding `acts_as_list` To An Existing Model Closes #210 # This is the 16th commit message: Update README.md (#243) add .all to fix undefined each method in scope option # This is the 17th commit message: bulk update through act_as_list_no_update

This reverts commit f426b10.

This reverts commit 771f8df.

randoum · 2016-12-18T07:49:22Z

I am not sure we can do without Thread.current.

If you can do @widget.paper_trail.without_versioning {@widget.update_attributes :name => 'Ford'} it's because the without_versioning statement concerns only @widget, which is a single record.

In our case an act_as_list_no_update block would apply to the behavior of multiple records or a whole model.

In terms of use case, you would never write:

TodoItem.find(5).acts_as_list_no_callback do
  update(:position => 5)
end

Because it's plain useless.

Instead, you would be using acts_as_list_no_callback for mass updates, mass ordering, mass import or such. Hence the statements would be concerning multiple records.

How can you write a block statement that concern multiple record? There's only one way, by using a block at the class level :

TodoItem.acts_as_list_no_callback do
  mass_import_method
end

2 choices: class instance variable or Thread.current variable. The second one is way safer.

So I understand you guys fears toward Thread.current, but I'd like to remind you that my original PR was based on http://api.rubyonrails.org/classes/ActiveRecord/NoTouching/ClassMethods.html which is part or rails core and using Thread.current.

If rails core team is using it, we can. We are using Thread.current in a block context, no around thread synchronization or between different webserver sessions, it's safe to use in our case. The only important thing is to clear the thread variable in an ensureblock.

Some reading : http://stackoverflow.com/questions/7896298/safety-of-thread-current-usage-in-rails

brendon · 2016-12-18T19:43:28Z

Hi @randoum, you make a good point :) @swanandp, can you chime in?

@randoum, your latest rebase has reverted a bunch of changes on master (Appraisal config, CHANGELOG etc...)

randoum · 2016-12-18T21:47:51Z

Yeah I messed up big time with git. I have no idea what I did. Finally I deleted my fork and I already started from a fresh one.

randoum · 2016-12-19T17:09:17Z

Kindly continue there #244

randoum · 2017-01-16T09:00:52Z

@wikyd @nickpoorman @StoneFrog if you guys don't mind to participate to #244. Thanks

houndci-bot reviewed Sep 29, 2015
View reviewed changes

fabn added the enhancement label Dec 16, 2015

fabn added the awaiting-feedback label Dec 16, 2015

houndci-bot reviewed Dec 27, 2015
View reviewed changes

StoneFrog and others added 6 commits December 17, 2016 11:14

bulk update through act_as_list_no_update

f426b10

merging conflicts

3d87aea

Revert "bulk update through act_as_list_no_update"

bac5e0c

This reverts commit f426b10.

Revert "# This is a combination of 17 commits."

356cab9

This reverts commit 771f8df.

git.............

2fdbc38

randoum closed this Dec 19, 2016

randoum mentioned this pull request Jan 16, 2017

acts_as_list_no_update #244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bulk update through act_as_list_no_update #171

bulk update through act_as_list_no_update #171

randoum commented Sep 29, 2015 •

edited

Loading

houndci-bot Sep 29, 2015

randoum commented Oct 3, 2015

swanandp commented Oct 4, 2015

randoum commented Oct 4, 2015

randoum commented Oct 7, 2015

swanandp commented Oct 7, 2015

randoum commented Oct 7, 2015

randoum commented Nov 15, 2015

fabn commented Dec 16, 2015

houndci-bot Dec 27, 2015

nickpoorman commented Dec 5, 2016 •

edited

Loading

brendon commented Dec 5, 2016

wikyd commented Dec 5, 2016

brendon commented Dec 5, 2016

nickpoorman commented Dec 5, 2016 •

edited

Loading

brendon commented Dec 5, 2016

nickpoorman commented Dec 5, 2016

nickpoorman commented Dec 5, 2016

brendon commented Dec 6, 2016

brendon commented Dec 6, 2016

nickpoorman commented Dec 6, 2016 •

edited

Loading

brendon commented Dec 6, 2016

swanandp commented Dec 7, 2016

brendon commented Dec 7, 2016

randoum commented Dec 7, 2016

brendon commented Dec 7, 2016

swanandp commented Dec 7, 2016 via email

randoum commented Dec 18, 2016

brendon commented Dec 18, 2016

randoum commented Dec 18, 2016

randoum commented Dec 19, 2016

randoum commented Jan 16, 2017

bulk update through act_as_list_no_update #171

bulk update through act_as_list_no_update #171

Conversation

randoum commented Sep 29, 2015 • edited Loading

To newcomers : The conversation for this PR continues here #244

houndci-bot Sep 29, 2015

Choose a reason for hiding this comment

randoum commented Oct 3, 2015

swanandp commented Oct 4, 2015

randoum commented Oct 4, 2015

randoum commented Oct 7, 2015

swanandp commented Oct 7, 2015

randoum commented Oct 7, 2015

randoum commented Nov 15, 2015

fabn commented Dec 16, 2015

houndci-bot Dec 27, 2015

Choose a reason for hiding this comment

nickpoorman commented Dec 5, 2016 • edited Loading

brendon commented Dec 5, 2016

wikyd commented Dec 5, 2016

brendon commented Dec 5, 2016

nickpoorman commented Dec 5, 2016 • edited Loading

brendon commented Dec 5, 2016

nickpoorman commented Dec 5, 2016

nickpoorman commented Dec 5, 2016

brendon commented Dec 6, 2016

brendon commented Dec 6, 2016

nickpoorman commented Dec 6, 2016 • edited Loading

brendon commented Dec 6, 2016

swanandp commented Dec 7, 2016

brendon commented Dec 7, 2016

randoum commented Dec 7, 2016

brendon commented Dec 7, 2016

swanandp commented Dec 7, 2016 via email

randoum commented Dec 18, 2016

brendon commented Dec 18, 2016

randoum commented Dec 18, 2016

randoum commented Dec 19, 2016

randoum commented Jan 16, 2017

randoum commented Sep 29, 2015 •

edited

Loading

nickpoorman commented Dec 5, 2016 •

edited

Loading

nickpoorman commented Dec 5, 2016 •

edited

Loading

nickpoorman commented Dec 6, 2016 •

edited

Loading