Merge pull request #163 from edx/clintonb/test-improvements

Model and Spec Helper Cleanup

models/comment.rb

@@ -6,6 +6,8 @@ class Comment < Content
   include Mongoid::Timestamps
   include Mongoid::MagicCounterCache
   include ActiveModel::MassAssignmentSecurity
+  include Tire::Model::Search
+  include Tire::Model::Callbacks
 
   voteable self, :up => +1, :down => -1
 
@@ -17,21 +19,11 @@ class Comment < Content
   field :anonymous_to_peers, type: Boolean, default: false
   field :commentable_id, type: String
   field :at_position_list, type: Array, default: []
+  field :sk, type: String, default: nil
 
   index({author_id: 1, course_id: 1})
   index({_type: 1, comment_thread_id: 1, author_id: 1, updated_at: 1})
 
-  field :sk, type: String, default: nil
-  before_save :set_sk  
-  def set_sk()
-    # this attribute is explicitly write-once
-    if self.sk.nil?
-      self.sk = (self.parent_ids.dup << self.id).join("-") 
-    end
-  end
-
-  include Tire::Model::Search
-  include Tire::Model::Callbacks
 
   index_name Content::ES_INDEX_NAME
 
@@ -45,10 +37,10 @@ class Comment < Content
     indexes :created_at, type: :date, included_in_all: false
     indexes :updated_at, type: :date, included_in_all: false
   end
-  
+
 
   belongs_to :comment_thread, index: true
-  belongs_to :author, class_name: "User", index: true
+  belongs_to :author, class_name: 'User', index: true
 
   attr_accessible :body, :course_id, :anonymous, :anonymous_to_peers, :endorsed, :endorsement
 
@@ -59,13 +51,13 @@ class Comment < Content
 
   counter_cache :comment_thread
 
-  before_destroy :destroy_children # TODO async
-
+  before_destroy :destroy_children
   before_create :set_thread_last_activity_at
   before_update :set_thread_last_activity_at
+  before_save :set_sk
 
   def self.hash_tree(nodes)
-    nodes.map{|node, sub_nodes| node.to_hash.merge("children" => hash_tree(sub_nodes).compact)}
+    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
@@ -76,9 +68,9 @@ class Comment < Content
   # actually creates the subtree.
   def self.flatten_subtree(x)
     if x.is_a? Array
-      x.flatten.map{|y| self.flatten_subtree(y)}
+      x.flatten.map { |y| self.flatten_subtree(y) }
     elsif x.is_a? Hash
-      x.to_a.map{|y| self.flatten_subtree(y)}.flatten
+      x.to_a.map { |y| self.flatten_subtree(y) }.flatten
     else
       x
     end
@@ -99,20 +91,20 @@ class Comment < Content
       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')
     end
   end
-  
+
   def commentable_id
     #we need this to have a universal access point for the flag rake task
     if self.comment_thread_id
@@ -149,16 +141,22 @@ class Comment < Content
   end
 
   def self.by_date_range_and_thread_ids from_when, to_when, thread_ids
-     #return all content between from_when and to_when
+    #return all content between from_when and to_when
 
-     self.where(:created_at.gte => (from_when)).where(:created_at.lte => (to_when)).
-       where(:comment_thread_id.in => thread_ids)
+    self.where(:created_at.gte => (from_when)).where(:created_at.lte => (to_when)).
+        where(:comment_thread_id.in => thread_ids)
   end
-  
-private
+
+  private
 
   def set_thread_last_activity_at
     self.comment_thread.update_attribute(:last_activity_at, Time.now.utc)
   end
 
+  def set_sk
+    # this attribute is explicitly write-once
+    if self.sk.nil?
+      self.sk = (self.parent_ids.dup << self.id).join("-")
+    end
+  end
 end

models/comment_thread.rb

@@ -7,6 +7,8 @@ class CommentThread < Content
   include Mongoid::Timestamps
   include Mongoid::Attributes::Dynamic
   include ActiveModel::MassAssignmentSecurity
+  include Tire::Model::Search
+  include Tire::Model::Callbacks
   extend Enumerize
 
   voteable self, :up => +1, :down => -1
@@ -30,8 +32,6 @@ class CommentThread < Content
 
   index({author_id: 1, course_id: 1})
 
-  include Tire::Model::Search
-  include Tire::Model::Callbacks
 
   index_name Content::ES_INDEX_NAME
 
@@ -50,12 +50,12 @@ class CommentThread < Content
     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
     indexes :group_id, type: :integer, as: 'group_id', index: :not_analyzed, included_in_all: false
-    indexes :id,         :index    => :not_analyzed
-    indexes :thread_id, :analyzer => :keyword, :as => "_id"
+    indexes :id, :index => :not_analyzed
+    indexes :thread_id, :analyzer => :keyword, :as => '_id'
   end
 
