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

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)

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

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

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

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

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).

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