add error handling for validation & more specs

app.rb

@@ -27,8 +27,6 @@ get '/api/v1/search/threads' do
     with(:commentable_id, params["commentable_id"]) if params["commentable_id"]
     with(:tags).all_of(params["tags"].split /,/) if params["tags"]
   end.results
-  puts params["tags"].split /,/
-  puts results
   results.map(&:to_hash).to_json
 end
 
@@ -45,11 +43,13 @@ post '/api/v1/:commentable_id/threads' do |commentable_id|
   thread = CommentThread.new(params.slice(*%w[title body course_id]).merge(commentable_id: commentable_id))
   thread.tags = params["tags"] || ""
   thread.author = user
-  thread.save!
-  if params["auto_subscribe"] and author
-    author.subscribe(thread)
-  end
+  thread.save
+  if thread.errors.any?
+    error 400, thread.errors.messages.to_json
+  else
+    author.subscribe(thread) if params["auto_subscribe"] and author
     thread.to_hash.to_json
+  end
 end
 
 get '/api/v1/threads/tags' do
@@ -57,7 +57,7 @@ get '/api/v1/threads/tags' do
 end
 
 get '/api/v1/threads/tags/autocomplete' do
-  CommentThread.tags_autocomplete(params["value"], max: 5, sort_by_count: true).map(&:first).to_json
+  CommentThread.tags_autocomplete(params["value"].strip, max: 5, sort_by_count: true).map(&:first).to_json
 end
 
 get '/api/v1/threads/:thread_id' do |thread_id|
@@ -65,22 +65,28 @@ get '/api/v1/threads/:thread_id' do |thread_id|
 end
 
 put '/api/v1/threads/:thread_id' do |thread_id|
-  thread.update_attributes!(params.slice(*%w[title body]))
+  thread.update_attributes(params.slice(*%w[title body]))
   if params["tags"]
     thread.tags = params["tags"]
-    thread.save!
+    thread.save
   end
+  if thread.errors.any?
+    error 400, thread.errors.messages.to_json
+  else
     thread.to_hash.to_json
+  end
 end
 
 post '/api/v1/threads/:thread_id/comments' do |thread_id|
   comment = thread.comments.new(params.slice(*%w[body course_id]))
   comment.author = user 
-  comment.save!
-  if params["auto_subscribe"] and author
-    author.subscribe(thread)
-  end
+  comment.save
+  if comment.errors.any?
+    error 400, comment.errors.messages.to_json
+  else
+    author.subscribe(thread) if params["auto_subscribe"] and author
     comment.to_hash.to_json
+  end
 end
 
 delete '/api/v1/threads/:thread_id' do |thread_id|
@@ -93,16 +99,24 @@ get '/api/v1/comments/:comment_id' do |comment_id|
 end
 
 put '/api/v1/comments/:comment_id' do |comment_id|
-  comment.update_attributes!(params.slice(*%w[body endorsed]))
+  comment.update_attributes(params.slice(*%w[body endorsed]))
+  if comment.errors.any?
+    error 400, comment.errors.messages.to_json
+  else
     comment.to_hash.to_json
+  end
 end
 
 post '/api/v1/comments/:comment_id' do |comment_id|
   sub_comment = comment.children.new(params.slice(*%w[body course_id]))
   sub_comment.author = user
   sub_comment.comment_thread = comment.comment_thread
-  sub_comment.save!
+  sub_comment.save
+  if sub_comment.errors.any?
+    error 400, sub_comment.errors.messages.to_json
+  else
     sub_comment.to_hash.to_json
+  end
 end
 
 delete '/api/v1/comments/:comment_id' do |comment_id|
@@ -164,3 +178,7 @@ end
 error ValueError do
   error 400, env['sinatra.error'].message
 end
+
+error Mongoid::Errors::Validations do
+  error 400, env['sinatra.error'].message
+end

lib/helpers.rb

@@ -23,13 +23,15 @@ helpers do
       CommentThread.find(params["source_id"])
     when "other"
       Commentable.find(params["source_id"])
+    else
+      raise ValueError, "Source type must be 'user', 'thread' or 'other'"
     end
   end
 
   def vote_for(obj)
-    raise ValueError, "must provide user id" unless user
-    raise ValueError, "must provide value" unless params["value"]
-    raise ValueError, "must provide valid value" unless %w[up down].include? params["value"]
+    raise ValueError, "User id is required" unless user
+    raise ValueError, "Value is required" unless params["value"]
+    raise ValueError, "Value is invalid" unless %w[up down].include? params["value"]
     user.vote(obj, params["value"].to_sym)
     obj.reload.to_hash.to_json
   end

lib/utils.rb

@@ -2,7 +2,4 @@ class NilClass
   def to_hash
     {}
   end
-
-  def destroy
-  end
 end

models/user.rb

@@ -59,7 +59,7 @@ class User
 
   def subscribe(source)
     if source._id == self._id and source.class == self.class
-      nil
+      raise ValueError, "Cannot follow oneself"
     else
       Subscription.find_or_create_by(subscriber_id: self._id.to_s, source_id: source._id.to_s, source_type: source.class.to_s)
     end
