Fix notification issues on HipChatService, add HipChatMessage specs

adding specs for HipChatMessage exposed some issues with the way I was
building the matrix-commit style notification message, so it ended up being
quite a bit more changes than I expected.

The save call from the service configure form submits an empty string as
the server URL if left blank, which I've indicated to do in order to use
the default server, but Hash#merge will happily overwrite a full string with
a blank string if asked, so we need to break that out, along with the worker
options which get mutated into string hash keys, of which the HipChat client
seems to not understand. Additionally, add another spec to make sure we call
the Sidekiq worker with expected arguments.

5 files changed, 139 insertions, 25 deletions

diff --git a/app/models/project_services/hip_chat_message.rb b/app/models/project_services/hip_chat_message.rb
index 85695cc..8350bb6 100644
--- a/app/models/project_services/hip_chat_message.rb
+++ b/app/models/project_services/hip_chat_message.rb
@@ -7,42 +7,56 @@ class HipChatMessage
 
   def to_s
     lines = Array.new
-    lines.push("<a href=\"#{RoutesHelper.project_url(build.project)}\">#{build.project.name}</a> - ")
-    if build.commit.matrix?
-      lines.push("<a href=\"#{RoutesHelper.project_ref_commit_url(build.project, build.commit.ref, build.commit.sha)}\">Commit ##{commit.id}</a></br>")
+    lines.push("<a href=\"#{RoutesHelper.project_url(project)}\">#{project.name}</a> - ")
+    if commit.matrix?
+      lines.push("<a href=\"#{RoutesHelper.project_ref_commit_url(project, commit.ref, commit.sha)}\">Commit ##{commit.id}</a></br>")
     else
-      lines.push("<a href=\"#{RoutesHelper.project_build_url(build.project, build)}\">Build '#{build.job_name}' ##{build.id}</a></br>")
+      first_build = commit.builds_without_retry.first
+      lines.push("<a href=\"#{RoutesHelper.project_build_url(project, first_build)}\">Build '#{first_build.job_name}' ##{first_build.id}</a></br>")
     end
-    lines.push("#{build.commit.short_sha} #{build.commit.git_author_name} - #{build.commit.git_commit_message}</br>")
-    lines.push("#{humanized_status} in #{build.commit.duration} second(s).")
+    lines.push("#{commit.short_sha} #{commit.git_author_name} - #{commit.git_commit_message}</br>")
+    lines.push("#{humanized_status(commit_status)} in #{commit.duration} second(s).")
     lines.join('')
   end
 
-  def color
-    case status
+  def status_color(build_or_commit=nil)
+    build_or_commit ||= commit_status
+    case build_or_commit
     when :success
       'green'
     when :failed, :canceled
       'red'
-    when :pending, :running
+    else # :pending, :running or unknown
       'yellow'
-    else
-      'random'
     end
   end
 
   def notify?
-    [:failed, :canceled].include?(status)
+    [:failed, :canceled].include?(commit_status)
   end
 
+
   private
 
-  def status
+  def commit
+    build.commit
+  end
+
+  def project
+    commit.project
+  end
+
+  def build_status
     build.status.to_sym
   end
 
-  def humanized_status
-    case status
+  def commit_status
+    commit.status.to_sym
+  end
+
+  def humanized_status(build_or_commit=nil)
+    build_or_commit ||= commit_status
+    case build_or_commit
     when :pending
       "Pending"
     when :running
diff --git a/app/models/project_services/hip_chat_service.rb b/app/models/project_services/hip_chat_service.rb
index 0faaca5..8e5f024 100644
--- a/app/models/project_services/hip_chat_service.rb
+++ b/app/models/project_services/hip_chat_service.rb
@@ -47,21 +47,34 @@ class HipChatService < Service
     commit = build.commit
     return unless commit
     return unless commit.builds_without_retry.include? build
+    return if commit.success? and notify_only_broken_builds?
+    return if commit.running?
 
     msg = HipChatMessage.new(build)
-    HipChatNotifierWorker.perform_async(hipchat_room, hipchat_token, msg.to_s, {
-      message_format: 'html',
-      color: msg.color,
-      notify: notify_only_broken_builds? && msg.notify?
-    })
+    opts = default_options.merge(
+      token: hipchat_token,
+      room: hipchat_room,
+      server: server_url,
+      color: msg.status_color,
+      notify: msg.notify?
+    )
+    HipChatNotifierWorker.perform_async(msg.to_s, opts)
   end
 
   private
 
   def default_options
     {
-      hipchat_server: 'https://api.hipchat.com'
+      service_name: 'GitLab CI',
+      message_format: 'html'
     }
   end
 
+  def server_url
+    if hipchat_server.blank?
+      'https://api.hipchat.com'
+    else
+      hipchat_server
+    end
+  end
 end
