Merge pull request #145 from edx/jia/MA-1189

MA-1189 ThreadGET - optionally include response comments via 'recursive' bool

api/comment_threads.rb

@@ -47,7 +47,7 @@ get "#{APIPREFIX}/threads/:thread_id" do |thread_id|
   else
     resp_limit = nil
   end
-  presenter.to_hash(true, resp_skip, resp_limit).to_json
+  presenter.to_hash(true, resp_skip, resp_limit, bool_recursive).to_json
 end
 
 put "#{APIPREFIX}/threads/:thread_id" do |thread_id|

presenters/thread.rb

@@ -23,7 +23,7 @@ class ThreadPresenter
     @is_endorsed = is_endorsed
   end
 
-  def to_hash with_responses=false, resp_skip=0, resp_limit=nil
+  def to_hash with_responses=false, resp_skip=0, resp_limit=nil, recursive=true
     raise ArgumentError unless resp_skip >= 0
     raise ArgumentError unless resp_limit.nil? or resp_limit >= 1
     h = @thread.to_hash
@@ -32,7 +32,11 @@ class ThreadPresenter
     h["endorsed"] = @is_endorsed || false
     if with_responses
       if @thread.thread_type.discussion? && resp_skip == 0 && resp_limit.nil?
+        if recursive
           content = Comment.where(comment_thread_id: @thread._id).order_by({"sk" => 1})
+        else
+          content = Comment.where(comment_thread_id: @thread._id, "parent_ids" => []).order_by({"sk" => 1})
+        end
         h["children"] = merge_response_content(content)
         h["resp_total"] = content.to_a.select{|d| d.depth == 0 }.length
       else
@@ -41,19 +45,20 @@ class ThreadPresenter
         when "question"
           endorsed_responses = responses.where(endorsed: true)
           non_endorsed_responses = responses.where(endorsed: false)
-          endorsed_response_info = get_paged_merged_responses(@thread._id, endorsed_responses, 0, nil)
+          endorsed_response_info = get_paged_merged_responses(@thread._id, endorsed_responses, 0, nil, recursive)
           non_endorsed_response_info = get_paged_merged_responses(
             @thread._id,
             non_endorsed_responses,
             resp_skip,
-            resp_limit
+            resp_limit,
+            recursive
           )
           h["endorsed_responses"] = endorsed_response_info["responses"]
           h["non_endorsed_responses"] = non_endorsed_response_info["responses"]
           h["non_endorsed_resp_total"] = non_endorsed_response_info["response_count"]
           h["resp_total"] = non_endorsed_response_info["response_count"] + endorsed_response_info["response_count"]
         when "discussion"
-          response_info = get_paged_merged_responses(@thread._id, responses, resp_skip, resp_limit)
+          response_info = get_paged_merged_responses(@thread._id, responses, resp_skip, resp_limit, recursive)
           h["children"] = response_info["responses"]
           h["resp_total"] = response_info["response_count"]
         end
@@ -68,15 +73,20 @@ class ThreadPresenter
   # a hash containing the following:
   #   responses
   #     An array of hashes representing the page of responses (including
-  #     children)
+  #     children, if recursive is true)
   #   response_count
   #     The total number of responses
-  def get_paged_merged_responses(thread_id, responses, skip, limit)
+  def get_paged_merged_responses(thread_id, responses, skip, limit, recursive=false)
     response_ids = responses.only(:_id).sort({"sk" => 1}).to_a.map{|doc| doc["_id"]}
     paged_response_ids = limit.nil? ? response_ids.drop(skip) : response_ids.drop(skip).take(limit)
+    if recursive
       content = Comment.where(comment_thread_id: thread_id).
         or({:parent_id => {"$in" => paged_response_ids}}, {:id => {"$in" => paged_response_ids}}).
         sort({"sk" => 1})
+    else
+      content = Comment.where(comment_thread_id: thread_id, "parent_ids" => []).
+        where({:id => {"$in" => paged_response_ids}}).sort({"sk" => 1})
+    end
     {"responses" => merge_response_content(content), "response_count" => response_ids.length}
   end
 

spec/api/comment_thread_spec.rb

