Add resource state event tracking

Also add module for StateEventable
and extracted ResourceEvent modules.

Change note text for state notes

app/services/event_create_service.rb

@@ -11,30 +11,44 @@ class EventCreateService
   IllegalActionError = Class.new(StandardError)
 
   def open_issue(issue, current_user)
+    create_resource_event(issue, current_user, :opened)
+
     create_record_event(issue, current_user, Event::CREATED)
   end
 
   def close_issue(issue, current_user)
+    create_resource_event(issue, current_user, :closed)
+
     create_record_event(issue, current_user, Event::CLOSED)
   end
 
   def reopen_issue(issue, current_user)
+    create_resource_event(issue, current_user, :reopened)
+
     create_record_event(issue, current_user, Event::REOPENED)
   end
 
   def open_mr(merge_request, current_user)
+    create_resource_event(merge_request, current_user, :opened)
+
     create_record_event(merge_request, current_user, Event::CREATED)
   end
 
   def close_mr(merge_request, current_user)
+    create_resource_event(merge_request, current_user, :closed)
+
     create_record_event(merge_request, current_user, Event::CLOSED)
   end
 
   def reopen_mr(merge_request, current_user)
+    create_resource_event(merge_request, current_user, :reopened)
+
     create_record_event(merge_request, current_user, Event::REOPENED)
   end
 
   def merge_mr(merge_request, current_user)
+    create_resource_event(merge_request, current_user, :merged)
+
     create_record_event(merge_request, current_user, Event::MERGED)
   end
 
@@ -157,6 +171,18 @@ class EventCreateService
 
     Event.create!(attributes)
   end
+
+  def create_resource_event(issuable, current_user, status)
+    return unless state_change_tracking_enabled?(issuable)
+
+    ResourceEvents::ChangeStateService.new(resource: issuable, user: current_user)
+      .execute(status)
+  end
+
+  def state_change_tracking_enabled?(issuable)
+    issuable&.respond_to?(:resource_state_events) &&
+      ::Feature.enabled?(:track_resource_state_change_events, issuable&.project)
+  end
 end
 
 EventCreateService.prepend_if_ee('EE::EventCreateService')

app/services/resource_events/change_state_service.rb

+# frozen_string_literal: true
+
+module ResourceEvents
+  class ChangeStateService
+    attr_reader :resource, :user
+
+    def initialize(user:, resource:)
+      @user, @resource = user, resource
+    end
+
+    def execute(state)
+      ResourceStateEvent.create(
+        user: user,
+        issue: issue,
+        merge_request: merge_request,
+        state: ResourceStateEvent.states[state],
+        created_at: Time.zone.now)
+
+      resource.expire_note_etag_cache
+    end
+
+    private
+
+    def issue
+      return unless resource.is_a?(Issue)
+
+      resource
+    end
+
+    def merge_request
+      return unless resource.is_a?(MergeRequest)
+
+      resource
+    end
+  end
+end

app/services/resource_events/merge_into_notes_service.rb

@@ -11,7 +11,8 @@ module ResourceEvents
 
     SYNTHETIC_NOTE_BUILDER_SERVICES = [
       SyntheticLabelNotesBuilderService,
-      SyntheticMilestoneNotesBuilderService
+      SyntheticMilestoneNotesBuilderService,
+      SyntheticStateNotesBuilderService
     ].freeze
 
     attr_reader :resource, :current_user, :params
@@ -23,7 +24,7 @@ module ResourceEvents
     end
 
     def execute(notes = [])
-      (notes + synthetic_notes).sort_by { |n| n.created_at }
+      (notes + synthetic_notes).sort_by(&:created_at)
     end
 
     private

app/services/resource_events/synthetic_state_notes_builder_service.rb

+# frozen_string_literal: true
+
+module ResourceEvents
+  class SyntheticStateNotesBuilderService < BaseSyntheticNotesBuilderService
+    private
+
+    def synthetic_notes
+      state_change_events.map do |event|
+        StateNote.from_event(event, resource: resource, resource_parent: resource_parent)
+      end
+    end
+
+    def state_change_events
+      return [] unless resource.respond_to?(:resource_state_events)
+
+      events = resource.resource_state_events.includes(user: :status) # rubocop: disable CodeReuse/ActiveRecord
+      since_fetch_at(events)
+    end
+  end
+end

app/services/system_notes/issuables_service.rb

@@ -225,8 +225,13 @@ module SystemNotes
 
       action = status == 'reopened' ? 'opened' : status
 
