Merge pull request #131 from edx/benmcmorran/team-discussions

TNL-1943 Add context field to CommentThread

AUTHORS

@@ -20,3 +20,4 @@ Sarina Canelake <sarina@edx.org>
 Alexandre Dubus <alexandre.dubus@inria.fr>
 Alan Boudreault <alan@alanb.ca>
 Matjaz Gregoric <mtyaka@gmail.com>
+Ben McMorran <ben.mcmorran@gmail.com>

api/commentables.rb

@@ -20,7 +20,8 @@ get "#{APIPREFIX}/:commentable_id/threads" do |commentable_id|
     params["sort_key"],
     params["sort_order"],
     params["page"],
-    params["per_page"]
+    params["per_page"],
+    params["context"] ? params["context"] : :course
   ).to_json
 end
 
@@ -34,6 +35,10 @@ post "#{APIPREFIX}/:commentable_id/threads" do |commentable_id|
   if params["group_id"]
     thread.group_id = params["group_id"]
   end
+
+  if params["context"]
+    thread.context = params["context"]
+  end
   
   thread.author = user
   thread.save

api/search.rb

@@ -3,6 +3,7 @@ require 'new_relic/agent/method_tracer'
 get "#{APIPREFIX}/search/threads" do
   local_params = params # Necessary for params to be available inside blocks
   group_ids = get_group_ids_from_params(local_params)
+  context = local_params["context"] ? local_params["context"] : "course"
   search_text = local_params["text"]
   if !search_text
     {}.to_json
@@ -23,6 +24,10 @@ get "#{APIPREFIX}/search/threads" do
               filter :term, :commentable_id => local_params["commentable_id"] if local_params["commentable_id"]
               filter :terms, :commentable_id => local_params["commentable_ids"].split(",") if local_params["commentable_ids"]
               filter :term, :course_id => local_params["course_id"] if local_params["course_id"]
+              filter :or, [
+                {:not => {:exists => {:field => :context}}},
+                {:term => {:context => context}}
+              ]
 
               if not group_ids.empty?
                 if group_ids.length > 1
@@ -87,7 +92,8 @@ get "#{APIPREFIX}/search/threads" do
       local_params["sort_key"],
       local_params["sort_order"],
       local_params["page"],
-      local_params["per_page"]
+      local_params["per_page"],
+      context
     )
     if !result_obj.empty?
       result_obj[:corrected_text] = corrected_text

lib/helpers.rb

@@ -129,11 +129,17 @@ helpers do
     sort_key,
     sort_order,
     page,
-    per_page
+    per_page,
+    context=:course
   )
 
+    context_threads = comment_threads.where({:context => context})
+
     if not group_ids.empty?
-      comment_threads = get_group_id_criteria(comment_threads, group_ids)
+      group_threads = get_group_id_criteria(comment_threads, group_ids)
+      comment_threads = comment_threads.all_of(context_threads.selector, group_threads.selector)
+    else
+      comment_threads = context_threads
     end
 
     if filter_flagged

models/comment.rb

@@ -39,6 +39,7 @@ class Comment < Content
     indexes :comment_thread_id, type: :string, index: :not_analyzed, included_in_all: false, as: 'comment_thread_id'
     indexes :commentable_id, type: :string, index: :not_analyzed, included_in_all: false, as: 'commentable_id'
     indexes :group_id, type: :string, index: :not_analyzed, included_in_all: false, as: 'group_id'
+    indexes :context, type: :string, index: :not_analyzed, included_in_all: false, as: 'context'
     indexes :created_at, type: :date, included_in_all: false
     indexes :updated_at, type: :date, included_in_all: false
   end
@@ -133,6 +134,17 @@ class Comment < Content
     nil
   end
 
+  def context
+    if self.comment_thread_id
+      t = CommentThread.find self.comment_thread_id
+      if t
+        t.context
+      end
+    end
+  rescue Mongoid::Errors::DocumentNotFound
+    nil
+  end
+
   def self.by_date_range_and_thread_ids from_when, to_when, thread_ids
      #return all content between from_when and to_when
 

