Merge pull request #109 from aboudreault/query-multiple-group-ids

Add the ability to query multiple group ids

AUTHORS

@@ -18,3 +18,4 @@ Jim Abramson <jsa@edx.org>
 Greg Price <gprice@edx.org>
 Sarina Canelake <sarina@edx.org>
 Alexandre Dubus <alexandre.dubus@inria.fr>
+Alan Boudreault <alan@alanb.ca>

api/comment_threads.rb

@@ -9,7 +9,7 @@ get "#{APIPREFIX}/threads" do # retrieve threads by course
     threads,
     params["user_id"],
     params["course_id"],
-    params["group_id"],
+    get_group_ids_from_params(params),
     value_to_boolean(params["flagged"]),
     value_to_boolean(params["unread"]),
     value_to_boolean(params["unanswered"]),

api/commentables.rb

@@ -13,7 +13,7 @@ get "#{APIPREFIX}/:commentable_id/threads" do |commentable_id|
     threads,
     params["user_id"],
     params["course_id"],
-    params["group_id"],
+    get_group_ids_from_params(params),
     value_to_boolean(params["flagged"]),
     value_to_boolean(params["unread"]),
     value_to_boolean(params["unanswered"]),

api/notifications_and_subscriptions.rb

@@ -7,7 +7,7 @@ get "#{APIPREFIX}/users/:user_id/subscribed_threads" do |user_id|
     user.subscribed_threads.where({"course_id" => params[:course_id]}),
     params["user_id"],
     params["course_id"],
-    params["group_id"],
+    get_group_ids_from_params(params),
     value_to_boolean(params["flagged"]),
     value_to_boolean(params["unread"]),
     value_to_boolean(params["unanswered"]),

api/search.rb

@@ -2,6 +2,7 @@ require 'new_relic/agent/method_tracer'
 
 get "#{APIPREFIX}/search/threads" do
   local_params = params # Necessary for params to be available inside blocks
+  group_ids = get_group_ids_from_params(local_params)
   search_text = local_params["text"]
   if !search_text
     {}.to_json
@@ -22,12 +23,20 @@ get "#{APIPREFIX}/search/threads" do
               filter :term, :commentable_id => local_params["commentable_id"] if local_params["commentable_id"]
               filter :terms, :commentable_id => local_params["commentable_ids"].split(",") if local_params["commentable_ids"]
               filter :term, :course_id => local_params["course_id"] if local_params["course_id"]
-              if local_params["group_id"]
+
+              if not group_ids.empty?
+                if group_ids.length > 1
+                  group_id_criteria = {:terms => {:group_id => group_ids}}
+                else
+                  group_id_criteria = {:term => {:group_id => group_ids[0]}}
+                end
+
                 filter :or, [
                   {:not => {:exists => {:field => :group_id}}},
-                  {:term => {:group_id => local_params["group_id"]}}
+                  group_id_criteria
                 ]
               end
+
             end
           end
           sort do
@@ -71,7 +80,7 @@ get "#{APIPREFIX}/search/threads" do
       CommentThread.in({"_id" => thread_ids.to_a}),
       local_params["user_id"],
       local_params["course_id"],
-      local_params["group_id"],
+      group_ids,
       value_to_boolean(local_params["flagged"]),
       value_to_boolean(local_params["unread"]),
       value_to_boolean(local_params["unanswered"]),

api/users.rb

@@ -39,11 +39,9 @@ get "#{APIPREFIX}/users/:user_id/active_threads" do |user_id|
 
   threads = CommentThread.in({"_id" => active_thread_ids})
 
-  if params["group_id"]
-    threads = threads.any_of(
-      {"group_id" => params["group_id"].to_i},
-      {"group_id" => {"$exists" => false}}
-    )
+  group_ids = get_group_ids_from_params(params)
+  if not group_ids.empty?
+    threads = get_group_id_criteria(threads, group_ids)
   end
 
   num_pages = [1, (threads.count / per_page.to_f).ceil].max

lib/helpers.rb

@@ -122,7 +122,7 @@ helpers do
     comment_threads,
     user_id,
     course_id,
-    group_id,
+    group_ids,
     filter_flagged,
     filter_unread,
     filter_unanswered,
@@ -131,11 +131,9 @@ helpers do
     page,
     per_page
   )
-    if group_id
-      comment_threads = comment_threads.any_of(
-        {"group_id" => group_id.to_i},
-        {"group_id" => {"$exists" => false}}
-      )
+
+    if not group_ids.empty?
+      comment_threads = get_group_id_criteria(comment_threads, group_ids)
     end
 
     if filter_flagged
@@ -257,6 +255,33 @@ helpers do
     end
   end
 