+      # A state event which results in a synthetic note will be
+      # created by EventCreateService if change event tracking
+      # is enabled.
+      unless state_change_tracking_enabled?
         create_note(NoteSummary.new(noteable, project, author, body, action: action))
       end
+    end
 
     # Check if a cross reference to a noteable from a mentioner already exists
     #
@@ -318,6 +323,11 @@ module SystemNotes
     def self.cross_reference?(note_text)
       note_text =~ /\A#{cross_reference_note_prefix}/i
     end
+
+    def state_change_tracking_enabled?
+      noteable.respond_to?(:resource_state_events) &&
+        ::Feature.enabled?(:track_resource_state_change_events, noteable.project)
+    end
   end
 end
 

spec/lib/gitlab/email/handler/create_note_handler_spec.rb

@@ -65,20 +65,28 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
         end
       end
 
-      context 'and current user can update noteable' do
+      [true, false].each do |state_tracking_enabled|
+        context "and current user can update noteable #{state_tracking_enabled ? 'enabled' : 'disabled'}" do
           before do
+            stub_feature_flags(track_resource_state_change_events: state_tracking_enabled)
+
             project.add_developer(user)
           end
 
           it 'does not raise an error' do
+            if state_tracking_enabled
+              expect { receiver.execute }.to change { noteable.resource_state_events.count }.by(1)
+            else
               # One system note is created for the 'close' event
               expect { receiver.execute }.to change { noteable.notes.count }.by(1)
+            end
 
             expect(noteable.reload).to be_closed
           end
         end
       end
     end
+  end
 
   context 'when the note contains quick actions' do
     let!(:email_raw) { fixture_file("emails/commands_in_reply.eml") }

spec/models/merge_request_spec.rb

@@ -2157,6 +2157,11 @@ describe MergeRequest do
       end
     end
 
+    context 'when state event tracking is disabled' do
+      before do
+        stub_feature_flags(track_resource_state_change_events: false)
+      end
+
       context 'when merging note is persisted, but no metrics or merge event exists' do
         let(:user) { create(:user) }
         let(:merge_request) { create(:merge_request, :merged) }
@@ -2179,6 +2184,25 @@ describe MergeRequest do
       end
     end
 
+    context 'when state event tracking is enabled' do
+      let(:user) { create(:user) }
+      let(:merge_request) { create(:merge_request, :merged) }
+
+      before do
+        merge_request.metrics.destroy!
+
+        SystemNoteService.change_status(merge_request,
+                                        merge_request.target_project,
+                                        user,
+                                        merge_request.state, nil)
+      end
+
+      it 'does not create a system note' do
+        expect(merge_request.notes).to be_empty
+      end
+    end
+  end
+
   describe '#participants' do
     let(:project) { create(:project, :public) }
 

spec/services/event_create_service_spec.rb

@@ -13,6 +13,7 @@ describe EventCreateService do
 
       it "creates new event" do
         expect { service.open_issue(issue, issue.author) }.to change { Event.count }
+        expect { service.open_issue(issue, issue.author) }.to change { ResourceStateEvent.count }
       end
     end
 
@@ -23,6 +24,7 @@ describe EventCreateService do
 
       it "creates new event" do
         expect { service.close_issue(issue, issue.author) }.to change { Event.count }
+        expect { service.close_issue(issue, issue.author) }.to change { ResourceStateEvent.count }
       end
     end
 
@@ -33,6 +35,7 @@ describe EventCreateService do
 
       it "creates new event" do
         expect { service.reopen_issue(issue, issue.author) }.to change { Event.count }
+        expect { service.reopen_issue(issue, issue.author) }.to change { ResourceStateEvent.count }
       end
     end
   end
@@ -45,6 +48,7 @@ describe EventCreateService do
 
       it "creates new event" do
         expect { service.open_mr(merge_request, merge_request.author) }.to change { Event.count }
+        expect { service.open_mr(merge_request, merge_request.author) }.to change { ResourceStateEvent.count }
       end
     end
 
@@ -55,6 +59,7 @@ describe EventCreateService do
 
       it "creates new event" do
         expect { service.close_mr(merge_request, merge_request.author) }.to change { Event.count }
+        expect { service.close_mr(merge_request, merge_request.author) }.to change { ResourceStateEvent.count }
       end
     end
 
@@ -65,6 +70,7 @@ describe EventCreateService do
 
       it "creates new event" do
         expect { service.merge_mr(merge_request, merge_request.author) }.to change { Event.count }
+        expect { service.merge_mr(merge_request, merge_request.author) }.to change { ResourceStateEvent.count }
       end
     end
 
