Allow filtering by group_id in more endpoints

All endpoints that return lists of threads (except the endpoint used by
the notifier component) now allow filtering by group_id and are tested
accordingly.

api/comment_threads.rb

@@ -4,18 +4,12 @@
   if params[:commentable_ids]
     threads = threads.in({"commentable_id" => params[:commentable_ids].split(",")})
   end
-  #if a group id is sent, then process the set of threads with that group id or with no group id
-  if params["group_id"]
-    threads = threads.any_of(
-      {"group_id" => params[:group_id].to_i},
-      {"group_id" => {"$exists" => false}},
-    )
-  end
 
   handle_threads_query(
     threads,
     params["user_id"],
     params["course_id"],
+    params["group_id"],
     value_to_boolean(params["flagged"]),
     value_to_boolean(params["unread"]),
     value_to_boolean(params["unanswered"]),

api/commentables.rb

@@ -5,17 +5,12 @@
 
 get "#{APIPREFIX}/:commentable_id/threads" do |commentable_id|
   threads = Content.where({"_type" => "CommentThread", "commentable_id" => commentable_id})
-  if params["group_id"]
-    threads = threads.any_of(
-      {"group_id" => params[:group_id].to_i},
-      {"group_id" => {"$exists" => false}},
-    )
-  end
 
   handle_threads_query(
     threads,
     params["user_id"],
     params["course_id"],
+    params["group_id"],
     value_to_boolean(params["flagged"]),
     value_to_boolean(params["unread"]),
     value_to_boolean(params["unanswered"]),

api/notifications_and_subscriptions.rb

@@ -7,6 +7,7 @@
     user.subscribed_threads.where({"course_id" => params[:course_id]}),
     params["user_id"],
     params["course_id"],
+    params["group_id"],
     value_to_boolean(params["flagged"]),
     value_to_boolean(params["unread"]),
     value_to_boolean(params["unanswered"]),

api/search.rb

@@ -71,6 +71,7 @@
       CommentThread.in({"_id" => thread_ids.to_a}),
       local_params["user_id"],
       local_params["course_id"],
+      local_params["group_id"],
       value_to_boolean(local_params["flagged"]),
       value_to_boolean(local_params["unread"]),
       value_to_boolean(local_params["unanswered"]),

api/users.rb

@@ -37,19 +37,22 @@
     thread_ids
   end
 
-  num_pages = [1, (active_thread_ids.count / per_page.to_f).ceil].max
-  page = [num_pages, [1, page].max].min
-
-  paged_thread_ids = active_thread_ids[(page - 1) * per_page, per_page]
+  threads = CommentThread.in({"_id" => active_thread_ids})
 
-  # 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
-  # order as the provided ids.
-  paged_active_threads = CommentThread.find(paged_thread_ids).sort_by do |t| 
-    paged_thread_ids.index(t.id)
+  if params["group_id"]
+    threads = threads.any_of(
+      {"group_id" => params["group_id"].to_i},
+      {"group_id" => {"$exists" => false}}
+    )
   end
 
-  presenter = ThreadListPresenter.new(paged_active_threads.to_a, user, params[:course_id])
+  num_pages = [1, (threads.count / per_page.to_f).ceil].max
+  page = [num_pages, [1, page].max].min
+
+  sorted_threads = threads.sort_by {|t| active_thread_ids.index(t.id)}
+  paged_threads = sorted_threads[(page - 1) * per_page, per_page]
+
+  presenter = ThreadListPresenter.new(paged_threads, user, params[:course_id])
   collection = presenter.to_hash
 
   json_output = nil

lib/helpers.rb

@@ -116,8 +116,26 @@ def handle_paged_threads_query(paged_comment_threads)
 
   end
 
-  def handle_threads_query(comment_threads, user_id, course_id, filter_flagged, filter_unread, filter_unanswered, sort_key, sort_order, page, per_page)
-
+  def handle_threads_query(
+    comment_threads,
+    user_id,
+    course_id,
+    group_id,
+    filter_flagged,
+    filter_unread,
+    filter_unanswered,
+    sort_key,
+    sort_order,
+    page,
+    per_page
+  )
+    if group_id
+      comment_threads = comment_threads.any_of(
+        {"group_id" => group_id.to_i},
+        {"group_id" => {"$exists" => false}}
+      )
+    end
+
     if filter_flagged
       self.class.trace_execution_scoped(['Custom/handle_threads_query/find_flagged']) do
         # TODO replace with aggregate query?

spec/api/commentable_spec.rb

@@ -21,20 +21,32 @@
       end
     end
     describe "GET /api/v1/:commentable_id/threads" do
-      it "get all comment threads associated with a commentable object" do
-        get "/api/v1/question_1/threads"
+      def thread_result(commentable_id, params={})
+        get "/api/v1/#{commentable_id}/threads", params
         last_response.should be_ok
-        response = parse last_response.body
-        threads = response['collection']
+        parse(last_response.body)["collection"]
+      end
+      it "get all comment threads associated with a commentable object" do
+        threads = thread_result "question_1"
         threads.length.should == 2
         threads.index{|c| c["body"] == "can anyone help me?"}.should_not be_nil
         threads.index{|c| c["body"] == "it is unsolvable"}.should_not be_nil
       end
+      it "filters by group_id" do
+        group_thread = Commentable.find("question_1").comment_threads.first
+        threads = thread_result "question_1", group_id: 42
+        threads.length.should == 2
+        group_thread.group_id = 43
+        group_thread.save!
+        threads = thread_result "question_1", group_id: 42
+        threads.length.should == 1
+        group_thread.group_id = 42
+        group_thread.save!
+        threads = thread_result "question_1", group_id: 42
+        threads.length.should == 2
+      end
       it "returns an empty array when the commentable object does not exist (no threads)" do
-        get "/api/v1/does_not_exist/threads"
-        last_response.should be_ok
-        response = parse last_response.body
-        threads = response['collection']
+        threads = thread_result "does_not_exist"
         threads.length.should == 0
       end
 

spec/api/notifications_and_subscriptions_spec.rb

@@ -41,6 +41,18 @@ def thread_result(params)
           rs.length.should == 0 
         end
       end
+      it "filters by group_id" do
+        rs = thread_result course_id: DFLT_COURSE_ID, group_id: 42
+        rs.length.should == 5
+        @threads["t3"].group_id = 43
+        @threads["t3"].save!
+        rs = thread_result course_id: DFLT_COURSE_ID, group_id: 42
+        rs.length.should == 4
+        @threads["t3"].group_id = 42
+        @threads["t3"].save!
+        rs = thread_result course_id: DFLT_COURSE_ID, group_id: 42
+        rs.length.should == 5
+      end
       it "filters unread posts" do
         rs = thread_result course_id: DFLT_COURSE_ID
         rs.length.should == 5

spec/api/user_spec.rb

@@ -85,6 +85,21 @@ def thread_result(user_id, params)
         check_thread_result_json(@users["u100"], @threads["t0"], rs[1])
       end
 
+      it "filters by group_id" do
+        @threads["t1"].author = @users["u100"]
+        @threads["t1"].save!
+        rs = thread_result 100, course_id: DFLT_COURSE_ID, group_id: 42
+        rs.length.should == 2
+        @threads["t1"].group_id = 43
+        @threads["t1"].save!
+        rs = thread_result 100, course_id: DFLT_COURSE_ID, group_id: 42
+        rs.length.should == 1
+        @threads["t1"].group_id = 42
+        @threads["t1"].save!
+        rs = thread_result 100, course_id: DFLT_COURSE_ID, group_id: 42
+        rs.length.should == 2
+      end
+
       it "does not return threads in which the user has only participated anonymously" do
         @comments["t3 c4"].author = @users["u100"]
         @comments["t3 c4"].anonymous_to_peers = true