Merge branch 'approval-notification-bug' into 'master'

Fix merge request chat messages for adding and removing approvals

See merge request gitlab-org/gitlab!41775

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