@@ -75,6 +81,7 @@ describe EventCreateService do
 
       it "creates new event" do
         expect { service.reopen_mr(merge_request, merge_request.author) }.to change { Event.count }
+        expect { service.reopen_mr(merge_request, merge_request.author) }.to change { ResourceStateEvent.count }
       end
     end
   end

spec/services/issues/close_service_spec.rb

@@ -224,12 +224,27 @@ describe Issues::CloseService do
         expect(email.subject).to include(issue.title)
       end
 
-      it 'creates system note about issue reassign' do
+      context 'when resource state events are disabled' do
+        before do
+          stub_feature_flags(track_resource_state_change_events: false)
+        end
+
+        it 'creates system note about the issue being closed' do
           close_issue
 
           note = issue.notes.last
           expect(note.note).to include "closed"
         end
+      end
+
+      context 'when resource state events are enabled' do
+        it 'creates resource state event about the issue being closed' do
+          close_issue
+
+          event = issue.resource_state_events.last
+          expect(event.state).to eq('closed')
+        end
+      end
 
       it 'marks todos as done' do
         close_issue

spec/services/merge_requests/close_service_spec.rb

@@ -19,10 +19,13 @@ describe MergeRequests::CloseService do
   describe '#execute' do
     it_behaves_like 'cache counters invalidator'
 
-    context 'valid params' do
+    [true, false].each do |state_tracking_enabled|
+      context "valid params with state_tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}" do
         let(:service) { described_class.new(project, user, {}) }
 
         before do
+          stub_feature_flags(track_resource_state_change_events: state_tracking_enabled)
+
           allow(service).to receive(:execute_hooks)
 
           perform_enqueued_jobs do
@@ -45,9 +48,14 @@ describe MergeRequests::CloseService do
         end
 
         it 'creates system note about merge_request reassign' do
+          if state_tracking_enabled
+            event = @merge_request.resource_state_events.last
+            expect(event.state).to eq('closed')
+          else
             note = @merge_request.notes.last
             expect(note.note).to include 'closed'
           end
+        end
 
         it 'marks todos as done' do
           expect(todo.reload).to be_done
@@ -61,6 +69,7 @@ describe MergeRequests::CloseService do
           end
         end
       end
+    end
 
     it 'updates metrics' do
       metrics = merge_request.metrics

spec/services/merge_requests/ff_merge_service_spec.rb

@@ -21,7 +21,8 @@ describe MergeRequests::FfMergeService do
   end
 
   describe '#execute' do
-    context 'valid params' do
+    [true, false].each do |state_tracking_enabled|
+      context "valid params with state_tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}" do
         let(:service) { described_class.new(project, user, valid_merge_params) }
 
         def execute_ff_merge
@@ -31,6 +32,8 @@ describe MergeRequests::FfMergeService do
         end
 
         before do
+          stub_feature_flags(track_resource_state_change_events: state_tracking_enabled)
+
           allow(service).to receive(:execute_hooks)
         end
 
@@ -66,9 +69,14 @@ describe MergeRequests::FfMergeService do
         it 'creates system note about merge_request merge' do
           execute_ff_merge
 
+          if state_tracking_enabled
+            event = merge_request.resource_state_events.last
+            expect(event.state).to eq('merged')
+          else
             note = merge_request.notes.last
             expect(note.note).to include 'merged'
           end
+        end
 
         it 'does not update squash_commit_sha if it is not a squash' do
           expect { execute_ff_merge }.not_to change { merge_request.squash_commit_sha }
@@ -82,6 +90,7 @@ describe MergeRequests::FfMergeService do
             .from(nil)
         end
       end
+    end
 
     context 'error handling' do
       let(:service) { described_class.new(project, user, valid_merge_params.merge(commit_message: 'Awesome message')) }

spec/services/merge_requests/merge_service_spec.rb

@@ -20,7 +20,11 @@ describe MergeRequests::MergeService do
     end
 
     context 'valid params' do
+      let(:state_tracking) { true }
+
       before do
+        stub_feature_flags(track_resource_state_change_events: state_tracking)
+
         allow(service).to receive(:execute_hooks)
 
         perform_enqueued_jobs do
@@ -42,10 +46,23 @@ describe MergeRequests::MergeService do
         expect(email.subject).to include(merge_request.title)
       end
 
+      context 'note creation' do
+        context 'when resource state event tracking is disabled' do
+          let(:state_tracking) { false }
+
           it 'creates system note about merge_request merge' do
             note = merge_request.notes.last
             expect(note.note).to include 'merged'
           end
