Merge pull request #64 from edx/feature/jsa/more-new-indexes

Feature/jsa/more new indexes

api/comment_threads.rb

 get "#{APIPREFIX}/threads" do # retrieve threads by course
   #if a group id is sent, then process the set of threads with that group id or with no group id
+  threads = Content.where(_type:"CommentThread", course_id: params["course_id"])
   if params["group_id"]
-    threads = CommentThread.any_of(
-    {:course_id => params["course_id"],:group_id => params[:group_id]},
-    {:course_id => params["course_id"],:group_id.exists => false},
+    threads = threads.any_of(
+      {:group_id => params[:group_id].to_i},
+      {:group_id.exists => false},
     )
-  else
-    threads = CommentThread.where(course_id: params["course_id"])
-    #else process them all
   end
   handle_threads_query(threads)
 end

api/commentables.rb

@@ -4,13 +4,12 @@ delete "#{APIPREFIX}/:commentable_id/threads" do |commentable_id|
 end
 
 get "#{APIPREFIX}/:commentable_id/threads" do |commentable_id|
+  threads = Content.where(_type:"CommentThread", commentable_id: commentable_id)
   if params["group_id"]
-    threads = CommentThread.any_of(
-    {:commentable_id => commentable_id, :group_id => params[:group_id]}, 
-    {:commentable_id => commentable_id, :group_id.exists => false}, 
+    threads = threads.any_of(
+      {:group_id => params[:group_id].to_i}, 
+      {:group_id.exists => false}, 
     )
-  else
-    threads = commentable.comment_threads
   end
     handle_threads_query(threads)    
 end

api/search.rb

@@ -37,29 +37,17 @@ get "#{APIPREFIX}/search/threads" do
       results = CommentThread.perform_search(params, options.merge(page: results.total_pages))
     end
 
-    threads = CommentThread.where(id: {"$in" => results.map {|r| r.id} }).to_a
-    if threads.length == 0
+    if results.length == 0
       collection = []
     else
-      pres_threads = ThreadPresenter.new(
-        threads,
+      pres_threads = ThreadSearchResultPresenter.new(
+        results,
         params[:user_id] ? user : nil,
-        params[:course_id] || threads.first.course_id
+        params[:course_id] || results.first.course_id
       )
       collection = pres_threads.to_hash_array(bool_recursive)
     end
 
-    # merge highlighted text / highlighted body.  not sure if this is used by the client,
-    # but doing it to preserve the legacy behavior
-    # TODO: move this logic into a presenter object for search results
-    result_map = Hash[results.map { |t| [t.id, t] }]
-    collection.each do |thread_hash|
-      thread_key = thread_hash["id"].to_s
-      highlight = result_map[thread_key].highlight || {}
-      thread_hash["highlighted_body"] = (highlight[:body] || []).first || thread_hash["body"]
-      thread_hash["highlighted_title"] = (highlight[:title] || []).first || thread_hash["title"]
-    end
-
     num_pages = results.total_pages
     page = [num_pages, [1, page].max].min
     {

lib/helpers.rb

+require 'new_relic/agent/method_tracer'
+
 helpers do
 
   def commentable
@@ -120,6 +122,7 @@ helpers do
       comment_threads = comment_threads.where(:course_id=>params[:course_id])
 
       if params[:flagged]
+        self.class.trace_execution_scoped(['Custom/handle_threads_query/find_flagged']) do
           #get flagged threads and threads containing flagged responses
           comment_ids = Comment.where(:course_id=>params[:course_id]).
             where(:abuse_flaggers.ne => [],:abuse_flaggers.exists => true).
@@ -131,7 +134,7 @@ helpers do
           comment_ids += thread_ids
           
           comment_threads = comment_threads.where(:id.in => comment_ids)
-        
+        end
       end
     end
 

models/comment_thread.rb

@@ -24,7 +24,6 @@ class CommentThread < Content
   field :pinned, type: Boolean
 
   index({author_id: 1, course_id: 1})
-  index({_type: 1, course_id: 1, pinned: -1, created_at: -1})
 
   include Tire::Model::Search
   include Tire::Model::Callbacks

models/content.rb

@@ -8,6 +8,10 @@ class Content
   field :historical_abuse_flaggers, type: Array, default: [] #preserve abuse flaggers after a moderator unflags
   field :author_username, type: String, default: nil
   
+  index({_type: 1, course_id: 1, pinned: -1, created_at: -1 }, {background: true} )
+  index({_type: 1, course_id: 1, pinned: -1, comment_count: -1, created_at: -1}, {background: true})
+  index({_type: 1, course_id: 1, pinned: -1, "votes.point" => -1, created_at: -1}, {background: true})
+
   index({comment_thread_id: 1, sk: 1}, {sparse: true})
   index({comment_thread_id: 1, endorsed: 1}, {sparse: true})
 

presenters/thread_search_result.rb

+require_relative 'thread'
+
+class ThreadSearchResultPresenter < ThreadPresenter
+
+  alias :super_to_hash :to_hash
+
+  def initialize(search_results, user, course_id)
+    @search_result_map = Hash[search_results.map { |t| [t.id, t] }]
+    threads = CommentThread.where(id: {"$in" => @search_result_map.keys}).to_a
+    # reorder fetched threads to match the original result order
+    threads = Hash[threads.map { |t| [t._id.to_s, t] }].values_at *search_results.map { |t| t.id }
+    super(threads, user, course_id)
+  end
+
+  def to_hash(thread, with_comments=false)
+    thread_hash = super_to_hash(thread, with_comments)
+    highlight = @search_result_map[thread.id.to_s].highlight || {}
+    thread_hash["highlighted_body"] = (highlight[:body] || []).first || thread_hash["body"]
+    thread_hash["highlighted_title"] = (highlight[:title] || []).first || thread_hash["title"]
+    thread_hash
+  end
+
+end

scripts/db/migrate-004-update-indexes.js

+db.contents.ensureIndex({_type: 1, course_id: 1, pinned: -1, comment_count: -1, created_at: -1}, {background: true})
+db.contents.ensureIndex({_type: 1, course_id: 1, pinned: -1, "votes.point": -1, created_at: -1}, {background: true})

scripts/db/revert-migrate-004-update-indexes.js

+db.contents.dropIndex({_type: 1, course_id: 1, pinned: -1, comment_count: -1, created_at: -1})
+db.contents.dropIndex({_type: 1, course_id: 1, pinned: -1, "votes.point": -1, created_at: -1})

spec/presenters/thread_search_result_spec.rb

+require 'spec_helper'
+
+describe ThreadSearchResultPresenter do
+  context "#to_hash" do
+  
+    before(:each) { setup_10_threads }
+
+    # NOTE: throrough coverage of search result hash structure is presently provided in spec/api/search_spec
+    def check_search_result_hash(search_result, hash)
+      hash["highlighted_body"].should == ((search_result.highlight[:body] || []).first || hash["body"])
+      hash["highlighted_title"].should == ((search_result.highlight[:title] || []).first || hash["title"])
+    end
+
+    def check_search_results_hash_array(search_results, hashes)
+      expected_order = search_results.map {|t| t.id}
+      actual_order = hashes.map {|h| h["id"].to_s}
+      actual_order.should == expected_order
+      hashes.each_with_index { |hash, i| check_search_result_hash(search_results[i], hash) }
+    end
+
+    it "presents search results in correct order" do
+      threads_random_order = @threads.values.shuffle
+      mock_results = threads_random_order.map do |t| 
+        double(Tire::Results::Item, :id => t._id.to_s, :highlight => {:body => ["foo"], :title => ["bar"]})
+      end
+      pres = ThreadSearchResultPresenter.new(mock_results, nil, DFLT_COURSE_ID)
+      check_search_results_hash_array(mock_results, pres.to_hash_array)
+    end
+
+    it "presents search results with correct default highlights" do
+      threads_random_order = @threads.values.shuffle
+      mock_results = threads_random_order.map do |t| 
+        double(Tire::Results::Item, :id => t._id.to_s, :highlight => {})
+      end
+      pres = ThreadSearchResultPresenter.new(mock_results, nil, DFLT_COURSE_ID)
+      check_search_results_hash_array(mock_results, pres.to_hash_array)
+    end
+
+  end
+end