Fix merge request chat messages for adding and removing approvals

With GitLab 13.2 we have ported optional MR approvals to Core. If a user
uses an integration like Slack or Mattermost, the chat message will not
send the correct chat message. We can simply backport the methods to CE
in order to fix this problem.

See https://gitlab.com/gitlab-org/gitlab/-/issues/28138

app/models/project_services/chat_message/merge_message.rb

@@ -5,6 +5,7 @@ module ChatMessage
     attr_reader :merge_request_iid
     attr_reader :source_branch
     attr_reader :target_branch
+    attr_reader :action
     attr_reader :state
     attr_reader :title
 
@@ -16,6 +17,7 @@ module ChatMessage
       @merge_request_iid = obj_attr[:iid]
       @source_branch = obj_attr[:source_branch]
       @target_branch = obj_attr[:target_branch]
+      @action = obj_attr[:action]
       @state = obj_attr[:state]
       @title = format_title(obj_attr[:title])
     end
@@ -63,11 +65,17 @@ module ChatMessage
       "#{project_url}/-/merge_requests/#{merge_request_iid}"
     end
 
-    # overridden in EE
     def state_or_action_text
-      state
+      case action
+      when 'approved', 'unapproved'
+        action
+      when 'approval'
+        'added their approval to'
+      when 'unapproval'
+        'removed their approval from'
+      else
+        state
+      end
     end
   end
 end
-
-ChatMessage::MergeMessage.prepend_if_ee('::EE::ChatMessage::MergeMessage')

changelogs/unreleased/approval-notification-bug.yml

+---
+title: Fix merge request chat messages for adding and removing approvals
+merge_request: 41775
+author:
+type: fixed

ee/app/models/project_services/ee/chat_message/merge_message.rb

-# frozen_string_literal: true
-
-module EE
-  module ChatMessage
-    module MergeMessage
-      extend ActiveSupport::Concern
-      extend ::Gitlab::Utils::Override
-
-      prepended do
-        attr_reader :action
-      end
-
-      def initialize(params)
-        super
-
-        @action = params[:object_attributes][:action]
-      end
-
-      override :state_or_action_text
-      def state_or_action_text
-        case action
-        when 'approved', 'unapproved'
-          action
-        when 'approval'
-          'added their approval to'
-        when 'unapproval'
-          'removed their approval from'
-        else
-          super
-        end
-      end
-    end
-  end
-end

ee/spec/models/project_services/chat_message/merge_message_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe ChatMessage::MergeMessage do
-  subject { described_class.new(args) }
-
-  let(:args) do
-    {
-      user: {
-          name: 'Test User',
-          username: 'test.user',
-          avatar_url: 'http://someavatar.com'
-      },
-      project_name: 'project_name',
-      project_url: 'http://somewhere.com',
-
-      object_attributes: {
-        title: "Merge Request title\nSecond line",
-        id: 10,
-        iid: 100,
-        assignee_id: 1,
-        url: 'http://url.com',
-        state: 'opened',
-        description: 'merge request description',
-        source_branch: 'source_branch',
-        target_branch: 'target_branch'
-      }
-    }
-  end
-
-  context 'approved' do
-    before do
-      args[:object_attributes][:action] = 'approved'
-    end
-
-    it 'returns a message regarding completed approval of merge requests' do
-      expect(subject.pretext).to eq(
-        'Test User (test.user) approved merge request <http://somewhere.com/-/merge_requests/100|!100 *Merge Request title*> '\
-        'in <http://somewhere.com|project_name>')
-      expect(subject.attachments).to be_empty
-    end
-  end
-
-  context 'unapproved' do
-    before do
-      args[:object_attributes][:action] = 'unapproved'
-    end
-
-    it 'returns a message regarding revocation of completed approval of merge requests' do
-      expect(subject.pretext).to eq(
-        'Test User (test.user) unapproved merge request <http://somewhere.com/-/merge_requests/100|!100 *Merge Request title*> '\
-        'in <http://somewhere.com|project_name>')
-      expect(subject.attachments).to be_empty
-    end
-  end
-
-  context 'approval' do
-    before do
-      args[:object_attributes][:action] = 'approval'
-    end
-
-    it 'returns a message regarding added approval of merge requests' do
-      expect(subject.pretext).to eq(
-        'Test User (test.user) added their approval to merge request <http://somewhere.com/-/merge_requests/100|!100 *Merge Request title*> '\
-        'in <http://somewhere.com|project_name>')
-      expect(subject.attachments).to be_empty
-    end
-  end
-
-  context 'unapproval' do
-    before do
-      args[:object_attributes][:action] = 'unapproval'
-    end
-
-    it 'returns a message regarding revoking approval of merge requests' do
-      expect(subject.pretext).to eq(
-        'Test User (test.user) removed their approval from merge request <http://somewhere.com/-/merge_requests/100|!100 *Merge Request title*> '\
-        'in <http://somewhere.com|project_name>')
-      expect(subject.attachments).to be_empty
-    end
-  end
-end