+        end
+
+        context 'when resource state event tracking is enabled' do
+          it 'creates resource state event about merge_request merge' do
+            event = merge_request.resource_state_events.last
+            expect(event.state).to eq('merged')
+          end
+        end
+      end
 
       context 'when squashing' do
         let(:merge_params) do
@@ -55,7 +72,7 @@ describe MergeRequests::MergeService do
         end
 
         let(:merge_request) do
-          # A merge reqeust with 5 commits
+          # A merge request with 5 commits
           create(:merge_request, :simple,
                  author: user2,
                  assignees: [user2],

spec/services/merge_requests/refresh_service_spec.rb

@@ -362,7 +362,12 @@ describe MergeRequests::RefreshService do
       end
     end
 
-    context 'push to origin repo target branch', :sidekiq_might_not_need_inline do
+    [true, false].each do |state_tracking_enabled|
+      context "push to origin repo target branch with state tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}", :sidekiq_might_not_need_inline do
+        before do
+          stub_feature_flags(track_resource_state_change_events: state_tracking_enabled)
+        end
+
         context 'when all MRs to the target branch had diffs' do
           before do
             service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature')
@@ -370,12 +375,18 @@ describe MergeRequests::RefreshService do
           end
 
           it 'updates the merge state' do
-          expect(@merge_request.notes.last.note).to include('merged')
             expect(@merge_request).to be_merged
             expect(@fork_merge_request).to be_merged
-          expect(@fork_merge_request.notes.last.note).to include('merged')
             expect(@build_failed_todo).to be_done
             expect(@fork_build_failed_todo).to be_done
+
+            if state_tracking_enabled
+              expect(@merge_request.resource_state_events.last.state).to eq('merged')
+              expect(@fork_merge_request.resource_state_events.last.state).to eq('merged')
+            else
+              expect(@merge_request.notes.last.note).to include('merged')
+              expect(@fork_merge_request.notes.last.note).to include('merged')
+            end
           end
         end
 
@@ -400,20 +411,27 @@ describe MergeRequests::RefreshService do
 
           it 'only updates the non-empty MRs' do
             expect(@merge_request).to be_merged
-          expect(@merge_request.notes.last.note).to include('merged')
-
             expect(@fork_merge_request).to be_merged
-          expect(@fork_merge_request.notes.last.note).to include('merged')
 
             expect(empty_fork_merge_request).to be_open
             expect(empty_fork_merge_request.merge_request_diff.state).to eq('empty')
             expect(empty_fork_merge_request.notes).to be_empty
+
+            if state_tracking_enabled
+              expect(@merge_request.resource_state_events.last.state).to eq('merged')
+              expect(@fork_merge_request.resource_state_events.last.state).to eq('merged')
+            else
+              expect(@merge_request.notes.last.note).to include('merged')
+              expect(@fork_merge_request.notes.last.note).to include('merged')
+            end
           end
         end
       end
 
-    context 'manual merge of source branch', :sidekiq_might_not_need_inline do
+      context "manual merge of source branch #{state_tracking_enabled ? 'enabled' : 'disabled'}", :sidekiq_might_not_need_inline do
         before do
+          stub_feature_flags(track_resource_state_change_events: state_tracking_enabled)
+
           # Merge master -> feature branch
           @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, 'Test message')
           commit = @project.repository.commit('feature')
@@ -422,15 +440,22 @@ describe MergeRequests::RefreshService do
         end
 
         it 'updates the merge state' do
+          if state_tracking_enabled
+            expect(@merge_request.resource_state_events.last.state).to eq('merged')
+            expect(@fork_merge_request.resource_state_events.last.state).to eq('merged')
+          else
             expect(@merge_request.notes.last.note).to include('merged')
+            expect(@fork_merge_request.notes.last.note).to include('merged')
+          end
+
           expect(@merge_request).to be_merged
           expect(@merge_request.diffs.size).to be > 0
           expect(@fork_merge_request).to be_merged
-        expect(@fork_merge_request.notes.last.note).to include('merged')
           expect(@build_failed_todo).to be_done
           expect(@fork_build_failed_todo).to be_done
         end
       end
+    end
 
     context 'push to fork repo source branch', :sidekiq_might_not_need_inline do
       let(:refresh_service) { service.new(@fork_project, @user) }
@@ -583,15 +608,23 @@ describe MergeRequests::RefreshService do
       end
     end
 
