diff --git a/api/comment_threads.rb b/api/comment_threads.rb index 433b23ba07f..7ee1a2615ee 100644 --- a/api/comment_threads.rb +++ b/api/comment_threads.rb @@ -13,7 +13,6 @@ value_to_boolean(params["unread"]), value_to_boolean(params["unanswered"]), params["sort_key"], - params["sort_order"], params["page"], params["per_page"] ).to_json diff --git a/api/commentables.rb b/api/commentables.rb index 9c34aa42baf..ce83982766a 100644 --- a/api/commentables.rb +++ b/api/commentables.rb @@ -18,7 +18,6 @@ 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 diff --git a/api/notifications_and_subscriptions.rb b/api/notifications_and_subscriptions.rb index 32db8eaaea4..55820afe996 100644 --- a/api/notifications_and_subscriptions.rb +++ b/api/notifications_and_subscriptions.rb @@ -12,7 +12,6 @@ value_to_boolean(params["unread"]), value_to_boolean(params["unanswered"]), params["sort_key"], - params["sort_order"], params["page"], params["per_page"] ).to_json diff --git a/api/search.rb b/api/search.rb index c66e06d9369..0c88aca2f2b 100644 --- a/api/search.rb +++ b/api/search.rb @@ -88,7 +88,6 @@ 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 diff --git a/lib/helpers.rb b/lib/helpers.rb index 384bf4b3477..471705c2d48 100644 --- a/lib/helpers.rb +++ b/lib/helpers.rb @@ -137,7 +137,6 @@ def handle_threads_query( filter_unread, filter_unanswered, sort_key, - sort_order, page, per_page, context=:course @@ -175,7 +174,7 @@ def handle_threads_query( 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 @@ def handle_threads_query( # 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 @@ def get_sort_criteria(sort_key, sort_order) "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 diff --git a/lib/tasks/benchmark.rake b/lib/tasks/benchmark.rake index 5709569a851..bc80fb446c1 100644 --- a/lib/tasks/benchmark.rake +++ b/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 diff --git a/lib/tasks/deep_search.rake b/lib/tasks/deep_search.rake index 9facff085eb..b0c57c9ae73 100644 --- a/lib/tasks/deep_search.rake +++ b/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 diff --git a/spec/api/comment_thread_spec.rb b/spec/api/comment_thread_spec.rb index 1510f1389e1..19ba3f317a4 100644 --- a/spec/api/comment_thread_spec.rb +++ b/spec/api/comment_thread_spec.rb @@ -22,10 +22,10 @@ def thread_result(params) 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 @@ def thread_result(params) 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 @@ def thread_result(params) @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 @@ def thread_result(params) @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 @@ def thread_result(params) 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 @@ def move_to_front(ary, *vals) 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 @@ def move_to_front(ary, *vals) 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 @@ def thread_result_page (sort_key, sort_order, page, per_page, course_id=DFLT_COU 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 @@ def thread_result_page (sort_key, sort_order, page, per_page, course_id=DFLT_COU 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 @@ def test_paged_order (sort_spec, expected_order, filter_spec=[], user_id=nil) 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 @@ def test_paged_order (sort_spec, expected_order, filter_spec=[], user_id=nil) 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 diff --git a/spec/api/search_spec.rb b/spec/api/search_spec.rb index 53c69b77a4c..3382209134c 100644 --- a/spec/api/search_spec.rb +++ b/spec/api/search_spec.rb @@ -21,18 +21,13 @@ def assert_empty_response 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 @@ def assert_response_contains(expected_thread_indexes) 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 @@ def check_sort(sort_key, sort_order, expected_thread_indexes) 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