+  def get_group_ids_from_params(params)
+    if params["group_id"] and params["group_ids"]
+      raise ArgumentError, t(:cannot_specify_group_id_and_group_ids)
+    end
+    group_ids = []
+    if params["group_id"]
+      group_ids << params["group_id"].to_i
+    elsif params["group_ids"]
+      group_ids.concat(params["group_ids"].split(",").map(&:to_i))
+    end
+    group_ids
+  end
+
+  def get_group_id_criteria(threads, group_ids)
+    if group_ids.length > 1
+      threads.any_of(
+        {"group_id" => {"$in" => group_ids}},
+        {"group_id" => {"$exists" => false}},
+      )
+    else
+      threads.any_of(
+        {"group_id" => group_ids[0]},
+        {"group_id" => {"$exists" => false}},
+      )
+    end
+  end
+
   def notifications_by_date_range_and_user_ids(start_date_time, end_date_time, user_ids)
     #given a date range and a user, find all of the notifiable content
     #key by thread id, and return notification messages for each user

locale/en-US.yml

@@ -8,3 +8,4 @@ en-US:
   blocked_content_with_body_hash: "blocked content with body hash %{hash}"
   param_must_be_a_non_negative_number: "%{param} must be a non-negative number"
   param_must_be_a_number_greater_than_zero: "%{param} must be a number greater than zero"
+  cannot_specify_group_id_and_group_ids: "Cannot specify both group_id and group_ids as filters."

spec/api/comment_thread_spec.rb

@@ -58,6 +58,16 @@ describe "app" do
           rs.length.should == 1
           check_thread_result_json(nil, @threads["t1"], rs.first)
         end
+        it "returns only threads where course id and group ids match" do
+          @threads["t1"].course_id = "omg"
+          @threads["t1"].group_id = 100
+          @threads["t1"].save!
+          @threads["t2"].course_id = "omg"
+          @threads["t2"].group_id = 101
+          @threads["t2"].save!
+          rs = thread_result course_id: "omg", group_ids: "100,101", sort_order: "asc"
+          rs.length.should == 2
+        end
         it "returns only threads where course id and group id match or group id is nil" do
           @threads["t1"].course_id = "omg"
           @threads["t1"].group_id = 100

spec/api/commentable_spec.rb

@@ -52,6 +52,21 @@ describe "app" do
         threads = thread_result "question_1", group_id: 42
         threads.length.should == 2
       end
+      it "filters by group_ids" do
+        group_thread = Commentable.find("question_1").comment_threads.first
+        group_thread.group_id = 42
+        group_thread.save!
+        threads = thread_result "question_1", group_ids: "42,43"
+        threads.length.should == 2
+        group_thread.group_id = 43
+        group_thread.save!
+        threads = thread_result "question_1", group_ids: "42,43"
+        threads.length.should == 2
+        group_thread.group_id = 44
+        group_thread.save
+        threads = thread_result "question_1", group_ids: "42,43"
+        threads.length.should == 1
+      end
       it "returns an empty array when the commentable object does not exist (no threads)" do
         threads = thread_result "does_not_exist"
         threads.length.should == 0

spec/api/notifications_and_subscriptions_spec.rb

@@ -53,6 +53,16 @@ describe "app" do
         rs = thread_result course_id: DFLT_COURSE_ID, group_id: 42
         rs.length.should == 5
       end
+      it "filters by group_ids" do
+        rs = thread_result course_id: DFLT_COURSE_ID, group_ids: "42"
+        rs.length.should == 5
+        @threads["t3"].group_id = 43
+        @threads["t3"].save!
+        rs = thread_result course_id: DFLT_COURSE_ID, group_ids: "42"
+        rs.length.should == 4
+        rs = thread_result course_id: DFLT_COURSE_ID, group_ids: "42,43"
+        rs.length.should == 5
+      end
       it "filters unread posts" do
         rs = thread_result course_id: DFLT_COURSE_ID
         rs.length.should == 5

spec/api/search_spec.rb

@@ -125,6 +125,12 @@ describe "app" do
           assert_response_contains((0..29).find_all {|i| i % 5 == 0 || i % 5 == 1})
         end
 
+        it "by group_ids" do
+          get "/api/v1/search/threads", text: "text", group_ids: "1,2"
+          expected_ids = (0..29).find_all {|i| i % 5 == 0 || i % 5 == 1 || i % 5 == 2}
+          assert_response_contains(expected_ids)
+        end
+
         it "by all filters combined" do
           get "/api/v1/search/threads", text: "text", course_id: "test/course/id0", commentable_id: "commentable0", group_id: "1"
           assert_response_contains([0, 6])

spec/api/user_spec.rb

@@ -100,6 +100,19 @@ describe "app" do
         rs.length.should == 2
       end
 
+      it "filters by group_ids" do
+        @threads["t1"].author = @users["u100"]
+        @threads["t1"].save!
+        rs = thread_result 100, course_id: DFLT_COURSE_ID, group_ids: "42"
+        rs.length.should == 2
+        @threads["t1"].group_id = 43
+        @threads["t1"].save!
+        rs = thread_result 100, course_id: DFLT_COURSE_ID, group_ids: "42"
+        rs.length.should == 1
+        rs = thread_result 100, course_id: DFLT_COURSE_ID, group_ids: "42,43"
+        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