Merge branch '22033-milestone-validation-returns-422' into 'master'

Ensure validation messages are shown within the milestone form

Fixes a bug where upon entering data to create an invalid milestone via the Web UI or the API would raise an exception rather than render the validation messages

We'd rather render the validation messages to inform the user than raise an exception and result in a 422 error being displayed

Closes #22033

See merge request !6345

4 files changed, 41 insertions, 6 deletions

diff --git a/CHANGELOG b/CHANGELOG
index e9445a18a18..2394864467e 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -139,6 +139,7 @@ v 8.12.0 (unreleased)
   - Use default clone protocol on "check out, review, and merge locally" help page URL
   - API for Ci Lint !5953 (Katarzyna Kobierska Urszula Budziszewska)
   - Allow bulk update merge requests from merge requests index page
+  - Ensure validation messages are shown within the milestone form
   - Add notification_settings API calls !5632 (mahcsig)
   - Remove duplication between project builds and admin builds view !5680 (Katarzyna Kobierska Ula Budziszewska)
   - Fix URLs with anchors in wiki !6300 (houqp)
diff --git a/app/services/milestones/create_service.rb b/app/services/milestones/create_service.rb
index 3b90399af64..b8e08c9f1eb 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/spec/features/milestone_spec.rb b/spec/features/milestone_spec.rb
index c43661e5681..b8c838bf7ab 100644
--- a/spec/features/milestone_spec.rb
+++ b/spec/features/milestone_spec.rb
@@ -3,9 +3,8 @@ require 'rails_helper'
 feature 'Milestone', feature: true do
   include WaitForAjax
 
-  let(:project) { create(:project, :public) }
+  let(:project) { create(:empty_project, :public) }
   let(:user)   { create(:user) }
-  let(:milestone) { create(:milestone, project: project, title: 8.7) }
 
   before do
     project.team << [user, :master]
@@ -13,7 +12,7 @@ feature 'Milestone', feature: true do
   end
 
   feature 'Create a milestone' do
-    scenario 'shows an informative message for a new issue' do
+    scenario 'shows an informative message for a new milestone' do
       visit new_namespace_project_milestone_path(project.namespace, project)
       page.within '.milestone-form' do
         fill_in "milestone_title", with: '8.7'
@@ -26,10 +25,26 @@ feature 'Milestone', feature: true do
 
   feature 'Open a milestone with closed issues' do
     scenario 'shows an informative message' do
+      milestone = create(:milestone, project: project, title: 8.7)
+
       create(:issue, title: "Bugfix1", project: project, milestone: milestone, state: "closed")
       visit namespace_project_milestone_path(project.namespace, project, milestone)
 
       expect(find('.alert-success')).to have_content('All issues for this milestone are closed. You may close this milestone now.')
     end
   end
+
+  feature 'Open a milestone with an existing title' do
+    scenario 'displays validation message' do
+      milestone = create(:milestone, project: project, title: 8.7)
+
+      visit new_namespace_project_milestone_path(project.namespace, project)
+      page.within '.milestone-form' do
+        fill_in "milestone_title", with: milestone.title
+      end
+      find('input[name="commit"]').click
+
+      expect(find('.alert-danger')).to have_content('Title has already been taken')
+    end
+  end
 end
diff --git a/spec/requests/api/milestones_spec.rb b/spec/requests/api/milestones_spec.rb
index d6a0c656e74..b89dac01040 100644
--- a/spec/requests/api/milestones_spec.rb
+++ b/spec/requests/api/milestones_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
 describe API::API, api: true  do
   include ApiHelpers
   let(:user) { create(:user) }
-  let!(:project) { create(:project, namespace: user.namespace ) }
+  let!(:project) { create(:empty_project, namespace: user.namespace ) }
   let!(:closed_milestone) { create(:closed_milestone, project: project) }
   let!(:milestone) { create(:milestone, project: project) }
 
@@ -12,6 +12,7 @@ describe API::API, api: true  do
   describe 'GET /projects/:id/milestones' do
     it 'returns project milestones' do
       get api("/projects/#{project.id}/milestones", user)
