Merge branch 'api/improve-error-reporting' of https://github.com/jubianchi/gitlabhq into jubianchi-api/improve-error-reporting

5 files changed, 237 insertions, 50 deletions

diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index e8eebda95b4..9876452f81d 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -169,6 +169,15 @@ describe API::API, api: true  do
       response.status.should == 400
       json_response['message']['labels']['?']['title'].should == ['is invalid']
     end
+
+    it 'should return 400 if title is too long' do
+      post api("/projects/#{project.id}/issues", user),
+           title: 'g' * 256
+      response.status.should == 400
+      json_response['message']['title'].should == [
+        'is too long (maximum is 255 characters)'
+      ]
+    end
   end
 
   describe "PUT /projects/:id/issues/:issue_id to update only title" do
@@ -237,6 +246,15 @@ describe API::API, api: true  do
       json_response['labels'].should include 'label_bar'
       json_response['labels'].should include 'label/bar'
     end
+
+    it 'should return 400 if title is too long' do
+      put api("/projects/#{project.id}/issues/#{issue.id}", user),
+          title: 'g' * 256
+      response.status.should == 400
+      json_response['message']['title'].should == [
+        'is too long (maximum is 255 characters)'
+      ]
+    end
   end
 
   describe "PUT /projects/:id/issues/:issue_id to update state and label" do
diff --git a/spec/requests/api/labels_spec.rb b/spec/requests/api/labels_spec.rb
index ee9088933a1..dbddc8a7da4 100644
--- a/spec/requests/api/labels_spec.rb
+++ b/spec/requests/api/labels_spec.rb
@@ -47,7 +47,7 @@ describe API::API, api: true  do
            name: 'Foo',
            color: '#FFAA'
       response.status.should == 400
-      json_response['message'].should == 'Color is invalid'
+      json_response['message']['color'].should == ['is invalid']
     end
 
     it 'should return 400 for too long color code' do
@@ -55,7 +55,7 @@ describe API::API, api: true  do
            name: 'Foo',
            color: '#FFAAFFFF'
       response.status.should == 400
-      json_response['message'].should == 'Color is invalid'
+      json_response['message']['color'].should == ['is invalid']
     end
 
     it 'should return 400 for invalid name' do
@@ -63,7 +63,7 @@ describe API::API, api: true  do
            name: '?',
            color: '#FFAABB'
       response.status.should == 400
-      json_response['message'].should == 'Title is invalid'
+      json_response['message']['title'].should == ['is invalid']
     end
 
     it 'should return 409 if label already exists' do
@@ -84,7 +84,7 @@ describe API::API, api: true  do
     it 'should return 404 for non existing label' do
       delete api("/projects/#{project.id}/labels", user), name: 'label2'
       response.status.should == 404
-      json_response['message'].should == 'Label not found'
+      json_response['message'].should == '404 Label Not Found'
     end
 
     it 'should return 400 for wrong parameters' do
@@ -132,11 +132,14 @@ describe API::API, api: true  do
     it 'should return 400 if no label name given' do
       put api("/projects/#{project.id}/labels", user), new_name: 'label2'
       response.status.should == 400
+      json_response['message'].should == '400 (Bad request) "name" not given'
     end
 
     it 'should return 400 if no new parameters given' do
       put api("/projects/#{project.id}/labels", user), name: 'label1'
       response.status.should == 400
+      json_response['message'].should == 'Required parameters '\
+                                         '"new_name" or "color" missing'
     end
 
     it 'should return 400 for invalid name' do
@@ -145,7 +148,7 @@ describe API::API, api: true  do
           new_name: '?',
           color: '#FFFFFF'
       response.status.should == 400
-      json_response['message'].should == 'Title is invalid'
+      json_response['message']['title'].should == ['is invalid']
     end
 
     it 'should return 400 for invalid name' do
@@ -153,7 +156,7 @@ describe API::API, api: true  do
           name: 'label1',
           color: '#FF'
       response.status.should == 400
-      json_response['message'].should == 'Color is invalid'
+      json_response['message']['color'].should == ['is invalid']
     end
 
     it 'should return 400 for too long color code' do
@@ -161,7 +164,7 @@ describe API::API, api: true  do
            name: 'Foo',
            color: '#FFAAFFFF'
       response.status.should == 400
-      json_response['message'].should == 'Color is invalid'
+      json_response['message']['color'].should == ['is invalid']
     end
   end
 end
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index d39962670b1..5ba3a330991 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -163,6 +163,28 @@ describe API::API, api: true  do
         json_response['message']['labels']['?']['title'].should ==
           ['is invalid']
       end
