Merge branch '424736-milestone-titles-unique-in-heirarchy' into 'master'

Milestone titles should be unique in group hierarchy

See merge request !142857



Merged-by: Doug Stull <dstull@gitlab.com>
Approved-by: Ian Anderson <ianderson@gitlab.com>
Approved-by: Nick Leonard <nleonard@gitlab.com>
Approved-by: Doug Stull <dstull@gitlab.com>
Reviewed-by: Brett Walker <bwalker@gitlab.com>
Reviewed-by: Doug Stull <dstull@gitlab.com>
Reviewed-by: Ian Anderson <ianderson@gitlab.com>
Reviewed-by: Nick Leonard <nleonard@gitlab.com>
Co-authored-by: Brett Walker <bwalker@gitlab.com>

app/controllers/groups/milestones_controller.rb

@@ -46,7 +46,11 @@ def update
 
     respond_to do |format|
       format.html do
-        redirect_to milestone_path(@milestone)
+        if @milestone.valid?
+          redirect_to milestone_path(@milestone)
+        else
+          render :edit
+        end
       end
 
       format.json do

app/models/milestone.rb

@@ -300,9 +300,9 @@ def issues_finder_params
   # milestone titles must be unique across project and group milestones
   def uniqueness_of_title
     if project
-      relation = self.class.for_projects_and_groups([project_id], [project.group&.id])
+      relation = self.class.for_projects_and_groups([project_id], [project.group&.self_and_ancestors_ids])
     elsif group
-      relation = self.class.for_projects_and_groups(group.projects.select(:id), [group.id])
+      relation = self.class.for_projects_and_groups(group.all_project_ids, [group.self_and_hierarchy])
     end
 
     title_exists = relation.find_by_title(title)

spec/controllers/groups/milestones_controller_spec.rb

@@ -247,30 +247,42 @@
   describe "#update" do
     let(:milestone) { create(:milestone, group: group) }
 
-    it "updates group milestone" do
-      milestone_params[:title] = "title changed"
-
+    subject do
       put :update, params: {
         id: milestone.iid,
-        group_id: group.to_param,
-        milestone: milestone_params
+        milestone: milestone_params,
+        group_id: group.to_param
       }
+    end
 
+    it "updates group milestone" do
+      milestone_params[:title] = "title changed"
+
+      subject
       milestone.reload
+
       expect(response).to redirect_to(group_milestone_path(group, milestone.iid))
       expect(milestone.title).to eq("title changed")
     end
 
+    it "handles validation error" do
+      subgroup = create(:group, parent: group)
+      subgroup_milestone = create(:milestone, group: subgroup)
+
+      milestone_params[:title] = subgroup_milestone.title
+
+      subject
+
+      expect(response).not_to redirect_to(group_milestone_path(group, milestone.iid))
+      expect(response).to render_template(:edit)
+    end
+
     it "handles ActiveRecord::StaleObjectError" do
       milestone_params[:title] = "title changed"
       # Purposely reduce the lock_version to trigger an ActiveRecord::StaleObjectError
       milestone_params[:lock_version] = milestone.lock_version - 1
 
-      put :update, params: {
-        id: milestone.iid,
-        group_id: group.to_param,
-        milestone: milestone_params
-      }
+      subject
 
       expect(response).not_to redirect_to(group_milestone_path(group, milestone.iid))
       expect(response).to render_template(:edit)

spec/controllers/projects/milestones_controller_spec.rb

@@ -186,6 +186,19 @@ def render_index(project:, page:, search_title: '')
       expect(milestone.title).to eq milestone_params[:title]
     end
 
+    it "handles validation error" do
+      group = create(:group)
+      group_milestone = create(:milestone, group: group)
+      project.update!(namespace: group)
+
+      milestone_params[:title] = group_milestone.title
+
+      subject
+
+      expect(response).not_to redirect_to(project_milestone_path(project, milestone.iid))
+      expect(response).to render_template(:edit)
+    end
+
     it "handles ActiveRecord::StaleObjectError" do
       # Purposely reduce the lock_version to trigger an ActiveRecord::StaleObjectError
       milestone_params[:lock_version] = milestone.lock_version - 1

spec/lib/banzai/filter/references/milestone_reference_filter_spec.rb

@@ -504,7 +504,7 @@
     context 'when referencing both group and subgroup milestones using absolute references' do
       let(:subgroup) { create(:group, :public, parent: group) }
       let(:group_milestone) { create(:milestone, title: 'group_milestone', group: group) }
