Merge branch '5571-hipchat-notifications-missing-colors' into 'master'

Resolve "HipChat notifications missing colors"

_Originally opened at !2322 by @eisnerd._

- - -

## What does this MR do?

This MR restores the colors mentioned in #5571 by overriding the color setting with green/red just for build events.

## What are the relevant issue numbers?

Closes #5571.

See merge request !5498

CHANGELOG

@@ -11,6 +11,7 @@ v 8.11.0 (unreleased)
   - Add green outline to New Branch button. !5447 (winniehell)
   - Retrieve rendered HTML from cache in one request
   - Nokogiri's various parsing methods are now instrumented
+  - Add build event color in HipChat messages (David Eisner)
   - Make fork counter always clickable. !5463 (winniehell)
   - Remove `search_id` of labels dropdown filter to fix 'Missleading URI for labels in Merge Requests and Issues view'. !5368 (Scott Le)
   - Load project invited groups and members eagerly in `ProjectTeam#fetch_members`

app/models/project_services/hipchat_service.rb

@@ -46,7 +46,7 @@ class HipchatService < Service
     return unless supported_events.include?(data[:object_kind])
     message = create_message(data)
     return unless message.present?
-    gate[room].send('GitLab', message, message_options)
+    gate[room].send('GitLab', message, message_options(data))
   end
 
   def test(data)
@@ -67,8 +67,8 @@ class HipchatService < Service
     @gate ||= HipChat::Client.new(token, options)
   end
 
-  def message_options
-    { notify: notify.present? && notify == '1', color: color || 'yellow' }
+  def message_options(data = nil)
+    { notify: notify.present? && notify == '1', color: message_color(data) }
   end
 
   def create_message(data)
@@ -240,6 +240,21 @@ class HipchatService < Service
     "#{project_link}: Commit #{commit_link} of #{branch_link} #{ref_type} by #{user_name} #{humanized_status(status)} in #{duration} second(s)"
   end
 
+  def message_color(data)
+    build_status_color(data) || color || 'yellow'
+  end
+
+  def build_status_color(data)
+    return unless data && data[:object_kind] == 'build'
+
+    case data[:commit][:status]
+    when 'success'
+      'green'
+    else
+      'red'
+    end
+  end
+
   def project_name
     project.name_with_namespace.gsub(/\s/, '')
   end

spec/models/project_services/hipchat_service_spec.rb

@@ -340,18 +340,36 @@ describe HipchatService, models: true do
     end
 
     context "#message_options" do
-      it "should be set to the defaults" do
-        expect(hipchat.send(:message_options)).to eq({ notify: false, color: 'yellow' })
+      it "is set to the defaults" do
+        expect(hipchat.__send__(:message_options)).to eq({ notify: false, color: 'yellow' })
       end
 
-      it "should set notfiy to true" do
+      it "sets notify to true" do
         allow(hipchat).to receive(:notify).and_return('1')
-        expect(hipchat.send(:message_options)).to eq({ notify: true, color: 'yellow' })
+
+        expect(hipchat.__send__(:message_options)).to eq({ notify: true, color: 'yellow' })
       end
 
-      it "should set the color" do
+      it "sets the color" do
         allow(hipchat).to receive(:color).and_return('red')
-        expect(hipchat.send(:message_options)).to eq({ notify: false, color: 'red' })
+
+        expect(hipchat.__send__(:message_options)).to eq({ notify: false, color: 'red' })
+      end
+
+      context 'with a successful build' do
+        it 'uses the green color' do
+          build_data = { object_kind: 'build', commit: { status: 'success' } }
+
+          expect(hipchat.__send__(:message_options, build_data)).to eq({ notify: false, color: 'green' })
+        end
+      end
+
+      context 'with a failed build' do
+        it 'uses the red color' do
+          build_data = { object_kind: 'build', commit: { status: 'failed' } }
+
+          expect(hipchat.__send__(:message_options, build_data)).to eq({ notify: false, color: 'red' })
+        end
       end
     end
   end