Merge pull request #135 from edx/bderusha/forum-filter-standalone-threads-TNL-2971

Do not return standalone context threads for users

api/users.rb

@@ -39,7 +39,7 @@ get "#{APIPREFIX}/users/:user_id/active_threads" do |user_id|
     thread_ids
   end
 
-  threads = CommentThread.in({"_id" => active_thread_ids})
+  threads = CommentThread.course_context.in({"_id" => active_thread_ids})
 
   group_ids = get_group_ids_from_params(params)
   if not group_ids.empty?

models/comment.rb

@@ -135,14 +135,15 @@ class Comment < Content
   end
 
   def context
-    if self.comment_thread_id
-      t = CommentThread.find self.comment_thread_id
-      if t
-        t.context
+    self.comment_thread_id ? self.comment_thread.context : nil
   end
+
+  def course_context?
+    self.context == 'course'
   end
-  rescue Mongoid::Errors::DocumentNotFound
-    nil
+
+  def standalone_context?
+    self.context == 'standalone'
   end
 
   def self.by_date_range_and_thread_ids from_when, to_when, thread_ids

models/comment_thread.rb

@@ -73,6 +73,8 @@ class CommentThread < Content
   before_destroy :destroy_subscriptions
 
   scope :active_since, ->(from_time) { where(:last_activity_at => {:$gte => from_time}) }
+  scope :standalone_context, ->() { where(:context => :standalone) }
+  scope :course_context, ->() { where(:context => :course) }
 
   def self.new_dumb_thread(options={})
     c = self.new

models/user.rb

@@ -52,7 +52,7 @@ class User
         if not params[:group_ids].empty?
           # Get threads in either the specified group(s) or posted to all groups (nil).
           specified_groups_or_global = params[:group_ids] << nil
