Commit 3f6e45ed authored by Felipe Artur's avatar Felipe Artur

Fix promoting milestone updating all issuables without milestone

parent 16299d57
...@@ -6,14 +6,14 @@ module Milestones ...@@ -6,14 +6,14 @@ module Milestones
check_project_milestone!(milestone) check_project_milestone!(milestone)
Milestone.transaction do Milestone.transaction do
# Destroy all milestones with same title across projects
destroy_old_milestones(milestone)
group_milestone = clone_project_milestone(milestone) group_milestone = clone_project_milestone(milestone)
move_children_to_group_milestone(group_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? unless group_milestone.valid?
raise_error(group_milestone.errors.full_messages.to_sentence) raise_error(group_milestone.errors.full_messages.to_sentence)
end end
...@@ -35,7 +35,7 @@ module Milestones ...@@ -35,7 +35,7 @@ module Milestones
end end
def move_children_to_group_milestone(group_milestone) 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) update_children(group_milestone, milestone_ids)
end end
end end
...@@ -49,7 +49,12 @@ module Milestones ...@@ -49,7 +49,12 @@ module Milestones
create_service = CreateService.new(group, current_user, params) 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 end
def update_children(group_milestone, milestone_ids) def update_children(group_milestone, milestone_ids)
...@@ -65,12 +70,12 @@ module Milestones ...@@ -65,12 +70,12 @@ module Milestones
@group ||= parent.group || raise_error('Project does not belong to a group.') @group ||= parent.group || raise_error('Project does not belong to a group.')
end end
def destroy_old_milestones(group_milestone) def destroy_old_milestones(milestone)
Milestone.where(id: milestone_ids_for_merge(group_milestone)).destroy_all Milestone.where(id: milestone_ids_for_merge(milestone)).destroy_all
end end
def group_project_ids def group_project_ids
@group_project_ids ||= group.projects.map(&:id) @group_project_ids ||= group.projects.pluck(:id)
end end
def raise_error(message) def raise_error(message)
......
---
title: Fix promoting milestone updating all issuables without milestone
merge_request:
author:
type: fixed
...@@ -25,6 +25,18 @@ describe Milestones::PromoteService do ...@@ -25,6 +25,18 @@ describe Milestones::PromoteService do
expect { service.execute(milestone) }.to raise_error(described_class::PromoteMilestoneError) expect { service.execute(milestone) }.to raise_error(described_class::PromoteMilestoneError)
end 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 end
context 'without duplicated milestone titles across projects' do context 'without duplicated milestone titles across projects' do
...@@ -34,6 +46,16 @@ describe Milestones::PromoteService do ...@@ -34,6 +46,16 @@ describe Milestones::PromoteService do
expect(promoted_milestone).to be_group_milestone expect(promoted_milestone).to be_group_milestone
end 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 it 'sets issuables with new promoted milestone' do
issue = create(:issue, milestone: milestone, project: project) issue = create(:issue, milestone: milestone, project: project)
merge_request = create(:merge_request, milestone: milestone, source_project: project) merge_request = create(:merge_request, milestone: milestone, source_project: project)
...@@ -59,6 +81,20 @@ describe Milestones::PromoteService do ...@@ -59,6 +81,20 @@ describe Milestones::PromoteService do
expect(Milestone.exists?(milestone_2.id)).to be_falsy expect(Milestone.exists?(milestone_2.id)).to be_falsy
end 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 it 'sets all issuables with new promoted milestone' do
issue = create(:issue, milestone: milestone, project: project) issue = create(:issue, milestone: milestone, project: project)
issue_2 = create(:issue, milestone: milestone_2, project: project_2) issue_2 = create(:issue, milestone: milestone_2, project: project_2)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment