From 1bfa818a1f3ac9c40a8d4fc727cfe848efcef962 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 4 Jul 2017 17:45:07 +0900 Subject: [PATCH] zj nice catchies 3 --- .../projects/pipeline_schedules_controller.rb | 2 +- .../pipeline_schedules_controller_spec.rb | 50 +++++++++---------- .../projects/pipeline_schedules_spec.rb | 2 + spec/models/ci/pipeline_schedule_spec.rb | 2 +- .../access_matchers_for_controller.rb | 6 ++- 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