Merge branch 'issue_14012' into 'master'

Fix problem when creating milestones in groups without projects

Fixes #14012 

See merge request !3481

5 files changed, 42 insertions, 6 deletions

diff --git a/CHANGELOG b/CHANGELOG
index c6febbd93d3..12df0492020 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -20,6 +20,7 @@ v 8.7.0 (unreleased)
   - Ensure empty recipients are rejected in BuildsEmailService
   - API: Ability to filter milestones by state `active` and `closed` (Robert Schilling)
   - Implement 'Groups View' as an option for dashboard preferences !3379 (Elias W.)
+  - Better errors handling when creating milestones inside groups
   - Implement 'TODOs View' as an option for dashboard preferences !3379 (Elias W.)
   - Gracefully handle notes on deleted commits in merge requests (Stan Hu)
   - Fix creation of merge requests for orphaned branches (Stan Hu)
diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb
index b23c3022fb5..9d5a28e8d4d 100644
--- a/app/controllers/groups/milestones_controller.rb
+++ b/app/controllers/groups/milestones_controller.rb
@@ -18,14 +18,14 @@ class Groups::MilestonesController < Groups::ApplicationController
   end
 
   def create
-    project_ids = params[:milestone][:project_ids]
+    project_ids = params[:milestone][:project_ids].reject(&:blank?)
     title = milestone_params[:title]
 
-    @projects.where(id: project_ids).each do |project|
-      Milestones::CreateService.new(project, current_user, milestone_params).execute
+    if create_milestones(project_ids)
+      redirect_to milestone_path(title)
+    else
+      render_new_with_error(project_ids.empty?)
     end
-
-    redirect_to milestone_path(title)
   end
 
   def show
@@ -41,6 +41,27 @@ class Groups::MilestonesController < Groups::ApplicationController
 
   private
 
+  def create_milestones(project_ids)
+    return false unless project_ids.present?
+
+    ActiveRecord::Base.transaction do
+      @projects.where(id: project_ids).each do |project|
+        Milestones::CreateService.new(project, current_user, milestone_params).execute
+      end
+    end
+
+    true
+  rescue ActiveRecord::ActiveRecordError => e
+    flash.now[:alert] = "An error occurred while creating the milestone: #{e.message}"
+    false
+  end
+
+  def render_new_with_error(empty_project_ids)
+    @milestone = Milestone.new(milestone_params)
+    @milestone.errors.add(:project_id, "Please select at least one project.") if empty_project_ids
+    render :new
+  end
+
   def authorize_admin_milestones!
     return render_404 unless can?(current_user, :admin_milestones, group)
   end
diff --git a/app/services/milestones/create_service.rb b/app/services/milestones/create_service.rb
index b8e08c9f1eb..3b90399af64 100644
--- a/app/services/milestones/create_service.rb
+++ b/app/services/milestones/create_service.rb
@@ -3,7 +3,7 @@ module Milestones
     def execute
       milestone = project.milestones.new(params)
 
-      if milestone.save
+      if milestone.save!
         event_service.open_milestone(milestone, current_user)
       end
 
diff --git a/app/views/groups/milestones/new.html.haml b/app/views/groups/milestones/new.html.haml
index a8e1ed77da9..4290e0bf72e 100644
--- a/app/views/groups/milestones/new.html.haml
+++ b/app/views/groups/milestones/new.html.haml
@@ -10,6 +10,14 @@
 
 = form_for @milestone, url: group_milestones_path(@group), html: { class: 'form-horizontal milestone-form gfm-form js-quick-submit js-requires-input' }  do |f|
   .row
+    - if @milestone.errors.any?
+      #error_explanation
+        .alert.alert-danger
+          %ul
+            - @milestone.errors.full_messages.each do |msg|
+              %li
+                = msg
+
     .col-md-6
       .form-group
         = f.label :title, "Title", class: "control-label"
diff --git a/spec/controllers/groups/milestones_controller_spec.rb b/spec/controllers/groups/milestones_controller_spec.rb
index eb0c6ac6d80..b0793cb1655 100644
--- a/spec/controllers/groups/milestones_controller_spec.rb
+++ b/spec/controllers/groups/milestones_controller_spec.rb
@@ -23,5 +23,11 @@ describe Groups::MilestonesController do
       expect(response).to redirect_to(group_milestone_path(group, title.to_slug.to_s, title: title))
       expect(Milestone.where(title: title).count).to eq(2)
     end
+
+    it "redirects to new when there are no project ids" do
+      post :create, group_id: group.id, milestone: { title: title, project_ids: [""] }
+      expect(response).to render_template :new
+      expect(assigns(:milestone).errors).not_to be_nil
+    end
   end
 end