@@ -544,28 +544,28 @@ describe "app" do
         it "returns all responses when no skip/limit params given" do
           @threads.each do |n, thread|
             res = thread_result thread.id, {}
-            check_thread_response_paging_json thread, res
+            check_thread_response_paging_json thread, res, 0, nil, false
           end 
         end
 
         it "skips the specified number of responses" do
           @threads.each do |n, thread|
             res = thread_result thread.id, {:resp_skip => 1}
-            check_thread_response_paging_json thread, res, 1, nil
+            check_thread_response_paging_json thread, res, 1, nil, false
           end 
         end
 
         it "limits the specified number of responses" do
           @threads.each do |n, thread|
             res = thread_result thread.id, {:resp_limit => 2}
-            check_thread_response_paging_json thread, res, 0, 2
+            check_thread_response_paging_json thread, res, 0, 2, false
           end 
         end
 
         it "skips and limits responses" do
           @threads.each do |n, thread|
             res = thread_result thread.id, {:resp_skip => 3, :resp_limit => 5}
-            check_thread_response_paging_json thread, res, 3, 5
+            check_thread_response_paging_json thread, res, 3, 5, false
           end 
         end
 

spec/presenters/thread_spec.rb

@@ -77,19 +77,33 @@ describe ThreadPresenter do
         @reader = create_test_user('thread reader')
       end
 
-      it "handles with_responses=false" do
+      it "handles with_responses=false and recursive has no impact" do
         @threads_with_num_comments.each do |thread, num_comments|
           is_endorsed = num_comments > 0 && endorse_responses
-          hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash
+          # with response=false and recursive=false
+          hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(false, 0, nil, false)
           check_thread_result(@reader, thread, hash)
           ['children', 'resp_skip', 'resp_limit', 'resp_total'].each {|k| (hash.has_key? k).should be_false }
+          # with response=false and recursive=true
+          hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(false, 0, nil, true)
+          check_thread_result(@reader, thread, hash)
+          ['children', 'resp_skip', 'resp_limit', 'resp_total'].each {|k| (hash.has_key? k).should be_false }
+        end
+      end
+
+      it "handles with_responses=true and recursive=true" do
+        @threads_with_num_comments.each do |thread, num_comments|
+          is_endorsed = num_comments > 0 && endorse_responses
+          hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, 0, nil, true)
+          check_thread_result(@reader, thread, hash)
+          check_thread_response_paging(thread, hash, 0, nil, false, true)
         end
       end
 
-      it "handles with_responses=true" do
+      it "handles with_responses=true and recursive=false" do
         @threads_with_num_comments.each do |thread, num_comments|
           is_endorsed = num_comments > 0 && endorse_responses
-          hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash true
+          hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, 0, nil, false)
           check_thread_result(@reader, thread, hash)
           check_thread_response_paging(thread, hash)
         end
@@ -99,7 +113,7 @@ describe ThreadPresenter do
         @threads_with_num_comments.each do |thread, num_comments|
           is_endorsed = num_comments > 0 && endorse_responses
           [0, 1, 2, 9, 10, 11, 1000].each do |skip|
-            hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash true, skip
+            hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, skip, nil, true)
             check_thread_result(@reader, thread, hash)
             check_thread_response_paging(thread, hash, skip)
           end
@@ -111,7 +125,7 @@ describe ThreadPresenter do
           is_endorsed = num_comments > 0 && endorse_responses
           [1, 2, 3, 9, 10, 11, 1000].each do |limit|
             [0, 1, 2, 9, 10, 11, 1000].each do |skip|
-              hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash true, skip, limit
+              hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, skip, limit, true)
               check_thread_result(@reader, thread, hash)
               check_thread_response_paging(thread, hash, skip, limit)
             end
@@ -122,9 +136,9 @@ describe ThreadPresenter do
       it "fails with invalid arguments" do
         @threads_with_num_comments.each do |thread, num_comments|
           is_endorsed = num_comments > 0 && endorse_responses
-          expect{ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, -1, nil)}.to raise_error(ArgumentError)
+          expect{ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, -1, nil, true)}.to raise_error(ArgumentError)
           [-1, 0].each do |limit|
-            expect{ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, 0, limit)}.to raise_error(ArgumentError)
+            expect{ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, 0, limit, true)}.to raise_error(ArgumentError)
           end
         end
       end