@@ -67,7 +67,7 @@ class User
 
   def unsubscribe(source)
     subscription = Subscription.where(subscriber_id: self._id.to_s, source_id: source._id.to_s, source_type: source.class.to_s).first
-    subscription.destroy
+    subscription.destroy if subscription
     subscription
   end
 

spec/api/comment_thread_spec.rb

@@ -81,10 +81,11 @@ describe "app" do
       end
     end
     describe "POST /api/v1/threads/:thread_id/comments" do
+      default_params = {body: "new comment", course_id: "1", user_id: User.first.id}
       it "create a comment to the comment thread" do
         thread = CommentThread.first.to_hash(recursive: true)
         user = User.first
-        post "/api/v1/threads/#{thread["id"]}/comments", body: "new comment", course_id: "1", user_id: User.first.id
+        post "/api/v1/threads/#{thread["id"]}/comments", default_params
         last_response.should be_ok
         changed_thread = CommentThread.find(thread["id"]).to_hash(recursive: true)
         changed_thread["children"].length.should == thread["children"].length + 1
@@ -94,7 +95,7 @@ describe "app" do
       end
       it "allows anonymous comment" do
         thread = CommentThread.first.to_hash(recursive: true)
-        post "/api/v1/threads/#{thread["id"]}/comments", body: "new comment", course_id: "1", user_id: nil
+        post "/api/v1/threads/#{thread["id"]}/comments", default_params.merge(user_id: nil)
         last_response.should be_ok
         changed_thread = CommentThread.find(thread["id"]).to_hash(recursive: true)
         changed_thread["children"].length.should == thread["children"].length + 1
@@ -102,7 +103,15 @@ describe "app" do
         comment.should_not be_nil
       end
       it "returns 400 when the thread does not exist" do
-        post "/api/v1/threads/does_not_exist/comments", body: "new comment", course_id: "1", user_id: User.first.id
+        post "/api/v1/threads/does_not_exist/comments", default_params
+        last_response.status.should == 400
+      end
+      it "returns error when body or course_id does not exist, or when body is blank" do
+        post "/api/v1/threads/#{CommentThread.first.id}/comments", default_params.merge(body: nil)
+        last_response.status.should == 400
+        post "/api/v1/threads/#{CommentThread.first.id}/comments", default_params.merge(course_id: nil)
+        last_response.status.should == 400
+        post "/api/v1/threads/#{CommentThread.first.id}/comments", default_params.merge(body: "    \n      \n  ")
         last_response.status.should == 400
       end
     end

spec/api/commentable_spec.rb

@@ -71,6 +71,26 @@ describe "app" do
         Commentable.find("does_not_exist").comment_threads.length.should == 1
         Commentable.find("does_not_exist").comment_threads.first.body.should == "cool"
       end
+      it "returns error when title, body or course id does not exist" do
+        params = default_params.dup
+        params.delete(:title)
+        post '/api/v1/question_1/threads', params
+        last_response.status.should == 400
+        params = default_params.dup
+        params.delete(:body)
+        post '/api/v1/question_1/threads', params
+        last_response.status.should == 400
+        params = default_params.dup
+        params.delete(:course_id)
+        post '/api/v1/question_1/threads', params
+        last_response.status.should == 400
+      end
+      it "returns error when title or body is blank (only consists of spaces and new lines)" do
+        post '/api/v1/question_1/threads', default_params.merge(title: "     ")
+        last_response.status.should == 400
+        post '/api/v1/question_1/threads', default_params.merge(body: "     \n    \n")
+        last_response.status.should == 400
+      end
       it "create a new comment thread with tag" do
         post '/api/v1/question_1/threads', default_params.merge(tags: "a, b, c")
         last_response.should be_ok

spec/api/subscription_and_notification_spec.rb

@@ -16,6 +16,10 @@ describe "app" do
         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 empty array if user does not exist" do #TODO may change later if have user service
+        get "/api/v1/users/does_not_exist/notifications"
+        parse(last_response.body).length.should == 0
+      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"
@@ -25,7 +29,13 @@ describe "app" do
         problem_wrong = notifications.select{|f| f["notification_type"] == "post_topic"}.first
         problem_wrong["info"]["thread_title"].should == "This problem is wrong"
       end
-      # TODO spec for followed user
+      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
@@ -36,6 +46,20 @@ describe "app" do
         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 = User.find_or_create_by(external_id: "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")
@@ -43,6 +67,14 @@ describe "app" do
         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 = User.find_or_create_by(external_id: "3")
         post "/api/v1/users/#{user3.external_id}/subscriptions", source_type: "other", source_id: "question_1"

spec/spec_helper.rb

@@ -134,4 +134,8 @@ def init_with_subscriptions
   thread.author = user2
   user2.subscribe(thread)
   thread.save!
+
+  thread = CommentThread.new(title: "I don't know what to say", body: "lol", course_id: "2", commentable_id: "something else")
+  thread.author = user1
+  thread.save!
 end