Merge branch '229147-remove-resource-event-feature-flags' into 'master'

Remove track_resource_milestone_change_events flag

See merge request gitlab-org/gitlab!40946

app/services/issuable/common_system_notes_service.rb

@@ -22,7 +22,7 @@ module Issuable
         end
 
         create_due_date_note if issuable.previous_changes.include?('due_date')
-        create_milestone_note(old_milestone) if issuable.previous_changes.include?('milestone_id')
+        create_milestone_change_event(old_milestone) if issuable.previous_changes.include?('milestone_id')
         create_labels_note(old_labels) if old_labels && issuable.labels != old_labels
       end
     end
@@ -94,23 +94,11 @@ module Issuable
       SystemNoteService.change_time_spent(issuable, issuable.project, issuable.time_spent_user)
     end
 
-    def create_milestone_note(old_milestone)
-      if milestone_changes_tracking_enabled?
-        create_milestone_change_event(old_milestone)
-      else
-        SystemNoteService.change_milestone(issuable, issuable.project, current_user, issuable.milestone)
-      end
-    end
-
     def create_milestone_change_event(old_milestone)
       ResourceEvents::ChangeMilestoneService.new(issuable, current_user, old_milestone: old_milestone)
         .execute
     end
 
-    def milestone_changes_tracking_enabled?
-      ::Feature.enabled?(:track_resource_milestone_change_events, issuable.project, default_enabled: true)
-    end
-
     def create_due_date_note
       SystemNoteService.change_due_date(issuable, issuable.project, current_user, issuable.due_date)
     end

app/services/system_note_service.rb

@@ -41,10 +41,6 @@ module SystemNoteService
     ::SystemNotes::IssuablesService.new(noteable: issuable, project: project, author: author).change_issuable_assignees(old_assignees)
   end
 
-  def change_milestone(noteable, project, author, milestone)
-    ::SystemNotes::IssuablesService.new(noteable: noteable, project: project, author: author).change_milestone(milestone)
-  end
-
   def relate_issue(noteable, noteable_ref, user)
     ::SystemNotes::IssuablesService.new(noteable: noteable, project: noteable.project, author: user).relate_issue(noteable_ref)
   end

app/services/system_notes/issuables_service.rb

@@ -77,24 +77,6 @@ module SystemNotes
       create_note(NoteSummary.new(noteable, project, author, body, action: 'assignee'))
     end
 
-    # Called when the milestone of a Noteable is changed
-    #
-    # milestone - Milestone being assigned, or nil
-    #
-    # Example Note text:
-    #
-    #   "removed milestone"
-    #
-    #   "changed milestone to 7.11"
-    #
-    # Returns the created Note object
-    def change_milestone(milestone)
-      format = milestone&.group_milestone? ? :name : :iid
-      body = milestone.nil? ? 'removed milestone' : "changed milestone to #{milestone.to_reference(project, format: format)}"
-
-      create_note(NoteSummary.new(noteable, project, author, body, action: 'milestone'))
-    end
-
     # Called when the title of a Noteable is changed
     #
     # old_title - Previous String title

spec/controllers/projects/milestones_controller_spec.rb

@@ -135,10 +135,6 @@ RSpec.describe Projects::MilestonesController do
   end
 
   describe "#destroy" do
-    before do
-      stub_feature_flags(track_resource_milestone_change_events: false)
-    end
-
     it "removes milestone" do
       expect(issue.milestone_id).to eq(milestone.id)
 
@@ -153,10 +149,6 @@ RSpec.describe Projects::MilestonesController do
 
       merge_request.reload
       expect(merge_request.milestone_id).to eq(nil)
-
-      # Check system note left for milestone removal
-      last_note = project.issues.find(issue.id).notes[-1].note
-      expect(last_note).to eq('removed milestone')
     end
   end
 

spec/services/issuable/common_system_notes_service_spec.rb

@@ -32,17 +32,6 @@ RSpec.describe Issuable::CommonSystemNotesService do
       end
     end
 
-    context 'when new milestone is assigned' do
-      before do
-        milestone = create(:milestone, project: project)
-        issuable.milestone_id = milestone.id
-
-        stub_feature_flags(track_resource_milestone_change_events: false)
-      end
-
-      it_behaves_like 'system note creation', {}, 'changed milestone'
-    end
-
     context 'with merge requests Draft note' do
       context 'adding Draft note' do
         let(:issuable) { create(:merge_request, title: "merge request") }
