From e9d94451959e4717e0ba4ca882b5a54437efbee1 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 21 Feb 2017 17:20:50 +0900 Subject: [PATCH 01/57] - Add new parameters for Pipeline API - Expand PipelinesFinder functions --- .../projects/pipelines_controller.rb | 10 +- app/finders/pipelines_finder.rb | 108 ++++++++++++++---- lib/api/pipelines.rb | 12 +- 3 files changed, 104 insertions(+), 26 deletions(-) diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb index 1780cc0233c..454b8ee17af 100644 --- a/app/controllers/projects/pipelines_controller.rb +++ b/app/controllers/projects/pipelines_controller.rb @@ -9,19 +9,19 @@ class Projects::PipelinesController < Projects::ApplicationController def index @scope = params[:scope] @pipelines = PipelinesFinder - .new(project) - .execute(scope: @scope) + .new(project, scope: @scope) + .execute .page(params[:page]) .per(30) @running_count = PipelinesFinder - .new(project).execute(scope: 'running').count + .new(project, scope: 'running').execute.count @pending_count = PipelinesFinder - .new(project).execute(scope: 'pending').count + .new(project, scope: 'pending').execute.count @finished_count = PipelinesFinder - .new(project).execute(scope: 'finished').count + .new(project, scope: 'finished').execute.count @pipelines_count = PipelinesFinder .new(project).execute.count diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index a9172f6767f..5a5416d00cc 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -1,29 +1,21 @@ class PipelinesFinder - attr_reader :project, :pipelines + attr_reader :project, :pipelines, :params - def initialize(project) + def initialize(project, params = {}) @project = project @pipelines = project.pipelines + @params = params end - def execute(scope: nil) - scoped_pipelines = - case scope - when 'running' - pipelines.running - when 'pending' - pipelines.pending - when 'finished' - pipelines.finished - when 'branches' - from_ids(ids_for_ref(branches)) - when 'tags' - from_ids(ids_for_ref(tags)) - else - pipelines - end - - scoped_pipelines.order(id: :desc) + def execute + items = pipelines + items = by_scope(items) + items = by_status(items) + items = by_ref(items) + items = by_user(items) + items = by_duration(items) + items = by_yaml_error(items) + order_and_sort(items) end private @@ -43,4 +35,80 @@ class PipelinesFinder def tags project.repository.tag_names end + + def by_scope(items) + case params[:scope] + when 'running' + items.running + when 'pending' + items.pending + when 'finished' + items.finished + when 'branches' + from_ids(ids_for_ref(branches)) + when 'tags' + from_ids(ids_for_ref(tags)) + else + items + end + end + + def by_status(items) + case params[:status] + when 'running' + items.running + when 'pending' + items.pending + when 'success' + items.success + when 'failed' + items.failed + when 'canceled' + items.canceled + when 'skipped' + items.skipped + else + items + end + end + + def by_ref(items) + if params[:ref].present? + items.where(ref: params[:ref]) + else + items + end + end + + def by_user(items) + if params[:user_id].present? + items.where(user_id: params[:user_id]) + else + items + end + end + + def by_duration(items) + if params[:duration].present? + items.where("duration > ?", params[:duration]) + else + items + end + end + + def by_yaml_error(items) + if params[:yaml_error].present? && params[:yaml_error] + items.where("yaml_errors IS NOT NULL") + else + items + end + end + + def order_and_sort(items) + if params[:order_by].present? && params[:sort].present? + items.order("#{params[:order_by]} #{params[:sort]}") + else + items.order(id: :desc) + end + end end diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 754c3d85a04..905e72a3a95 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -16,11 +16,21 @@ module API use :pagination optional :scope, type: String, values: %w(running branches tags), desc: 'Either running, branches, or tags' + optional :status, type: String, values: ['running', 'pending', 'success', 'failed', 'canceled', 'skipped'], + desc: 'Pipeline Status' + optional :ref, type: String, desc: 'Pipeline Ref' + optional :duration, type: Integer, desc: 'Greater than the specified duration' + optional :yaml_error, type: Boolean, desc: 'If true, returns only yaml error pipelines.' + optional :user_id, type: String, desc: 'User who executed pipelines' + optional :order_by, type: String, values: ['id', 'status', 'ref', 'user_id', 'started_at', 'finished_at', 'created_at', 'updated_at'], default: 'id', + desc: 'Return issues ordered by `created_at` or `updated_at` fields.' + optional :sort, type: String, values: ['asc', 'desc'], default: 'desc', + desc: 'Return pipelines sorted in `asc` or `desc` order.' end get ':id/pipelines' do authorize! :read_pipeline, user_project - pipelines = PipelinesFinder.new(user_project).execute(scope: params[:scope]) + pipelines = PipelinesFinder.new(user_project, params).execute(scope: params[:scope]) present paginate(pipelines), with: Entities::PipelineBasic end From 01f6083939f8f9559f7e134f111bd40d17d357f9 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 21 Feb 2017 18:19:27 +0900 Subject: [PATCH 02/57] Remove unnecessary comment --- app/finders/pipelines_finder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 5a5416d00cc..3a95a1eb2ae 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -87,7 +87,7 @@ class PipelinesFinder items end end - + def by_duration(items) if params[:duration].present? items.where("duration > ?", params[:duration]) From 994e49b3fbc261f8e59429c1681d83c81ba25df3 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 28 Feb 2017 21:24:49 +0900 Subject: [PATCH 03/57] Fixed those points. - username to user_id - Drop duration - Resolve comments - Add Changelog - Edit docs --- app/finders/pipelines_finder.rb | 27 +++++++------------ ...nclude-search-options-to-pipelines-api.yml | 4 +++ doc/api/pipelines.md | 7 +++++ lib/api/pipelines.rb | 17 ++++++------ 4 files changed, 29 insertions(+), 26 deletions(-) create mode 100644 changelogs/unreleased/28408-feature-proposal-include-search-options-to-pipelines-api.yml diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 3a95a1eb2ae..c2e247a7ded 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -12,9 +12,8 @@ class PipelinesFinder items = by_scope(items) items = by_status(items) items = by_ref(items) - items = by_user(items) - items = by_duration(items) - items = by_yaml_error(items) + items = by_username(items) + items = by_yaml_errors(items) order_and_sort(items) end @@ -80,24 +79,16 @@ class PipelinesFinder end end - def by_user(items) - if params[:user_id].present? - items.where(user_id: params[:user_id]) + def by_username(items) + if params[:username].present? + items.joins(:user).where("users.name = ?", params[:username]) else items end end - def by_duration(items) - if params[:duration].present? - items.where("duration > ?", params[:duration]) - else - items - end - end - - def by_yaml_error(items) - if params[:yaml_error].present? && params[:yaml_error] + def by_yaml_errors(items) + if params[:yaml_errors].present? && params[:yaml_errors] items.where("yaml_errors IS NOT NULL") else items @@ -105,7 +96,9 @@ class PipelinesFinder end 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]) && + (params[:sort].downcase == 'asc' || params[:sort].downcase == 'desc') items.order("#{params[:order_by]} #{params[:sort]}") else items.order(id: :desc) diff --git a/changelogs/unreleased/28408-feature-proposal-include-search-options-to-pipelines-api.yml b/changelogs/unreleased/28408-feature-proposal-include-search-options-to-pipelines-api.yml new file mode 100644 index 00000000000..6fb3da99fb4 --- /dev/null +++ b/changelogs/unreleased/28408-feature-proposal-include-search-options-to-pipelines-api.yml @@ -0,0 +1,4 @@ +--- +title: Resolve Feature Proposal Include search options to pipelines API +merge_request: 9367 +author: dosuken123 diff --git a/doc/api/pipelines.md b/doc/api/pipelines.md index 732ad8da4ac..9281e6bb6d5 100644 --- a/doc/api/pipelines.md +++ b/doc/api/pipelines.md @@ -11,6 +11,13 @@ GET /projects/:id/pipelines | Attribute | Type | Required | Description | |-----------|---------|----------|---------------------| | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `scope` | string | no | The scope of pipelines, one of: `running`, `pending`, `finished`, `branches`, `tags`; | +| `status` | string | no | The status of pipelines, one of: `running`, `pending`, `success`, `failed`, `canceled`, `skipped`; | +| `ref` | string | no | The ref of pipelines | +| `yaml_errors`| string | no | If true, Returns only yaml error pipelines | +| `username`| string | no | The name of user who triggered pipelines | +| `order_by`| string | no | The order_by which is combined with a `sort`, one of: `id`, `status`, `ref`, `user_id`, `started_at`, `finished_at`, `created_at`, `updated_at`; | +| `sort` | string | no | The sort method which is combined with an `order_by`, one of: `asc`, `desc`; | ``` curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects/1/pipelines" diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 905e72a3a95..48ab5c21780 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -14,18 +14,17 @@ module API end params do use :pagination - optional :scope, type: String, values: %w(running branches tags), - desc: 'Either running, branches, or tags' + optional :scope, type: String, values: %w(running pending finished branches tags), + desc: 'The scope of pipelines' optional :status, type: String, values: ['running', 'pending', 'success', 'failed', 'canceled', 'skipped'], - desc: 'Pipeline Status' - optional :ref, type: String, desc: 'Pipeline Ref' - optional :duration, type: Integer, desc: 'Greater than the specified duration' - optional :yaml_error, type: Boolean, desc: 'If true, returns only yaml error pipelines.' - optional :user_id, type: String, desc: 'User who executed pipelines' + desc: 'The status of pipelines' + optional :ref, type: String, desc: 'The ref of pipelines' + optional :yaml_errors, type: Boolean, desc: 'If true, Returns only yaml error pipelines' + optional :username, type: String, desc: 'The name of user who triggered pipelines' optional :order_by, type: String, values: ['id', 'status', 'ref', 'user_id', 'started_at', 'finished_at', 'created_at', 'updated_at'], default: 'id', - desc: 'Return issues ordered by `created_at` or `updated_at` fields.' + desc: 'The order_by which is combined with a sort' optional :sort, type: String, values: ['asc', 'desc'], default: 'desc', - desc: 'Return pipelines sorted in `asc` or `desc` order.' + desc: 'The sort method which is combined with an order_by' end get ':id/pipelines' do authorize! :read_pipeline, user_project From df834306c1794ed72d6d655c7941dee28f7e85c7 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 1 Mar 2017 02:34:48 +0900 Subject: [PATCH 04/57] Add specs. Plus, minor fixes. --- app/finders/pipelines_finder.rb | 8 +- doc/api/pipelines.md | 2 +- lib/api/pipelines.rb | 2 +- spec/finders/pipelines_finder_spec.rb | 220 +++++++++++++++++++++++--- 4 files changed, 210 insertions(+), 22 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index c2e247a7ded..ff16d7305ad 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -88,8 +88,12 @@ class PipelinesFinder end def by_yaml_errors(items) - if params[:yaml_errors].present? && params[:yaml_errors] - items.where("yaml_errors IS NOT NULL") + if params[:yaml_errors].present? + if params[:yaml_errors] + items.where("yaml_errors IS NOT NULL") + else + items.where("yaml_errors IS NULL") + end else items end diff --git a/doc/api/pipelines.md b/doc/api/pipelines.md index 9281e6bb6d5..677b96c74cc 100644 --- a/doc/api/pipelines.md +++ b/doc/api/pipelines.md @@ -16,7 +16,7 @@ GET /projects/:id/pipelines | `ref` | string | no | The ref of pipelines | | `yaml_errors`| string | no | If true, Returns only yaml error pipelines | | `username`| string | no | The name of user who triggered pipelines | -| `order_by`| string | no | The order_by which is combined with a `sort`, one of: `id`, `status`, `ref`, `user_id`, `started_at`, `finished_at`, `created_at`, `updated_at`; | +| `order_by`| string | no | The order_by which is combined with a `sort`, one of: `id`, `status`, `ref`, `username`, `started_at`, `finished_at`, `created_at`, `updated_at`; | | `sort` | string | no | The sort method which is combined with an `order_by`, one of: `asc`, `desc`; | ``` diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 48ab5c21780..ecd6b64cfa7 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -21,7 +21,7 @@ module API optional :ref, type: String, desc: 'The ref of pipelines' optional :yaml_errors, type: Boolean, desc: 'If true, Returns only yaml error pipelines' optional :username, type: String, desc: 'The name of user who triggered pipelines' - optional :order_by, type: String, values: ['id', 'status', 'ref', 'user_id', 'started_at', 'finished_at', 'created_at', 'updated_at'], default: 'id', + optional :order_by, type: String, values: ['id', 'status', 'ref', 'username', 'started_at', 'finished_at', 'created_at', 'updated_at'], default: 'id', desc: 'The order_by which is combined with a sort' optional :sort, type: String, values: ['asc', 'desc'], default: 'desc', desc: 'The sort method which is combined with an order_by' diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 6bada7b3eb9..3bb828000d4 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -1,22 +1,75 @@ require 'spec_helper' describe PipelinesFinder do + let(:user) { create(:user) } let(:project) { create(:project, :repository) } - let!(:tag_pipeline) { create(:ci_pipeline, project: project, ref: 'v1.0.0') } - let!(:branch_pipeline) { create(:ci_pipeline, project: project) } + let!(:tag_pipeline) { create(:ci_pipeline, project: project, user: user, ref: 'v1.0.0') } + let!(:created_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'created') } + let!(:pending_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'pending') } + let!(:running_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'running') } + let!(:success_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'success') } + let!(:failed_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'failed') } + let!(:canceled_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'canceled') } + let!(:skipped_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'skipped') } + let!(:yaml_errors_pipeline) { create(:ci_pipeline, project: project, user: user, yaml_errors: 'Syntax error') } - subject { described_class.new(project).execute(params) } + subject { described_class.new(project, params).execute } describe "#execute" do - context 'when a scope is passed' do - context 'when scope is nil' do - let(:params) { { scope: nil } } + context 'when nothing is passed' do + let(:params) { {} } - it 'selects all pipelines' do - expect(subject.count).to be 2 - expect(subject).to include tag_pipeline - expect(subject).to include branch_pipeline + it 'selects all pipelines' do + expect(subject.count).to be 9 + expect(subject).to include tag_pipeline + 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 + + it 'orders in descending order on ID' do + expected_ids = [ + tag_pipeline.id, + 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 + + context 'when a scope is passed' do + context 'when selecting running' do + let(:params) { { scope: 'running' } } + + it 'has only running status' do + expect(subject.map(&:status)).to include('running') + end + end + + context 'when selecting pending' do + let(:params) { { scope: 'pending' } } + + it 'has only pending status' do + expect(subject.map(&:status)).to include('pending') + end + end + + context 'when selecting finished' do + let(:params) { { scope: 'finished' } } + + it 'has only finished status' do + expect(subject.map(&:status)).to match_array %w(success canceled failed) end end @@ -24,8 +77,9 @@ describe PipelinesFinder do let(:params) { { scope: 'branches' } } it 'excludes tags' do + expect(subject.count).to be 1 expect(subject).not_to include tag_pipeline - expect(subject).to include branch_pipeline + expect(subject.map(&:ref)).to include('master') end end @@ -33,20 +87,150 @@ describe PipelinesFinder do let(:params) { { scope: 'tags' } } it 'excludes branches' do + expect(subject.count).to be 1 expect(subject).to include tag_pipeline - expect(subject).not_to include branch_pipeline + expect(subject.map(&:ref)).not_to include('master') end end end - # Scoping to pending will speed up the test as it doesn't hit the FS - let(:params) { { scope: 'pending' } } + context 'when a status is passed' do + context 'when selecting running' do + let(:params) { { scope: 'running' } } - it 'orders in descending order on ID' do - feature_pipeline = create(:ci_pipeline, project: project, ref: 'feature') + it 'has only running status' do + expect(subject.map(&:status)).to include('running') + end + end - expected_ids = [feature_pipeline.id, branch_pipeline.id, tag_pipeline.id].sort.reverse - expect(subject.map(&:id)).to eq expected_ids + context 'when selecting pending' do + let(:params) { { scope: 'pending' } } + + it 'has only pending status' do + expect(subject.map(&:status)).to include('pending') + end + end + + context 'when selecting success' do + let(:params) { { scope: 'success' } } + + it 'has only success status' do + expect(subject.map(&:status)).to include('success') + end + end + + context 'when selecting failed' do + let(:params) { { scope: 'failed' } } + + it 'has only failed status' do + expect(subject.map(&:status)).to include('failed') + end + end + + context 'when selecting canceled' do + let(:params) { { scope: 'canceled' } } + + it 'has only canceled status' do + expect(subject.map(&:status)).to include('canceled') + end + end + + context 'when selecting skipped' do + let(:params) { { scope: 'skipped' } } + + it 'has only skipped status' do + expect(subject.map(&:status)).to include('skipped') + end + end + end + + context 'when a ref is passed' do + context 'when a ref exists' do + let(:params) { { ref: 'master' } } + + it 'selects all pipelines which belong to the ref' do + expect(subject.count).to be 8 + 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 + + context 'when a ref does not exist' do + let(:params) { { ref: 'unique-ref' } } + + it 'selects nothing' do + expect(subject).to be_empty + end + end + end + + context 'when a username is passed' do + context 'when a username exists' do + let(:params) { { username: user.name } } + + it 'selects all pipelines which belong to the username' do + expect(subject).to include success_pipeline + expect(subject).to include failed_pipeline + end + end + + context 'when a username does not exist' do + let(:params) { { username: 'unique-username' } } + + it 'selects nothing' do + expect(subject).to be_empty + end + end + end + + context 'when a yaml_errors is passed' do + context 'when yaml_errors is true' do + let(:params) { { yaml_errors: true } } + + it 'selects only pipelines has yaml_errors' do + expect(subject).to include yaml_errors_pipeline + end + end + + context 'when yaml_errors is false' do + let(:params) { { yaml_errors: false } } + + it 'selects only pipelines does not have yaml_errors' do + 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 + end + end + end + + context 'when a order_by and sort are passed' do + context 'when order by started_at asc' do + let(:params) { { order_by: 'created_at', sort: 'asc' } } + + it 'sorts by started_at asc' do + expected_created_at = [ + tag_pipeline.created_at, + created_pipeline.created_at, + pending_pipeline.created_at, + running_pipeline.created_at, + success_pipeline.created_at, + failed_pipeline.created_at, + canceled_pipeline.created_at, + skipped_pipeline.created_at, + yaml_errors_pipeline.created_at].sort + expect(subject.map(&:created_at)).to eq expected_created_at + end + end end end end From fd302061f915f535b2dd419d5a76efb76ab534be Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 1 Mar 2017 15:58:06 +0900 Subject: [PATCH 05/57] Fix rubocop offences and rspec failures --- app/finders/pipelines_finder.rb | 4 +-- lib/api/pipelines.rb | 6 ++-- spec/finders/pipelines_finder_spec.rb | 45 +++++++++++++-------------- 3 files changed, 27 insertions(+), 28 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index ff16d7305ad..4a44a469a48 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -101,8 +101,8 @@ class PipelinesFinder def order_and_sort(items) if params[:order_by].present? && params[:sort].present? && - items.column_names.include?(params[:order_by]) && - (params[:sort].downcase == 'asc' || params[:sort].downcase == 'desc') + items.column_names.include?(params[:order_by]) && + (params[:sort].casecmp('ASC') || params[:sort].casecmp('DESC')) items.order("#{params[:order_by]} #{params[:sort]}") else items.order(id: :desc) diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index ecd6b64cfa7..70837d91c7a 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -16,14 +16,14 @@ module API use :pagination optional :scope, type: String, values: %w(running pending finished branches tags), desc: 'The scope of pipelines' - optional :status, type: String, values: ['running', 'pending', 'success', 'failed', 'canceled', 'skipped'], + optional :status, type: String, values: %w(running pending success failed canceled skipped), desc: 'The status of pipelines' optional :ref, type: String, desc: 'The ref of pipelines' optional :yaml_errors, type: Boolean, desc: 'If true, Returns only yaml error pipelines' optional :username, type: String, desc: 'The name of user who triggered pipelines' - optional :order_by, type: String, values: ['id', 'status', 'ref', 'username', 'started_at', 'finished_at', 'created_at', 'updated_at'], default: 'id', + optional :order_by, type: String, values: %w(id status ref username started_at finished_at created_at updated_at), default: 'id', desc: 'The order_by which is combined with a sort' - optional :sort, type: String, values: ['asc', 'desc'], default: 'desc', + optional :sort, type: String, values: %w(asc desc), default: 'desc', desc: 'The sort method which is combined with an order_by' end get ':id/pipelines' do diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 3bb828000d4..336901d0264 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -34,16 +34,15 @@ describe PipelinesFinder do end it 'orders in descending order on ID' do - expected_ids = [ - tag_pipeline.id, - 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 + expected_ids = [tag_pipeline.id, + 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 @@ -214,21 +213,21 @@ describe PipelinesFinder do end context 'when a order_by and sort are passed' do - context 'when order by started_at asc' do + context 'when order by created_at asc' do let(:params) { { order_by: 'created_at', sort: 'asc' } } - it 'sorts by started_at asc' do - expected_created_at = [ - tag_pipeline.created_at, - created_pipeline.created_at, - pending_pipeline.created_at, - running_pipeline.created_at, - success_pipeline.created_at, - failed_pipeline.created_at, - canceled_pipeline.created_at, - skipped_pipeline.created_at, - yaml_errors_pipeline.created_at].sort - expect(subject.map(&:created_at)).to eq expected_created_at + it 'sorts by created_at asc' do + expect(subject.first).to eq(tag_pipeline) + expect(subject.last).to eq(yaml_errors_pipeline) + end + end + + context 'when order by created_at desc' do + let(:params) { { order_by: 'created_at', sort: 'desc' } } + + it 'sorts by created_at desc' do + expect(subject.first).to eq(yaml_errors_pipeline) + expect(subject.last).to eq(tag_pipeline) end end end From d15c120f72a375b6c9634e9aaeaeb159bb0c1998 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 1 Mar 2017 18:07:39 +0900 Subject: [PATCH 06/57] Fix created_at test case because of sorting failure --- spec/finders/pipelines_finder_spec.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 336901d0264..ba2e85f95e3 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -4,14 +4,14 @@ describe PipelinesFinder do let(:user) { create(:user) } let(:project) { create(:project, :repository) } - let!(:tag_pipeline) { create(:ci_pipeline, project: project, user: user, ref: 'v1.0.0') } - let!(:created_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'created') } - let!(:pending_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'pending') } - let!(:running_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'running') } - let!(:success_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'success') } - let!(:failed_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'failed') } - let!(:canceled_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'canceled') } - let!(:skipped_pipeline) { create(:ci_pipeline, project: project, user: user, status: 'skipped') } + let!(:tag_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 10.minutes.ago, ref: 'v1.0.0') } + let!(:created_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 9.minutes.ago, status: 'created') } + let!(:pending_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 8.minutes.ago, status: 'pending') } + let!(:running_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 7.minutes.ago, status: 'running') } + let!(:success_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 6.minutes.ago, status: 'success') } + let!(:failed_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 5.minutes.ago, status: 'failed') } + 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!(:yaml_errors_pipeline) { create(:ci_pipeline, project: project, user: user, yaml_errors: 'Syntax error') } subject { described_class.new(project, params).execute } From a114c988b4c17e37508f1d0f96b93fd8b96d2df9 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 1 Mar 2017 21:43:25 +0900 Subject: [PATCH 07/57] Fixed SQL injection --- app/finders/pipelines_finder.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 4a44a469a48..5408cc09096 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -103,9 +103,9 @@ class PipelinesFinder if params[:order_by].present? && params[:sort].present? && items.column_names.include?(params[:order_by]) && (params[:sort].casecmp('ASC') || params[:sort].casecmp('DESC')) - items.order("#{params[:order_by]} #{params[:sort]}") + items.reorder(params[:order_by] => params[:sort]) else - items.order(id: :desc) + items.reorder(id: :desc) end end end From f0bc9af36b716066347549de4c0a5a3ec997a983 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 2 Mar 2017 17:43:37 +0900 Subject: [PATCH 08/57] Fixed the following. - Fix inappropriate evaluation(casecmp) to regex - Fix missed boolean conversion - Improve spec --- app/finders/pipelines_finder.rb | 7 ++-- spec/finders/pipelines_finder_spec.rb | 59 ++++++++++++++++++--------- 2 files changed, 43 insertions(+), 23 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 5408cc09096..03183efadda 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -88,8 +88,9 @@ class PipelinesFinder end def by_yaml_errors(items) - if params[:yaml_errors].present? - if params[:yaml_errors] + flg = Gitlab::Utils.to_boolean(params[:yaml_errors]) + if flg.present? + if flg items.where("yaml_errors IS NOT NULL") else items.where("yaml_errors IS NULL") @@ -102,7 +103,7 @@ class PipelinesFinder def order_and_sort(items) if params[:order_by].present? && params[:sort].present? && 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]) else items.reorder(id: :desc) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index ba2e85f95e3..42c5868fa91 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -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!(: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(: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 } @@ -21,29 +32,12 @@ describe PipelinesFinder do let(:params) { {} } it 'selects all pipelines' do - expect(subject.count).to be 9 - expect(subject).to include tag_pipeline - 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 + expect(subject.count).to be dummy_pipelines.count + expect(subject).to match_array(dummy_pipelines) end it 'orders in descending order on ID' do - expected_ids = [tag_pipeline.id, - 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 + expect(subject.map(&:id)).to eq dummy_pipelines.map(&:id).sort.reverse end end @@ -210,6 +204,15 @@ describe PipelinesFinder do expect(subject).to include skipped_pipeline 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 context 'when a order_by and sort are passed' do @@ -230,6 +233,22 @@ describe PipelinesFinder do expect(subject.last).to eq(tag_pipeline) 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 From e3fd8caf94c4d5ca0b6bc086d43effc2fb7c5792 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 2 Mar 2017 18:41:27 +0900 Subject: [PATCH 09/57] Improved CI. Fix yaml_errors boolean evaluation. --- app/finders/pipelines_finder.rb | 2 +- spec/finders/pipelines_finder_spec.rb | 93 +++++++++++---------------- 2 files changed, 39 insertions(+), 56 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 03183efadda..93f1ab9598f 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -89,7 +89,7 @@ class PipelinesFinder def by_yaml_errors(items) flg = Gitlab::Utils.to_boolean(params[:yaml_errors]) - if flg.present? + if !flg.nil? if flg items.where("yaml_errors IS NOT NULL") else diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 42c5868fa91..a1c89ba03db 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -1,28 +1,22 @@ require 'spec_helper' describe PipelinesFinder do - let(:user) { create(:user) } - let(:project) { create(:project, :repository) } + let!(:user1) { create(:user) } + let!(:user2) { create(:user) } + let!(:project) { create(:project, :repository) } - let!(:tag_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 10.minutes.ago, ref: 'v1.0.0') } - let!(:created_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 9.minutes.ago, status: 'created') } - let!(:pending_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 8.minutes.ago, status: 'pending') } - let!(:running_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 7.minutes.ago, status: 'running') } - let!(:success_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 6.minutes.ago, status: 'success') } - let!(:failed_pipeline) { create(:ci_pipeline, project: project, user: user, created_at: 5.minutes.ago, status: 'failed') } - 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!(: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] + let!(:dummy_pipelines) do + { + tag: create(:ci_pipeline, project: project, user: user1, created_at: 9.minutes.ago, ref: 'v1.0.0'), + created: create(:ci_pipeline, project: project, user: user1, created_at: 8.minutes.ago, status: 'created'), + pending: create(:ci_pipeline, project: project, user: user1, created_at: 7.minutes.ago, status: 'pending'), + running: create(:ci_pipeline, project: project, user: user1, created_at: 6.minutes.ago, status: 'running'), + success: create(:ci_pipeline, project: project, user: user1, created_at: 5.minutes.ago, status: 'success'), + failed: create(:ci_pipeline, project: project, user: user2, created_at: 4.minutes.ago, status: 'failed'), + canceled: create(:ci_pipeline, project: project, user: user2, created_at: 3.minutes.ago, status: 'canceled'), + skipped: create(:ci_pipeline, project: project, user: user2, created_at: 2.minutes.ago, status: 'skipped'), + yaml_errors: create(:ci_pipeline, project: project, user: user2, created_at: 1.minute.ago, yaml_errors: 'Syntax error'), + } end subject { described_class.new(project, params).execute } @@ -33,11 +27,11 @@ describe PipelinesFinder do it 'selects all pipelines' do expect(subject.count).to be dummy_pipelines.count - expect(subject).to match_array(dummy_pipelines) + expect(subject).to match_array dummy_pipelines.values end it 'orders in descending order on ID' do - expect(subject.map(&:id)).to eq dummy_pipelines.map(&:id).sort.reverse + expect(subject.map(&:id)).to eq dummy_pipelines.values.map(&:id).sort.reverse end end @@ -71,7 +65,7 @@ describe PipelinesFinder do it 'excludes tags' do expect(subject.count).to be 1 - expect(subject).not_to include tag_pipeline + expect(subject).not_to include dummy_pipelines[:tag] expect(subject.map(&:ref)).to include('master') end end @@ -81,7 +75,7 @@ describe PipelinesFinder do it 'excludes branches' do expect(subject.count).to be 1 - expect(subject).to include tag_pipeline + expect(subject).to include dummy_pipelines[:tag] expect(subject.map(&:ref)).not_to include('master') end end @@ -142,15 +136,8 @@ describe PipelinesFinder do let(:params) { { ref: 'master' } } it 'selects all pipelines which belong to the ref' do - expect(subject.count).to be 8 - 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 + expected = dummy_pipelines.except(:tag) + expect(subject).to match_array expected.values end end @@ -165,11 +152,11 @@ describe PipelinesFinder do context 'when a username is passed' do context 'when a username exists' do - let(:params) { { username: user.name } } + let(:params) { { username: user1.name } } it 'selects all pipelines which belong to the username' do - expect(subject).to include success_pipeline - expect(subject).to include failed_pipeline + expected = dummy_pipelines.except(:failed).except(:canceled).except(:skipped).except(:yaml_errors) + expect(subject).to match_array expected.values end end @@ -186,22 +173,19 @@ describe PipelinesFinder do context 'when yaml_errors is true' do let(:params) { { yaml_errors: true } } - it 'selects only pipelines has yaml_errors' do - expect(subject).to include yaml_errors_pipeline + it 'selects only pipelines have yaml_errors' do + expect(subject.count).to be 1 + expect(subject.first).to eq dummy_pipelines[:yaml_errors] end end context 'when yaml_errors is false' do let(:params) { { yaml_errors: false } } - it 'selects only pipelines does not have yaml_errors' do - 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 + it 'selects only pipelines do not have yaml_errors' do + expected = dummy_pipelines.except(:yaml_errors) + expect(subject.count).to be expected.count + expect(subject).to match_array expected.values end end @@ -209,8 +193,7 @@ describe PipelinesFinder 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) + expect(subject).to match_array dummy_pipelines.values end end end @@ -220,8 +203,8 @@ describe PipelinesFinder do let(:params) { { order_by: 'created_at', sort: 'asc' } } it 'sorts by created_at asc' do - expect(subject.first).to eq(tag_pipeline) - expect(subject.last).to eq(yaml_errors_pipeline) + expect(subject.first).to eq dummy_pipelines[:tag] + expect(subject.last).to eq dummy_pipelines[:yaml_errors] end end @@ -229,8 +212,8 @@ describe PipelinesFinder do let(:params) { { order_by: 'created_at', sort: 'desc' } } it 'sorts by created_at desc' do - expect(subject.first).to eq(yaml_errors_pipeline) - expect(subject.last).to eq(tag_pipeline) + expect(subject.first).to eq dummy_pipelines[:yaml_errors] + expect(subject.last).to eq dummy_pipelines[:tag] end end @@ -238,7 +221,7 @@ describe PipelinesFinder 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 + expect(subject.map(&:id)).to eq dummy_pipelines.values.map(&:id).sort.reverse end end @@ -246,7 +229,7 @@ describe PipelinesFinder 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 + expect(subject.map(&:id)).to eq dummy_pipelines.values.map(&:id).sort.reverse end end end From fa64da65e7205b101569a3e515a0a65ae2d679c9 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 21:35:15 +0900 Subject: [PATCH 10/57] Use 'case/when/end' --- app/finders/pipelines_finder.rb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 93f1ab9598f..46588cf3c16 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -88,13 +88,11 @@ class PipelinesFinder end def by_yaml_errors(items) - flg = Gitlab::Utils.to_boolean(params[:yaml_errors]) - if !flg.nil? - if flg - items.where("yaml_errors IS NOT NULL") - else - items.where("yaml_errors IS NULL") - end + case Gitlab::Utils.to_boolean(params[:yaml_errors]) + when true + items.where("yaml_errors IS NOT NULL") + when false + items.where("yaml_errors IS NULL") else items end From 7e421e55233bbb26594de2a88e62854b468fb02a Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 21:36:08 +0900 Subject: [PATCH 11/57] Fix inappropriate words in doc --- doc/api/pipelines.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/pipelines.md b/doc/api/pipelines.md index 677b96c74cc..48e0d10180b 100644 --- a/doc/api/pipelines.md +++ b/doc/api/pipelines.md @@ -14,10 +14,10 @@ GET /projects/:id/pipelines | `scope` | string | no | The scope of pipelines, one of: `running`, `pending`, `finished`, `branches`, `tags`; | | `status` | string | no | The status of pipelines, one of: `running`, `pending`, `success`, `failed`, `canceled`, `skipped`; | | `ref` | string | no | The ref of pipelines | -| `yaml_errors`| string | no | If true, Returns only yaml error pipelines | +| `yaml_errors`| string | no | If true, returns only yaml error pipelines | | `username`| string | no | The name of user who triggered pipelines | -| `order_by`| string | no | The order_by which is combined with a `sort`, one of: `id`, `status`, `ref`, `username`, `started_at`, `finished_at`, `created_at`, `updated_at`; | -| `sort` | string | no | The sort method which is combined with an `order_by`, one of: `asc`, `desc`; | +| `order_by`| string | no | Return requests ordered by `id`, `status`, `ref`, `username`, `started_at`, `finished_at`, `created_at` or `updated_at` fields. Default is `id` | +| `sort` | string | no | Return requests sorted in `asc` or `desc` order. Default is `desc` | ``` curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects/1/pipelines" From af7cb5b93191453de4ff7561089bb0b0146c688f Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 21:36:45 +0900 Subject: [PATCH 12/57] %w() to %[] --- lib/api/pipelines.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 70837d91c7a..fcec7e7d3bb 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -14,16 +14,16 @@ module API end params do use :pagination - optional :scope, type: String, values: %w(running pending finished branches tags), + optional :scope, type: String, values: %[running pending finished branches tags], desc: 'The scope of pipelines' - optional :status, type: String, values: %w(running pending success failed canceled skipped), + optional :status, type: String, values: %[running pending success failed canceled skipped], desc: 'The status of pipelines' optional :ref, type: String, desc: 'The ref of pipelines' - optional :yaml_errors, type: Boolean, desc: 'If true, Returns only yaml error pipelines' + optional :yaml_errors, type: Boolean, desc: 'If true, returns only yaml error pipelines' optional :username, type: String, desc: 'The name of user who triggered pipelines' - optional :order_by, type: String, values: %w(id status ref username started_at finished_at created_at updated_at), default: 'id', + optional :order_by, type: String, values: %[id status ref username started_at finished_at created_at updated_at], default: 'id', desc: 'The order_by which is combined with a sort' - optional :sort, type: String, values: %w(asc desc), default: 'desc', + optional :sort, type: String, values: %[asc desc], default: 'desc', desc: 'The sort method which is combined with an order_by' end get ':id/pipelines' do From ac00cd1021cd4e2d6b7a11683b0af61065918a1c Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 21:37:14 +0900 Subject: [PATCH 13/57] %w() to %[] --- spec/finders/pipelines_finder_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index a1c89ba03db..395eb2f9701 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -56,7 +56,7 @@ describe PipelinesFinder do let(:params) { { scope: 'finished' } } it 'has only finished status' do - expect(subject.map(&:status)).to match_array %w(success canceled failed) + expect(subject.map(&:status)).to match_array %[success canceled failed] end end From d33e762167a197f30d075ed409ca1a07ae9f83c6 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 22:02:29 +0900 Subject: [PATCH 14/57] %[] to %w[] --- lib/api/pipelines.rb | 8 ++++---- spec/finders/pipelines_finder_spec.rb | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index fcec7e7d3bb..bcb4fdcfe47 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -14,16 +14,16 @@ module API end params do use :pagination - optional :scope, type: String, values: %[running pending finished branches tags], + optional :scope, type: String, values: %w[running pending finished branches tags], desc: 'The scope of pipelines' - optional :status, type: String, values: %[running pending success failed canceled skipped], + optional :status, type: String, values: %w[running pending success failed canceled skipped], desc: 'The status of pipelines' optional :ref, type: String, desc: 'The ref of pipelines' optional :yaml_errors, type: Boolean, desc: 'If true, returns only yaml error pipelines' optional :username, type: String, desc: 'The name of user who triggered pipelines' - optional :order_by, type: String, values: %[id status ref username started_at finished_at created_at updated_at], default: 'id', + optional :order_by, type: String, values: %w[id status ref username started_at finished_at created_at updated_at], default: 'id', desc: 'The order_by which is combined with a sort' - optional :sort, type: String, values: %[asc desc], default: 'desc', + optional :sort, type: String, values: %w[asc desc], default: 'desc', desc: 'The sort method which is combined with an order_by' end get ':id/pipelines' do diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 395eb2f9701..a16d3bb8d25 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -56,7 +56,7 @@ describe PipelinesFinder do let(:params) { { scope: 'finished' } } it 'has only finished status' do - expect(subject.map(&:status)).to match_array %[success canceled failed] + expect(subject.map(&:status)).to match_array %w[success canceled failed] end end From 40e8940195a05bf2e3802550a571f889853c0200 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 22:19:15 +0900 Subject: [PATCH 15/57] include to all(eq()). Test status, not scope. --- spec/finders/pipelines_finder_spec.rb | 32 +++++++++++++-------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index a16d3bb8d25..444ce7e1267 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -40,7 +40,7 @@ describe PipelinesFinder do let(:params) { { scope: 'running' } } it 'has only running status' do - expect(subject.map(&:status)).to include('running') + expect(subject.map(&:status)).to all(eq('running')) end end @@ -48,7 +48,7 @@ describe PipelinesFinder do let(:params) { { scope: 'pending' } } it 'has only pending status' do - expect(subject.map(&:status)).to include('pending') + expect(subject.map(&:status)).to all(eq('pending')) end end @@ -66,7 +66,7 @@ describe PipelinesFinder do it 'excludes tags' do expect(subject.count).to be 1 expect(subject).not_to include dummy_pipelines[:tag] - expect(subject.map(&:ref)).to include('master') + expect(subject.map(&:ref)).to all(eq('master')) end end @@ -76,57 +76,57 @@ describe PipelinesFinder do it 'excludes branches' do expect(subject.count).to be 1 expect(subject).to include dummy_pipelines[:tag] - expect(subject.map(&:ref)).not_to include('master') + expect(subject.map(&:ref)).not_to all(eq('master')) end end end context 'when a status is passed' do context 'when selecting running' do - let(:params) { { scope: 'running' } } + let(:params) { { status: 'running' } } it 'has only running status' do - expect(subject.map(&:status)).to include('running') + expect(subject.map(&:status)).to all(eq('running')) end end context 'when selecting pending' do - let(:params) { { scope: 'pending' } } + let(:params) { { status: 'pending' } } it 'has only pending status' do - expect(subject.map(&:status)).to include('pending') + expect(subject.map(&:status)).to all(eq('pending')) end end context 'when selecting success' do - let(:params) { { scope: 'success' } } + let(:params) { { status: 'success' } } it 'has only success status' do - expect(subject.map(&:status)).to include('success') + expect(subject.map(&:status)).to all(eq('success')) end end context 'when selecting failed' do - let(:params) { { scope: 'failed' } } + let(:params) { { status: 'failed' } } it 'has only failed status' do - expect(subject.map(&:status)).to include('failed') + expect(subject.map(&:status)).to all(eq('failed')) end end context 'when selecting canceled' do - let(:params) { { scope: 'canceled' } } + let(:params) { { status: 'canceled' } } it 'has only canceled status' do - expect(subject.map(&:status)).to include('canceled') + expect(subject.map(&:status)).to all(eq('canceled')) end end context 'when selecting skipped' do - let(:params) { { scope: 'skipped' } } + let(:params) { { status: 'skipped' } } it 'has only skipped status' do - expect(subject.map(&:status)).to include('skipped') + expect(subject.map(&:status)).to all(eq('skipped')) end end end From 8d85887c3879ec162c8658b37f894a65edc27104 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 23:27:49 +0900 Subject: [PATCH 16/57] Check if any master exists --- spec/finders/pipelines_finder_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 444ce7e1267..418f95bc115 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -76,7 +76,7 @@ describe PipelinesFinder do it 'excludes branches' do expect(subject.count).to be 1 expect(subject).to include dummy_pipelines[:tag] - expect(subject.map(&:ref)).not_to all(eq('master')) + expect(subject.map(&:ref)).not_to include('master') end end end From 658f2cb148b63b284fe9ce346cb149c9e6986b9b Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 23:31:32 +0900 Subject: [PATCH 17/57] Add tag true --- spec/finders/pipelines_finder_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 418f95bc115..6c28dee365f 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -7,7 +7,7 @@ describe PipelinesFinder do let!(:dummy_pipelines) do { - tag: create(:ci_pipeline, project: project, user: user1, created_at: 9.minutes.ago, ref: 'v1.0.0'), + tag: create(:ci_pipeline, project: project, user: user1, created_at: 9.minutes.ago, ref: 'v1.0.0', tag: true), created: create(:ci_pipeline, project: project, user: user1, created_at: 8.minutes.ago, status: 'created'), pending: create(:ci_pipeline, project: project, user: user1, created_at: 7.minutes.ago, status: 'pending'), running: create(:ci_pipeline, project: project, user: user1, created_at: 6.minutes.ago, status: 'running'), From 44ae99399f75f0b23e9d78eb4217fad4911ccbca Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 02:44:38 +0900 Subject: [PATCH 18/57] Use ActiveRecord for comparison --- spec/finders/pipelines_finder_spec.rb | 84 +++++++++++---------------- 1 file changed, 35 insertions(+), 49 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 6c28dee365f..aa8ad66cdf4 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -1,22 +1,20 @@ require 'spec_helper' describe PipelinesFinder do - let!(:user1) { create(:user) } - let!(:user2) { create(:user) } - let!(:project) { create(:project, :repository) } + let(:user1) { create(:user) } + let(:user2) { create(:user) } + let(:project) { create(:project, :repository) } - let!(:dummy_pipelines) do - { - tag: create(:ci_pipeline, project: project, user: user1, created_at: 9.minutes.ago, ref: 'v1.0.0', tag: true), - created: create(:ci_pipeline, project: project, user: user1, created_at: 8.minutes.ago, status: 'created'), - pending: create(:ci_pipeline, project: project, user: user1, created_at: 7.minutes.ago, status: 'pending'), - running: create(:ci_pipeline, project: project, user: user1, created_at: 6.minutes.ago, status: 'running'), - success: create(:ci_pipeline, project: project, user: user1, created_at: 5.minutes.ago, status: 'success'), - failed: create(:ci_pipeline, project: project, user: user2, created_at: 4.minutes.ago, status: 'failed'), - canceled: create(:ci_pipeline, project: project, user: user2, created_at: 3.minutes.ago, status: 'canceled'), - skipped: create(:ci_pipeline, project: project, user: user2, created_at: 2.minutes.ago, status: 'skipped'), - yaml_errors: create(:ci_pipeline, project: project, user: user2, created_at: 1.minute.ago, yaml_errors: 'Syntax error'), - } + before do + create(:ci_pipeline, project: project, user: user1, ref: 'v1.0.0', tag: true) + create(:ci_pipeline, project: project, user: user1, status: 'created') + create(:ci_pipeline, project: project, user: user1, status: 'pending') + create(:ci_pipeline, project: project, user: user1, status: 'running') + create(:ci_pipeline, project: project, user: user1, status: 'success') + create(:ci_pipeline, project: project, user: user2, status: 'failed') + create(:ci_pipeline, project: project, user: user2, status: 'canceled') + create(:ci_pipeline, project: project, user: user2, status: 'skipped') + create(:ci_pipeline, project: project, user: user2, yaml_errors: 'Syntax error') end subject { described_class.new(project, params).execute } @@ -26,12 +24,11 @@ describe PipelinesFinder do let(:params) { {} } it 'selects all pipelines' do - expect(subject.count).to be dummy_pipelines.count - expect(subject).to match_array dummy_pipelines.values + expect(subject).to match_array(Ci::Pipeline.all) end it 'orders in descending order on ID' do - expect(subject.map(&:id)).to eq dummy_pipelines.values.map(&:id).sort.reverse + expect(subject).to match_array(Ci::Pipeline.order(id: :desc)) end end @@ -40,7 +37,7 @@ describe PipelinesFinder do let(:params) { { scope: 'running' } } it 'has only running status' do - expect(subject.map(&:status)).to all(eq('running')) + expect(subject).to match_array(Ci::Pipeline.running) end end @@ -48,7 +45,7 @@ describe PipelinesFinder do let(:params) { { scope: 'pending' } } it 'has only pending status' do - expect(subject.map(&:status)).to all(eq('pending')) + expect(subject).to match_array(Ci::Pipeline.pending) end end @@ -56,7 +53,7 @@ describe PipelinesFinder do let(:params) { { scope: 'finished' } } it 'has only finished status' do - expect(subject.map(&:status)).to match_array %w[success canceled failed] + expect(subject).to match_array(Ci::Pipeline.finished) end end @@ -64,9 +61,7 @@ describe PipelinesFinder do let(:params) { { scope: 'branches' } } it 'excludes tags' do - expect(subject.count).to be 1 - expect(subject).not_to include dummy_pipelines[:tag] - expect(subject.map(&:ref)).to all(eq('master')) + expect(subject).to eq([Ci::Pipeline.where(tag: false).last]) end end @@ -74,9 +69,7 @@ describe PipelinesFinder do let(:params) { { scope: 'tags' } } it 'excludes branches' do - expect(subject.count).to be 1 - expect(subject).to include dummy_pipelines[:tag] - expect(subject.map(&:ref)).not_to include('master') + expect(subject).to eq([Ci::Pipeline.where(tag: true).last]) end end end @@ -86,7 +79,7 @@ describe PipelinesFinder do let(:params) { { status: 'running' } } it 'has only running status' do - expect(subject.map(&:status)).to all(eq('running')) + expect(subject).to match_array(Ci::Pipeline.running) end end @@ -94,7 +87,7 @@ describe PipelinesFinder do let(:params) { { status: 'pending' } } it 'has only pending status' do - expect(subject.map(&:status)).to all(eq('pending')) + expect(subject).to match_array(Ci::Pipeline.pending) end end @@ -102,7 +95,7 @@ describe PipelinesFinder do let(:params) { { status: 'success' } } it 'has only success status' do - expect(subject.map(&:status)).to all(eq('success')) + expect(subject).to match_array(Ci::Pipeline.success) end end @@ -110,7 +103,7 @@ describe PipelinesFinder do let(:params) { { status: 'failed' } } it 'has only failed status' do - expect(subject.map(&:status)).to all(eq('failed')) + expect(subject).to match_array(Ci::Pipeline.failed) end end @@ -118,7 +111,7 @@ describe PipelinesFinder do let(:params) { { status: 'canceled' } } it 'has only canceled status' do - expect(subject.map(&:status)).to all(eq('canceled')) + expect(subject).to match_array(Ci::Pipeline.canceled) end end @@ -126,7 +119,7 @@ describe PipelinesFinder do let(:params) { { status: 'skipped' } } it 'has only skipped status' do - expect(subject.map(&:status)).to all(eq('skipped')) + expect(subject).to match_array(Ci::Pipeline.skipped) end end end @@ -136,8 +129,7 @@ describe PipelinesFinder do let(:params) { { ref: 'master' } } it 'selects all pipelines which belong to the ref' do - expected = dummy_pipelines.except(:tag) - expect(subject).to match_array expected.values + expect(subject).to match_array(Ci::Pipeline.where(ref: 'master')) end end @@ -155,8 +147,7 @@ describe PipelinesFinder do let(:params) { { username: user1.name } } it 'selects all pipelines which belong to the username' do - expected = dummy_pipelines.except(:failed).except(:canceled).except(:skipped).except(:yaml_errors) - expect(subject).to match_array expected.values + expect(subject).to match_array(Ci::Pipeline.where(user: user1)) end end @@ -174,8 +165,7 @@ describe PipelinesFinder do let(:params) { { yaml_errors: true } } it 'selects only pipelines have yaml_errors' do - expect(subject.count).to be 1 - expect(subject.first).to eq dummy_pipelines[:yaml_errors] + expect(subject).to match_array(Ci::Pipeline.where("yaml_errors IS NOT NULL")) end end @@ -183,9 +173,7 @@ describe PipelinesFinder do let(:params) { { yaml_errors: false } } it 'selects only pipelines do not have yaml_errors' do - expected = dummy_pipelines.except(:yaml_errors) - expect(subject.count).to be expected.count - expect(subject).to match_array expected.values + expect(subject).to match_array(Ci::Pipeline.where("yaml_errors IS NULL")) end end @@ -193,7 +181,7 @@ describe PipelinesFinder do let(:params) { { yaml_errors: "UnexpectedValue" } } it 'selects all pipelines' do - expect(subject).to match_array dummy_pipelines.values + expect(subject).to match_array(Ci::Pipeline.all) end end end @@ -203,8 +191,7 @@ describe PipelinesFinder do let(:params) { { order_by: 'created_at', sort: 'asc' } } it 'sorts by created_at asc' do - expect(subject.first).to eq dummy_pipelines[:tag] - expect(subject.last).to eq dummy_pipelines[:yaml_errors] + expect(subject).to match_array(Ci::Pipeline.order(created_at: :asc)) end end @@ -212,8 +199,7 @@ describe PipelinesFinder do let(:params) { { order_by: 'created_at', sort: 'desc' } } it 'sorts by created_at desc' do - expect(subject.first).to eq dummy_pipelines[:yaml_errors] - expect(subject.last).to eq dummy_pipelines[:tag] + expect(subject).to match_array(Ci::Pipeline.order(created_at: :desc)) end end @@ -221,7 +207,7 @@ describe PipelinesFinder do let(:params) { { order_by: 'abnormal_column', sort: 'desc' } } it 'sorts by default' do - expect(subject.map(&:id)).to eq dummy_pipelines.values.map(&:id).sort.reverse + expect(subject).to match_array(Ci::Pipeline.order(id: :desc)) end end @@ -229,7 +215,7 @@ describe PipelinesFinder do let(:params) { { order_by: 'created_at', sort: 'abnormal_sort' } } it 'sorts by default' do - expect(subject.map(&:id)).to eq dummy_pipelines.values.map(&:id).sort.reverse + expect(subject).to match_array(Ci::Pipeline.order(id: :desc)) end end end From 56f50cbb3a63ae914f50eda3756b49d5cf516207 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 17:24:20 +0900 Subject: [PATCH 19/57] Fix how to use PipelinesFinder --- lib/api/pipelines.rb | 2 +- lib/api/v3/pipelines.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index bcb4fdcfe47..986dd607e23 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -29,7 +29,7 @@ module API get ':id/pipelines' do authorize! :read_pipeline, user_project - pipelines = PipelinesFinder.new(user_project, params).execute(scope: params[:scope]) + pipelines = PipelinesFinder.new(user_project, params).execute present paginate(pipelines), with: Entities::PipelineBasic end diff --git a/lib/api/v3/pipelines.rb b/lib/api/v3/pipelines.rb index 82827249244..c48cbd2b765 100644 --- a/lib/api/v3/pipelines.rb +++ b/lib/api/v3/pipelines.rb @@ -21,7 +21,7 @@ module API get ':id/pipelines' do authorize! :read_pipeline, user_project - pipelines = PipelinesFinder.new(user_project).execute(scope: params[:scope]) + pipelines = PipelinesFinder.new(user_project, scope: params[:scope]).execute present paginate(pipelines), with: ::API::Entities::Pipeline end end From a46c91f43225931f94a7d3c7f47beef82152f0ce Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 17:33:10 +0900 Subject: [PATCH 20/57] Fix inappropriate regex --- app/finders/pipelines_finder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 46588cf3c16..aa0210fa7f2 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -101,7 +101,7 @@ class PipelinesFinder def order_and_sort(items) if params[:order_by].present? && params[:sort].present? && items.column_names.include?(params[:order_by]) && - params[:sort] =~ /\A(ASC|DESC)\Z/i + params[:sort] =~ /\A(ASC|DESC)\z/i items.reorder(params[:order_by] => params[:sort]) else items.reorder(id: :desc) From 83d02a0b609ea71ef8448b9012221962dda69aba Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 18:19:11 +0900 Subject: [PATCH 21/57] Change name to username --- app/finders/pipelines_finder.rb | 2 +- spec/finders/pipelines_finder_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index aa0210fa7f2..c01a1a73666 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -81,7 +81,7 @@ class PipelinesFinder def by_username(items) if params[:username].present? - items.joins(:user).where("users.name = ?", params[:username]) + items.joins(:user).where("users.username = ?", params[:username]) else items end diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index aa8ad66cdf4..3a840eca44f 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -144,7 +144,7 @@ describe PipelinesFinder do context 'when a username is passed' do context 'when a username exists' do - let(:params) { { username: user1.name } } + let(:params) { { username: user1.username } } it 'selects all pipelines which belong to the username' do expect(subject).to match_array(Ci::Pipeline.where(user: user1)) From 175800299bf497591e625e82fd71420644c0bc6b Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 20:24:00 +0900 Subject: [PATCH 22/57] Add name(User) --- app/finders/pipelines_finder.rb | 8 +++++++ doc/api/pipelines.md | 3 ++- lib/api/pipelines.rb | 3 ++- spec/finders/pipelines_finder_spec.rb | 30 +++++++++++++++++++++------ 4 files changed, 36 insertions(+), 8 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index c01a1a73666..10c8f5e0b2e 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -79,6 +79,14 @@ class PipelinesFinder end end + def by_name(items) + if params[:name].present? + items.joins(:user).where("users.name = ?", params[:name]) + else + items + end + end + def by_username(items) if params[:username].present? items.joins(:user).where("users.username = ?", params[:username]) diff --git a/doc/api/pipelines.md b/doc/api/pipelines.md index 48e0d10180b..b843d64e000 100644 --- a/doc/api/pipelines.md +++ b/doc/api/pipelines.md @@ -15,7 +15,8 @@ GET /projects/:id/pipelines | `status` | string | no | The status of pipelines, one of: `running`, `pending`, `success`, `failed`, `canceled`, `skipped`; | | `ref` | string | no | The ref of pipelines | | `yaml_errors`| string | no | If true, returns only yaml error pipelines | -| `username`| string | no | The name of user who triggered pipelines | +| `name`| string | no | The name of user who triggered pipelines | +| `username`| string | no | The username of user who triggered pipelines | | `order_by`| string | no | Return requests ordered by `id`, `status`, `ref`, `username`, `started_at`, `finished_at`, `created_at` or `updated_at` fields. Default is `id` | | `sort` | string | no | Return requests sorted in `asc` or `desc` order. Default is `desc` | diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 986dd607e23..79eea7e2e28 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -20,7 +20,8 @@ module API desc: 'The status of pipelines' optional :ref, type: String, desc: 'The ref of pipelines' optional :yaml_errors, type: Boolean, desc: 'If true, returns only yaml error pipelines' - optional :username, type: String, desc: 'The name of user who triggered pipelines' + optional :name, type: String, desc: 'The name of user who triggered pipelines' + optional :username, type: String, desc: 'The username of user who triggered pipelines' optional :order_by, type: String, values: %w[id status ref username started_at finished_at created_at updated_at], default: 'id', desc: 'The order_by which is combined with a sort' optional :sort, type: String, values: %w[asc desc], default: 'desc', diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 3a840eca44f..8e214a71a32 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -134,13 +134,31 @@ describe PipelinesFinder do end context 'when a ref does not exist' do - let(:params) { { ref: 'unique-ref' } } + let(:params) { { ref: 'invalid-ref' } } it 'selects nothing' do expect(subject).to be_empty end end - end + end + + context 'when a name is passed' do + context 'when a name exists' do + let(:params) { { name: user1.name } } + + it 'selects all pipelines which belong to the name' do + expect(subject).to match_array(Ci::Pipeline.where(user: user1)) + end + end + + context 'when a name does not exist' do + let(:params) { { name: 'invalid-name' } } + + it 'selects nothing' do + expect(subject).to be_empty + end + end + end context 'when a username is passed' do context 'when a username exists' do @@ -152,13 +170,13 @@ describe PipelinesFinder do end context 'when a username does not exist' do - let(:params) { { username: 'unique-username' } } + let(:params) { { username: 'invalid-username' } } it 'selects nothing' do expect(subject).to be_empty end end - end + end context 'when a yaml_errors is passed' do context 'when yaml_errors is true' do @@ -204,7 +222,7 @@ describe PipelinesFinder do end context 'when order_by does not exist' do - let(:params) { { order_by: 'abnormal_column', sort: 'desc' } } + let(:params) { { order_by: 'invalid_column', sort: 'desc' } } it 'sorts by default' do expect(subject).to match_array(Ci::Pipeline.order(id: :desc)) @@ -212,7 +230,7 @@ describe PipelinesFinder do end context 'when sort does not exist' do - let(:params) { { order_by: 'created_at', sort: 'abnormal_sort' } } + let(:params) { { order_by: 'created_at', sort: 'invalid_sort' } } it 'sorts by default' do expect(subject).to match_array(Ci::Pipeline.order(id: :desc)) From 284cf0bfb5d56dd847e48b74b6ae6c2627e164d1 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 21:05:33 +0900 Subject: [PATCH 23/57] Add name. Improve order_and_sort. --- app/finders/pipelines_finder.rb | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 10c8f5e0b2e..f6532377374 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -12,6 +12,7 @@ class PipelinesFinder items = by_scope(items) items = by_status(items) items = by_ref(items) + items = by_name(items) items = by_username(items) items = by_yaml_errors(items) order_and_sort(items) @@ -107,12 +108,16 @@ class PipelinesFinder end def order_and_sort(items) - if params[:order_by].present? && params[:sort].present? && - items.column_names.include?(params[:order_by]) && - params[:sort] =~ /\A(ASC|DESC)\z/i - items.reorder(params[:order_by] => params[:sort]) - else - items.reorder(id: :desc) - end + order_by = if params[:order_by].present? && items.column_names.include?(params[:order_by]) + params[:order_by] + else + :id + end + sort = if params[:sort].present? && params[:sort] =~ /\A(ASC|DESC)\z/i + params[:sort] + else + :desc + end + items.reorder(order_by => sort) end end From 9324a464ef456dff3670df227c10d5cdec473256 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 21:18:09 +0900 Subject: [PATCH 24/57] Reduce playable columns for sorting --- doc/api/pipelines.md | 2 +- lib/api/pipelines.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/pipelines.md b/doc/api/pipelines.md index b843d64e000..c2aca779710 100644 --- a/doc/api/pipelines.md +++ b/doc/api/pipelines.md @@ -17,7 +17,7 @@ GET /projects/:id/pipelines | `yaml_errors`| string | no | If true, returns only yaml error pipelines | | `name`| string | no | The name of user who triggered pipelines | | `username`| string | no | The username of user who triggered pipelines | -| `order_by`| string | no | Return requests ordered by `id`, `status`, `ref`, `username`, `started_at`, `finished_at`, `created_at` or `updated_at` fields. Default is `id` | +| `order_by`| string | no | Return requests ordered by `id`, `status`, `ref`, `sha`, or `user_id` fields. Default is `id` | | `sort` | string | no | Return requests sorted in `asc` or `desc` order. Default is `desc` | ``` diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 79eea7e2e28..7d91900212a 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -22,7 +22,7 @@ module API optional :yaml_errors, type: Boolean, desc: 'If true, returns only yaml error pipelines' optional :name, type: String, desc: 'The name of user who triggered pipelines' optional :username, type: String, desc: 'The username of user who triggered pipelines' - optional :order_by, type: String, values: %w[id status ref username started_at finished_at created_at updated_at], default: 'id', + optional :order_by, type: String, values: %w[id status ref sha user_id], default: 'id', desc: 'The order_by which is combined with a sort' optional :sort, type: String, values: %w[asc desc], default: 'desc', desc: 'The sort method which is combined with an order_by' From 171229d4b1501780b2a5e7a2eab9c714addcadea Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 9 Mar 2017 02:20:00 +0900 Subject: [PATCH 25/57] No need 'a' --- spec/finders/pipelines_finder_spec.rb | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 8e214a71a32..a2f33b7baa0 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -32,7 +32,7 @@ describe PipelinesFinder do end end - context 'when a scope is passed' do + context 'when scope is passed' do context 'when selecting running' do let(:params) { { scope: 'running' } } @@ -74,7 +74,7 @@ describe PipelinesFinder do end end - context 'when a status is passed' do + context 'when status is passed' do context 'when selecting running' do let(:params) { { status: 'running' } } @@ -124,8 +124,8 @@ describe PipelinesFinder do end end - context 'when a ref is passed' do - context 'when a ref exists' do + context 'when ref is passed' do + context 'when ref exists' do let(:params) { { ref: 'master' } } it 'selects all pipelines which belong to the ref' do @@ -133,7 +133,7 @@ describe PipelinesFinder do end end - context 'when a ref does not exist' do + context 'when ref does not exist' do let(:params) { { ref: 'invalid-ref' } } it 'selects nothing' do @@ -142,8 +142,8 @@ describe PipelinesFinder do end end - context 'when a name is passed' do - context 'when a name exists' do + context 'when name is passed' do + context 'when name exists' do let(:params) { { name: user1.name } } it 'selects all pipelines which belong to the name' do @@ -151,7 +151,7 @@ describe PipelinesFinder do end end - context 'when a name does not exist' do + context 'when name does not exist' do let(:params) { { name: 'invalid-name' } } it 'selects nothing' do @@ -160,8 +160,8 @@ describe PipelinesFinder do end end - context 'when a username is passed' do - context 'when a username exists' do + context 'when username is passed' do + context 'when username exists' do let(:params) { { username: user1.username } } it 'selects all pipelines which belong to the username' do @@ -169,7 +169,7 @@ describe PipelinesFinder do end end - context 'when a username does not exist' do + context 'when username does not exist' do let(:params) { { username: 'invalid-username' } } it 'selects nothing' do @@ -178,7 +178,7 @@ describe PipelinesFinder do end end - context 'when a yaml_errors is passed' do + context 'when yaml_errors is passed' do context 'when yaml_errors is true' do let(:params) { { yaml_errors: true } } @@ -204,7 +204,7 @@ describe PipelinesFinder do end end - context 'when a order_by and sort are passed' do + context 'when order_by and sort are passed' do context 'when order by created_at asc' do let(:params) { { order_by: 'created_at', sort: 'asc' } } From 959b4e99c0a81ad7b974ab3871f4b1857da9bfc4 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 9 Mar 2017 02:20:50 +0900 Subject: [PATCH 26/57] Add spec for Pipeline API (Halfway) --- spec/requests/api/pipelines_spec.rb | 145 ++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+) diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index 762345cd41c..e5f1765490b 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -24,6 +24,151 @@ describe API::Pipelines do expect(json_response.first['id']).to eq pipeline.id expect(json_response.first.keys).to contain_exactly(*%w[id sha ref status]) end + + context 'when parameter is passed' do + let(:user1) { create(:user) } + let(:user2) { create(:user) } + let(:project) { create(:project, :repository) } + + before do + create(:ci_pipeline, project: project, user: user1, ref: 'v1.0.0', tag: true) + create(:ci_pipeline, project: project, user: user1, status: 'created') + create(:ci_pipeline, project: project, user: user1, status: 'pending') + create(:ci_pipeline, project: project, user: user1, status: 'running') + create(:ci_pipeline, project: project, user: user1, status: 'success') + create(:ci_pipeline, project: project, user: user2, status: 'failed') + create(:ci_pipeline, project: project, user: user2, status: 'canceled') + create(:ci_pipeline, project: project, user: user2, status: 'skipped') + create(:ci_pipeline, project: project, user: user2, yaml_errors: 'Syntax error') + end + + context 'when scope is passed' do + %w[running pending finished branches tags].each do |target| + it "returns only scope=#{target} pipelines" do + get api("/projects/#{project.id}/pipelines?scope=#{target}", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.count).to be > 0 + if target == 'running' || target == 'pending' + json_response.each { |r| expect(r['status']).to eq(target) } + elsif target == 'finished' + json_response.each { |r| expect(r['status']).to be_in(%w[success failed canceled]) } + elsif target == 'branches' + expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: false).last.sha) + elsif target == 'tags' + expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: true).last.sha) + end + end + end + end + + context 'when status is passed' do + %w[running pending success failed canceled skipped].each do |target| + it "returns only status=#{target} pipelines" do + get api("/projects/#{project.id}/pipelines?status=#{target}", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.count).to be > 0 + json_response.each { |r| expect(r['status']).to eq(target) } + end + end + end + + context 'when ref is passed' do + %w[master invalid-ref].each do |target| + it "returns only ref=#{target} pipelines" do + get api("/projects/#{project.id}/pipelines?ref=#{target}", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + if target == 'master' + expect(json_response.count).to be > 0 + json_response.each { |r| expect(r['ref']).to eq(target) } + else + expect(json_response.count).to eq(0) + end + end + end + end + + context 'when name is passed' do + context 'when name exists' do + it "returns only pipelines related to the name" do + get api("/projects/#{project.id}/pipelines?name=#{user1.name}", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.first['sha']).to eq(Ci::Pipeline.where(user: user1).order(id: :desc).first.sha) + end + end + + context 'when name does not exist' do + it "returns nothing" do + get api("/projects/#{project.id}/pipelines?name=invalid-name", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.count).to eq(0) + end + end + end + + context 'when username is passed' do + context 'when username exists' do + it "returns only pipelines related to the username" do + get api("/projects/#{project.id}/pipelines?username=#{user1.username}", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.first['sha']).to eq(Ci::Pipeline.where(user: user1).order(id: :desc).first.sha) + end + end + + context 'when username does not exist' do + it "returns nothing" do + get api("/projects/#{project.id}/pipelines?username=invalid-username", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.count).to eq(0) + end + end + end + + context 'when yaml_errors is passed' do + context 'when yaml_errors is true' do + it "returns only pipelines related to the yaml_errors" do + get api("/projects/#{project.id}/pipelines?yaml_errors=true", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.first['id']).to eq(Ci::Pipeline.where("yaml_errors IS NOT NULL").order(id: :desc).first.id) + end + end + + context 'when yaml_errors is false' do + it "returns nothing" do + get api("/projects/#{project.id}/pipelines?yaml_errors=false", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.first['id']).to eq(Ci::Pipeline.where("yaml_errors IS NULL").order(id: :desc).first.id) + #TODO: Better checking all + end + end + + context 'when argument is invalid' do + it 'selects all pipelines' do + get api("/projects/#{project.id}/pipelines?yaml_errors=invalid-yaml_errors", user) + + #TODO: Eliminate repeting + expect(response).to have_http_status(400) + end + end + end + end end context 'unauthorized user' do From d74a04c499d511d6fec8d1f0ddcfc087596b9224 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 14 Mar 2017 23:47:06 +0900 Subject: [PATCH 27/57] Avoid hardcode table name --- app/finders/pipelines_finder.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index f6532377374..5e50eb46c7e 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -82,7 +82,7 @@ class PipelinesFinder def by_name(items) if params[:name].present? - items.joins(:user).where("users.name = ?", params[:name]) + items.joins(:user).where(users: { name: params[:name] }) else items end @@ -90,7 +90,7 @@ class PipelinesFinder def by_username(items) if params[:username].present? - items.joins(:user).where("users.username = ?", params[:username]) + items.joins(:user).where(users: { username: params[:username] }) else items end From d1ca5f46d4268ca3bba7801e581395e038c97129 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 14 Mar 2017 23:52:17 +0900 Subject: [PATCH 28/57] No need to support sha for sorting --- doc/api/pipelines.md | 2 +- lib/api/pipelines.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/pipelines.md b/doc/api/pipelines.md index c2aca779710..d231dfc5241 100644 --- a/doc/api/pipelines.md +++ b/doc/api/pipelines.md @@ -17,7 +17,7 @@ GET /projects/:id/pipelines | `yaml_errors`| string | no | If true, returns only yaml error pipelines | | `name`| string | no | The name of user who triggered pipelines | | `username`| string | no | The username of user who triggered pipelines | -| `order_by`| string | no | Return requests ordered by `id`, `status`, `ref`, `sha`, or `user_id` fields. Default is `id` | +| `order_by`| string | no | Return requests ordered by `id`, `status`, `ref`, or `user_id` fields. Default is `id` | | `sort` | string | no | Return requests sorted in `asc` or `desc` order. Default is `desc` | ``` diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 7d91900212a..b0f586b08da 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -22,7 +22,7 @@ module API optional :yaml_errors, type: Boolean, desc: 'If true, returns only yaml error pipelines' optional :name, type: String, desc: 'The name of user who triggered pipelines' optional :username, type: String, desc: 'The username of user who triggered pipelines' - optional :order_by, type: String, values: %w[id status ref sha user_id], default: 'id', + optional :order_by, type: String, values: %w[id status ref user_id], default: 'id', desc: 'The order_by which is combined with a sort' optional :sort, type: String, values: %w[asc desc], default: 'desc', desc: 'The sort method which is combined with an order_by' From 2894f293a28f06d22c392a6bc2bf19403b496fe0 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 15 Mar 2017 00:01:18 +0900 Subject: [PATCH 29/57] Use eq instead of match_array for comparison of array --- spec/finders/pipelines_finder_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index a2f33b7baa0..76068f21976 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -28,7 +28,7 @@ describe PipelinesFinder do end it 'orders in descending order on ID' do - expect(subject).to match_array(Ci::Pipeline.order(id: :desc)) + expect(subject).to eq(Ci::Pipeline.order(id: :desc)) end end @@ -209,7 +209,7 @@ describe PipelinesFinder do let(:params) { { order_by: 'created_at', sort: 'asc' } } it 'sorts by created_at asc' do - expect(subject).to match_array(Ci::Pipeline.order(created_at: :asc)) + expect(subject).to eq(Ci::Pipeline.order(created_at: :asc)) end end @@ -217,7 +217,7 @@ describe PipelinesFinder do let(:params) { { order_by: 'created_at', sort: 'desc' } } it 'sorts by created_at desc' do - expect(subject).to match_array(Ci::Pipeline.order(created_at: :desc)) + expect(subject).to eq(Ci::Pipeline.order(created_at: :desc)) end end @@ -225,7 +225,7 @@ describe PipelinesFinder do let(:params) { { order_by: 'invalid_column', sort: 'desc' } } it 'sorts by default' do - expect(subject).to match_array(Ci::Pipeline.order(id: :desc)) + expect(subject).to eq(Ci::Pipeline.order(id: :desc)) end end @@ -233,7 +233,7 @@ describe PipelinesFinder do let(:params) { { order_by: 'created_at', sort: 'invalid_sort' } } it 'sorts by default' do - expect(subject).to match_array(Ci::Pipeline.order(id: :desc)) + expect(subject).to eq(Ci::Pipeline.order(id: :desc)) end end end From 2075d7ce52ea9d5885fb1bb1ca70eee2e7898c31 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 15 Mar 2017 00:10:37 +0900 Subject: [PATCH 30/57] Unveil iteration --- spec/requests/api/pipelines_spec.rb | 39 +++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index e5f1765490b..c1077368559 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -43,24 +43,43 @@ describe API::Pipelines do end context 'when scope is passed' do - %w[running pending finished branches tags].each do |target| + %w[running pending].each do |target| it "returns only scope=#{target} pipelines" do get api("/projects/#{project.id}/pipelines?scope=#{target}", user) expect(response).to have_http_status(200) expect(response).to include_pagination_headers expect(json_response.count).to be > 0 - if target == 'running' || target == 'pending' - json_response.each { |r| expect(r['status']).to eq(target) } - elsif target == 'finished' - json_response.each { |r| expect(r['status']).to be_in(%w[success failed canceled]) } - elsif target == 'branches' - expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: false).last.sha) - elsif target == 'tags' - expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: true).last.sha) - end + json_response.each { |r| expect(r['status']).to eq(target) } end end + + it "returns only scope=finished pipelines" do + get api("/projects/#{project.id}/pipelines?scope=finished", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.count).to be > 0 + json_response.each { |r| expect(r['status']).to be_in(%w[success failed canceled]) } + end + + it "returns only scope=branches pipelines" do + get api("/projects/#{project.id}/pipelines?scope=branches", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.count).to be > 0 + expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: false).last.sha) + end + + it "returns only scope=tags pipelines" do + get api("/projects/#{project.id}/pipelines?scope=tags", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.count).to be > 0 + expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: true).last.sha) + end end context 'when status is passed' do From 2e43e50e742e96a60ef00e1431f39a80a11ca168 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 15 Mar 2017 01:33:25 +0900 Subject: [PATCH 31/57] Finish pipelines_spec --- spec/finders/pipelines_finder_spec.rb | 2 +- spec/requests/api/pipelines_spec.rb | 151 ++++++++++++++++++-------- 2 files changed, 106 insertions(+), 47 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 76068f21976..db2829137ac 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -233,7 +233,7 @@ describe PipelinesFinder do let(:params) { { order_by: 'created_at', sort: 'invalid_sort' } } it 'sorts by default' do - expect(subject).to eq(Ci::Pipeline.order(id: :desc)) + expect(subject).to eq(Ci::Pipeline.order(created_at: :desc)) end end end diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index c1077368559..90214f785d9 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -44,77 +44,109 @@ describe API::Pipelines do context 'when scope is passed' do %w[running pending].each do |target| - it "returns only scope=#{target} pipelines" do - get api("/projects/#{project.id}/pipelines?scope=#{target}", user) + context "when scope is #{target}" do + it "returns matched pipelines" do + get api("/projects/#{project.id}/pipelines?scope=#{target}", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.count).to be > 0 + json_response.each { |r| expect(r['status']).to eq(target) } + end + end + end + + context 'when scope is finished' do + it 'returns matched pipelines' do + get api("/projects/#{project.id}/pipelines?scope=finished", user) expect(response).to have_http_status(200) expect(response).to include_pagination_headers expect(json_response.count).to be > 0 - json_response.each { |r| expect(r['status']).to eq(target) } + json_response.each { |r| expect(r['status']).to be_in(%w[success failed canceled]) } end end - it "returns only scope=finished pipelines" do - get api("/projects/#{project.id}/pipelines?scope=finished", user) + context 'when scope is branches' do + it 'returns matched pipelines' do + get api("/projects/#{project.id}/pipelines?scope=branches", user) - expect(response).to have_http_status(200) - expect(response).to include_pagination_headers - expect(json_response.count).to be > 0 - json_response.each { |r| expect(r['status']).to be_in(%w[success failed canceled]) } + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.count).to be > 0 + expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: false).last.sha) + end end - it "returns only scope=branches pipelines" do - get api("/projects/#{project.id}/pipelines?scope=branches", user) + context 'when scope is tags' do + it 'returns matched pipelines' do + get api("/projects/#{project.id}/pipelines?scope=tags", user) - expect(response).to have_http_status(200) - expect(response).to include_pagination_headers - expect(json_response.count).to be > 0 - expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: false).last.sha) + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.count).to be > 0 + expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: true).last.sha) + end end - it "returns only scope=tags pipelines" do - get api("/projects/#{project.id}/pipelines?scope=tags", user) + context 'when scope is invalid' do + it 'returns 400' do + get api("/projects/#{project.id}/pipelines?scope=invalid-scope", user) - expect(response).to have_http_status(200) - expect(response).to include_pagination_headers - expect(json_response.count).to be > 0 - expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: true).last.sha) + expect(response).to have_http_status(400) + end end end context 'when status is passed' do %w[running pending success failed canceled skipped].each do |target| - it "returns only status=#{target} pipelines" do - get api("/projects/#{project.id}/pipelines?status=#{target}", user) + context "when status is #{target}" do + it 'returns matched pipelines' do + get api("/projects/#{project.id}/pipelines?status=#{target}", user) - expect(response).to have_http_status(200) - expect(response).to include_pagination_headers - expect(json_response.count).to be > 0 - json_response.each { |r| expect(r['status']).to eq(target) } + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.count).to be > 0 + json_response.each { |r| expect(r['status']).to eq(target) } + end + end + end + + context 'when status is invalid' do + it 'returns 400' do + get api("/projects/#{project.id}/pipelines?status=invalid-status", user) + + expect(response).to have_http_status(400) end end end context 'when ref is passed' do - %w[master invalid-ref].each do |target| - it "returns only ref=#{target} pipelines" do - get api("/projects/#{project.id}/pipelines?ref=#{target}", user) + context 'when ref exists' do + it 'returns matched pipelines' do + get api("/projects/#{project.id}/pipelines?ref=master", user) expect(response).to have_http_status(200) expect(response).to include_pagination_headers - if target == 'master' - expect(json_response.count).to be > 0 - json_response.each { |r| expect(r['ref']).to eq(target) } - else - expect(json_response.count).to eq(0) - end + expect(json_response.count).to be > 0 + json_response.each { |r| expect(r['ref']).to eq('master') } + end + end + + context 'when ref does not exist' do + it 'returns empty' do + get api("/projects/#{project.id}/pipelines?ref=invalid-ref", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.count).to eq(0) end end end context 'when name is passed' do context 'when name exists' do - it "returns only pipelines related to the name" do + it 'returns matched pipelines' do get api("/projects/#{project.id}/pipelines?name=#{user1.name}", user) expect(response).to have_http_status(200) @@ -124,7 +156,7 @@ describe API::Pipelines do end context 'when name does not exist' do - it "returns nothing" do + it 'returns empty' do get api("/projects/#{project.id}/pipelines?name=invalid-name", user) expect(response).to have_http_status(200) @@ -136,7 +168,7 @@ describe API::Pipelines do context 'when username is passed' do context 'when username exists' do - it "returns only pipelines related to the username" do + it 'returns matched pipelines' do get api("/projects/#{project.id}/pipelines?username=#{user1.username}", user) expect(response).to have_http_status(200) @@ -146,7 +178,7 @@ describe API::Pipelines do end context 'when username does not exist' do - it "returns nothing" do + it 'returns empty' do get api("/projects/#{project.id}/pipelines?username=invalid-username", user) expect(response).to have_http_status(200) @@ -158,7 +190,7 @@ describe API::Pipelines do context 'when yaml_errors is passed' do context 'when yaml_errors is true' do - it "returns only pipelines related to the yaml_errors" do + it 'returns matched pipelines' do get api("/projects/#{project.id}/pipelines?yaml_errors=true", user) expect(response).to have_http_status(200) @@ -168,21 +200,48 @@ describe API::Pipelines do end context 'when yaml_errors is false' do - it "returns nothing" do + it 'returns matched pipelines' do get api("/projects/#{project.id}/pipelines?yaml_errors=false", user) expect(response).to have_http_status(200) expect(response).to include_pagination_headers expect(json_response.first['id']).to eq(Ci::Pipeline.where("yaml_errors IS NULL").order(id: :desc).first.id) - #TODO: Better checking all end end - context 'when argument is invalid' do - it 'selects all pipelines' do + context 'when yaml_errors is invalid' do + it 'returns 400' do get api("/projects/#{project.id}/pipelines?yaml_errors=invalid-yaml_errors", user) - #TODO: Eliminate repeting + expect(response).to have_http_status(400) + end + end + end + + context 'when order_by and sort are passed' do + context 'when order_by and sort are valid' do + it 'sorts pipelines' do + get api("/projects/#{project.id}/pipelines?order_by=id&sort=asc", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.first['id']).to eq(Ci::Pipeline.order(id: :asc).first.id) + expect(json_response.last['id']).to eq(Ci::Pipeline.order(id: :asc).last.id) + end + end + + context 'when order_by is invalid' do + it 'returns 400' do + get api("/projects/#{project.id}/pipelines?order_by=lock_version&sort=asc", user) + + expect(response).to have_http_status(400) + end + end + + context 'when sort is invalid' do + it 'returns 400' do + get api("/projects/#{project.id}/pipelines?order_by=id&sort=hack", user) + expect(response).to have_http_status(400) end end From 974c3c132f5241cf58103e5e01e8b54256749a87 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 15 Mar 2017 01:42:31 +0900 Subject: [PATCH 32/57] Normalize wording --- spec/finders/pipelines_finder_spec.rb | 86 ++++++++++++--------------- 1 file changed, 39 insertions(+), 47 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index db2829137ac..02422bb6ebc 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -23,7 +23,7 @@ describe PipelinesFinder do context 'when nothing is passed' do let(:params) { {} } - it 'selects all pipelines' do + it 'returns all pipelines' do expect(subject).to match_array(Ci::Pipeline.all) end @@ -33,92 +33,92 @@ describe PipelinesFinder do end context 'when scope is passed' do - context 'when selecting running' do + context 'when scope is running' do let(:params) { { scope: 'running' } } - it 'has only running status' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.running) end end - context 'when selecting pending' do + context 'when scope is pending' do let(:params) { { scope: 'pending' } } - it 'has only pending status' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.pending) end end - context 'when selecting finished' do + context 'when scope is finished' do let(:params) { { scope: 'finished' } } - it 'has only finished status' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.finished) end end - context 'when selecting branches' do + context 'when scope is branches' do let(:params) { { scope: 'branches' } } - it 'excludes tags' do + it 'returns matched pipelines' do expect(subject).to eq([Ci::Pipeline.where(tag: false).last]) end end - context 'when selecting tags' do + context 'when scope is tags' do let(:params) { { scope: 'tags' } } - it 'excludes branches' do + it 'returns matched pipelines' do expect(subject).to eq([Ci::Pipeline.where(tag: true).last]) end end end context 'when status is passed' do - context 'when selecting running' do + context 'when status is running' do let(:params) { { status: 'running' } } - it 'has only running status' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.running) end end - context 'when selecting pending' do + context 'when status is pending' do let(:params) { { status: 'pending' } } - it 'has only pending status' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.pending) end end - context 'when selecting success' do + context 'when status is success' do let(:params) { { status: 'success' } } - it 'has only success status' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.success) end end - context 'when selecting failed' do + context 'when status is failed' do let(:params) { { status: 'failed' } } - it 'has only failed status' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.failed) end end - context 'when selecting canceled' do + context 'when status is canceled' do let(:params) { { status: 'canceled' } } - it 'has only canceled status' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.canceled) end end - context 'when selecting skipped' do + context 'when status is skipped' do let(:params) { { status: 'skipped' } } - it 'has only skipped status' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.skipped) end end @@ -128,7 +128,7 @@ describe PipelinesFinder do context 'when ref exists' do let(:params) { { ref: 'master' } } - it 'selects all pipelines which belong to the ref' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.where(ref: 'master')) end end @@ -136,7 +136,7 @@ describe PipelinesFinder do context 'when ref does not exist' do let(:params) { { ref: 'invalid-ref' } } - it 'selects nothing' do + it 'returns empty' do expect(subject).to be_empty end end @@ -146,7 +146,7 @@ describe PipelinesFinder do context 'when name exists' do let(:params) { { name: user1.name } } - it 'selects all pipelines which belong to the name' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.where(user: user1)) end end @@ -154,7 +154,7 @@ describe PipelinesFinder do context 'when name does not exist' do let(:params) { { name: 'invalid-name' } } - it 'selects nothing' do + it 'returns empty' do expect(subject).to be_empty end end @@ -164,7 +164,7 @@ describe PipelinesFinder do context 'when username exists' do let(:params) { { username: user1.username } } - it 'selects all pipelines which belong to the username' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.where(user: user1)) end end @@ -172,7 +172,7 @@ describe PipelinesFinder do context 'when username does not exist' do let(:params) { { username: 'invalid-username' } } - it 'selects nothing' do + it 'returns empty' do expect(subject).to be_empty end end @@ -182,7 +182,7 @@ describe PipelinesFinder do context 'when yaml_errors is true' do let(:params) { { yaml_errors: true } } - it 'selects only pipelines have yaml_errors' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.where("yaml_errors IS NOT NULL")) end end @@ -190,49 +190,41 @@ describe PipelinesFinder do context 'when yaml_errors is false' do let(:params) { { yaml_errors: false } } - it 'selects only pipelines do not have yaml_errors' do + it 'returns matched pipelines' do expect(subject).to match_array(Ci::Pipeline.where("yaml_errors IS NULL")) end end - context 'when an argument is invalid' do + context 'when yaml_errors is invalid' do let(:params) { { yaml_errors: "UnexpectedValue" } } - it 'selects all pipelines' do + it 'returns all pipelines' do expect(subject).to match_array(Ci::Pipeline.all) end end end context 'when order_by and sort are passed' do - context 'when order by created_at asc' do + context 'when order_by and sort are valid' do let(:params) { { order_by: 'created_at', sort: 'asc' } } - it 'sorts by created_at asc' do + it 'sorts pipelines' do expect(subject).to eq(Ci::Pipeline.order(created_at: :asc)) end end - context 'when order by created_at desc' do - let(:params) { { order_by: 'created_at', sort: 'desc' } } - - it 'sorts by created_at desc' do - expect(subject).to eq(Ci::Pipeline.order(created_at: :desc)) - end - end - - context 'when order_by does not exist' do + context 'when order_by is invalid' do let(:params) { { order_by: 'invalid_column', sort: 'desc' } } - it 'sorts by default' do + it 'sorts pipelines, but order_by is default' do expect(subject).to eq(Ci::Pipeline.order(id: :desc)) end end - context 'when sort does not exist' do + context 'when sort is invalid' do let(:params) { { order_by: 'created_at', sort: 'invalid_sort' } } - it 'sorts by default' do + it 'sorts pipelines, but sort is default' do expect(subject).to eq(Ci::Pipeline.order(created_at: :desc)) end end From f0e3076a32f1f127ef4d31b53979edd5e0218469 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 17 Mar 2017 17:59:13 +0900 Subject: [PATCH 33/57] 'to be > 0' to 'not_to be_empty'. 'to eq(0)' to 'to be_empty' --- spec/requests/api/pipelines_spec.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index 90214f785d9..e3cda85b244 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -50,7 +50,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.count).to be > 0 + expect(json_response).not_to be_empty json_response.each { |r| expect(r['status']).to eq(target) } end end @@ -62,7 +62,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.count).to be > 0 + expect(json_response).not_to be_empty json_response.each { |r| expect(r['status']).to be_in(%w[success failed canceled]) } end end @@ -73,7 +73,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.count).to be > 0 + expect(json_response).not_to be_empty expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: false).last.sha) end end @@ -84,7 +84,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.count).to be > 0 + expect(json_response).not_to be_empty expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: true).last.sha) end end @@ -106,7 +106,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.count).to be > 0 + expect(json_response).not_to be_empty json_response.each { |r| expect(r['status']).to eq(target) } end end @@ -128,7 +128,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.count).to be > 0 + expect(json_response).not_to be_empty json_response.each { |r| expect(r['ref']).to eq('master') } end end @@ -139,7 +139,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.count).to eq(0) + expect(json_response).to be_empty end end end @@ -161,7 +161,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.count).to eq(0) + expect(json_response).to be_empty end end end @@ -183,7 +183,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.count).to eq(0) + expect(json_response).to be_empty end end end From 3735b8aaa1f48ea3803e31e18f1e40d2fd091b26 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 17 Mar 2017 18:27:11 +0900 Subject: [PATCH 34/57] Allow only indexed columns in #order_and_sort. Remove present (Because unnecessary) from condition. Added spec just in case. --- app/finders/pipelines_finder.rb | 4 ++-- spec/finders/pipelines_finder_spec.rb | 10 +++++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 5e50eb46c7e..6a92aedc873 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -108,12 +108,12 @@ class PipelinesFinder end def order_and_sort(items) - order_by = if params[:order_by].present? && items.column_names.include?(params[:order_by]) + order_by = if %w[id status ref user_id].include?(params[:order_by]) # Allow only indexed columns params[:order_by] else :id end - sort = if params[:sort].present? && params[:sort] =~ /\A(ASC|DESC)\z/i + sort = if params[:sort] =~ /\A(ASC|DESC)\z/i params[:sort] else :desc diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 02422bb6ebc..02c91cbf465 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -208,7 +208,7 @@ describe PipelinesFinder do context 'when order_by and sort are valid' do let(:params) { { order_by: 'created_at', sort: 'asc' } } - it 'sorts pipelines' do + it 'sorts pipelines by default' do expect(subject).to eq(Ci::Pipeline.order(created_at: :asc)) end end @@ -228,6 +228,14 @@ describe PipelinesFinder do expect(subject).to eq(Ci::Pipeline.order(created_at: :desc)) end end + + context 'when both are nil' do + let(:params) { { order_by: nil, sort: nil } } + + it 'sorts pipelines by default' do + expect(subject).to eq(Ci::Pipeline.order(id: :desc)) + end + end end end end From e72c50fcb2f60516284fb66aa322cd0880cbda80 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 17 Mar 2017 18:36:10 +0900 Subject: [PATCH 35/57] Change sort spec description --- spec/finders/pipelines_finder_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 02c91cbf465..a218f315f79 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -216,7 +216,7 @@ describe PipelinesFinder do context 'when order_by is invalid' do let(:params) { { order_by: 'invalid_column', sort: 'desc' } } - it 'sorts pipelines, but order_by is default' do + it 'sorts pipelines with default order_by (id:)' do expect(subject).to eq(Ci::Pipeline.order(id: :desc)) end end @@ -224,7 +224,7 @@ describe PipelinesFinder do context 'when sort is invalid' do let(:params) { { order_by: 'created_at', sort: 'invalid_sort' } } - it 'sorts pipelines, but sort is default' do + it 'sorts pipelines with default sort (:desc)' do expect(subject).to eq(Ci::Pipeline.order(created_at: :desc)) end end From c79c389525d8f31dfc9119a1eb6e9a41585facb9 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 17 Mar 2017 22:22:50 +0900 Subject: [PATCH 36/57] Fix created_at to user_id in spec --- spec/finders/pipelines_finder_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index a218f315f79..b5cb7d75c5e 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -206,26 +206,26 @@ describe PipelinesFinder do context 'when order_by and sort are passed' do context 'when order_by and sort are valid' do - let(:params) { { order_by: 'created_at', sort: 'asc' } } + let(:params) { { order_by: 'user_id', sort: 'asc' } } it 'sorts pipelines by default' do - expect(subject).to eq(Ci::Pipeline.order(created_at: :asc)) + expect(subject).to eq(Ci::Pipeline.order(user_id: :asc)) end end context 'when order_by is invalid' do - let(:params) { { order_by: 'invalid_column', sort: 'desc' } } + let(:params) { { order_by: 'invalid_column', sort: 'asc' } } it 'sorts pipelines with default order_by (id:)' do - expect(subject).to eq(Ci::Pipeline.order(id: :desc)) + expect(subject).to eq(Ci::Pipeline.order(id: :asc)) end end context 'when sort is invalid' do - let(:params) { { order_by: 'created_at', sort: 'invalid_sort' } } + let(:params) { { order_by: 'user_id', sort: 'invalid_sort' } } it 'sorts pipelines with default sort (:desc)' do - expect(subject).to eq(Ci::Pipeline.order(created_at: :desc)) + expect(subject).to eq(Ci::Pipeline.order(user_id: :desc)) end end From 7fb3a78a6d23b1fe0b14fab30e1fac4ec8d27d85 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 17 Mar 2017 22:34:38 +0900 Subject: [PATCH 37/57] Fix improper method name and spec description --- app/finders/pipelines_finder.rb | 4 ++-- spec/finders/pipelines_finder_spec.rb | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 6a92aedc873..0c91c136a8f 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -15,7 +15,7 @@ class PipelinesFinder items = by_name(items) items = by_username(items) items = by_yaml_errors(items) - order_and_sort(items) + sort_items(items) end private @@ -107,7 +107,7 @@ class PipelinesFinder end end - def order_and_sort(items) + def sort_items(items) order_by = if %w[id status ref user_id].include?(params[:order_by]) # Allow only indexed columns params[:order_by] else diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index b5cb7d75c5e..ac64df8aeb8 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -208,7 +208,7 @@ describe PipelinesFinder do context 'when order_by and sort are valid' do let(:params) { { order_by: 'user_id', sort: 'asc' } } - it 'sorts pipelines by default' do + it 'sorts pipelines' do expect(subject).to eq(Ci::Pipeline.order(user_id: :asc)) end end @@ -216,7 +216,7 @@ describe PipelinesFinder do context 'when order_by is invalid' do let(:params) { { order_by: 'invalid_column', sort: 'asc' } } - it 'sorts pipelines with default order_by (id:)' do + it 'sorts pipelines with id: (default)' do expect(subject).to eq(Ci::Pipeline.order(id: :asc)) end end @@ -224,7 +224,7 @@ describe PipelinesFinder do context 'when sort is invalid' do let(:params) { { order_by: 'user_id', sort: 'invalid_sort' } } - it 'sorts pipelines with default sort (:desc)' do + it 'sorts pipelines with :desc (default)' do expect(subject).to eq(Ci::Pipeline.order(user_id: :desc)) end end From 8f32724fcb7f05052b53dcd365a064ad87a9535e Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 17 Mar 2017 23:50:20 +0900 Subject: [PATCH 38/57] Ci::Pipeline to project.pipelines --- spec/finders/pipelines_finder_spec.rb | 46 +++++++++++++-------------- spec/requests/api/pipelines_spec.rb | 18 +++++------ 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index ac64df8aeb8..13218cd8b50 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -24,11 +24,11 @@ describe PipelinesFinder do let(:params) { {} } it 'returns all pipelines' do - expect(subject).to match_array(Ci::Pipeline.all) + expect(subject).to match_array(project.pipelines) end it 'orders in descending order on ID' do - expect(subject).to eq(Ci::Pipeline.order(id: :desc)) + expect(subject).to eq(project.pipelines.order(id: :desc)) end end @@ -37,7 +37,7 @@ describe PipelinesFinder do let(:params) { { scope: 'running' } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.running) + expect(subject).to match_array(project.pipelines.running) end end @@ -45,7 +45,7 @@ describe PipelinesFinder do let(:params) { { scope: 'pending' } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.pending) + expect(subject).to match_array(project.pipelines.pending) end end @@ -53,7 +53,7 @@ describe PipelinesFinder do let(:params) { { scope: 'finished' } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.finished) + expect(subject).to match_array(project.pipelines.finished) end end @@ -61,7 +61,7 @@ describe PipelinesFinder do let(:params) { { scope: 'branches' } } it 'returns matched pipelines' do - expect(subject).to eq([Ci::Pipeline.where(tag: false).last]) + expect(subject).to eq([project.pipelines.where(tag: false).last]) end end @@ -69,7 +69,7 @@ describe PipelinesFinder do let(:params) { { scope: 'tags' } } it 'returns matched pipelines' do - expect(subject).to eq([Ci::Pipeline.where(tag: true).last]) + expect(subject).to eq([project.pipelines.where(tag: true).last]) end end end @@ -79,7 +79,7 @@ describe PipelinesFinder do let(:params) { { status: 'running' } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.running) + expect(subject).to match_array(project.pipelines.running) end end @@ -87,7 +87,7 @@ describe PipelinesFinder do let(:params) { { status: 'pending' } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.pending) + expect(subject).to match_array(project.pipelines.pending) end end @@ -95,7 +95,7 @@ describe PipelinesFinder do let(:params) { { status: 'success' } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.success) + expect(subject).to match_array(project.pipelines.success) end end @@ -103,7 +103,7 @@ describe PipelinesFinder do let(:params) { { status: 'failed' } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.failed) + expect(subject).to match_array(project.pipelines.failed) end end @@ -111,7 +111,7 @@ describe PipelinesFinder do let(:params) { { status: 'canceled' } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.canceled) + expect(subject).to match_array(project.pipelines.canceled) end end @@ -119,7 +119,7 @@ describe PipelinesFinder do let(:params) { { status: 'skipped' } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.skipped) + expect(subject).to match_array(project.pipelines.skipped) end end end @@ -129,7 +129,7 @@ describe PipelinesFinder do let(:params) { { ref: 'master' } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.where(ref: 'master')) + expect(subject).to match_array(project.pipelines.where(ref: 'master')) end end @@ -147,7 +147,7 @@ describe PipelinesFinder do let(:params) { { name: user1.name } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.where(user: user1)) + expect(subject).to match_array(project.pipelines.where(user: user1)) end end @@ -165,7 +165,7 @@ describe PipelinesFinder do let(:params) { { username: user1.username } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.where(user: user1)) + expect(subject).to match_array(project.pipelines.where(user: user1)) end end @@ -183,7 +183,7 @@ describe PipelinesFinder do let(:params) { { yaml_errors: true } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.where("yaml_errors IS NOT NULL")) + expect(subject).to match_array(project.pipelines.where("yaml_errors IS NOT NULL")) end end @@ -191,7 +191,7 @@ describe PipelinesFinder do let(:params) { { yaml_errors: false } } it 'returns matched pipelines' do - expect(subject).to match_array(Ci::Pipeline.where("yaml_errors IS NULL")) + expect(subject).to match_array(project.pipelines.where("yaml_errors IS NULL")) end end @@ -199,7 +199,7 @@ describe PipelinesFinder do let(:params) { { yaml_errors: "UnexpectedValue" } } it 'returns all pipelines' do - expect(subject).to match_array(Ci::Pipeline.all) + expect(subject).to match_array(project.pipelines.all) end end end @@ -209,7 +209,7 @@ describe PipelinesFinder do let(:params) { { order_by: 'user_id', sort: 'asc' } } it 'sorts pipelines' do - expect(subject).to eq(Ci::Pipeline.order(user_id: :asc)) + expect(subject).to eq(project.pipelines.order(user_id: :asc)) end end @@ -217,7 +217,7 @@ describe PipelinesFinder do let(:params) { { order_by: 'invalid_column', sort: 'asc' } } it 'sorts pipelines with id: (default)' do - expect(subject).to eq(Ci::Pipeline.order(id: :asc)) + expect(subject).to eq(project.pipelines.order(id: :asc)) end end @@ -225,7 +225,7 @@ describe PipelinesFinder do let(:params) { { order_by: 'user_id', sort: 'invalid_sort' } } it 'sorts pipelines with :desc (default)' do - expect(subject).to eq(Ci::Pipeline.order(user_id: :desc)) + expect(subject).to eq(project.pipelines.order(user_id: :desc)) end end @@ -233,7 +233,7 @@ describe PipelinesFinder do let(:params) { { order_by: nil, sort: nil } } it 'sorts pipelines by default' do - expect(subject).to eq(Ci::Pipeline.order(id: :desc)) + expect(subject).to eq(project.pipelines.order(id: :desc)) end end end diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index e3cda85b244..5e20a823d39 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -74,7 +74,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers expect(json_response).not_to be_empty - expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: false).last.sha) + expect(json_response.last['sha']).to eq(project.pipelines.where(tag: false).last.sha) end end @@ -85,7 +85,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers expect(json_response).not_to be_empty - expect(json_response.last['sha']).to eq(Ci::Pipeline.where(tag: true).last.sha) + expect(json_response.last['sha']).to eq(project.pipelines.where(tag: true).last.sha) end end @@ -151,7 +151,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.first['sha']).to eq(Ci::Pipeline.where(user: user1).order(id: :desc).first.sha) + expect(json_response.first['sha']).to eq(project.pipelines.where(user: user1).order(id: :desc).first.sha) end end @@ -173,7 +173,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.first['sha']).to eq(Ci::Pipeline.where(user: user1).order(id: :desc).first.sha) + expect(json_response.first['sha']).to eq(project.pipelines.where(user: user1).order(id: :desc).first.sha) end end @@ -195,7 +195,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.first['id']).to eq(Ci::Pipeline.where("yaml_errors IS NOT NULL").order(id: :desc).first.id) + expect(json_response.first['id']).to eq(project.pipelines.where("yaml_errors IS NOT NULL").order(id: :desc).first.id) end end @@ -205,7 +205,7 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.first['id']).to eq(Ci::Pipeline.where("yaml_errors IS NULL").order(id: :desc).first.id) + expect(json_response.first['id']).to eq(project.pipelines.where("yaml_errors IS NULL").order(id: :desc).first.id) end end @@ -221,12 +221,12 @@ describe API::Pipelines do context 'when order_by and sort are passed' do context 'when order_by and sort are valid' do it 'sorts pipelines' do - get api("/projects/#{project.id}/pipelines?order_by=id&sort=asc", user) + get api("/projects/#{project.id}/pipelines?order_by=user_id&sort=asc", user) expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.first['id']).to eq(Ci::Pipeline.order(id: :asc).first.id) - expect(json_response.last['id']).to eq(Ci::Pipeline.order(id: :asc).last.id) + expect(json_response.first['id']).to eq(project.pipelines.order(user_id: :asc).first.id) + expect(json_response.last['id']).to eq(project.pipelines.order(user_id: :asc).last.id) end end From 98ac988d4d9525b3f07a19e495a9c2666ebee3c6 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sun, 19 Mar 2017 02:50:42 +0900 Subject: [PATCH 39/57] Use order instead of reorder. Improve tests. --- app/finders/pipelines_finder.rb | 2 +- spec/requests/api/pipelines_spec.rb | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 0c91c136a8f..22507472e15 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -118,6 +118,6 @@ class PipelinesFinder else :desc end - items.reorder(order_by => sort) + items.order(order_by => sort) end end diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index 5e20a823d39..a0122d1d300 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -225,8 +225,11 @@ describe API::Pipelines do expect(response).to have_http_status(200) expect(response).to include_pagination_headers - expect(json_response.first['id']).to eq(project.pipelines.order(user_id: :asc).first.id) - expect(json_response.last['id']).to eq(project.pipelines.order(user_id: :asc).last.id) + expect(json_response).not_to be_empty + pipelines = project.pipelines.order(user_id: :asc) + json_response.each_with_index do |r, i| + expect(r['id']).to eq(pipelines[i].id) + end end end From 22a4d124f70598c7c21b08b11db3f38c7bcb71ec Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 22 Mar 2017 02:37:19 +0900 Subject: [PATCH 40/57] Use JSON type for sorting parameter (halfway) --- app/finders/pipelines_finder.rb | 26 +++++++++++++++----------- lib/api/pipelines.rb | 8 ++++---- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 22507472e15..d56cf8fe790 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -108,16 +108,20 @@ class PipelinesFinder end def sort_items(items) - order_by = if %w[id status ref user_id].include?(params[:order_by]) # Allow only indexed columns - params[:order_by] - else - :id - end - sort = if params[:sort] =~ /\A(ASC|DESC)\z/i - params[:sort] - else - :desc - end - items.order(order_by => sort) + return items.order(id: :desc) unless params[:sort].present? + params[:sort].each do |s| + order_by = if %w[id status ref user_id].include?(s['order_by']) # Allow only indexed columns + s['order_by'] + else + :id + end + sort = if s['asc_desc'] =~ /\A(ASC|DESC)\z/i + s['asc_desc'] + else + :desc + end + items = items.order(order_by => sort) + end + items end end diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index b0f586b08da..6bbb679cb5e 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -22,10 +22,10 @@ module API optional :yaml_errors, type: Boolean, desc: 'If true, returns only yaml error pipelines' optional :name, type: String, desc: 'The name of user who triggered pipelines' optional :username, type: String, desc: 'The username of user who triggered pipelines' - optional :order_by, type: String, values: %w[id status ref user_id], default: 'id', - desc: 'The order_by which is combined with a sort' - optional :sort, type: String, values: %w[asc desc], default: 'desc', - desc: 'The sort method which is combined with an order_by' + optional :sort, type: JSON, desc: 'order_by and asc_desc' do + requires :order_by, type: String, values: %w[id status ref user_id] + requires :asc_desc, type: String, values: %w[asc desc] + end end get ':id/pipelines' do authorize! :read_pipeline, user_project From 0e8266f2386351906e2d6357282e011d373b2c94 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 24 Mar 2017 16:06:19 +0900 Subject: [PATCH 41/57] Revert "Use JSON type for sorting parameter (halfway)" This reverts commit 34127cb13ad72f65a24bdc8fc051363d3edd77cb. --- app/finders/pipelines_finder.rb | 26 +++++++++++--------------- lib/api/pipelines.rb | 8 ++++---- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index d56cf8fe790..22507472e15 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -108,20 +108,16 @@ class PipelinesFinder end def sort_items(items) - return items.order(id: :desc) unless params[:sort].present? - params[:sort].each do |s| - order_by = if %w[id status ref user_id].include?(s['order_by']) # Allow only indexed columns - s['order_by'] - else - :id - end - sort = if s['asc_desc'] =~ /\A(ASC|DESC)\z/i - s['asc_desc'] - else - :desc - end - items = items.order(order_by => sort) - end - items + order_by = if %w[id status ref user_id].include?(params[:order_by]) # Allow only indexed columns + params[:order_by] + else + :id + end + sort = if params[:sort] =~ /\A(ASC|DESC)\z/i + params[:sort] + else + :desc + end + items.order(order_by => sort) end end diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 6bbb679cb5e..b0f586b08da 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -22,10 +22,10 @@ module API optional :yaml_errors, type: Boolean, desc: 'If true, returns only yaml error pipelines' optional :name, type: String, desc: 'The name of user who triggered pipelines' optional :username, type: String, desc: 'The username of user who triggered pipelines' - optional :sort, type: JSON, desc: 'order_by and asc_desc' do - requires :order_by, type: String, values: %w[id status ref user_id] - requires :asc_desc, type: String, values: %w[asc desc] - end + optional :order_by, type: String, values: %w[id status ref user_id], default: 'id', + desc: 'The order_by which is combined with a sort' + optional :sort, type: String, values: %w[asc desc], default: 'desc', + desc: 'The sort method which is combined with an order_by' end get ':id/pipelines' do authorize! :read_pipeline, user_project From f5f7f90abe6bc0f4e19e0abace72c8b1fd69f519 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 24 Mar 2017 17:30:26 +0900 Subject: [PATCH 42/57] Revise document. string to boolean. --- doc/api/pipelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/pipelines.md b/doc/api/pipelines.md index d231dfc5241..06307158e82 100644 --- a/doc/api/pipelines.md +++ b/doc/api/pipelines.md @@ -14,7 +14,7 @@ GET /projects/:id/pipelines | `scope` | string | no | The scope of pipelines, one of: `running`, `pending`, `finished`, `branches`, `tags`; | | `status` | string | no | The status of pipelines, one of: `running`, `pending`, `success`, `failed`, `canceled`, `skipped`; | | `ref` | string | no | The ref of pipelines | -| `yaml_errors`| string | no | If true, returns only yaml error pipelines | +| `yaml_errors`| boolean | no | Returns pipelines which have an error of gitlab-ci.yml | | `name`| string | no | The name of user who triggered pipelines | | `username`| string | no | The username of user who triggered pipelines | | `order_by`| string | no | Return requests ordered by `id`, `status`, `ref`, or `user_id` fields. Default is `id` | From f7b5800a1dc6a2a59758aea502a2ddb8f103634b Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 27 Mar 2017 14:51:28 +0900 Subject: [PATCH 43/57] Add a blank line between blocks --- app/finders/pipelines_finder.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 22507472e15..7935878d1d5 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -113,11 +113,13 @@ class PipelinesFinder else :id end + sort = if params[:sort] =~ /\A(ASC|DESC)\z/i params[:sort] else :desc end + items.order(order_by => sort) end end From 33c284fa8cdfe10f20334866b1043b4f9350f055 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 27 Mar 2017 15:14:52 +0900 Subject: [PATCH 44/57] Separate parameters from literal url string --- spec/requests/api/pipelines_spec.rb | 38 ++++++++++++++--------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index a0122d1d300..4e4564fdccd 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -46,7 +46,7 @@ describe API::Pipelines do %w[running pending].each do |target| context "when scope is #{target}" do it "returns matched pipelines" do - get api("/projects/#{project.id}/pipelines?scope=#{target}", user) + get api("/projects/#{project.id}/pipelines", user), { scope: target } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -58,7 +58,7 @@ describe API::Pipelines do context 'when scope is finished' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines?scope=finished", user) + get api("/projects/#{project.id}/pipelines", user), { scope: 'finished' } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -69,7 +69,7 @@ describe API::Pipelines do context 'when scope is branches' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines?scope=branches", user) + get api("/projects/#{project.id}/pipelines", user), { scope: 'branches' } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -80,7 +80,7 @@ describe API::Pipelines do context 'when scope is tags' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines?scope=tags", user) + get api("/projects/#{project.id}/pipelines", user), { scope: 'tags' } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -91,7 +91,7 @@ describe API::Pipelines do context 'when scope is invalid' do it 'returns 400' do - get api("/projects/#{project.id}/pipelines?scope=invalid-scope", user) + get api("/projects/#{project.id}/pipelines", user), { scope: 'invalid-scope' } expect(response).to have_http_status(400) end @@ -102,7 +102,7 @@ describe API::Pipelines do %w[running pending success failed canceled skipped].each do |target| context "when status is #{target}" do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines?status=#{target}", user) + get api("/projects/#{project.id}/pipelines", user), { status: target } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -114,7 +114,7 @@ describe API::Pipelines do context 'when status is invalid' do it 'returns 400' do - get api("/projects/#{project.id}/pipelines?status=invalid-status", user) + get api("/projects/#{project.id}/pipelines", user), { status: 'invalid-status' } expect(response).to have_http_status(400) end @@ -124,7 +124,7 @@ describe API::Pipelines do context 'when ref is passed' do context 'when ref exists' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines?ref=master", user) + get api("/projects/#{project.id}/pipelines", user), { ref: 'master' } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -135,7 +135,7 @@ describe API::Pipelines do context 'when ref does not exist' do it 'returns empty' do - get api("/projects/#{project.id}/pipelines?ref=invalid-ref", user) + get api("/projects/#{project.id}/pipelines", user), { ref: 'invalid-ref' } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -147,7 +147,7 @@ describe API::Pipelines do context 'when name is passed' do context 'when name exists' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines?name=#{user1.name}", user) + get api("/projects/#{project.id}/pipelines", user), { name: user1.name } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -157,7 +157,7 @@ describe API::Pipelines do context 'when name does not exist' do it 'returns empty' do - get api("/projects/#{project.id}/pipelines?name=invalid-name", user) + get api("/projects/#{project.id}/pipelines", user), { name: 'invalid-name' } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -169,7 +169,7 @@ describe API::Pipelines do context 'when username is passed' do context 'when username exists' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines?username=#{user1.username}", user) + get api("/projects/#{project.id}/pipelines", user), { username: user1.username } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -179,7 +179,7 @@ describe API::Pipelines do context 'when username does not exist' do it 'returns empty' do - get api("/projects/#{project.id}/pipelines?username=invalid-username", user) + get api("/projects/#{project.id}/pipelines", user), { username: 'invalid-username' } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -191,7 +191,7 @@ describe API::Pipelines do context 'when yaml_errors is passed' do context 'when yaml_errors is true' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines?yaml_errors=true", user) + get api("/projects/#{project.id}/pipelines", user), { yaml_errors: true } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -201,7 +201,7 @@ describe API::Pipelines do context 'when yaml_errors is false' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines?yaml_errors=false", user) + get api("/projects/#{project.id}/pipelines", user), { yaml_errors: false } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -211,7 +211,7 @@ describe API::Pipelines do context 'when yaml_errors is invalid' do it 'returns 400' do - get api("/projects/#{project.id}/pipelines?yaml_errors=invalid-yaml_errors", user) + get api("/projects/#{project.id}/pipelines", user), { yaml_errors: 'invalid-yaml_errors' } expect(response).to have_http_status(400) end @@ -221,7 +221,7 @@ describe API::Pipelines do context 'when order_by and sort are passed' do context 'when order_by and sort are valid' do it 'sorts pipelines' do - get api("/projects/#{project.id}/pipelines?order_by=user_id&sort=asc", user) + get api("/projects/#{project.id}/pipelines", user), { order_by: 'user_id', sort: 'asc' } expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -235,7 +235,7 @@ describe API::Pipelines do context 'when order_by is invalid' do it 'returns 400' do - get api("/projects/#{project.id}/pipelines?order_by=lock_version&sort=asc", user) + get api("/projects/#{project.id}/pipelines", user), { order_by: 'lock_version', sort: 'asc' } expect(response).to have_http_status(400) end @@ -243,7 +243,7 @@ describe API::Pipelines do context 'when sort is invalid' do it 'returns 400' do - get api("/projects/#{project.id}/pipelines?order_by=id&sort=hack", user) + get api("/projects/#{project.id}/pipelines", user), { order_by: 'id', sort: 'hack' } expect(response).to have_http_status(400) end From f65cd3289cb555d008d7792635f12b6273c65cfb Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 27 Mar 2017 19:12:09 +0900 Subject: [PATCH 45/57] Improve pipelines_finder_spec.rb --- spec/finders/pipelines_finder_spec.rb | 185 ++++++++++---------------- 1 file changed, 71 insertions(+), 114 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 13218cd8b50..01b22b1a044 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -1,67 +1,49 @@ require 'spec_helper' describe PipelinesFinder do - let(:user1) { create(:user) } - let(:user2) { create(:user) } let(:project) { create(:project, :repository) } - before do - create(:ci_pipeline, project: project, user: user1, ref: 'v1.0.0', tag: true) - create(:ci_pipeline, project: project, user: user1, status: 'created') - create(:ci_pipeline, project: project, user: user1, status: 'pending') - create(:ci_pipeline, project: project, user: user1, status: 'running') - create(:ci_pipeline, project: project, user: user1, status: 'success') - create(:ci_pipeline, project: project, user: user2, status: 'failed') - create(:ci_pipeline, project: project, user: user2, status: 'canceled') - create(:ci_pipeline, project: project, user: user2, status: 'skipped') - create(:ci_pipeline, project: project, user: user2, yaml_errors: 'Syntax error') - end - subject { described_class.new(project, params).execute } describe "#execute" do - context 'when nothing is passed' do + context 'when params is empty' do let(:params) { {} } + let!(:pipelines) { create_list(:ci_pipeline, 2, project: project) } it 'returns all pipelines' do - expect(subject).to match_array(project.pipelines) - end - - it 'orders in descending order on ID' do - expect(subject).to eq(project.pipelines.order(id: :desc)) + is_expected.to eq(pipelines.sort_by{ |p| -p.id }) end end - context 'when scope is passed' do - context 'when scope is running' do - let(:params) { { scope: 'running' } } + %w[running pending].each do |target| + context "when scope is #{target}" do + let(:params) { { scope: target } } + let!(:pipeline) { create(:ci_pipeline, project: project, status: target) } it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.running) + is_expected.to eq([pipeline]) end end + end - context 'when scope is pending' do - let(:params) { { scope: 'pending' } } + context 'when scope is finished' do + let(:params) { { scope: 'finished' } } + let!(:pipeline) { create(:ci_pipeline, project: project, status: 'success') } - it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.pending) - end + it 'returns matched pipelines' do + is_expected.to eq([pipeline]) end + end - context 'when scope is finished' do - let(:params) { { scope: 'finished' } } - - it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.finished) - end - end + context 'when scope is branches or tags' do + let!(:pipeline_branch) { create(:ci_pipeline, project: project) } + let!(:pipeline_tag) { create(:ci_pipeline, project: project, ref: 'v1.0.0', tag: true) } context 'when scope is branches' do let(:params) { { scope: 'branches' } } it 'returns matched pipelines' do - expect(subject).to eq([project.pipelines.where(tag: false).last]) + is_expected.to eq([pipeline_branch]) end end @@ -69,67 +51,30 @@ describe PipelinesFinder do let(:params) { { scope: 'tags' } } it 'returns matched pipelines' do - expect(subject).to eq([project.pipelines.where(tag: true).last]) + is_expected.to eq([pipeline_tag]) end end end - context 'when status is passed' do - context 'when status is running' do - let(:params) { { status: 'running' } } + %w[running pending success failed canceled skipped].each do |target| + context "when status is #{target}" do + let(:params) { { status: target } } + let!(:pipeline) { create(:ci_pipeline, project: project, status: target) } it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.running) + is_expected.to eq([pipeline]) end end + end - context 'when status is pending' do - let(:params) { { status: 'pending' } } + context 'when ref is specified' do + let!(:pipeline) { create(:ci_pipeline, project: project) } - it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.pending) - end - end - - context 'when status is success' do - let(:params) { { status: 'success' } } - - it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.success) - end - end - - context 'when status is failed' do - let(:params) { { status: 'failed' } } - - it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.failed) - end - end - - context 'when status is canceled' do - let(:params) { { status: 'canceled' } } - - it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.canceled) - end - end - - context 'when status is skipped' do - let(:params) { { status: 'skipped' } } - - it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.skipped) - end - end - end - - context 'when ref is passed' do context 'when ref exists' do let(:params) { { ref: 'master' } } it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.where(ref: 'master')) + is_expected.to eq([pipeline]) end end @@ -137,17 +82,20 @@ describe PipelinesFinder do let(:params) { { ref: 'invalid-ref' } } it 'returns empty' do - expect(subject).to be_empty + is_expected.to be_empty end end end - context 'when name is passed' do + context 'when name is specified' do + let(:user) { create(:user) } + let!(:pipeline) { create(:ci_pipeline, project: project, user: user) } + context 'when name exists' do - let(:params) { { name: user1.name } } + let(:params) { { name: user.name } } it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.where(user: user1)) + is_expected.to eq([pipeline]) end end @@ -155,17 +103,20 @@ describe PipelinesFinder do let(:params) { { name: 'invalid-name' } } it 'returns empty' do - expect(subject).to be_empty + is_expected.to be_empty end end end - context 'when username is passed' do + context 'when username is specified' do + let(:user) { create(:user) } + let!(:pipeline) { create(:ci_pipeline, project: project, user: user) } + context 'when username exists' do - let(:params) { { username: user1.username } } + let(:params) { { username: user.username } } it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.where(user: user1)) + is_expected.to eq([pipeline]) end end @@ -173,17 +124,20 @@ describe PipelinesFinder do let(:params) { { username: 'invalid-username' } } it 'returns empty' do - expect(subject).to be_empty + is_expected.to be_empty end end end - context 'when yaml_errors is passed' do + context 'when yaml_errors is specified' do + let!(:pipeline1) { create(:ci_pipeline, project: project, yaml_errors: 'Syntax error') } + let!(:pipeline2) { create(:ci_pipeline, project: project) } + context 'when yaml_errors is true' do let(:params) { { yaml_errors: true } } it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.where("yaml_errors IS NOT NULL")) + is_expected.to eq([pipeline1]) end end @@ -191,49 +145,52 @@ describe PipelinesFinder do let(:params) { { yaml_errors: false } } it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.where("yaml_errors IS NULL")) + is_expected.to eq([pipeline2]) end end context 'when yaml_errors is invalid' do - let(:params) { { yaml_errors: "UnexpectedValue" } } + let(:params) { { yaml_errors: "invalid-yaml_errors" } } it 'returns all pipelines' do - expect(subject).to match_array(project.pipelines.all) + is_expected.to eq([pipeline1, pipeline2].sort_by{ |p| -p.id }) end end end - context 'when order_by and sort are passed' do - context 'when order_by and sort are valid' do + context 'when order_by and sort are specified' do + context 'when order_by user_id' do let(:params) { { order_by: 'user_id', sort: 'asc' } } + let!(:pipelines) { create_list(:ci_pipeline, 2, project: project, user: create(:user)) } - it 'sorts pipelines' do - expect(subject).to eq(project.pipelines.order(user_id: :asc)) + it 'sorts as user_id: :desc' do + is_expected.to eq(pipelines.sort_by{ |p| p.user.id }) + end + + context 'when sort is invalid' do + let(:params) { { order_by: 'user_id', sort: 'invalid_sort' } } + + it 'sorts as user_id: :desc' do + is_expected.to eq(pipelines.sort_by{ |p| -p.user.id }) + end end end context 'when order_by is invalid' do let(:params) { { order_by: 'invalid_column', sort: 'asc' } } + let!(:pipelines) { create_list(:ci_pipeline, 2, project: project) } - it 'sorts pipelines with id: (default)' do - expect(subject).to eq(project.pipelines.order(id: :asc)) - end - end - - context 'when sort is invalid' do - let(:params) { { order_by: 'user_id', sort: 'invalid_sort' } } - - it 'sorts pipelines with :desc (default)' do - expect(subject).to eq(project.pipelines.order(user_id: :desc)) + it 'sorts as id: :asc' do + is_expected.to eq(pipelines.sort_by{ |p| p.id }) end end context 'when both are nil' do let(:params) { { order_by: nil, sort: nil } } + let!(:pipelines) { create_list(:ci_pipeline, 2, project: project) } - it 'sorts pipelines by default' do - expect(subject).to eq(project.pipelines.order(id: :desc)) + it 'sorts as id: :desc' do + is_expected.to eq(pipelines.sort_by{ |p| -p.id }) end end end From 673693888ad5fcee2f7b7af66e02b7ec0a3c7f95 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 27 Mar 2017 19:15:21 +0900 Subject: [PATCH 46/57] Remove unnecessary hash --- spec/requests/api/pipelines_spec.rb | 38 ++++++++++++++--------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index 4e4564fdccd..bbb2dfe1d47 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -46,7 +46,7 @@ describe API::Pipelines do %w[running pending].each do |target| context "when scope is #{target}" do it "returns matched pipelines" do - get api("/projects/#{project.id}/pipelines", user), { scope: target } + get api("/projects/#{project.id}/pipelines", user), scope: target expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -58,7 +58,7 @@ describe API::Pipelines do context 'when scope is finished' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines", user), { scope: 'finished' } + get api("/projects/#{project.id}/pipelines", user), scope: 'finished' expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -69,7 +69,7 @@ describe API::Pipelines do context 'when scope is branches' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines", user), { scope: 'branches' } + get api("/projects/#{project.id}/pipelines", user), scope: 'branches' expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -80,7 +80,7 @@ describe API::Pipelines do context 'when scope is tags' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines", user), { scope: 'tags' } + get api("/projects/#{project.id}/pipelines", user), scope: 'tags' expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -91,7 +91,7 @@ describe API::Pipelines do context 'when scope is invalid' do it 'returns 400' do - get api("/projects/#{project.id}/pipelines", user), { scope: 'invalid-scope' } + get api("/projects/#{project.id}/pipelines", user), scope: 'invalid-scope' expect(response).to have_http_status(400) end @@ -102,7 +102,7 @@ describe API::Pipelines do %w[running pending success failed canceled skipped].each do |target| context "when status is #{target}" do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines", user), { status: target } + get api("/projects/#{project.id}/pipelines", user), status: target expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -114,7 +114,7 @@ describe API::Pipelines do context 'when status is invalid' do it 'returns 400' do - get api("/projects/#{project.id}/pipelines", user), { status: 'invalid-status' } + get api("/projects/#{project.id}/pipelines", user), status: 'invalid-status' expect(response).to have_http_status(400) end @@ -124,7 +124,7 @@ describe API::Pipelines do context 'when ref is passed' do context 'when ref exists' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines", user), { ref: 'master' } + get api("/projects/#{project.id}/pipelines", user), ref: 'master' expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -135,7 +135,7 @@ describe API::Pipelines do context 'when ref does not exist' do it 'returns empty' do - get api("/projects/#{project.id}/pipelines", user), { ref: 'invalid-ref' } + get api("/projects/#{project.id}/pipelines", user), ref: 'invalid-ref' expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -147,7 +147,7 @@ describe API::Pipelines do context 'when name is passed' do context 'when name exists' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines", user), { name: user1.name } + get api("/projects/#{project.id}/pipelines", user), name: user1.name expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -157,7 +157,7 @@ describe API::Pipelines do context 'when name does not exist' do it 'returns empty' do - get api("/projects/#{project.id}/pipelines", user), { name: 'invalid-name' } + get api("/projects/#{project.id}/pipelines", user), name: 'invalid-name' expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -169,7 +169,7 @@ describe API::Pipelines do context 'when username is passed' do context 'when username exists' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines", user), { username: user1.username } + get api("/projects/#{project.id}/pipelines", user), username: user1.username expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -179,7 +179,7 @@ describe API::Pipelines do context 'when username does not exist' do it 'returns empty' do - get api("/projects/#{project.id}/pipelines", user), { username: 'invalid-username' } + get api("/projects/#{project.id}/pipelines", user), username: 'invalid-username' expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -191,7 +191,7 @@ describe API::Pipelines do context 'when yaml_errors is passed' do context 'when yaml_errors is true' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines", user), { yaml_errors: true } + get api("/projects/#{project.id}/pipelines", user), yaml_errors: true expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -201,7 +201,7 @@ describe API::Pipelines do context 'when yaml_errors is false' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines", user), { yaml_errors: false } + get api("/projects/#{project.id}/pipelines", user), yaml_errors: false expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -211,7 +211,7 @@ describe API::Pipelines do context 'when yaml_errors is invalid' do it 'returns 400' do - get api("/projects/#{project.id}/pipelines", user), { yaml_errors: 'invalid-yaml_errors' } + get api("/projects/#{project.id}/pipelines", user), yaml_errors: 'invalid-yaml_errors' expect(response).to have_http_status(400) end @@ -221,7 +221,7 @@ describe API::Pipelines do context 'when order_by and sort are passed' do context 'when order_by and sort are valid' do it 'sorts pipelines' do - get api("/projects/#{project.id}/pipelines", user), { order_by: 'user_id', sort: 'asc' } + get api("/projects/#{project.id}/pipelines", user), order_by: 'user_id', sort: 'asc' expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -235,7 +235,7 @@ describe API::Pipelines do context 'when order_by is invalid' do it 'returns 400' do - get api("/projects/#{project.id}/pipelines", user), { order_by: 'lock_version', sort: 'asc' } + get api("/projects/#{project.id}/pipelines", user), order_by: 'lock_version', sort: 'asc' expect(response).to have_http_status(400) end @@ -243,7 +243,7 @@ describe API::Pipelines do context 'when sort is invalid' do it 'returns 400' do - get api("/projects/#{project.id}/pipelines", user), { order_by: 'id', sort: 'hack' } + get api("/projects/#{project.id}/pipelines", user), order_by: 'id', sort: 'hack' expect(response).to have_http_status(400) end From 6f83553ab6849519699a2e956d4e7a6104c1aa18 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 29 Mar 2017 00:55:55 +0900 Subject: [PATCH 47/57] Add space before bracket --- spec/finders/pipelines_finder_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 01b22b1a044..948a33d379f 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -11,7 +11,7 @@ describe PipelinesFinder do let!(:pipelines) { create_list(:ci_pipeline, 2, project: project) } it 'returns all pipelines' do - is_expected.to eq(pipelines.sort_by{ |p| -p.id }) + is_expected.to eq(pipelines.sort_by { |p| -p.id }) end end @@ -153,7 +153,7 @@ describe PipelinesFinder do let(:params) { { yaml_errors: "invalid-yaml_errors" } } it 'returns all pipelines' do - is_expected.to eq([pipeline1, pipeline2].sort_by{ |p| -p.id }) + is_expected.to eq([pipeline1, pipeline2].sort_by { |p| -p.id }) end end end @@ -164,14 +164,14 @@ describe PipelinesFinder do let!(:pipelines) { create_list(:ci_pipeline, 2, project: project, user: create(:user)) } it 'sorts as user_id: :desc' do - is_expected.to eq(pipelines.sort_by{ |p| p.user.id }) + is_expected.to eq(pipelines.sort_by { |p| p.user.id }) end context 'when sort is invalid' do let(:params) { { order_by: 'user_id', sort: 'invalid_sort' } } it 'sorts as user_id: :desc' do - is_expected.to eq(pipelines.sort_by{ |p| -p.user.id }) + is_expected.to eq(pipelines.sort_by { |p| -p.user.id }) end end end @@ -181,7 +181,7 @@ describe PipelinesFinder do let!(:pipelines) { create_list(:ci_pipeline, 2, project: project) } it 'sorts as id: :asc' do - is_expected.to eq(pipelines.sort_by{ |p| p.id }) + is_expected.to eq(pipelines.sort_by { |p| p.id }) end end @@ -190,7 +190,7 @@ describe PipelinesFinder do let!(:pipelines) { create_list(:ci_pipeline, 2, project: project) } it 'sorts as id: :desc' do - is_expected.to eq(pipelines.sort_by{ |p| -p.id }) + is_expected.to eq(pipelines.sort_by { |p| -p.id }) end end end From 7d48cb015d5590cee85fcce863e1a6bb698ab1e4 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 29 Mar 2017 01:02:17 +0900 Subject: [PATCH 48/57] Add another pipeline for spec status --- spec/finders/pipelines_finder_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 948a33d379f..772c18b22d3 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -61,6 +61,11 @@ describe PipelinesFinder do let(:params) { { status: target } } let!(:pipeline) { create(:ci_pipeline, project: project, status: target) } + before do + exception_status = %w[running pending success failed canceled skipped] - [target] + create(:ci_pipeline, project: project, status: exception_status.sample) + end + it 'returns matched pipelines' do is_expected.to eq([pipeline]) end From fda48d3091cefeb8c981fcb689ca1ed94f841958 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 29 Mar 2017 03:07:11 +0900 Subject: [PATCH 49/57] Improve api/pipelines_spec.rb --- spec/finders/pipelines_finder_spec.rb | 10 +- spec/requests/api/pipelines_spec.rb | 205 ++++++++++++++------------ 2 files changed, 116 insertions(+), 99 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 772c18b22d3..cffe5a46622 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -28,10 +28,14 @@ describe PipelinesFinder do context 'when scope is finished' do let(:params) { { scope: 'finished' } } - let!(:pipeline) { create(:ci_pipeline, project: project, status: 'success') } + let!(:pipelines) do + [create(:ci_pipeline, project: project, status: 'success'), + create(:ci_pipeline, project: project, status: 'failed'), + create(:ci_pipeline, project: project, status: 'canceled')] + end it 'returns matched pipelines' do - is_expected.to eq([pipeline]) + is_expected.to eq(pipelines.sort_by { |p| -p.id }) end end @@ -168,7 +172,7 @@ describe PipelinesFinder do let(:params) { { order_by: 'user_id', sort: 'asc' } } let!(:pipelines) { create_list(:ci_pipeline, 2, project: project, user: create(:user)) } - it 'sorts as user_id: :desc' do + it 'sorts as user_id: :asc' do is_expected.to eq(pipelines.sort_by { |p| p.user.id }) end diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index bbb2dfe1d47..6406b60219c 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -26,55 +26,52 @@ describe API::Pipelines do end context 'when parameter is passed' do - let(:user1) { create(:user) } - let(:user2) { create(:user) } - let(:project) { create(:project, :repository) } - - before do - create(:ci_pipeline, project: project, user: user1, ref: 'v1.0.0', tag: true) - create(:ci_pipeline, project: project, user: user1, status: 'created') - create(:ci_pipeline, project: project, user: user1, status: 'pending') - create(:ci_pipeline, project: project, user: user1, status: 'running') - create(:ci_pipeline, project: project, user: user1, status: 'success') - create(:ci_pipeline, project: project, user: user2, status: 'failed') - create(:ci_pipeline, project: project, user: user2, status: 'canceled') - create(:ci_pipeline, project: project, user: user2, status: 'skipped') - create(:ci_pipeline, project: project, user: user2, yaml_errors: 'Syntax error') - end - - context 'when scope is passed' do - %w[running pending].each do |target| - context "when scope is #{target}" do - it "returns matched pipelines" do - get api("/projects/#{project.id}/pipelines", user), scope: target - - expect(response).to have_http_status(200) - expect(response).to include_pagination_headers - expect(json_response).not_to be_empty - json_response.each { |r| expect(r['status']).to eq(target) } - end + %w[running pending].each do |target| + context "when scope is #{target}" do + before do + create(:ci_pipeline, project: project, status: target) end - end - context 'when scope is finished' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines", user), scope: 'finished' + get api("/projects/#{project.id}/pipelines", user), scope: target - expect(response).to have_http_status(200) + expect(response).to have_http_status(:ok) expect(response).to include_pagination_headers expect(json_response).not_to be_empty - json_response.each { |r| expect(r['status']).to be_in(%w[success failed canceled]) } + json_response.each { |r| expect(r['status']).to eq(target) } end end + end + + context 'when scope is finished' do + before do + create(:ci_pipeline, project: project, status: 'success') + create(:ci_pipeline, project: project, status: 'failed') + create(:ci_pipeline, project: project, status: 'canceled') + end + + it 'returns matched pipelines' do + get api("/projects/#{project.id}/pipelines", user), scope: 'finished' + + expect(response).to have_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response).not_to be_empty + json_response.each { |r| expect(r['status']).to be_in(%w[success failed canceled]) } + end + end + + context 'when scope is branches or tags' do + let!(:pipeline_branch) { create(:ci_pipeline, project: project) } + let!(:pipeline_tag) { create(:ci_pipeline, project: project, ref: 'v1.0.0', tag: true) } context 'when scope is branches' do it 'returns matched pipelines' do get api("/projects/#{project.id}/pipelines", user), scope: 'branches' - expect(response).to have_http_status(200) + expect(response).to have_http_status(:ok) expect(response).to include_pagination_headers expect(json_response).not_to be_empty - expect(json_response.last['sha']).to eq(project.pipelines.where(tag: false).last.sha) + expect(json_response.last['id']).to eq(pipeline_branch.id) end end @@ -82,51 +79,59 @@ describe API::Pipelines do it 'returns matched pipelines' do get api("/projects/#{project.id}/pipelines", user), scope: 'tags' - expect(response).to have_http_status(200) + expect(response).to have_http_status(:ok) expect(response).to include_pagination_headers expect(json_response).not_to be_empty - expect(json_response.last['sha']).to eq(project.pipelines.where(tag: true).last.sha) - end - end - - context 'when scope is invalid' do - it 'returns 400' do - get api("/projects/#{project.id}/pipelines", user), scope: 'invalid-scope' - - expect(response).to have_http_status(400) + expect(json_response.last['id']).to eq(pipeline_tag.id) end end end - context 'when status is passed' do - %w[running pending success failed canceled skipped].each do |target| - context "when status is #{target}" do - it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines", user), status: target + context 'when scope is invalid' do + it 'returns 400' do + get api("/projects/#{project.id}/pipelines", user), scope: 'invalid-scope' - expect(response).to have_http_status(200) - expect(response).to include_pagination_headers - expect(json_response).not_to be_empty - json_response.each { |r| expect(r['status']).to eq(target) } - end - end + expect(response).to have_http_status(:bad_request) end + end - context 'when status is invalid' do - it 'returns 400' do - get api("/projects/#{project.id}/pipelines", user), status: 'invalid-status' + %w[running pending success failed canceled skipped].each do |target| + context "when status is #{target}" do + before do + create(:ci_pipeline, project: project, status: target) + exception_status = %w[running pending success failed canceled skipped] - [target] + create(:ci_pipeline, project: project, status: exception_status.sample) + end - expect(response).to have_http_status(400) + it 'returns matched pipelines' do + get api("/projects/#{project.id}/pipelines", user), status: target + + expect(response).to have_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response).not_to be_empty + json_response.each { |r| expect(r['status']).to eq(target) } end end end - context 'when ref is passed' do + context 'when status is invalid' do + it 'returns :bad_request' do + get api("/projects/#{project.id}/pipelines", user), status: 'invalid-status' + + expect(response).to have_http_status(:bad_request) + end + end + + context 'when ref is specified' do + before do + create(:ci_pipeline, project: project) + end + context 'when ref exists' do it 'returns matched pipelines' do get api("/projects/#{project.id}/pipelines", user), ref: 'master' - expect(response).to have_http_status(200) + expect(response).to have_http_status(:ok) expect(response).to include_pagination_headers expect(json_response).not_to be_empty json_response.each { |r| expect(r['ref']).to eq('master') } @@ -137,21 +142,23 @@ describe API::Pipelines do it 'returns empty' do get api("/projects/#{project.id}/pipelines", user), ref: 'invalid-ref' - expect(response).to have_http_status(200) + expect(response).to have_http_status(:ok) expect(response).to include_pagination_headers expect(json_response).to be_empty end end end - context 'when name is passed' do + context 'when name is specified' do + let!(:pipeline) { create(:ci_pipeline, project: project, user: user) } + context 'when name exists' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines", user), name: user1.name + get api("/projects/#{project.id}/pipelines", user), name: user.name - expect(response).to have_http_status(200) + expect(response).to have_http_status(:ok) expect(response).to include_pagination_headers - expect(json_response.first['sha']).to eq(project.pipelines.where(user: user1).order(id: :desc).first.sha) + expect(json_response.first['id']).to eq(pipeline.id) end end @@ -159,21 +166,23 @@ describe API::Pipelines do it 'returns empty' do get api("/projects/#{project.id}/pipelines", user), name: 'invalid-name' - expect(response).to have_http_status(200) + expect(response).to have_http_status(:ok) expect(response).to include_pagination_headers expect(json_response).to be_empty end end end - context 'when username is passed' do + context 'when username is specified' do + let!(:pipeline) { create(:ci_pipeline, project: project, user: user) } + context 'when username exists' do it 'returns matched pipelines' do - get api("/projects/#{project.id}/pipelines", user), username: user1.username + get api("/projects/#{project.id}/pipelines", user), username: user.username - expect(response).to have_http_status(200) + expect(response).to have_http_status(:ok) expect(response).to include_pagination_headers - expect(json_response.first['sha']).to eq(project.pipelines.where(user: user1).order(id: :desc).first.sha) + expect(json_response.first['id']).to eq(pipeline.id) end end @@ -181,21 +190,24 @@ describe API::Pipelines do it 'returns empty' do get api("/projects/#{project.id}/pipelines", user), username: 'invalid-username' - expect(response).to have_http_status(200) + expect(response).to have_http_status(:ok) expect(response).to include_pagination_headers expect(json_response).to be_empty end end end - context 'when yaml_errors is passed' do + context 'when yaml_errors is specified' do + let!(:pipeline1) { create(:ci_pipeline, project: project, yaml_errors: 'Syntax error') } + let!(:pipeline2) { create(:ci_pipeline, project: project) } + context 'when yaml_errors is true' do it 'returns matched pipelines' do get api("/projects/#{project.id}/pipelines", user), yaml_errors: true - expect(response).to have_http_status(200) + expect(response).to have_http_status(:ok) expect(response).to include_pagination_headers - expect(json_response.first['id']).to eq(project.pipelines.where("yaml_errors IS NOT NULL").order(id: :desc).first.id) + expect(json_response.first['id']).to eq(pipeline1.id) end end @@ -203,49 +215,50 @@ describe API::Pipelines do it 'returns matched pipelines' do get api("/projects/#{project.id}/pipelines", user), yaml_errors: false - expect(response).to have_http_status(200) + expect(response).to have_http_status(:ok) expect(response).to include_pagination_headers - expect(json_response.first['id']).to eq(project.pipelines.where("yaml_errors IS NULL").order(id: :desc).first.id) + expect(json_response.first['id']).to eq(pipeline2.id) end end context 'when yaml_errors is invalid' do - it 'returns 400' do + it 'returns :bad_request' do get api("/projects/#{project.id}/pipelines", user), yaml_errors: 'invalid-yaml_errors' - expect(response).to have_http_status(400) + expect(response).to have_http_status(:bad_request) end end end - context 'when order_by and sort are passed' do - context 'when order_by and sort are valid' do - it 'sorts pipelines' do + context 'when order_by and sort are specified' do + context 'when order_by user_id' do + let!(:pipeline) { create_list(:ci_pipeline, 2, project: project, user: create(:user)) } + + it 'sorts as user_id: :asc' do get api("/projects/#{project.id}/pipelines", user), order_by: 'user_id', sort: 'asc' - expect(response).to have_http_status(200) + expect(response).to have_http_status(:ok) expect(response).to include_pagination_headers expect(json_response).not_to be_empty - pipelines = project.pipelines.order(user_id: :asc) - json_response.each_with_index do |r, i| - expect(r['id']).to eq(pipelines[i].id) + pipeline.sort_by { |p| p.user.id }.tap do |sorted_pipeline| + json_response.each_with_index { |r, i| expect(r['id']).to eq(sorted_pipeline[i].id) } + end + end + + context 'when sort is invalid' do + it 'sorts as user_id: :desc' do + get api("/projects/#{project.id}/pipelines", user), order_by: 'user_id', sort: 'invalid_sort' + + expect(response).to have_http_status(:bad_request) end end end context 'when order_by is invalid' do - it 'returns 400' do + it 'returns :bad_request' do get api("/projects/#{project.id}/pipelines", user), order_by: 'lock_version', sort: 'asc' - expect(response).to have_http_status(400) - end - end - - context 'when sort is invalid' do - it 'returns 400' do - get api("/projects/#{project.id}/pipelines", user), order_by: 'id', sort: 'hack' - - expect(response).to have_http_status(400) + expect(response).to have_http_status(:bad_request) end end end From 4bd0d8e433cdffba9e28a24657104eb2b0b0e761 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 30 Mar 2017 18:39:46 +0900 Subject: [PATCH 50/57] Adopt awesome axil idea --- doc/api/pipelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/pipelines.md b/doc/api/pipelines.md index 06307158e82..733c9479ec2 100644 --- a/doc/api/pipelines.md +++ b/doc/api/pipelines.md @@ -17,8 +17,8 @@ GET /projects/:id/pipelines | `yaml_errors`| boolean | no | Returns pipelines which have an error of gitlab-ci.yml | | `name`| string | no | The name of user who triggered pipelines | | `username`| string | no | The username of user who triggered pipelines | -| `order_by`| string | no | Return requests ordered by `id`, `status`, `ref`, or `user_id` fields. Default is `id` | -| `sort` | string | no | Return requests sorted in `asc` or `desc` order. Default is `desc` | +| `order_by`| string | no | Return requests ordered by `id`, `status`, `ref`, or `user_id`. Default is `id`. Can be combined with `sort`. If you omit `sort`, its default value is used (`desc`) | +| `sort` | string | no | Return requests sorted in `asc` or `desc` order. Default is `desc`. Can be combined with `order_by`. If you omit `order_by`, its default value is used (`id`) | ``` curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects/1/pipelines" From 0a36bfa994582b690a7935fed4c15d42b22bd0ed Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 30 Mar 2017 18:59:45 +0900 Subject: [PATCH 51/57] Use HasStatus::AVAILABLE_STATUSES instead of hard coding --- app/finders/pipelines_finder.rb | 19 +++---------------- lib/api/pipelines.rb | 2 +- spec/finders/pipelines_finder_spec.rb | 4 ++-- spec/requests/api/pipelines_spec.rb | 4 ++-- 4 files changed, 8 insertions(+), 21 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index 7935878d1d5..c6666802b7f 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -54,22 +54,9 @@ class PipelinesFinder end def by_status(items) - case params[:status] - when 'running' - items.running - when 'pending' - items.pending - when 'success' - items.success - when 'failed' - items.failed - when 'canceled' - items.canceled - when 'skipped' - items.skipped - else - items - end + return items unless HasStatus::AVAILABLE_STATUSES.include?(params[:status]) + + items.where(status: params[:status]) end def by_ref(items) diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index b0f586b08da..29757dd9935 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -16,7 +16,7 @@ module API use :pagination optional :scope, type: String, values: %w[running pending finished branches tags], desc: 'The scope of pipelines' - optional :status, type: String, values: %w[running pending success failed canceled skipped], + optional :status, type: String, values: HasStatus::AVAILABLE_STATUSES, desc: 'The status of pipelines' optional :ref, type: String, desc: 'The ref of pipelines' optional :yaml_errors, type: Boolean, desc: 'If true, returns only yaml error pipelines' diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index cffe5a46622..276a680b457 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -60,13 +60,13 @@ describe PipelinesFinder do end end - %w[running pending success failed canceled skipped].each do |target| + HasStatus::AVAILABLE_STATUSES.each do |target| context "when status is #{target}" do let(:params) { { status: target } } let!(:pipeline) { create(:ci_pipeline, project: project, status: target) } before do - exception_status = %w[running pending success failed canceled skipped] - [target] + exception_status = HasStatus::AVAILABLE_STATUSES - [target] create(:ci_pipeline, project: project, status: exception_status.sample) end diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index 6406b60219c..b7d61b2cd7c 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -95,11 +95,11 @@ describe API::Pipelines do end end - %w[running pending success failed canceled skipped].each do |target| + HasStatus::AVAILABLE_STATUSES.each do |target| context "when status is #{target}" do before do create(:ci_pipeline, project: project, status: target) - exception_status = %w[running pending success failed canceled skipped] - [target] + exception_status = HasStatus::AVAILABLE_STATUSES - [target] create(:ci_pipeline, project: project, status: exception_status.sample) end From 8653c2dfc943b5536ab99155c8b950e30ba1f567 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 30 Mar 2017 19:30:02 +0900 Subject: [PATCH 52/57] Add constant as ALLOWED_INDEXED_COLUMNS --- app/finders/pipelines_finder.rb | 4 +++- lib/api/pipelines.rb | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/finders/pipelines_finder.rb b/app/finders/pipelines_finder.rb index c6666802b7f..f187a3b61fe 100644 --- a/app/finders/pipelines_finder.rb +++ b/app/finders/pipelines_finder.rb @@ -1,6 +1,8 @@ class PipelinesFinder attr_reader :project, :pipelines, :params + ALLOWED_INDEXED_COLUMNS = %w[id status ref user_id].freeze + def initialize(project, params = {}) @project = project @pipelines = project.pipelines @@ -95,7 +97,7 @@ class PipelinesFinder end def sort_items(items) - order_by = if %w[id status ref user_id].include?(params[:order_by]) # Allow only indexed columns + order_by = if ALLOWED_INDEXED_COLUMNS.include?(params[:order_by]) params[:order_by] else :id diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 29757dd9935..6a054544d70 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -22,7 +22,7 @@ module API optional :yaml_errors, type: Boolean, desc: 'If true, returns only yaml error pipelines' optional :name, type: String, desc: 'The name of user who triggered pipelines' optional :username, type: String, desc: 'The username of user who triggered pipelines' - optional :order_by, type: String, values: %w[id status ref user_id], default: 'id', + optional :order_by, type: String, values: PipelinesFinder::ALLOWED_INDEXED_COLUMNS, default: 'id', desc: 'The order_by which is combined with a sort' optional :sort, type: String, values: %w[asc desc], default: 'desc', desc: 'The sort method which is combined with an order_by' From dad973b59af297723c20726ef129d0e90b0bca87 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 5 Apr 2017 20:48:59 +0900 Subject: [PATCH 53/57] Revise documents --- ...e-proposal-include-search-options-to-pipelines-api.yml | 2 +- doc/api/pipelines.md | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/changelogs/unreleased/28408-feature-proposal-include-search-options-to-pipelines-api.yml b/changelogs/unreleased/28408-feature-proposal-include-search-options-to-pipelines-api.yml index 6fb3da99fb4..9b9f0032810 100644 --- a/changelogs/unreleased/28408-feature-proposal-include-search-options-to-pipelines-api.yml +++ b/changelogs/unreleased/28408-feature-proposal-include-search-options-to-pipelines-api.yml @@ -1,4 +1,4 @@ --- -title: Resolve Feature Proposal Include search options to pipelines API +title: 'API: Add parameters to allow filtering project pipelines' merge_request: 9367 author: dosuken123 diff --git a/doc/api/pipelines.md b/doc/api/pipelines.md index 733c9479ec2..31a28325a58 100644 --- a/doc/api/pipelines.md +++ b/doc/api/pipelines.md @@ -11,12 +11,12 @@ GET /projects/:id/pipelines | Attribute | Type | Required | Description | |-----------|---------|----------|---------------------| | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | -| `scope` | string | no | The scope of pipelines, one of: `running`, `pending`, `finished`, `branches`, `tags`; | -| `status` | string | no | The status of pipelines, one of: `running`, `pending`, `success`, `failed`, `canceled`, `skipped`; | +| `scope` | string | no | The scope of pipelines, one of: `running`, `pending`, `finished`, `branches`, `tags` | +| `status` | string | no | The status of pipelines, one of: `running`, `pending`, `success`, `failed`, `canceled`, `skipped` | | `ref` | string | no | The ref of pipelines | | `yaml_errors`| boolean | no | Returns pipelines which have an error of gitlab-ci.yml | -| `name`| string | no | The name of user who triggered pipelines | -| `username`| string | no | The username of user who triggered pipelines | +| `name`| string | no | The name of the user who triggered pipelines | +| `username`| string | no | The username of the user who triggered pipelines | | `order_by`| string | no | Return requests ordered by `id`, `status`, `ref`, or `user_id`. Default is `id`. Can be combined with `sort`. If you omit `sort`, its default value is used (`desc`) | | `sort` | string | no | Return requests sorted in `asc` or `desc` order. Default is `desc`. Can be combined with `order_by`. If you omit `order_by`, its default value is used (`id`) | From 41d064659d24d979ab8c1dd282e2162eae318000 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 5 Apr 2017 20:52:02 +0900 Subject: [PATCH 54/57] Avoid using sample --- spec/finders/pipelines_finder_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 276a680b457..3b04100ff57 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -67,7 +67,7 @@ describe PipelinesFinder do before do exception_status = HasStatus::AVAILABLE_STATUSES - [target] - create(:ci_pipeline, project: project, status: exception_status.sample) + create(:ci_pipeline, project: project, status: exception_status.first) end it 'returns matched pipelines' do From 0aebc829ad15558d5b6c1bb2212956fb193a1def Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 5 Apr 2017 21:10:10 +0900 Subject: [PATCH 55/57] Correct typo in pipelines_spec.rb --- spec/requests/api/pipelines_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index b7d61b2cd7c..f9e5316b3de 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -88,7 +88,7 @@ describe API::Pipelines do end context 'when scope is invalid' do - it 'returns 400' do + it 'returns bad_request' do get api("/projects/#{project.id}/pipelines", user), scope: 'invalid-scope' expect(response).to have_http_status(:bad_request) @@ -115,7 +115,7 @@ describe API::Pipelines do end context 'when status is invalid' do - it 'returns :bad_request' do + it 'returns bad_request' do get api("/projects/#{project.id}/pipelines", user), status: 'invalid-status' expect(response).to have_http_status(:bad_request) @@ -222,7 +222,7 @@ describe API::Pipelines do end context 'when yaml_errors is invalid' do - it 'returns :bad_request' do + it 'returns bad_request' do get api("/projects/#{project.id}/pipelines", user), yaml_errors: 'invalid-yaml_errors' expect(response).to have_http_status(:bad_request) @@ -246,7 +246,7 @@ describe API::Pipelines do end context 'when sort is invalid' do - it 'sorts as user_id: :desc' do + it 'returns bad_request' do get api("/projects/#{project.id}/pipelines", user), order_by: 'user_id', sort: 'invalid_sort' expect(response).to have_http_status(:bad_request) @@ -255,7 +255,7 @@ describe API::Pipelines do end context 'when order_by is invalid' do - it 'returns :bad_request' do + it 'returns bad_request' do get api("/projects/#{project.id}/pipelines", user), order_by: 'lock_version', sort: 'asc' expect(response).to have_http_status(:bad_request) From 255bfd658340e36a882108d4a9911d7f9cde638d Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 27 Apr 2017 22:09:18 +0900 Subject: [PATCH 56/57] Improve documentation --- doc/api/pipelines.md | 6 +++--- lib/api/pipelines.rb | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/doc/api/pipelines.md b/doc/api/pipelines.md index 31a28325a58..890945cfc7e 100644 --- a/doc/api/pipelines.md +++ b/doc/api/pipelines.md @@ -14,11 +14,11 @@ GET /projects/:id/pipelines | `scope` | string | no | The scope of pipelines, one of: `running`, `pending`, `finished`, `branches`, `tags` | | `status` | string | no | The status of pipelines, one of: `running`, `pending`, `success`, `failed`, `canceled`, `skipped` | | `ref` | string | no | The ref of pipelines | -| `yaml_errors`| boolean | no | Returns pipelines which have an error of gitlab-ci.yml | +| `yaml_errors`| boolean | no | Returns pipelines with invalid configurations | | `name`| string | no | The name of the user who triggered pipelines | | `username`| string | no | The username of the user who triggered pipelines | -| `order_by`| string | no | Return requests ordered by `id`, `status`, `ref`, or `user_id`. Default is `id`. Can be combined with `sort`. If you omit `sort`, its default value is used (`desc`) | -| `sort` | string | no | Return requests sorted in `asc` or `desc` order. Default is `desc`. Can be combined with `order_by`. If you omit `order_by`, its default value is used (`id`) | +| `order_by`| string | no | Order pipelines by `id`, `status`, `ref`, or `user_id` (default: `id`) | +| `sort` | string | no | Sort pipelines in `asc` or `desc` order (default: `desc`) | ``` curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects/1/pipelines" diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 6a054544d70..9117704aa46 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -19,13 +19,13 @@ module API optional :status, type: String, values: HasStatus::AVAILABLE_STATUSES, desc: 'The status of pipelines' optional :ref, type: String, desc: 'The ref of pipelines' - optional :yaml_errors, type: Boolean, desc: 'If true, returns only yaml error pipelines' - optional :name, type: String, desc: 'The name of user who triggered pipelines' - optional :username, type: String, desc: 'The username of user who triggered pipelines' + optional :yaml_errors, type: Boolean, desc: 'Returns pipelines with invalid configurations' + optional :name, type: String, desc: 'The name of the user who triggered pipelines' + optional :username, type: String, desc: 'The username of the user who triggered pipelines' optional :order_by, type: String, values: PipelinesFinder::ALLOWED_INDEXED_COLUMNS, default: 'id', - desc: 'The order_by which is combined with a sort' + desc: 'Order pipelines' optional :sort, type: String, values: %w[asc desc], default: 'desc', - desc: 'The sort method which is combined with an order_by' + desc: 'Sort pipelines' end get ':id/pipelines' do authorize! :read_pipeline, user_project From 4fe7c25556c7343e46369ffc1e72db1346cc1360 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 27 Apr 2017 22:23:06 +0900 Subject: [PATCH 57/57] Improve pipelines_finder.rb --- spec/finders/pipelines_finder_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index 3b04100ff57..f2aeda241c1 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -11,7 +11,7 @@ describe PipelinesFinder do let!(:pipelines) { create_list(:ci_pipeline, 2, project: project) } it 'returns all pipelines' do - is_expected.to eq(pipelines.sort_by { |p| -p.id }) + is_expected.to match_array(pipelines) end end @@ -35,7 +35,7 @@ describe PipelinesFinder do end it 'returns matched pipelines' do - is_expected.to eq(pipelines.sort_by { |p| -p.id }) + is_expected.to match_array(pipelines) end end @@ -162,7 +162,7 @@ describe PipelinesFinder do let(:params) { { yaml_errors: "invalid-yaml_errors" } } it 'returns all pipelines' do - is_expected.to eq([pipeline1, pipeline2].sort_by { |p| -p.id }) + is_expected.to match_array([pipeline1, pipeline2]) end end end @@ -173,7 +173,7 @@ describe PipelinesFinder do let!(:pipelines) { create_list(:ci_pipeline, 2, project: project, user: create(:user)) } it 'sorts as user_id: :asc' do - is_expected.to eq(pipelines.sort_by { |p| p.user.id }) + is_expected.to match_array(pipelines) end context 'when sort is invalid' do