+
+      context 'with existing MR' do
+        before do
+          post api("/projects/#{project.id}/merge_requests", user),
+               title: 'Test merge_request',
+               source_branch: 'stable',
+               target_branch: 'master',
+               author: user
+          @mr = MergeRequest.all.last
+        end
+
+        it 'should return 409 when MR already exists for source/target' do
+          expect do
+            post api("/projects/#{project.id}/merge_requests", user),
+                 title: 'New test merge_request',
+                 source_branch: 'stable',
+                 target_branch: 'master',
+                 author: user
+          end.to change { MergeRequest.count }.by(0)
+          response.status.should == 409
+        end
+      end
     end
 
     context 'forked projects' do
@@ -210,16 +232,26 @@ describe API::API, api: true  do
         response.status.should == 400
       end
 
-      it "should return 404 when target_branch is specified and not a forked project" do
-        post api("/projects/#{project.id}/merge_requests", user),
-        title: 'Test merge_request', target_branch: 'master', source_branch: 'stable', author: user, target_project_id: fork_project.id
-        response.status.should == 404
-      end
+      context 'when target_branch is specified' do
+        it 'should return 422 if not a forked project' do
+          post api("/projects/#{project.id}/merge_requests", user),
+               title: 'Test merge_request',
+               target_branch: 'master',
+               source_branch: 'stable',
+               author: user,
+               target_project_id: fork_project.id
+          response.status.should == 422
+        end
 
-      it "should return 404 when target_branch is specified and for a different fork" do
-        post api("/projects/#{fork_project.id}/merge_requests", user2),
-        title: 'Test merge_request', target_branch: 'master', source_branch: 'stable', author: user2, target_project_id: unrelated_project.id
-        response.status.should == 404
+        it 'should return 422 if targeting a different fork' do
+          post api("/projects/#{fork_project.id}/merge_requests", user2),
+               title: 'Test merge_request',
+               target_branch: 'master',
+               source_branch: 'stable',
+               author: user2,
+               target_project_id: unrelated_project.id
+          response.status.should == 422
+        end
       end
 
       it "should return 201 when target_branch is specified and for the same project" do
@@ -256,7 +288,7 @@ describe API::API, api: true  do
       merge_request.close
       put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user)
       response.status.should == 405
-      json_response['message'].should == 'Method Not Allowed'
+      json_response['message'].should == '405 Method Not Allowed'
     end
 
     it "should return 401 if user has no permissions to merge" do
@@ -316,7 +348,8 @@ describe API::API, api: true  do
     end
 
     it "should return 404 if note is attached to non existent merge request" do
-      post api("/projects/#{project.id}/merge_request/111/comments", user), note: "My comment"
+      post api("/projects/#{project.id}/merge_request/404/comments", user),
+           note: 'My comment'
       response.status.should == 404
     end
   end
diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index 5575da86c2e..571d8506277 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/spec/requests/api/projects_spec.rb
@@ -188,9 +188,24 @@ describe API::API, api: true  do
       response.status.should == 201
     end
 
-    it "should respond with 404 on failure" do
+    it 'should respond with 400 on failure' do
       post api("/projects/user/#{user.id}", admin)
-      response.status.should == 404
+      response.status.should == 400
+      json_response['message']['creator'].should == ['can\'t be blank']
+      json_response['message']['namespace'].should == ['can\'t be blank']
+      json_response['message']['name'].should == [
+        'can\'t be blank',
+        'is too short (minimum is 0 characters)',
+        'can contain only letters, digits, \'_\', \'-\' and \'.\' and '\
+        'space. It must start with letter, digit or \'_\'.'
+      ]
+      json_response['message']['path'].should == [
+        'can\'t be blank',
+        'is too short (minimum is 0 characters)',
+        'can contain only letters, digits, \'_\', \'-\' and \'.\'. It must '\
+        'start with letter, digit or \'_\', optionally preceeded by \'.\'. '\
+        'It must not end in \'.git\'.'
+      ]
     end
 
     it "should assign attributes to project" do
@@ -410,9 +425,9 @@ describe API::API, api: true  do
       response.status.should == 200
     end
 
-    it "should return success when deleting unknown snippet id" do
+    it 'should return 404 when deleting unknown snippet id' do
       delete api("/projects/#{project.id}/snippets/1234", user)
-      response.status.should == 200
+      response.status.should == 404
     end
   end
 
@@ -459,7 +474,21 @@ describe API::API, api: true  do
     describe "POST /projects/:id/keys" do
       it "should not create an invalid ssh key" do
         post api("/projects/#{project.id}/keys", user), { title: "invalid key" }
-        response.status.should == 404
+        response.status.should == 400
+        json_response['message']['key'].should == [
+          'can\'t be blank',
+          'is too short (minimum is 0 characters)',
+          'is invalid'
+        ]
+      end
+
+      it 'should not create a key without title' do
+        post api("/projects/#{project.id}/keys", user), key: 'some key'
+        response.status.should == 400
+        json_response['message']['title'].should == [
+          'can\'t be blank',
+          'is too short (minimum is 0 characters)'
+        ]
       end
 
       it "should create new ssh key" do
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 8bbe9b5b736..b0752ebe43c 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -51,6 +51,7 @@ describe API::API, api: true  do
     it "should return a 404 error if user id not found" do
       get api("/users/9999", user)
       response.status.should == 404
