Merge branch 'remove-resource-state-event-feature-flag' into 'master'

Remove track_resource_state_change_events flag

See merge request gitlab-org/gitlab!41287

app/models/merge_request.rb

@@ -1511,6 +1511,7 @@ class MergeRequest < ApplicationRecord
 
       metrics&.merged_at ||
         merge_event&.created_at ||
+        resource_state_events.find_by(state: :merged)&.created_at ||
         notes.system.reorder(nil).find_by(note: 'merged')&.created_at
     end
   end

app/services/system_notes/issuables_service.rb

@@ -242,19 +242,7 @@ module SystemNotes
     #
     # Returns the created Note object
     def change_status(status, source = nil)
-      body = status.dup
-      body << " via #{source.gfm_reference(project)}" if source
-
-      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.
-      if state_change_tracking_enabled?
       create_resource_state_event(status: status, mentionable_source: source)
-      else
-        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
@@ -312,23 +300,11 @@ module SystemNotes
     end
 
     def close_after_error_tracking_resolve
-      if state_change_tracking_enabled?
       create_resource_state_event(status: 'closed', close_after_error_tracking_resolve: true)
-      else
-        body = 'resolved the corresponding error and closed the issue.'
-
-        create_note(NoteSummary.new(noteable, project, author, body, action: 'closed'))
-      end
     end
 
     def auto_resolve_prometheus_alert
-      if state_change_tracking_enabled?
       create_resource_state_event(status: 'closed', close_auto_resolve_prometheus_alert: true)
-      else
-        body = 'automatically closed this issue because the alert resolved.'
-
-        create_note(NoteSummary.new(noteable, project, author, body, action: 'closed'))
-      end
     end
 
     private
@@ -361,11 +337,6 @@ module SystemNotes
         .execute(params)
     end
 
-    def state_change_tracking_enabled?
-      noteable.respond_to?(:resource_state_events) &&
-        ::Feature.enabled?(:track_resource_state_change_events, noteable.project, default_enabled: true)
-    end
-
     def issue_activity_counter
       Gitlab::UsageDataCounters::IssueActivityUniqueCounter
     end

config/feature_flags/development/track_resource_state_change_events.yml

----
-name: track_resource_state_change_events
-introduced_by_url: 
-rollout_issue_url: 
-group: 
-type: development
-default_enabled: true

ee/spec/services/epics/close_service_spec.rb

@@ -42,11 +42,6 @@ RSpec.describe Epics::CloseService do
             expect { subject.execute(epic) }.to change { epic.closed_at }
           end
 
-          context 'when state event tracking is enabled' do
-            before do
-              stub_feature_flags(track_resource_state_change_events: true)
-            end
-
           it 'creates a resource state event' do
             expect { subject.execute(epic) }.to change { epic.resource_state_events.count }.by(1)
 
@@ -54,22 +49,6 @@ RSpec.describe Epics::CloseService do
 
             expect(event.state).to eq('closed')
           end
-          end
-
-          context 'when state event tracking is disabled' do
-            before do
-              stub_feature_flags(track_resource_state_change_events: false)
-            end
-
-            it 'creates a system note about epic close' do
-              expect { subject.execute(epic) }.to change { epic.notes.count }.by(1)
-
-              note = epic.notes.last
-
-              expect(note.note).to eq('closed')
-              expect(note.system_note_metadata.action).to eq('closed')
-            end
-          end
 
           it 'notifies the subscribers' do
             notification_service = double

ee/spec/services/epics/reopen_service_spec.rb

@@ -42,11 +42,6 @@ RSpec.describe Epics::ReopenService do
             expect { subject.execute(epic) }.to change { epic.closed_at }.to(nil)
           end
 
-          context 'when state event tracking is enabled' do
-            before do
-              stub_feature_flags(track_resource_state_change_events: true)
-            end
-
           it 'creates a resource state event' do
             expect { subject.execute(epic) }.to change { epic.resource_state_events.count }.by(1)
 
@@ -54,22 +49,6 @@ RSpec.describe Epics::ReopenService do
 
             expect(event.state).to eq('opened')
           end
-          end
-
-          context 'when state event tracking is disabled' do
-            before do
-              stub_feature_flags(track_resource_state_change_events: false)
-            end
-
-            it 'creates a system note about epic reopen' do
-              expect { subject.execute(epic) }.to change { epic.notes.count }.by(1)
-
-              note = epic.notes.last
-
-              expect(note.note).to eq('opened')
-              expect(note.system_note_metadata.action).to eq('opened')
-            end
-          end
 
           it 'notifies the subscribers' do
             notification_service = double

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

@@ -65,28 +65,19 @@ RSpec.describe Gitlab::Email::Handler::CreateNoteHandler do
         end
       end
 
