Little refactor, add specs, and a CHANGELOG entry

Signed-off-by: Rémy Coutable <remy@rymai.me>

3 files changed, 31 insertions, 9 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 181829a86a5..bae44702fc8 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -9,6 +9,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`
diff --git a/app/models/project_services/hipchat_service.rb b/app/models/project_services/hipchat_service.rb
index 413200199b4..4fc8d640310 100644
--- a/app/models/project_services/hipchat_service.rb
+++ b/app/models/project_services/hipchat_service.rb
@@ -67,7 +67,7 @@ class HipchatService < Service
     @gate ||= HipChat::Client.new(token, options)
   end
 
-  def message_options(data)
+  def message_options(data = nil)
     { notify: notify.present? && notify == '1', color: build_status_color(data) || color || 'yellow' }
   end
 
@@ -241,11 +241,14 @@ class HipchatService < Service
   end
 
   def build_status_color(data)
-    if data[:commit][:status] == 'success'
+    return unless data && data[:object_kind] == 'build'
+
+    case data[:commit][:status]
+    when 'success'
       'green'
     else
       'red'
-    end if data[:object_kind] == 'build'
+    end
   end
 
   def project_name
diff --git a/spec/models/project_services/hipchat_service_spec.rb b/spec/models/project_services/hipchat_service_spec.rb
index 5f618322aab..62ae5f6cf74 100644
--- a/spec/models/project_services/hipchat_service_spec.rb
+++ b/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