Merge pull request #208 from edx/robrap/TNL-5115

TNL-5115: Remove sort order from cs_comment_service

api/comment_threads.rb

@@ -13,7 +13,6 @@ get "#{APIPREFIX}/threads" do # retrieve threads by course
     value_to_boolean(params["unread"]),
     value_to_boolean(params["unanswered"]),
     params["sort_key"],
-    params["sort_order"],
     params["page"],
     params["per_page"]
   ).to_json

api/commentables.rb

@@ -18,7 +18,6 @@ get "#{APIPREFIX}/:commentable_id/threads" do |commentable_id|
     value_to_boolean(params["unread"]),
     value_to_boolean(params["unanswered"]),
     params["sort_key"],
-    params["sort_order"],
     params["page"],
     params["per_page"],
     params["context"] ? params["context"] : :course

api/notifications_and_subscriptions.rb

@@ -12,7 +12,6 @@ get "#{APIPREFIX}/users/:user_id/subscribed_threads" do |user_id|
     value_to_boolean(params["unread"]),
     value_to_boolean(params["unanswered"]),
     params["sort_key"],
-    params["sort_order"],
     params["page"],
     params["per_page"]
   ).to_json

api/search.rb

@@ -88,7 +88,6 @@ get "#{APIPREFIX}/search/threads" do
       value_to_boolean(local_params["unread"]),
       value_to_boolean(local_params["unanswered"]),
       local_params["sort_key"],
-      local_params["sort_order"],
       local_params["page"],
       local_params["per_page"],
       context

lib/helpers.rb

@@ -137,7 +137,6 @@ helpers do
     filter_unread,
     filter_unanswered,
     sort_key,
-    sort_order,
     page,
     per_page,
     context=:course
@@ -175,7 +174,7 @@ helpers do
       end
     end
 
-    sort_criteria = get_sort_criteria(sort_key, sort_order)
+    sort_criteria = get_sort_criteria(sort_key)
     if not sort_criteria
       {}
     else
@@ -240,7 +239,7 @@ helpers do
 
   # Given query params, return sort criteria appropriate for passing to the
   # order_by function of a Mongoid query. Returns nil if params are not valid.
-  def get_sort_criteria(sort_key, sort_order)
+  def get_sort_criteria(sort_key)
     sort_key_mapper = {
       "date" => :created_at,
       "activity" => :last_activity_at,
@@ -248,16 +247,11 @@ helpers do
       "comments" => :comment_count,
     }
 
-    sort_order_mapper = {
-      "desc" => :desc,
-      "asc" => :asc,
-    }
-
     sort_key = sort_key_mapper[params["sort_key"] || "date"]
-    sort_order = sort_order_mapper[params["sort_order"] || "desc"]
 
-    if sort_key && sort_order
-      sort_criteria = [[:pinned, :desc], [sort_key, sort_order]]
+    if sort_key
+      # only sort order of :desc is supported.  support for :asc would require new indices.
+      sort_criteria = [[:pinned, :desc], [sort_key, :desc]]
       if ![:created_at, :last_activity_at].include? sort_key
         sort_criteria << [:created_at, :desc]
       end

lib/tasks/benchmark.rake

@@ -76,19 +76,18 @@ namespace :benchmark do
 
     Benchmark.bm(31) do |x|
       sort_keys = %w[date activity votes comments]
-      sort_order = "desc"
 
       x.report("querying threads in a course") do
         
         (1..COURSE_THREAD_QUERY).each do |seed|
-          query_params = { course_id: "1", sort_key: sort_keys[seed % 4], sort_order: sort_order, page: seed % 5 + 1, per_page: 5 }
+          query_params = { course_id: "1", sort_key: sort_keys[seed % 4], page: seed % 5 + 1, per_page: 5 }
           RestClient.get "#{PREFIX}/threads", params: query_params
         end
       end
       x.report("searching threads in a course") do
         
         (1..COURSE_THREAD_QUERY).each do |seed|
-          query_params = { course_id: "1", text: "token#{seed % 10} token#{(seed * seed) % 10}", sort_key: sort_keys[seed % 4], sort_order: sort_order, page: seed % 5 + 1, per_page: 5 }
+          query_params = { course_id: "1", text: "token#{seed % 10} token#{(seed * seed) % 10}", sort_key: sort_keys[seed % 4], page: seed % 5 + 1, per_page: 5 }
           RestClient.get "#{PREFIX}/search/threads", params: query_params
         end
       end

lib/tasks/deep_search.rake

@@ -50,7 +50,6 @@ namespace :deep_search do
     end
 
     sort_keys = %w[date activity votes comments]