-  belongs_to :author, class_name: "User", inverse_of: :comment_threads, index: true#, autosave: true
-  has_many :comments, dependent: :destroy#, autosave: true# Use destroy to envoke callback on the top-level comments TODO async
+  belongs_to :author, class_name: 'User', inverse_of: :comment_threads, index: true
+  has_many :comments, dependent: :destroy # Use destroy to invoke callback on the top-level comments
   has_many :activities, autosave: true
 
   attr_accessible :title, :body, :course_id, :commentable_id, :anonymous, :anonymous_to_peers, :closed, :thread_type
@@ -71,24 +71,12 @@ class CommentThread < Content
   before_create :set_last_activity_at
   before_update :set_last_activity_at, :unless => lambda { closed_changed? }
   after_update :clear_endorsements
-
   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
-    c.title = options[:title] || "title"
-    c.body = options[:body] || "body"
-    c.commentable_id = options[:commentable_id] || "commentable_id"
-    c.course_id = options[:course_id] || "course_id"
-    c.author = options[:author] || User.first
-    c.save!
-    c
-  end
-
   def activity_since(from_time=nil)
     if from_time
       activities.where(:created_at => {:$gte => from_time})
@@ -97,13 +85,21 @@ class CommentThread < Content
     end
   end
 
-  def activity_today; activity_since(Date.today.to_time); end
+  def activity_today
+    activity_since(Date.today.to_time)
+  end
 
-  def activity_this_week; activity_since(Date.today.to_time - 1.weeks); end
+  def activity_this_week
+    activity_since(Date.today.to_time - 1.weeks)
+  end
 
-  def activity_this_month; activity_since(Date.today.to_time - 1.months); end
+  def activity_this_month
+    activity_since(Date.today.to_time - 1.months)
+  end
 
-  def activity_overall; activity_since(nil); end
+  def activity_overall
+    activity_since(nil)
+  end
 
   def root_comments
     Comment.roots.where(comment_thread_id: self.id)
@@ -127,24 +123,25 @@ class CommentThread < Content
 
   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 context])
-                     .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" => [],
-                            "type" => "thread",
-                            "group_id" => group_id,
-                            "pinned" => pinned?,
-                            "comments_count" => comment_count)
-    
+        .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' => [],
+               'type' => 'thread',
+               'group_id' => group_id,
+               'pinned' => pinned?,
+               'comments_count' => comment_count)
+
   end
 
   def comment_thread_id
     #so that we can use the comment thread id as a common attribute for flagging
     self.id
-  end  
-  
-private
+  end
+
+  private
 
   def set_last_activity_at
     self.last_activity_at = Time.now.utc unless last_activity_at_changed?
@@ -165,5 +162,4 @@ private
   def destroy_subscriptions
     subscriptions.delete_all
   end
-
 end

models/content.rb

 class Content
-  
+
   include Mongoid::Document
   include Mongo::Voteable
-  
+
   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
   field :author_username, type: String, default: nil
-  
-  index({_type: 1, course_id: 1, pinned: -1, created_at: -1 }, {background: true} )
+
+  index({_type: 1, course_id: 1, pinned: -1, created_at: -1}, {background: true})
   index({_type: 1, course_id: 1, pinned: -1, comment_count: -1, created_at: -1}, {background: true})
-  index({_type: 1, course_id: 1, pinned: -1, "votes.point" => -1, created_at: -1}, {background: true})
+  index({_type: 1, course_id: 1, pinned: -1, 'votes.point' => -1, created_at: -1}, {background: true})
   index({_type: 1, course_id: 1, pinned: -1, last_activity_at: -1, created_at: -1}, {background: true})
-
-  # To be added during Mongo Mania 
-  # index({_type: -1, course_id: 1, pinned: -1, created_at: -1 }, {background: true} )
-  # index({_type: -1, course_id: 1, pinned: -1, comment_count: -1, created_at: -1}, {background: true})
-  # index({_type: -1, course_id: 1, pinned: -1, "votes.point" => -1, created_at: -1}, {background: true})
-  # index({_type: -1, course_id: 1, pinned: -1, last_activity_at: -1, created_at: -1}, {background: true})
-
-
-
   index({comment_thread_id: 1, sk: 1}, {sparse: true})
   index({comment_thread_id: 1, endorsed: 1}, {sparse: true})
   index({commentable_id: 1}, {sparse: true, background: true})
@@ -36,10 +27,7 @@ class Content
   end
 
   before_save :set_username
-  def set_username
-    # avoid having to look this attribute up later, since it does not change
-    self.author_username = author.username
-  end
+
 
   def author_with_anonymity(attr=nil, attr_when_anonymous=nil)
     if not attr
@@ -52,7 +40,7 @@ class Content
   def self.flagged
     #return an array of flagged content
     holder = []
