Merge branch 'patch/fix-merge-request-notifications' into 'master'

Fix MR notifications for slack and hipchat when approvals are fullfiled

A Slack notification comes in after opening a MR about the MR being opened but it also comes in after a MR gets the minimum number of approvers and it uses the same message.

Same behavior is present in hipchat notifications.

Fixes #462

See merge request !325

CHANGELOG-EE

@@ -3,6 +3,9 @@ Please view this file on the master branch, on stable branches it's out of date.
 v 8.8.0 (unreleased)
   - [Elastic] Database indexer prints its status
 
+v 8.7.2
+  - Fix MR notifications for slack and hipchat when approvals are fullfiled. !325
+
 v 8.7.0
   - Update GitLab Pages to 0.2.1: support user-defined 404 pages
   - Refactor group sync to pull access level logic to its own class. !306

app/models/project_services/hipchat_service.rb

@@ -177,14 +177,17 @@ class HipchatService < Service
 
     obj_attr = data[:object_attributes]
     obj_attr = HashWithIndifferentAccess.new(obj_attr)
-    merge_request_id = obj_attr[:iid]
+    merge_request_iid = obj_attr[:iid]
     state = obj_attr[:state]
     description = obj_attr[:description]
     title = obj_attr[:title]
+    action = obj_attr[:action]
 
-    merge_request_url = "#{project_url}/merge_requests/#{merge_request_id}"
-    merge_request_link = "<a href=\"#{merge_request_url}\">merge request !#{merge_request_id}</a>"
-    message = "#{user_name} #{state} #{merge_request_link} in " \
+    state_or_action_text = (action == 'approved') ? action : state
+
+    merge_request_url = "#{project_url}/merge_requests/#{merge_request_iid}"
+    merge_request_link = "<a href=\"#{merge_request_url}\">merge request !#{merge_request_iid}</a>"
+    message = "#{user_name} #{state_or_action_text} #{merge_request_link} in " \
       "#{project_link}: <b>#{title}</b>"
 
     if description

app/models/project_services/slack_service/merge_message.rb

@@ -13,6 +13,7 @@ class SlackService
       @user_name = params[:user][:name]
       @project_name = params[:project_name]
       @project_url = params[:project_url]
+      @action = params[:object_attributes][:action]
 
       obj_attr = params[:object_attributes]
       obj_attr = HashWithIndifferentAccess.new(obj_attr)
@@ -46,7 +47,7 @@ class SlackService
     end
 
     def merge_request_message
-      "#{user_name} #{state} #{merge_request_link} in #{project_link}: #{title}"
+      "#{user_name} #{state_or_action_text} #{merge_request_link} in #{project_link}: #{title}"
     end
 
     def merge_request_link
@@ -56,5 +57,9 @@ class SlackService
     def merge_request_url
       "#{project_url}/merge_requests/#{merge_request_id}"
     end
+
+    def state_or_action_text
+      @action == 'approved' ? @action : state
+    end
   end
 end

spec/models/project_services/hipchat_service_spec.rb

@@ -139,6 +139,7 @@ describe HipchatService, models: true do
       let(:merge_request) { create(:merge_request, description: 'please fix', title: 'Awesome merge request', target_project: project, source_project: project) }
       let(:merge_service) { MergeRequests::CreateService.new(project, user) }
       let(:merge_sample_data) { merge_service.hook_data(merge_request, 'open') }
+      let(:approved_merge_sample_data) { merge_service.hook_data(merge_request, 'approved') }
 
       it "should call Hipchat API for merge requests events" do
         hipchat.execute(merge_sample_data)
@@ -146,16 +147,28 @@ describe HipchatService, models: true do
         expect(WebMock).to have_requested(:post, api_url).once
       end
 
-      it "should create a merge request message" do
-        message = hipchat.send(:create_merge_request_message,
-                               merge_sample_data)
+      context 'merge request message' do
+        it 'creates a message for opened merge requests' do
+          message = hipchat.send(:create_merge_request_message, merge_sample_data)
 
-        obj_attr = merge_sample_data[:object_attributes]
-        expect(message).to eq("#{user.name} opened " \
-            "<a href=\"#{obj_attr[:url]}\">merge request !#{obj_attr["iid"]}</a> in " \
+          obj_attr = merge_sample_data[:object_attributes]
+          expect(message).to eq("#{user.name} opened " \
+            "<a href=\"#{obj_attr[:url]}\">merge request !#{obj_attr['iid']}</a> in " \
             "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
-            "<b>Awesome merge request</b>" \
-            "<pre>please fix</pre>")
+            '<b>Awesome merge request</b>' \
+            '<pre>please fix</pre>')
+        end
+
+        it 'creates a message for approved merge requests' do
+          message = hipchat.send(:create_merge_request_message, approved_merge_sample_data)
+
+          obj_attr = approved_merge_sample_data[:object_attributes]
+          expect(message).to eq("#{user.name} approved " \
+            "<a href=\"#{obj_attr[:url]}\">merge request !#{obj_attr['iid']}</a> in " \
+            "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
+            '<b>Awesome merge request</b>' \
+            '<pre>please fix</pre>')
+        end
       end
     end
 

spec/models/project_services/slack_service/merge_message_spec.rb

@@ -37,10 +37,24 @@ describe SlackService::MergeMessage, models: true do
     end
   end
 
+  context 'approval' do
+    before do
+      args[:object_attributes][:action] = 'approved'
+    end
+
+    it 'returns a message regarding approval of merge requests' do
+      expect(subject.pretext).to eq(
+        'Test User approved <somewhere.com/merge_requests/100|merge request !100> '\
+        'in <somewhere.com|project_name>: *Issue title*')
+      expect(subject.attachments).to be_empty
+    end
+  end
+
   context 'close' do
     before do
       args[:object_attributes][:state] = 'closed'
     end
+
     it 'returns a message regarding closing of merge requests' do
       expect(subject.pretext).to eq(
         'Test User closed <somewhere.com/merge_requests/100|merge request !100> '\