+      json_response['message'].should == '404 Not found'
     end
   end
 
@@ -98,18 +99,47 @@ describe API::API, api: true  do
     end
 
     it "should not create user with invalid email" do
-      post api("/users", admin), { email: "invalid email", password: 'password' }
+      post api('/users', admin),
+           email: 'invalid email',
+           password: 'password',
+           name: 'test'
       response.status.should == 400
     end
 
-    it "should return 400 error if password not given" do
-      post api("/users", admin), { email: 'test@example.com' }
+    it 'should return 400 error if name not given' do
+      post api('/users', admin), email: 'test@example.com', password: 'pass1234'
+      response.status.should == 400
+    end
+
+    it 'should return 400 error if password not given' do
+      post api('/users', admin), email: 'test@example.com', name: 'test'
       response.status.should == 400
     end
 
     it "should return 400 error if email not given" do
-      post api("/users", admin), { password: 'pass1234' }
+      post api('/users', admin), password: 'pass1234', name: 'test'
+      response.status.should == 400
+    end
+
+    it 'should return 400 error if user does not validate' do
+      post api('/users', admin),
+           password: 'pass',
+           email: 'test@example.com',
+           username: 'test!',
+           name: 'test',
+           bio: 'g' * 256,
+           projects_limit: -1
       response.status.should == 400
+      json_response['message']['password'].
+          should == ['is too short (minimum is 8 characters)']
+      json_response['message']['bio'].
+          should == ['is too long (maximum is 255 characters)']
+      json_response['message']['projects_limit'].
+          should == ['must be greater than or equal to 0']
+      json_response['message']['username'].
+          should == ['can contain only letters, digits, '\
+          '\'_\', \'-\' and \'.\'. It must start with letter, digit or '\
+          '\'_\', optionally preceeded by \'.\'. It must not end in \'.git\'.']
     end
 
     it "shouldn't available for non admin users" do
@@ -117,21 +147,37 @@ describe API::API, api: true  do
       response.status.should == 403
     end
 
-    context "with existing user" do
-      before { post api("/users", admin), { email: 'test@example.com', password: 'password', username: 'test' } }
+    context 'with existing user' do
+      before do
+        post api('/users', admin),
+             email: 'test@example.com',
+             password: 'password',
+             username: 'test',
+             name: 'foo'
+      end
 
-      it "should not create user with same email" do
+      it 'should return 409 conflict error if user with same email exists' do
         expect {
-          post api("/users", admin), { email: 'test@example.com', password: 'password' }
+          post api('/users', admin),
+               name: 'foo',
+               email: 'test@example.com',
+               password: 'password',
+               username: 'foo'
         }.to change { User.count }.by(0)
+        response.status.should == 409
+        json_response['message'].should == 'Email has already been taken'
       end
 
-      it "should return 409 conflict error if user with email exists" do
-        post api("/users", admin), { email: 'test@example.com', password: 'password' }
-      end
-
-      it "should return 409 conflict error if same username exists" do
-        post api("/users", admin), { email: 'foo@example.com', password: 'pass', username: 'test' }
+      it 'should return 409 conflict error if same username exists' do
+        expect do
+          post api('/users', admin),
+               name: 'foo',
+               email: 'foo@example.com',
+               password: 'password',
+               username: 'test'
+        end.to change { User.count }.by(0)
+        response.status.should == 409
+        json_response['message'].should == 'Username has already been taken'
       end
     end
   end
@@ -173,6 +219,20 @@ describe API::API, api: true  do
       user.reload.bio.should == 'new test bio'
     end
 
+    it 'should update user with his own email' do
+      put api("/users/#{user.id}", admin), email: user.email
+      response.status.should == 200
+      json_response['email'].should == user.email
+      user.reload.email.should == user.email
+    end
+
+    it 'should update user with his own username' do
+      put api("/users/#{user.id}", admin), username: user.username
+      response.status.should == 200
+      json_response['username'].should == user.username
+      user.reload.username.should == user.username
+    end
+
     it "should update admin status" do
       put api("/users/#{user.id}", admin), {admin: true}
       response.status.should == 200
@@ -190,7 +250,7 @@ describe API::API, api: true  do
 
     it "should not allow invalid update" do
       put api("/users/#{user.id}", admin), {email: 'invalid email'}
-      response.status.should == 404
+      response.status.should == 400
       user.reload.email.should_not == 'invalid email'
     end
 
