Merge branch 'improve-hipchat-service-test' into 'master'

Provide more feedback what went wrong if HipChat service failed test

This MR adds support for displaying the error message during HipChat service test. Before an Error 500 would be displayed with no helpful remarks.

Screenshot:

![image](https://gitlab.com/stanhu/gitlab-ce/uploads/10f82eb02db138f9680e69cdb3d3ed82/image.png)

Issue gitlab-com/support-forum#213

See merge request !1144

7 files changed, 76 insertions, 3 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 8a378670c41..bbbee1d929a 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 7.14.0 (unreleased)
+  - Provide more feedback what went wrong if HipChat service failed test (Stan Hu)
   - Disable turbolinks when linking to Bitbucket import status (Stan Hu)
   - Fix broken code import and display error messages if something went wrong with creating project (Stan Hu)
   - Fix corrupted binary files when using API files endpoint (Stan Hu)
diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb
index 1e435be8275..01105532479 100644
--- a/app/controllers/projects/services_controller.rb
+++ b/app/controllers/projects/services_controller.rb
@@ -39,10 +39,13 @@ class Projects::ServicesController < Projects::ApplicationController
 
   def test
     data = Gitlab::PushDataBuilder.build_sample(project, current_user)
-    if @service.execute(data)
+    outcome = @service.test(data)
+    if outcome[:success]
       message = { notice: 'We sent a request to the provided URL' }
     else
-      message = { alert: 'We tried to send a request to the provided URL but an error occured' }
+      error_message = "We tried to send a request to the provided URL but an error occurred"
+      error_message << ": #{outcome[:result]}" if outcome[:result].present?
+      message = { alert: error_message }
     end
 
     redirect_to :back, message
diff --git a/app/models/project_services/hipchat_service.rb b/app/models/project_services/hipchat_service.rb
index 6761f00183e..7a15a861abc 100644
--- a/app/models/project_services/hipchat_service.rb
+++ b/app/models/project_services/hipchat_service.rb
@@ -60,6 +60,16 @@ class HipchatService < Service
     gate[room].send('GitLab', message, message_options)
   end
 
+  def test(data)
+    begin
+      result = execute(data)
+    rescue StandardError => error
+      return { success: false, result: error }
+    end
+
+    { success: true, result: result }
+  end
+
   private
 
   def gate
diff --git a/app/models/service.rb b/app/models/service.rb
index 818a6808db5..dcef2866c3b 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -87,10 +87,16 @@ class Service < ActiveRecord::Base
     %w(push tag_push issue merge_request)
   end
 
-  def execute
+  def execute(data)
     # implement inside child
   end
 
+  def test(data)
+    # default implementation
+    result = execute(data)
+    { success: result.present?, result: result }
+  end
+
   def can_test?
     !project.empty_repo?
   end
diff --git a/spec/controllers/projects/services_controller_spec.rb b/spec/controllers/projects/services_controller_spec.rb
new file mode 100644
index 00000000000..d4ecd98e12d
--- /dev/null
+++ b/spec/controllers/projects/services_controller_spec.rb
@@ -0,0 +1,35 @@
+require 'spec_helper'
+
+describe Projects::ServicesController do
+  let(:project) { create(:project) }
+  let(:user)    { create(:user) }
+  let(:service) { create(:service, project: project) }
+
+  before do
+    sign_in(user)
+    project.team << [user, :master]
+    controller.instance_variable_set(:@project, project)
+    controller.instance_variable_set(:@service, service)
+    request.env["HTTP_REFERER"] = "/"
+  end
+
+  describe "#test" do
+    context 'success' do
+      it "should redirect and show success message" do
+        expect(service).to receive(:test).and_return({ success: true, result: 'done' })
+        get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html
+        expect(response.status).to redirect_to('/')
+        expect(flash[:notice]).to eq('We sent a request to the provided URL')
+      end
+    end
+
+    context 'failure' do
+      it "should redirect and show failure message" do
+        expect(service).to receive(:test).and_return({ success: false, result: 'Bad test' })
+        get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html
+        expect(response.status).to redirect_to('/')
+        expect(flash[:alert]).to eq('We tried to send a request to the provided URL but an error occurred: Bad test')
+      end
+    end
+  end
+end
diff --git a/spec/models/project_services/hipchat_service_spec.rb b/spec/models/project_services/hipchat_service_spec.rb
index 4707673269a..65d16beef91 100644
--- a/spec/models/project_services/hipchat_service_spec.rb
+++ b/spec/models/project_services/hipchat_service_spec.rb
@@ -47,6 +47,14 @@ describe HipchatService do
       WebMock.stub_request(:post, api_url)
     end
 
+    it 'should test and return errors' do
+      allow(hipchat).to receive(:execute).and_raise(StandardError, 'no such room')
+      result = hipchat.test(push_sample_data)
+
+      expect(result[:success]).to be_falsey
+      expect(result[:result].to_s).to eq('no such room')
+    end
+
     it 'should use v1 if version is provided' do
       allow(hipchat).to receive(:api_version).and_return('v1')
       expect(HipChat::Client).to receive(:new).
diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb
index ca11758ee06..a213ffe6c4b 100644
--- a/spec/models/service_spec.rb
+++ b/spec/models/service_spec.rb
@@ -46,6 +46,16 @@ describe Service do
       describe :can_test do
         it { expect(@testable).to eq(true) }
       end
+
+      describe :test do
+        let(:data) { 'test' }
+
+        it 'test runs execute' do
+          expect(@service).to receive(:execute).with(data)
+
+          @service.test(data)
+        end
+      end
     end
 
     describe "With commits" do