Merge pull request #103 from edx/gprice/search-total-results

Return total result count for text search queries

api/search.rb

@@ -19,7 +19,7 @@ get "#{APIPREFIX}/search/threads" do
 
   sort_keyword_valid = (!params["sort_key"] && !params["sort_order"] || sort_key && sort_order)
 
-  if (!params["text"] && !params["commentable_ids"]) || !sort_keyword_valid
+  if !params["text"] || !sort_keyword_valid
     {}.to_json
   else
     page = (params["page"] || DEFAULT_PAGE).to_i
@@ -33,10 +33,14 @@ get "#{APIPREFIX}/search/threads" do
       per_page: per_page,
     }
 
-    results = CommentThread.perform_search(params, options)
+    result_hash = CommentThread.perform_search(params, options)
+    results = result_hash[:results]
+    total_results = result_hash[:total_results]
 
     if page > results.total_pages #TODO find a better way for this
-      results = CommentThread.perform_search(params, options.merge(page: results.total_pages))
+      result_hash = CommentThread.perform_search(params, options.merge(page: results.total_pages))
+      results = result_hash[:results]
+      total_results = result_hash[:total_results]
     end
 
     if results.length == 0
@@ -56,6 +60,7 @@ get "#{APIPREFIX}/search/threads" do
     self.class.trace_execution_scoped(['Custom/get_search_threads/json_serialize']) do
       json_output = {
         collection: collection,
+        total_results: total_results,
         num_pages: num_pages,
         page: page,
       }.to_json

models/comment_thread.rb

@@ -115,74 +115,61 @@ class CommentThread < Content
     search.sort {|sort| sort.by sort_key, sort_order} if sort_key && sort_order #TODO should have search option 'auto sort or sth'
 
     #again, b/c there is no relationship in ordinality, we cannot paginate if it's a text query
-    
-    if not params["text"]
-      search.size per_page
-      search.from per_page * (page - 1)
-    end
-
     results = search.results
-    
-    #if this is a search query, then also search the comments and harvest the matching comments
-    if params["text"]
 
-      search = Tire::Search::Search.new 'comments'
-      search.query {|query| query.match :body, params["text"]} if params["text"]
-      search.filter(:term, course_id: params["course_id"]) if params["course_id"]
-      search.size CommentService.config["max_deep_search_comment_count"].to_i
-      
-      #unforutnately, we cannot paginate here, b/c we don't know how the ordinality is totally
-      #unrelated to that of threads
-      
-      c_results = comment_ids = comments = thread_ids = nil
-      self.class.trace_execution_scoped(['Custom/perform_search/collect_comment_search_results']) do
-        c_results = search.results
-        comment_ids = c_results.collect{|c| c.id}.uniq
-      end
-      self.class.trace_execution_scoped(['Custom/perform_search/collect_comment_thread_ids']) do
-        comments = Comment.where(:id.in => comment_ids)
-        thread_ids = comments.collect{|c| c.comment_thread_id}
-      end
-
-      #thread_ids = c_results.collect{|c| c.comment_thread_id}
-      #as soon as we can add comment thread id to the ES index, via Tire updgrade, we'll 
-      #use ES instead of mongo to collect the thread ids
-     
-      #use the elasticsearch index instead to avoid DB hit
-      
-      self.class.trace_execution_scoped(['Custom/perform_search/collect_unique_thread_ids']) do
-        original_thread_ids = results.collect{|r| r.id}
+    search = Tire::Search::Search.new 'comments'
+    search.query {|query| query.match :body, params["text"]} if params["text"]
+    search.filter(:term, course_id: params["course_id"]) if params["course_id"]
+    search.size CommentService.config["max_deep_search_comment_count"].to_i
 
-        #now add the original search thread ids
-        thread_ids += original_thread_ids
-        
-        thread_ids = thread_ids.uniq
-      end
+    #unforutnately, we cannot paginate here, b/c we don't know how the ordinality is totally
+    #unrelated to that of threads
+
+    c_results = comment_ids = comments = thread_ids = nil
+    self.class.trace_execution_scoped(['Custom/perform_search/collect_comment_search_results']) do
+      c_results = search.results
+      comment_ids = c_results.collect{|c| c.id}.uniq
+    end
+    self.class.trace_execution_scoped(['Custom/perform_search/collect_comment_thread_ids']) do
+      comments = Comment.where(:id.in => comment_ids)
+      thread_ids = comments.collect{|c| c.comment_thread_id.to_s}
+    end
 