models/comment_thread.rb

@@ -11,6 +11,8 @@ class CommentThread < Content
 
   field :thread_type, type: String, default: :discussion
   enumerize :thread_type, in: [:question, :discussion]
+  field :context, type: String, default: :course
+  enumerize :context, in: [:course, :standalone]
   field :comment_count, type: Integer, default: 0
   field :title, type: String
   field :body, type: String
@@ -41,6 +43,7 @@ class CommentThread < Content
     indexes :comment_count, type: :integer, included_in_all: false
     indexes :votes_point, type: :integer, as: 'votes_point', included_in_all: false
 
+    indexes :context, type: :string, index: :not_analyzed, included_in_all: false
     indexes :course_id, type: :string, index: :not_analyzed, included_in_all: false
     indexes :commentable_id, type: :string, index: :not_analyzed, included_in_all: false
     indexes :author_id, type: :string, as: 'author_id', index: :not_analyzed, included_in_all: false
@@ -56,6 +59,7 @@ class CommentThread < Content
   attr_accessible :title, :body, :course_id, :commentable_id, :anonymous, :anonymous_to_peers, :closed, :thread_type
 
   validates_presence_of :thread_type
+  validates_presence_of :context
   validates_presence_of :title
   validates_presence_of :body
   validates_presence_of :course_id # do we really need this?
@@ -118,7 +122,7 @@ class CommentThread < Content
   end
 
   def to_hash(params={})
-    as_document.slice(*%w[thread_type title body course_id anonymous anonymous_to_peers commentable_id created_at updated_at at_position_list closed])
+    as_document.slice(*%w[thread_type title body course_id anonymous anonymous_to_peers commentable_id created_at updated_at at_position_list closed context])
                      .merge("id" => _id, "user_id" => author_id,
                             "username" => author_username,
                             "votes" => votes.slice(*%w[count up_count down_count point]),

scripts/db/migrate-008-context.js

+print ("Add the new indexes for the context field");
+db.contents.ensureIndex({ _type: 1, course_id: 1, context: 1, pinned: -1, created_at: -1 }, {background: true})
+db.contents.ensureIndex({ _type: 1, commentable_id: 1, context: 1, pinned: -1, created_at: -1 }, {background: true})
+
+print ("Adding context to all comment threads where it does not yet exist\n");
+db.contents.update(
+    {_type: "CommentThread", context: {$exists: false}},
+    {$set: {context: "course"}},
+    {multi: true}
+);
+printjson (db.runCommand({ getLastError: 1, w: "majority", wtimeout: 5000 } ));

scripts/db/revert-migrate-008-context.js

+print ("remove the indexes for the context field");
+db.contents.dropIndex({ _type: 1, course_id: 1, context: 1, pinned: -1, created_at: -1 })
+db.contents.dropIndex({ _type: 1, commentable_id: 1, context: 1, pinned: -1, created_at: -1 })
+
+print ("Removing context from all comment threads\n");
+db.contents.update(
+    {_type: "CommentThread"},
+    {$unset: {context: ""}},
+    {multi: true}
+);
+printjson (db.runCommand({ getLastError: 1, w: "majority", wtimeout: 5000 } ));

spec/api/comment_thread_spec.rb

@@ -29,6 +29,18 @@ describe "app" do
             res["course_id"].should == "abc"
           }
         end
+        it "does not return standalone threads" do
+          [@threads["t1"], @threads["t2"], @threads["t3"]].each do |t|
+            t.course_id = "abc"
+            t.save!
+          end
+          @threads["t2"].context = :standalone
+          @threads["t2"].save!
+          rs = thread_result course_id: "abc", sort_order: "asc"
+          rs.length.should == 2
+          check_thread_result_json(nil, @threads["t1"], rs[0])
+          check_thread_result_json(nil, @threads["t3"], rs[1])
+        end
         it "returns only threads where course id and commentable id match" do
           @threads["t1"].course_id = "course1"
           @threads["t1"].commentable_id = "commentable1"

spec/api/commentable_spec.rb

