From d6b7d9033d8f7b891f0f710c83a8802cc60fe446 Mon Sep 17 00:00:00 2001 From: wajeeha-khalid Date: Mon, 14 Dec 2015 12:33:55 +0500 Subject: [PATCH] jia/MA-1815 retrieve count for child comments --- api/comment_threads.rb | 1 + api/comments.rb | 24 ++++++++++++++++++++--- lib/helpers.rb | 10 ++++++++++ models/comment.rb | 34 ++++++++++++++++++++++----------- spec/api/comment_spec.rb | 27 +++++++++++++++++++++++--- spec/api/comment_thread_spec.rb | 2 ++ spec/models/comment_spec.rb | 28 +++++++++++++++++++++++++++ spec/spec_helper.rb | 5 ++++- 8 files changed, 113 insertions(+), 18 deletions(-) diff --git a/api/comment_threads.rb b/api/comment_threads.rb index 4b45edc8331..cef1f09fa77 100644 --- a/api/comment_threads.rb +++ b/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 diff --git a/api/comments.rb b/api/comments.rb index d954fb462d0..7fc44aa6675 100644 --- a/api/comments.rb +++ b/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 diff --git a/lib/helpers.rb b/lib/helpers.rb index 45a3c24bdee..083e23f6638 100644 --- a/lib/helpers.rb +++ b/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" diff --git a/models/comment.rb b/models/comment.rb index 32b5ff6c15a..87c15a4852e 100644 --- a/models/comment.rb +++ b/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 diff --git a/spec/api/comment_spec.rb b/spec/api/comment_spec.rb index 587c3d9d29f..335b0d7c26c 100644 --- a/spec/api/comment_spec.rb +++ b/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 diff --git a/spec/api/comment_thread_spec.rb b/spec/api/comment_thread_spec.rb index d43a638cf75..6c4fd5b59fa 100644 --- a/spec/api/comment_thread_spec.rb +++ b/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 diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index e7a62b9f091..723dc7160f6 100644 --- a/spec/models/comment_spec.rb +++ b/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 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 65d35603bb8..04cdd4982c2 100644 --- a/spec/spec_helper.rb +++ b/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