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

Collection filters #947

Merged
merged 1 commit into from
Aug 16, 2019
Merged

Collection filters #947

merged 1 commit into from
Aug 16, 2019

Conversation

koppen
Copy link
Contributor

@koppen koppen commented Jul 22, 2017

This adds the option to define a set of filters that can be triggered in order to reduce the number of resources listed on the index page of a dashboard. Currently the only way to trigger the filters is via the search field, using a syntax similar to the one used here on GitHub.

Why

Often we need to expose more detailed/involved queries than those currently capable via simple searches. Previously we'd have to add special actions that query the database correctly and customize the views with links to trigger those actions.

Example

For example, if we want to be able to find "inactive" customers (for some definition of inactive), we can add

COLLECTION_FILTERS = {
  inactive: ->(resources) { resources.where("login_at < ?", 1.week.ago) }
}

to a dashboard. This way, we can query the resources of that dashboard by typing bob inactive: in the search field to find users named "bob" who hasn't logged in the last week.

Credits

This is basically a reimplementation of @nando's work from #276, I merely took some of his thoughts and ideas and repackaged them in a way that makes sense for Administrate anno 2017.

Also in an effort to keep an otherwise huge pull request as small as possible, I haven't gone all the way that he did in the original pull request, so we're missing automatic filtering setup and exposing available filters in the UI.

Nor are the filters implemented here as flexible or powerful as those proposed in #276.

Further details

If you already had the inactive scope you could define the filter like the following to take advantage of existing ActiveRecord scopes (and other class methods on the resource class).

COLLECTION_FILTERS = {
  inactive: ->(resources) { resources.inactive }
}

While the chosen hash-based syntax is a bit more verbose than simply exposing already defined scopes like so:

# app/dashboards/customer_dashboard.rb
COLLECTION_FILTERS = [:inactive]

it allows us to define filters for use in Administrate without having to clutter the resource classes with scopes.

Future proofing

The chosen syntax should allow us to add the above mentioned simpler syntax in a backwards compatible way at some point down the line if we feel the need. For example it could end up looking like:

COLLECTION_FILTERS = {
  vip: :vip, # This could call the method `vip` on resources
  inactive: ->(resources) { resources.where("login_at < ?", 1.week.ago) }
}

but that's a step I am not ready to take just yet.

It also opens up for a way to have filters that take arguments, ie we could define:

COLLECTION_FILTERS = {
  before: ->(resources, args) {
            resources.where("created_at < ?", args)
          }
}

and query it as before:2016-01-12 which, again, is a step I am not ready to take here and now.

This was referenced Jul 22, 2017
@nickcharlton
Copy link
Member

nickcharlton commented Jul 30, 2017

This is going to be a great feature, I like the direction it’s going in and I appreciate the work you’ve put in to pick up a previous PR.

I’ve been starting to think about what the 1.0 version of administrate should be and I think I’d like to hold off this until then. Would you be okay with keeping this alive until then? I imagine it might be a few rebases, but I’m expecting the features we add to slow down and it only be bug fixes.

I’m going to tag this with “post-1.0”, so it’s easy to pick up soon.

@koppen
Copy link
Contributor Author

koppen commented Jul 30, 2017

I'll definitely keep it up to date, not least because I very much want this feature in Administrate proper, so I can get back to deploying the main line.

If I have to wait, so be it, but I'd obviously prefer seeing it merged sooner than later - you know what they say about long-running feature branches ;)

I might want to add a bit more features on top of this (somehow exposing the filters in the UI for starters), although I'd prefer keeping those in separate branches. I guess I can hold off on creating new PRs for those and still keep this one lean and mergeable.

@nickcharlton
Copy link
Member

You could open more PRs, but based upon this one. As long as we keep this one updated it won't be too bad.

I'm hoping we'll not be too long until we get this merged, regardless!

@nickcharlton nickcharlton mentioned this pull request Dec 4, 2017
@coletivo
Copy link

coletivo commented Jan 9, 2018

What is the status on this?

@koppen
Copy link
Contributor Author

koppen commented Jan 11, 2018

Rebased on the latest master, still applies cleanly and should be usable from our fork. Still awaiting that 1.0 release 🤞

@macouella
Copy link

Please release...

@svenaas
Copy link

svenaas commented Feb 11, 2018

We need a way to do this now too. Here's another plea for a release!

@koppen
Copy link
Contributor Author

koppen commented Feb 12, 2018