@@ -202,25 +262,49 @@ describe API::API, api: true  do
     it "should return 404 for non-existing user" do
       put api("/users/999999", admin), {bio: 'update should fail'}
       response.status.should == 404
+      json_response['message'].should == '404 Not found'
+    end
+
+    it 'should return 400 error if user does not validate' do
+      put api("/users/#{user.id}", admin),
+          password: 'pass',
+          email: 'test@example.com',
+          username: 'test!',
+          name: 'test',
+          bio: 'g' * 256,
+          projects_limit: -1
+      response.status.should == 400
+      json_response['message']['password'].
+          should == ['is too short (minimum is 8 characters)']
+      json_response['message']['bio'].
+          should == ['is too long (maximum is 255 characters)']
+      json_response['message']['projects_limit'].
+          should == ['must be greater than or equal to 0']
+      json_response['message']['username'].
+          should == ['can contain only letters, digits, '\
+          '\'_\', \'-\' and \'.\'. It must start with letter, digit or '\
+          '\'_\', optionally preceeded by \'.\'. It must not end in \'.git\'.']
     end
 
     context "with existing user" do
       before {
         post api("/users", admin), { email: 'test@example.com', password: 'password', username: 'test', name: 'test' }
         post api("/users", admin), { email: 'foo@bar.com', password: 'password', username: 'john', name: 'john' }
-        @user_id = User.all.last.id
+        @user = User.all.last
       }
 
-#      it "should return 409 conflict error if email address exists" do
-#        put api("/users/#{@user_id}", admin), { email: 'test@example.com' }
-#        response.status.should == 409
-#      end
-#
-#      it "should return 409 conflict error if username taken" do
-#        @user_id = User.all.last.id
-#        put api("/users/#{@user_id}", admin), { username: 'test' }
-#        response.status.should == 409
-#      end
+      it 'should return 409 conflict error if email address exists' do
+        put api("/users/#{@user.id}", admin), email: 'test@example.com'
+        response.status.should == 409
+        @user.reload.email.should == @user.email
+      end
+
+      it 'should return 409 conflict error if username taken' do
+        @user_id = User.all.last.id
+        put api("/users/#{@user.id}", admin), username: 'test'
+        response.status.should == 409
+        @user.reload.username.should == @user.username
+      end
     end
   end
 
@@ -229,7 +313,14 @@ describe API::API, api: true  do
 
     it "should not create invalid ssh key" do
       post api("/users/#{user.id}/keys", admin), { title: "invalid key" }
-      response.status.should == 404
+      response.status.should == 400
+      json_response['message'].should == '400 (Bad request) "key" not given'
+    end
+
+    it 'should not create key without title' do
+      post api("/users/#{user.id}/keys", admin), key: 'some key'
+      response.status.should == 400
+      json_response['message'].should == '400 (Bad request) "title" not given'
     end
 
     it "should create ssh key" do
@@ -254,6 +345,7 @@ describe API::API, api: true  do
       it 'should return 404 for non-existing user' do
         get api('/users/999999/keys', admin)
         response.status.should == 404
+        json_response['message'].should == '404 User Not Found'
       end
 
       it 'should return array of ssh keys' do
@@ -292,11 +384,13 @@ describe API::API, api: true  do
         user.save
         delete api("/users/999999/keys/#{key.id}", admin)
         response.status.should == 404
+        json_response['message'].should == '404 User Not Found'
       end
 
       it 'should return 404 error if key not foud' do
         delete api("/users/#{user.id}/keys/42", admin)
         response.status.should == 404
+        json_response['message'].should == '404 Key Not Found'
       end
     end
   end
@@ -324,6 +418,7 @@ describe API::API, api: true  do
     it "should return 404 for non-existing user" do
       delete api("/users/999999", admin)
       response.status.should == 404
+      json_response['message'].should == '404 User Not Found'
     end
   end
 
@@ -375,6 +470,7 @@ describe API::API, api: true  do
     it "should return 404 Not Found within invalid ID" do
       get api("/user/keys/42", user)
       response.status.should == 404
+      json_response['message'].should == '404 Not found'
     end
 
     it "should return 404 error if admin accesses user's ssh key" do
@@ -383,6 +479,7 @@ describe API::API, api: true  do
       admin
       get api("/user/keys/#{key.id}", admin)
       response.status.should == 404
+      json_response['message'].should == '404 Not found'
     end
   end
 
@@ -403,6 +500,13 @@ describe API::API, api: true  do
     it "should not create ssh key without key" do
       post api("/user/keys", user), title: 'title'
       response.status.should == 400
+      json_response['message'].should == '400 (Bad request) "key" not given'
+    end
+
+    it 'should not create ssh key without title' do
+      post api('/user/keys', user), key: 'some key'
+      response.status.should == 400
+      json_response['message'].should == '400 (Bad request) "title" not given'
     end
 
     it "should not create ssh key without title" do