merge master

.gitignore

@@ -30,7 +30,6 @@ benchmark_log
 bin/
 
 log/
-
 #redcar
 .redcar/
 /nbproject

AUTHORS

+PRIMARY AUTHORS
+
+Rocky Duan <dementrock@gmail.com>
+Arjun Singh <arjun810@gmail.com>
+Kevin Chugh <kevinchugh@edx.org>
+Ibrahim Awwal <ibrahim.awwal@gmail.com>
+
+CONTRIBUTORS
+
+Mike Chen <ccp0101@gmail.com>
+David Ormsbee <dave@edx.org>
+Matthew Mongeau <halogenandtoast@gmail.com>
+Christina Roberts <christina@edx.org>
+Calen Pennington <calen.pennington@gmail.com>
+Ed Zarecor <ed@edx.org>
+Jay Zoldak <zoldak@edx.org>

README.md

-comment_as_a_service
-====================
-
-An independent comment system which supports voting and nested comments. It also supports features including instructor endorsement for education-aimed discussion platforms.

README.rst

+Part of `edX code`__.
+
+__ http://code.edx.org/
+
+comment_as_a_service
+====================
+
+An independent comment system which supports voting and nested comments. It also supports features including instructor endorsement for education-aimed discussion platforms.
+
+
+License
+-------
+
+The code in this repository is licensed under version 3 of the AGPL unless
+otherwise noted.
+
+Please see ``LICENSE.txt`` for details.
+
+How to Contribute
+-----------------
+
+Contributions are very welcome. The easiest way is to fork this repo, and then
+make a pull request from your fork. The first time you make a pull request, you
+may be asked to sign a Contributor Agreement.
+
+Reporting Security Issues
+-------------------------
+
+Please do not report security issues in public. Please email security@edx.org
+
+Mailing List and IRC Channel
+----------------------------
+
+You can discuss this code on the `edx-code Google Group`__ or in the
+``edx-code`` IRC channel on Freenode.
+
+__ https://groups.google.com/forum/#!forum/edx-code

Rakefile

@@ -24,10 +24,10 @@ task :environment do
 
   Dir[File.dirname(__FILE__) + '/lib/**/*.rb'].each {|file| require file}
   Dir[File.dirname(__FILE__) + '/models/*.rb'].each {|file| require file}
-  Dir[File.dirname(__FILE__) + '/models/observers/*.rb'].each {|file| require file}
+  #Dir[File.dirname(__FILE__) + '/models/observers/*.rb'].each {|file| require file}
 
-  Mongoid.observers = PostReplyObserver, PostTopicObserver, AtUserObserver
-  Mongoid.instantiate_observers
+  #Mongoid.observers = PostReplyObserver, PostTopicObserver, AtUserObserver
+  #Mongoid.instantiate_observers
 
 end
 
@@ -224,14 +224,41 @@ namespace :db do
   end
 
   task :reindex_search => :environment do
-    Tire.index('comment_threads').delete
-    CommentThread.create_elasticsearch_index
-    Tire.index('comment_threads') { import CommentThread.all }
 
-    Tire.index('comments').delete
-    Comment.create_elasticsearch_index
-    Tire.index('comments') { import Comment.all }
+    Mongoid.identity_map_enabled = false
+
+    klass = CommentThread
+    ENV['CLASS'] = klass.name
+    ENV['INDEX'] = new_index = klass.tire.index.name << '_' << Time.now.strftime('%Y%m%d%H%M%S')
+
+    Rake::Task["tire:import"].invoke
+
+    puts '[IMPORT] about to swap index'
+    if a = Tire::Alias.find(klass.tire.index.name)
+      puts "[IMPORT] aliases found: #{Tire::Alias.find(klass.tire.index.name).indices.to_ary.join(',')}. deleting."
+      old_indices = Tire::Alias.find(klass.tire.index.name).indices
+      old_indices.each do |index|
+        a.indices.delete index
+      end
+
+      a.indices.add new_index
+      a.save
+
+      old_indices.each do |index|
+        puts "[IMPORT] deleting index: #{index}"
+        i = Tire::Index.new(index)
+        i.delete if i.exists?
+      end
+    else
+      puts "[IMPORT] no aliases found. deleting index. creating new one and setting up alias."
+      klass.tire.index.delete
+      a = Tire::Alias.new
+      a.name(klass.tire.index.name)
+      a.index(new_index)
+      a.save
+    end
 
