From 7f39b0f78c2a811ddf2f6d04c8016dca90bf2f1c Mon Sep 17 00:00:00 2001
From: James Edwards-Jones <jedwardsjones@gitlab.com>
Date: Sat, 24 Feb 2018 03:18:14 +0000
Subject: 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
---
 app/controllers/projects/services_controller.rb    | 28 ++++++++++++----------
 config/routes/project.rb                           |  2 +-
 .../projects/services_controller_spec.rb           | 12 ++++++++++
 3 files changed, 29 insertions(+), 13 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?
+      render json: service_test_response, status: :ok
+    else
+      render json: {}, status: :not_found
+    end
+  end
+
+  private
 
-    if @service.can_test? && @service.update_attributes(service_params[:service])
+  def service_test_response
+    if @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 }
+      if outcome[:success]
+        {}
+      else
+        { error: true, message: 'Test failed.', service_response: outcome[:result].to_s }
       end
-
-      status = :ok
     else
-      status = :not_found
+      { error: true, message: 'Validations failed.', service_response: @service.errors.full_messages.join(',') }
     end
-
-    render json: message, status: status
   end
 
-  private
-
   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) }
-- 
cgit v1.2.1