-          threads_count = CommentThread.where(
+          threads_count = CommentThread.course_context.where(
             author_id: id,
             course_id: params[:course_id],
             group_id: {"$in" => specified_groups_or_global},
@@ -61,12 +61,17 @@ class User
           ).count
 
           # Note that the comments may have been responses to a thread not started by author_id.
-          comment_thread_ids = Comment.where(
+
+          # comment.standalone_context? gets the context from the parent comment_thread
+          # we need to eager load the comment_thread to prevent an N+1 when we iterate through the results
+          comment_thread_ids = Comment.includes(:comment_thread).where(
             author_id: id,
             course_id: params[:course_id],
             anonymous: false,
             anonymous_to_peers: false
-          ).collect{|c| c.comment_thread_id}
+          ).
+          reject{ |comment| comment.standalone_context? }.
+          collect{ |comment| comment.comment_thread_id }
 
           # Filter to the unique thread ids visible to the specified group(s).
           group_comment_thread_ids = CommentThread.where(
@@ -81,18 +86,20 @@ class User
           }
 
         else
-          threads_count = CommentThread.where(
+          threads_count = CommentThread.course_context.where(
             author_id: id,
             course_id: params[:course_id],
             anonymous: false,
             anonymous_to_peers: false
           ).count
-          comments_count = Comment.where(
+          # comment.standalone_context? gets the context from the parent comment_thread
+          # we need to eager load the comment_thread to prevent an N+1 when we iterate through the results
+          comments_count = Comment.includes(:comment_thread).where(
             author_id: id,
             course_id: params[:course_id],
             anonymous: false,
             anonymous_to_peers: false
-          ).count
+          ).reject{ |comment| comment.standalone_context? }.count
         end
         hash = hash.merge("threads_count" => threads_count, "comments_count" => comments_count)
       end

spec/api/user_spec.rb

@@ -149,6 +149,19 @@ describe "app" do
              verify_counts_multiple_groups(2, 6, "100", "3000")
              verify_counts_multiple_groups(1, 5, "100", "8")
           end
+
+          context "standalone threads" do
+            before(:each) do
+              # creates a standalone thread with 3 comments by user 100
+              make_standalone_thread_with_comments(@users['u100'])
+            end
+
+            it 'does not return standalone thread or comments in counts' do
+              # user 100 already has 1 thread and 5 comments created by `setup_10_threads`
+              # verify that the new standalone thread is not added to the counts
+              verify_counts(1, 5, "100")
+            end
+          end
       end
     end
     describe "GET /api/v1/users/:user_id/active_threads" do
@@ -168,13 +181,36 @@ describe "app" do
         last_response.body.should == "{}"
       end
 
-      it "only returns threads with activity from the specified user"  do
+      context 'with standalone thread' do
+        before(:each) do
+          # creates a standalone thread with 3 comments by user 100, stored as "standalone t0 c{i}"
+          make_standalone_thread_with_comments(@users['u100'], 0)
+          # creates a standalone thread with 3 comments by user 101, with stored as "standalone t0 c{i}"
+          make_standalone_thread_with_comments(@users['u101'], 1)
+        end
+
+        it "only returns threads with non-standalone activity from the specified user"  do
+          # `setup_10_threads` creates a thread "t3" and 5 comments all owned by user 103 
+          # we are hijacking a course thread comment owned by user 103 and making it owned
+          # by user 100 instead, so this user has a comment on someone else's thread
           @comments["t3 c4"].author = @users["u100"]
           @comments["t3 c4"].save!
-        rs = thread_result 100, course_id: "xyz"
-        rs.length.should == 2
-        check_thread_result_json(@users["u100"], @threads["t3"], rs[0])
-        check_thread_result_json(@users["u100"], @threads["t0"], rs[1])
+
+          # do the same as above but with standalone
+          # hijack a standalone thread comment and make it owned by user 100
+          @comments["standalone t1 c1"].author = @users["u100"]
+          @comments["standalone t1 c1"].save!
+
+          results = thread_result 100, course_id: "xyz"
+          # it should not include the standalone thread we created for user 100
+          # not the standalone thread user 100 is a commenter on
+          results.length.should == 2
+
+          # it should include the course thread owned by user 100 (t0)
+          # and the course thread user 100 has a comment on (t3)
+          check_thread_result_json(@users["u100"], @threads["t3"], results[0])
+          check_thread_result_json(@users["u100"], @threads["t0"], results[1])
+        end
       end
 
       it "filters by group_id" do

spec/models/comment_spec.rb

@@ -6,15 +6,67 @@ describe Comment do
     create_test_user(42)
   end
 
-  let(:thread) do
-    make_thread(author, "Test thread", "test_course", "test_commentable")
+  let(:course_thread) do
+    make_thread(author, "Test course thread", "test_course", "test_commentable", :discussion, :course)
+  end
+
+  let(:standalone_thread) do
+    make_thread(author, "Test standalone thread", "test_course", "test_commentable", :discussion, :standalone)
   end
 
   def test_unicode_data(text)
-    comment = make_comment(author, thread, text)
+    comment = make_comment(author, course_thread, text)
     retrieved = Comment.find(comment._id)
     retrieved.body.should == text
   end
 
   include_examples "unicode data"
+
+  describe '#context' do
+    context 'with standalone_thread' do
+      it 'returns "standalone"' do
+        comment = make_comment(author, standalone_thread, "comment")
+        expect(comment.context).to eq("standalone")
+      end
+    end
+
+    context 'with course_thread' do
+      it 'returns "course"' do
+        comment = make_comment(author, course_thread, "comment")
+        expect(comment.context).to eq("course")
+      end
+    end
+  end
+
+  describe '#course_context?' do
+    context 'with standalone_thread' do
+      it 'returns false' do
+        comment = make_comment(author, standalone_thread, "comment")
+        expect(comment.course_context?).to be_false
+      end
+    end
+
+    context 'with course_thread' do
+      it 'returns true' do
+        comment = make_comment(author, course_thread, "comment")
+        expect(comment.course_context?).to be_true
+      end
+    end
+  end
+
+  describe '#standalone_context?' do
+    context 'with standalone_thread' do
+      it 'returns true' do
+        comment = make_comment(author, standalone_thread, "comment")
+        expect(comment.standalone_context?).to be_true
+      end
+    end
+
+    context 'with course_thread' do
+      it 'returns false' do
+        comment = make_comment(author, course_thread, "comment")
+        expect(comment.standalone_context?).to be_false
+      end
+    end
+  end
 end

spec/models/comment_thread_spec.rb

@@ -6,12 +6,11 @@ describe CommentThread do
     create_test_user(42)
   end
 
-  context "sorting" do
-
   before (:each) do
     [Comment, CommentThread, User].each(&:delete_all).each(&:remove_indexes).each(&:create_indexes)
   end
 
+  context "sorting" do
     it "indexes comments in hierarchical order" do
 
       author = create_test_user('billy')
@@ -57,6 +56,54 @@ describe CommentThread do
     end
   end
 
+  context "scoping" do
+    before(:each) do
+      author = create_test_user('billy')
+
+      # create a course thread
+      course_thread = CommentThread.new(title: "course thread", body: "testing 123", course_id: "foo", commentable_id: "bar")
+      course_thread.thread_type = :discussion
+      course_thread.author = author
+      course_thread.context = :course
+      course_thread.save!
+
+      # create a course thread (using the default context rather than setting it explicitly)
+      course_thread = CommentThread.new(title: "course thread", body: "testing 123", course_id: "foo", commentable_id: "bar")
+      course_thread.thread_type = :discussion
+      course_thread.author = author
+      course_thread.save!
+
+      # create a standalone thread
+      standalone_thread = CommentThread.new(title: "standalone_thread thread", body: "testing 123", course_id: "foo", commentable_id: "bear")
+      standalone_thread.thread_type = :discussion
+      standalone_thread.author = author
+      standalone_thread.context = :standalone
+      standalone_thread.save!
+    end
+    
+    context '#unscoped' do
+      it 'returns all' do
+        expect(CommentThread.count).to eq(3)
+      end
+    end
+
+    context "#course_context" do
+      it 'returns all' do
+        threads = CommentThread.course_context
+        expect(threads.count).to eq(2)
+        expect(threads.first.title).to eq("course thread")
+      end
+    end
+
+    context "#standalone_context" do
+      it 'returns all' do
+        threads = CommentThread.standalone_context
+        expect(threads.count).to eq(1)
+        expect(threads.first.title).to eq("standalone_thread thread")
+      end
+    end
+  end
+
   def test_unicode_data(text)
     thread = make_thread(author, text, "unicode_course", commentable_id: "unicode_commentable")
     retrieved = CommentThread.find(thread._id)

spec/spec_helper.rb

@@ -354,10 +354,11 @@ def check_thread_response_paging_json(thread, hash, resp_skip=0, resp_limit=nil)
 end
 
 # general purpose factory helpers
-def make_thread(author, text, course_id, commentable_id, thread_type=:discussion)
+def make_thread(author, text, course_id, commentable_id, thread_type=:discussion, context=:course)
   thread = CommentThread.new(title: text, body: text, course_id: course_id, commentable_id: commentable_id)
   thread.thread_type = thread_type
   thread.author = author
+  thread.context = context
   thread.save!
   thread
 end
@@ -377,6 +378,27 @@ def make_comment(author, parent, text)
   comment
 end
 
+# add standalone threads and comments to the @threads and @comments hashes
+# using the namespace "standalone t#{index}" for threads and "standalone t#{index} c#{i}" for comments
+# takes an index param if used within an iterator, otherwise will namespace using 0 for thread index
+# AKA this will overwrite "standalone t0" each time it is called. 
+def make_standalone_thread_with_comments(author, index=0)
+  thread = make_thread(
+    author,
+    "standalone thread #{index}",
+    DFLT_COURSE_ID,
+    "pdq",
+    :discussion,
+    :standalone
+  )
+
+  3.times do |i|
+    @comments["standalone t#{index} c#{i}"] = make_comment(author, thread, "stand alone comment #{i}")
+  end
+
+  @threads["standalone t#{index}"] = thread
+end
+
 DFLT_COURSE_ID = "xyz"
 
 def setup_10_threads