Merge branch 'fix-system-notes-when-moving-issues' into 'master'

Prevent duplicate system notes when moving issues

See merge request gitlab-org/gitlab!41087

app/services/issuable_base_service.rb

@@ -136,7 +136,7 @@ class IssuableBaseService < BaseService
     {}
   end
 
-  def create(issuable)
+  def create(issuable, skip_system_notes: false)
     handle_quick_actions(issuable)
     filter_params(issuable)
 
@@ -153,7 +153,7 @@ class IssuableBaseService < BaseService
     end
 
     if issuable_saved
-      Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, is_update: false)
+      create_system_notes(issuable, is_update: false) unless skip_system_notes
 
       after_create(issuable)
       execute_hooks(issuable)
@@ -220,8 +220,9 @@ class IssuableBaseService < BaseService
       end
 
       if issuable_saved
-        Issuable::CommonSystemNotesService.new(project, current_user).execute(
-          issuable, old_labels: old_associations[:labels], old_milestone: old_associations[:milestone])
+        create_system_notes(
+          issuable, old_labels: old_associations[:labels], old_milestone: old_associations[:milestone]
+        )
 
         handle_changes(issuable, old_associations: old_associations)
 
@@ -255,7 +256,7 @@ class IssuableBaseService < BaseService
       before_update(issuable, skip_spam_check: true)
 
       if issuable.with_transaction_returning_status { issuable.save }
-        Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, old_labels: nil)
+        create_system_notes(issuable, old_labels: nil)
 
         handle_task_changes(issuable)
         invalidate_cache_counts(issuable, users: issuable.assignees.to_a)
@@ -339,6 +340,10 @@ class IssuableBaseService < BaseService
     AwardEmojis::ToggleService.new(issuable, award, current_user).execute if award
   end
 
+  def create_system_notes(issuable, **options)
+    Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, **options)
+  end
+
   def associations_before_update(issuable)
     associations =
       {

app/services/issues/create_service.rb

@@ -5,13 +5,13 @@ module Issues
     include SpamCheckMethods
     include ResolveDiscussions
 
-    def execute
+    def execute(skip_system_notes: false)
       @issue = BuildService.new(project, current_user, params).execute
 
       filter_spam_check_params
       filter_resolve_discussion_params
 
-      create(@issue)
+      create(@issue, skip_system_notes: skip_system_notes)
     end
 
     def before_create(issue)

app/services/issues/move_service.rb

@@ -52,7 +52,10 @@ module Issues
                    }
 
       new_params = original_entity.serializable_hash.symbolize_keys.merge(new_params)
-      CreateService.new(@target_project, @current_user, new_params).execute
+
+      # Skip creation of system notes for existing attributes of the issue. The system notes of the old
+      # issue are copied over so we don't want to end up with duplicate notes.
+      CreateService.new(@target_project, @current_user, new_params).execute(skip_system_notes: true)
     end
 
     def mark_as_moved

changelogs/unreleased/fix-system-notes-when-moving-issues.yml

+---
+title: Prevent duplicate system notes and events when an issue is moved
+merge_request: 41087
+author:
+type: fixed

ee/app/services/ee/issuable/common_system_notes_service.rb

@@ -11,7 +11,7 @@ module EE
         super
 
         ActiveRecord::Base.no_touching do
-          handle_weight_change(is_update)
+          handle_weight_change
           handle_iteration_change
 
           if is_update
@@ -43,13 +43,11 @@ module EE
         end
       end
 
-      def handle_weight_change(is_update)
+      def handle_weight_change
         return unless issuable.previous_changes.include?('weight')
 
         if weight_changes_tracking_enabled?
-          # Only create a resource event here if is_update is true to exclude the move issue operation.
-          # ResourceEvents for moved issues are written within AttributesRewriter.
-          EE::ResourceEvents::ChangeWeightService.new([issuable], current_user, Time.current).execute if is_update
+          EE::ResourceEvents::ChangeWeightService.new([issuable], current_user, Time.current).execute
         else
           ::SystemNoteService.change_weight_note(issuable, issuable.project, current_user)
         end