@@ -100,32 +89,10 @@ RSpec.describe Issuable::CommonSystemNotesService do
       expect(event.user_id).to eq user.id
     end
 
-    context 'when milestone change event tracking is disabled' do
-      before do
-        stub_feature_flags(track_resource_milestone_change_events: false)
-
-        issuable.milestone = create(:milestone, project: project)
-        issuable.save
-      end
-
-      it 'creates a system note for milestone set' do
-        expect { subject }.to change { issuable.notes.count }.from(0).to(1)
-        expect(issuable.notes.last.note).to match('changed milestone')
-      end
-
-      it 'does not create a milestone change event' do
-        expect { subject }.not_to change { ResourceMilestoneEvent.count }
-      end
-    end
-
-    context 'when milestone change event tracking is enabled' do
+    context 'when changing milestones' do
       let_it_be(:milestone) { create(:milestone, project: project) }
       let_it_be(:issuable) { create(:issue, project: project, milestone: milestone) }
 
-      before do
-        stub_feature_flags(track_resource_milestone_change_events: true)
-      end
-
       it 'does not create a system note for milestone set' do
         expect { subject }.not_to change { issuable.notes.count }
       end

spec/services/issues/update_service_spec.rb

@@ -445,10 +445,6 @@ RSpec.describe Issues::UpdateService, :mailer do
       end
 
       context 'when the milestone is removed' do
-        before do
-          stub_feature_flags(track_resource_milestone_change_events: false)
-        end
-
         let!(:non_subscriber) { create(:user) }
 
         let!(:subscriber) do
@@ -458,8 +454,6 @@ RSpec.describe Issues::UpdateService, :mailer do
           end
         end
 
-        it_behaves_like 'system notes for milestones'
-
         it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do
           issue.milestone = create(:milestone, project: project)
 
@@ -490,10 +484,6 @@ RSpec.describe Issues::UpdateService, :mailer do
       end
 
       context 'when the milestone is assigned' do
-        before do
-          stub_feature_flags(track_resource_milestone_change_events: false)
-        end
-
         let!(:non_subscriber) { create(:user) }
 
         let!(:subscriber) do
@@ -509,8 +499,6 @@ RSpec.describe Issues::UpdateService, :mailer do
           expect(todo.reload.done?).to eq true
         end
 
-        it_behaves_like 'system notes for milestones'
-
         it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do
           perform_enqueued_jobs do
             update_issue(milestone: create(:milestone, project: project))

spec/services/merge_requests/update_service_spec.rb

@@ -380,10 +380,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
       end
 
       context 'when the milestone is removed' do
-        before do
-          stub_feature_flags(track_resource_milestone_change_events: false)
-        end
-
         let!(:non_subscriber) { create(:user) }
 
         let!(:subscriber) do
@@ -393,8 +389,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
           end
         end
 
-        it_behaves_like 'system notes for milestones'
-
         it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do
           merge_request.milestone = create(:milestone, project: project)
 
@@ -410,10 +404,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
       end
 
       context 'when the milestone is changed' do
-        before do
-          stub_feature_flags(track_resource_milestone_change_events: false)
-        end
-
         let!(:non_subscriber) { create(:user) }
 
         let!(:subscriber) do
@@ -429,8 +419,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
           expect(pending_todo.reload).to be_done
         end
 
-        it_behaves_like 'system notes for milestones'
-
         it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do
           perform_enqueued_jobs do
             update_merge_request(milestone: create(:milestone, project: project))

spec/services/resource_events/synthetic_milestone_notes_builder_service_spec.rb

@@ -6,20 +6,23 @@ RSpec.describe ResourceEvents::SyntheticMilestoneNotesBuilderService do
   describe '#execute' do
     let_it_be(:user) { create(:user) }
     let_it_be(:issue) { create(:issue, author: user) }
+    let_it_be(:milestone) { create(:milestone, project: issue.project) }
 
-    before do
-      create_list(:resource_milestone_event, 3, issue: issue)
-
-      stub_feature_flags(track_resource_milestone_change_events: false)
+    let_it_be(:events) do
+      [
+        create(:resource_milestone_event, issue: issue, milestone: milestone, action: :add, created_at: '2020-01-01 04:00'),
+        create(:resource_milestone_event, issue: issue, milestone: milestone, action: :remove, created_at: '2020-01-02 08:00')
+      ]
     end
 
