Merge branch 'issue_40337' into 'master'

Fix promoting milestone updating all issuables without milestone

Closes #40337

See merge request gitlab-org/gitlab-ce!15487

(cherry picked from commit 65545099554f97a30c66c2c8c36d24c589daddc3)

3f6e45ed Fix promoting milestone updating all issuables without milestone

3 files changed, 55 insertions, 9 deletions

diff --git a/app/services/milestones/promote_service.rb b/app/services/milestones/promote_service.rb
index bd9cfd4e0ea..2187f26d1ed 100644
--- a/app/services/milestones/promote_service.rb
+++ b/app/services/milestones/promote_service.rb
@@ -6,14 +6,14 @@ module Milestones
       check_project_milestone!(milestone)
 
       Milestone.transaction do
-        # Destroy all milestones with same title across projects
-        destroy_old_milestones(milestone)
-
         group_milestone = clone_project_milestone(milestone)
 
         move_children_to_group_milestone(group_milestone)
 
-        # Just to be safe
+        # Destroy all milestones with same title across projects
+        destroy_old_milestones(milestone)
+
+        # Rollback if milestone is not valid
         unless group_milestone.valid?
           raise_error(group_milestone.errors.full_messages.to_sentence)
         end
@@ -35,7 +35,7 @@ module Milestones
     end
 
     def move_children_to_group_milestone(group_milestone)
-      milestone_ids_for_merge(group_milestone).in_groups_of(100) do |milestone_ids|
+      milestone_ids_for_merge(group_milestone).in_groups_of(100, false) do |milestone_ids|
         update_children(group_milestone, milestone_ids)
       end
     end
@@ -49,7 +49,12 @@ module Milestones
 
       create_service = CreateService.new(group, current_user, params)
 
-      create_service.execute
+      milestone = create_service.execute
+
+      # milestone won't be valid here because of duplicated title
+      milestone.save(validate: false)
+
+      milestone
     end
 
     def update_children(group_milestone, milestone_ids)
@@ -65,12 +70,12 @@ module Milestones
       @group ||= parent.group || raise_error('Project does not belong to a group.')
     end
 
-    def destroy_old_milestones(group_milestone)
-      Milestone.where(id: milestone_ids_for_merge(group_milestone)).destroy_all
+    def destroy_old_milestones(milestone)
+      Milestone.where(id: milestone_ids_for_merge(milestone)).destroy_all
     end
 
     def group_project_ids
-      @group_project_ids ||= group.projects.map(&:id)
+      @group_project_ids ||= group.projects.pluck(:id)
     end
 
     def raise_error(message)
diff --git a/changelogs/unreleased/issue_40337.yml b/changelogs/unreleased/issue_40337.yml
new file mode 100644
index 00000000000..0cd6c5f46a9
--- /dev/null
+++ b/changelogs/unreleased/issue_40337.yml
@@ -0,0 +1,5 @@
+---
+title: Fix promoting milestone updating all issuables without milestone
+merge_request:
+author:
+type: fixed
diff --git a/spec/services/milestones/promote_service_spec.rb b/spec/services/milestones/promote_service_spec.rb
index 9f2df6d6d19..a0a2843b676 100644
--- a/spec/services/milestones/promote_service_spec.rb
+++ b/spec/services/milestones/promote_service_spec.rb
@@ -25,6 +25,18 @@ describe Milestones::PromoteService do
 
         expect { service.execute(milestone) }.to raise_error(described_class::PromoteMilestoneError)
       end
+
+      it 'does not promote milestone and update issuables if promoted milestone is not valid' do
+        issue = create(:issue, milestone: milestone, project: project)
+        merge_request = create(:merge_request, milestone: milestone, source_project: project)
+        allow_any_instance_of(Milestone).to receive(:valid?).and_return(false)
+
+        expect { service.execute(milestone) }.to raise_error(described_class::PromoteMilestoneError)
+
+        expect(milestone.reload).to be_persisted
+        expect(issue.reload.milestone).to eq(milestone)
+        expect(merge_request.reload.milestone).to eq(milestone)
+      end
     end
 
     context 'without duplicated milestone titles across projects' do
@@ -34,6 +46,16 @@ describe Milestones::PromoteService do
         expect(promoted_milestone).to be_group_milestone
       end
 
+      it 'does not update issuables without milestone with the new promoted milestone' do
+        issue_without_milestone = create(:issue, project: project, milestone: nil)
+        merge_request_without_milestone = create(:merge_request, milestone: nil, source_project: project)
+
+        service.execute(milestone)
+
+        expect(issue_without_milestone.reload.milestone).to be_nil
+        expect(merge_request_without_milestone.reload.milestone).to be_nil
+      end
+
       it 'sets issuables with new promoted milestone' do
         issue = create(:issue, milestone: milestone, project: project)
         merge_request = create(:merge_request, milestone: milestone, source_project: project)
@@ -59,6 +81,20 @@ describe Milestones::PromoteService do
         expect(Milestone.exists?(milestone_2.id)).to be_falsy
       end
 
+      it 'does not update issuables without milestone with the new promoted milestone' do
+        issue_without_milestone_1 = create(:issue, project: project, milestone: nil)
+        issue_without_milestone_2 = create(:issue, project: project_2, milestone: nil)
+        merge_request_without_milestone_1 = create(:merge_request, milestone: nil, source_project: project)
+        merge_request_without_milestone_2 = create(:merge_request, milestone: nil, source_project: project_2)
+
+        service.execute(milestone)
+
+        expect(issue_without_milestone_1.reload.milestone).to be_nil
+        expect(issue_without_milestone_2.reload.milestone).to be_nil
+        expect(merge_request_without_milestone_1.reload.milestone).to be_nil
+        expect(merge_request_without_milestone_2.reload.milestone).to be_nil
+      end
+
       it 'sets all issuables with new promoted milestone' do
         issue = create(:issue, milestone: milestone, project: project)
         issue_2 = create(:issue, milestone: milestone_2, project: project_2)