Merge branch...

Merge branch '29181-add-more-tests-for-spec-controllers-projects-builds-controller-spec-rb' into 'master'

Resolve "Add more tests for spec/controllers/projects/builds_controller_spec.rb"

Closes #29181

See merge request !10244

app/controllers/application_controller.rb

@@ -118,6 +118,10 @@ class ApplicationController < ActionController::Base
     end
   end
 
+  def respond_422
+    head :unprocessable_entity
+  end
+
   def no_cache_headers
     response.headers["Cache-Control"] = "no-cache, no-store, max-age=0, must-revalidate"
     response.headers["Pragma"] = "no-cache"

app/controllers/projects/builds_controller.rb

 class Projects::BuildsController < Projects::ApplicationController
   before_action :build, except: [:index, :cancel_all]
-  before_action :authorize_read_build!, except: [:cancel, :cancel_all, :retry, :play]
+  before_action :authorize_read_build!, only: [:index, :show, :status, :raw, :trace]
   before_action :authorize_update_build!, except: [:index, :show, :status, :raw, :trace]
   layout 'project'
 
@@ -60,20 +60,22 @@ class Projects::BuildsController < Projects::ApplicationController
   end
 
   def retry
-    return render_404 unless @build.retryable?
+    return respond_422 unless @build.retryable?
 
     build = Ci::Build.retry(@build, current_user)
     redirect_to build_path(build)
   end
 
   def play
-    return render_404 unless @build.playable?
+    return respond_422 unless @build.playable?
 
     build = @build.play(current_user)
     redirect_to build_path(build)
   end
 
   def cancel
+    return respond_422 unless @build.cancelable?
+
     @build.cancel
     redirect_to build_path(@build)
   end
@@ -85,9 +87,12 @@ class Projects::BuildsController < Projects::ApplicationController
   end
 
   def erase
-    @build.erase(erased_by: current_user)
+    if @build.erase(erased_by: current_user)
       redirect_to namespace_project_build_path(project.namespace, project, @build),
                 notice: "Build has been successfully erased!"
+    else
+      respond_422
+    end
   end
 
   def raw

changelogs/unreleased/29181-add-more-tests-for-spec-controllers-projects-builds-controller-spec-rb.yml

+---
+title: Resolve "Add more tests for spec/controllers/projects/builds_controller_spec.rb"
+merge_request: 10244
+author: dosuken123

config/routes/project.rb

@@ -173,7 +173,7 @@ constraints(ProjectUrlConstrainer.new) do
           post :retry
           post :play
           post :erase
-          get :trace
+          get :trace, defaults: { format: 'json' }
           get :raw
         end
 

spec/factories/ci/builds.rb

@@ -79,6 +79,19 @@ FactoryGirl.define do
       manual
     end
 
+    trait :retryable do
+      success
+    end
+
+    trait :cancelable do
+      pending
+    end
+
+    trait :erasable do
+      success
+      artifacts
+    end
+
     trait :tags do
       tag_list [:docker, :ruby]
     end

spec/features/security/project/internal_access_spec.rb

@@ -399,6 +399,44 @@ describe "Internal Project Access", feature: true  do
     end
   end
 
+  describe 'GET /:project_path/builds/:id/trace' do
+    let(:pipeline) { create(:ci_pipeline, project: project) }
+    let(:build) { create(:ci_build, pipeline: pipeline) }
+    subject { trace_namespace_project_build_path(project.namespace, project, build.id) }
+
+    context 'when allowed for public and internal' do
+      before do
+        project.update(public_builds: true)
+      end
+
+      it { is_expected.to be_allowed_for(:admin) }
+      it { is_expected.to be_allowed_for(:owner).of(project) }
+      it { is_expected.to be_allowed_for(:master).of(project) }
+      it { is_expected.to be_allowed_for(:developer).of(project) }
+      it { is_expected.to be_allowed_for(:reporter).of(project) }
+      it { is_expected.to be_allowed_for(:guest).of(project) }
+      it { is_expected.to be_allowed_for(:user) }
+      it { is_expected.to be_denied_for(:external) }
+      it { is_expected.to be_denied_for(:visitor) }
+    end
+
+    context 'when disallowed for public and internal' do
+      before do
+        project.update(public_builds: false)
+      end
+
+      it { is_expected.to be_allowed_for(:admin) }
+      it { is_expected.to be_allowed_for(:owner).of(project) }
+      it { is_expected.to be_allowed_for(:master).of(project) }
+      it { is_expected.to be_allowed_for(:developer).of(project) }
+      it { is_expected.to be_allowed_for(:reporter).of(project) }
+      it { is_expected.to be_denied_for(:guest).of(project) }
+      it { is_expected.to be_denied_for(:user) }
+      it { is_expected.to be_denied_for(:external) }
+      it { is_expected.to be_denied_for(:visitor) }
+    end
+  end
+
   describe "GET /:project_path/environments" do
     subject { namespace_project_environments_path(project.namespace, project) }
 

