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

Remove Typeahead Service #3257

Merged
merged 4 commits into from Aug 28, 2018

Conversation

Projects
None yet
4 participants
@stefannibrasil
Collaborator

stefannibrasil commented Aug 25, 2018

This is the first major attempt to decrease the amount of duplicated and ambiguous work that the API currently has. We removed a bunch of unused lines as well.

If we query the Search services from API/docs we see that Typeahead results aren't super useful or meaningful for the project. So our goal is to focus entirely on the Search Service, that currently returns better results than Typeahead. Since the beginning, we haven't touched on this Typeahead service, which is a signal that it isn't being used...

The tests are all green. For now, the queries are not so great, they will be improved along the next weeks (hopefully). With that in mind, we are planning the advanced search now. Our main goal is to have one good Search Service that can be useful to the community and returns insightful results.

@plotsbot

This comment has been minimized.

Collaborator

plotsbot commented Aug 25, 2018

2 Messages
📖 @stefannibrasil Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: Fixes #123.

Generated by 🚫 Danger

@@ -19,9 +18,7 @@ class API < Grape::API
models: [
SearchRequest::Entity,
DocResult::Entity,
TagResult::Entity,
DocList::Entity,
TagList::Entity

This comment has been minimized.

@jywarren

jywarren Aug 25, 2018

Contributor

Just to confirm, does the Search Service return tags? I think I remember that it may, just as part of a DocList or something.... but note how tags are returned in the site header's autocompletion search:

image

This comment has been minimized.

@jywarren

jywarren Aug 25, 2018

Contributor

Otherwise this cleanup looks great!

This comment has been minimized.

@stefannibrasil

stefannibrasil Aug 28, 2018

Collaborator

hey, @jywarren I pushed the changes to the unstable, but I don't know why the server isn't running... anyway, about that, I spent the night fixing this. DocResult also has those attributes, so I changed the typeahead script to use the items from DocResult instead of TagResult. I can't reproduce locally nor on the unstable to send to you a print, but the last commit shows how the icons are generated.

Interesting because now I finally get what are those DocResult params for! xD thanks for the help, I believe now this is good to go!

@@ -34,7 +34,7 @@ class Search < Grape::API
Search.execute(:profiles, params)
end
# Request URL should be /api/srch/notes?srchString=QRY[&seq=KEYCOUNT&showCount=NUM_ROWS&pageNum=PAGE_NUM]

This comment has been minimized.

@jywarren

jywarren Aug 25, 2018

Contributor

Just curious what the reason for removing these parameters... sorry, haven't looked at this code for a while, can you remind me if they're not used or something?

This comment has been minimized.

@stefannibrasil

stefannibrasil Aug 28, 2018

Collaborator

not used at all! I believe they were meant to be used like the sort_by and order_direction, or field params that we added, for example, but those old ones are not being used.

@stefannibrasil stefannibrasil force-pushed the milaaraujo:remove-typeaheadservice branch from 0985f22 to f2be3fc Aug 25, 2018

@milaaraujo milaaraujo self-assigned this Aug 25, 2018

@jywarren

This comment has been minimized.

Contributor

jywarren commented Aug 28, 2018

OK, looks good then! Merging this now. Thanks! And we'll double check that the tags still appear in stable's autocomplete search.

@jywarren jywarren merged commit 8d51151 into publiclab:master Aug 28, 2018

6 checks passed

ci/gitlab/unstable Pipeline passed on GitLab
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at ?%
Details
danger/danger All green. Jolly good show.
Details

@wafflebot wafflebot bot removed the in progress label Aug 28, 2018

@stefannibrasil stefannibrasil deleted the milaaraujo:remove-typeaheadservice branch Aug 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment