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

add params to collection filter #2005

Merged
merged 6 commits into from
Jul 12, 2021

Conversation

aka47
Copy link
Contributor

@aka47 aka47 commented Jun 21, 2021

To filter a collection you can define COLLECTION_FILTER and use them like "filter:".

With this PR you can also params to those filter and use them like "filter:param". The COLLECTION_FILTER will be called with the params a second argument.

This ideas was already mentioned in the initial PR for COLLECTION_FILTERS, but not implemented. (#947)

To filter a collection you can define COLLECTION_FILTER and use them like "filter:".

With this PR you can also params to those filter and use them like "filter:param". The COLLECTION_FILTER will be called with the params a second argument.

This ideas was already mentioned in the initial PR for COLLECTION_FILTERS, but not implemented. (github.com/thoughtbot/pull/947)
@pablobm
Copy link
Collaborator

pablobm commented Jun 24, 2021

Thank you for your PR @aka47 - I think it's good. Generally I'm inclined not to enhance the search, as I think it should be completely redesigned, but your change is a small increment of what we already have, so I think it's fine.

Would you be able to make the following two changes before I merge this?

  1. This PR creates a situation where searching for anything with a colon in the middle (eg: "order:12") will stop working if it's intended as a string search. Before it only broke things if the colon was at the end, which I think was a much less common case. Would you be able to tweak your code to avoid this? I think you could change it so that filter? not only does the regexp match, but also sees if the filter name exists in COLLECTION_FILTERS. Does that make sense?
  2. Fix that Hound warning (or any other that may come later).

@aka47
Copy link
Contributor Author

aka47 commented Jun 25, 2021

@pablobm thanks for your feedback - sure filters and terms should not get confused. It needed a bit more changes then I thought, as the Query class does not have access to the Search Class and the valid_filters from the dashboard.
If you need anything more - please tell me.

@mmrobins
Copy link

mmrobins commented Jul 9, 2021

FWIW this is great and I would love to see it released. I was running a hacky version of this in my local code to do the same. I get that search needs an overhaul, but until then something small like this would be a great incremental improvement in the meantime.

@pablobm pablobm merged commit 1ddb665 into thoughtbot:main Jul 12, 2021
@pablobm
Copy link
Collaborator

pablobm commented Jul 12, 2021

Thank you @aka47!

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.

3 participants