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

API Search profiles: adding parameter to search (only) by username #3235

Merged
merged 7 commits into from Aug 21, 2018

Conversation

Projects
None yet
4 participants
@milaaraujo
Collaborator

milaaraujo commented Aug 16, 2018

Hey everybody!

We are opening this request to fix the issue #3228.

We added a new parameter to the profile endpoint so now we can search only by username. We will use this new functionality on the autocomplete feature.

We would like some feedback, thanks! :)

fixes #3228
@publiclab/reviewers @publiclab/soc @jywarren

@plotsbot

This comment has been minimized.

Collaborator

plotsbot commented Aug 16, 2018

1 Message
📖 @milaaraujo 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.

Generated by 🚫 Danger

search_type = endpoint
search_criteria = SearchCriteria.new(search_query, tag: tag_query, sort_by: sort_query, order_direction: order_query)
search_criteria = SearchCriteria.new(search_query, tag: tag_query, sort_by: sort_query, order_direction: order_query, field: field_query)

This comment has been minimized.

@jywarren

jywarren Aug 16, 2018

Contributor

Do you think we could break this up over multiple lines? It could be indented so they line up. And is there an advantage to making variables to store these values in the above lines versus directly using params[] in the .new call?

user_scope = SrchScope.find_users(search_criteria.query, limit = 10)
user_scope =
if search_criteria.field == "username"
SrchScope.find_by_username(search_criteria.query, limit = 10)

This comment has been minimized.

@jywarren

jywarren Aug 16, 2018

Contributor

Here we might think of setting an optional limit parameter in profiles(search_criteria, limit = 10) and then we could override it if needed?

end
end
def self.find_by_username(query, limit)

This comment has been minimized.

@jywarren

jywarren Aug 16, 2018

Contributor

Great!

@@ -148,6 +148,34 @@ def app
assert matcher =~ json
end
# search by username and bio, returns users by id when order_by is not provided and sorted direction default DESC
test 'search profiles by username and bio without order_by and default sort_direction' do
get '/api/srch/profiles?srchString=steff'

This comment has been minimized.

@jywarren

jywarren Aug 16, 2018

Contributor

Sorry, looking on my phone... is there also a text for only bio content? Something that has no username matches?

This comment has been minimized.

@stefannibrasil

stefannibrasil Aug 17, 2018

Collaborator

we did like this: the previous tests (that we first had in mind to search only by username) have the field=username and therefore, returns only the users searched by username. So we added another one that would search only by username, but yeah, good point, we added a new one to search only by bio!

This comment has been minimized.

@stefannibrasil

stefannibrasil Aug 17, 2018

Collaborator

@jywarren we pushed some changes, thanks for the feedback!

stefannibrasil and others added some commits Aug 17, 2018

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Aug 18, 2018

Hey @jywarren,

We've just realized that code in the master branch contains an error. In line 9 of plots2/app/services/srch_scope.rb we forgot to replace input for query.
desenho sem titulo 1
This error only occurs in development/test environment, when using sqlite. That's why Travis did not complain about it. We are fixing this problem here in this PR!

@stefannibrasil

This comment has been minimized.

Collaborator

stefannibrasil commented Aug 21, 2018

@publiclab/reviewers anyone to review this PR? :)

@@ -129,7 +129,7 @@ test_user:
password_salt: <%= salt = Authlogic::Random.hex_token %>
crypted_password: <%= Authlogic::CryptoProviders::Sha512.encrypt("secretive" + salt) %>
persistence_token: <%= Authlogic::Random.hex_token %>
bio: ''
bio: 'I love ruby'

This comment has been minimized.

@jywarren

jywarren Aug 21, 2018

Contributor

😄 💎

crypted_password: <%= Authlogic::CryptoProviders::Sha512.encrypt("secretive" + salt) %>
persistence_token: <%= Authlogic::Random.hex_token %>
last_request_at: <%= Time.now %>
bio: 'data is a nice cat and he loves steff!'

This comment has been minimized.

@jywarren

jywarren Aug 21, 2018

Contributor

😹

@jywarren

This looks fantastic!

@jywarren

This comment has been minimized.

Contributor

jywarren commented Aug 21, 2018

Hi, this looks great, and I'm going to merge it. Just for future reference, if you can also say something like "ready to merge if you approve" then I can do so straight away... here it looks fine but sometimes folks want feedback but not actually to have it merged. Maybe that'll help me speed things up on my end a bit -- many thanks!

@jywarren jywarren merged commit ea2b5ba into publiclab:master Aug 21, 2018

3 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
danger/danger All green. Congrats.
Details

@wafflebot wafflebot bot removed the review-me label Aug 21, 2018

@stefannibrasil

This comment has been minimized.

Collaborator

stefannibrasil commented Aug 22, 2018

thank you, @jywarren ! Ok, from now on we are gonna add this "ready to merge if you approve" tag xD

@milaaraujo milaaraujo deleted the milaaraujo:api/add-username-to-profiles-endpoint branch Oct 31, 2018

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