Skip to content
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

Using node.search() in the search service methods #3313

Merged
merged 12 commits into from Sep 18, 2018

Conversation

Projects
None yet
4 participants
@milaaraujo
Copy link
Collaborator

milaaraujo commented Sep 12, 2018

Using node.search() in the search service methods.
Removing duplicates from some endpoints (#3310).
Adding 'limit' parameter to methods.

@plotsbot

This comment has been minimized.

Copy link
Collaborator

plotsbot commented Sep 12, 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

@milaaraujo milaaraujo requested a review from jywarren Sep 12, 2018

@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Sep 12, 2018

Hey @jywarren, here

  def textSearch_nodes(srchString)
    sresult = DocList.new

    nodes = find_nodes(srchString, 25)
    puts nodes.inspect
    nodes.each do |match|
      *doc = DocResult.fromSearch(match.nid, 'file', match.path, match.title, 'NOTES', 0)*
      sresult.addDoc(doc)
    end

    sresult
  end

Should we use 'NOTES' or something else?

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Sep 12, 2018

I'm not that familiar with this section -- DocResult.fromSearch -- where is it looking for NOTES?

I guess the "nodes" should return node type "note" and "page", and maybe even "map". But I'm not sure where the logic is for that. Can you link to fromSearch?

@stefannibrasil

This comment has been minimized.

Copy link
Collaborator

stefannibrasil commented Sep 12, 2018

It isn't, 'NOTES' is new, to use on the Category header for the typeahead. This DocResult.fromSearch method is used to get the result type, path, etc:

def self.fromSearch(idval, typeval, urlval, titleval, category, scoreval)

Right now they are all under the same category 'NOTES', but we were wondering if you have any ideas of another category for this method. Maybe 'PAGES' or 'WIKI'?

OR

Maybe we should have only a general find_nodes method that returns all of this together and use the 'NOTES' category for them? Instead of having two methods find_nodes and find_notes ? I don't know exactly what would be best for this context.

Recap: This category is only used for the typeahead. The /search will have its own way to filter by type, on its view sepparetly.

@stefannibrasil

This comment has been minimized.

Copy link
Collaborator

stefannibrasil commented Sep 12, 2018

I think what would be super helpful would to have a description of what those SearchService methods should return, in terms of type of nodes, especially (but we may not be able to work on those soon).

@milaaraujo and I haven't worked on the majority of them, just a heads up.

milaaraujo added some commits Sep 13, 2018

@milaaraujo milaaraujo changed the title Using node.search() in the /notes and /all endpoints Using node.search() in the search service methods Sep 13, 2018

milaaraujo added some commits Sep 13, 2018

@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Sep 13, 2018

Well, I'm using 'PAGES' to differentiate from 'NOTES'.
+
Removing duplicates from some endpoints (#3310).
Adding 'limit' parameter to methods.

@milaaraujo milaaraujo closed this Sep 13, 2018

@wafflebot wafflebot bot removed the review-me label Sep 13, 2018

@milaaraujo milaaraujo reopened this Sep 13, 2018

@wafflebot wafflebot bot added the in progress label Sep 13, 2018

@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Sep 13, 2018

Code Climate is complaining about similar blocks of code. But I really think that in this case, it makes the code clearer.

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Sep 13, 2018

Currently, as you guessed, 'page' type means wiki pages. Unfortunately, we have some legacy names guiding all this -- see more at https://github.com/publiclab/plots2/blob/master/doc/DATA_MODEL.md

However, eventually, the goal might be to break 'notes' into 'notes' vs. 'questions', but for now PAGES for wiki pages sounds great.

@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Sep 13, 2018

So, I think we are ready to merge?

@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Sep 14, 2018

I just made some last changes! And now, after your review, we're ready.

@@ -24,7 +24,7 @@ class Node < ActiveRecord::Base
self.table_name = 'node'
self.primary_key = 'nid'

def self.search(query:, order: :default, type: :natural, limit:)
def self.search(query:, order: :default, type: :natural, limit: "25")

This comment has been minimized.

@jywarren

jywarren Sep 14, 2018

Contributor

lets have this be an integer here, and convert to a string when splicing it in -- is that ok?

This comment has been minimized.

@milaaraujo

milaaraujo Sep 14, 2018

Author Collaborator

Sure. Actually, I don't think we need to convert it to a string.

milaaraujo added some commits Sep 14, 2018

Change parameter limit to integer (nodes.search())
self.search(query:, order: :default, type: :natural, limit: 25)
@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Sep 18, 2018

Ok, last questions and we're probably ready. "nodes" was used because although in theory its great to be able to search wiki pages and notes separately, in practice we are mostly searching them together, although the separation is useful because we're likely to be able to do more sophisticated searching soon (!! 😁). What I want to confirm before merging is that in the current cases, we aren't omitting notes or pages in common searching scenarios?

@jywarren
Copy link
Contributor

jywarren left a comment

These are the areas i had questions about. Overall this looks fantastic. If you ping me on slack after responding, I'll do my best to merge this asap. Thanks!

@@ -14,7 +14,7 @@ def results
@title = 'Search'
@tagnames = params[:id].split(',')
@users = SearchService.new.find_users(params[:id], limit = 10)
@nodes = SearchService.new.find_nodes(params[:id], 100, params[:order].to_s.to_sym, params[:type].to_s.to_sym)
@nodes = SearchService.new.find_pages(params[:id], 100, params[:order].to_s.to_sym, params[:type].to_s.to_sym)

This comment has been minimized.

@jywarren

jywarren Sep 18, 2018

Contributor

Like here, shouldn't we search both notes and pages? We could combine two "find" calls, or does this mean we may still need a third "find_nodes"?

This comment has been minimized.

@stefannibrasil

stefannibrasil Sep 18, 2018

Collaborator

hi, Jeff, all of this concerns are fixed on the current branch that we are working and this was something that I mentioned at the time Camila opened this PR... because it makes no sense to separate things like it was implemented.

This comment has been minimized.

@jywarren

jywarren Sep 18, 2018

Contributor

Hi, I'm sorry, it's a combination of issues on my end -- it's a big PR, doing multiple things, so it's a little hard to keep it all in my head at once when providing feedback. Maybe in the future, breaking ones like this into multiple parts (say, limits, distincts, and separation of node/notes/pages?) could help. But also, I didn't understand originally that the central premise of the PR was to present separate results in the Typeahead for NOTES vs. PAGES, as in this display there are only NOTES:

image

Now I understand, and I think separate sections for NOTES and PAGES sounds wonderful. However, I still want to be sure that, for example, a call like:

SearchService.textSearch_all()

will return both notes and pages. They could be together, merged, or concatenated separately, but we should be sure they aren't forgotten -- i'm mainly thinking if there are any cases where someone would search and expect to see a page, but somehow we've forgotten to display that type.

@@ -14,11 +14,11 @@ def execute(type, search_criteria)
when :profiles
sresult = sservice.profiles(search_criteria)
when :notes

This comment has been minimized.

@jywarren

jywarren Sep 18, 2018

Contributor

Here, are we skipping pages?

This comment has been minimized.

@jywarren

jywarren Sep 18, 2018

Contributor

I'm still curious about this but it doesn't seem that it needs to be resolved in this PR. But curious about your thoughts!

This comment has been minimized.

@milaaraujo

milaaraujo Sep 18, 2018

Author Collaborator

Yes, here we are just using notes. But this is how the endpoint was working before:

def find_nodes(input, limit = 5, order = :default, type = :natural)
   Node.search(query: input, order: order, type: type, limit: limit)
            .where('type = "note" AND node.status = 1 AND title LIKE ?', '%' + input + '%')
[...]

And now we are using Node.search():

def find_notes(input, limit = 25, order = :natural, type = :boolean)
   Node.search(query: input, order: order, type: type, limit: limit)
            .where("`node`.`type` = 'note'")
Show resolved Hide resolved app/services/search_service.rb
doc = DocResult.fromSearch(match.nid, 'file', match.path, match.title, 'NOTES', 0)
sresult.addDoc(doc)
end

sresult
end

# Search nodes package up as a DocResult
def textSearch_pages(srchString, limit = 25)

This comment has been minimized.

@jywarren

jywarren Sep 18, 2018

Contributor

Do we need an equivalent combined nodes function? Could we concatenate the two existing queries somewhere to reduce redundancy?

This comment has been minimized.

@stefannibrasil

stefannibrasil Sep 18, 2018

Collaborator

So, we can use this as our unique method/endpoint to search for nodes, right?

def find_nodes(input, _limit = 5, order = :default)
    Node.search(query: input, order: order, limit: 5)
        .group(:nid)
        .where('node.status': 1)
        .distinct
  end

This comment has been minimized.

@jywarren

jywarren Sep 18, 2018

Contributor

This looks good. Do you want to add it here?

This comment has been minimized.

@jywarren

jywarren Sep 18, 2018

Contributor

er, to this PR (i didn't mean to this place in the code, to be clear)

This comment has been minimized.

@jywarren

jywarren Sep 18, 2018

Contributor

So on close reading, I don't think we NEED this in this PR, so i vote to merge, and if you think that there is a good use for a generalized find_nodes we can do it in a follow-up PR.

This comment has been minimized.

@milaaraujo

milaaraujo Sep 18, 2018

Author Collaborator

We don’t have a /search/nodes endpoint. We had a find_nodes() method that I removed to use find_pages(). It is true it just search for pages. If we want to search for all type of nodes we just need:

def find_nodes(input, _limit = 5, order = :default)
    Node.search(query: input, order: order, limit: 5)
  end

So, it would be simpler just to call Node.search()?

This comment has been minimized.

@jywarren

jywarren Sep 18, 2018

Contributor

Agreed, simpler.

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Sep 18, 2018

OK, I left a couple clarifying comments. If you're happy with how this works at both the typeahead and /search/____ interfaces, I'm happy to merge it, so just tell me what you think we should do?

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Sep 18, 2018

OK, just read through everything one more time. I think this is good to merge, so if you 👍 i can do so. I looked to see what are "added on" questions or concerns and what are core to the purpose(s) of this PR. And I think it looks good.

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Sep 18, 2018

👍 thanks very much for your hard work on this one and apologies again for the ambiguity! I think it's great.

@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Sep 18, 2018

I think it would be better to merge it! If we need to do any modification I can open a new PR. Stéfanni needs this to finish the frontend part.

@jywarren jywarren merged commit acbcac9 into publiclab:master Sep 18, 2018

3 checks passed

codeclimate Approved by Jeffrey Warren.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
danger/danger All green. Congrats.
Details

@wafflebot wafflebot bot removed the in progress label Sep 18, 2018

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Sep 18, 2018

Great, done!!!

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Sep 18, 2018

Thanks, everybody!

@milaaraujo milaaraujo deleted the milaaraujo:endpoint-node-search branch Oct 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.