spec/features/security/project/private_access_spec.rb

@@ -388,6 +388,38 @@ describe "Private Project Access", feature: true  do
     end
   end
 
+  describe 'GET /:project_path/builds/:id/trace' do
+    let(:pipeline) { create(:ci_pipeline, project: project) }
+    let(:build) { create(:ci_build, pipeline: pipeline) }
+    subject { trace_namespace_project_build_path(project.namespace, project, build.id) }
+
+    it { is_expected.to be_allowed_for(:admin) }
+    it { is_expected.to be_allowed_for(:owner).of(project) }
+    it { is_expected.to be_allowed_for(:master).of(project) }
+    it { is_expected.to be_allowed_for(:developer).of(project) }
+    it { is_expected.to be_allowed_for(:reporter).of(project) }
+    it { is_expected.to be_denied_for(:guest).of(project) }
+    it { is_expected.to be_denied_for(:user) }
+    it { is_expected.to be_denied_for(:external) }
+    it { is_expected.to be_denied_for(:visitor) }
+
+    context 'when public builds is enabled' do
+      before do
+        project.update(public_builds: true)
+      end
+
+      it { is_expected.to be_allowed_for(:guest).of(project) }
+    end
+
+    context 'when public builds is disabled' do
+      before do
+        project.update(public_builds: false)
+      end
+
+      it { is_expected.to be_denied_for(:guest).of(project) }
+    end
+  end
+
   describe "GET /:project_path/environments" do
     subject { namespace_project_environments_path(project.namespace, project) }
 

spec/features/security/project/public_access_spec.rb

@@ -219,6 +219,44 @@ describe "Public Project Access", feature: true  do
     end
   end
 
+  describe 'GET /:project_path/builds/:id/trace' do
+    let(:pipeline) { create(:ci_pipeline, project: project) }
+    let(:build) { create(:ci_build, pipeline: pipeline) }
+    subject { trace_namespace_project_build_path(project.namespace, project, build.id) }
+
+    context 'when allowed for public' do
+      before do
+        project.update(public_builds: true)
+      end
+
+      it { is_expected.to be_allowed_for(:admin) }
+      it { is_expected.to be_allowed_for(:owner).of(project) }
+      it { is_expected.to be_allowed_for(:master).of(project) }
+      it { is_expected.to be_allowed_for(:developer).of(project) }
+      it { is_expected.to be_allowed_for(:reporter).of(project) }
+      it { is_expected.to be_allowed_for(:guest).of(project) }
+      it { is_expected.to be_allowed_for(:user) }
+      it { is_expected.to be_allowed_for(:external) }
+      it { is_expected.to be_allowed_for(:visitor) }
+    end
+
+    context 'when disallowed for public' do
+      before do
+        project.update(public_builds: false)
+      end
+
+      it { is_expected.to be_allowed_for(:admin) }
+      it { is_expected.to be_allowed_for(:owner).of(project) }
+      it { is_expected.to be_allowed_for(:master).of(project) }
+      it { is_expected.to be_allowed_for(:developer).of(project) }
+      it { is_expected.to be_allowed_for(:reporter).of(project) }
+      it { is_expected.to be_denied_for(:guest).of(project) }
+      it { is_expected.to be_denied_for(:user) }
+      it { is_expected.to be_denied_for(:external) }
+      it { is_expected.to be_denied_for(:visitor) }
+    end
+  end
+
   describe "GET /:project_path/environments" do
     subject { namespace_project_environments_path(project.namespace, project) }
 

spec/support/matchers/access_matchers.rb

@@ -51,7 +51,7 @@ module AccessMatchers
       emulate_user(user, @membership)
       visit(url)
 
-      status_code != 404 && current_path != new_user_session_path
+      status_code == 200 && current_path != new_user_session_path
     end
 
     chain :of do |membership|
@@ -66,7 +66,7 @@ module AccessMatchers
       emulate_user(user, @membership)
       visit(url)
 
-      status_code == 404 || current_path == new_user_session_path
+      [401, 404].include?(status_code) || current_path == new_user_session_path
     end
 
     chain :of do |membership|