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

Limit children for deletion notice #2491

Merged

Conversation

jibidus
Copy link
Contributor

@jibidus jibidus commented Nov 27, 2015

Limit number of future orphans displayed during deletion confirmation in order to avoid performance issue.

@jibidus jibidus force-pushed the limit_children_for_deletion_notice branch from 5a93245 to d79d5f7 Compare November 27, 2015 13:44
@jibidus jibidus force-pushed the limit_children_for_deletion_notice branch from c263202 to 28d1770 Compare November 27, 2015 16:48
@jibidus jibidus force-pushed the limit_children_for_deletion_notice branch from 28d1770 to a37fcae Compare November 27, 2015 19:38
yield(association, child)
end
children = object.send(association.name)
yield(association, Array.new(children))

Choose a reason for hiding this comment

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

The problem with this is that it still fetches all children from the database.
I implemented a quick and dirty fix for this here: https://github.com/alexandergitter/rails_admin/commit/6a172da72c98bfd5ca7285b356478a16e7a7a064

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that children are fetched from database here.
children variable contains a CollectionProxy returned to _delete_notice.html.haml where count and 'first(limit) is called. First n children are fetched here.
Am I wrong ?

Choose a reason for hiding this comment

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

Mhh that's interesting, I didn't notice that.
In my tests Active Record did an unlimited SELECT though, not sure if there's something else that would trigger that... maybe it's because children gets wrapped in a new array? Would that eagerly fetch all children?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, wrapping children in a a new array may fetch database values.

@mshibuya mshibuya merged commit a49f1eb into railsadminteam:master Sep 3, 2016
@mshibuya
Copy link
Member

mshibuya commented Sep 3, 2016

Seems okay, thanks!

@jibidus jibidus deleted the limit_children_for_deletion_notice branch September 3, 2016 10:30
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.

None yet

3 participants