-      [true, false].each do |state_tracking_enabled|
-        context "and current user can update noteable #{state_tracking_enabled ? 'enabled' : 'disabled'}" do
+      context "and current user can update noteable" 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

@@ -2358,48 +2358,43 @@ RSpec.describe MergeRequest, factory_default: :keep 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
+    context 'when no metrics or merge event exists' do
       let(:user) { create(:user) }
       let(:merge_request) { create(:merge_request, :merged) }
 
       before do
         merge_request.metrics.destroy!
+      end
 
+      context 'when resource event for the merge exists' do
+        before do
           SystemNoteService.change_status(merge_request,
                                           merge_request.target_project,
                                           user,
                                           merge_request.state, nil)
         end
 
-        it 'returns merging note creation date' do
+        it 'returns the resource event creation date' do
           expect(merge_request.reload.metrics).to be_nil
           expect(merge_request.merge_event).to be_nil
-          expect(merge_request.notes.count).to eq(1)
-          expect(merge_request.merged_at).to eq(merge_request.notes.first.created_at)
-        end
+          expect(merge_request.resource_state_events.count).to eq(1)
+          expect(merge_request.merged_at).to eq(merge_request.resource_state_events.first.created_at)
         end
       end
 
-    context 'when state event tracking is enabled' do
-      let(:user) { create(:user) }
-      let(:merge_request) { create(:merge_request, :merged) }
-
+      context 'when system note for the merge exists' do
         before do
-        merge_request.metrics.destroy!
-
-        SystemNoteService.change_status(merge_request,
-                                        merge_request.target_project,
-                                        user,
-                                        merge_request.state, nil)
+          # We do not create these system notes anymore but we need this to work for existing MRs
+          # that used system notes instead of resource state events
+          create(:note, :system, noteable: merge_request, note: 'merged')
         end
 
-      it 'does not create a system note' do
-        expect(merge_request.notes).to be_empty
+        it 'returns the merging note creation date' do
+          expect(merge_request.reload.metrics).to be_nil
+          expect(merge_request.merge_event).to be_nil
+          expect(merge_request.notes.count).to eq(1)
+          expect(merge_request.merged_at).to eq(merge_request.notes.first.created_at)
+        end
       end
     end
   end

spec/services/alert_management/process_prometheus_alert_service_spec.rb

@@ -148,12 +148,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
               expect { execute }.to change { alert.reload.resolved? }.to(true)
             end
 
-            [true, false].each do |state_tracking_enabled|
             context 'existing issue' do
-                before do
-                  stub_feature_flags(track_resource_state_change_events: state_tracking_enabled)
-                end
-
               let!(:alert) { create(:alert_management_alert, :with_issue, project: project, fingerprint: fingerprint) }
 
               it 'closes the issue' do
@@ -165,11 +160,8 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
                   .to('closed')
               end
 
-                if state_tracking_enabled
-                  specify { expect { execute }.to change(ResourceStateEvent, :count).by(1) }
-                else
-                  specify { expect { execute }.to change(Note, :count).by(1) }
-                end
+              it 'creates a resource state event' do
+                expect { execute }.to change(ResourceStateEvent, :count).by(1)
               end
             end
           end

spec/services/issues/close_service_spec.rb

@@ -233,27 +233,12 @@ RSpec.describe Issues::CloseService do
         expect(email.subject).to include(issue.title)
       end
 
-      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,13 +19,10 @@ RSpec.describe MergeRequests::CloseService do
   describe '#execute' do
     it_behaves_like 'cache counters invalidator'
 
-    [true, false].each do |state_tracking_enabled|
-      context "valid params with state_tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}" do
+    context 'valid params' 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
@@ -47,14 +44,9 @@ RSpec.describe MergeRequests::CloseService do
         expect(email.subject).to include(merge_request.title)
       end
 
-        it 'creates system note about merge_request reassign' do
-          if state_tracking_enabled
+      it 'creates a resource event' do
         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
@@ -69,7 +61,6 @@ RSpec.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

@@ -22,8 +22,7 @@ RSpec.describe MergeRequests::FfMergeService do
   end
 
   describe '#execute' do
-    [true, false].each do |state_tracking_enabled|
-      context "valid params with state_tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}" do
+    context 'valid params' do
       let(:service) { described_class.new(project, user, valid_merge_params) }
 
       def execute_ff_merge
@@ -33,8 +32,6 @@ RSpec.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
 
@@ -67,16 +64,11 @@ RSpec.describe MergeRequests::FfMergeService do
         expect(email.subject).to include(merge_request.title)
       end
 
-        it 'creates system note about merge_request merge' do
+      it 'creates resource event 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
@@ -91,7 +83,6 @@ RSpec.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,11 +20,7 @@ RSpec.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
@@ -47,22 +43,11 @@ RSpec.describe MergeRequests::MergeService do
       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