Rebased on a fresh master, fixing a few conflicts, and I have added backwards compatibility with Ruby versions < 2.4 (Administrate::Search::Query used String#match? which wasn't added until Ruby 2.4).

The current build failures are all because of the gemfiles are locked to a version of nokogiri with known vulnerabilities. Fixing this seems out of scope of this pull request:

ruby-advisory-db: 298 advisories
Name: nokogiri
Version: 1.8.1
Advisory: CVE-2017-15412
Criticality: Unknown
URL: https://github.com/sparklemotion/nokogiri/issues/1714
Title: Nokogiri gem, via libxml, is affected by DoS vulnerabilities
Solution: upgrade to >= 1.8.2

Vulnerabilities found!
Exited with code 1 

@jproctor
Copy link

Version 0.9.0 updates Nokogiri. Can we try rebuilding?

@koppen
Copy link
Contributor Author

koppen commented Feb 20, 2018

Rebased on master, awaiting CI judgement 🤞

@nickcharlton
Copy link
Member

This is looking great, and thanks for keeping it up to date!

I'm still planning on this for post-1.0, but we're moving close to that now. FWIW, my plan is to not merge too much in between now and then and this will go out in 1.1.0.

@nando
Copy link
Contributor

nando commented Jun 28, 2018

Thanks a lot Jacob!!

I haven't seen your work until now while I was looking for my old PR :(

Nice to had inspired your branch!! :D

Cheers!!
<3<3<3

@koppen
Copy link
Contributor Author

koppen commented Jun 29, 2018

@nando Now we just need to make sure this branch doesn't fall by the wayside as well - here's hoping for a soonish merge 🤞

@koppen
Copy link
Contributor Author

koppen commented Jun 29, 2018

Rebased on the most recent master, still squeaky clean, ready for an easy merge :hint hint: 😇

@hadees
Copy link

hadees commented Sep 28, 2018

@koppen nice job, hopefully @Graysonwright, @nickcharlton, or @tysongach can merge it in soon. Otherwise I'd love to see a fork run by you. I'd love to keep this going and add my own changes eventually.

@koppen
Copy link
Contributor Author

koppen commented Sep 28, 2018

And just like that, this branch is now up to date with the master branch and both CI and Hound are showing green checkmarks. And there was much rejoicing!

@hadees Thanks :) Not sure about the whole fork-business, but the branch at https://github.com/substancelab/administrate/tree/276-collection_filters should be perfectly functional. I can't promise to keep it up to date, but you should be able to work off it, though, I guess.

@@ -149,5 +156,26 @@ class User < ActiveRecord::Base; end
search.run
end
end

it "searches using a filter" do
begin

Choose a reason for hiding this comment

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

Style/RedundantBegin: Redundant begin block detected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, however if we want to maintain compatibility with rubies before 2.5, this begin block needs to be here.

@c4lliope c4lliope removed the post-1.0 label May 20, 2019
@Tashows
Copy link

Tashows commented Jun 20, 2019

Merge this pretty please? :D

@ACPK
Copy link

ACPK commented Aug 3, 2019

@nickcharlton - How long until v1 as I'm really looking forward to this feature in master.

If we add:

    COLLECTION_FILTERS = {
      inactive: ->(resources) { resources.where("login_at < ?", 1.week.ago) }
    }

to a dashboard, we can query the resources of that dashboard with

    bob inactive:

to find users named "bob" who hasn't logged in the last week.

If you already had the `inactive` scope you could define the filter like so to
take advantage of existing ActiveRecord scopes (and other class methods on the
resource class).

    COLLECTION_FILTERS = {
      inactive: ->(resources) { resources.inactive }
    }

While the chosen hash-based syntax is a bit more verbose than simply exposing
already defined scopes like so:

    # app/dashboards/customer_dashboard.rb
    COLLECTION_FILTERS = [:inactive]

it allows us to define filters for use in Administrate without having to clutter
the resource classes with scopes.

It still allows us to add the simpler syntax in a backwards compatible way at
some point down the line if we feel the need. For example it could end up
looking like:

    COLLECTION_FILTERS = {
      vip: :vip, # This could call the method `vip` on resources
      inactive: ->(resources) { resources.where("login_at < ?", 1.week.ago) }
    }

* Allow search_spec to be run on its own,
* Introduce the concept of a search query
  - adding collection scopes/filters means we need to add more involved
    search query parsing; this gives us a place for that,
@nickcharlton nickcharlton merged commit 56ede63 into thoughtbot:master Aug 16, 2019
@nickcharlton
Copy link
Member

Thanks for everyone's patience on this one! I've merged now and we'll cut a release shortly.

@ACPK
Copy link

ACPK commented Aug 21, 2019

@nickcharlton - What are your thoughts on having filters show up as a dropdown? This would be very similar to how active_admin works.

@nickcharlton
Copy link
Member

@ACPK, could you open a new issue with that? Perhaps with an example? And then we can take it from there.

@thoughtbot thoughtbot locked as resolved and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.