From ef15668d8214e8cce5732525c872d5b8f57e337a Mon Sep 17 00:00:00 2001
From: James Edwards-Jones <jedwardsjones@gitlab.com>
Date: Sat, 24 Feb 2018 03:18:14 +0000
Subject: [PATCH] Service integration displays validation errors on test fail

Fixes attempts to update a service integration which had `can_test?`
set to true but validations were causing the "Test and save changes"
button to return "Something went wrong on our end."

Removes references to index action left from 0af99433143727088b6a0a1b2163751c05d80ce6
---
 .../projects/services_controller.rb           | 34 +++++++++++--------
 config/routes/project.rb                      |  2 +-
 .../projects/services_controller_spec.rb      | 12 +++++++
 3 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb
index b0da0d4dac5..f14cb5f6a9f 100644
--- a/app/controllers/projects/services_controller.rb
+++ b/app/controllers/projects/services_controller.rb
@@ -4,7 +4,7 @@ class Projects::ServicesController < Projects::ApplicationController
   # Authorize
   before_action :authorize_admin_project!
   before_action :ensure_service_enabled
-  before_action :service, only: [:edit, :update, :test]
+  before_action :service
 
   respond_to :html
 
@@ -24,26 +24,30 @@ class Projects::ServicesController < Projects::ApplicationController
   end
 
   def test
-    message = {}
-
-    if @service.can_test? && @service.update_attributes(service_params[:service])
-      data = @service.test_data(project, current_user)
-      outcome = @service.test(data)
-
-      unless outcome[:success]
-        message = { error: true, message: 'Test failed.', service_response: outcome[:result].to_s }
-      end
-
-      status = :ok
+    if @service.can_test?
+      render json: service_test_response, status: :ok
     else
-      status = :not_found
+      render json: {}, status: :not_found
     end
-
-    render json: message, status: status
   end
 
   private
 
+  def service_test_response
+    if @service.update_attributes(service_params[:service])
+      data = @service.test_data(project, current_user)
+      outcome = @service.test(data)
+
+      if outcome[:success]
+        {}
+      else
+        { error: true, message: 'Test failed.', service_response: outcome[:result].to_s }
+      end
+    else
+      { error: true, message: 'Validations failed.', service_response: @service.errors.full_messages.join(',') }
+    end
+  end
+
   def success_message
     if @service.active?
       "#{@service.title} activated."
diff --git a/config/routes/project.rb b/config/routes/project.rb
index cb46c439415..710fe0ec325 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -69,7 +69,7 @@ constraints(ProjectUrlConstrainer.new) do
         end
       end
 
-      resources :services, constraints: { id: %r{[^/]+} }, only: [:index, :edit, :update] do
+      resources :services, constraints: { id: %r{[^/]+} }, only: [:edit, :update] do
         member do
           put :test
         end
diff --git a/spec/controllers/projects/services_controller_spec.rb b/spec/controllers/projects/services_controller_spec.rb
index 847ac6f2be0..e4dc61b3a68 100644
--- a/spec/controllers/projects/services_controller_spec.rb
+++ b/spec/controllers/projects/services_controller_spec.rb
@@ -23,6 +23,18 @@ describe Projects::ServicesController do
       end
     end
 
+    context 'when validations fail' do
+      let(:service_params) { { active: 'true', token: '' } }
+
+      it 'returns error messages in JSON response' do
+        put :test, namespace_id: project.namespace, project_id: project, id: :hipchat, service: service_params
+
+        expect(json_response['message']).to eq "Validations failed."
+        expect(json_response['service_response']).to eq "Token can't be blank"
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
+
     context 'success' do
       context 'with empty project' do
         let(:project) { create(:project) }