-      let(:subgroup_milestone) { create(:milestone, title: 'group_milestone', group: subgroup) }
+      let(:subgroup_milestone) { create(:milestone, title: 'subgroup_milestone', group: subgroup) }
       let(:context) { { project: project, group: nil } }
 
       it 'links to valid references' do

spec/models/milestone_spec.rb

@@ -33,43 +33,54 @@
   it_behaves_like 'a timebox', :milestone do
     let(:project) { create(:project, :public) }
     let(:timebox) { create(:milestone, project: project) }
+  end
 
-    describe "#uniqueness_of_title" do
-      context "per project" do
-        it "does not accept the same title in a project twice" do
-          new_timebox = timebox.dup
-          expect(new_timebox).not_to be_valid
-        end
+  describe "#uniqueness_of_title" do
+    let_it_be(:root_group) { create(:group) }
+    let_it_be(:sub_group) { create(:group, parent: root_group) }
+    let_it_be(:sub_sub_group) { create(:group, parent: sub_group) }
+    let_it_be(:sub_sub_project) { create(:project, group: sub_sub_group) }
+    let_it_be(:sub_sub_group_milestone) { create(:milestone, group: sub_sub_group) }
+    let_it_be(:sub_sub_project_milestone) { create(:milestone, project: sub_sub_project) }
 
-        it "accepts the same title in another project" do
-          project = create(:project)
-          new_timebox = timebox.dup
-          new_timebox.project = project
+    context "per project" do
+      it "does not accept the same title in a project twice" do
+        milestone = described_class.new(project: sub_sub_project, title: sub_sub_project_milestone.title)
 
-          expect(new_timebox).to be_valid
-        end
+        expect(milestone).not_to be_valid
       end
 
-      context "per group" do
-        let(:timebox) { create(:milestone, group: group) }
+      it "accepts the same title in another project" do
+        project = create(:project, group: sub_sub_group)
+        milestone = described_class.new(project: project, title: sub_sub_project_milestone.title)
 
-        before do
-          project.update!(group: group)
-        end
+        expect(milestone).to be_valid
+      end
+    end
 
-        it "does not accept the same title in a group twice" do
-          new_timebox = described_class.new(group: group, title: timebox.title)
+    context "per group" do
+      it "does not accept the same title in a group twice" do
+        milestone = described_class.new(group: sub_sub_group, title: sub_sub_group_milestone.title)
 
-          expect(new_timebox).not_to be_valid
-        end
+        expect(milestone).not_to be_valid
+      end
 
-        it "does not accept the same title of a child project timebox" do
-          create(:milestone, *timebox_args, project: group.projects.first)
+      it "does not accept the same title of a child project timebox" do
+        milestone = described_class.new(group: sub_sub_group, title: sub_sub_project_milestone.title)
 
-          new_timebox = described_class.new(group: group, title: timebox.title)
+        expect(milestone).not_to be_valid
+      end
 
-          expect(new_timebox).not_to be_valid
-        end
+      it "does not accept the same title in a descendant group" do
+        new_timebox = described_class.new(group: root_group, title: sub_sub_group_milestone.title)
+
+        expect(new_timebox).not_to be_valid
+      end
+
+      it "does not accept the same title in a descendant project" do
+        new_timebox = described_class.new(group: root_group, title: sub_sub_project_milestone.title)
+
+        expect(new_timebox).not_to be_valid
       end
     end
   end

spec/services/import_csv/preprocess_milestones_service_spec.rb

@@ -72,9 +72,18 @@
       context 'when duplicate milestones exist in the projects group and parent group' do
         let_it_be(:sub_group) { create(:group, parent: group) }
         let_it_be(:project) { create(:project, group: sub_group) }
-        let!(:ancestor_group_milestone) { create(:milestone, group: group, title: '15.10') }
+        let!(:ancestor_group_milestone) do
+          build(:milestone, group: group, title: '15.10').tap do |record|
+            record.save!(validate: false)
+          end
+        end
+
         let!(:ancestor_group_milestone_two) { create(:milestone, group: group, title: '10.1') }
-        let!(:group_milestone) { create(:milestone, group: sub_group, title: '10.1') }
+        let!(:group_milestone) do
+          build(:milestone, group: sub_group, title: '10.1').tap do |record|
+            record.save!(validate: false)
+          end
+        end
 
         it_behaves_like 'csv import', { is_success: true, milestone_errors: nil }
       end