Merge pull request #153 from edx/jia/MA-1815

jia/MA-1815 retrieve count for child comments

api/comment_threads.rb

@@ -78,6 +78,7 @@
   comment.anonymous_to_peers = bool_anonymous_to_peers || false
   comment.author = user
   comment.comment_thread = thread
+  comment.child_count = 0
   comment.save
   if comment.errors.any?
     error 400, comment.errors.full_messages.to_json

api/comments.rb

@@ -1,5 +1,8 @@
 get "#{APIPREFIX}/comments/:comment_id" do |comment_id|
-  comment.to_hash(recursive: bool_recursive).to_json
+  @comment = comment
+  comment_hash = @comment.to_hash(recursive: bool_recursive)
+  verify_or_fix_cached_comment_count(@comment, comment_hash)
+  comment_hash.to_json
 end
 
 put "#{APIPREFIX}/comments/:comment_id" do |comment_id|
@@ -31,16 +34,31 @@
   sub_comment.anonymous_to_peers = bool_anonymous_to_peers || false
   sub_comment.author = user
   sub_comment.comment_thread = comment.comment_thread
+  sub_comment.child_count = 0
   sub_comment.save
   if sub_comment.errors.any?
     error 400, sub_comment.errors.full_messages.to_json
   else
-    user.subscribe(comment.comment_thread) if bool_auto_subscribe
-    sub_comment.to_hash.to_json
+    comment.update_cached_child_count
+    if comment.errors.any?
+      error 400, comment.errors.full_messages.to_json
+    else
+      user.subscribe(comment.comment_thread) if bool_auto_subscribe
+      sub_comment.to_hash.to_json
+    end
   end
 end
 
 delete "#{APIPREFIX}/comments/:comment_id" do |comment_id|
+  parent_id = comment.parent_id
   comment.destroy
+  unless parent_id.nil?
+    begin
+      parent_comment = Comment.find(parent_id)
+      parent_comment.update_cached_child_count
+    rescue Mongoid::Errors::DocumentNotFound
+      pass
+    end
+  end
   comment.to_hash.to_json
 end

lib/helpers.rb

@@ -19,6 +19,16 @@ def comment
     @comment ||= Comment.find(params[:comment_id])
   end
 
+  def verify_or_fix_cached_comment_count(comment, comment_hash)
+    # if child count cached value gets stale; re-calculate and update it
+    unless comment_hash["children"].nil?
+      if comment_hash["child_count"] != comment_hash["children"].length
+        comment.update_cached_child_count
+        comment_hash["child_count"] = comment.get_cached_child_count
+      end
+    end
+  end
+
   def source
     @source ||= case params["source_type"]
     when "user"

models/comment.rb

@@ -20,6 +20,7 @@ class Comment < Content
   field :commentable_id, type: String
   field :at_position_list, type: Array, default: []
   field :sk, type: String, default: nil
+  field :child_count, type: Integer
 
   index({author_id: 1, course_id: 1})
   index({_type: 1, comment_thread_id: 1, author_id: 1, updated_at: 1})
@@ -91,20 +92,31 @@ def to_hash(params={})
       self.class.hash_tree(subtree_hash).first
     else
       as_document.slice(*%w[body course_id endorsed endorsement anonymous anonymous_to_peers created_at updated_at at_position_list])
-          .merge('id' => _id)
-          .merge('user_id' => author_id)
-          .merge('username' => author_username)
-          .merge('depth' => depth)
-          .merge('closed' => comment_thread.nil? ? false : comment_thread.closed)
-          .merge('thread_id' => comment_thread_id)
-          .merge('parent_id' => parent_ids[-1])
-          .merge('commentable_id' => comment_thread.nil? ? nil : comment_thread.commentable_id)
-          .merge('votes' => votes.slice(*%w[count up_count down_count point]))
-          .merge('abuse_flaggers' => abuse_flaggers)
-          .merge('type' => 'comment')
+          .merge("id" => _id)
+          .merge("user_id" => author_id)
+          .merge("username" => author_username)
+          .merge("depth" => depth)
+          .merge("closed" => comment_thread.nil? ? false : comment_thread.closed)
+          .merge("thread_id" => comment_thread_id)
+          .merge("parent_id" => parent_ids[-1])
+          .merge("commentable_id" => comment_thread.nil? ? nil : comment_thread.commentable_id)
+          .merge("votes" => votes.slice(*%w[count up_count down_count point]))
+          .merge("abuse_flaggers" => abuse_flaggers)
+          .merge("type" => "comment")
+          .merge("child_count" => get_cached_child_count)
     end
   end
 
+  def get_cached_child_count
+    update_cached_child_count if self.child_count.nil?
+    self.child_count
+  end
+
+  def update_cached_child_count
+    child_comments_count = Comment.where({"parent_id" => self._id}).count()
+    self.set(child_count: child_comments_count)
+  end
+
   def commentable_id
     #we need this to have a universal access point for the flag rake task
     if self.comment_thread_id

spec/api/comment_spec.rb

@@ -15,7 +15,7 @@
       last_response.content_type.should == 'application/json;charset=utf-8'
     end
 
-    it 'retrieve information of a single comment' do
+    it 'retrieves information of a single comment' do
       comment = thread.comments.first
       get "/api/v1/comments/#{comment.id}"
       last_response.should be_ok
