zj nice catchies 3

5 files changed, 34 insertions, 28 deletions

diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb
index f0ac0e7098c..aa71f606657 100644
--- a/app/controllers/projects/pipeline_schedules_controller.rb
+++ b/app/controllers/projects/pipeline_schedules_controller.rb
@@ -53,7 +53,7 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
       redirect_to pipeline_schedules_path(@project), status: 302
     else
       redirect_to pipeline_schedules_path(@project),
-                  status: 302,
+                  status: :forbidden,
                   alert: _("Failed to remove the pipeline schedule")
     end
   end
diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
index be6a758bb4d..bfd31f9409b 100644
--- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb
+++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
@@ -72,7 +72,7 @@ describe Projects::PipelineSchedulesController do
       end
 
       let(:basic_param) do
-        { description: 'aaaaaaaa', cron: '0 4 * * *', cron_timezone: 'UTC', ref: 'master', active: '1' }
+        attributes_for(:ci_pipeline_schedule)
       end
 
       context 'when variables_attributes is empty' do
@@ -100,8 +100,11 @@ describe Projects::PipelineSchedulesController do
             .and change { Ci::PipelineScheduleVariable.count }.by(1)
 
           expect(response).to have_http_status(:found)
-          expect(Ci::PipelineScheduleVariable.last.key).to eq("AAA")
-          expect(Ci::PipelineScheduleVariable.last.value).to eq("AAA123")
+
+          Ci::PipelineScheduleVariable.last.tap do |v|
+            expect(v.key).to eq("AAA")
+            expect(v.value).to eq("AAA123")
+          end
         end
 
         context 'when the same key has already been persisted' do
@@ -143,10 +146,16 @@ describe Projects::PipelineSchedulesController do
             .and change { Ci::PipelineScheduleVariable.count }.by(2)
 
           expect(response).to have_http_status(:found)
-          expect(Ci::PipelineScheduleVariable.first.key).to eq("AAA")
-          expect(Ci::PipelineScheduleVariable.first.value).to eq("AAA123")
-          expect(Ci::PipelineScheduleVariable.last.key).to eq("BBB")
-          expect(Ci::PipelineScheduleVariable.last.value).to eq("BBB123")
+
+          Ci::PipelineScheduleVariable.first.tap do |v|
+            expect(v.key).to eq("AAA")
+            expect(v.value).to eq("AAA123")
+          end
+
+          Ci::PipelineScheduleVariable.last.tap do |v|
+            expect(v.key).to eq("BBB")
+            expect(v.value).to eq("BBB123")
+          end
         end
       end
 
@@ -168,7 +177,7 @@ describe Projects::PipelineSchedulesController do
     end
 
     describe 'security' do
-      let(:schedule) { { description: 'aaaaaaaa', cron: '0 4 * * *', cron_timezone: 'UTC', ref: 'master', active: '1' } }
+      let(:schedule) { attributes_for(:ci_pipeline_schedule) }
 
       it { expect { go }.to be_allowed_for(:admin) }
       it { expect { go }.to be_allowed_for(:owner).of(project) }
@@ -198,7 +207,7 @@ describe Projects::PipelineSchedulesController do
 
       context 'when a pipeline schedule has no variables' do
         let(:basic_param) do
-          { description: 'updated_desc', cron: '0 1 * * *', cron_timezone: 'UTC', ref: 'patch-x', active: '1' }
+          { description: 'updated_desc', cron: '0 1 * * *', cron_timezone: 'UTC', ref: 'patch-x', active: true }
         end
 
         context 'when params do not include variables' do
@@ -209,11 +218,7 @@ describe Projects::PipelineSchedulesController do
 
             pipeline_schedule.reload
             expect(response).to have_http_status(:found)
-            expect(pipeline_schedule.description).to eq('updated_desc')
-            expect(pipeline_schedule.cron).to eq('0 1 * * *')
-            expect(pipeline_schedule.cron_timezone).to eq('UTC')
-            expect(pipeline_schedule.ref).to eq('patch-x')
-            expect(pipeline_schedule.active).to eq(true)
+            expect(pipeline_schedule).to have_attributes(basic_param)
             expect(pipeline_schedule.variables).to be_empty
           end
         end
@@ -272,7 +277,7 @@ describe Projects::PipelineSchedulesController do
 
       context 'when a pipeline schedule has one variable' do
         let(:basic_param) do
-          { description: 'updated_desc', cron: '0 1 * * *', cron_timezone: 'UTC', ref: 'patch-x', active: '1' }
+          { description: 'updated_desc', cron: '0 1 * * *', cron_timezone: 'UTC', ref: 'patch-x', active: true }
         end
 
         let!(:pipeline_schedule_variable) do
@@ -288,12 +293,7 @@ describe Projects::PipelineSchedulesController do
 
             pipeline_schedule.reload
             expect(response).to have_http_status(:found)
-            expect(pipeline_schedule.description).to eq('updated_desc')
-            expect(pipeline_schedule.cron).to eq('0 1 * * *')
-            expect(pipeline_schedule.cron_timezone).to eq('UTC')
-            expect(pipeline_schedule.ref).to eq('patch-x')
-            expect(pipeline_schedule.active).to eq(true)
-            expect(pipeline_schedule.variables.count).to eq(1)
+            expect(pipeline_schedule).to have_attributes(basic_param)
             expect(pipeline_schedule.variables.last.key).to eq('CCC')
           end
         end
