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

Allow a blacklist listing filters #1609

Merged
merged 1 commit into from
Sep 2, 2012

Conversation

tracedwax
Copy link

We want to preserve active generation of filters, while
opting out of certain filters, for example associations.

Sometimes associations can be prohibitively large, for
example when there are 100,000 users and several models
which belong_to :user. That filter would have too many
items for the list to display correctly, and was causing
serious performance issues.

Example:

class User < ActiveRecord::Base
  has_many :posts
end

class Post < ActiveRecord::Base
  belongs_to :user 
end

ActiveAdmin.register Post do
  unfilter :user
end

@travisbot
Copy link

This pull request passes (merged 7b54309f into 1a20a16).

@jpmckinney
Copy link
Contributor

I don't know about the name unfilter. I know you're trying to mirror the method filter. Maybe we can just call it remove_filter? (I would also rename delete_filter to remove_filter).

@stefanpenner
Copy link

I think the naming is fine, unless consensus around some other name is reached, we should just leave it.
#preventbikeshedding

@jpmckinney
Copy link
Contributor

It's not bikeshedding. unfilter doesn't make sense - it's not even a word. remove_filter is clearer. And to follow the conventions and style of the rest of the code, new methods should be called remove_* not delete_*.

@alexandru-calinoiu
Copy link

I totally agree with @jpmckinney remove_filter sounds like a very reasonable name, unfilter is not a word (I know, who cares about what I think but I will really like this feature implemented)

@stefanpenner
Copy link

looks like consensus has been reached, awesome. remove_filter it is. @tracedwax got a chance to update the pr?

We want to preserve active generation of filters, while
opting out of certain filters, for example associations.

Sometimes associations can be prohibitively large, for
example when there are 100,000 users and several models
which belong_to :user.  That filter would have too many 
items for the list to display correctly, and was causing 
serious performance issues.
@tracedwax
Copy link
Author

Done - changed the naming to remove_filter as discussed.

@travisbot
Copy link

This pull request passes (merged d4908c0 into a8cc231).

jpmckinney pushed a commit that referenced this pull request Sep 2, 2012
Allow a blacklist listing filters
@jpmckinney jpmckinney merged commit 7376cac into activeadmin:master Sep 2, 2012
@jpmckinney
Copy link
Contributor

Merged, 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.

5 participants