diff --git a/app/workers/hip_chat_notifier_worker.rb b/app/workers/hip_chat_notifier_worker.rb
index 5bc7dda..0403578 100644
--- a/app/workers/hip_chat_notifier_worker.rb
+++ b/app/workers/hip_chat_notifier_worker.rb
@@ -2,8 +2,17 @@
 class HipChatNotifierWorker
   include Sidekiq::Worker
 
-  def perform(room, token, message, options={})
-    client = HipChat::Client.new(token, api_version: 'v2') # v1.5.0 requires explicit version (
-    client[room].send("GitLab CI", message, options)
+  def perform(message, options={})
+    room   = options.delete('room')
+    token  = options.delete('token')
+    server = options.delete('server')
+    name   = options.delete('service_name')
+    client_opts = {
+      api_version: 'v2',
+      server_url: server
+    }
+
+    client = HipChat::Client.new(token, client_opts)
+    client[room].send(name, message, options.symbolize_keys)
   end
 end
diff --git a/spec/models/project_services/hip_chat_message_spec.rb b/spec/models/project_services/hip_chat_message_spec.rb
new file mode 100644
index 0000000..6096b5c
--- /dev/null
+++ b/spec/models/project_services/hip_chat_message_spec.rb
@@ -0,0 +1,65 @@
+require 'spec_helper'
+
+describe HipChatMessage do
+  subject { HipChatMessage.new(build) }
+
+  let(:project) { FactoryGirl.create(:project) }
+  let(:commit)  { FactoryGirl.create(:commit, project: project) }
+  let(:job)     { FactoryGirl.create(:job, project: project) }
+  let(:build)   { FactoryGirl.create(:build, commit: commit, job: job, status: 'success') }
+
+  context 'when build succeeds' do
+
+    before { build.save }
+
+    it 'returns a successful message' do
+      expect( subject.status_color ).to eq 'green'
+      expect( subject.notify? ).to be_false
+      expect( subject.to_s ).to match(/Build '[^']+' #\d+/)
+      expect( subject.to_s ).to match(/Successful in \d+ second\(s\)\./)
+    end
+  end
+
+  context 'when build fails' do
+
+    before do
+      build.status = 'failed'
+      build.save
+    end
+
+    it 'returns a failure message' do
+      expect( subject.status_color ).to eq 'red'
+      expect( subject.notify? ).to be_true
+      expect( subject.to_s ).to match(/Build '[^']+' #\d+/)
+      expect( subject.to_s ).to match(/Failed in \d+ second\(s\)\./)
+    end
+  end
+
+  context 'when all matrix builds succeed' do
+    let(:job2)    { FactoryGirl.create(:job, project: project, name: 'Another Job') }
+    let(:build2)  { FactoryGirl.create(:build, id: 10, commit: commit, job: job2, status: 'success') }
+
+    before { build.save; build2.save }
+
+    it 'returns a successful message' do
+      expect( subject.status_color ).to eq 'green'
+      expect( subject.notify? ).to be_false
+      expect( subject.to_s ).to match(/Commit #\d+/)
+      expect( subject.to_s ).to match(/Successful in \d+ second\(s\)\./)
+    end
+  end
+
+  context 'when at least one matrix build fails' do
+    let(:job2)    { FactoryGirl.create(:job, project: project, name: 'Another Job') }
+    let(:build2)  { FactoryGirl.create(:build, id: 10, commit: commit, job: job2, status: 'failed') }
+
+    before { build.save; build2.save }
+
+    it 'returns a failure message' do
+      expect( subject.status_color ).to eq 'red'
+      expect( subject.notify? ).to be_true
+      expect( subject.to_s ).to match(/Commit #\d+/)
+      expect( subject.to_s ).to match(/Failed in \d+ second\(s\)\./)
+    end
+  end
+end
diff --git a/spec/models/project_services/hip_chat_service_spec.rb b/spec/models/project_services/hip_chat_service_spec.rb
index e578461..5642221 100644
--- a/spec/models/project_services/hip_chat_service_spec.rb
+++ b/spec/models/project_services/hip_chat_service_spec.rb
@@ -41,7 +41,20 @@ describe HipChatService do
       service.execute(build)
       HipChatNotifierWorker.drain
 
-      WebMock.should have_requested(:post, api_url).once
+      expect( WebMock ).to have_requested(:post, api_url).once
+    end
+
+    it "calls the worker with expected arguments" do
+      expect( HipChatNotifierWorker ).to receive(:perform_async) \
+        .with(an_instance_of(String), hash_including(
+          token: 'a1b2c3d4e5f6',
+          room: 123,
+          server: 'https://api.hipchat.com',
+          color: 'red',
+          notify: true
+        ))
+
+      service.execute(build)
     end
   end
 end