Merge branch 'add-approved-event-for-MRs' into 'master'

Add approved event type for MRs

Closes #217101

See merge request gitlab-org/gitlab!32135

app/models/event.rb

@@ -21,6 +21,7 @@ class Event < ApplicationRecord
   LEFT      = 9 # User left project
   DESTROYED = 10
   EXPIRED   = 11 # User left project due to expiry
+  APPROVED  = 12
 
   ACTIONS = HashWithIndifferentAccess.new(
     created:    CREATED,
@@ -33,7 +34,8 @@ class Event < ApplicationRecord
     joined:     JOINED,
     left:       LEFT,
     destroyed:  DESTROYED,
-    expired:    EXPIRED
+    expired:    EXPIRED,
+    approved:   APPROVED
   ).freeze
 
   WIKI_ACTIONS = [CREATED, UPDATED, DESTROYED].freeze

ee/app/models/ee/event.rb

@@ -13,6 +13,7 @@ module EE
       scope :created, -> { where(action: ::Event::CREATED) }
       scope :closed, -> { where(action: ::Event::CLOSED) }
       scope :merged, -> { where(action: ::Event::MERGED) }
+      scope :approved, -> { where(action: ::Event::APPROVED) }
       scope :totals_by_author, -> { group(:author_id).count }
       scope :totals_by_author_target_type_action, -> { group(:author_id, :target_type, :action).count }
       scope :epics, -> { where(target_type: 'Epic') }
@@ -29,6 +30,15 @@ module EE
                       end
     end
 
+    override :action_name
+    def action_name
+      if approved_action?
+        'approved'
+      else
+        super
+      end
+    end
+
     def epic_note?
       note? && note_target.is_a?(::Epic)
     end
@@ -36,5 +46,9 @@ module EE
     def epic?
       target_type == 'Epic'
     end
+
+    def approved_action?
+      action == ::Event::APPROVED
+    end
   end
 end

ee/app/services/ee/event_create_service.rb

@@ -13,5 +13,9 @@ module EE
     def reopen_epic(epic, current_user)
       create_record_event(epic, current_user, ::Event::REOPENED)
     end
+
+    def approve_mr(merge_request, current_user)
+      create_record_event(merge_request, current_user, ::Event::APPROVED)
+    end
   end
 end

ee/app/services/merge_requests/approval_service.rb

@@ -13,10 +13,9 @@ module MergeRequests
 
       if save_approval(approval)
         merge_request.reset_approval_cache!
-
+        create_event(merge_request)
         create_approval_note(merge_request)
         mark_pending_todos_as_done(merge_request)
-        calculate_approvals_metrics(merge_request)
 
         if merge_request.approvals_left.zero?
           notification_service.async.approve_mr(merge_request, current_user)
@@ -51,7 +50,16 @@ module MergeRequests
     def calculate_approvals_metrics(merge_request)
       return unless merge_request.project.feature_available?(:code_review_analytics)
 
-      ::Analytics::RefreshApprovalsData.new(merge_request).execute_async
+      ::Analytics::RefreshApprovalsData.new(merge_request).execute
+    end
+
+    def create_event(merge_request)
+      # Making sure MergeRequest::Metrics updates are in sync with
+      # Event creation.
+      Event.transaction do
+        event_service.approve_mr(merge_request, current_user)
+        calculate_approvals_metrics(merge_request)
+      end
     end
   end
 end

ee/spec/factories/events.rb

@@ -9,5 +9,6 @@ FactoryBot.modify do
       action { Event::CREATED }
       project { nil }
     end
+    trait(:approved) { action { Event::APPROVED } }
   end
 end

ee/spec/models/ee/event_spec.rb

@@ -121,4 +121,24 @@ describe Event do
       end
     end
   end
+
+  describe '#action_name' do
+    let_it_be(:approved_event) {create(:event, :approved)}
+    let_it_be(:created_event) {create(:event, :created)}
+
+    it 'returns the appropriate action name' do
+      expect(approved_event.action_name).to eq 'approved'
+      expect(created_event.action_name).to eq 'created'
+    end
+  end
+
+  describe '#approved_action?' do
+    let_it_be(:approved_event) {create(:event, :approved)}
+    let_it_be(:created_event) {create(:event, :created)}
+
+    it 'return true only for approved event type' do
+      expect(approved_event.approved_action?).to be true
+      expect(created_event.approved_action?).to be false
+    end
+  end
 end

ee/spec/services/ee/event_create_service_spec.rb

@@ -49,4 +49,19 @@ describe EventCreateService do
       expect(event.group_id).to eq epic.group_id
     end
   end
+
+  describe 'Merge Requests' do
+    let_it_be(:user) { create(:user) }
+    let(:merge_request) { create(:merge_request) }
+
+    describe '#approve_mr' do
+      it { expect(service.approve_mr(merge_request, user)).to be_truthy }
+
+      it 'creates new event' do
+        service.approve_mr(merge_request, user)
+
+        change { Event.approved.where(target: merge_request).count }.by(1)
+      end
+    end
+  end
 end

ee/spec/services/merge_requests/approval_service_spec.rb

@@ -52,6 +52,15 @@ describe MergeRequests::ApprovalService do
         service.execute(merge_request)
       end
 
+      it 'creates approve MR event' do
+        expect_next_instance_of(EventCreateService) do |instance|
+          expect(instance).to receive(:approve_mr)
+            .with(merge_request, user)
+        end
+
+        service.execute(merge_request)
+      end
+
       context 'with remaining approvals' do
         it 'fires an approval webhook' do
           expect(merge_request).to receive(:approvals_left).and_return(5)
@@ -97,8 +106,8 @@ describe MergeRequests::ApprovalService do
           end
 
           it 'schedules RefreshApprovalsData' do
-            expect(Analytics::CodeReviewMetricsWorker)
-              .to receive(:perform_async).with('Analytics::RefreshApprovalsData', merge_request.id, {})
+            expect(::Analytics::RefreshApprovalsData)
+              .to receive_message_chain(:new, :execute)
 
             service.execute(merge_request)
           end
@@ -110,7 +119,7 @@ describe MergeRequests::ApprovalService do
           end
 
           it 'does not schedule for RefreshApprovalsData' do
-            expect(Analytics::CodeReviewMetricsWorker).not_to receive(:perform_async)
+            expect(::Analytics::RefreshApprovalsData).not_to receive(:new)
 
             service.execute(merge_request)
           end