-
Notifications
You must be signed in to change notification settings - Fork 0
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 support for exact search #35
Conversation
@@ -41,8 +41,8 @@ default_job: &default_job | |||
steps: | |||
- shared_steps | |||
# Run the tests against multiple versions of Rails | |||
- run: bundle exec appraisal install | |||
- run: bundle exec appraisal rspec | |||
# - run: bundle exec appraisal install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without any dependency changes, appraisal
does not successfully install locally or in the CI env. Resolving the dependency puzzle is just too much of a headache to take on, so I'm removing it. It's used to test a codebase with multiple versions of its dependencies.
A bit ironic that a tool to test dependencies ended up being the broken dependency here without any changes to its version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this in this repo instead of in Web!
Would you mind opening a github issue in the administrate repo?
It'd be good to know if they would accept an upstream PR or something adding this feature.
I was talking with Nic recently about updating administrate so that's on my mind.
Opened up the issue here: thoughtbot#2745 Curious to see what they make of it! Doesn't seem like it's that niche of a thing to ask for |
lib/administrate/search.rb
Outdated
if attribute_types[attr].search_exact? | ||
if attribute_types[attr].search_lower? | ||
"LOWER(#{table_name}.#{column_name}) = ?" | ||
else | ||
"#{table_name}.#{column_name} = ?" | ||
end | ||
else | ||
if attribute_types[attr].search_lower? | ||
"LOWER(CAST(#{table_name}.#{column_name} AS CHAR(256))) LIKE ?" | ||
else | ||
"CAST(#{table_name}.#{column_name} AS CHAR(256)) LIKE ?" | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the logic is preserved (in case the options are not set and using default); however... it's so bloody hard to read.
might be nice to have the a function and make use of the early return:
def map_search_attributes(table_name, column_name, search_exact, search_lower)
if search_exact && search_lower
return ...
end
if search_exact && !search_lower
return ..
end
...
end
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I implemented it in a more legible way (and with one extra tidbit to turn off char casting) in the PR against the upstream repo https://github.com/thoughtbot/administrate/pull/2752/files
I'll pull that approach back in here
We want to support search functionality in the emails admin dashboard in web, but the underlying administrate search functionality always uses substring matching and doesn't offer the option to use an equality comparison. We'd like to use this in order to have much more efficient and accurate queries, since those search terms are going to be pre-populated in the app through clickable links in the app wherever we use the
Email
field.This change adds that functionality.
Blocks this PR -> https://github.com/rinsed-org/web/pull/14327