@@ -27,9 +27,10 @@
       retrieved['votes']['point'].should == comment.votes_point
       retrieved['depth'].should == comment.depth
       retrieved['parent_id'].should == comment.parent_ids.map(&:to_s)[-1]
+      retrieved["child_count"].should == comment.children.length
     end
 
-    it 'retrieve information of a single comment with its sub comments' do
+    it 'retrieves information of a single comment with its sub comments' do
       comment = thread.comments.first
       get "/api/v1/comments/#{comment.id}", recursive: true
       last_response.should be_ok
@@ -41,12 +42,30 @@
 
       retrieved_children = retrieved['children']
       retrieved_children.length.should == comment.children.length
+      retrieved["child_count"].should == comment.children.length
 
       comment.children.each_with_index do |child, index|
         expect(retrieved_children[index]).to include('body' => child.body, 'parent_id' => comment.id.to_s)
       end
     end
 
+    it 'retrieves information of a single comment and fixes incorrect child count' do
+      comment = thread.comments.first
+      comment.set(child_count: 2000)
+      comment_hash = comment.to_hash(recursive: true)
+      comment_hash["child_count"].should == 2000
+      get "/api/v1/comments/#{comment.id}", recursive: true
+      last_response.should be_ok
+      retrieved = parse last_response.body
+      retrieved["child_count"].should == comment.children.length
+
+      comment.set(child_count: nil)
+      get "/api/v1/comments/#{comment.id}"
+      last_response.should be_ok
+      retrieved = parse last_response.body
+      retrieved["child_count"].should == comment.children.length
+    end
+
     it 'returns 400 when the comment does not exist' do
       get '/api/v1/comments/does_not_exist'
       last_response.status.should == 400
@@ -147,10 +166,12 @@ def test_unicode_data(text)
 
       comment.reload
       comment.children.length.should == previous_child_count + 1
+      comment.child_count.should == previous_child_count + 1
       sub_comment = comment.children.order_by(created_at: :desc).first
       sub_comment.body.should == body
       sub_comment.course_id.should == course_id
       sub_comment.author.should == user
+      sub_comment.child_count.should == 0
     end
 
     it 'returns 400 when the comment does not exist' do
@@ -180,7 +201,7 @@ def test_unicode_data(text)
   end
 
   describe 'DELETE /api/v1/comments/:comment_id' do
-    it 'delete the comment and its sub comments' do
+    it 'deletes the comment and its sub comments' do
       comment = thread.comments.first
       cnt_comments = comment.descendants_and_self.length
       prev_count = Comment.count

spec/api/comment_thread_spec.rb

@@ -703,11 +703,13 @@ def test_unicode_data(text)
         orig_count = thread.comment_count
         post "/api/v1/threads/#{thread.id}/comments", default_params
         last_response.should be_ok
+        retrieved = parse last_response.body
         changed_thread = CommentThread.find(thread.id)
         changed_thread.comment_count.should == orig_count + 1
         comment = changed_thread.comments.select { |c| c["body"] == "new comment" }.first
         comment.should_not be_nil
         comment.author_id.should == user.id
+        retrieved["child_count"].should == 0
       end
       it "allows anonymous comment" do
         thread = CommentThread.first

spec/models/comment_spec.rb

@@ -69,4 +69,32 @@ def test_unicode_data(text)
       end
     end
   end
+
+  describe '#child_count' do
+    context 'with course_thread' do
+      it 'returns cached child count' do
+        comment = make_comment(author, course_thread, "comment")
+        child_comment = make_comment(author, comment, "comment")
+        expect(comment.get_cached_child_count).to eq(1)
+      end
+
+      it 'returns cached child count' do
+        comment = make_comment(author, course_thread, "comment")
+        child_comment = make_comment(author, comment, "comment")
+        comment.child_count = nil
+        expect(comment.get_cached_child_count).to eq(1)
+      end
+
+      it 'updates cached child count' do
+        comment = make_comment(author, course_thread, "comment")
+        expect(comment.get_cached_child_count).to eq(0)
+        comment.child_count = 2
+        expect(comment.get_cached_child_count).to eq(2)
+        comment.update_cached_child_count
+        expect(comment.get_cached_child_count).to eq(0)
+      end
+    end
+  end
+
+
 end

spec/spec_helper.rb

@@ -251,8 +251,9 @@ def check_comment(comment, hash, is_json, recursive=false)
   hash["username"].should == comment.author_username
   hash["endorsed"].should == comment.endorsed
   hash["endorsement"].should == comment.endorsement
+  children = Comment.where({"parent_id" => comment.id}).sort({"sk" => 1}).to_a
+  hash["child_count"].should == children.length
   if recursive
-    children = Comment.where({"parent_id" => comment.id}).sort({"sk" => 1}).to_a
     hash["children"].length.should == children.length
     hash["children"].each_with_index do |child_hash, i|
       check_comment(children[i], child_hash, is_json)
@@ -330,6 +331,7 @@ def make_comment(author, parent, text)
   else
     coll = parent.children
     thread = parent.comment_thread
+    parent.set(child_count: coll.length + 1)
   end
   comment = coll.new(body: text, course_id: parent.course_id)
   comment.author = author
@@ -390,6 +392,7 @@ def create_comment_thread_and_comments
   # Create a comment along with a nested child comment
   comment = create(:comment, comment_thread: thread)
   create(:comment, parent: comment)
+  comment.set(child_count: 1)
 
   thread
 end