Fixed the following.
- Fix inappropriate evaluation(casecmp) to regex - Fix missed boolean conversion - Improve spec
This commit is contained in:
parent
a114c988b4
commit
f0bc9af36b
2 changed files with 43 additions and 23 deletions
|
@ -88,8 +88,9 @@ class PipelinesFinder
|
||||||
end
|
end
|
||||||
|
|
||||||
def by_yaml_errors(items)
|
def by_yaml_errors(items)
|
||||||
if params[:yaml_errors].present?
|
flg = Gitlab::Utils.to_boolean(params[:yaml_errors])
|
||||||
if params[:yaml_errors]
|
if flg.present?
|
||||||
|
if flg
|
||||||
items.where("yaml_errors IS NOT NULL")
|
items.where("yaml_errors IS NOT NULL")
|
||||||
else
|
else
|
||||||
items.where("yaml_errors IS NULL")
|
items.where("yaml_errors IS NULL")
|
||||||
|
@ -102,7 +103,7 @@ class PipelinesFinder
|
||||||
def order_and_sort(items)
|
def order_and_sort(items)
|
||||||
if params[:order_by].present? && params[:sort].present? &&
|
if params[:order_by].present? && params[:sort].present? &&
|
||||||
items.column_names.include?(params[:order_by]) &&
|
items.column_names.include?(params[:order_by]) &&
|
||||||
(params[:sort].casecmp('ASC') || params[:sort].casecmp('DESC'))
|
params[:sort] =~ /\A(ASC|DESC)\Z/i
|
||||||
items.reorder(params[:order_by] => params[:sort])
|
items.reorder(params[:order_by] => params[:sort])
|
||||||
else
|
else
|
||||||
items.reorder(id: :desc)
|
items.reorder(id: :desc)
|
||||||
|
|
|
@ -13,6 +13,17 @@ describe PipelinesFinder do
|
||||||
let!(:canceled_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 2.minutes.ago, status: 'canceled') }
|
let!(:canceled_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 2.minutes.ago, status: 'canceled') }
|
||||||
let!(:skipped_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 1.minute.ago, status: 'skipped') }
|
let!(:skipped_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 1.minute.ago, status: 'skipped') }
|
||||||
let!(:yaml_errors_pipeline) { create(:ci_pipeline, project: project, user: user, yaml_errors: 'Syntax error') }
|
let!(:yaml_errors_pipeline) { create(:ci_pipeline, project: project, user: user, yaml_errors: 'Syntax error') }
|
||||||
|
let(:dummy_pipelines) do
|
||||||
|
[tag_pipeline,
|
||||||
|
created_pipeline,
|
||||||
|
pending_pipeline,
|
||||||
|
running_pipeline,
|
||||||
|
success_pipeline,
|
||||||
|
failed_pipeline,
|
||||||
|
canceled_pipeline,
|
||||||
|
skipped_pipeline,
|
||||||
|
yaml_errors_pipeline]
|
||||||
|
end
|
||||||
|
|
||||||
subject { described_class.new(project, params).execute }
|
subject { described_class.new(project, params).execute }
|
||||||
|
|
||||||
|
@ -21,29 +32,12 @@ describe PipelinesFinder do
|
||||||
let(:params) { {} }
|
let(:params) { {} }
|
||||||
|
|
||||||
it 'selects all pipelines' do
|
it 'selects all pipelines' do
|
||||||
expect(subject.count).to be 9
|
expect(subject.count).to be dummy_pipelines.count
|
||||||
expect(subject).to include tag_pipeline
|
expect(subject).to match_array(dummy_pipelines)
|
||||||
expect(subject).to include created_pipeline
|
|
||||||
expect(subject).to include pending_pipeline
|
|
||||||
expect(subject).to include running_pipeline
|
|
||||||
expect(subject).to include success_pipeline
|
|
||||||
expect(subject).to include failed_pipeline
|
|
||||||
expect(subject).to include canceled_pipeline
|
|
||||||
expect(subject).to include skipped_pipeline
|
|
||||||
expect(subject).to include yaml_errors_pipeline
|
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'orders in descending order on ID' do
|
it 'orders in descending order on ID' do
|
||||||
expected_ids = [tag_pipeline.id,
|
expect(subject.map(&:id)).to eq dummy_pipelines.map(&:id).sort.reverse
|
||||||
created_pipeline.id,
|
|
||||||
pending_pipeline.id,
|
|
||||||
running_pipeline.id,
|
|
||||||
success_pipeline.id,
|
|
||||||
failed_pipeline.id,
|
|
||||||
canceled_pipeline.id,
|
|
||||||
skipped_pipeline.id,
|
|
||||||
yaml_errors_pipeline.id].sort.reverse
|
|
||||||
expect(subject.map(&:id)).to eq expected_ids
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -210,6 +204,15 @@ describe PipelinesFinder do
|
||||||
expect(subject).to include skipped_pipeline
|
expect(subject).to include skipped_pipeline
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context 'when an argument is invalid' do
|
||||||
|
let(:params) { { yaml_errors: "UnexpectedValue" } }
|
||||||
|
|
||||||
|
it 'selects all pipelines' do
|
||||||
|
expect(subject.count).to be dummy_pipelines.count
|
||||||
|
expect(subject).to match_array(dummy_pipelines)
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'when a order_by and sort are passed' do
|
context 'when a order_by and sort are passed' do
|
||||||
|
@ -230,6 +233,22 @@ describe PipelinesFinder do
|
||||||
expect(subject.last).to eq(tag_pipeline)
|
expect(subject.last).to eq(tag_pipeline)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context 'when order_by does not exist' do
|
||||||
|
let(:params) { { order_by: 'abnormal_column', sort: 'desc' } }
|
||||||
|
|
||||||
|
it 'sorts by default' do
|
||||||
|
expect(subject.map(&:id)).to eq dummy_pipelines.map(&:id).sort.reverse
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when sort does not exist' do
|
||||||
|
let(:params) { { order_by: 'created_at', sort: 'abnormal_sort' } }
|
||||||
|
|
||||||
|
it 'sorts by default' do
|
||||||
|
expect(subject.map(&:id)).to eq dummy_pipelines.map(&:id).sort.reverse
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in a new issue