@@ -350,7 +350,7 @@ describe Projects::PipelineSchedulesController do
       it { expect { go }.to be_allowed_for(:admin) }
       it { expect { go }.to be_allowed_for(:owner).of(project) }
       it { expect { go }.to be_allowed_for(:master).of(project) }
-      it { expect { go }.to be_allowed_for(:developer).of(project).own([pipeline_schedule]) }
+      it { expect { go }.to be_allowed_for(:developer).of(project).own(pipeline_schedule) }
       it { expect { go }.to be_denied_for(:reporter).of(project) }
       it { expect { go }.to be_denied_for(:guest).of(project) }
       it { expect { go }.to be_denied_for(:user) }
@@ -412,7 +412,7 @@ describe Projects::PipelineSchedulesController do
       it { expect { go }.to be_allowed_for(:admin) }
       it { expect { go }.to be_allowed_for(:owner).of(project) }
       it { expect { go }.to be_allowed_for(:master).of(project) }
-      it { expect { go }.to be_allowed_for(:developer).of(project).own([pipeline_schedule]) }
+      it { expect { go }.to be_allowed_for(:developer).of(project).own(pipeline_schedule) }
       it { expect { go }.to be_denied_for(:reporter).of(project) }
       it { expect { go }.to be_denied_for(:guest).of(project) }
       it { expect { go }.to be_denied_for(:user) }
@@ -430,7 +430,7 @@ describe Projects::PipelineSchedulesController do
       it { expect { go }.to be_allowed_for(:admin) }
       it { expect { go }.to be_allowed_for(:owner).of(project) }
       it { expect { go }.to be_allowed_for(:master).of(project) }
-      it { expect { go }.to be_allowed_for(:developer).of(project).own([pipeline_schedule]) }
+      it { expect { go }.to be_allowed_for(:developer).of(project).own(pipeline_schedule) }
       it { expect { go }.to be_denied_for(:reporter).of(project) }
       it { expect { go }.to be_denied_for(:guest).of(project) }
       it { expect { go }.to be_denied_for(:user) }
@@ -455,7 +455,7 @@ describe Projects::PipelineSchedulesController do
       end
 
       it 'does not delete the pipeline schedule' do
-        expect(response).not_to have_http_status(:ok)
+        expect(response).to have_http_status(:not_found)
       end
     end
 
diff --git a/spec/features/projects/pipeline_schedules_spec.rb b/spec/features/projects/pipeline_schedules_spec.rb
index fa7f8561f46..7ab6b3f0b75 100644
--- a/spec/features/projects/pipeline_schedules_spec.rb
+++ b/spec/features/projects/pipeline_schedules_spec.rb
@@ -157,6 +157,7 @@ feature 'Pipeline Schedules', :feature, js: true do
       create(:ci_pipeline_schedule, project: project, owner: user).tap do |pipeline_schedule|
         create(:ci_pipeline_schedule_variable, key: 'AAA', value: 'AAA123', pipeline_schedule: pipeline_schedule)
       end
+
       visit_pipelines_schedules
       find(".content-list .pipeline-schedule-table-row:nth-child(1) .btn-group a[title='Edit']").click
       all('[name="schedule[variables_attributes][][key]"]')[0].set('foo')
@@ -178,6 +179,7 @@ feature 'Pipeline Schedules', :feature, js: true do
       create(:ci_pipeline_schedule, project: project, owner: user).tap do |pipeline_schedule|
         create(:ci_pipeline_schedule_variable, key: 'AAA', value: 'AAA123', pipeline_schedule: pipeline_schedule)
       end
+
       visit_pipelines_schedules
       find(".content-list .pipeline-schedule-table-row:nth-child(1) .btn-group a[title='Edit']").click
       find('.pipeline-variable-list .pipeline-variable-row-remove-button').click
diff --git a/spec/models/ci/pipeline_schedule_spec.rb b/spec/models/ci/pipeline_schedule_spec.rb
index 54177bee5ce..4c3aa986bf9 100644
--- a/spec/models/ci/pipeline_schedule_spec.rb
+++ b/spec/models/ci/pipeline_schedule_spec.rb
@@ -120,7 +120,7 @@ describe Ci::PipelineSchedule, models: true do
   end
 
   describe '#job_variables' do
-    let!(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly) }
+    let!(:pipeline_schedule) { create(:ci_pipeline_schedule) }
 
     let!(:pipeline_schedule_variables) do
       create_list(:ci_pipeline_schedule_variable, 2, pipeline_schedule: pipeline_schedule)
diff --git a/spec/support/matchers/access_matchers_for_controller.rb b/spec/support/matchers/access_matchers_for_controller.rb
index 21128bcc73d..40d6e221428 100644
--- a/spec/support/matchers/access_matchers_for_controller.rb
+++ b/spec/support/matchers/access_matchers_for_controller.rb
@@ -77,7 +77,7 @@ module AccessMatchersForController
       @membership = membership
     end
 
-    chain :own do |objects|
+    chain :own do |*objects|
       @objects = objects
     end
 
@@ -98,6 +98,10 @@ module AccessMatchersForController
       @membership = membership
     end
 
+    chain :own do |*objects|
+      @objects = objects
+    end
+
     description { description_for(role, 'denied', EXPECTED_STATUS_CODE_DENIED, response.status) }
     supports_block_expectations
   end