Merge hotfix/speedups_5_12_new

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
 

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/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)
 
   {
@@ -40,7 +51,7 @@ get "#{APIPREFIX}/users/:user_id/active_threads" do |user_id|
     num_pages: num_pages,
     page: page,
   }.to_json
-  
+
 end
 
 put "#{APIPREFIX}/users/:user_id" do |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'

models/comment.rb

@@ -39,6 +39,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)
@@ -49,15 +65,22 @@ 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)

models/comment_thread.rb

@@ -172,7 +172,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])
-                     .merge("id" => _id, "user_id" => author.id,
+                     .merge("id" => _id, "user_id" => author_id,
                             "username" => author.username,
                             "votes" => votes.slice(*%w[count up_count down_count point]),
                             "abuse_flaggers" => abuse_flaggers,

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

spec/api/subscription_and_notification_spec.rb

 require 'spec_helper'
 
-describe "app" do
-  describe "subscriptions and notifications" do
-    before(:each) { init_with_subscriptions }
-    describe "GET /api/v1/users/:user_id/notifications" do
-      it "get all notifications on the subscribed comment threads for the user" do
-        user = User.find("1")
-        get "/api/v1/users/#{user.external_id}/notifications"
-        last_response.should be_ok
-        notifications = parse last_response.body
-        so_easy = Comment.all.select{|c| c.body == "this problem is so easy"}.first
-        not_for_me_neither = Comment.all.select{|c| c.body == "not for me neither!"}.first
-        notification_so_easy = notifications.select{|f| f["notification_type"] == "post_reply" and f["info"]["comment_id"] == so_easy.id.to_s}.first
-        notification_so_easy.should_not be_nil
-        notification_not_for_me_neither = notifications.select{|f| f["notification_type"] == "post_reply" and f["info"]["comment_id"] == not_for_me_neither.id.to_s}.first
-        notification_not_for_me_neither.should_not be_nil
-      end
-      it "returns error if user does not exist" do #TODO may change later if have user service
-        get "/api/v1/users/does_not_exist/notifications"
-        last_response.status.should == 400
-      end
-      it "get all notifications on the subscribed commentable for the user" do
-        user = User.find("1")
-        get "/api/v1/users/#{user.external_id}/notifications"
-        last_response.should be_ok
-        notifications = parse last_response.body
-        notifications.select{|f| f["notification_type"] == "post_topic"}.length.should == 1
-        problem_wrong = notifications.select{|f| f["notification_type"] == "post_topic"}.first
-        problem_wrong["info"]["thread_title"].should == "This problem is wrong"
-      end
-      it "get all notifications on the followed user for the user" do
-        user = User.find("2")
-        get "/api/v1/users/#{user.external_id}/notifications"
-        last_response.should be_ok
-        notifications = parse last_response.body
-        notifications.select{|f| f["info"]["thread_title"] =~ /what to say/}.first.should_not be_nil
-      end
-    end
-    describe "POST /api/v1/users/:user_id/subscriptions" do
-      it "follow user" do
-        user1 = User.find("1")
-        user2 = User.find("2")
-        post "/api/v1/users/#{user1.external_id}/subscriptions", source_type: "user", source_id: user2.external_id
-        last_response.should be_ok
-        User.find("2").followers.length.should == 1
-        User.find("2").followers.should include user1
-      end
-      it "does not follow the same user twice" do
-        user1 = User.find("1")
-        user2 = User.find("2")
-        post "/api/v1/users/#{user1.external_id}/subscriptions", source_type: "user", source_id: user2.external_id
-        post "/api/v1/users/#{user1.external_id}/subscriptions", source_type: "user", source_id: user2.external_id
-        last_response.should be_ok
-        User.find("2").followers.length.should == 1
-      end
-      it "does not follow oneself" do
-        user = create_test_user(3)
-        post "/api/v1/users/#{user.external_id}/subscriptions", source_type: "user", source_id: user.external_id
-        last_response.status.should == 400
-        user.reload.followers.length.should == 0
-      end
-      it "unfollow user" do
-        user1 = User.find("1")
-        user2 = User.find("2")
-        delete "/api/v1/users/#{user2.external_id}/subscriptions", source_type: "user", source_id: user1.external_id
-        last_response.should be_ok
-        User.find("1").followers.length.should == 0
-      end
-      it "respond ok when unfollowing user twice" do
-        user1 = User.find("1")
-        user2 = User.find("2")
-        delete "/api/v1/users/#{user2.external_id}/subscriptions", source_type: "user", source_id: user1.external_id
-        delete "/api/v1/users/#{user2.external_id}/subscriptions", source_type: "user", source_id: user1.external_id
-        last_response.should be_ok
-        User.find("1").followers.length.should == 0
-      end
-      it "subscribe a commentable" do
-        user3 = create_test_user(3)
-        post "/api/v1/users/#{user3.external_id}/subscriptions", source_type: "other", source_id: "question_1"
-        last_response.should be_ok
-        Commentable.find("question_1").subscribers.length.should == 3
-        Commentable.find("question_1").subscribers.should include user3
-      end
-      it "unsubscribe a commentable" do
-        user2 = User.find_by(external_id: "2")
-        delete "/api/v1/users/#{user2.external_id}/subscriptions", source_type: "other", source_id: "question_1"
-        last_response.should be_ok
-        Commentable.find("question_1").subscribers.length.should == 1
-        Commentable.find("question_1").subscribers.should_not include user2
-      end
-      it "subscribe a comment thread" do
-        user1 = User.find_by(external_id: "1")
-        thread = CommentThread.where(body: "it is unsolvable").first
-        post "/api/v1/users/#{user1.external_id}/subscriptions", source_type: "thread", source_id: thread.id
-        last_response.should be_ok
-        thread = CommentThread.where(body: "it is unsolvable").first
-        thread.subscribers.length.should == 2
-        thread.subscribers.should include user1
-      end
-      it "unsubscribe a comment thread" do
-        user2 = User.find_by(external_id: "2")
-        thread = CommentThread.where(body: "it is unsolvable").first
-        delete "/api/v1/users/#{user2.external_id}/subscriptions", source_type: "thread", source_id: thread.id
-        last_response.should be_ok
-        thread = CommentThread.where(body: "it is unsolvable").first
-        thread.subscribers.length.should == 0
-      end
-    end
-  end
-end
+# Commenting out until notifications are used again.
+#
+#describe "app" do
+#  describe "subscriptions and notifications" do
+#    before(:each) { init_with_subscriptions }
+#    describe "GET /api/v1/users/:user_id/notifications" do
+#      it "get all notifications on the subscribed comment threads for the user" do
+#        user = User.find("1")
+#        get "/api/v1/users/#{user.external_id}/notifications"
+#        last_response.should be_ok
+#        notifications = parse last_response.body
+#        so_easy = Comment.all.select{|c| c.body == "this problem is so easy"}.first
+#        not_for_me_neither = Comment.all.select{|c| c.body == "not for me neither!"}.first
+#        notification_so_easy = notifications.select{|f| f["notification_type"] == "post_reply" and f["info"]["comment_id"] == so_easy.id.to_s}.first
+#        notification_so_easy.should_not be_nil
+#        notification_not_for_me_neither = notifications.select{|f| f["notification_type"] == "post_reply" and f["info"]["comment_id"] == not_for_me_neither.id.to_s}.first
+#        notification_not_for_me_neither.should_not be_nil
+#      end
+#      it "returns error if user does not exist" do #TODO may change later if have user service
+#        get "/api/v1/users/does_not_exist/notifications"
+#        last_response.status.should == 400
+#      end
+#      it "get all notifications on the subscribed commentable for the user" do
+#        user = User.find("1")
+#        get "/api/v1/users/#{user.external_id}/notifications"
+#        last_response.should be_ok
+#        notifications = parse last_response.body
+#        notifications.select{|f| f["notification_type"] == "post_topic"}.length.should == 1
+#        problem_wrong = notifications.select{|f| f["notification_type"] == "post_topic"}.first
+#        problem_wrong["info"]["thread_title"].should == "This problem is wrong"
+#      end
+#      it "get all notifications on the followed user for the user" do
+#        user = User.find("2")
+#        get "/api/v1/users/#{user.external_id}/notifications"
+#        last_response.should be_ok
+#        notifications = parse last_response.body
+#        notifications.select{|f| f["info"]["thread_title"] =~ /what to say/}.first.should_not be_nil
+#      end
+#    end
+#    describe "POST /api/v1/users/:user_id/subscriptions" do
+#      it "follow user" do
+#        user1 = User.find("1")
+#        user2 = User.find("2")
+#        post "/api/v1/users/#{user1.external_id}/subscriptions", source_type: "user", source_id: user2.external_id
+#        last_response.should be_ok
+#        User.find("2").followers.length.should == 1
+#        User.find("2").followers.should include user1
+#      end
+#      it "does not follow the same user twice" do
+#        user1 = User.find("1")
+#        user2 = User.find("2")
+#        post "/api/v1/users/#{user1.external_id}/subscriptions", source_type: "user", source_id: user2.external_id
+#        post "/api/v1/users/#{user1.external_id}/subscriptions", source_type: "user", source_id: user2.external_id
+#        last_response.should be_ok
+#        User.find("2").followers.length.should == 1
+#      end
+#      it "does not follow oneself" do
+#        user = create_test_user(3)
+#        post "/api/v1/users/#{user.external_id}/subscriptions", source_type: "user", source_id: user.external_id
+#        last_response.status.should == 400
+#        user.reload.followers.length.should == 0
+#      end
+#      it "unfollow user" do
+#        user1 = User.find("1")
+#        user2 = User.find("2")
+#        delete "/api/v1/users/#{user2.external_id}/subscriptions", source_type: "user", source_id: user1.external_id
+#        last_response.should be_ok
+#        User.find("1").followers.length.should == 0
+#      end
+#      it "respond ok when unfollowing user twice" do
+#        user1 = User.find("1")
+#        user2 = User.find("2")
+#        delete "/api/v1/users/#{user2.external_id}/subscriptions", source_type: "user", source_id: user1.external_id
+#        delete "/api/v1/users/#{user2.external_id}/subscriptions", source_type: "user", source_id: user1.external_id
+#        last_response.should be_ok
+#        User.find("1").followers.length.should == 0
+#      end
+#      it "subscribe a commentable" do
+#        user3 = create_test_user(3)
+#        post "/api/v1/users/#{user3.external_id}/subscriptions", source_type: "other", source_id: "question_1"
+#        last_response.should be_ok
+#        Commentable.find("question_1").subscribers.length.should == 3
+#        Commentable.find("question_1").subscribers.should include user3
+#      end
+#      it "unsubscribe a commentable" do
+#        user2 = User.find_by(external_id: "2")
+#        delete "/api/v1/users/#{user2.external_id}/subscriptions", source_type: "other", source_id: "question_1"
+#        last_response.should be_ok
+#        Commentable.find("question_1").subscribers.length.should == 1
+#        Commentable.find("question_1").subscribers.should_not include user2
+#      end
+#      it "subscribe a comment thread" do
+#        user1 = User.find_by(external_id: "1")
+#        thread = CommentThread.where(body: "it is unsolvable").first
+#        post "/api/v1/users/#{user1.external_id}/subscriptions", source_type: "thread", source_id: thread.id
+#        last_response.should be_ok
+#        thread = CommentThread.where(body: "it is unsolvable").first
+#        thread.subscribers.length.should == 2
+#        thread.subscribers.should include user1
+#      end
+#      it "unsubscribe a comment thread" do
+#        user2 = User.find_by(external_id: "2")
+#        thread = CommentThread.where(body: "it is unsolvable").first
+#        delete "/api/v1/users/#{user2.external_id}/subscriptions", source_type: "thread", source_id: thread.id
+#        last_response.should be_ok
+#        thread = CommentThread.where(body: "it is unsolvable").first
+#        thread.subscribers.length.should == 0
+#      end
+#    end
+#  end
+#end

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

@@ -23,6 +23,7 @@ RSpec.configure do |config|
   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|