-    sort_order = "desc"
 
     #set the sinatra env to test to avoid 401'ing
     set :environment, :test
@@ -58,7 +57,7 @@ namespace :deep_search do
     start_time = Time.now
     puts "Starting test at #{start_time}"
     1000.times do |i|
-      query_params = { course_id: "1", sort_key: sort_keys.sample, sort_order: sort_order, page: 1, per_page: 5, text: bodies.sample }
+      query_params = { course_id: "1", sort_key: sort_keys.sample, page: 1, per_page: 5, text: bodies.sample }
       RestClient.get "#{PREFIX}/threads", params: query_params
     end
     end_time = Time.now

spec/api/comment_thread_spec.rb

@@ -22,10 +22,10 @@ describe "app" do
             t.course_id = "abc"
             t.save!
           end
-          rs = thread_result course_id: "abc", sort_order: "asc"
+          rs = thread_result course_id: "abc"
           rs.length.should == 2
           rs.each_with_index { |res, i|
-            check_thread_result_json(nil, @threads["t#{i+1}"], res)
+            check_thread_result_json(nil, @threads["t#{2-i}"], res)
             res["course_id"].should == "abc"
           }
         end
@@ -36,10 +36,10 @@ describe "app" do
           end
           @threads["t2"].context = :standalone
           @threads["t2"].save!
-          rs = thread_result course_id: "abc", sort_order: "asc"
+          rs = thread_result course_id: "abc"
           rs.length.should == 2
-          check_thread_result_json(nil, @threads["t1"], rs[0])
-          check_thread_result_json(nil, @threads["t3"], rs[1])
+          check_thread_result_json(nil, @threads["t3"], rs[0])
+          check_thread_result_json(nil, @threads["t1"], rs[1])
         end
         it "returns only threads where course id and commentable id match" do
           @threads["t1"].course_id = "course1"
@@ -77,7 +77,7 @@ describe "app" do
           @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 = thread_result course_id: "omg", group_ids: "100,101"
           rs.length.should == 2
         end
         it "returns only threads where course id and group id match or group id is nil" do
@@ -88,10 +88,10 @@ describe "app" do
           @threads["t2"].save!
           @threads["t3"].group_id = 100
           @threads["t3"].save!
-          rs = thread_result course_id: "omg", group_id: 100, sort_order: "asc"
+          rs = thread_result course_id: "omg", group_id: 100
           rs.length.should == 2
           rs.each_with_index { |res, i|
-            check_thread_result_json(nil, @threads["t#{i+1}"], res)
+            check_thread_result_json(nil, @threads["t#{2-i}"], res)
             res["course_id"].should == "omg"
           }
         end
@@ -184,52 +184,52 @@ describe "app" do
             t.course_id = "abc"
             t.save!
           end
-          rs = thread_result course_id: "abc", user_id: "123", sort_order: "asc"
+          rs = thread_result course_id: "abc", user_id: "123"
           rs.length.should == 2
           rs.each_with_index { |result, i|
-            check_thread_result_json(user, @threads["t#{i+1}"], result)
+            check_thread_result_json(user, @threads["t#{2-i}"], result)
             result["course_id"].should == "abc"
             result["unread_comments_count"].should == 5
             result["read"].should == false
           }
 
           user.mark_as_read(@threads["t1"])
-          rs = thread_result course_id: "abc", user_id: "123", sort_order: "asc"
+          rs = thread_result course_id: "abc", user_id: "123"
           rs.length.should == 2
           rs.each_with_index { |result, i|
-            check_thread_result_json(user, @threads["t#{i+1}"], result)
+            check_thread_result_json(user, @threads["t#{2-i}"], result)
           }
-          rs[0]["read"].should == true
-          rs[0]["unread_comments_count"].should == 0
-          rs[1]["read"].should == false
-          rs[1]["unread_comments_count"].should == 5
+          rs[1]["read"].should == true
+          rs[1]["unread_comments_count"].should == 0
+          rs[0]["read"].should == false
+          rs[0]["unread_comments_count"].should == 5
 
           @threads["t1"].updated_at += 1 # 1 second later
           @threads["t1"].save!
-          rs = thread_result course_id: "abc", user_id: "123", sort_order: "asc"
+          rs = thread_result course_id: "abc", user_id: "123"
           rs.length.should == 2
           rs.each_with_index { |result, i|
-            check_thread_result_json(user, @threads["t#{i+1}"], result)
+            check_thread_result_json(user, @threads["t#{2-i}"], result)
           }
