From ef60b8e1685a8761477e822b3190a3a0cf4b0cfa Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Wed, 18 May 2016 13:02:10 -0500
Subject: [PATCH] Use pipelines.errors when communicating the error

---
 .../projects/pipelines_controller.rb          | 14 ++++----
 app/services/ci/create_pipeline_service.rb    | 36 +++++++++++--------
 app/views/projects/pipelines/new.html.haml    |  7 ++--
 spec/features/pipelines_spec.rb               | 15 +++++---
 4 files changed, 41 insertions(+), 31 deletions(-)

diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb
index 19071cc9e43..1282913d981 100644
--- a/app/controllers/projects/pipelines_controller.rb
+++ b/app/controllers/projects/pipelines_controller.rb
@@ -15,19 +15,17 @@ class Projects::PipelinesController < Projects::ApplicationController
   end
 
   def new
+    @pipeline = project.ci_commits.new
   end
 
   def create
-    begin
-      pipeline = Ci::CreatePipelineService.new(project, current_user, create_params).execute
-      redirect_to namespace_project_pipeline_path(project.namespace, project, pipeline)
-    rescue ArgumentError => e
-      flash[:alert] = e.message
-      render 'new'
-    rescue
-      flash[:alert] = 'The pipeline could not be created. Please try again.'
+    @pipeline = Ci::CreatePipelineService.new(project, current_user, create_params).execute
+    unless @pipeline.persisted?
       render 'new'
+      return
     end
+
+    redirect_to namespace_project_pipeline_path(project.namespace, project, @pipeline)
   end
 
   def show
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index e13f4fce13d..b864807ec35 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -1,27 +1,39 @@
 module Ci
   class CreatePipelineService < BaseService
     def execute
+      pipeline = project.ci_commits.new
+
       unless ref_names.include?(params[:ref])
-        raise ArgumentError, 'Reference not found'
+        pipeline.errors.add(:base, 'Reference not found')
+        return pipeline
       end
 
       unless commit
-        raise ArgumentError, 'Commit not found'
+        pipeline.errors.add(:base, 'Commit not found')
+        return pipeline
       end
 
       unless can?(current_user, :create_pipeline, project)
-        raise RuntimeError, 'Insufficient permissions to create a new pipeline'
+        pipeline.errors.add(:base, 'Insufficient permissions to create a new pipeline')
+        return pipeline
       end
 
-      pipeline = new_pipeline
+      begin
+        Ci::Commit.transaction do
+          pipeline.sha = commit.id
+          pipeline.ref = params[:ref]
+          pipeline.before_sha = Gitlab::Git::BLANK_SHA
 
-      Ci::Commit.transaction do
-        unless pipeline.config_processor
-          raise ArgumentError, pipeline.yaml_errors || 'Missing .gitlab-ci.yml file'
-        end
+          unless pipeline.config_processor
+            pipeline.errors.add(:base, pipeline.yaml_errors || 'Missing .gitlab-ci.yml file')
+            raise ActiveRecord::Rollback
+          end
 
-        pipeline.save!
-        pipeline.create_builds(current_user)
+          pipeline.save!
+          pipeline.create_builds(current_user)
+        end
+      rescue
+        pipeline.errors.add(:base, 'The pipeline could not be created. Please try again.')
       end
 
       pipeline
@@ -29,10 +41,6 @@ module Ci
 
     private
 
-    def new_pipeline
-      project.ci_commits.new(sha: commit.id, ref: params[:ref], before_sha: Gitlab::Git::BLANK_SHA)
-    end
-
     def ref_names
       @ref_names ||= project.repository.ref_names
     end
diff --git a/app/views/projects/pipelines/new.html.haml b/app/views/projects/pipelines/new.html.haml
index 534a495dd85..1050b28b381 100644
--- a/app/views/projects/pipelines/new.html.haml
+++ b/app/views/projects/pipelines/new.html.haml
@@ -1,15 +1,12 @@
 - page_title "New Pipeline"
 = render "header_title"
 
-- if @error
-  .alert.alert-danger
-    %button{ type: "button", class: "close", "data-dismiss" => "alert"} &times;
-    = @error
 %h3.page-title
   New Pipeline
 %hr
 
-= form_tag namespace_project_pipelines_path, method: :post, id: "new-pipeline-form", class: "form-horizontal js-new-pipeline-form js-requires-input" do
+= form_for @pipeline, url: namespace_project_pipelines_path(@project.namespace, @project), html: { id: "new-pipeline-form", class: "form-horizontal js-new-pipeline-form js-requires-input" } do
+  = form_errors(@pipeline)
   .form-group
     = label_tag :ref, 'Create for', class: 'control-label'
     .col-sm-10
diff --git a/spec/features/pipelines_spec.rb b/spec/features/pipelines_spec.rb
index 1df516eafd5..32665aadd22 100644
--- a/spec/features/pipelines_spec.rb
+++ b/spec/features/pipelines_spec.rb
@@ -126,12 +126,19 @@ describe "Pipelines" do
     before { visit new_namespace_project_pipeline_path(project.namespace, project) }
 
     context 'for valid commit' do
-      before do
-        fill_in('Create for', with: 'master')
-        stub_ci_commit_to_return_yaml_file
+      before { fill_in('Create for', with: 'master') }
+
+      context 'with gitlab-ci.yml' do
+        before { stub_ci_commit_to_return_yaml_file }
+
+        it { expect{ click_on 'Create pipeline' }.to change{ Ci::Commit.count }.by(1) }
       end
 
-      it { expect{ click_on 'Create pipeline' }.to change{ Ci::Commit.count }.by(1) }
+      context 'without gitlab-ci.yml' do
+        before { click_on 'Create pipeline' }
+
+        it { expect(page).to have_content('Missing .gitlab-ci.yml file') }
+      end
     end
 
     context 'for invalid commit' do
-- 
GitLab