Merge branch 'fix-broken-testing-of-some-integrations' into 'master'

Fix inability to test some project integrations

Closes gitlab-ee#3194

See merge request !13729

app/controllers/projects/services_controller.rb

@@ -4,7 +4,6 @@ class Projects::ServicesController < Projects::ApplicationController
   # Authorize
   before_action :authorize_admin_project!
   before_action :service, only: [:edit, :update, :test]
-  before_action :update_service, only: [:update, :test]
 
   respond_to :html
 
@@ -14,6 +13,8 @@ class Projects::ServicesController < Projects::ApplicationController
   end
 
   def update
+    @service.attributes = service_params[:service]
+
     if @service.save(context: :manual_change)
       redirect_to(project_settings_integrations_path(@project), notice: success_message)
     else
@@ -24,7 +25,7 @@ class Projects::ServicesController < Projects::ApplicationController
   def test
     message = {}
 
-    if @service.can_test?
+    if @service.can_test? && @service.update_attributes(service_params[:service])
       data = @service.test_data(project, current_user)
       outcome = @service.test(data)
 
@@ -50,10 +51,6 @@ class Projects::ServicesController < Projects::ApplicationController
     end
   end
 
-  def update_service
-    @service.assign_attributes(service_params[:service])
-  end
-
   def service
     @service ||= @project.find_or_initialize_service(params[:id])
   end

changelogs/unreleased/rd-fix-broken-configuration-for-some-integrations.yml

+---
+title: Testing of some integrations were broken due to missing ServiceHook record.
+merge_request:
+author:
+type: fixed

spec/controllers/projects/services_controller_spec.rb

@@ -10,9 +10,6 @@ describe Projects::ServicesController do
   before do
     sign_in(user)
     project.team << [user, :master]
-
-    controller.instance_variable_set(:@project, project)
-    controller.instance_variable_set(:@service, service)
   end
 
   describe '#test' do
@@ -20,7 +17,7 @@ describe Projects::ServicesController do
       it 'renders 404' do
         allow_any_instance_of(Service).to receive(:can_test?).and_return(false)
 
-        put :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id
+        put :test, namespace_id: project.namespace, project_id: project, id: service.to_param
 
         expect(response).to have_http_status(404)
       end
@@ -36,7 +33,7 @@ describe Projects::ServicesController do
           it 'returns success' do
             allow_any_instance_of(MicrosoftTeams::Notifier).to receive(:ping).and_return(true)
 
-            put :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id
+            put :test, namespace_id: project.namespace, project_id: project, id: service.to_param
 
             expect(response.status).to eq(200)
           end
@@ -45,7 +42,7 @@ describe Projects::ServicesController do
         it 'returns success' do
           expect(HipChat::Client).to receive(:new).with('hipchat_token_p', anything).and_return(hipchat_client)
 
-          put :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, service: service_params
+          put :test, namespace_id: project.namespace, project_id: project, id: service.to_param, service: service_params
 
           expect(response.status).to eq(200)
         end
@@ -54,17 +51,42 @@ describe Projects::ServicesController do
       it 'returns success' do
         expect(HipChat::Client).to receive(:new).with('hipchat_token_p', anything).and_return(hipchat_client)
 
-        put :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, service: service_params
+        put :test, namespace_id: project.namespace, project_id: project, id: service.to_param, service: service_params
 
         expect(response.status).to eq(200)
       end
+
+      context 'when service is configured for the first time' do
+        before do
+          allow_any_instance_of(ServiceHook).to receive(:execute).and_return(true)
+        end
+
+        it 'persist the object' do
+          do_put
+
+          expect(BuildkiteService.first).to be_present
+        end
+
+        it 'creates the ServiceHook object' do
+          do_put
+
+          expect(BuildkiteService.first.service_hook).to be_present
+        end
+
+        def do_put
+          put :test, namespace_id: project.namespace,
+                     project_id: project,
+                     id: 'buildkite',
+                     service: { 'active' => '1', 'push_events' => '1', token: 'token', 'project_url' => 'http://test.com' }
+        end
+      end
     end
 
     context 'failure' do
       it 'returns success status code and the error message' do
         expect(HipChat::Client).to receive(:new).with('hipchat_token_p', anything).and_raise('Bad test')
 
-        put :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, service: service_params
+        put :test, namespace_id: project.namespace, project_id: project, id: service.to_param, service: service_params
 
         expect(response.status).to eq(200)
         expect(JSON.parse(response.body))
@@ -77,7 +99,7 @@ describe Projects::ServicesController do
     context 'when param `active` is set to true' do
       it 'activates the service and redirects to integrations paths' do
         put :update,
-          namespace_id: project.namespace.id, project_id: project.id, id: service.id, service: { active: true }
+          namespace_id: project.namespace, project_id: project, id: service.to_param, service: { active: true }
 
         expect(response).to redirect_to(project_settings_integrations_path(project))
         expect(flash[:notice]).to eq 'HipChat activated.'
@@ -87,7 +109,7 @@ describe Projects::ServicesController do
     context 'when param `active` is set to false' do
       it 'does not  activate the service but saves the settings' do
         put :update,
-          namespace_id: project.namespace.id, project_id: project.id, id: service.id, service: { active: false }
+          namespace_id: project.namespace, project_id: project, id: service.to_param, service: { active: false }
 
         expect(flash[:notice]).to eq 'HipChat settings saved, but not activated.'
       end