-          rs[0]["read"].should == true
-          rs[0]["unread_comments_count"].should == 0
-          rs[1]["read"].should == false
-          rs[1]["unread_comments_count"].should == 5
+          rs[1]["read"].should == true
+          rs[1]["unread_comments_count"].should == 0
+          rs[0]["read"].should == false
+          rs[0]["unread_comments_count"].should == 5
 
           # author's own posts should not count as unread
           make_comment(user, @threads["t1"], "my two cents")
-          rs = thread_result course_id: "abc", user_id: "123", sort_order: "asc"
-          rs[0]["unread_comments_count"].should == 0
+          rs = thread_result course_id: "abc", user_id: "123"
+          rs[1]["unread_comments_count"].should == 0
 
           # other's posts do, though
           make_comment(@threads["t1"].author, @threads["t1"], "the last word")
-          rs = thread_result course_id: "abc", user_id: "123", sort_order: "asc"
-          rs[0]["unread_comments_count"].should == 1
+          rs = thread_result course_id: "abc", user_id: "123"
+          rs[1]["unread_comments_count"].should == 1
         end
 
         context "sorting" do
-          def thread_result_order (sort_key, sort_order)
-            results = thread_result course_id: DFLT_COURSE_ID, sort_key: sort_key, sort_order: sort_order
+          def thread_result_order (sort_key)
+            results = thread_result course_id: DFLT_COURSE_ID, sort_key: sort_key
             results.length.should == 10
             results.map { |t| t["title"] }
           end
@@ -248,13 +248,8 @@ describe "app" do
             ary
           end
 
-          it "sorts using create date / ascending" do
-            actual_order = thread_result_order("date", "asc")
-            expected_order = @default_order.reverse
-            actual_order.should == expected_order
-          end
           it "sorts using create date / descending" do
-            actual_order = thread_result_order("date", "desc")
+            actual_order = thread_result_order("date")
             expected_order = @default_order
             actual_order.should == expected_order
           end
@@ -262,141 +257,92 @@ describe "app" do
             t5 = @threads["t5"]
             t5.update(body: "changed!")
             t5.save!
-            actual_order = thread_result_order("activity", "desc")
+            actual_order = thread_result_order("activity")
             expected_order = @default_order
             actual_order.should == expected_order
           end
-          it "sort unchanged using last activity / ascending when thread is updated" do
-            t5 = @threads["t5"]
-            t5.update(body: "changed!")
-            t5.save!
-            actual_order = thread_result_order("activity", "asc")
-            expected_order = @default_order.reverse
-            actual_order.should == expected_order
-          end
           it "sort unchanged using last activity / descending when comment is updated" do
             t5c = @threads["t5"].comments.first
             t5c.update(body: "changed!")
             t5c.save!
-            actual_order = thread_result_order("activity", "desc")
+            actual_order = thread_result_order("activity")
             expected_order = @default_order
             actual_order.should == expected_order
           end
-          it "sort unchanged using last activity / ascending when comment is updated" do
-            t5c = @threads["t5"].comments.first
-            t5c.update(body: "changed!")
-            t5c.save!
-            actual_order = thread_result_order("activity", "asc")
-            expected_order = @default_order.reverse
-            actual_order.should == expected_order
-          end
           it "sorts using last activity / descending when response is created" do
             t5 = @threads["t5"]
             comment = t5.comments.new(body: "this problem is so easy", course_id: "1")
             comment.author = User.first
             comment.save!
 
-            actual_order = thread_result_order("activity", "desc")
+            actual_order = thread_result_order("activity")
             expected_order = move_to_front(@default_order, "t5")
             actual_order.should == expected_order
           end
-          it "sorts using last activity / ascending when response is created" do
-            t5 = @threads["t5"]
-            comment = t5.comments.new(body: "this problem is so easy", course_id: "1")
-            comment.author = User.first
-            comment.save!
-
-            actual_order = thread_result_order("activity", "asc")
-            expected_order = move_to_end(@default_order.reverse, "t5")
-            actual_order.should == expected_order
-          end
           it "sorts using vote count / descending" do
             user = User.all.first
             t5 = @threads["t5"]
             user.vote(t5, :up)
             t5.save!
-            actual_order = thread_result_order("votes", "desc")
+            actual_order = thread_result_order("votes")
             expected_order = move_to_front(@default_order, "t5")
             actual_order.should == expected_order
           end
-          it "sorts using vote count / ascending" do
-            user = User.all.first
-            t5 = @threads["t5"]
-            user.vote(t5, :up)
-            t5.save!
-            actual_order = thread_result_order("votes", "asc")
-            expected_order = move_to_end(@default_order, "t5")
-            actual_order.should == expected_order
-          end
           it "sorts using comment count / descending" do
             make_comment(@threads["t5"].author, @threads["t5"], "extra comment")