+
       expect(response).to have_http_status(200)
       expect(json_response).to be_an Array
       expect(json_response.first['title']).to eq(milestone.title)
@@ -19,6 +20,7 @@ describe API::API, api: true  do
 
     it 'returns a 401 error if user not authenticated' do
       get api("/projects/#{project.id}/milestones")
+
       expect(response).to have_http_status(401)
     end
 
@@ -44,6 +46,7 @@ describe API::API, api: true  do
   describe 'GET /projects/:id/milestones/:milestone_id' do
     it 'returns a project milestone by id' do
       get api("/projects/#{project.id}/milestones/#{milestone.id}", user)
+
       expect(response).to have_http_status(200)
       expect(json_response['title']).to eq(milestone.title)
       expect(json_response['iid']).to eq(milestone.iid)
@@ -60,11 +63,13 @@ describe API::API, api: true  do
 
     it 'returns 401 error if user not authenticated' do
       get api("/projects/#{project.id}/milestones/#{milestone.id}")
+
       expect(response).to have_http_status(401)
     end
 
     it 'returns a 404 error if milestone id not found' do
       get api("/projects/#{project.id}/milestones/1234", user)
+
       expect(response).to have_http_status(404)
     end
   end
@@ -72,6 +77,7 @@ describe API::API, api: true  do
   describe 'POST /projects/:id/milestones' do
     it 'creates a new project milestone' do
       post api("/projects/#{project.id}/milestones", user), title: 'new milestone'
+
       expect(response).to have_http_status(201)
       expect(json_response['title']).to eq('new milestone')
       expect(json_response['description']).to be_nil
@@ -80,6 +86,7 @@ describe API::API, api: true  do
     it 'creates a new project milestone with description and due date' do
       post api("/projects/#{project.id}/milestones", user),
         title: 'new milestone', description: 'release', due_date: '2013-03-02'
+
       expect(response).to have_http_status(201)
       expect(json_response['description']).to eq('release')
       expect(json_response['due_date']).to eq('2013-03-02')
@@ -87,6 +94,14 @@ describe API::API, api: true  do
 
     it 'returns a 400 error if title is missing' do
       post api("/projects/#{project.id}/milestones", user)
+
+      expect(response).to have_http_status(400)
+    end
+
+    it 'returns a 400 error if params are invalid (duplicate title)' do
+      post api("/projects/#{project.id}/milestones", user),
+        title: milestone.title, description: 'release', due_date: '2013-03-02'
+
       expect(response).to have_http_status(400)
     end
   end
@@ -95,6 +110,7 @@ describe API::API, api: true  do
     it 'updates a project milestone' do
       put api("/projects/#{project.id}/milestones/#{milestone.id}", user),
         title: 'updated title'
+
       expect(response).to have_http_status(200)
       expect(json_response['title']).to eq('updated title')
     end
@@ -102,6 +118,7 @@ describe API::API, api: true  do
     it 'returns a 404 error if milestone id not found' do
       put api("/projects/#{project.id}/milestones/1234", user),
         title: 'updated title'
+
       expect(response).to have_http_status(404)
     end
   end
@@ -131,6 +148,7 @@ describe API::API, api: true  do
     end
     it 'returns project issues for a particular milestone' do
       get api("/projects/#{project.id}/milestones/#{milestone.id}/issues", user)
+
       expect(response).to have_http_status(200)
       expect(json_response).to be_an Array
       expect(json_response.first['milestone']['title']).to eq(milestone.title)
@@ -138,11 +156,12 @@ describe API::API, api: true  do
 
     it 'returns a 401 error if user not authenticated' do
       get api("/projects/#{project.id}/milestones/#{milestone.id}/issues")
+
       expect(response).to have_http_status(401)
     end
 
     describe 'confidential issues' do
-      let(:public_project) { create(:project, :public) }
+      let(:public_project) { create(:empty_project, :public) }
       let(:milestone) { create(:milestone, project: public_project) }
       let(:issue) { create(:issue, project: public_project) }
       let(:confidential_issue) { create(:issue, confidential: true, project: public_project) }