-    context 'push to origin repo target branch after fork project was removed' do
+    [true, false].each do |state_tracking_enabled|
+      context "push to origin repo target branch after fork project was removed #{state_tracking_enabled ? 'enabled' : 'disabled'}" do
         before do
+          stub_feature_flags(track_resource_state_change_events: state_tracking_enabled)
+
           @fork_project.destroy
           service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature')
           reload_mrs
         end
 
         it 'updates the merge request state' do
+          if state_tracking_enabled
+            expect(@merge_request.resource_state_events.last.state).to eq('merged')
+          else
             expect(@merge_request.notes.last.note).to include('merged')
+          end
+
           expect(@merge_request).to be_merged
           expect(@fork_merge_request).to be_open
           expect(@fork_merge_request.notes).to be_empty
@@ -599,6 +632,7 @@ describe MergeRequests::RefreshService do
           expect(@fork_build_failed_todo).to be_done
         end
       end
+    end
 
     context 'push new branch that exists in a merge request' do
       let(:refresh_service) { service.new(@fork_project, @user) }

spec/services/merge_requests/reopen_service_spec.rb

@@ -20,8 +20,11 @@ describe MergeRequests::ReopenService do
 
     context 'valid params' do
       let(:service) { described_class.new(project, user, {}) }
+      let(:state_tracking) { true }
 
       before do
+        stub_feature_flags(track_resource_state_change_events: state_tracking)
+
         allow(service).to receive(:execute_hooks)
 
         perform_enqueued_jobs do
@@ -43,12 +46,25 @@ describe MergeRequests::ReopenService do
         expect(email.subject).to include(merge_request.title)
       end
 
+      context 'note creation' do
+        context 'when state event tracking is disabled' do
+          let(:state_tracking) { false }
+
           it 'creates system note about merge_request reopen' do
             note = merge_request.notes.last
             expect(note.note).to include 'reopened'
           end
         end
 
+        context 'when state event tracking is enabled' do
+          it 'creates resource state event about merge_request reopen' do
+            event = merge_request.resource_state_events.last
+            expect(event.state).to eq('reopened')
+          end
+        end
+      end
+    end
+
     it 'caches merge request closing issues' do
       expect(merge_request).to receive(:cache_merge_request_closes_issues!)
 

spec/services/resource_events/change_state_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ResourceEvents::ChangeStateService do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+
+  let(:issue) { create(:issue, project: project) }
+  let(:merge_request) { create(:merge_request, source_project: project) }
+
+  describe '#execute' do
+    context 'when resource is an issue' do
+      %w[opened reopened closed locked].each do |state|
+        it "creates the expected event if issue has #{state} state" do
+          described_class.new(user: user, resource: issue).execute(state)
+
+          event = issue.resource_state_events.last
+          expect(event.issue).to eq(issue)
+          expect(event.merge_request).to be_nil
+          expect(event.state).to eq(state)
+        end
+      end
+    end
+
+    context 'when resource is a merge request' do
+      %w[opened reopened closed locked merged].each do |state|
+        it "creates the expected event if merge request has #{state} state" do
+          described_class.new(user: user, resource: merge_request).execute(state)
+
+          event = merge_request.resource_state_events.last
+          expect(event.issue).to be_nil
+          expect(event.merge_request).to eq(merge_request)
+          expect(event.state).to eq(state)
+        end
+      end
+    end
+  end
+end

spec/services/system_notes/issuables_service_spec.rb

@@ -157,7 +157,18 @@ describe ::SystemNotes::IssuablesService do
   describe '#change_status' do
     subject { service.change_status(status, source) }
 
+    context 'when resource state event tracking is enabled' do
+      let(:status) { 'reopened' }
+      let(:source) { nil }
+
+      it { is_expected.to be_nil }
+    end
+
     context 'with status reopened' do
+      before do
+        stub_feature_flags(track_resource_state_change_events: false)
+      end
+
       let(:status) { 'reopened' }
       let(:source) { nil }
 
@@ -169,6 +180,10 @@ describe ::SystemNotes::IssuablesService do
     end
 
     context 'with a source' do
+      before do
+        stub_feature_flags(track_resource_state_change_events: false)
+      end
+
       let(:status) { 'opened' }
       let(:source) { double('commit', gfm_reference: 'commit 123456') }
 

spec/support/shared_examples/features/discussion_comments_shared_example.rb

@@ -30,6 +30,8 @@ RSpec.shared_examples 'thread comments' do |resource_name|
 
       click_button 'Comment & close issue'
 
+      wait_for_all_requests
+
       expect(page).to have_content(comment)
       expect(page).to have_content "@#{user.username} closed"