-            actual_order = thread_result_order("comments", "desc")
+            actual_order = thread_result_order("comments")
             expected_order = move_to_front(@default_order, "t5")
             actual_order.should == expected_order
           end
-          it "sorts using comment count / ascending" do
-            make_comment(@threads["t5"].author, @threads["t5"], "extra comment")
-            actual_order = thread_result_order("comments", "asc")
-            expected_order = move_to_end(@default_order, "t5")
-            actual_order.should == expected_order
-          end
           it "sorts pinned items first" do
             make_comment(@threads["t5"].author, @threads["t5"], "extra comment")
             @threads["t7"].pinned = true
             @threads["t7"].save!
 
-            actual_order = thread_result_order("comments", "asc")
-            expected_order = move_to_front(move_to_end(@default_order, "t5"), "t7")
-            actual_order.should == expected_order
-
-            actual_order = thread_result_order("comments", "desc")
-            expected_order = move_to_front(move_to_front(@default_order, "t5"), "t7")
+            actual_order = thread_result_order("comments")
+            expected_order = move_to_front(@default_order, "t7", "t5")
             actual_order.should == expected_order
 
             @threads["t8"].pinned = true
             @threads["t8"].save!
 
-            actual_order = thread_result_order("comments", "asc")
-            expected_order = move_to_front(move_to_end(@default_order, "t5"), "t8", "t7")
+            actual_order = thread_result_order("comments")
+            expected_order = move_to_front(@default_order, "t8", "t7", "t5")
             actual_order.should == expected_order
 
-            actual_order = thread_result_order("date", "desc")
+            actual_order = thread_result_order("date")
             expected_order = move_to_front(@default_order, "t8", "t7")
             actual_order.should == expected_order
-
-            actual_order = thread_result_order("date", "asc")
-            expected_order = move_to_front(@default_order.reverse, "t7", "t8")
-            actual_order.should == expected_order
           end
 
           context "pagination" do
-            def thread_result_page (sort_key, sort_order, page, per_page, course_id=DFLT_COURSE_ID, user_id=nil, unread=false)
-              get "/api/v1/threads", course_id: course_id, sort_key: sort_key, sort_order: sort_order, page: page, per_page: per_page, user_id: user_id, unread: unread
+            def thread_result_page (sort_key, page, per_page, course_id=DFLT_COURSE_ID, user_id=nil, unread=false)
+              get "/api/v1/threads", course_id: course_id, sort_key: sort_key, page: page, per_page: per_page, user_id: user_id, unread: unread
               last_response.should be_ok
               parse(last_response.body)
             end
             it "returns single page with no threads in a course" do
-              result = thread_result_page("date", "desc", 1, 20, "99")
+              result = thread_result_page("date", 1, 20, "99")
               result["collection"].length.should == 0
               result["thread_count"].should == 0
               result["num_pages"].should == 1
               result["page"].should == 1
             end
             it "returns single page" do
-              result = thread_result_page("date", "desc", 1, 20)
+              result = thread_result_page("date", 1, 20)
               result["collection"].length.should == 10
               result["thread_count"].should == 10
               result["num_pages"].should == 1
               result["page"].should == 1
             end
             it "returns multiple pages" do
-              result = thread_result_page("date", "desc", 1, 5)
+              result = thread_result_page("date", 1, 5)
               result["collection"].length.should == 5
               result["thread_count"].should == 10
               result["num_pages"].should == 2
               result["page"].should == 1
 
-              result = thread_result_page("date", "desc", 2, 5)
+              result = thread_result_page("date", 2, 5)
               result["collection"].length.should == 5
               result["thread_count"].should == 10
               result["num_pages"].should == 2
@@ -404,7 +350,7 @@ describe "app" do
             end
             it "returns page exceeding available pages with no results" do
               #TODO: Review whether we can switch pagination endpoint to raise an exception; rather than an empty page
-              result = thread_result_page("date", "desc", 3, 5)
+              result = thread_result_page("date", 3, 5)
               result["collection"].length.should == 0
               result["thread_count"].should == 10
               result["num_pages"].should == 2
@@ -412,7 +358,7 @@ describe "app" do
             end
 
             def test_paged_order (sort_spec, expected_order, filter_spec=[], user_id=nil)
-              # sort spec is a hash with keys: sort_key, sort_dir, per_page
+              # sort spec is a hash with keys: sort_key, per_page
               # filter spec is an array of filters to set, e.g. "unread", "flagged"
               # expected order is an array of the expected titles of returned threads, in the expected order
               actual_order = []
