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

Search endpoint most recently people #3126

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@milaaraujo
Collaborator

milaaraujo commented Jul 22, 2018

Hey everyone!

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

  1. We created a new endpoint recentprofiles that returns the profiles with most recent activity.
  2. We fixed recentPeople method. In one of our latest PR (#3045) we did the following change that was not correct. Because in this part of the code the limit is for the node search. So we undid this change and added a count to control the number of profiles returned (line 290).

nodes = Node.all.order("changed DESC").limit(100).distinct -> nodes = Node.all.order("changed DESC").limit(srchString).distinct

  1. We don't think the name of the recentPeople method is appropriate for the peopleLocations endpoint, since the method returns the location of people with most recent activity with a specific tag. Can we change it to peopleLocations? So maybe we can use recentPeople for our new method.

We would like some feedback about our strategy here, thank you! :)

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

@milaaraujo milaaraujo force-pushed the milaaraujo:search-endpoint-most-recently-people branch from 2a0fa7e to 4ab4f06 Jul 22, 2018

@jywarren

This comment has been minimized.

Contributor

jywarren commented Jul 22, 2018

@plotsbot

This comment has been minimized.

Collaborator

plotsbot commented Jul 22, 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

@stefannibrasil

This comment has been minimized.

Collaborator

stefannibrasil commented Jul 22, 2018

HI, @jywarren we believe so because right now it is receiving a searchString and we are not doing any restriction about the string format.

We tested locally and it is returning the profiles with the most recent activity, so we think we can help with the #2925 new feature, yeah :)

@stefannibrasil

This comment has been minimized.

Collaborator

stefannibrasil commented Jul 22, 2018

this week we will refine the method, try to return a better list of results and then we'll fix the codeclimate issues, we wanted first to know if that is what you needed for the mentioned PR :)

oh, what do you think of changing the method name for the peoplelocations endpoint? we think that recentPeople for this endpoint does not reflect what it is doing... let us know anything!

@jywarren

This comment has been minimized.

Contributor

jywarren commented Jul 23, 2018

Just catching up here- what's the difference betwen recentPeople and recentprofiles? Sorry if I'm being slow on a monday morning! 😄

@jywarren

This comment has been minimized.

Contributor

jywarren commented Jul 23, 2018

And did you want to change recentPeople or peoplelocations, and to what? Thanks!

@jywarren

This comment has been minimized.

Contributor

jywarren commented Jul 23, 2018

Ah, sorry, i see you asked for clarification here: #3069 (comment)

Could we imagine these as a single endpoint, which has different sorting parameters, like a keyword to match usernames against (if there's no keyword, it just shows recent profiles?) and that the default sort is recency of activity, but it can be set to other sorts, like maybe created date?

Just trying to think what would be simplest!

@jywarren

This comment has been minimized.

Contributor

jywarren commented Jul 23, 2018

See how, for example, https://publiclab.org/tags gets additional parameters, like https://publiclab.org/tags?sort=uses&order=desc when you click the headers. Do you think this would be more compact to code? Then perhaps it could just be /people and the recent would be a sort, rather than the endpoint itself?

Not set on this, just wondering what you think! Thanks!

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Jul 24, 2018

Hi @jywarren,

What's the difference betwen recentPeople and recentprofiles?

textSearch_recentPeople is the method called for the /api/srch/peoplelocations endpoint. The method gets X number of latest contributors - with X equal to srchString - optionally we can also filter the search by tag. For example, if I call /api/srch/peoplelocations?srchString=10&tagName=collaborator the api will return the location of the 10 users with most recent activities and with the tag 'collaborator'. Or if I call just /api/srch/peoplelocations?srchString=10 the api will return the location of the 10 users with the most recent activities.

/api/srch/recentprofiles is the new endpoint we are creating to search for more recently updated profiles for matching text. It works like profile endpoint, however it sorts users and returns those with recent activity first.

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Jul 24, 2018

@stefannibrasil and I are thinking that maybe, for now, it would it be simpler just to change the implementation of endpoint /api/srch/profiles by our implementation /api/srch/recentprofiles. So we would have only one endpoint that always returns the most recent users. Then we could open other pull request to add other ways to sort the results as you suggested. But for now, it makes sense that by default we order by activity, right?

@jywarren

This comment has been minimized.

Contributor

jywarren commented Jul 24, 2018

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Jul 24, 2018

Hey @jywarren, I implemented a new version of profiles endpoint following your idea.

  1. If we call '/api/srch/profiles?srchString=Jeff' the api perform the default search.
  2. If we call '/api/srch/profiles?srchString=Jeff&order=recentdesc' the api returns more recently updated profiles for matching text.

That way I do not take the risk of breaking anything that already uses the endpoint. I'm going to open a new PR with these changes.

@milaaraujo milaaraujo closed this Jul 24, 2018

@wafflebot wafflebot bot removed the in progress label Jul 24, 2018

@milaaraujo milaaraujo deleted the milaaraujo:search-endpoint-most-recently-people branch Oct 31, 2018

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