+    puts "[IMPORT] done. Index: '#{new_index}' created."
   end
 
   task :add_anonymous_to_peers => :environment do

api/comment_threads.rb

@@ -16,7 +16,7 @@ get "#{APIPREFIX}/threads/:thread_id" do |thread_id|
   thread = CommentThread.find(thread_id)
 
   if params["user_id"] and bool_mark_as_read
-    user = User.only([:id, :read_states]).find_by(external_id: params["user_id"])
+    user = User.only([:id, :username, :read_states]).find_by(external_id: params["user_id"])
     user.mark_as_read(thread) if user
   end
 

api/flags.rb

+put "#{APIPREFIX}/threads/:thread_id/abuse_flag" do |thread_id|
+  flag_as_abuse thread
+end
+
+put "#{APIPREFIX}/threads/:thread_id/abuse_unflag" do |thread_id|
+  un_flag_as_abuse thread
+end
+
+put "#{APIPREFIX}/comments/:comment_id/abuse_flag" do |comment_id|
+  flag_as_abuse comment
+end
+
+put "#{APIPREFIX}/comments/:comment_id/abuse_unflag" do |comment_id|
+  un_flag_as_abuse comment
+end

api/pins.rb

+put "#{APIPREFIX}/threads/:thread_id/pin" do |thread_id|
+  pin thread
+end
+
+put "#{APIPREFIX}/threads/:thread_id/unpin" do |thread_id|
+  unpin thread
+end
+

api/users.rb

@@ -30,9 +30,20 @@ get "#{APIPREFIX}/users/:user_id/active_threads" do |user_id|
   page = [num_pages, [1, page].max].min
 
   paged_active_contents = active_contents.page(page).per(per_page)