-      #now run one more search to harvest the threads and filter by group
-      search = Tire::Search::Search.new 'comment_threads'
-      search.filter(:terms, :thread_id => thread_ids)
-      search.filter(:terms, commentable_id: params["commentable_ids"]) if params["commentable_ids"]
-      search.filter(:term, course_id: params["course_id"]) if params["course_id"]
+    #thread_ids = c_results.collect{|c| c.comment_thread_id}
+    #as soon as we can add comment thread id to the ES index, via Tire updgrade, we'll 
+    #use ES instead of mongo to collect the thread ids
 
-      search.size per_page
-      search.from per_page * (page - 1)
+    #use the elasticsearch index instead to avoid DB hit
 
-      if params["group_id"]
+    self.class.trace_execution_scoped(['Custom/perform_search/collect_unique_thread_ids']) do
+      original_thread_ids = results.collect{|r| r.id}
 
-        search.filter :or, [
-          {:not => {:exists => {:field => :group_id}}},
-          {:term => {:group_id => params["group_id"]}}
+      #now add the original search thread ids
+      thread_ids += original_thread_ids
 
-        ]
-      end
+      thread_ids = thread_ids.uniq
+    end
+    
+    #now run one more search to harvest the threads and filter by group
+    search = Tire::Search::Search.new 'comment_threads'
+    search.filter(:terms, :thread_id => thread_ids)
+    search.filter(:terms, commentable_id: params["commentable_ids"]) if params["commentable_ids"]
+    search.filter(:term, course_id: params["course_id"]) if params["course_id"]
+
+    search.size per_page
+    search.from per_page * (page - 1)
 
-      search.sort {|sort| sort.by sort_key, sort_order} if sort_key && sort_order 
-      results = search.results
+    if params["group_id"]
+
+      search.filter :or, [
+        {:not => {:exists => {:field => :group_id}}},
+        {:term => {:group_id => params["group_id"]}}
 
+      ]
     end
 
-    results
+    search.sort {|sort| sort.by sort_key, sort_order} if sort_key && sort_order 
+    {results: search.results, total_results: thread_ids.length}
   end
 
   def activity_since(from_time=nil)

spec/api/search_spec.rb

@@ -9,6 +9,39 @@ describe "app" do
     let(:author) { create_test_user(42) }
 
     describe "GET /api/v1/search/threads" do
+      it "returns the correct values for total_results and num_pages", :focus => true do
+        course_id = "test_course_id"
+        for i in 1..100 do
+          text = "all"
+          text += " half" if i % 2 == 0
+          text += " quarter" if i % 4 == 0
+          text += " tenth" if i % 10 == 0
+          text += " one" if i == 100
+          # There is currently a bug that causes only 10 threads with matching
+          # titles/bodies to be considered, so this test case uses comments.
+          thread = make_thread(author, "dummy text", course_id, "dummy_commentable")
+          make_comment(author, thread, text)
+        end
+        # Elasticsearch does not necessarily make newly indexed content
+        # available immediately, so we must explicitly refresh the index
+        CommentThread.tire.index.refresh
+        Comment.tire.index.refresh
+
+        test_text = lambda do |text, expected_total_results, expected_num_pages|
+          get "/api/v1/search/threads", course_id: course_id, text: text, per_page: "10"
+          last_response.should be_ok
+          result = parse(last_response.body)
+          result["total_results"].should == expected_total_results
+          result["num_pages"].should == expected_num_pages
+        end
+
+        test_text.call("all", 100, 10)
+        test_text.call("half", 50, 5)
+        test_text.call("quarter", 25, 3)
+        test_text.call("tenth", 10, 1)
+        test_text.call("one", 1, 1)
+      end
+
       def test_unicode_data(text)
         # Elasticsearch may not be able to handle searching for non-ASCII text,
         # so prepend the text with an ASCII term we can search for.