Improve textSearch_profiles result #3134

milaaraujo · 2018-07-24T05:17:33Z

Hey everyone!

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

We created a new endpoint recentprofiles that returns the profiles with most recent activity.
We fixed recentPeople method. In one of our latest PR (Remove API Search endpoints duplication #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

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 · 2018-07-24T05:18:17Z

Moved from #3126

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

If we call /api/srch/profiles?srchString=Jeff the api perform the default search.
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.

plotsbot · 2018-07-24T05:31:05Z

	1 Warning
⚠️	New migrations added. Please update `schema.rb.example` by overwriting it with a copy of the up-to-date `db/schema.rb`. Also, be aware to preserve the MySQL-specific conditions for full-text indices.

	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

milaaraujo · 2018-07-25T04:00:06Z

Hey @jywarren, regarding your question from the other PR (# 3126):

Have you tried requesting both and comparing their responses?

I created a silly test to illustrate the difference between the two ways of using the profiles endpoint.

First, I created users, in this order: user5, user4, user1, user2 and user3.
Then I created an activity (question) for users, in this order: user5, user4 and user3.
If I call http://localhost:3000/api/srch/profiles?srchString=test, then:

{
"items": [
{
"docId": 0,
"docType": "user",
"docUrl": "/profile/test3",
"docTitle": "test3",
"docSummary": "",
"docScore": 0
},
{
"docId": 0,
"docType": "user",
"docUrl": "/profile/test2",
"docTitle": "test2",
"docSummary": "",
"docScore": 0
},
{
"docId": 0,
"docType": "user",
"docUrl": "/profile/test1",
"docTitle": "test1",
"docSummary": "",
"docScore": 0
},
{
"docId": 0,
"docType": "user",
"docUrl": "/profile/test4",
"docTitle": "test4",
"docSummary": "",
"docScore": 0
},
{
"docId": 0,
"docType": "user",
"docUrl": "/profile/test5",
"docTitle": "test5",
"docSummary": "",
"docScore": 0
}
],
"srchParams": {
"srchString": "test",
"seq": null,
"showCount": null,
"pageNum": null,
"tagName": null
}
}

If I call http://localhost:3000/api/srch/profiles?srchString=test&order=recentdesc, then:

{
"items": [
{
"docId": 0,
"docType": "user",
"docUrl": "/profile/test3",
"docTitle": "test3",
"docSummary": "",
"docScore": 0
},
{
"docId": 0,
"docType": "user",
"docUrl": "/profile/test4",
"docTitle": "test4",
"docSummary": "",
"docScore": 0
},
{
"docId": 0,
"docType": "user",
"docUrl": "/profile/test5",
"docTitle": "test5",
"docSummary": "",
"docScore": 0
}
],
"srchParams": {
"srchString": "test",
"seq": null,
"showCount": null,
"pageNum": null,
"tagName": null
}
}

jywarren · 2018-07-25T14:06:22Z

Looks like a good moment to address this optimization issue too! #3147

jywarren · 2018-07-25T14:10:34Z

app/services/search_service.rb

+  def getRecentProfiles
+    sresult = DocList.new
+
+    nodes = Node.all.order("changed DESC").limit(100).distinct


I'm a little worried about the speed of this query. Could we collect up the uids, then run:

User.find_by(uid: uids.uniq).where(status: 1)

Then, we may also want to get recent edits, so what if we instead of doing Node, we searched Revision - that way we get both notes and wiki revisions?

jywarren · 2018-07-25T14:14:44Z

app/services/search_service.rb

-    users = SrchScope.find_users(srchString, limit = nil)
+    users =
+      if order == "recentdesc"
+        filterMatchingUserName(getRecentProfiles, srchString)


Here, I worry we might not get any results if we filter after selecting the most recent 100. Is there another way to do this, like finding matching usernames first, sorting by last activity using the user table -- i think there's a last activity column, but not sure when we update that...

It might take some kind of clever join and group query, like, joining revisions to the node table, and sorting by the revision timestamp...

jywarren · 2018-07-25T15:39:48Z

Hi, sorry, we've created a conflict while fixing #3148 - but it should be pretty easy to resolve. Thanks!

stefannibrasil · 2018-07-25T21:19:40Z

@jywarren agree with your comments! we will talk with our coaches today to help us improve the query, will keep you updated!

stefannibrasil · 2018-07-26T04:30:59Z

@jywarren we started doing the sorting and ordering today, still need to work on some things, but we believe we are targeting your concerns on them.

We are passing the a order_by and sort_direction as a parameter to the search/profiles? so we can use it on them.

The approach that our coaches are suggesting us to so is searching the profiles, joining them with nodes and then ordering them by the latest activities (ASC or DESC).

stefannibrasil · 2018-07-27T03:54:01Z

hi @jywarren could you review here, please? We worked today on improving the textSearch_profiles query. We have some questions, could you please take a look at the comments along the code?

We will work on the recentProfiles later after we finish this one. Also tomorrow we will write a test.

jywarren · 2018-07-27T14:59:46Z

Great! First, looks like a missing or extra "end" in this file?

SyntaxError: /app/app/services/search_service.rb:251: syntax error, unexpected keyword_end

Reading now...

jywarren · 2018-07-27T15:12:44Z

app/services/search_service.rb

+
+    if search_criteria.order_by == "recent"
+      user_scope = user_scope.joins(:revisions)\
+                             .order("timestamp #{search_criteria.sort_direction}")\


I guess i have a couple thoughts here. In general, I think it's best not to be adding activeRecord filters or conditions to an object that's had them added at an earlier level of abstraction (like, within SrchScope.find_users), because you can't see them when you're editing, so there's a strong possibility of doing things redundantly or in conflict. Better to do all your ActiveRecord stuff in one place as much as possible.

Second, I wonder if there's a way to reduce the # of levels of abstraction. Here it seems like we do:

A call arrives at app/api/srch/search.rb for get :profiles do

its sent to app/services/execute_search.rb for execute()

that's sent to app/services/search_service.rb for textSearch_profiles()

that's sent to app/services/srch_scope.rb for SrchScope.find_users()

finally an ActiveRecord call is made: User.search()...

Each file is now much more readable, but tracing the whole thing is a bit harder. The readability improvements are super, and the files are cleaner and less redundant. Great work! Now, I wonder if we could try flattening the system without losing those advantages? One way to try (and we must admit it's possible every layer is important! But let's try.) is to list why each layer is needed.

where incoming calls get routed

utility 'execute' function

back-end standard service available inside the app

scope?

do the ActiveRecord call

I'm not saying there is an obvious or easy answer here. And if we stick with 5 layers, that's fine! But this is how I'd recommend we look at the question of "what complexity is necessary" and how readable it is. Make sense? What do you think?

To your narrower question of the optimization, I think it makes sense that you're doing an initial call for users, then making a more complex version of it by joining revisions and then sorting using that new joined table. This looks great! Because the incoming query is already going to be pretty efficient, we think, and then we're just joining something and adding an ordering. How do you like the results it returns?

jywarren · 2018-08-09T20:21:16Z

The only difference is that the Typeahead returns results with different info(tagId instead of docId, which I still don't understand why)

As to this, I think it's not a meaningful difference, although originally i think the idea was that it would return some representation of a tag or topic, while docId would represent a page on the site. I think we should aim consolidate them to remove ambiguity...

jywarren · 2018-08-09T20:28:44Z

I'm going to open a new PR to get Travis to run against the with-bio version of User.search...

stefannibrasil · 2018-08-09T20:36:07Z

on travis it works only with bio... so you need to pass a username as the param to see the errors. For example, if you pass "jeff" it returns nil, but if you pass "something interesting about jeff" it returns something. At least that's how it was before

stefannibrasil · 2018-08-09T20:38:17Z

okay, about the meeting, tomorrow we can all attend at 5:00 pm EET, so can we confirm your -virtual- presence? :)

jywarren · 2018-08-09T20:42:48Z

Wait, i'm sorry -- does that mean Eastern European Time? https://www.worldtimebuddy.com/?qm=1&lid=5,13,5128581&h=5&date=2018-8-10&sln=10-10.5 If so, that would be fine. I'm sorry, I had thought ET (eastern time). Thanks!

…

On Thu, Aug 9, 2018 at 4:38 PM Stefanni ***@***.***> wrote: okay, about the meeting, tomorrow, Friday *August 10th 5:00 pm EET*, then? :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3134 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJzySLis_ljjSv7CygW0CYCmd-H0Uks5uPJ25gaJpZM4VcGMQ> .

jywarren · 2018-08-09T20:44:11Z

I think i have an idea about searching for "jeff"... hang on...did it happen only for "jeff" or also for "steff"?

Do you think you could try to reproduce the Travis error in a separate PR? Or was it exactly the error we're seeing here: #3224 ?

jywarren · 2018-08-09T20:47:08Z

And was the error you saw in test/unit/user_test.rb or in /test/functional/search_api_test.rb#L95 like in my PR?

stefannibrasil · 2018-08-09T21:04:53Z

yes, it's Eastern European Time! So, tomorrow 5:30 EET. See you there!

jywarren · 2018-08-09T21:34:18Z

great, thanks.

…

On Thu, Aug 9, 2018 at 5:05 PM Stefanni ***@***.***> wrote: yes, it's Eastern European Time! So, tomorrow 5:30 EET. See you there! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3134 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ0BkUU3YVcYaMEB259K6-1HzWMTvks5uPKP1gaJpZM4VcGMQ> .

jywarren · 2018-08-13T03:07:12Z

OK, I modified the test PR at #3224 to build on your latest commits from this PR, and see what the issue was with the nil results. But I see you've modified to go back to user.search searching bio and username... did that work for you? It's passing Travis, and we can do the "switch to only username searching via parameter" variant in a separate PR if we want -- let's just get this PR merged!

Can you give me an update and I am happy to check in on it once I land.. it'll be sometime tomorrow ✈️

If it's ready to go, I approve and can merge it!

stefannibrasil · 2018-08-14T01:43:17Z

Hi, @jywarren thanks for the approval and for the help! Saturday we were able to make it work searching for username and bio!! You can try now with our last commit if you want to check it out too! We don't know exactly where things started going wrong. I guess the errors that Travis was showing confused us and we missed something small and everything became a snowball of errors...

Yeah, I agree that it's better to create a new PR for the autocomplete feature, will start now!

So... I can't believe I'm saying this, LOL, but yes this can be merged!! 🎉 thank you!!

stefannibrasil · 2018-08-14T01:50:28Z

app/models/user.rb

@@ -42,7 +43,7 @@ class User < ActiveRecord::Base
  after_destroy :destroy_drupal_user

  def self.search(query)
-    User.where('MATCH(username, bio) AGAINST(?)', query)
+    User.where('MATCH(bio, username) AGAINST(? IN BOOLEAN MODE)', query + '*')


@jywarren gold lesson from this PR about the MySQL fulltext search: the 'IN BOOLEAN MODE' modifier with the * wildcard.

https://dev.mysql.com/doc/refman/8.0/en/fulltext-boolean.html

really glad that this is working now, thanks for the help!

jywarren · 2018-08-14T09:19:53Z

Congratulations!!! 👍🏼⚡⚡

* Version 1: implementing new version of profiles endpoint and fixing recentPeople endpoint * Code climate * Code climate * Code climate [2] * Code climate [3] * Code climate [4] * Refactoring getRecentProfiles method * fixing order_by X sort_by * Trying to fix travis error * trying to fix travis again * using find_users * Fix User.search query * Fixing conflict * Giving better names for the methods * Hacking travis

ghost assigned milaaraujo Jul 24, 2018

ghost added the in progress label Jul 24, 2018

milaaraujo assigned stefannibrasil Jul 24, 2018

milaaraujo force-pushed the profiles-endpoint-most-recently-people branch 3 times, most recently from 3970a28 to 6c6c84a Compare July 25, 2018 05:53

jywarren reviewed Jul 25, 2018

View reviewed changes

stefannibrasil force-pushed the profiles-endpoint-most-recently-people branch 2 times, most recently from c01cae5 to 45ccc31 Compare July 26, 2018 04:26

stefannibrasil changed the title ~~New version of 'profiles' endpoint and fixing 'recentPeople' endpoint~~ [WIP] version of 'profiles' endpoint and fixing 'recentPeople' endpoint Jul 26, 2018

stefannibrasil closed this Jul 26, 2018

ghost removed the in progress label Jul 26, 2018

stefannibrasil reopened this Jul 26, 2018

ghost added the in progress label Jul 26, 2018

stefannibrasil force-pushed the profiles-endpoint-most-recently-people branch from 45ccc31 to 90603f5 Compare July 27, 2018 03:50

stefannibrasil mentioned this pull request Jul 27, 2018

Optimization needed for /api/srch/profiles API query #3147

Closed

jywarren reviewed Jul 27, 2018

View reviewed changes

stefannibrasil force-pushed the profiles-endpoint-most-recently-people branch 2 times, most recently from 04a9b81 to d210f2d Compare July 29, 2018 01:04

stefannibrasil mentioned this pull request Jul 29, 2018

Link addition to email for marking comment as spam #3163

Merged

5 tasks

jywarren mentioned this pull request Aug 9, 2018

Profile api test PR #3224

Closed

stefannibrasil force-pushed the profiles-endpoint-most-recently-people branch 4 times, most recently from d42a395 to 10441b4 Compare August 11, 2018 05:01

Hacking travis

42c5e32

stefannibrasil force-pushed the profiles-endpoint-most-recently-people branch from 10441b4 to 42c5e32 Compare August 11, 2018 05:24

jywarren approved these changes Aug 13, 2018

View reviewed changes

stefannibrasil mentioned this pull request Aug 14, 2018

Community Check-in #13 Summer Of Code Week 14 #3225

Closed

stefannibrasil reviewed Aug 14, 2018

View reviewed changes

stefannibrasil added ready and removed in progress labels Aug 14, 2018

stefannibrasil mentioned this pull request Aug 14, 2018

API Search profiles by username endpoint #3228

Closed

jywarren merged commit 17827d1 into publiclab:master Aug 14, 2018

ghost removed the ready label Aug 14, 2018

stefannibrasil mentioned this pull request Aug 21, 2018

Fix schema-rb-example file for SQLite #3242

Merged

milaaraujo deleted the profiles-endpoint-most-recently-people branch October 31, 2018 04:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve textSearch_profiles result #3134

Improve textSearch_profiles result #3134

milaaraujo commented Jul 24, 2018 •

edited

Loading

milaaraujo commented Jul 24, 2018 •

edited by stefannibrasil

Loading

plotsbot commented Jul 24, 2018 •

edited

Loading

milaaraujo commented Jul 25, 2018

jywarren commented Jul 25, 2018

jywarren Jul 25, 2018

jywarren Jul 25, 2018

jywarren Jul 25, 2018

jywarren commented Jul 25, 2018

stefannibrasil commented Jul 25, 2018

stefannibrasil commented Jul 26, 2018

stefannibrasil commented Jul 27, 2018

jywarren commented Jul 27, 2018

jywarren Jul 27, 2018

jywarren commented Aug 9, 2018

jywarren commented Aug 9, 2018

stefannibrasil commented Aug 9, 2018

stefannibrasil commented Aug 9, 2018 •

edited

Loading

jywarren commented Aug 9, 2018 via email

jywarren commented Aug 9, 2018

jywarren commented Aug 9, 2018

stefannibrasil commented Aug 9, 2018

jywarren commented Aug 9, 2018 via email

jywarren commented Aug 13, 2018

stefannibrasil commented Aug 14, 2018 •

edited

Loading

stefannibrasil Aug 14, 2018

jywarren commented Aug 14, 2018

Improve textSearch_profiles result #3134

Improve textSearch_profiles result #3134

Conversation

milaaraujo commented Jul 24, 2018 • edited Loading

milaaraujo commented Jul 24, 2018 • edited by stefannibrasil Loading

Moved from #3126

plotsbot commented Jul 24, 2018 • edited Loading

milaaraujo commented Jul 25, 2018

jywarren commented Jul 25, 2018

jywarren Jul 25, 2018

Choose a reason for hiding this comment

jywarren Jul 25, 2018

Choose a reason for hiding this comment

jywarren Jul 25, 2018

Choose a reason for hiding this comment

jywarren commented Jul 25, 2018

stefannibrasil commented Jul 25, 2018

stefannibrasil commented Jul 26, 2018

stefannibrasil commented Jul 27, 2018

jywarren commented Jul 27, 2018

jywarren Jul 27, 2018

Choose a reason for hiding this comment

jywarren commented Aug 9, 2018

jywarren commented Aug 9, 2018

stefannibrasil commented Aug 9, 2018

stefannibrasil commented Aug 9, 2018 • edited Loading

jywarren commented Aug 9, 2018 via email

jywarren commented Aug 9, 2018

jywarren commented Aug 9, 2018

stefannibrasil commented Aug 9, 2018

jywarren commented Aug 9, 2018 via email

jywarren commented Aug 13, 2018

stefannibrasil commented Aug 14, 2018 • edited Loading

stefannibrasil Aug 14, 2018

Choose a reason for hiding this comment

jywarren commented Aug 14, 2018

milaaraujo commented Jul 24, 2018 •

edited

Loading

milaaraujo commented Jul 24, 2018 •

edited by stefannibrasil

Loading

plotsbot commented Jul 24, 2018 •

edited

Loading

stefannibrasil commented Aug 9, 2018 •

edited

Loading

stefannibrasil commented Aug 14, 2018 •

edited

Loading