ee/changelogs/unreleased/fix-system-notes-when-moving-issues.yml

+---
+title: Create resource weight event when setting the weight during issue creation
+merge_request: 41087
+author:
+type: fixed

ee/spec/services/ee/issuable/common_system_notes_service_spec.rb

@@ -115,17 +115,13 @@ RSpec.describe Issuable::CommonSystemNotesService do
       issuable.update(weight: 5, health_status: 'at_risk')
     end
 
-    it 'does not create common system notes' do
-      expect { subject }.not_to change { issuable.notes.count }
-    end
-
     context 'when resource weight event tracking is enabled' do
       before do
         stub_feature_flags(track_issue_weight_change_events: true)
       end
 
-      it 'does not create a resource weight event' do
-        expect { subject }.not_to change { ResourceWeightEvent.count }
+      it 'creates a resource weight event' do
+        expect { subject }.to change { ResourceWeightEvent.count }
       end
 
       it 'does not create a system note' do

ee/spec/services/ee/issues/move_service_spec.rb

@@ -32,6 +32,7 @@ RSpec.describe Issues::MoveService do
     end
 
     context 'resource weight events' do
+      let(:old_issue) { create(:issue, project: old_project, author: user, weight: 5) }
       let!(:event1) { create(:resource_weight_event, issue: old_issue, weight: 1) }
       let!(:event2) { create(:resource_weight_event, issue: old_issue, weight: 42) }
       let!(:event3) { create(:resource_weight_event, issue: old_issue, weight: 5) }

spec/services/issues/create_service_spec.rb

@@ -29,6 +29,8 @@ RSpec.describe Issues::CreateService do
       end
 
       it 'creates the issue with the given params' do
+        expect(Issuable::CommonSystemNotesService).to receive_message_chain(:new, :execute)
+
         expect(issue).to be_persisted
         expect(issue.title).to eq('Awesome issue')
         expect(issue.assignees).to eq [assignee]
@@ -37,6 +39,16 @@ RSpec.describe Issues::CreateService do
         expect(issue.due_date).to eq Date.tomorrow
       end
 
+      context 'when skip_system_notes is true' do
+        let(:issue) { described_class.new(project, user, opts).execute(skip_system_notes: true) }
+
+        it 'does not call Issuable::CommonSystemNotesService' do
+          expect(Issuable::CommonSystemNotesService).not_to receive(:new)
+
+          issue
+        end
+      end
+
       it 'refreshes the number of open issues', :use_clean_rails_memory_store_caching do
         expect { issue }.to change { project.open_issues_count }.from(0).to(1)
       end

spec/services/issues/move_service_spec.rb

@@ -101,6 +101,41 @@ RSpec.describe Issues::MoveService do
         end
       end
 
+      context 'issue with milestone' do
+        let(:milestone) { create(:milestone, group: sub_group_1) }
+        let(:new_project) { create(:project, namespace: sub_group_1) }
+
+        let(:old_issue) do
+          create(:issue, title: title, description: description, project: old_project, author: author, milestone: milestone)
+        end
+
+        before do
+          create(:resource_milestone_event, issue: old_issue, milestone: milestone, action: :add)
+        end
+
+        it 'does not create extra milestone events' do
+          new_issue = move_service.execute(old_issue, new_project)
+
+          expect(new_issue.resource_milestone_events.count).to eq(old_issue.resource_milestone_events.count)
+        end
+      end
+
+      context 'issue with due date' do
+        let(:old_issue) do
+          create(:issue, title: title, description: description, project: old_project, author: author, due_date: '2020-01-10')
+        end
+
+        before do
+          SystemNoteService.change_due_date(old_issue, old_project, author, old_issue.due_date)
+        end
+
+        it 'does not create extra system notes' do
+          new_issue = move_service.execute(old_issue, new_project)
+
+          expect(new_issue.notes.count).to eq(old_issue.notes.count)
+        end
+      end
+
       context 'issue with assignee' do
         let_it_be(:assignee) { create(:user) }