Fix active threads endpoint

Previously, pagination was done before reducing from content to threads,
which meant that fewer threads than requested could be returned and that
the number of pages returned could be incorrect.

api/users.rb

@@ -23,20 +23,25 @@ end
 get "#{APIPREFIX}/users/:user_id/active_threads" do |user_id|
   return {}.to_json if not params["course_id"]
 
-  get_thread_id = lambda {|c| c._type == "Comment" ? c.comment_thread_id : c.id}
-  get_thread = lambda {|thread_id| CommentThread.find(thread_id)}
-
   page = (params["page"] || DEFAULT_PAGE).to_i
   per_page = (params["per_page"] || DEFAULT_PER_PAGE).to_i
+  per_page = DEFAULT_PER_PAGE if per_page <= 0
 
   active_contents = Content.where(author_id: user_id, anonymous: false, anonymous_to_peers: false, course_id: params["course_id"])
                            .order_by(updated_at: :desc)
 
-  num_pages = [1, (active_contents.count / per_page.to_f).ceil].max
+  # Get threads ordered by most recent activity, taking advantage of the fact
+  # that active_contents is already sorted that way
+  active_thread_ids = active_contents.inject([]) do |thread_ids, content|
+    thread_id = content._type == "Comment" ? content.comment_thread_id : content.id
+    thread_ids << thread_id if not thread_ids.include?(thread_id)
+    thread_ids
+  end
+
+  num_pages = [1, (active_thread_ids.count / per_page.to_f).ceil].max
   page = [num_pages, [1, page].max].min
 
-  paged_active_contents = active_contents.page(page).per(per_page)
-  paged_thread_ids = paged_active_contents.map(&get_thread_id).uniq
+  paged_thread_ids = active_thread_ids[(page - 1) * per_page, per_page]
 
   # Find all the threads by id, and then put them in the order found earlier.
   # Necessary because CommentThread.find does return results in the same

spec/api/user_spec.rb

@@ -92,6 +92,9 @@ describe "app" do
         @comments["t3 c4"].author = @users["u100"]
         @comments["t3 c4"].anonymous_to_peers = true
         @comments["t3 c4"].save!
+        @comments["t5 c1"].author = @users["u100"]
+        @comments["t5 c1"].anonymous = true
+        @comments["t5 c1"].save!
         rs = thread_result 100, course_id: "xyz"
         rs.length.should == 1
         check_thread_result_json(@users["u100"], @threads["t0"], rs.first)
@@ -108,22 +111,25 @@ describe "app" do
         rs.length.should == 9
       end
 
-      it "correctly orders results by most recently updated" do
-        @threads.each do |k, v|
-          v.author = @users["u100"]
-          v.save!
-        end
-        @threads["t5"].updated_at = DateTime.now
-        @threads["t5"].save!
-        expected_order = @threads.keys.reverse.select{|k| k!="t5"}.insert(0, "t5")
+      it "correctly orders results by most recent update by selected user" do
+        user = @users["u100"]
+        base_time = DateTime.now
+        @comments["t2 c2"].author = user
+        @comments["t2 c2"].updated_at = base_time
+        @comments["t2 c2"].save!
+        @comments["t4 c4"].author = user
+        @comments["t4 c4"].updated_at = base_time + 1
+        @comments["t4 c4"].save!
+        @threads["t2"].updated_at = base_time + 2
+        @threads["t2"].save!
+        @threads["t3"].author = user
+        @threads["t3"].updated_at = base_time + 4
+        @threads["t3"].save!
         rs = thread_result 100, course_id: "xyz"
         actual_order = rs.map {|v| v["title"]}
-        actual_order.should == expected_order
+        actual_order.should == ["t3", "t4", "t2", "t0"]
       end
 
-      # TODO: note the checks on result["num_pages"] are disabled.
-      # there is a bug in GET "#{APIPREFIX}/users/:user_id/active_threads
-      # and this value is often wrong.
       context "pagination" do
         def thread_result_page (page, per_page)
           get "/api/v1/users/100/active_threads", course_id: "xyz", page: page, per_page: per_page
@@ -132,27 +138,27 @@ describe "app" do
         end
 
         before(:each) do
-          @threads.each do |k, v|
-            @comments["#{k} c4"].author = @users["u100"]
-            @comments["#{k} c4"].save!
+          @comments.each do |k, v|
+            @comments[k].author = @users["u100"]
+            @comments[k].save!
           end
         end
 
         it "returns single page" do
           result = thread_result_page(1, 20)
           result["collection"].length.should == 10
-          #result["num_pages"].should == 1
+          result["num_pages"].should == 1
           result["page"].should == 1
         end
         it "returns multiple pages" do
           result = thread_result_page(1, 5)
           result["collection"].length.should == 5
-          #result["num_pages"].should == 2
+          result["num_pages"].should == 2
           result["page"].should == 1
 
           result = thread_result_page(2, 5)
           result["collection"].length.should == 5
-          #result["num_pages"].should == 2
+          result["num_pages"].should == 2
           result["page"].should == 2
         end
         it "orders correctly across pages" do
@@ -164,12 +170,30 @@ describe "app" do
             page = i + 1
             result = thread_result_page(page, per_page)
             result["collection"].length.should == (page * per_page <= @threads.length ? per_page : @threads.length % per_page)
-            #result["num_pages"].should == num_pages
+            result["num_pages"].should == num_pages
             result["page"].should == page
             actual_order += result["collection"].map {|v| v["title"]}
           end
           actual_order.should == expected_order
         end
+        it "accepts negative parameters" do
+          result = thread_result_page(-5, -5)
+          result["collection"].length.should == 10
+          result["num_pages"].should == 1
+          result["page"].should == 1
+        end
+        it "accepts excessively large parameters" do
+          result = thread_result_page(9999, 9999)
+          result["collection"].length.should == 10
+          result["num_pages"].should == 1
+          result["page"].should == 1
+        end
+        it "accepts empty parameters" do
+          result = thread_result_page("", "")
+          result["collection"].length.should == 10
+          result["num_pages"].should == 1
+          result["page"].should == 1
+        end
       end
 
       def test_unicode_data(text)