@@ -422,7 +368,6 @@ describe "app" do
                 page = i + 1
                 result = thread_result_page(
                     sort_spec['sort_key'],
-                    sort_spec['sort_dir'],
                     page,
                     per_page,
                     DFLT_COURSE_ID,
@@ -446,20 +391,20 @@ describe "app" do
               make_comment(@threads["t5"].author, @threads["t5"], "extra comment")
               @threads["t7"].pinned = true
               @threads["t7"].save!
-              expected_order = move_to_front(move_to_end(@default_order, "t5"), "t7")
-              test_paged_order({'sort_key' => 'comments', 'sort_dir' => 'asc', 'per_page' => 3}, expected_order)
+              expected_order = move_to_front(@default_order, "t7", "t5")
+              test_paged_order({'sort_key' => 'comments', 'per_page' => 3}, expected_order)
             end
 
-            it "orders correctly acrosss pages with unread filter" do
+            it "orders correctly across pages with unread filter" do
               user = create_test_user(Random.new)
               user.mark_as_read(@threads["t0"])
               user.mark_as_read(@threads["t9"])
               make_comment(@threads["t5"].author, @threads["t5"], "extra comment")
               @threads["t7"].pinned = true
               @threads["t7"].save!
-              expected_order = move_to_front(move_to_end(@default_order[1..8], "t5"), "t7")
+              expected_order = move_to_front(@default_order[1..8], "t7", "t5")
               test_paged_order(
-                  {'sort_key' => 'comments', 'sort_dir' => 'asc', 'per_page' => 3},
+                  {'sort_key' => 'comments', 'per_page' => 3},
                   expected_order,
                   ["unread"],
                   user.id

spec/api/search_spec.rb

@@ -21,18 +21,13 @@ describe "app" do
         result.should == {}
       end
 
-      it "returns an empty reuslt if text parameter is missing" do
+      it "returns an empty result if text parameter is missing" do
         get "/api/v1/search/threads", course_id: course_id
         assert_empty_response
       end
 
-      it "returns an empty reuslt if sort key is invalid" do
-        get "/api/v1/search/threads", course_id: course_id, text: "foobar", sort_key: "invalid", sort_order: "desc"
-        assert_empty_response
-      end
-
-      it "returns an empty reuslt if sort order is invalid" do
-        get "/api/v1/search/threads", course_id: course_id, text: "foobar", sort_key: "date", sort_order: "invalid"
+      it "returns an empty result if sort key is invalid" do
+        get "/api/v1/search/threads", course_id: course_id, text: "foobar", sort_key: "invalid"
         assert_empty_response
       end
 
@@ -159,8 +154,8 @@ describe "app" do
           threads
         end
 
-        def check_sort(sort_key, sort_order, expected_thread_indexes)
-          get "/api/v1/search/threads", text: "text", course_id: course_id, sort_key: sort_key, sort_order: sort_order
+        def check_sort(sort_key, expected_thread_indexes)
+          get "/api/v1/search/threads", text: "text", course_id: course_id, sort_key: sort_key
           last_response.should be_ok
           result = parse(last_response.body)
           actual_ids = get_result_ids(result)
@@ -169,29 +164,23 @@ describe "app" do
         end
 
         it "by date" do
-          asc_order = [0, 1, 2, 3, 4, 5]
-          check_sort("date", "asc", asc_order)
-          check_sort("date", "desc", asc_order.reverse)
+          check_sort("date", [5, 4, 3, 2, 1, 0])
         end
 
         it "by activity" do
-          asc_order = [0, 1, 2, 3, 4, 5]
-          check_sort("activity", "asc", asc_order)
-          check_sort("activity", "desc", asc_order.reverse)
+          check_sort("activity", [5, 4, 3, 2, 1, 0])
         end
 
         it "by votes" do
-          check_sort("votes", "asc", [5, 4, 3, 0, 2, 1])
-          check_sort("votes", "desc", [2, 1, 5, 4, 3, 0])
+          check_sort("votes", [2, 1, 5, 4, 3, 0])
         end
 
         it "by comments" do
-          check_sort("comments", "asc", [5, 4, 2, 0, 3, 1])
-          check_sort("comments", "desc", [3, 1, 5, 4, 2, 0])
+          check_sort("comments", [3, 1, 5, 4, 2, 0])
         end
 
         it "by default" do
-          check_sort(nil, nil, [5, 4, 3, 2, 1, 0])
+          check_sort(nil, [5, 4, 3, 2, 1, 0])
         end
       end