-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Order tags based on best match to search query #9879
Conversation
app/services/search_service.rb
Outdated
CASE | ||
WHEN name LIKE "' + query + '" THEN 1 | ||
WHEN name LIKE "'+ query+'%" THEN 2 | ||
WHEN name LIKE "%'+ query +'" THEN 4 |
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.
Surrounding space missing for operator +
.
app/services/search_service.rb
Outdated
CASE | ||
WHEN name LIKE "' + query + '" THEN 1 | ||
WHEN name LIKE "'+ query+'%" THEN 2 | ||
WHEN name LIKE "%'+ query +'" THEN 4 |
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.
Surrounding space missing for operator +
.
app/services/search_service.rb
Outdated
.order(' | ||
CASE | ||
WHEN name LIKE "' + query + '" THEN 1 | ||
WHEN name LIKE "'+ query+'%" THEN 2 |
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.
Surrounding space missing for operator +
.
app/services/search_service.rb
Outdated
.order(' | ||
CASE | ||
WHEN name LIKE "' + query + '" THEN 1 | ||
WHEN name LIKE "'+ query+'%" THEN 2 |
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.
Surrounding space missing for operator +
.
Codecov Report
@@ Coverage Diff @@
## main #9879 +/- ##
===========================================
- Coverage 82.06% 26.05% -56.02%
===========================================
Files 98 98
Lines 5945 7629 +1684
===========================================
- Hits 4879 1988 -2891
- Misses 1066 5641 +4575
|
app/services/search_service.rb
Outdated
@@ -84,6 +84,13 @@ def search_tags(query, limit = 10) | |||
.includes(:node) | |||
.references(:node) | |||
.where('node.status = 1') | |||
.order(' | |||
CASE |
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.
Hey!
Just a quick suggestion can we store the case
string in a variable and then just use that inside order?
For example: tags_order = "CASE WHEN name like #{query} THEN 1 WHEN name LIKE #{query}% THEN 2 WHEN name LIKE %{query} THEN 4 ELSE 3"
and then just use order(tags_order)
.
Also, should we include one more case? WHEN name like %#{query}%
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.
@namangupta01, the first suggestion seems to be a good idea to store the string as a variable and using it. I'm adding a commit with the changes.
As per WHEN name like %#{query}%
case is concerned, I think it will show up in the ELSE condition as it is the only condition left for else ig. What do you think?
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.
Hi @namangupta01, kindly give this another look when you get some time. Thanks and nice to you see around!
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.
Just a couple of suggestions. Other than this everything looks great. 👍
@@ -78,12 +78,16 @@ def search_maps(input, limit = 25, order = :natural, type = :boolean) | |||
# chained to the notes that are tagged with those values | |||
def search_tags(query, limit = 10) | |||
suggestions = [] | |||
# order tag autosuggestions by placing exact match of the tag on the top, then tags with query and some other text as suffix, | |||
# then tags with some text as prefix or suffix and lastly the tags with some text as prefix | |||
tags_order = 'CASE WHEN name LIKE ? OR name LIKE ? THEN 1 WHEN name LIKE ? OR name LIKE ? THEN 2 WHEN name LIKE ? OR name LIKE ? THEN 4 ELSE 3 END', "#{query}", "#{query.to_s.gsub(' ', '-')}", "#{query}%", "#{query.to_s.gsub(' ', '-')}%", "%#{query}", "%#{query.to_s.gsub(' ', '-')}" |
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.
Prefer to_s
over string interpolation.
@@ -78,12 +78,16 @@ def search_maps(input, limit = 25, order = :natural, type = :boolean) | |||
# chained to the notes that are tagged with those values | |||
def search_tags(query, limit = 10) | |||
suggestions = [] | |||
# order tag autosuggestions by placing exact match of the tag on the top, then tags with query and some other text as suffix, | |||
# then tags with some text as prefix or suffix and lastly the tags with some text as prefix | |||
tags_order = 'CASE WHEN name LIKE ? OR name LIKE ? THEN 1 WHEN name LIKE ? OR name LIKE ? THEN 2 WHEN name LIKE ? OR name LIKE ? THEN 4 ELSE 3 END', "#{query}", "#{query.to_s.gsub(' ', '-')}", "#{query}%", "#{query.to_s.gsub(' ', '-')}%", "%#{query}", "%#{query.to_s.gsub(' ', '-')}" |
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.
Prefer to_s
over string interpolation.
Trying to resolve in #9873 |
Sorry this is so annoying! I actually just merged #9881 so if you'd like to try rebasing over it it might help! |
3453e23
to
b1c2144
Compare
Code Climate has analyzed commit b1c2144 and detected 2 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
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.
This is looking good 🎉 🎉
We can ignore code climate warnings...let me know if we are good to merge...Thanks @17sushmita 🎉 🎉
Yes, we are good to merge @cesswairimu. Thanks 😄 |
Looks great! I resolved the CodeClimate and for some reason the CodeCov error just went away instantly. 🤷 |
this is so great! |
Hooray, thanks so much everyone!! 😀 |
* order tags based on best match to search query * minor changes in ordering tags based on best match
* order tags based on best match to search query * minor changes in ordering tags based on best match
Fixes #9623
rake test
@publiclab/reviewers
for help, in a comment below