-  paged_active_threads = paged_active_contents.map(&get_thread_id)
-                                              .uniq.map(&get_thread)
-  collection = paged_active_threads.map{|t| t.to_hash(recursive: true)}
+  paged_thread_ids = paged_active_contents.map(&get_thread_id).uniq
+
+  # Find all the threads by id, and then put them in the order found earlier.
+  # Necessary because CommentThread.find does return results in the same
+  # order as the provided ids.
+  paged_active_threads = CommentThread.find(paged_thread_ids).sort_by do |t| 
+    paged_thread_ids.index(t.id)
+  end
+
+  # Fetch all the usernames in bulk to save on queries. Since we're using the
+  # identity map, the users won't need to be fetched again later.
+  User.only(:username).find(paged_active_threads.map{|x| x.author_id})
+
+  collection = paged_active_threads.map{|t| t.to_hash recursive: true}
   collection = author_contents_only(collection, user_id)
 
   {

app.rb

@@ -36,10 +36,11 @@ Mongoid.logger.level = Logger::INFO
 
 Dir[File.dirname(__FILE__) + '/lib/**/*.rb'].each {|file| require file}
 Dir[File.dirname(__FILE__) + '/models/*.rb'].each {|file| require file}
-Dir[File.dirname(__FILE__) + '/models/observers/*.rb'].each {|file| require file}
 
-Mongoid.observers = PostReplyObserver, PostTopicObserver, AtUserObserver
-Mongoid.instantiate_observers
+# Comment out observers until notifications are actually set up properly.
+#Dir[File.dirname(__FILE__) + '/models/observers/*.rb'].each {|file| require file}
+#Mongoid.observers = PostReplyObserver, PostTopicObserver, AtUserObserver
+#Mongoid.instantiate_observers
 
 APIPREFIX = CommentService::API_PREFIX
 DEFAULT_PAGE = 1
@@ -51,8 +52,12 @@ if RACK_ENV.to_s != "test" # disable api_key auth in test environment
   end
 end
 
-# these files must be required in order
+# Enable the identity map. The middleware ensures that the identity map is
+# cleared for every request.
+Mongoid.identity_map_enabled = true
+use Rack::Mongoid::Middleware::IdentityMap
 
+# these files must be required in order
 require './api/search'
 require './api/commentables'
 require './api/tags'
@@ -60,6 +65,8 @@ require './api/comment_threads'
 require './api/comments'
 require './api/users'
 require './api/votes'
+require './api/flags'
+require './api/pins'
 require './api/notifications_and_subscriptions'
 
 if RACK_ENV.to_s == "development"

lib/helpers.rb

@@ -37,12 +37,50 @@ helpers do
     obj.reload.to_hash.to_json
   end
 
+  def flag_as_abuse(obj)
+    raise ArgumentError, "User id is required" unless user
+    obj.abuse_flaggers << user.id unless obj.abuse_flaggers.include? user.id
+    obj.save
+    obj.reload.to_hash.to_json
+  end
+  
+  def un_flag_as_abuse(obj)
+    raise ArgumentError, "User id is required" unless user
+    if params["all"]
+      obj.historical_abuse_flaggers += obj.abuse_flaggers
+      obj.historical_abuse_flaggers = obj.historical_abuse_flaggers.uniq
+      obj.abuse_flaggers.clear
+    else
+      obj.abuse_flaggers.delete user.id
+    end
+    
+    obj.save
+    obj.reload.to_hash.to_json
+  end
+
   def undo_vote_for(obj)
     raise ArgumentError, "must provide user id" unless user
     user.unvote(obj)
     obj.reload.to_hash.to_json
   end
   
+
+  def pin(obj)
+    raise ArgumentError, "User id is required" unless user
+    obj.pinned = true
+    obj.save
+    obj.reload.to_hash.to_json
+  end
+  
+  def unpin(obj)
+    raise ArgumentError, "User id is required" unless user
+    obj.pinned = nil
+    obj.save
+    obj.reload.to_hash.to_json
+  end  
+  
+  
+  
   def value_to_boolean(value)
     !!(value.to_s =~ /^true$/i)
   end
@@ -76,7 +114,26 @@ helpers do
   end
 
   def handle_threads_query(comment_threads)
+    
+      
+    if params[:course_id]
       comment_threads = comment_threads.where(:course_id=>params[:course_id])
+      
+      if params[:flagged]
+        #get flagged threads and threads containing flagged responses
+        comment_ids = Comment.where(:course_id=>params[:course_id]).
+          where(:abuse_flaggers.ne => [],:abuse_flaggers.exists => true).
+          collect{|c| c.comment_thread_id}.uniq
+          
+        thread_ids = comment_threads.where(:abuse_flaggers.ne => [],:abuse_flaggers.exists => true).
+          collect{|c| c.id}
+          
+        comment_ids += thread_ids
+        
+        comment_threads = comment_threads.where(:id.in => comment_ids)
+        
+      end
+    end
     if CommentService.config[:cache_enabled]
       query_params = params.slice(*%w[course_id commentable_id sort_key sort_order page per_page user_id])
       memcached_key = "threads_query_#{query_params.hash}"
@@ -111,7 +168,9 @@ helpers do
     else
       page = (params["page"] || DEFAULT_PAGE).to_i
       per_page = (params["per_page"] || DEFAULT_PER_PAGE).to_i
-      comment_threads = comment_threads.order_by("#{sort_key} #{sort_order}") if sort_key && sort_order
+      #KChugh turns out we don't need to go through all the extra work on the back end because the client is resorting anyway
+      #KChugh boy was I wrong, we need to sort for pagination
+      comment_threads = comment_threads.order_by("pinned DESC,#{sort_key} #{sort_order}") if sort_key && sort_order
       num_pages = [1, (comment_threads.count / per_page.to_f).ceil].max
       page = [num_pages, [1, page].max].min
       paged_comment_threads = comment_threads.page(page).per(per_page)
@@ -123,6 +182,7 @@ helpers do
         }
         Sinatra::Application.cache.set(memcached_key, cached_results, CommentService.config[:cache_timeout][:threads_query].to_i)
       end