spec/spec_helper.rb

@@ -282,30 +282,33 @@ def check_thread_result_json(user, thread, json_response)
   check_thread_result(user, thread, json_response, true)
 end
 
-def check_thread_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is_json=false)
+def check_thread_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is_json=false, recursive=false)
   case thread.thread_type
   when "discussion"
-    check_discussion_response_paging(thread, hash, resp_skip, resp_limit, is_json)
+    check_discussion_response_paging(thread, hash, resp_skip, resp_limit, is_json, recursive)
   when "question"
-    check_question_response_paging(thread, hash, resp_skip, resp_limit, is_json)
+    check_question_response_paging(thread, hash, resp_skip, resp_limit, is_json, recursive)
   end
 end
 
-def check_comment(comment, hash, is_json)
+def check_comment(comment, hash, is_json, recursive=false)
   hash["id"].should == (is_json ? comment.id.to_s : comment.id) # Convert from ObjectId if necessary
   hash["body"].should == comment.body
   hash["user_id"].should == comment.author_id
   hash["username"].should == comment.author_username
   hash["endorsed"].should == comment.endorsed
   hash["endorsement"].should == comment.endorsement
+  if recursive
     children = Comment.where({"parent_id" => comment.id}).sort({"sk" => 1}).to_a
     hash["children"].length.should == children.length
     hash["children"].each_with_index do |child_hash, i|
       check_comment(children[i], child_hash, is_json)
     end
+  end
 end
 
-def check_discussion_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is_json=false)
+
+def check_discussion_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is_json=false, recursive=false)
   all_responses = thread.root_comments.sort({"sk" => 1}).to_a
   total_responses = all_responses.length
   hash["resp_total"].should == total_responses
@@ -313,8 +316,9 @@ def check_discussion_response_paging(thread, hash, resp_skip=0, resp_limit=nil,
     all_responses.drop(resp_skip) :
     all_responses.drop(resp_skip).take(resp_limit)
   hash["children"].length.should == expected_responses.length
+
   hash["children"].each_with_index do |response_hash, i|
-    check_comment(expected_responses[i], response_hash, is_json)
+    check_comment(expected_responses[i], response_hash, is_json, recursive)
   end
   hash["resp_skip"].to_i.should == resp_skip
   if resp_limit.nil?
@@ -324,13 +328,13 @@ def check_discussion_response_paging(thread, hash, resp_skip=0, resp_limit=nil,
   end
 end
 
-def check_question_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is_json=false)
+def check_question_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is_json=false, recursive=false)
   all_responses = thread.root_comments.sort({"sk" => 1}).to_a
   endorsed_responses, non_endorsed_responses = all_responses.partition { |resp| resp.endorsed }
 
   hash["endorsed_responses"].length.should == endorsed_responses.length
   hash["endorsed_responses"].each_with_index do |response_hash, i|
-    check_comment(endorsed_responses[i], response_hash, is_json)
+    check_comment(endorsed_responses[i], response_hash, is_json, recursive)
   end
 
   hash["non_endorsed_resp_total"] == non_endorsed_responses.length
@@ -339,7 +343,7 @@ def check_question_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is
     non_endorsed_responses.drop(resp_skip).take(resp_limit)
   hash["non_endorsed_responses"].length.should == expected_non_endorsed_responses.length
   hash["non_endorsed_responses"].each_with_index do |response_hash, i|
-    check_comment(expected_non_endorsed_responses[i], response_hash, is_json)
+    check_comment(expected_non_endorsed_responses[i], response_hash, is_json, recursive)
   end
   total_responses = endorsed_responses.length + non_endorsed_responses.length
   hash["resp_total"].should == total_responses
@@ -352,8 +356,8 @@ def check_question_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is
   end
 end
 
-def check_thread_response_paging_json(thread, hash, resp_skip=0, resp_limit=nil)
-  check_thread_response_paging(thread, hash, resp_skip, resp_limit, true)
+def check_thread_response_paging_json(thread, hash, resp_skip=0, resp_limit=nil, recursive=false)
+  check_thread_response_paging(thread, hash, resp_skip, resp_limit, true, recursive)
 end
 
 # general purpose factory helpers