spec/models/project_services/chat_message/merge_message_spec.rb

@@ -29,23 +29,6 @@ RSpec.describe ChatMessage::MergeMessage do
     }
   end
 
-  # Integration point in EE
-  context 'when state is overridden' do
-    it 'respects the overridden state' do
-      allow(subject).to receive(:state_or_action_text) { 'devoured' }
-
-      aggregate_failures do
-        expect(subject.summary).not_to include('opened')
-        expect(subject.summary).to include('devoured')
-
-        activity_title = subject.activity[:title]
-
-        expect(activity_title).not_to include('opened')
-        expect(activity_title).to include('devoured')
-      end
-    end
-  end
-
   context 'without markdown' do
     let(:color) { '#345' }
 
@@ -106,4 +89,56 @@ RSpec.describe ChatMessage::MergeMessage do
       end
     end
   end
+
+  context 'approved' do
+    before do
+      args[:object_attributes][:action] = 'approved'
+    end
+
+    it 'returns a message regarding completed approval of merge requests' do
+      expect(subject.pretext).to eq(
+        'Test User (test.user) approved merge request <http://somewhere.com/-/merge_requests/100|!100 *Merge Request title*> '\
+        'in <http://somewhere.com|project_name>')
+      expect(subject.attachments).to be_empty
+    end
+  end
+
+  context 'unapproved' do
+    before do
+      args[:object_attributes][:action] = 'unapproved'
+    end
+
+    it 'returns a message regarding revocation of completed approval of merge requests' do
+      expect(subject.pretext).to eq(
+        'Test User (test.user) unapproved merge request <http://somewhere.com/-/merge_requests/100|!100 *Merge Request title*> '\
+        'in <http://somewhere.com|project_name>')
+      expect(subject.attachments).to be_empty
+    end
+  end
+
+  context 'approval' do
+    before do
+      args[:object_attributes][:action] = 'approval'
+    end
+
+    it 'returns a message regarding added approval of merge requests' do
+      expect(subject.pretext).to eq(
+        'Test User (test.user) added their approval to merge request <http://somewhere.com/-/merge_requests/100|!100 *Merge Request title*> '\
+        'in <http://somewhere.com|project_name>')
+      expect(subject.attachments).to be_empty
+    end
+  end
+
+  context 'unapproval' do
+    before do
+      args[:object_attributes][:action] = 'unapproval'
+    end
+
+    it 'returns a message regarding revoking approval of merge requests' do
+      expect(subject.pretext).to eq(
+        'Test User (test.user) removed their approval from merge request <http://somewhere.com/-/merge_requests/100|!100 *Merge Request title*> '\
+        'in <http://somewhere.com|project_name>')
+      expect(subject.attachments).to be_empty
+    end
+  end
 end