+      
       {
         collection: paged_comment_threads.map{|t| t.to_hash(recursive: bool_recursive, user_id: params["user_id"])},
         num_pages: num_pages,

lib/tasks/flags.rake

+require 'rest_client'
+roots = {}
+roots['development'] = "http://localhost:8000"
+roots['test'] = "http://localhost:8000"
+roots['production'] = "http://edx.org"
+ROOT = roots[ENV['SINATRA_ENV']]
+
+namespace :flags do
+
+
+  #USAGE
+  #SINATRA_ENV=development rake flags:flagged
+
+  task :flagged => :environment do
+    flagged = Content.flagged
+
+    courses = {}
+
+    flagged.each do |f|
+
+      if not courses[f.course_id]
+        courses[f.course_id] = []
+      end
+
+      courses[f.course_id] << f
+    end
+
+    courses.each do |k,v|
+      puts "#{k.upcase}"
+      puts "****************"
+      v.each do |f|
+        puts "#{ROOT}/courses/#{f.course_id}/discussion/forum/#{f.commentable_id}/threads/#{f.comment_thread_id} (#{f.class})"
+      end
+      puts "\n\n\n\n"
+    end
+
+  end
+end

models/comment.rb

@@ -49,6 +49,22 @@ class Comment < Content
     nodes.map{|node, sub_nodes| node.to_hash.merge("children" => hash_tree(sub_nodes).compact)}
   end
 
+  # This should really go somewhere else, but sticking it here for now. This is
+  # used to flatten out the subtree fetched by calling self.subtree. This is
+  # equivalent to calling descendants_and_self; however, calling
+  # descendants_and_self and subtree both is very inefficient. It's cheaper to
+  # just flatten out the subtree, and simpler than duplicating the code that
+  # actually creates the subtree.
+  def self.flatten_subtree(x)
+    if x.is_a? Array
+      x.flatten.map{|y| self.flatten_subtree(y)}
+    elsif x.is_a? Hash
+      x.to_a.map{|y| self.flatten_subtree(y)}.flatten
+    else
+      x
+    end
+  end
+
   def to_hash(params={})
     sort_by_parent_and_time = Proc.new do |x, y|
       arr_cmp = x.parent_ids.map(&:to_s) <=> y.parent_ids.map(&:to_s)
@@ -59,21 +75,38 @@ class Comment < Content
       end
     end
     if params[:recursive]
-      self.class.hash_tree(subtree(sort: sort_by_parent_and_time)).first
+      subtree_hash = subtree(sort: sort_by_parent_and_time)
+
+      # Flatten out the subtree and fetch all users in bulk; makes getting the
+      # usernames faster. Should probably denormalize usernames.
+      flattened_subtree = Comment.flatten_subtree(subtree_hash)
+      User.only(:username).find(flattened_subtree.map{|x| x.author_id})
+
+      self.class.hash_tree(subtree_hash).first
     else
       as_document.slice(*%w[body course_id endorsed anonymous anonymous_to_peers created_at updated_at at_position_list])
                  .merge("id" => _id)
-                 .merge("user_id" => author.id)
+                 .merge("user_id" => author_id)
                  .merge("username" => author.username)
                  .merge("depth" => depth)
                  .merge("closed" => comment_thread.closed)
-                 .merge("thread_id" => comment_thread.id)
+                 .merge("thread_id" => comment_thread_id)
                  .merge("commentable_id" => comment_thread.commentable_id)
                  .merge("votes" => votes.slice(*%w[count up_count down_count point]))
+                 .merge("abuse_flaggers" => abuse_flaggers)
                  .merge("type" => "comment")
     end
   end
   
+  def commentable_id
+    #we need this to have a universal access point for the flag rake task
+    if self.comment_thread_id
+      t = CommentThread.find self.comment_thread_id
+      if t
+        t.commentable_id
+      end
+    end
+  end
 private
 
   def set_thread_last_activity_at

models/comment_thread.rb

@@ -22,6 +22,7 @@ class CommentThread < Content
   field :at_position_list, type: Array, default: []
   field :last_activity_at, type: Time
   field :group_id, type: Integer
+  field :pinned, type: Boolean
 
   index({author_id: 1, course_id: 1})
 
@@ -237,6 +238,7 @@ class CommentThread < Content
 
   def to_hash(params={})
     doc = as_document.slice(*%w[title body course_id anonymous anonymous_to_peers commentable_id created_at updated_at at_position_list closed])
+<<<<<<< HEAD
     .merge("id" => _id, "user_id" => author.id,
     "username" => author.username,
     "votes" => votes.slice(*%w[count up_count down_count point]),
@@ -244,6 +246,17 @@ class CommentThread < Content
     "type" => "thread",
     "group_id" => group_id,
     "endorsed" => endorsed?)
+=======
+                     .merge("id" => _id, "user_id" => author_id,
+                            "username" => author.username,
+                            "votes" => votes.slice(*%w[count up_count down_count point]),
+                            "abuse_flaggers" => abuse_flaggers,
+                            "tags" => tags_array,
+                            "type" => "thread",
+                            "group_id" => group_id,
+                            "pinned" => pinned?,
+                            "endorsed" => endorsed?)
+>>>>>>> master
 
     if params[:recursive]
       doc = doc.merge("children" => root_comments.map{|c| c.to_hash(recursive: true)})
@@ -291,7 +304,16 @@ class CommentThread < Content
     !!(tag =~ RE_TAG)
   end
 
+<<<<<<< HEAD
   private
+=======
+  def comment_thread_id
+    #so that we can use the comment thread id as a common attribute for flagging
+    self.id
+  end  
+  
+private
+>>>>>>> master
 
   RE_HEADCHAR = /[a-z0-9]/
   RE_ENDONLYCHAR = /\+/
@@ -319,4 +341,5 @@ class CommentThread < Content
   def destroy_subscriptions
     subscriptions.delete_all
   end
+  
 end

models/content.rb

@@ -2,6 +2,10 @@ class Content
   
   include Mongoid::Document
   
+  field :visible, type: Boolean, default: true
+  field :abuse_flaggers, type: Array, default: []
+  field :historical_abuse_flaggers, type: Array, default: [] #preserve abuse flaggers after a moderator unflags
+  
   def author_with_anonymity(attr=nil, attr_when_anonymous=nil)
     if not attr
       (anonymous || anonymous_to_peers) ? nil : author
@@ -10,6 +14,15 @@ class Content
     end
   end
   
+  def self.flagged
+    #return an array of flagged content
+    holder = []
+    Content.where(:abuse_flaggers.ne => [],:abuse_flaggers.exists => true).each do |c|
+      holder << c
+    end
+    holder
+  end
+
   def self.prolific_metric what, count
     #take a hash of criteria (what) and return a hash of hashes
     #course => user => count
@@ -71,6 +84,4 @@ class Content
     answer
   end
 
-  
-  
 end

spec/api/abuse_spec.rb

+require 'spec_helper'
+
+def create_comment_flag(comment_id, user_id)
+  create_flag("/api/v1/comments/" + comment_id + "/abuse_flag", user_id)
+end
+
+def create_thread_flag(thread_id, user_id)
+  create_flag("/api/v1/threads/" + thread_id + "/abuse_flag", user_id)
+end
+
+def remove_thread_flag(thread_id, user_id)
+  remove_flag("/api/v1/threads/" + thread_id + "/abuse_unflag", user_id)
+end
+
+def remove_comment_flag(comment_id, user_id)
+  remove_flag("/api/v1/comments/" + comment_id + "/abuse_unflag", user_id)
+end
+
+def create_flag(put_command, user_id)
+  if user_id.nil?
+    put put_command
+  else
+    put put_command, user_id: user_id
+  end
+end
+
+def remove_flag(put_command, user_id)
+  if user_id.nil?
+    put put_command
+  else
+    put put_command, user_id: user_id
+  end
+end
+
+describe "app" do
+  describe "abuse" do
+    before(:each) { init_without_subscriptions }
+    describe "flag a comment as abusive" do
+      it "create or update the abuse_flags on the comment" do
+        comment = Comment.first
+        
+        # We get the count rather than just keeping the array, because the array
+        # will update as the Comment updates since the IdentityMap is enabled.
+        prev_abuse_flaggers_count = comment.abuse_flaggers.length
+        create_comment_flag("#{comment.id}", User.first.id)
+
+        comment = Comment.find(comment.id)
+        comment.abuse_flaggers.count.should == prev_abuse_flaggers_count + 1
+        # verify that the thread doesn't automatically get flagged
+        comment.comment_thread.abuse_flaggers.length.should == 0
+      end
+      it "returns 400 when the comment does not exist" do
+        create_comment_flag("does_not_exist", User.first.id)
+        last_response.status.should == 400
+      end
+      it "returns 400 when user_id is not provided" do
+        create_comment_flag("#{Comment.first.id}", nil)
+        last_response.status.should == 400
+      end
+      #Would like to test the output of to_hash, but not sure how to deal with a Moped::BSON::Document object
+      #it "has a correct hash" do
+      #  create_flag("#{Comment.first.id}", User.first.id)
+      #  Comment.first.to_hash
+      #end
+    end
+    describe "flag a thread as abusive" do
+      it "create or update the abuse_flags on the comment" do
+        comment = Comment.first
+        thread = comment.comment_thread
+        prev_abuse_flaggers_count = thread.abuse_flaggers.count
+        create_thread_flag("#{thread.id}", User.first.id)
+
+        comment = Comment.find(comment.id)
+        comment.comment_thread.abuse_flaggers.count.should == prev_abuse_flaggers_count + 1
+        # verify that the comment doesn't automatically get flagged
+        comment.abuse_flaggers.length.should == 0
+      end
+      it "returns 400 when the thread does not exist" do
+        create_thread_flag("does_not_exist", User.first.id)
+        last_response.status.should == 400
+      end
+      it "returns 400 when user_id is not provided" do
+        create_thread_flag("#{Comment.first.comment_thread.id}", nil)
+        last_response.status.should == 400
+      end
+      #Would like to test the output of to_hash, but not sure how to deal with a Moped::BSON::Document object
+      #it "has a correct hash" do
+      #  create_thread_flag("#{Comment.first.comment_thread.id}", User.first.id)
+      #  Comment.first.comment_thread.to_hash
+      #end
+    end
+    describe "unflag a comment as abusive" do
+      it "removes the user from the existing abuse_flaggers" do
+        comment = Comment.first
+        create_comment_flag("#{comment.id}", User.first.id)
+        
+        comment = Comment.first
+        prev_abuse_flaggers       = comment.abuse_flaggers
+        prev_abuse_flaggers_count = prev_abuse_flaggers.count
+        
+        prev_abuse_flaggers.should include User.first.id 
+        
+        remove_comment_flag("#{comment.id}", User.first.id)
+
+        comment = Comment.find(comment.id)
+        comment.abuse_flaggers.count.should == prev_abuse_flaggers_count - 1 
+        comment.abuse_flaggers.to_a.should_not include User.first.id
+      end
+      it "returns 400 when the thread does not exist" do
+        remove_comment_flag("does_not_exist", User.first.id)
+        last_response.status.should == 400
+      end
+      it "returns 400 when user_id is not provided" do
+        remove_comment_flag("#{Comment.first.comment_thread.id}", nil)
+        last_response.status.should == 400
+      end
+      #Would like to test the output of to_hash, but not sure how to deal with a Moped::BSON::Document object
+      #it "has a correct hash" do
+      #  create_thread_flag("#{Comment.first.comment_thread.id}", User.first.id)
+      #  Comment.first.comment_thread.to_hash
+      #end
+    end
+    describe "unflag a thread as abusive" do
+      it "removes the user from the existing abuse_flaggers" do
+        thread = CommentThread.first
+        create_thread_flag("#{thread.id}", User.first.id)
+        
+        thread = CommentThread.first
+        prev_abuse_flaggers = thread.abuse_flaggers
+        prev_abuse_flaggers_count = prev_abuse_flaggers.count
+        
+        prev_abuse_flaggers.should include User.first.id 
+        
+        remove_thread_flag("#{thread.id}", User.first.id)
+
+        thread = CommentThread.find(thread.id)
+        thread.abuse_flaggers.count.should == prev_abuse_flaggers_count - 1 
+        thread.abuse_flaggers.to_a.should_not include User.first.id
+      end
+      it "returns 400 when the thread does not exist" do
+        remove_thread_flag("does_not_exist", User.first.id)
+        last_response.status.should == 400
+      end
+      it "returns 400 when user_id is not provided" do
+        remove_thread_flag("#{Comment.first.comment_thread.id}", nil)
+        last_response.status.should == 400
+      end
+      #Would like to test the output of to_hash, but not sure how to deal with a Moped::BSON::Document object
+      #it "has a correct hash" do
+      #  create_thread_flag("#{Comment.first.comment_thread.id}", User.first.id)
+      #  Comment.first.comment_thread.to_hash
+      #end
+    end
+  end
+end

spec/api/comment_thread_spec.rb

@@ -16,6 +16,28 @@ describe "app" do
         thread.commentable_id.should == response_thread["commentable_id"]
         response_thread["children"].should be_nil
       end
+
+      # This is a test to ensure that the username is included even if the
+      # thread's author is the one looking at the comment. This is because of a
+      # regression in which we used User.only(:id, :read_states). This worked
+      # before we included the identity map, but afterwards, the user was
+      # missing the username and was not refetched.
+      it "includes the username even if the thread is being marked as read for the thread author" do
+        thread = CommentThread.first
+        expected_username = thread.author.username
+
+        # We need to clear the IdentityMap after getting the expected data to
+        # ensure that this spec fails when it should. If we don't do this, then
+        # in the cases where the User is fetched without its username, the spec
+        # won't fail because the User will already be in the identity map. 
+        Mongoid::IdentityMap.clear
+
+        get "/api/v1/threads/#{thread.id}", {:user_id => thread.author_id, :mark_as_read => true}
+        last_response.should be_ok
+        response_thread = parse last_response.body
+        response_thread["username"].should == expected_username
+      end
+
       it "get information of a single comment thread with its comments" do
         thread = CommentThread.first
         get "/api/v1/threads/#{thread.id}", recursive: true
@@ -29,6 +51,7 @@ describe "app" do
         response_thread["children"].length.should == thread.root_comments.length
         response_thread["children"].index{|c| c["body"] == thread.root_comments.first.body}.should_not be_nil
       end
+
       it "returns 400 when the thread does not exist" do
         get "/api/v1/threads/does_not_exist"
         last_response.status.should == 400
@@ -62,7 +85,7 @@ describe "app" do
         changed_thread = CommentThread.find(thread.id)
         changed_thread.body.should == "new body"
         changed_thread.title.should == "new title"
-        changed_thread.commentable_id.should == "new commentable_id"
+        changed_thread.commentable_id.should == "new_commentable_id"
       end
       it "returns 400 when the thread does not exist" do
         put "/api/v1/threads/does_not_exist", body: "new body", title: "new title"

spec/models/at_user_observer_spec.rb

 require 'spec_helper'
 
-describe AtUserObserver do
-  before :each do
-    @text =
-"""
-hi @tom, I have a question from @pi314 about the following code:
-```
-class A
-  def set_some_variable
-    @some_variable = 1
-  end
-end
-```
-and also the following code
-    class A
-      def get_some_variable
-        @some_variable
-      end
-    end
-what is the 'at' symbol doing there? @dementrock
-"""
-    User.delete_all
-    User.create!(external_id: "1", username: "tom", email: "tom@test.com")
-    User.create!(external_id: "2", username: "pi314", email: "pi314@test.com")
-  end
-
-  describe "#get_marked_text(text)" do
-    it "returns marked at text" do
-      converted = AtUserObserver.send :get_marked_text, @text
-      converted.should include "@tom_0"
-      converted.should include "@pi314_1"
-      converted.should include "@some_variable_2"
-      converted.should include "@some_variable_3"
-      converted.should include "@dementrock_4"
-    end
-  end
-
-  describe "#get_valid_at_position_list(text)" do
-    it "returns the list of positions for the valid @ notifications, filtering out the ones in code blocks" do
-      list = AtUserObserver.send :get_valid_at_position_list, @text
-      list.should include({ position: 0, username: "tom", user_id: "1" })
-      list.should include({ position: 1, username: "pi314", user_id: "2" })
-      list.length.should == 2
-    end
-  end
-end
+# Commenting out until notifications are used again.
+#
+#describe AtUserObserver do
+#  before :each do
+#    @text =
+#"""
+#hi @tom, I have a question from @pi314 about the following code:
+#```
+#class A
+#  def set_some_variable
+#    @some_variable = 1
+#  end
+#end
+#```
+#and also the following code
+#    class A
+#      def get_some_variable
+#        @some_variable
+#      end
+#    end
+#what is the 'at' symbol doing there? @dementrock
+#"""
+#    User.delete_all
+#    User.create!(external_id: "1", username: "tom", email: "tom@test.com")
+#    User.create!(external_id: "2", username: "pi314", email: "pi314@test.com")
+#  end
+#
+#  describe "#get_marked_text(text)" do
+#    it "returns marked at text" do
+#      converted = AtUserObserver.send :get_marked_text, @text
+#      converted.should include "@tom_0"
+#      converted.should include "@pi314_1"
+#      converted.should include "@some_variable_2"
+#      converted.should include "@some_variable_3"
+#      converted.should include "@dementrock_4"
+#    end
+#  end
+#
+#  describe "#get_valid_at_position_list(text)" do
+#    it "returns the list of positions for the valid @ notifications, filtering out the ones in code blocks" do
+#      list = AtUserObserver.send :get_valid_at_position_list, @text
+#      list.should include({ position: 0, username: "tom", user_id: "1" })
+#      list.should include({ position: 1, username: "pi314", user_id: "2" })
+#      list.length.should == 2
+#    end
+#  end
+#end

spec/spec_helper.rb

@@ -20,6 +20,10 @@ end
 
 RSpec.configure do |config|
   config.include Rack::Test::Methods
+  config.treat_symbols_as_metadata_keys_with_true_values = true
+  config.filter_run focus: true
+  config.run_all_when_everything_filtered = true
+  config.before(:each) { Mongoid::IdentityMap.clear }
 end
 
 Mongoid.configure do |config|