-    Content.where(:abuse_flaggers.ne => [],:abuse_flaggers.exists => true).each do |c|
+    Content.where(:abuse_flaggers.ne => [], :abuse_flaggers.exists => true).each do |c|
       holder << c
     end
     holder
@@ -62,61 +50,65 @@ class Content
     #take a hash of criteria (what) and return a hash of hashes
     #course => user => count
 
-    contributors = {}
-
-    map =  "function(){emit(this.author_id,1)}"
-    reduce  =  "function(k, vals) { var sum = 0; for(var i in vals) sum += vals[i]; return sum; }"
+    map = 'function(){emit(this.author_id,1)}'
+    reduce = 'function(k, vals) { var sum = 0; for(var i in vals) sum += vals[i]; return sum; }'
 
     contributors = []
-    self.where(what).map_reduce(map,reduce).out(replace: "results").each do |d|
+    self.where(what).map_reduce(map, reduce).out(replace: 'results').each do |d|
       contributors << d
     end
 
     #now sort and limit them
 
     #first sort destructively
-    contributors.sort! {|a,b| -a["value"] <=> -b["value"]}
+    contributors.sort! { |a, b| -a['value'] <=> -b['value'] }
     #then trim it
     contributors = contributors[0..(count - 1)]
 
     contributors
 
   end
-  
+
   def self.summary what
     #take a hash of criteria (what) and return a hash of hashes
     #of total users, votes, comments, endorsements, 
-    
+
     answer = {}
     vote_count = 0
     thread_count = 0
     comment_count = 0
     contributors = []
     content = self.where(what)
-    
+
     content.each do |c|
       contributors << c.author_id
-      contributors << c["votes"]["up"]
-      contributors << c["votes"]["down"]
-      vote_count += c["votes"]["count"]
-      if c._type == "CommentThread"
+      contributors << c['votes']['up']
+      contributors << c['votes']['down']
+      vote_count += c['votes']['count']
+      if c._type == 'CommentThread'
         thread_count += 1
-      elsif c._type == "Comment"
+      elsif c._type == 'Comment'
         comment_count += 1
       end
     end
 
     #uniquify contributors
     contributors = contributors.uniq
-    
+
     #assemble the answer and ship
-    
-    answer["vote_count"] = vote_count
-    answer["thread_count"] = thread_count
-    answer["comment_count"] = comment_count
-    answer["contributor_count"] = contributors.count
-    
+
+    answer['vote_count'] = vote_count
+    answer['thread_count'] = thread_count
+    answer['comment_count'] = comment_count
+    answer['contributor_count'] = contributors.count
+
     answer
   end
 
+  private
+
+  def set_username
+    # avoid having to look this attribute up later, since it does not change
+    self.author_username = author.username
+  end
 end

spec/spec_helper.rb

@@ -14,6 +14,9 @@ require 'rack/test'
 require 'yajl'
 require 'database_cleaner'
 
+require 'support/database_cleaner'
+require 'support/elasticsearch'
+
 # setup test environment
 set :environment, :test
 set :run, false
@@ -36,35 +39,13 @@ def set_api_key_header
   current_session.header "X-Edx-Api-Key", TEST_API_KEY
 end
 
-def delete_es_index
-  Tire.index Content::ES_INDEX_NAME do delete end
-end
 
-def create_es_index
-  new_index = Tire.index Content::ES_INDEX_NAME
-  new_index.create
-  [CommentThread, Comment].each do |klass|
-    klass.put_search_index_mapping
-  end
-end
-
-def refresh_es_index
-  # we are using the same index for two types, which is against the
-  # grain of Tire's design.  This is why this method works for both
-  # comment_threads and comments.
-  CommentThread.tire.index.refresh
-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) do
-    DatabaseCleaner.clean
-    delete_es_index
-    create_es_index
-  end
 end
 
 Mongoid.configure do |config|

spec/support/database_cleaner.rb

+require 'database_cleaner'
+
+RSpec.configure do |config|
+  config.before(:suite) do
+    # Mongoid only supports truncation.
+    DatabaseCleaner.strategy = :truncation
+    DatabaseCleaner.clean_with(:truncation)
+  end
+
+  config.around(:each) do |example|
+    DatabaseCleaner.cleaning do
+      example.run
+    end
+  end
+end

spec/support/elasticsearch.rb

+def delete_es_index
+  Tire.index Content::ES_INDEX_NAME do
+    delete
+  end
+end
+
+def create_es_index
+  new_index = Tire.index Content::ES_INDEX_NAME
+  new_index.create
+  [CommentThread, Comment].each do |klass|
+    klass.put_search_index_mapping
+  end
+end
+
+def refresh_es_index
+  es_index_name = Content::ES_INDEX_NAME
+  Tire.index es_index_name do
+    refresh
+  end
+end
+
+RSpec.configure do |config|
+  config.before(:each) do
+    delete_es_index
+    create_es_index
+  end
+end