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

No update list for collection classes #260

Merged

Conversation

IlkhamGaysin
Copy link
Contributor

@IlkhamGaysin IlkhamGaysin commented Mar 1, 2017

I see that we can block auto-updating positions in a list only for class to whom call the method acts_as_list_no_update
But what if I need to disable for parent or child classes inside a block for example:

class TodoList < ActiveRecord::Base
  has_many :todo_items, -> { order(position: :asc) }
end

class TodoItem < ActiveRecord::Base
  belongs_to :todo_list
  has_many :todo_attachments, -> { order(position: :asc) }

  acts_as_list scope: :todo_list
end

class TodoAttachment < ActiveRecord::Base
  belongs_to :todo_list
  acts_as_list scope: :todo_item
end

TodoItem.acts_as_list_no_update([TodoAttachment]) do
  TodoItem.find(10).update(position: 2)
  TodoAttachment.find(10).update(position: 1)
  TodoAttachment.find(11).update(position: 2)
end

These changes under allow to pass additional classes as an argument and don't update positions for records of the passed classes inside of the block where there might be interactions with them.

Thanks!

@IlkhamGaysin IlkhamGaysin force-pushed the no-update-for-collection-classes branch from 456277b to fe848c1 Compare March 1, 2017 13:13
IlkhamGaysin added 2 commits March 1, 2017 17:07
* master:
  Refactor column definer module (brendon#258)
  Fix deprecation introduced in ActiveRecord 5.1.
  Add rails 5.1.0.beta1 and ruby 2.4 to test matrix.
@IlkhamGaysin
Copy link
Contributor Author

Please review guys @swanandp @brendon 🍻

Copy link
Owner

@brendon brendon left a comment

Choose a reason for hiding this comment

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

Hi @IlkhamGaysin :) Thanks for this useful enhancement. I just had a few tweaks for you to make. Let me know if you have any questions :)

README.md Outdated
the default value from the database. It is your responsibility to correctly manage `positions` values.
the default value from the database. It is your responsibility to correctly manage `positions` values.

Also you can pass an argument as array of extracted calsses to disable from database updates. It might be any ActiveRecord class that is able to acts as list.
Copy link
Owner

Choose a reason for hiding this comment

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

Should read:

You can also pass an array of classes as an argument to disable database updates on just those classes. It can be any ActiveRecord class that has acts_as_list enabled.

end

TodoItem.acts_as_list_no_update([TodoAttachment]) do
TodoItem.find(10).update(position: 2)
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps your example could show an update to TodoList also, with a comment next to it stating that callbacks will be called on that object.

# TodoList.first.update(position: 2)
# end
#
# Also you can pass an argument as array of extracted calsses
Copy link
Owner

Choose a reason for hiding this comment

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

Re-use the wording from the updated README.

# end
#
# TodoItem.acts_as_list_no_update([TodoAttachment]) do
# TodoItem.find(10).update(position: 2)
Copy link
Owner

Choose a reason for hiding this comment

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

Update the example here also as per the README.

# TodoAttachment.find(11).update(position: 2)
# end

def acts_as_list_no_update(extra_klasses = [], &block)
Copy link
Owner

Choose a reason for hiding this comment

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

Since extra_klasses isn't a reserved word I think it should be extra_classes. Would you agree @swanandp?

assert_equal @sub_mixin_1.pos, 2
assert_equal @sub_mixin_2.pos, 2
end

Copy link
Owner

Choose a reason for hiding this comment

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

I think you need a test here that tests that callbacks are still performed on a non-specified acts_as_list class when called within the block (as per the README example update.)

@IlkhamGaysin IlkhamGaysin force-pushed the no-update-for-collection-classes branch from e6a78f3 to f6cd28a Compare March 2, 2017 10:40
@IlkhamGaysin
Copy link
Contributor Author

@brendon I've updated the PR please review 🍺

@brendon brendon merged commit 1f6175d into brendon:master Mar 3, 2017
@brendon
Copy link
Owner

brendon commented Mar 3, 2017

Thanks @IlkhamGaysin, great work! I've merged that for you now :)

@IlkhamGaysin
Copy link
Contributor Author

@brendon awyeah, thanks 🍻

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.

2 participants