-    context 'when resource milestone events are disabled' do
-      # https://gitlab.com/gitlab-org/gitlab/-/issues/212985
-      it 'still builds notes for existing resource milestone events' do
-        notes = described_class.new(issue, user).execute
+    it 'builds milestone notes for resource milestone events' do
+      notes = described_class.new(issue, user).execute
 
-        expect(notes.size).to eq(3)
-      end
+      expect(notes.map(&:created_at)).to eq(events.map(&:created_at))
+      expect(notes.map(&:note)).to eq([
+        "changed milestone to %#{milestone.iid}",
+        'removed milestone'
+      ])
     end
   end
 end

spec/services/system_note_service_spec.rb

@@ -74,18 +74,6 @@ RSpec.describe SystemNoteService do
     end
   end
 
-  describe '.change_milestone' do
-    let(:milestone) { double }
-
-    it 'calls IssuableService' do
-      expect_next_instance_of(::SystemNotes::IssuablesService) do |service|
-        expect(service).to receive(:change_milestone).with(milestone)
-      end
-
-      described_class.change_milestone(noteable, project, author, milestone)
-    end
-  end
-
   describe '.relate_issue' do
     let(:noteable_ref) { double }
     let(:noteable) { double }

spec/services/system_notes/issuables_service_spec.rb

@@ -128,64 +128,6 @@ RSpec.describe ::SystemNotes::IssuablesService do
     end
   end
 
-  describe '#change_milestone' do
-    subject { service.change_milestone(milestone) }
-
-    context 'for a project milestone' do
-      let(:milestone) { create(:milestone, project: project) }
-
-      it_behaves_like 'a system note' do
-        let(:action) { 'milestone' }
-      end
-
-      context 'when milestone added' do
-        it 'sets the note text' do
-          reference = milestone.to_reference(format: :iid)
-
-          expect(subject.note).to eq "changed milestone to #{reference}"
-        end
-
-        it_behaves_like 'a note with overridable created_at'
-      end
-
-      context 'when milestone removed' do
-        let(:milestone) { nil }
-
-        it 'sets the note text' do
-          expect(subject.note).to eq 'removed milestone'
-        end
-
-        it_behaves_like 'a note with overridable created_at'
-      end
-    end
-
-    context 'for a group milestone' do
-      let(:milestone) { create(:milestone, group: group) }
-
-      it_behaves_like 'a system note' do
-        let(:action) { 'milestone' }
-      end
-
-      context 'when milestone added' do
-        it 'sets the note text to use the milestone name' do
-          expect(subject.note).to eq "changed milestone to #{milestone.to_reference(format: :name)}"
-        end
-
-        it_behaves_like 'a note with overridable created_at'
-      end
-
-      context 'when milestone removed' do
-        let(:milestone) { nil }
-
-        it 'sets the note text' do
-          expect(subject.note).to eq 'removed milestone'
-        end
-
-        it_behaves_like 'a note with overridable created_at'
-      end
-    end
-  end
-
   describe '#change_status' do
     subject { service.change_status(status, source) }
 

spec/support/shared_examples/services/issuable_shared_examples.rb

@@ -8,37 +8,6 @@ RSpec.shared_examples 'cache counters invalidator' do
   end
 end
 
-RSpec.shared_examples 'system notes for milestones' do
-  def update_issuable(opts)
-    issuable = try(:issue) || try(:merge_request)
-    described_class.new(project, user, opts).execute(issuable)
-  end
-
-  context 'group milestones' do
-    let(:group) { create(:group) }
-    let(:group_milestone) { create(:milestone, group: group) }
-
-    before do
-      project.update!(namespace: group)
-      create(:group_member, group: group, user: user)
-    end
-
-    it 'creates a system note' do
-      expect do
-        update_issuable(milestone: group_milestone)
-      end.to change { Note.system.count }.by(1)
-    end
-  end
-
-  context 'project milestones' do
-    it 'creates a system note' do
-      expect do
-        update_issuable(milestone: create(:milestone, project: project))
-      end.to change { Note.system.count }.by(1)
-    end
-  end
-end
-
 RSpec.shared_examples 'updating a single task' do
   def update_issuable(opts)
     issuable = try(:issue) || try(:merge_request)