Sync to master

api/comment_threads.rb

@@ -62,7 +62,9 @@
   unless (resp_limit <= size_limit)
     error 400, [t(:param_exceeds_limit, :param => resp_limit, :limit => size_limit)].to_json
   end
-  presenter.to_hash(bool_with_responses, resp_skip, resp_limit, bool_recursive, bool_flagged_comments).to_json
+  presenter.to_hash(
+    bool_with_responses, resp_skip, resp_limit, bool_recursive, bool_flagged_comments, bool_reverse_order
+  ).to_json
 end
 
 put "#{APIPREFIX}/threads/:thread_id" do |thread_id|

api/users.rb

@@ -52,17 +52,28 @@
   if usernames.empty?
     paginated_stats = User.collection
                           .aggregate([
+                                       # Match only users that have stats for this course
                                        { '$match' => { "course_stats.course_id" => course_id } },
+                                       # Get only the username and course stats since that's all we need
                                        { '$project' => { 'username' => 1, 'course_stats' => 1 } },
+                                       # Get rid of other course entries by expanding the course stats
+                                       # and filtering out other courses
                                        { '$unwind' => '$course_stats' },
                                        { '$match' => { "course_stats.course_id" => course_id } },
                                        { '$sort' => sort_criterion },
-                                       { '$limit' => per_page },
-                                       { '$skip' => (page - 1) * per_page },
-                                     ])
-    total_count = paginated_stats.count
+                                       # Split the query and get a total count in one facet and
+                                       # perform the pagination iin the other
+                                       { '$facet' => {
+                                         'pagination' => [{"$count" => "total_count"}],
+                                         'data' => [
+                                           { '$skip' => (page - 1) * per_page },
+                                           { '$limit' => per_page },
+                                         ]
+                                       }}
+                                     ]).to_a[0]
+    total_count = paginated_stats["pagination"][0]["total_count"]
     num_pages = [1, (total_count / per_page.to_f).ceil].max
-    data = paginated_stats.to_a.map do |user_stats|
+    data = paginated_stats["data"].map do |user_stats|
       {
         :username => user_stats["username"]
       }.merge(user_stats["course_stats"].except(*exclude_from_stats))
@@ -146,7 +157,7 @@
 
   threads_data = handle_threads_query(
     threads,
-    params["user_id"],
+    user_id,
     params["course_id"],
     get_group_ids_from_params(params),
     params["author_id"],
@@ -162,7 +173,7 @@
     raw_query: raw_query
   )
 
-  if sort_key == 'user_activity'
+  if raw_query
     num_pages = [1, (threads_data.count / per_page.to_f).ceil].max
     page = [num_pages, [1, page].max].min
 

lib/helpers.rb

@@ -143,6 +143,10 @@ def bool_flagged_comments
     value_to_boolean params["flagged_comments"]
   end
 
+  def bool_reverse_order
+    value_to_boolean params["reverse_order"]
+  end
+
   def handle_paged_threads_query(paged_comment_threads)
 
   end
