From 3f6e45ed28408429295fba30df8f53a6672f3bbe Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Mon, 20 Nov 2017 12:44:10 -0200 Subject: [PATCH] Fix promoting milestone updating all issuables without milestone --- app/services/milestones/promote_service.rb | 23 +++++++----- changelogs/unreleased/issue_40337.yml | 5 +++ .../milestones/promote_service_spec.rb | 36 +++++++++++++++++++ 3 files changed, 55 insertions(+), 9 deletions(-) create mode 100644 changelogs/unreleased/issue_40337.yml 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) -- GitLab