spec/services/merge_requests/refresh_service_spec.rb

@@ -367,12 +367,7 @@ RSpec.describe MergeRequests::RefreshService do
       end
     end
 
-    [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 'push to origin repo target branch', :sidekiq_might_not_need_inline do
       context 'when all MRs to the target branch had diffs' do
         before do
           service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature')
@@ -385,13 +380,8 @@ RSpec.describe MergeRequests::RefreshService do
           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
 
@@ -422,21 +412,13 @@ RSpec.describe MergeRequests::RefreshService do
           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 #{state_tracking_enabled ? 'enabled' : 'disabled'}", :sidekiq_might_not_need_inline do
+      context 'manual merge of source branch', :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')
@@ -445,13 +427,8 @@ RSpec.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
@@ -616,22 +593,15 @@ RSpec.describe MergeRequests::RefreshService do
       end
     end
 
-    [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
+    context 'push to origin repo target branch after fork project was removed' 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
@@ -640,7 +610,6 @@ RSpec.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,11 +20,8 @@ RSpec.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
@@ -47,23 +44,12 @@ RSpec.describe MergeRequests::ReopenService do
       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/projects/alerting/notify_service_spec.rb

@@ -127,24 +127,9 @@ RSpec.describe Projects::Alerting::NotifyService do
                   let(:alert) { create(:alert_management_alert, :with_issue, project: project, fingerprint: fingerprint_sha) }
                   let(:issue) { alert.issue }
 
-                  context 'state_tracking is enabled' do
-                    before do
-                      stub_feature_flags(track_resource_state_change_events: true)
-                    end
-
                   it { expect { subject }.to change { issue.reload.state }.from('opened').to('closed') }
                   it { expect { subject }.to change(ResourceStateEvent, :count).by(1) }
                 end
-
-                  context 'state_tracking is disabled' do
-                    before do
-                      stub_feature_flags(track_resource_state_change_events: false)
-                    end
-
-                    it { expect { subject }.to change { issue.reload.state }.from('opened').to('closed') }
-                    it { expect { subject }.to change(Note, :count).by(1) }
-                  end
-                end
               end
             end
 

spec/services/system_notes/issuables_service_spec.rb

@@ -131,43 +131,11 @@ RSpec.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 'does not change note count' do
-        expect { subject }.not_to change { Note.count }
-      end
-    end
-
-    context 'with status reopened' do
-      before do
-        stub_feature_flags(track_resource_state_change_events: false)
-      end
-
-      let(:status) { 'reopened' }
-      let(:source) { nil }
-
-      it_behaves_like 'a note with overridable created_at'
-
-      it_behaves_like 'a system note' do
-        let(:action) { 'opened' }
-      end
-    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') }
-
-      it_behaves_like 'a note with overridable created_at'
-
-      it 'sets the note text' do
-        expect(subject.note).to eq "#{status} via commit 123456"
-      end
+    it 'creates a resource state event' do
+      expect { subject }.to change { ResourceStateEvent.count }.by(1)
     end
   end
 
@@ -636,11 +604,6 @@ RSpec.describe ::SystemNotes::IssuablesService do
   describe '#close_after_error_tracking_resolve' do
     subject { service.close_after_error_tracking_resolve }
 
-    context 'when state tracking is enabled' do
-      before do
-        stub_feature_flags(track_resource_state_change_events: true)
-      end
-
     it 'creates the expected state event' do
       subject
 
@@ -651,30 +614,9 @@ RSpec.describe ::SystemNotes::IssuablesService do
     end
   end
 
-    context 'when state tracking is disabled' do
-      before do
-        stub_feature_flags(track_resource_state_change_events: false)
-      end
-
-      it_behaves_like 'a system note' do
-        let(:action) { 'closed' }
-      end
-
-      it 'creates the expected system note' do
-        expect(subject.note)
-          .to eq('resolved the corresponding error and closed the issue.')
-      end
-    end
-  end
-
   describe '#auto_resolve_prometheus_alert' do
     subject { service.auto_resolve_prometheus_alert }
 
-    context 'when state tracking is enabled' do
-      before do
-        stub_feature_flags(track_resource_state_change_events: true)
-      end
-
     it 'creates the expected state event' do
       subject
 
@@ -684,19 +626,4 @@ RSpec.describe ::SystemNotes::IssuablesService do
       expect(event.state).to eq('closed')
     end
   end
-
-    context 'when state tracking is disabled' do
-      before do
-        stub_feature_flags(track_resource_state_change_events: false)
-      end
-
-      it_behaves_like 'a system note' do
-        let(:action) { 'closed' }
-      end
-
-      it 'creates the expected system note' do
-        expect(subject.note).to eq('automatically closed this issue because the alert resolved.')
-      end
-    end
-  end
 end