@@ -239,7 +243,9 @@ def handle_threads_query(
         comment_threads.batch_size(CommentService.config["manual_pagination_batch_size"].to_i).each do |thread|
          thread_key = thread._id.to_s
           if !read_dates.has_key?(thread_key) || read_dates[thread_key] < thread.last_activity_at
-            if skipped >= to_skip or raw_query
+            if raw_query
+              threads << thread
+            elsif skipped >= to_skip
               if threads.length == per_page
                 has_more = true
                 break

presenters/thread.rb

@@ -29,7 +29,14 @@ def initialize(thread, user, is_read, unread_count, is_endorsed, abuse_flagged_c
     @abuse_flagged_count = abuse_flagged_count
   end
 
-  def to_hash(with_responses=false, resp_skip=0, resp_limit=nil, recursive=true, flagged_comments=false)
+  def to_hash(
+    with_responses=false,
+    resp_skip=0,
+    resp_limit=nil,
+    recursive=true,
+    flagged_comments=false,
+    reverse_order=false
+  )
     raise ArgumentError unless resp_skip >= 0
     raise ArgumentError unless resp_limit.nil? or resp_limit >= 1
     h = @thread.to_hash
@@ -39,12 +46,13 @@ def to_hash(with_responses=false, resp_skip=0, resp_limit=nil, recursive=true, f
     unless @abuse_flagged_count.nil?
       h["abuse_flagged_count"] = @abuse_flagged_count
     end
+    sorting_key_order = reverse_order ? -1 : 1
     if with_responses
       if @thread.thread_type.discussion? && resp_skip == 0 && resp_limit.nil?
         if recursive
-          content = Comment.where(comment_thread_id: @thread._id).order_by({"sk" => 1})
+          content = Comment.where(comment_thread_id: @thread._id).order_by({"sk" => sorting_key_order})
         else
-          content = Comment.where(comment_thread_id: @thread._id, "parent_ids" => []).order_by({"sk" => 1})
+          content = Comment.where(comment_thread_id: @thread._id, "parent_ids" => []).order_by({"sk" => sorting_key_order})
         end
         if flagged_comments
           content = content.where(:abuse_flaggers.nin => [nil, []])
@@ -60,20 +68,35 @@ def to_hash(with_responses=false, resp_skip=0, resp_limit=nil, recursive=true, f
         when "question"
           endorsed_responses = responses.where(endorsed: true)
           non_endorsed_responses = responses.where(endorsed: false)
-          endorsed_response_info = get_paged_merged_responses(@thread._id, endorsed_responses, 0, nil, recursive)
+          endorsed_response_info = get_paged_merged_responses(
+            @thread._id,
+            endorsed_responses,
+            0,
+            nil,
+            recursive,
+            sorting_key_order
+          )
           non_endorsed_response_info = get_paged_merged_responses(
             @thread._id,
             non_endorsed_responses,
             resp_skip,
             resp_limit,
-            recursive
+            recursive,
+            sorting_key_order
           )
           h["endorsed_responses"] = endorsed_response_info["responses"]
           h["non_endorsed_responses"] = non_endorsed_response_info["responses"]
           h["non_endorsed_resp_total"] = non_endorsed_response_info["response_count"]
           h["resp_total"] = non_endorsed_response_info["response_count"] + endorsed_response_info["response_count"]
         when "discussion"
-          response_info = get_paged_merged_responses(@thread._id, responses, resp_skip, resp_limit, recursive)
+          response_info = get_paged_merged_responses(
+            @thread._id,
+            responses,
+            resp_skip,
+            resp_limit,
+            recursive,
+            sorting_key_order
+          )
           h["children"] = response_info["responses"]
           h["resp_total"] = response_info["response_count"]
         end
@@ -91,16 +114,16 @@ def to_hash(with_responses=false, resp_skip=0, resp_limit=nil, recursive=true, f
   #     children, if recursive is true)
   #   response_count
   #     The total number of responses
-  def get_paged_merged_responses(thread_id, responses, skip, limit, recursive=false)
-    response_ids = responses.only(:_id).sort({"sk" => 1}).to_a.map{|doc| doc["_id"]}
+  def get_paged_merged_responses(thread_id, responses, skip, limit, recursive=false, sorting_key_order)
+    response_ids = responses.only(:_id).sort({"sk" => sorting_key_order}).to_a.map{|doc| doc["_id"]}
     paged_response_ids = limit.nil? ? response_ids.drop(skip) : response_ids.drop(skip).take(limit)
     if recursive
       content = Comment.where(comment_thread_id: thread_id).
         or({:parent_id => {"$in" => paged_response_ids}}, {:id => {"$in" => paged_response_ids}}).
-        sort({"sk" => 1})
+        sort({"sk" => sorting_key_order})
     else
       content = Comment.where(comment_thread_id: thread_id, "parent_ids" => []).
-        where({:id => {"$in" => paged_response_ids}}).sort({"sk" => 1})
+        where({:id => {"$in" => paged_response_ids}}).sort({"sk" => sorting_key_order})
     end
     {"responses" => merge_response_content(content), "response_count" => response_ids.length}
   end
@@ -111,25 +134,44 @@ def get_paged_merged_responses(thread_id, responses, skip, limit, recursive=fals
   def merge_response_content(content)
     top_level = []
     ancestry = []
+    orphans = []
     content.each do |item|
       item_hash = item.to_hash.merge!("children" => [])
       if item.parent_id.nil?
         top_level << item_hash
         ancestry = [item_hash]
+        # When the content is reversed, we collect orphan items
+        # until reach their parent. Here we iterate through
+        # orphans and assign as children to the top item.
+        unless orphans.empty?
+          orphans.each do |orphan|
+            if item.id == orphan["parent_id"]
+              item_hash["children"] << orphan
+            end
+          end
+          orphans = []
+        end
       else
+        # "ancestry" can be empty only when the order is reversed.
+        if ancestry.empty?
+          ancestry << item_hash
+          orphans << item_hash
+          next
+        end
+
         while ancestry.length > 0 do
           if item.parent_id == ancestry.last["id"]
             ancestry.last["children"] << item_hash
             ancestry << item_hash
             break
+          elsif ancestry.length == 1
+            # "ancestry" here can equal to 1 only when the order is reversed.
+            orphans << item_hash
+            ancestry.pop
           else
             ancestry.pop
-            next
           end
         end
-        if ancestry.empty? # invalid parent; ignore item
-          next
-        end
       end
     end
     top_level

spec/api/user_spec.rb

@@ -236,55 +236,76 @@ def thread_result(user_id, params)
         end
       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
-        expect(rs.length).to eq(2)
-        @threads["t1"].group_id = 43
-        @threads["t1"].save!
-        rs = thread_result 100, course_id: DFLT_COURSE_ID, group_id: 42
-        expect(rs.length).to eq(1)
-        @threads["t1"].group_id = 42
-        @threads["t1"].save!
-        rs = thread_result 100, course_id: DFLT_COURSE_ID, group_id: 42
-        expect(rs.length).to eq(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"
-        expect(rs.length).to eq(2)
-        @threads["t1"].group_id = 43
-        @threads["t1"].save!
-        rs = thread_result 100, course_id: DFLT_COURSE_ID, group_ids: "42"
-        expect(rs.length).to eq(1)
-        rs = thread_result 100, course_id: DFLT_COURSE_ID, group_ids: "42,43"
-        expect(rs.length).to eq(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
-        @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"
-        expect(rs.length).to eq(1)
-        check_thread_result_json(@users["u100"], @threads["t0"], rs.first)
-      end
-
-      it "only returns threads from the specified course" do
-        @threads.each do |k, v|
-          v.author = @users["u100"]
-          v.save!
-        end
-        @threads["t9"].course_id = "zzz"
-        @threads["t9"].save!
-        rs = thread_result 100, course_id: "xyz"
-        expect(rs.length).to eq(9)
+      context 'filtering' do
+
+        it "filters by unread", :new => true do
+          # All 10 threads are are assigned to the requesting user
+          (1...10).each { |tid|
+            @threads["t#{tid}"].author = @users["u100"]
+            @threads["t#{tid}"].save!
+          }
+          # However one of them is marked as read
+          @users["u100"].mark_as_read(@threads["t3"])
+          # The results should include 9 entries, and exclude t3 which is marked as read.
+          rs = thread_result 100, course_id: DFLT_COURSE_ID, unread: true, per_page: 5
+          expect(rs.length).to eq(5)
+          expect(rs).not_to include have_attributes(:title => "t3")
+          rs2 = thread_result 100, course_id: DFLT_COURSE_ID, unread: true, per_page: 5, page: 2
+          expect(rs2.length).to eq(4)
+          expect(rs2).not_to include have_attributes(:title => "t3")
+        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
+          expect(rs.length).to eq(2)
+          @threads["t1"].group_id = 43
+          @threads["t1"].save!
+          rs = thread_result 100, course_id: DFLT_COURSE_ID, group_id: 42
+          expect(rs.length).to eq(1)
+          @threads["t1"].group_id = 42
+          @threads["t1"].save!
+          rs = thread_result 100, course_id: DFLT_COURSE_ID, group_id: 42
+          expect(rs.length).to eq(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"
+          expect(rs.length).to eq(2)
+          @threads["t1"].group_id = 43
+          @threads["t1"].save!
+          rs = thread_result 100, course_id: DFLT_COURSE_ID, group_ids: "42"
+          expect(rs.length).to eq(1)
+          rs = thread_result 100, course_id: DFLT_COURSE_ID, group_ids: "42,43"
+          expect(rs.length).to eq(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
+          @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"
+          expect(rs.length).to eq(1)
+          check_thread_result_json(@users["u100"], @threads["t0"], rs.first)
+        end
+
+        it "only returns threads from the specified course" do
+          @threads.each do |k, v|
+            v.author = @users["u100"]
+            v.save!
+          end
+          @threads["t9"].course_id = "zzz"
+          @threads["t9"].save!
+          rs = thread_result 100, course_id: "xyz"
+          expect(rs.length).to eq(9)
+        end
+
       end
 
       context "sorting" do
@@ -516,6 +537,7 @@ def build_structure_and_response(course_id, authors, build_initial_stats = true,
       end
 
       it "returns user's stats with recency sort" do
+        build_structure_and_response course_id, authors
         get "/api/v1/users/#{course_id}/stats", sort_key: "recency", with_timestamps: true
         expect(last_response.status).to eq(200)
         res = parse(last_response.body)

spec/presenters/thread_spec.rb

@@ -158,6 +158,28 @@ def random_flag_abuses!(comment)
         end
       end
 
+      it "handles reversed_order and recursive" do
+        @threads_with_num_comments.each do |thread, num_comments|
+          is_endorsed = num_comments > 0 && endorse_responses
+          hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed, nil).to_hash(true, 0, default_resp_limit, true, false, true)
+          check_thread_result(@reader, thread, hash)
+          check_thread_response_paging(thread, hash, 0, default_resp_limit, false, false, true)
+        end
+      end
+
+      it "handles reversed_order and recursive with skip and limit" do
+        @threads_with_num_comments.each do |thread, num_comments|
+          is_endorsed = num_comments > 0 && endorse_responses
+          [1, 2, 3, 9, 10, 11, 1000].each do |limit|
+            [0, 1, 2, 9, 10, 11, 1000].each do |skip|
+              hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed, nil).to_hash(true, skip, limit, true, false, true)
+              check_thread_result(@reader, thread, hash)
+              check_thread_response_paging(thread, hash, skip, limit, false, false, true)
+            end
+          end
+        end
+      end
+
       it "fails with invalid arguments" do
         @threads_with_num_comments.each do |thread, num_comments|
           is_endorsed = num_comments > 0 && endorse_responses