@@ -32,6 +32,11 @@ describe "app" do
         threads.index{|c| c["body"] == "can anyone help me?"}.should_not be_nil
         threads.index{|c| c["body"] == "it is unsolvable"}.should_not be_nil
       end
+      it "returns standalone threads if explicitly requested" do
+        threads = thread_result "question_1", context: "standalone"
+        threads.length.should == 1
+        threads[0]["body"].should == "no one can see us"
+      end
       it "filters by course_id" do
         course1_threads = thread_result "question_1", course_id: "1"
         course1_threads.length.should == 1
@@ -98,7 +103,17 @@ describe "app" do
         result["unread_comments_count"].should == 0
         result["endorsed"].should == false
         CommentThread.count.should == old_count + 1
-        CommentThread.where(title: "Interesting question").first.should_not be_nil
+        thread = CommentThread.where(title: "Interesting question").first
+        thread.should_not be_nil
+        thread.context.should == "course"
+      end
+      it "can create a standalone thread" do
+        old_count = CommentThread.count
+        post '/api/v1/question_1/threads', default_params.merge(:context => "standalone")
+        CommentThread.count.should == old_count + 1
+        thread = CommentThread.where(title: "Interesting question").first
+        thread.should_not be_nil
+        thread.context.should == "standalone"
       end
       CommentThread.thread_type.values.each do |thread_type|
         it "can create a #{thread_type} thread" do

spec/api/search_spec.rb

@@ -38,7 +38,7 @@ describe "app" do
 
       describe "filtering works" do
         let!(:threads) do
-          threads = (0..29).map do |i|
+          threads = (0..34).map do |i|
             thread = make_thread(author, "text", course_id + (i % 2).to_s, "commentable" + (i % 3).to_s)
             if i < 2
               comment = make_comment(author, thread, "objectionable")
@@ -55,6 +55,10 @@ describe "app" do
               comment = make_comment(author, thread, "response")
               comment.save!
             end
+            if i > 29
+              thread.context = :standalone
+              thread.save!
+            end
             thread
           end
           refresh_es_index
@@ -74,6 +78,11 @@ describe "app" do
           assert_response_contains((0..29).find_all {|i| i % 2 == 0})
         end
 
+        it "by context" do
+          get "api/v1/search/threads", text: "text", context: "standalone"
+          assert_response_contains(30..34)
+        end
+
         it "with unread filter" do
           user = create_test_user(Random.new)
           user.mark_as_read(threads[0])

spec/spec_helper.rb

@@ -131,6 +131,13 @@ def init_without_subscriptions
   comment1.comment_thread = thread
   comment1.save!
 
+  thread = CommentThread.new(title: "Our super secret discussion", body: "no one can see us", course_id: "2", commentable_id: commentable.id)
+  thread.thread_type = :discussion
+  thread.context = :standalone
+  thread.author = user
+  thread.save!
+  user.subscribe(thread)
+
   thread = CommentThread.new(title: "I don't know what to say", body: "lol", course_id: "2", commentable_id: "something else")
   thread.thread_type = :discussion
   thread.author = users[1]
@@ -205,7 +212,7 @@ end
 # this method is used to test results produced using the helper function handle_threads_query
 # which is used in multiple areas of the API
 def check_thread_result(user, thread, hash, is_json=false)
-  expected_keys = %w(id thread_type title body course_id commentable_id created_at updated_at)
+  expected_keys = %w(id thread_type title body course_id commentable_id created_at updated_at context)
   expected_keys += %w(anonymous anonymous_to_peers at_position_list closed user_id)
   expected_keys += %w(username votes abuse_flaggers tags type group_id pinned)
   expected_keys += %w(comments_count unread_comments_count read endorsed)
@@ -237,6 +244,7 @@ def check_thread_result(user, thread, hash, is_json=false)
   hash["pinned"].should == thread.pinned?
   hash["endorsed"].should == thread.endorsed?
   hash["comments_count"].should == thread.comments.length
+  hash["context"] = thread.context
 
   if is_json
     hash["id"].should == thread._id.to_s