From 3bfe3febef2903e27b06e319cbf259546cea7841 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Thu, 6 Apr 2017 21:47:14 +0200 Subject: Make MR link in build sidebar bold It adds some consistency compared to the links in the header, which are also bold. --- app/views/projects/builds/_sidebar.html.haml | 3 ++- changelogs/unreleased/tc-job-page-mr-bold.yml | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/tc-job-page-mr-bold.yml diff --git a/app/views/projects/builds/_sidebar.html.haml b/app/views/projects/builds/_sidebar.html.haml index f4a66398c85..694f524faed 100644 --- a/app/views/projects/builds/_sidebar.html.haml +++ b/app/views/projects/builds/_sidebar.html.haml @@ -48,7 +48,8 @@ - if @build.merge_request %p.build-detail-row %span.build-light-text Merge Request: - = link_to "#{@build.merge_request.to_reference}", merge_request_path(@build.merge_request) + = link_to merge_request_path(@build.merge_request) do + %strong= "#{@build.merge_request.to_reference}" - if @build.duration %p.build-detail-row %span.build-light-text Duration: diff --git a/changelogs/unreleased/tc-job-page-mr-bold.yml b/changelogs/unreleased/tc-job-page-mr-bold.yml new file mode 100644 index 00000000000..0243a259119 --- /dev/null +++ b/changelogs/unreleased/tc-job-page-mr-bold.yml @@ -0,0 +1,4 @@ +--- +title: Make MR link in build sidebar bold +merge_request: +author: -- cgit v1.2.1 From 3f2578bce5afbb1f92acab2481ec6de2f38a6296 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 21 Apr 2017 15:09:37 +0100 Subject: Commit view correctly spans the full width when parallel view Closes #30881 --- app/assets/stylesheets/pages/issuable.scss | 11 ++++++++--- app/views/projects/commit/show.html.haml | 4 +++- .../unreleased/commit-limited-container-width.yml | 4 ++++ spec/features/projects/commit/container_spec.rb | 23 ++++++++++++++++++++++ 4 files changed, 38 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/commit-limited-container-width.yml create mode 100644 spec/features/projects/commit/container_spec.rb diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 8d3d1a72b9b..cf8735e6fad 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -6,7 +6,13 @@ } .limit-container-width { - .detail-page-header { + .detail-page-header, + .page-content-header, + .commit-box, + .info-well, + .notes, + .commit-ci-menu, + .files-changed { @extend .fixed-width-container; } @@ -36,8 +42,7 @@ } .diffs { - .mr-version-controls, - .files-changed { + .mr-version-controls { @extend .fixed-width-container; } } diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml index 0d11da2451a..cdf0f11dc5f 100644 --- a/app/views/projects/commit/show.html.haml +++ b/app/views/projects/commit/show.html.haml @@ -1,9 +1,11 @@ - @no_container = true +- container_class = !fluid_layout && diff_view == :inline ? 'container-limited' : '' +- limited_container_width = fluid_layout || diff_view == :inline ? '' : 'limit-container-width' - page_title "#{@commit.title} (#{@commit.short_id})", "Commits" - page_description @commit.description = render "projects/commits/head" -%div{ class: container_class } +%div.container-fluid{ class: [limited_container_width, container_class] } = render "commit_box" - if @commit.status = render "ci_menu" diff --git a/changelogs/unreleased/commit-limited-container-width.yml b/changelogs/unreleased/commit-limited-container-width.yml new file mode 100644 index 00000000000..253646b13da --- /dev/null +++ b/changelogs/unreleased/commit-limited-container-width.yml @@ -0,0 +1,4 @@ +--- +title: Side-by-side view in commits correcly expands full window width +merge_request: +author: diff --git a/spec/features/projects/commit/container_spec.rb b/spec/features/projects/commit/container_spec.rb new file mode 100644 index 00000000000..80b758ec14f --- /dev/null +++ b/spec/features/projects/commit/container_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +describe 'Commit container', :js, :feature do + let(:user) { create(:user) } + let(:project) { create(:project) } + + before do + project.team << [user, :master] + login_as(user) + end + + it 'keeps container-limited when view type is inline' do + visit namespace_project_commit_path(project.namespace, project, project.commit.id, view: :inline) + + expect(page).not_to have_selector('.limit-container-width') + end + + it 'diff spans full width when view type is parallel' do + visit namespace_project_commit_path(project.namespace, project, project.commit.id, view: :parallel) + + expect(page).to have_selector('.limit-container-width') + end +end -- cgit v1.2.1 From 064739431a837ea644851f3f3d01f5da6ee05951 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 24 Apr 2017 16:20:23 +0100 Subject: Fixed HAML lint --- app/views/projects/commit/show.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml index cdf0f11dc5f..16d2646cb4e 100644 --- a/app/views/projects/commit/show.html.haml +++ b/app/views/projects/commit/show.html.haml @@ -5,7 +5,7 @@ - page_description @commit.description = render "projects/commits/head" -%div.container-fluid{ class: [limited_container_width, container_class] } +.container-fluid{ class: [limited_container_width, container_class] } = render "commit_box" - if @commit.status = render "ci_menu" -- cgit v1.2.1 From caadfe4cfdb2df56ea4b8bd104c94e8236fc9f9c Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Mon, 24 Apr 2017 14:59:36 -0500 Subject: Use bold class --- app/views/projects/builds/_sidebar.html.haml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/views/projects/builds/_sidebar.html.haml b/app/views/projects/builds/_sidebar.html.haml index 694f524faed..26c892d0fd2 100644 --- a/app/views/projects/builds/_sidebar.html.haml +++ b/app/views/projects/builds/_sidebar.html.haml @@ -48,8 +48,7 @@ - if @build.merge_request %p.build-detail-row %span.build-light-text Merge Request: - = link_to merge_request_path(@build.merge_request) do - %strong= "#{@build.merge_request.to_reference}" + = link_to "#{@build.merge_request.to_reference}", merge_request_path(@build.merge_request), class: 'bold' - if @build.duration %p.build-detail-row %span.build-light-text Duration: -- cgit v1.2.1 From 967019f7792175c9fc010069657a7e09cf278575 Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Thu, 19 Jan 2017 18:11:12 -0600 Subject: Added a description for non project masters or owners for the members settings page Truncated the project name if it exceeds 18 characters --- app/views/projects/project_members/_index.html.haml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/views/projects/project_members/_index.html.haml b/app/views/projects/project_members/_index.html.haml index ab0771b5751..f83521052ed 100644 --- a/app/views/projects/project_members/_index.html.haml +++ b/app/views/projects/project_members/_index.html.haml @@ -6,6 +6,12 @@ %p Add a new member to %strong= @project.name + - else + %p + Members can be added by project + %i Masters + or + %i Owners .col-lg-9 .light.prepend-top-default - if can?(current_user, :admin_project_member, @project) -- cgit v1.2.1 From 49e9fd05bfbb4dfe15b1698cc00883afdb3ef313 Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Mon, 23 Jan 2017 17:35:31 -0600 Subject: Changed the way to truncate the panel to title from ruby to a scss mixin --- app/assets/stylesheets/framework/common.scss | 4 ++++ app/views/projects/project_members/_team.html.haml | 6 ++++-- changelogs/unreleased/26883-members-page-layout-looks-broken.yml | 4 ++++ 3 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/26883-members-page-layout-looks-broken.yml diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss index 0fd7203e72b..f4f285f0fdc 100644 --- a/app/assets/stylesheets/framework/common.scss +++ b/app/assets/stylesheets/framework/common.scss @@ -425,6 +425,10 @@ table { } .str-truncated { + &-30 { + @include str-truncated(30%); + } + &-60 { @include str-truncated(60%); } diff --git a/app/views/projects/project_members/_team.html.haml b/app/views/projects/project_members/_team.html.haml index 81d57c77edf..b875fa2eec4 100644 --- a/app/views/projects/project_members/_team.html.haml +++ b/app/views/projects/project_members/_team.html.haml @@ -1,7 +1,9 @@ .panel.panel-default .panel-heading - Members with access to - %strong= @project.name + %span.str-truncated-30 + Members of + %strong + #{@project.name} %span.badge= @project_members.total_count = form_tag namespace_project_settings_members_path(@project.namespace, @project), method: :get, class: 'form-inline member-search-form' do .form-group diff --git a/changelogs/unreleased/26883-members-page-layout-looks-broken.yml b/changelogs/unreleased/26883-members-page-layout-looks-broken.yml new file mode 100644 index 00000000000..e0e3a529c3e --- /dev/null +++ b/changelogs/unreleased/26883-members-page-layout-looks-broken.yml @@ -0,0 +1,4 @@ +--- +title: Improved UX on project members settings view +merge_request: +author: -- cgit v1.2.1 From 9d6c769f18dace5699454ff211e8bb679420ddab Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Thu, 26 Jan 2017 14:16:56 -0600 Subject: Added a media query when there's a more width available to show more of the title --- app/assets/stylesheets/framework/common.scss | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss index f4f285f0fdc..1edea8b997f 100644 --- a/app/assets/stylesheets/framework/common.scss +++ b/app/assets/stylesheets/framework/common.scss @@ -427,6 +427,9 @@ table { .str-truncated { &-30 { @include str-truncated(30%); + @media (max-width: $screen-xs-max){ + max-width: 90%; + } } &-60 { -- cgit v1.2.1 From 88c772459e093d78823538fbb18f4942046ae2f1 Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Mon, 24 Apr 2017 13:24:51 -0500 Subject: Added flex wrapping --- app/assets/stylesheets/framework/common.scss | 7 --- app/assets/stylesheets/pages/members.scss | 52 ++++++++++++++++++++++ app/views/projects/project_members/_team.html.haml | 6 +-- 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss index 1edea8b997f..0fd7203e72b 100644 --- a/app/assets/stylesheets/framework/common.scss +++ b/app/assets/stylesheets/framework/common.scss @@ -425,13 +425,6 @@ table { } .str-truncated { - &-30 { - @include str-truncated(30%); - @media (max-width: $screen-xs-max){ - max-width: 90%; - } - } - &-60 { @include str-truncated(60%); } diff --git a/app/assets/stylesheets/pages/members.scss b/app/assets/stylesheets/pages/members.scss index be7193bae04..8dbac76e30a 100644 --- a/app/assets/stylesheets/pages/members.scss +++ b/app/assets/stylesheets/pages/members.scss @@ -133,3 +133,55 @@ right: 160px; } } + +.flex-project-members-panel { + display: flex; + flex-direction: row; + align-items: center; + justify-content: center; + + @media (max-width: $screen-sm-min) { + display: block; + + .flex-project-title { + vertical-align: top; + display: inline-block; + max-width: 90%; + } + } + + .flex-project-title { + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; + } + + .badge { + height: 17px; + line-height: 16px; + margin-right: 5px; + padding-top: 1px; + padding-bottom: 1px; + } + + .flex-project-members-form { + flex-wrap: nowrap; + white-space: nowrap; + margin-left: auto; + } +} + +.panel { + .panel-heading { + .badge { + margin-top: 0; + } + + @media (max-width: $screen-sm-min) { + .badge { + margin-right: 0; + margin-left: 0; + } + } + } +} \ No newline at end of file diff --git a/app/views/projects/project_members/_team.html.haml b/app/views/projects/project_members/_team.html.haml index b875fa2eec4..7b1a26043e1 100644 --- a/app/views/projects/project_members/_team.html.haml +++ b/app/views/projects/project_members/_team.html.haml @@ -1,11 +1,11 @@ .panel.panel-default - .panel-heading - %span.str-truncated-30 + .panel-heading.flex-project-members-panel + %span.flex-project-title Members of %strong #{@project.name} %span.badge= @project_members.total_count - = form_tag namespace_project_settings_members_path(@project.namespace, @project), method: :get, class: 'form-inline member-search-form' do + = form_tag namespace_project_settings_members_path(@project.namespace, @project), method: :get, class: 'form-inline member-search-form flex-project-members-form' do .form-group = search_field_tag :search, params[:search], { placeholder: 'Find existing members by name', class: 'form-control', spellcheck: false } %button.member-search-btn{ type: "submit", "aria-label" => "Submit search" } -- cgit v1.2.1 From 52d59a4e5108d2ffd6f2bc543ee9aef1a87a8f14 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 25 Apr 2017 15:25:20 +0100 Subject: Load milestone tabs asynchronously --- app/assets/javascripts/milestone.js | 29 +++++++++++----- app/controllers/concerns/milestone_actions.rb | 42 +++++++++++++++++++++++ app/controllers/groups/milestones_controller.rb | 4 ++- app/controllers/projects/milestones_controller.rb | 4 ++- app/helpers/milestones_helper.rb | 24 +++++++++++++ app/views/shared/milestones/_tabs.html.haml | 24 ++++++++----- config/routes/group.rb | 8 ++++- config/routes/project.rb | 3 ++ 8 files changed, 118 insertions(+), 20 deletions(-) create mode 100644 app/controllers/concerns/milestone_actions.rb diff --git a/app/assets/javascripts/milestone.js b/app/assets/javascripts/milestone.js index 38c673e8907..1026f458733 100644 --- a/app/assets/javascripts/milestone.js +++ b/app/assets/javascripts/milestone.js @@ -81,9 +81,7 @@ }; function Milestone() { - var oldMouseStart; this.bindIssuesSorting(); - this.bindMergeRequestSorting(); this.bindTabsSwitching(); } @@ -100,13 +98,14 @@ }; Milestone.prototype.bindTabsSwitching = function() { - return $('a[data-toggle="tab"]').on('show.bs.tab', function(e) { - var currentTabClass, previousTabClass; - currentTabClass = $(e.target).data('show'); - previousTabClass = $(e.relatedTarget).data('show'); - $(previousTabClass).hide(); - $(currentTabClass).removeClass('hidden'); - return $(currentTabClass).show(); + return $('a[data-toggle="tab"]').on('show.bs.tab', (e) => { + const $target = $(e.target); + const endpoint = $target.data('endpoint'); + + if (endpoint && !$target.hasClass('is-loaded')) { + this.loadMergeRequests($target.attr('href'), endpoint) + .done(() => $target.addClass('is-loaded')); + } }); }; @@ -169,6 +168,18 @@ }); }; + Milestone.prototype.loadMergeRequests = function(elId, url) { + return $.ajax({ + url, + dataType: 'JSON', + }) + .fail(() => new Flash('Error loading merge requests')) + .done((data) => { + $(elId).html(data.html); + this.bindMergeRequestSorting(); + }); + }; + return Milestone; })(); }).call(window); diff --git a/app/controllers/concerns/milestone_actions.rb b/app/controllers/concerns/milestone_actions.rb new file mode 100644 index 00000000000..c28d08201e0 --- /dev/null +++ b/app/controllers/concerns/milestone_actions.rb @@ -0,0 +1,42 @@ +module MilestoneActions + extend ActiveSupport::Concern + + def merge_requests + respond_to do |format| + format.json do + render json: tabs_json("shared/milestones/_merge_requests_tab", { + merge_requests: @milestone.merge_requests, + show_project_name: true + }) + end + end + end + + def participants + respond_to do |format| + format.json do + render json: tabs_json("shared/milestones/_participants_tab", { + users: @milestone.participants + }) + end + end + end + + def labels + respond_to do |format| + format.json do + render json: tabs_json("shared/milestones/_labels_tab", { + labels: @milestone.labels + }) + end + end + end + + private + + def tabs_json(partial, data = {}) + { + html: view_to_html_string(partial, data) + } + end +end diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb index 43102596201..e52fa766044 100644 --- a/app/controllers/groups/milestones_controller.rb +++ b/app/controllers/groups/milestones_controller.rb @@ -1,6 +1,8 @@ class Groups::MilestonesController < Groups::ApplicationController + include MilestoneActions + before_action :group_projects - before_action :milestone, only: [:show, :update] + before_action :milestone, only: [:show, :update, :merge_requests, :participants, :labels] before_action :authorize_admin_milestones!, only: [:new, :create, :update] def index diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb index d0dd524c484..5c270501a24 100644 --- a/app/controllers/projects/milestones_controller.rb +++ b/app/controllers/projects/milestones_controller.rb @@ -1,6 +1,8 @@ class Projects::MilestonesController < Projects::ApplicationController + include MilestoneActions + before_action :module_enabled - before_action :milestone, only: [:edit, :update, :destroy, :show, :sort_issues, :sort_merge_requests] + before_action :milestone, only: [:edit, :update, :destroy, :show, :sort_issues, :sort_merge_requests, :merge_requests, :participants, :labels] # Allow read any milestone before_action :authorize_read_milestone! diff --git a/app/helpers/milestones_helper.rb b/app/helpers/milestones_helper.rb index c9e70faa52e..d46b010f535 100644 --- a/app/helpers/milestones_helper.rb +++ b/app/helpers/milestones_helper.rb @@ -115,4 +115,28 @@ module MilestonesHelper end end end + + def milestone_merge_request_path(milestone) + if @project + merge_requests_namespace_project_milestone_path(@project.namespace, @project, milestone, format: :json) + elsif @group + merge_requests_group_milestone_path(@group, milestone.safe_title, title: milestone.title, format: :json) + end + end + + def milestone_participants_path(milestone) + if @project + participants_namespace_project_milestone_path(@project.namespace, @project, milestone, format: :json) + elsif @group + participants_group_milestone_path(@group, milestone.safe_title, title: milestone.title, format: :json) + end + end + + def milestone_labels_path(milestone) + if @project + labels_namespace_project_milestone_path(@project.namespace, @project, milestone, format: :json) + elsif @group + labels_group_milestone_path(@group, milestone.safe_title, title: milestone.title, format: :json) + end + end end diff --git a/app/views/shared/milestones/_tabs.html.haml b/app/views/shared/milestones/_tabs.html.haml index 9a4502873ef..bde2db756ce 100644 --- a/app/views/shared/milestones/_tabs.html.haml +++ b/app/views/shared/milestones/_tabs.html.haml @@ -8,20 +8,20 @@ Issues %span.badge= milestone.issues_visible_to_user(current_user).size %li - = link_to '#tab-merge-requests', 'data-toggle' => 'tab', 'data-show' => '.tab-merge-requests-buttons' do + = link_to '#tab-merge-requests', 'data-toggle' => 'tab', 'data-endpoint': milestone_merge_request_path(milestone) do Merge Requests %span.badge= milestone.merge_requests.size - else %li.active - = link_to '#tab-merge-requests', 'data-toggle' => 'tab', 'data-show' => '.tab-merge-requests-buttons' do + = link_to '#tab-merge-requests', 'data-toggle' => 'tab', 'data-endpoint': milestone_merge_request_path(milestone) do Merge Requests %span.badge= milestone.merge_requests.size %li - = link_to '#tab-participants', 'data-toggle' => 'tab' do + = link_to '#tab-participants', 'data-toggle' => 'tab', 'data-endpoint': milestone_participants_path(milestone) do Participants %span.badge= milestone.participants.count %li - = link_to '#tab-labels', 'data-toggle' => 'tab' do + = link_to '#tab-labels', 'data-toggle' => 'tab', 'data-endpoint': milestone_labels_path(milestone) do Labels %span.badge= milestone.labels.count @@ -33,11 +33,19 @@ .tab-pane.active#tab-issues = render 'shared/milestones/issues_tab', issues: milestone.issues_visible_to_user(current_user).include_associations, show_project_name: show_project_name, show_full_project_name: show_full_project_name .tab-pane#tab-merge-requests - = render 'shared/milestones/merge_requests_tab', merge_requests: milestone.merge_requests, show_project_name: show_project_name, show_full_project_name: show_full_project_name + -# loaded async + .text-center.prepend-top-default + = icon('spin spinner 2x') - else .tab-pane.active#tab-merge-requests - = render 'shared/milestones/merge_requests_tab', merge_requests: milestone.merge_requests, show_project_name: show_project_name, show_full_project_name: show_full_project_name + -# loaded async + .text-center.prepend-top-default + = icon('spin spinner 2x') .tab-pane#tab-participants - = render 'shared/milestones/participants_tab', users: milestone.participants + -# loaded async + .text-center.prepend-top-default + = icon('spin spinner 2x') .tab-pane#tab-labels - = render 'shared/milestones/labels_tab', labels: milestone.labels + -# loaded async + .text-center.prepend-top-default + = icon('spin spinner 2x') diff --git a/config/routes/group.rb b/config/routes/group.rb index 73f69d76995..7b29e0e807c 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -10,7 +10,13 @@ scope(path: 'groups/*group_id', end resource :avatar, only: [:destroy] - resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :update, :new, :create] + resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :update, :new, :create] do + member do + get :merge_requests + get :participants + get :labels + end + end resources :labels, except: [:show] do post :toggle_subscription, on: :member diff --git a/config/routes/project.rb b/config/routes/project.rb index fa92202c1ea..e0f40e74500 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -205,6 +205,9 @@ constraints(ProjectUrlConstrainer.new) do member do put :sort_issues put :sort_merge_requests + get :merge_requests + get :participants + get :labels end end -- cgit v1.2.1 From 79c7188a870bbbf4fba294a8d88530fcfe2403be Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 25 Apr 2017 16:06:17 +0100 Subject: Change the hash when changing tab This allows the tab to be loaded by default when the page loads & the hash is present --- app/assets/javascripts/milestone.js | 54 +++++++++++++++------- app/helpers/milestones_helper.rb | 6 +-- app/views/shared/milestones/_tab_loading.html.haml | 2 + app/views/shared/milestones/_tabs.html.haml | 22 ++++----- changelogs/unreleased/async-milestone-tabs.yml | 4 ++ 5 files changed, 55 insertions(+), 33 deletions(-) create mode 100644 app/views/shared/milestones/_tab_loading.html.haml create mode 100644 changelogs/unreleased/async-milestone-tabs.yml diff --git a/app/assets/javascripts/milestone.js b/app/assets/javascripts/milestone.js index 1026f458733..cd6f94e1365 100644 --- a/app/assets/javascripts/milestone.js +++ b/app/assets/javascripts/milestone.js @@ -21,7 +21,7 @@ Milestone.sortIssues = function(data) { var sort_issues_url; - sort_issues_url = location.href + "/sort_issues"; + sort_issues_url = location.pathname + "/sort_issues"; return $.ajax({ type: "PUT", url: sort_issues_url, @@ -38,7 +38,7 @@ Milestone.sortMergeRequests = function(data) { var sort_mr_url; - sort_mr_url = location.href + "/sort_merge_requests"; + sort_mr_url = location.pathname + "/sort_merge_requests"; return $.ajax({ type: "PUT", url: sort_mr_url, @@ -83,6 +83,12 @@ function Milestone() { this.bindIssuesSorting(); this.bindTabsSwitching(); + + this.loadInitialTab(); + + // Load merge request tab if it is active + // merge request tab is active based on different conditions in the backend + this.loadTab($('.js-milestone-tabs .active a')); } Milestone.prototype.bindIssuesSorting = function() { @@ -100,12 +106,9 @@ Milestone.prototype.bindTabsSwitching = function() { return $('a[data-toggle="tab"]').on('show.bs.tab', (e) => { const $target = $(e.target); - const endpoint = $target.data('endpoint'); - if (endpoint && !$target.hasClass('is-loaded')) { - this.loadMergeRequests($target.attr('href'), endpoint) - .done(() => $target.addClass('is-loaded')); - } + location.hash = $target.attr('href'); + this.loadTab($target); }); }; @@ -168,16 +171,33 @@ }); }; - Milestone.prototype.loadMergeRequests = function(elId, url) { - return $.ajax({ - url, - dataType: 'JSON', - }) - .fail(() => new Flash('Error loading merge requests')) - .done((data) => { - $(elId).html(data.html); - this.bindMergeRequestSorting(); - }); + Milestone.prototype.loadInitialTab = function() { + const $target = $(`.js-milestone-tabs a[href="${location.hash}"]`); + + if ($target.length) { + $target.tab('show'); + } + }; + + Milestone.prototype.loadTab = function($target) { + const endpoint = $target.data('endpoint'); + const tabElId = $target.attr('href'); + + if (endpoint && !$target.hasClass('is-loaded')) { + $.ajax({ + url: endpoint, + dataType: 'JSON', + }) + .fail(() => new Flash('Error loading milestone tab')) + .done((data) => { + $(tabElId).html(data.html); + $target.addClass('is-loaded'); + + if (tabElId === '#tab-merge-requests') { + this.bindMergeRequestSorting(); + } + }); + } }; return Milestone; diff --git a/app/helpers/milestones_helper.rb b/app/helpers/milestones_helper.rb index d46b010f535..c515774140c 100644 --- a/app/helpers/milestones_helper.rb +++ b/app/helpers/milestones_helper.rb @@ -116,7 +116,7 @@ module MilestonesHelper end end - def milestone_merge_request_path(milestone) + def milestone_merge_request_tab_path(milestone) if @project merge_requests_namespace_project_milestone_path(@project.namespace, @project, milestone, format: :json) elsif @group @@ -124,7 +124,7 @@ module MilestonesHelper end end - def milestone_participants_path(milestone) + def milestone_participants_tab_path(milestone) if @project participants_namespace_project_milestone_path(@project.namespace, @project, milestone, format: :json) elsif @group @@ -132,7 +132,7 @@ module MilestonesHelper end end - def milestone_labels_path(milestone) + def milestone_labels_tab_path(milestone) if @project labels_namespace_project_milestone_path(@project.namespace, @project, milestone, format: :json) elsif @group diff --git a/app/views/shared/milestones/_tab_loading.html.haml b/app/views/shared/milestones/_tab_loading.html.haml new file mode 100644 index 00000000000..68458c2d0aa --- /dev/null +++ b/app/views/shared/milestones/_tab_loading.html.haml @@ -0,0 +1,2 @@ +.text-center.prepend-top-default + = icon('spin spinner 2x', 'aria-hidden': 'true', 'aria-label': 'Loading tab content') diff --git a/app/views/shared/milestones/_tabs.html.haml b/app/views/shared/milestones/_tabs.html.haml index bde2db756ce..6717d59f033 100644 --- a/app/views/shared/milestones/_tabs.html.haml +++ b/app/views/shared/milestones/_tabs.html.haml @@ -1,27 +1,27 @@ .scrolling-tabs-container.inner-page-scroll-tabs.is-smaller .fade-left= icon('angle-left') .fade-right= icon('angle-right') - %ul.nav-links.scrolling-tabs + %ul.nav-links.scrolling-tabs.js-milestone-tabs - if milestone.is_a?(GlobalMilestone) || can?(current_user, :read_issue, @project) %li.active = link_to '#tab-issues', 'data-toggle' => 'tab', 'data-show' => '.tab-issues-buttons' do Issues %span.badge= milestone.issues_visible_to_user(current_user).size %li - = link_to '#tab-merge-requests', 'data-toggle' => 'tab', 'data-endpoint': milestone_merge_request_path(milestone) do + = link_to '#tab-merge-requests', 'data-toggle' => 'tab', 'data-endpoint': milestone_merge_request_tab_path(milestone) do Merge Requests %span.badge= milestone.merge_requests.size - else %li.active - = link_to '#tab-merge-requests', 'data-toggle' => 'tab', 'data-endpoint': milestone_merge_request_path(milestone) do + = link_to '#tab-merge-requests', 'data-toggle' => 'tab', 'data-endpoint': milestone_merge_request_tab_path(milestone) do Merge Requests %span.badge= milestone.merge_requests.size %li - = link_to '#tab-participants', 'data-toggle' => 'tab', 'data-endpoint': milestone_participants_path(milestone) do + = link_to '#tab-participants', 'data-toggle' => 'tab', 'data-endpoint': milestone_participants_tab_path(milestone) do Participants %span.badge= milestone.participants.count %li - = link_to '#tab-labels', 'data-toggle' => 'tab', 'data-endpoint': milestone_labels_path(milestone) do + = link_to '#tab-labels', 'data-toggle' => 'tab', 'data-endpoint': milestone_labels_tab_path(milestone) do Labels %span.badge= milestone.labels.count @@ -34,18 +34,14 @@ = render 'shared/milestones/issues_tab', issues: milestone.issues_visible_to_user(current_user).include_associations, show_project_name: show_project_name, show_full_project_name: show_full_project_name .tab-pane#tab-merge-requests -# loaded async - .text-center.prepend-top-default - = icon('spin spinner 2x') + = render "shared/milestones/tab_loading" - else .tab-pane.active#tab-merge-requests -# loaded async - .text-center.prepend-top-default - = icon('spin spinner 2x') + = render "shared/milestones/tab_loading" .tab-pane#tab-participants -# loaded async - .text-center.prepend-top-default - = icon('spin spinner 2x') + = render "shared/milestones/tab_loading" .tab-pane#tab-labels -# loaded async - .text-center.prepend-top-default - = icon('spin spinner 2x') + = render "shared/milestones/tab_loading" diff --git a/changelogs/unreleased/async-milestone-tabs.yml b/changelogs/unreleased/async-milestone-tabs.yml new file mode 100644 index 00000000000..c199a95610c --- /dev/null +++ b/changelogs/unreleased/async-milestone-tabs.yml @@ -0,0 +1,4 @@ +--- +title: Load milestone tabs asynchronously to increase initial load performance +merge_request: +author: -- cgit v1.2.1 From 8efa88a306f599f131100296f2b8bb7ae1590741 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 25 Apr 2017 17:41:13 +0100 Subject: Fixed feature spec not waiting for ajax request to finish Fixed 404 in project milestones when not logged in --- app/controllers/projects/milestones_controller.rb | 2 +- spec/features/milestones/milestones_spec.rb | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb index 5c270501a24..c56bce19eee 100644 --- a/app/controllers/projects/milestones_controller.rb +++ b/app/controllers/projects/milestones_controller.rb @@ -8,7 +8,7 @@ class Projects::MilestonesController < Projects::ApplicationController before_action :authorize_read_milestone! # Allow admin milestone - before_action :authorize_admin_milestone!, except: [:index, :show] + before_action :authorize_admin_milestone!, except: [:index, :show, :merge_requests, :participants, :labels] respond_to :html diff --git a/spec/features/milestones/milestones_spec.rb b/spec/features/milestones/milestones_spec.rb index 50d7ca39045..9eec3d7f270 100644 --- a/spec/features/milestones/milestones_spec.rb +++ b/spec/features/milestones/milestones_spec.rb @@ -86,6 +86,9 @@ describe 'Milestone draggable', feature: true, js: true do visit namespace_project_milestone_path(project.namespace, project, milestone) page.find("a[href='#tab-merge-requests']").click + + wait_for_ajax + scroll_into_view('.milestone-content') drag_to(selector: '.merge_requests-sortable-list', list_to_index: 1) -- cgit v1.2.1 From 3c077fad8367264bed3b33fafa537d5c75c4c249 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 25 Apr 2017 17:45:11 +0100 Subject: Fixed tabs loading the ajax request twice --- app/assets/javascripts/milestone.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/milestone.js b/app/assets/javascripts/milestone.js index cd6f94e1365..cd2bd976160 100644 --- a/app/assets/javascripts/milestone.js +++ b/app/assets/javascripts/milestone.js @@ -84,11 +84,11 @@ this.bindIssuesSorting(); this.bindTabsSwitching(); - this.loadInitialTab(); - // Load merge request tab if it is active // merge request tab is active based on different conditions in the backend this.loadTab($('.js-milestone-tabs .active a')); + + this.loadInitialTab(); } Milestone.prototype.bindIssuesSorting = function() { -- cgit v1.2.1 From 45e5d12122ac04d7558c84e47003fc8c85a9dddd Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 26 Apr 2017 08:37:50 +0100 Subject: Fixed spinach tests Removed a spinach test as it wasn't actually testing what it said it should be --- features/group/milestones.feature | 1 + features/project/milestone.feature | 8 -------- features/steps/group/milestones.rb | 3 +++ features/steps/project/project_milestone.rb | 3 +++ 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/features/group/milestones.feature b/features/group/milestones.feature index d6c05df9840..1c1539b3e12 100644 --- a/features/group/milestones.feature +++ b/features/group/milestones.feature @@ -38,6 +38,7 @@ Feature: Group Milestones And I should see the "feature" label And I should see the project name in the Issue row + @javascript Scenario: I should see the Labels tab Given Group has projects with milestones When I visit group "Owned" page diff --git a/features/project/milestone.feature b/features/project/milestone.feature index 713f0f3b979..5e7b211fa27 100644 --- a/features/project/milestone.feature +++ b/features/project/milestone.feature @@ -7,14 +7,6 @@ Feature: Project Milestone And milestone has issue "Bugfix1" with labels: "bug", "feature" And milestone has issue "Bugfix2" with labels: "bug", "enhancement" - - @javascript - Scenario: Listing issues from issues tab - Given I visit project "Shop" milestones page - And I click link "v2.2" - Then I should see the labels "bug", "enhancement" and "feature" - And I should see the "bug" label listed only once - @javascript Scenario: Listing labels from labels tab Given I visit project "Shop" milestones page diff --git a/features/steps/group/milestones.rb b/features/steps/group/milestones.rb index f8f5e3f2382..49fcd6f1201 100644 --- a/features/steps/group/milestones.rb +++ b/features/steps/group/milestones.rb @@ -1,4 +1,5 @@ class Spinach::Features::GroupMilestones < Spinach::FeatureSteps + include WaitForAjax include SharedAuthentication include SharedPaths include SharedGroup @@ -90,6 +91,8 @@ class Spinach::Features::GroupMilestones < Spinach::FeatureSteps end step 'I should see the list of labels' do + wait_for_ajax + page.within('#tab-labels') do expect(page).to have_content 'bug' expect(page).to have_content 'feature' diff --git a/features/steps/project/project_milestone.rb b/features/steps/project/project_milestone.rb index 1864b3a2b52..dc1190b7eea 100644 --- a/features/steps/project/project_milestone.rb +++ b/features/steps/project/project_milestone.rb @@ -2,6 +2,7 @@ class Spinach::Features::ProjectMilestone < Spinach::FeatureSteps include SharedAuthentication include SharedProject include SharedPaths + include WaitForAjax step 'milestone has issue "Bugfix1" with labels: "bug", "feature"' do project = Project.find_by(name: "Shop") @@ -34,6 +35,8 @@ class Spinach::Features::ProjectMilestone < Spinach::FeatureSteps end step 'I should see the labels "bug", "enhancement" and "feature"' do + wait_for_ajax + page.within('#tab-issues') do expect(page).to have_content 'bug' expect(page).to have_content 'enhancement' -- cgit v1.2.1 From 480fcb5533e59151a6e9ae11baef59ebab64cc6c Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 26 Apr 2017 11:32:21 +0100 Subject: Added controller specs --- app/controllers/concerns/milestone_actions.rb | 11 ++++ .../groups/milestones_controller_spec.rb | 12 ++++ .../projects/milestones_controller_spec.rb | 5 ++ spec/support/milestone_tabs_examples.rb | 67 ++++++++++++++++++++++ 4 files changed, 95 insertions(+) create mode 100644 spec/support/milestone_tabs_examples.rb diff --git a/app/controllers/concerns/milestone_actions.rb b/app/controllers/concerns/milestone_actions.rb index c28d08201e0..2cc7e15c27d 100644 --- a/app/controllers/concerns/milestone_actions.rb +++ b/app/controllers/concerns/milestone_actions.rb @@ -3,6 +3,7 @@ module MilestoneActions def merge_requests respond_to do |format| + format.html { redirect_to milestone_path } format.json do render json: tabs_json("shared/milestones/_merge_requests_tab", { merge_requests: @milestone.merge_requests, @@ -14,6 +15,7 @@ module MilestoneActions def participants respond_to do |format| + format.html { redirect_to milestone_path } format.json do render json: tabs_json("shared/milestones/_participants_tab", { users: @milestone.participants @@ -24,6 +26,7 @@ module MilestoneActions def labels respond_to do |format| + format.html { redirect_to milestone_path } format.json do render json: tabs_json("shared/milestones/_labels_tab", { labels: @milestone.labels @@ -39,4 +42,12 @@ module MilestoneActions html: view_to_html_string(partial, data) } end + + def milestone_path + if @project + namespace_project_milestone_path(@project.namespace, @project, @milestone) + else + group_milestone_path(@group, @milestone.safe_title, title: @milestone.title) + end + end end diff --git a/spec/controllers/groups/milestones_controller_spec.rb b/spec/controllers/groups/milestones_controller_spec.rb index 6e4b5f78e33..7cf2996ffd0 100644 --- a/spec/controllers/groups/milestones_controller_spec.rb +++ b/spec/controllers/groups/milestones_controller_spec.rb @@ -6,6 +6,16 @@ describe Groups::MilestonesController do let(:project2) { create(:empty_project, group: group) } let(:user) { create(:user) } let(:title) { '肯定不是中文的问题' } + let(:milestone) do + project_milestone = create(:milestone, project: project) + + GroupMilestone.build( + group, + [project], + project_milestone.title + ) + end + let(:milestone_path) { group_milestone_path(group, milestone.safe_title, title: milestone.title) } before do sign_in(user) @@ -14,6 +24,8 @@ describe Groups::MilestonesController do controller.instance_variable_set(:@group, group) end + it_behaves_like 'milestone tabs' + describe "#create" do it "creates group milestone with Chinese title" do post :create, diff --git a/spec/controllers/projects/milestones_controller_spec.rb b/spec/controllers/projects/milestones_controller_spec.rb index 47e61c3cea8..6f500468f95 100644 --- a/spec/controllers/projects/milestones_controller_spec.rb +++ b/spec/controllers/projects/milestones_controller_spec.rb @@ -7,6 +7,7 @@ describe Projects::MilestonesController do let(:issue) { create(:issue, project: project, milestone: milestone) } let!(:label) { create(:label, project: project, title: 'Issue Label', issues: [issue]) } let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, milestone: milestone) } + let(:milestone_path) { namespace_project_milestone_path } before do sign_in(user) @@ -14,6 +15,8 @@ describe Projects::MilestonesController do controller.instance_variable_set(:@project, project) end + it_behaves_like 'milestone tabs' + describe "#show" do render_views @@ -49,4 +52,6 @@ describe Projects::MilestonesController do expect(last_note).to eq('removed milestone') end end + + end diff --git a/spec/support/milestone_tabs_examples.rb b/spec/support/milestone_tabs_examples.rb new file mode 100644 index 00000000000..c48ecb1672e --- /dev/null +++ b/spec/support/milestone_tabs_examples.rb @@ -0,0 +1,67 @@ +shared_examples 'milestone tabs' do + def go(path, extra_params = {}) + params = if milestone.is_a?(GlobalMilestone) + { group_id: group.id, id: milestone.safe_title, title: milestone.title } + else + { namespace_id: project.namespace.to_param, project_id: project, id: milestone.iid } + end + + get path, params.merge(extra_params) + end + describe '#merge_requests' do + context 'as html' do + before { go(:merge_requests, format: 'html') } + + it 'redirects to milestone#show' do + expect(response).to redirect_to(milestone_path) + end + end + + context 'as json' do + before { go(:merge_requests, format: 'json') } + + it 'renders the merge requests tab template to a string' do + expect(response).to render_template('shared/milestones/_merge_requests_tab') + expect(json_response).to have_key('html') + end + end + end + + describe '#participants' do + context 'as html' do + before { go(:participants, format: 'html') } + + it 'redirects to milestone#show' do + expect(response).to redirect_to(milestone_path) + end + end + + context 'as json' do + before { go(:participants, format: 'json') } + + it 'renders the participants tab template to a string' do + expect(response).to render_template('shared/milestones/_participants_tab') + expect(json_response).to have_key('html') + end + end + end + + describe '#labels' do + context 'as html' do + before { go(:labels, format: 'html') } + + it 'redirects to milestone#show' do + expect(response).to redirect_to(milestone_path) + end + end + + context 'as json' do + before { go(:labels, format: 'json') } + + it 'renders the labels tab template to a string' do + expect(response).to render_template('shared/milestones/_labels_tab') + expect(json_response).to have_key('html') + end + end + end +end -- cgit v1.2.1 From 4b812d979eafa244c9e6b014448bf09bf1b458fd Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 26 Apr 2017 12:22:09 +0100 Subject: Fixed failing specs --- app/controllers/concerns/milestone_actions.rb | 8 ++++---- spec/controllers/projects/milestones_controller_spec.rb | 2 -- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/app/controllers/concerns/milestone_actions.rb b/app/controllers/concerns/milestone_actions.rb index 2cc7e15c27d..3e2a0fe4f8b 100644 --- a/app/controllers/concerns/milestone_actions.rb +++ b/app/controllers/concerns/milestone_actions.rb @@ -3,7 +3,7 @@ module MilestoneActions def merge_requests respond_to do |format| - format.html { redirect_to milestone_path } + format.html { redirect_to milestone_redirect_path } format.json do render json: tabs_json("shared/milestones/_merge_requests_tab", { merge_requests: @milestone.merge_requests, @@ -15,7 +15,7 @@ module MilestoneActions def participants respond_to do |format| - format.html { redirect_to milestone_path } + format.html { redirect_to milestone_redirect_path } format.json do render json: tabs_json("shared/milestones/_participants_tab", { users: @milestone.participants @@ -26,7 +26,7 @@ module MilestoneActions def labels respond_to do |format| - format.html { redirect_to milestone_path } + format.html { redirect_to milestone_redirect_path } format.json do render json: tabs_json("shared/milestones/_labels_tab", { labels: @milestone.labels @@ -43,7 +43,7 @@ module MilestoneActions } end - def milestone_path + def milestone_redirect_path if @project namespace_project_milestone_path(@project.namespace, @project, @milestone) else diff --git a/spec/controllers/projects/milestones_controller_spec.rb b/spec/controllers/projects/milestones_controller_spec.rb index 6f500468f95..84a61b2784e 100644 --- a/spec/controllers/projects/milestones_controller_spec.rb +++ b/spec/controllers/projects/milestones_controller_spec.rb @@ -52,6 +52,4 @@ describe Projects::MilestonesController do expect(last_note).to eq('removed milestone') end end - - end -- cgit v1.2.1 From 471888d60e0d91f4ab75e5d773bd69dee85e6cea Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 28 Apr 2017 11:08:18 +0100 Subject: Moved sort endpoints into data attributes --- app/assets/javascripts/milestone.js | 27 +++++++++++++++++---------- app/views/shared/milestones/_tabs.html.haml | 6 +++--- spec/support/milestone_tabs_examples.rb | 1 + 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/milestone.js b/app/assets/javascripts/milestone.js index cd2bd976160..841b24a60a3 100644 --- a/app/assets/javascripts/milestone.js +++ b/app/assets/javascripts/milestone.js @@ -19,12 +19,10 @@ }); }; - Milestone.sortIssues = function(data) { - var sort_issues_url; - sort_issues_url = location.pathname + "/sort_issues"; + Milestone.sortIssues = function(url, data) { return $.ajax({ type: "PUT", - url: sort_issues_url, + url, data: data, success: function(_data) { return Milestone.successCallback(_data); @@ -36,12 +34,10 @@ }); }; - Milestone.sortMergeRequests = function(data) { - var sort_mr_url; - sort_mr_url = location.pathname + "/sort_merge_requests"; + Milestone.sortMergeRequests = function(url, data) { return $.ajax({ type: "PUT", - url: sort_mr_url, + url, data: data, success: function(_data) { return Milestone.successCallback(_data); @@ -81,6 +77,9 @@ }; function Milestone() { + this.issuesSortEndpoint = $('#tab-issues').data('sort-endpoint'); + this.mergeRequestsSortEndpoint = $('#tab-merge-requests').data('sort-endpoint'); + this.bindIssuesSorting(); this.bindTabsSwitching(); @@ -92,12 +91,16 @@ } Milestone.prototype.bindIssuesSorting = function() { + if (!this.issuesSortEndpoint) return; + $('#issues-list-unassigned, #issues-list-ongoing, #issues-list-closed').each(function (i, el) { this.createSortable(el, { group: 'issue-list', listEls: $('.issues-sortable-list'), fieldName: 'issue', - sortCallback: Milestone.sortIssues, + sortCallback: (data) => { + Milestone.sortIssues(this.issuesSortEndpoint, data); + }, updateCallback: Milestone.updateIssue, }); }.bind(this)); @@ -113,12 +116,16 @@ }; Milestone.prototype.bindMergeRequestSorting = function() { + if (!this.mergeRequestsSortEndpoint) return; + $("#merge_requests-list-unassigned, #merge_requests-list-ongoing, #merge_requests-list-closed").each(function (i, el) { this.createSortable(el, { group: 'merge-request-list', listEls: $(".merge_requests-sortable-list:not(#merge_requests-list-merged)"), fieldName: 'merge_request', - sortCallback: Milestone.sortMergeRequests, + sortCallback: (data) => { + Milestone.sortMergeRequests(this.mergeRequestsSortEndpoint, data); + }, updateCallback: Milestone.updateMergeRequest, }); }.bind(this)); diff --git a/app/views/shared/milestones/_tabs.html.haml b/app/views/shared/milestones/_tabs.html.haml index 6717d59f033..6a6d817b344 100644 --- a/app/views/shared/milestones/_tabs.html.haml +++ b/app/views/shared/milestones/_tabs.html.haml @@ -30,13 +30,13 @@ .tab-content.milestone-content - if milestone.is_a?(GlobalMilestone) || can?(current_user, :read_issue, @project) - .tab-pane.active#tab-issues + .tab-pane.active#tab-issues{ data: { sort_endpoint: (sort_issues_namespace_project_milestone_path(@project.namespace, @project, @milestone) if @project && current_user) } } = render 'shared/milestones/issues_tab', issues: milestone.issues_visible_to_user(current_user).include_associations, show_project_name: show_project_name, show_full_project_name: show_full_project_name - .tab-pane#tab-merge-requests + .tab-pane#tab-merge-requests{ data: { sort_endpoint: (sort_merge_requests_namespace_project_milestone_path(@project.namespace, @project, @milestone) if @project && current_user) } } -# loaded async = render "shared/milestones/tab_loading" - else - .tab-pane.active#tab-merge-requests + .tab-pane.active#tab-merge-requests{ data: { sort_endpoint: (sort_merge_requests_namespace_project_milestone_path(@project.namespace, @project, @milestone) if @project && current_user) } } -# loaded async = render "shared/milestones/tab_loading" .tab-pane#tab-participants diff --git a/spec/support/milestone_tabs_examples.rb b/spec/support/milestone_tabs_examples.rb index c48ecb1672e..c69f8e11008 100644 --- a/spec/support/milestone_tabs_examples.rb +++ b/spec/support/milestone_tabs_examples.rb @@ -8,6 +8,7 @@ shared_examples 'milestone tabs' do get path, params.merge(extra_params) end + describe '#merge_requests' do context 'as html' do before { go(:merge_requests, format: 'html') } -- cgit v1.2.1 From 7fd3a5883e5a6135873f2e6def9dabf2ab82f361 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 28 Apr 2017 12:11:30 +0100 Subject: Changed spec to a view spec --- spec/features/projects/commit/container_spec.rb | 23 ------------ spec/views/projects/commit/show.html.haml_spec.rb | 45 +++++++++++++++++++++++ 2 files changed, 45 insertions(+), 23 deletions(-) delete mode 100644 spec/features/projects/commit/container_spec.rb create mode 100644 spec/views/projects/commit/show.html.haml_spec.rb diff --git a/spec/features/projects/commit/container_spec.rb b/spec/features/projects/commit/container_spec.rb deleted file mode 100644 index 80b758ec14f..00000000000 --- a/spec/features/projects/commit/container_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -require 'spec_helper' - -describe 'Commit container', :js, :feature do - let(:user) { create(:user) } - let(:project) { create(:project) } - - before do - project.team << [user, :master] - login_as(user) - end - - it 'keeps container-limited when view type is inline' do - visit namespace_project_commit_path(project.namespace, project, project.commit.id, view: :inline) - - expect(page).not_to have_selector('.limit-container-width') - end - - it 'diff spans full width when view type is parallel' do - visit namespace_project_commit_path(project.namespace, project, project.commit.id, view: :parallel) - - expect(page).to have_selector('.limit-container-width') - end -end diff --git a/spec/views/projects/commit/show.html.haml_spec.rb b/spec/views/projects/commit/show.html.haml_spec.rb new file mode 100644 index 00000000000..abbdc65ea71 --- /dev/null +++ b/spec/views/projects/commit/show.html.haml_spec.rb @@ -0,0 +1,45 @@ +require 'spec_helper' + +describe 'projects/commit/show.html.haml', :view do + let(:user) { create(:user) } + let(:project) { create(:project, :repository) } + + before do + assign(:project, project) + assign(:repository, project.repository) + assign(:commit, project.commit) + assign(:noteable, project.commit) + assign(:notes, []) + assign(:diffs, project.commit.diffs) + + allow(view).to receive(:current_user).and_return(user) + allow(view).to receive(:can?).and_return(false) + allow(view).to receive(:can_collaborate_with_project?).and_return(false) + allow(view).to receive(:current_ref).and_return(project.repository.root_ref) + allow(view).to receive(:diff_btn).and_return('') + end + + context 'inline diff view' do + before do + allow(view).to receive(:diff_view).and_return(:inline) + + render + end + + it 'keeps container-limited' do + expect(rendered).not_to have_selector('.limit-container-width') + end + end + + context 'parallel diff view' do + before do + allow(view).to receive(:diff_view).and_return(:parallel) + + render + end + + it 'spans full width' do + expect(rendered).to have_selector('.limit-container-width') + end + end +end -- cgit v1.2.1 From ce5f7008fbd697e1af4a0bcf91b8a0d7c26cb643 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 2 May 2017 16:02:58 +0100 Subject: Fixed view specs --- spec/views/projects/commit/show.html.haml_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/views/projects/commit/show.html.haml_spec.rb b/spec/views/projects/commit/show.html.haml_spec.rb index abbdc65ea71..122075cc10e 100644 --- a/spec/views/projects/commit/show.html.haml_spec.rb +++ b/spec/views/projects/commit/show.html.haml_spec.rb @@ -1,7 +1,6 @@ require 'spec_helper' describe 'projects/commit/show.html.haml', :view do - let(:user) { create(:user) } let(:project) { create(:project, :repository) } before do @@ -12,7 +11,7 @@ describe 'projects/commit/show.html.haml', :view do assign(:notes, []) assign(:diffs, project.commit.diffs) - allow(view).to receive(:current_user).and_return(user) + allow(view).to receive(:current_user).and_return(nil) allow(view).to receive(:can?).and_return(false) allow(view).to receive(:can_collaborate_with_project?).and_return(false) allow(view).to receive(:current_ref).and_return(project.repository.root_ref) -- cgit v1.2.1 From e9d94451959e4717e0ba4ca882b5a54437efbee1 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 21 Feb 2017 17:20:50 +0900 Subject: - Add new parameters for Pipeline API - Expand PipelinesFinder functions --- app/controllers/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 -- cgit v1.2.1 From 01f6083939f8f9559f7e134f111bd40d17d357f9 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 21 Feb 2017 18:19:27 +0900 Subject: 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]) -- cgit v1.2.1 From 994e49b3fbc261f8e59429c1681d83c81ba25df3 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 28 Feb 2017 21:24:49 +0900 Subject: Fixed those points. - username to user_id - Drop duration - Resolve comments - Add Changelog - Edit docs --- app/finders/pipelines_finder.rb | 27 ++++++++-------------- ...sal-include-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 -- cgit v1.2.1 From df834306c1794ed72d6d655c7941dee28f7e85c7 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 1 Mar 2017 02:34:48 +0900 Subject: 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 | 218 +++++++++++++++++++++++++++++++--- 4 files changed, 209 insertions(+), 21 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 nothing is passed' 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 + 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 scope is nil' do - let(:params) { { scope: nil } } + 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 'selects all pipelines' do - expect(subject.count).to be 2 - expect(subject).to include tag_pipeline - expect(subject).to include branch_pipeline + 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 '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 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 'orders in descending order on ID' do - feature_pipeline = create(:ci_pipeline, project: project, ref: 'feature') + it 'has only canceled status' do + expect(subject.map(&:status)).to include('canceled') + 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 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 -- cgit v1.2.1 From fd302061f915f535b2dd419d5a76efb76ab534be Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 1 Mar 2017 15:58:06 +0900 Subject: 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 -- cgit v1.2.1 From d15c120f72a375b6c9634e9aaeaeb159bb0c1998 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 1 Mar 2017 18:07:39 +0900 Subject: 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 } -- cgit v1.2.1 From a114c988b4c17e37508f1d0f96b93fd8b96d2df9 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 1 Mar 2017 21:43:25 +0900 Subject: 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 -- cgit v1.2.1 From f0bc9af36b716066347549de4c0a5a3ec997a983 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 2 Mar 2017 17:43:37 +0900 Subject: 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 -- cgit v1.2.1 From e3fd8caf94c4d5ca0b6bc086d43effc2fb7c5792 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 2 Mar 2017 18:41:27 +0900 Subject: Improved CI. Fix yaml_errors boolean evaluation. --- app/finders/pipelines_finder.rb | 2 +- spec/finders/pipelines_finder_spec.rb | 95 ++++++++++++++--------------------- 2 files changed, 40 insertions(+), 57 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!(: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!(: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'), + 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 -- cgit v1.2.1 From fa64da65e7205b101569a3e515a0a65ae2d679c9 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 21:35:15 +0900 Subject: 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 -- cgit v1.2.1 From 7e421e55233bbb26594de2a88e62854b468fb02a Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 21:36:08 +0900 Subject: 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" -- cgit v1.2.1 From af7cb5b93191453de4ff7561089bb0b0146c688f Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 21:36:45 +0900 Subject: %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 -- cgit v1.2.1 From ac00cd1021cd4e2d6b7a11683b0af61065918a1c Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 21:37:14 +0900 Subject: %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 -- cgit v1.2.1 From d33e762167a197f30d075ed409ca1a07ae9f83c6 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 22:02:29 +0900 Subject: %[] 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 -- cgit v1.2.1 From 40e8940195a05bf2e3802550a571f889853c0200 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 22:19:15 +0900 Subject: 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 -- cgit v1.2.1 From 8d85887c3879ec162c8658b37f894a65edc27104 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 23:27:49 +0900 Subject: 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 -- cgit v1.2.1 From 658f2cb148b63b284fe9ce346cb149c9e6986b9b Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 7 Mar 2017 23:31:32 +0900 Subject: 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'), -- cgit v1.2.1 From 44ae99399f75f0b23e9d78eb4217fad4911ccbca Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 02:44:38 +0900 Subject: Use ActiveRecord for comparison --- spec/finders/pipelines_finder_spec.rb | 86 +++++++++++++++-------------------- 1 file changed, 36 insertions(+), 50 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!(: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'), - } + 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 } @@ -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 -- cgit v1.2.1 From 56f50cbb3a63ae914f50eda3756b49d5cf516207 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 17:24:20 +0900 Subject: 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 -- cgit v1.2.1 From a46c91f43225931f94a7d3c7f47beef82152f0ce Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 17:33:10 +0900 Subject: 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) -- cgit v1.2.1 From 83d02a0b609ea71ef8448b9012221962dda69aba Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 18:19:11 +0900 Subject: 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)) -- cgit v1.2.1 From 175800299bf497591e625e82fd71420644c0bc6b Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 20:24:00 +0900 Subject: 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)) -- cgit v1.2.1 From 284cf0bfb5d56dd847e48b74b6ae6c2627e164d1 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 21:05:33 +0900 Subject: 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 -- cgit v1.2.1 From 9324a464ef456dff3670df227c10d5cdec473256 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 8 Mar 2017 21:18:09 +0900 Subject: 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' -- cgit v1.2.1 From 171229d4b1501780b2a5e7a2eab9c714addcadea Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 9 Mar 2017 02:20:00 +0900 Subject: 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' } } -- cgit v1.2.1 From 959b4e99c0a81ad7b974ab3871f4b1857da9bfc4 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 9 Mar 2017 02:20:50 +0900 Subject: 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 -- cgit v1.2.1 From d74a04c499d511d6fec8d1f0ddcfc087596b9224 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 14 Mar 2017 23:47:06 +0900 Subject: 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 -- cgit v1.2.1 From d1ca5f46d4268ca3bba7801e581395e038c97129 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 14 Mar 2017 23:52:17 +0900 Subject: 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' -- cgit v1.2.1 From 2894f293a28f06d22c392a6bc2bf19403b496fe0 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 15 Mar 2017 00:01:18 +0900 Subject: 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 -- cgit v1.2.1 From 2075d7ce52ea9d5885fb1bb1ca70eee2e7898c31 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 15 Mar 2017 00:10:37 +0900 Subject: 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 -- cgit v1.2.1 From 2e43e50e742e96a60ef00e1431f39a80a11ca168 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 15 Mar 2017 01:33:25 +0900 Subject: Finish pipelines_spec --- spec/finders/pipelines_finder_spec.rb | 2 +- spec/requests/api/pipelines_spec.rb | 149 ++++++++++++++++++++++++---------- 2 files changed, 105 insertions(+), 46 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) } + 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 + 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 expect(json_response.count).to be > 0 - json_response.each { |r| expect(r['status']).to eq(target) } + json_response.each { |r| expect(r['ref']).to eq('master') } 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 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 - 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 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 -- cgit v1.2.1 From 974c3c132f5241cf58103e5e01e8b54256749a87 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 15 Mar 2017 01:42:31 +0900 Subject: 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 -- cgit v1.2.1 From f0e3076a32f1f127ef4d31b53979edd5e0218469 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 17 Mar 2017 17:59:13 +0900 Subject: '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 -- cgit v1.2.1 From 3735b8aaa1f48ea3803e31e18f1e40d2fd091b26 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 17 Mar 2017 18:27:11 +0900 Subject: 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 -- cgit v1.2.1 From e72c50fcb2f60516284fb66aa322cd0880cbda80 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 17 Mar 2017 18:36:10 +0900 Subject: 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 -- cgit v1.2.1 From c79c389525d8f31dfc9119a1eb6e9a41585facb9 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 17 Mar 2017 22:22:50 +0900 Subject: 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 -- cgit v1.2.1 From 7fb3a78a6d23b1fe0b14fab30e1fac4ec8d27d85 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 17 Mar 2017 22:34:38 +0900 Subject: 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 -- cgit v1.2.1 From 8f32724fcb7f05052b53dcd365a064ad87a9535e Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 17 Mar 2017 23:50:20 +0900 Subject: 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 -- cgit v1.2.1 From 98ac988d4d9525b3f07a19e495a9c2666ebee3c6 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sun, 19 Mar 2017 02:50:42 +0900 Subject: 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 -- cgit v1.2.1 From 22a4d124f70598c7c21b08b11db3f38c7bcb71ec Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 22 Mar 2017 02:37:19 +0900 Subject: 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 -- cgit v1.2.1 From 0e8266f2386351906e2d6357282e011d373b2c94 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 24 Mar 2017 16:06:19 +0900 Subject: 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 -- cgit v1.2.1 From f5f7f90abe6bc0f4e19e0abace72c8b1fd69f519 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 24 Mar 2017 17:30:26 +0900 Subject: 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` | -- cgit v1.2.1 From f7b5800a1dc6a2a59758aea502a2ddb8f103634b Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 27 Mar 2017 14:51:28 +0900 Subject: 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 -- cgit v1.2.1 From 33c284fa8cdfe10f20334866b1043b4f9350f055 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 27 Mar 2017 15:14:52 +0900 Subject: 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 -- cgit v1.2.1 From f65cd3289cb555d008d7792635f12b6273c65cfb Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 27 Mar 2017 19:12:09 +0900 Subject: Improve pipelines_finder_spec.rb --- spec/finders/pipelines_finder_spec.rb | 183 +++++++++++++--------------------- 1 file changed, 70 insertions(+), 113 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' } } - - it 'returns matched pipelines' do - expect(subject).to match_array(project.pipelines.running) - end - end - - context 'when status is pending' do - let(:params) { { status: 'pending' } } - - 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' } } + %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.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) + is_expected.to eq([pipeline]) end end + end - context 'when status is skipped' do - let(:params) { { status: 'skipped' } } + 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.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 - end - context 'when order_by is invalid' do - let(:params) { { order_by: 'invalid_column', sort: 'asc' } } + context 'when sort is invalid' do + let(:params) { { order_by: 'user_id', sort: 'invalid_sort' } } - it 'sorts pipelines with id: (default)' do - expect(subject).to eq(project.pipelines.order(id: :asc)) + it 'sorts as user_id: :desc' do + is_expected.to eq(pipelines.sort_by{ |p| -p.user.id }) + end end end - context 'when sort is invalid' do - let(:params) { { order_by: 'user_id', sort: 'invalid_sort' } } + 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 :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 -- cgit v1.2.1 From 673693888ad5fcee2f7b7af66e02b7ec0a3c7f95 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 27 Mar 2017 19:15:21 +0900 Subject: 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 -- cgit v1.2.1 From 6f83553ab6849519699a2e956d4e7a6104c1aa18 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 29 Mar 2017 00:55:55 +0900 Subject: 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 -- cgit v1.2.1 From 7d48cb015d5590cee85fcce863e1a6bb698ab1e4 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 29 Mar 2017 01:02:17 +0900 Subject: 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 -- cgit v1.2.1 From fda48d3091cefeb8c981fcb689ca1ed94f841958 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 29 Mar 2017 03:07:11 +0900 Subject: Improve api/pipelines_spec.rb --- spec/finders/pipelines_finder_spec.rb | 10 +- spec/requests/api/pipelines_spec.rb | 197 ++++++++++++++++++---------------- 2 files changed, 112 insertions(+), 95 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) + expect(json_response.last['id']).to eq(pipeline_tag.id) end end + end - context 'when scope is invalid' do - it 'returns 400' do - get api("/projects/#{project.id}/pipelines", user), scope: 'invalid-scope' + 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) - end + expect(response).to have_http_status(:bad_request) 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 + %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(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 + 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 status is invalid' do - it 'returns 400' do - get api("/projects/#{project.id}/pipelines", user), status: 'invalid-status' + 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(400) - end + expect(response).to have_http_status(:bad_request) end end - context 'when ref is passed' do + 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 - end - context 'when order_by is invalid' do - it 'returns 400' do - get api("/projects/#{project.id}/pipelines", user), order_by: 'lock_version', sort: 'asc' + 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(400) + expect(response).to have_http_status(:bad_request) + end end end - context 'when sort is invalid' do - it 'returns 400' do - get api("/projects/#{project.id}/pipelines", user), order_by: 'id', sort: 'hack' + context 'when order_by is invalid' 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) + expect(response).to have_http_status(:bad_request) end end end -- cgit v1.2.1 From 4bd0d8e433cdffba9e28a24657104eb2b0b0e761 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 30 Mar 2017 18:39:46 +0900 Subject: 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" -- cgit v1.2.1 From 0a36bfa994582b690a7935fed4c15d42b22bd0ed Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 30 Mar 2017 18:59:45 +0900 Subject: 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 -- cgit v1.2.1 From 8653c2dfc943b5536ab99155c8b950e30ba1f567 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 30 Mar 2017 19:30:02 +0900 Subject: 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' -- cgit v1.2.1 From dad973b59af297723c20726ef129d0e90b0bca87 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 5 Apr 2017 20:48:59 +0900 Subject: Revise documents --- ...8-feature-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`) | -- cgit v1.2.1 From 41d064659d24d979ab8c1dd282e2162eae318000 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 5 Apr 2017 20:52:02 +0900 Subject: 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 -- cgit v1.2.1 From 0aebc829ad15558d5b6c1bb2212956fb193a1def Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 5 Apr 2017 21:10:10 +0900 Subject: 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) -- cgit v1.2.1 From 255bfd658340e36a882108d4a9911d7f9cde638d Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 27 Apr 2017 22:09:18 +0900 Subject: 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 -- cgit v1.2.1 From 4fe7c25556c7343e46369ffc1e72db1346cc1360 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 27 Apr 2017 22:23:06 +0900 Subject: 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 -- cgit v1.2.1 From 9548942cef4483aa4b414762fffc3d4bc7669c32 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Tue, 2 May 2017 20:11:34 +0200 Subject: Fix misaligned buttons in wiki pages --- app/assets/stylesheets/pages/wiki.scss | 1 - changelogs/unreleased/31704-misaligned-buttons-in-wiki-pages.yml | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/31704-misaligned-buttons-in-wiki-pages.yml diff --git a/app/assets/stylesheets/pages/wiki.scss b/app/assets/stylesheets/pages/wiki.scss index 04ff2d52b91..b64b89485f7 100644 --- a/app/assets/stylesheets/pages/wiki.scss +++ b/app/assets/stylesheets/pages/wiki.scss @@ -71,7 +71,6 @@ .nav-controls { width: auto; min-width: 50%; - white-space: nowrap; } } diff --git a/changelogs/unreleased/31704-misaligned-buttons-in-wiki-pages.yml b/changelogs/unreleased/31704-misaligned-buttons-in-wiki-pages.yml new file mode 100644 index 00000000000..46368b4510e --- /dev/null +++ b/changelogs/unreleased/31704-misaligned-buttons-in-wiki-pages.yml @@ -0,0 +1,4 @@ +--- +title: Fix misaligned buttons in wiki pages +merge_request: 11043 +author: -- cgit v1.2.1 From 96757715cbb346f5c303527912ae0c0bff52de7b Mon Sep 17 00:00:00 2001 From: Marcia Ramos Date: Tue, 2 May 2017 17:06:58 -0300 Subject: add ldap article and changes from !10299 --- .../img/gitlab_ou.png | Bin 0 -> 27877 bytes .../img/ldap_ou.gif | Bin 0 -> 222162 bytes .../img/user_auth.gif | Bin 0 -> 110971 bytes .../how_to_configure_ldap_gitlab_ce/index.md | 266 +++++++++++++++++++++ doc/articles/index.md | 5 + doc/topics/authentication/index.md | 1 + 6 files changed, 272 insertions(+) create mode 100644 doc/articles/how_to_configure_ldap_gitlab_ce/img/gitlab_ou.png create mode 100644 doc/articles/how_to_configure_ldap_gitlab_ce/img/ldap_ou.gif create mode 100644 doc/articles/how_to_configure_ldap_gitlab_ce/img/user_auth.gif create mode 100644 doc/articles/how_to_configure_ldap_gitlab_ce/index.md diff --git a/doc/articles/how_to_configure_ldap_gitlab_ce/img/gitlab_ou.png b/doc/articles/how_to_configure_ldap_gitlab_ce/img/gitlab_ou.png new file mode 100644 index 00000000000..11ce324f938 Binary files /dev/null and b/doc/articles/how_to_configure_ldap_gitlab_ce/img/gitlab_ou.png differ diff --git a/doc/articles/how_to_configure_ldap_gitlab_ce/img/ldap_ou.gif b/doc/articles/how_to_configure_ldap_gitlab_ce/img/ldap_ou.gif new file mode 100644 index 00000000000..a6727a3d85f Binary files /dev/null and b/doc/articles/how_to_configure_ldap_gitlab_ce/img/ldap_ou.gif differ diff --git a/doc/articles/how_to_configure_ldap_gitlab_ce/img/user_auth.gif b/doc/articles/how_to_configure_ldap_gitlab_ce/img/user_auth.gif new file mode 100644 index 00000000000..36e6085259f Binary files /dev/null and b/doc/articles/how_to_configure_ldap_gitlab_ce/img/user_auth.gif differ diff --git a/doc/articles/how_to_configure_ldap_gitlab_ce/index.md b/doc/articles/how_to_configure_ldap_gitlab_ce/index.md new file mode 100644 index 00000000000..8fc2d49e8bd --- /dev/null +++ b/doc/articles/how_to_configure_ldap_gitlab_ce/index.md @@ -0,0 +1,266 @@ +# How to configure LDAP with GitLab CE + +> **Type:** admin guide || +> **Level:** intermediary || +> **Author:** [Chris Wilson](https://gitlab.com/MrChrisW) || +> **Publication date:** 2017/05/02 + +## Introduction + +Managing a large number of users in GitLab can become a burden for system administrators. As an organization grows so do user accounts. Keeping these user accounts in sync across multiple enterprise applications often becomes a time consuming task. + +In this guide we will focus on configuring GitLab with Active Directory. [Active Directory](https://en.wikipedia.org/wiki/Active_Directory) is a popular LDAP compatible directory service provided by Microsoft, included in all modern Windows Server operating systems. + +GitLab has supported LDAP integration since [version 2.2](https://about.gitlab.com/2012/02/22/gitlab-version-2-2/). With GitLab LDAP [group syncing](#group-syncing-ee) being added to GitLab Enterprise Edition in [version 6.0](https://about.gitlab.com/2013/08/20/gitlab-6-dot-0-released/). LDAP integration has become one of the most popular features in GitLab. + +## Getting started + +### Choosing an LDAP Server + +The main reason organizations choose to utilize a LDAP server is to keep the entire organization's user base consolidated into a central repository. Users can access multiple applications and systems across the IT environment using a single login. Because LDAP is an open, vendor-neutral, industry standard application protocol, the number of applications using LDAP authentication continues to increase. + +There are many commercial and open source [directory servers](https://en.wikipedia.org/wiki/Directory_service#LDAP_implementations) that support the LDAP protocol. Deciding on the right directory server highly depends on the existing IT environment in which the server will be integrated with. + +For example, [Active Directory](https://technet.microsoft.com/en-us/library/hh831484(v=ws.11).aspx) is generally favored in a primarily Windows environment, as this allows quick integration with existing services. Other popular directory services include: + +- [Oracle Internet Directory](http://www.oracle.com/technetwork/middleware/id-mgmt/overview/index-082035.html) +- [OpenLDAP](http://www.openldap.org/) +- [389 Directory](http://directory.fedoraproject.org/) +- [OpenDJ](https://forgerock.org/opendj/) +- [ApacheDS](https://directory.apache.org/) + +> GitLab uses the [Net::LDAP](https://rubygems.org/gems/net-ldap) library under the hood. This means it supports all [IETF](https://tools.ietf.org/html/rfc2251) compliant LDAPv3 servers. + +### Active Directory (AD) + +We won't cover the installation and configuration of Windows Server or Active Directory Domain Services in this tutorial. There are a number of resources online to guide you through this process: + +- Install Windows Server 2012 - (_technet.microsoft.com_) - [Installing Windows Server 2012 ](https://technet.microsoft.com/en-us/library/jj134246(v=ws.11).aspx) + +- Install Active Directory Domain Services (AD DS) (_technet.microsoft.com_)- [Install Active Directory Domain Services](https://technet.microsoft.com/windows-server-docs/identity/ad-ds/deploy/install-active-directory-domain-services--level-100-#BKMK_PS) + +> **Shortcut:** You can quickly install AD DS via PowerShell using +`Install-WindowsFeature AD-Domain-Services -IncludeManagementTools` + +### Creating an AD **OU** structure + +Configuring organizational units (**OU**s) is an important part of setting up Active Directory. **OU**s form the base for an entire organizational structure. Using GitLab as an example we have designed the **OU** structure below using the geographic **OU** model. In the Geographic Model we separate **OU**s for different geographic regions. + +| GitLab **OU** Design | GitLab AD Structure | +| :----------------------------: | :------------------------------: | +| ![GitLab OU Design][gitlab_ou] | ![GitLab AD Structure][ldap_ou] | + +[gitlab_ou]: img/gitlab_ou.png +[ldap_ou]: img/ldap_ou.gif + +Using PowerShell you can output the **OU** structure as a table (_all names are examples only_): + +```ps +Get-ADObject -LDAPFilter "(objectClass=*)" -SearchBase 'OU=GitLab INT,DC=GitLab,DC=org' -Properties CanonicalName | Format-Table Name,CanonicalName -A +``` + +``` +OU CanonicalName +---- ------------- +GitLab INT GitLab.org/GitLab INT +United States GitLab.org/GitLab INT/United States +Developers GitLab.org/GitLab INT/United States/Developers +Gary Johnson GitLab.org/GitLab INT/United States/Developers/Gary Johnson +Ellis Matthews GitLab.org/GitLab INT/United States/Developers/Ellis Matthews +William Collins GitLab.org/GitLab INT/United States/Developers/William Collins +People Ops GitLab.org/GitLab INT/United States/People Ops +Margaret Baker GitLab.org/GitLab INT/United States/People Ops/Margaret Baker +Libby Hartzler GitLab.org/GitLab INT/United States/People Ops/Libby Hartzler +Victoria Ryles GitLab.org/GitLab INT/United States/People Ops/Victoria Ryles +The Netherlands GitLab.org/GitLab INT/The Netherlands +Developers GitLab.org/GitLab INT/The Netherlands/Developers +John Doe GitLab.org/GitLab INT/The Netherlands/Developers/John Doe +Jon Mealy GitLab.org/GitLab INT/The Netherlands/Developers/Jon Mealy +Jane Weingarten GitLab.org/GitLab INT/The Netherlands/Developers/Jane Weingarten +Production GitLab.org/GitLab INT/The Netherlands/Production +Sarah Konopka GitLab.org/GitLab INT/The Netherlands/Production/Sarah Konopka +Cynthia Bruno GitLab.org/GitLab INT/The Netherlands/Production/Cynthia Bruno +David George GitLab.org/GitLab INT/The Netherlands/Production/David George +United Kingdom GitLab.org/GitLab INT/United Kingdom +Developers GitLab.org/GitLab INT/United Kingdom/Developers +Leroy Fox GitLab.org/GitLab INT/United Kingdom/Developers/Leroy Fox +Christopher Alley GitLab.org/GitLab INT/United Kingdom/Developers/Christopher Alley +Norris Morita GitLab.org/GitLab INT/United Kingdom/Developers/Norris Morita +Support GitLab.org/GitLab INT/United Kingdom/Support +Laura Stanley GitLab.org/GitLab INT/United Kingdom/Support/Laura Stanley +Nikki Schuman GitLab.org/GitLab INT/United Kingdom/Support/Nikki Schuman +Harriet Butcher GitLab.org/GitLab INT/United Kingdom/Support/Harriet Butcher +Global Groups GitLab.org/GitLab INT/Global Groups +DevelopersNL GitLab.org/GitLab INT/Global Groups/DevelopersNL +DevelopersUK GitLab.org/GitLab INT/Global Groups/DevelopersUK +DevelopersUS GitLab.org/GitLab INT/Global Groups/DevelopersUS +ProductionNL GitLab.org/GitLab INT/Global Groups/ProductionNL +SupportUK GitLab.org/GitLab INT/Global Groups/SupportUK +People Ops US GitLab.org/GitLab INT/Global Groups/People Ops US +Global Admins GitLab.org/GitLab INT/Global Groups/Global Admins +``` + +> See [more information](https://technet.microsoft.com/en-us/library/ff730967.aspx) on searching Active Directory with Windows PowerShell from [The Scripting Guys](https://technet.microsoft.com/en-us/scriptcenter/dd901334.aspx) + +## GitLab LDAP configuration + +The initial configuration of LDAP in GitLab requires changes to the `gitlab.rb` configuration file. Below is an example of a complete configuration using an Active Directory. + +The two Active Directory specific values are `active_directory: true` and `uid: 'sAMAccountName'`. `sAMAccountName` is an attribute returned by Active Directory used for GitLab usernames. See the example output from `ldapsearch` for a full list of attributes a "person" object (user) has in **AD** - [`ldapsearch` example](#using-ldapsearch-unix) + +> Both group_base and admin_group configuration options are only available in GitLab Enterprise Edition. See [GitLab EE - LDAP Features](#gitlab-enterprise-edition---ldap-features) + +### Example `gitlab.rb` LDAP + +``` +gitlab_rails['ldap_enabled'] = true +gitlab_rails['ldap_servers'] = { +'main' => { + 'label' => 'GitLab AD', + 'host' => 'ad.example.org', + 'port' => 636, + 'uid' => 'sAMAccountName', + 'method' => 'ssl', + 'bind_dn' => 'CN=GitLabSRV,CN=Users,DC=GitLab,DC=org', + 'password' => 'Password1', + 'active_directory' => true, + 'base' => 'OU=GitLab INT,DC=GitLab,DC=org', + 'group_base' => 'OU=Global Groups,OU=GitLab INT,DC=GitLab,DC=org', + 'admin_group' => 'Global Admins' + } +} +``` + +> **Note:** Remember to run `gitlab-ctl reconfigure` after modifying `gitlab.rb` + +## Security improvements (LDAPS) + +Security is an important aspect when deploying an LDAP server. By default, LDAP traffic is transmitted unsecured. LDAP can be secured using SSL/TLS called LDAPS, or commonly "LDAP over SSL". + +Securing LDAP (enabling LDAPS) on Windows Server 2012 involves installing a valid SSL certificate. For full details see Microsoft's guide [How to enable LDAP over SSL with a third-party certification authority](https://support.microsoft.com/en-us/help/321051/how-to-enable-ldap-over-ssl-with-a-third-party-certification-authority) + +> By default a LDAP service listens for connections on TCP and UDP port 389. LDAPS (LDAP over SSL) listens on port 636 + +### Testing you AD server + +#### Using **AdFind** (Windows) + +You can use the [`AdFind`](https://social.technet.microsoft.com/wiki/contents/articles/7535.adfind-command-examples.aspx) utility (on Windows based systems) to test that your LDAP server is accessible and authentication is working correctly. This is a freeware utility built by [Joe Richards](http://www.joeware.net/freetools/tools/adfind/index.htm). + +**Return all objects** + +You can use the filter `objectclass=*` to return all directory objects. + +```sh +adfind -h ad.example.org:636 -ssl -u "CN=GitLabSRV,CN=Users,DC=GitLab,DC=org" -up Password1 -b "OU=GitLab INT,DC=GitLab,DC=org" -f (objectClass=*) +``` + +**Return single object using filter** + +You can also retrieve a single object by **specifying** the object name or full **DN**. In this example we specify the object name only `CN=Leroy Fox`. + +```sh +adfind -h ad.example.org:636 -ssl -u "CN=GitLabSRV,CN=Users,DC=GitLab,DC=org" -up Password1 -b "OU=GitLab INT,DC=GitLab,DC=org" -f (&(objectcategory=person)(CN=Leroy Fox))” +``` + +#### Using **ldapsearch** (Unix) + +You can use the `ldapsearch` utility (on Unix based systems) to test that your LDAP server is accessible and authentication is working correctly. This utility is included in the [`ldap-utils`](https://wiki.debian.org/LDAP/LDAPUtils) package. + +**Return all objects** + +You can use the filter `objectclass=*` to return all directory objects. + +```sh +ldapsearch -D "CN=GitLabSRV,CN=Users,DC=GitLab,DC=org" \ +-w Password1 -p 636 -h ad.example.org \ +-b "OU=GitLab INT,DC=GitLab,DC=org" -Z \ +-s sub "(objectclass=*)" +``` + +**Return single object using filter** + +You can also retrieve a single object by **specifying** the object name or full **DN**. In this example we specify the object name only `CN=Leroy Fox`. + +```sh +ldapsearch -D "CN=GitLabSRV,CN=Users,DC=GitLab,DC=org" -w Password1 -p 389 -h ad.example.org -b "OU=GitLab INT,DC=GitLab,DC=org" -Z -s sub "CN=Leroy Fox" +``` + +**Full output of `ldapsearch` command:** - Filtering for _CN=Leroy Fox_ + +``` +# LDAPv3 +# base with scope subtree +# filter: CN=Leroy Fox +# requesting: ALL +# + +# Leroy Fox, Developers, United Kingdom, GitLab INT, GitLab.org +dn: CN=Leroy Fox,OU=Developers,OU=United Kingdom,OU=GitLab INT,DC=GitLab,DC=or + g +objectClass: top +objectClass: person +objectClass: organizationalPerson +objectClass: user +cn: Leroy Fox +sn: Fox +givenName: Leroy +distinguishedName: CN=Leroy Fox,OU=Developers,OU=United Kingdom,OU=GitLab INT, + DC=GitLab,DC=org +instanceType: 4 +whenCreated: 20170210030500.0Z +whenChanged: 20170213050128.0Z +displayName: Leroy Fox +uSNCreated: 16790 +memberOf: CN=DevelopersUK,OU=Global Groups,OU=GitLab INT,DC=GitLab,DC=org +uSNChanged: 20812 +name: Leroy Fox +objectGUID:: rBCAo6NR6E6vfSKgzcUILg== +userAccountControl: 512 +badPwdCount: 0 +codePage: 0 +countryCode: 0 +badPasswordTime: 0 +lastLogoff: 0 +lastLogon: 0 +pwdLastSet: 131311695009850084 +primaryGroupID: 513 +objectSid:: AQUAAAAAAAUVAAAA9GMAb7tdJZvsATf7ZwQAAA== +accountExpires: 9223372036854775807 +logonCount: 0 +sAMAccountName: Leroyf +sAMAccountType: 805306368 +userPrincipalName: Leroyf@GitLab.org +objectCategory: CN=Person,CN=Schema,CN=Configuration,DC=GitLab,DC=org +dSCorePropagationData: 16010101000000.0Z +lastLogonTimestamp: 131314356887754250 + +# search result +search: 2 +result: 0 Success + +# numResponses: 2 +# numEntries: 1 +``` + +## Basic user authentication + +After configuring LDAP, basic authentication will be available. Users can then login using their directory credentials. An extra tab is added to the GitLab login screen for the configured LDAP server (e.g "**GitLab AD**"). + +![GitLab OU Structure](img/user_auth.gif) + +Users that are removed from the LDAP base group (e.g `OU=GitLab INT,DC=GitLab,DC=org`) will be **blocked** in GitLab. [More information](../../administration/auth/ldap.md#security) on LDAP security. + +If `allow_username_or_email_login` is enabled in the LDAP configuration, GitLab will ignore everything after the first '@' in the LDAP username used on login. Example: The username `jon.doe@example.com` is converted to `jon.doe` when authenticating with the LDAP server. Disable this setting if you use `userPrincipalName` as the `uid`. + +## LDAP extended features on GitLab EE + +With [GitLab Enterprise Edition (EE)](https://about.gitlab.com/giltab-ee/), besides everything we just described, you'll +have extended functionalities with LDAP, such as: + +- Group sync +- Group permissions +- Updating user permissions +- Multiple LDAP servers + +Read through the article on [LDAP for GitLab EE](https://docs.gitlab.com/ee/articles/how_to_configure_ldap_gitlab_ee/) for an overview. diff --git a/doc/articles/index.md b/doc/articles/index.md index 67eab36bf2c..49db64134f5 100644 --- a/doc/articles/index.md +++ b/doc/articles/index.md @@ -7,6 +7,11 @@ to provide the community with guidance on specific processes to achieve certain They are written by members of the GitLab Team and by [Community Writers](https://about.gitlab.com/handbook/product/technical-writing/community-writers/). +## Authentication + +- **LDAP** + - [How to configure LDAP with GitLab CE](how_to_configure_ldap_gitlab_ce/index.md) + ## GitLab Pages - **GitLab Pages from A to Z** diff --git a/doc/topics/authentication/index.md b/doc/topics/authentication/index.md index eafd2fd9d04..4a807f89d56 100644 --- a/doc/topics/authentication/index.md +++ b/doc/topics/authentication/index.md @@ -18,6 +18,7 @@ This page gathers all the resources for the topic **Authentication** within GitL - [LDAP (Enterprise Edition)](https://docs.gitlab.com/ee/administration/auth/ldap-ee.html) - [Enforce Two-factor Authentication (2FA)](../../security/two_factor_authentication.md#enforce-two-factor-authentication-2fa) - **Articles:** + - [How to Configure LDAP with GitLab CE](../../articles/how_to_configure_ldap_gitlab_ce/index.md) - [Feature Highlight: LDAP Integration](https://about.gitlab.com/2014/07/10/feature-highlight-ldap-sync/) - [Debugging LDAP](https://about.gitlab.com/handbook/support/workflows/ldap/debugging_ldap.html) - **Integrations:** -- cgit v1.2.1 From 0664a580bddd1516c697ae2265716139bc29e9d6 Mon Sep 17 00:00:00 2001 From: Marcia Ramos Date: Tue, 2 May 2017 18:46:07 -0300 Subject: link ldap-ee article from auth index --- doc/topics/authentication/index.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/topics/authentication/index.md b/doc/topics/authentication/index.md index 4a807f89d56..3e756d96ed2 100644 --- a/doc/topics/authentication/index.md +++ b/doc/topics/authentication/index.md @@ -19,6 +19,7 @@ This page gathers all the resources for the topic **Authentication** within GitL - [Enforce Two-factor Authentication (2FA)](../../security/two_factor_authentication.md#enforce-two-factor-authentication-2fa) - **Articles:** - [How to Configure LDAP with GitLab CE](../../articles/how_to_configure_ldap_gitlab_ce/index.md) + - [How to Configure LDAP with GitLab EE](https://docs.gitlab.com/articles/how_to_configure_ldap_gitlab_ee/) - [Feature Highlight: LDAP Integration](https://about.gitlab.com/2014/07/10/feature-highlight-ldap-sync/) - [Debugging LDAP](https://about.gitlab.com/handbook/support/workflows/ldap/debugging_ldap.html) - **Integrations:** -- cgit v1.2.1 From 0c87d9fd59941000f373f252ad78d0f2247e8e4f Mon Sep 17 00:00:00 2001 From: Marcia Ramos Date: Tue, 2 May 2017 19:12:55 -0300 Subject: update article date --- doc/articles/how_to_configure_ldap_gitlab_ce/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/articles/how_to_configure_ldap_gitlab_ce/index.md b/doc/articles/how_to_configure_ldap_gitlab_ce/index.md index 8fc2d49e8bd..1702c2184f2 100644 --- a/doc/articles/how_to_configure_ldap_gitlab_ce/index.md +++ b/doc/articles/how_to_configure_ldap_gitlab_ce/index.md @@ -3,7 +3,7 @@ > **Type:** admin guide || > **Level:** intermediary || > **Author:** [Chris Wilson](https://gitlab.com/MrChrisW) || -> **Publication date:** 2017/05/02 +> **Publication date:** 2017/05/03 ## Introduction -- cgit v1.2.1 From 4f7811b7381f8475aef58101004238870f2fbcb9 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 2 May 2017 17:46:24 -0500 Subject: Extract common parts of snippet and blob pages into partial --- app/views/projects/blob/_header.html.haml | 11 +---------- app/views/projects/blob/_header_content.html.haml | 10 ++++++++++ app/views/shared/snippets/_blob.html.haml | 11 +---------- 3 files changed, 12 insertions(+), 20 deletions(-) create mode 100644 app/views/projects/blob/_header_content.html.haml diff --git a/app/views/projects/blob/_header.html.haml b/app/views/projects/blob/_header.html.haml index 219dc14645b..cd098acda81 100644 --- a/app/views/projects/blob/_header.html.haml +++ b/app/views/projects/blob/_header.html.haml @@ -1,15 +1,6 @@ - blame = local_assigns.fetch(:blame, false) .js-file-title.file-title-flex-parent - .file-header-content - = blob_icon blob.mode, blob.name - - %strong.file-title-name - = blob.name - - = copy_file_path_button(blob.path) - - %small - = number_to_human_size(blob.raw_size) + = render 'projects/blob/header_content', blob: blob .file-actions.hidden-xs = render 'projects/blob/viewer_switcher', blob: blob unless blame diff --git a/app/views/projects/blob/_header_content.html.haml b/app/views/projects/blob/_header_content.html.haml new file mode 100644 index 00000000000..98bedae650a --- /dev/null +++ b/app/views/projects/blob/_header_content.html.haml @@ -0,0 +1,10 @@ +.file-header-content + = blob_icon blob.mode, blob.name + + %strong.file-title-name + = blob.name + + = copy_file_path_button(blob.path) + + %small + = number_to_human_size(blob.raw_size) diff --git a/app/views/shared/snippets/_blob.html.haml b/app/views/shared/snippets/_blob.html.haml index 9bcb4544b97..11f0fa7c49f 100644 --- a/app/views/shared/snippets/_blob.html.haml +++ b/app/views/shared/snippets/_blob.html.haml @@ -1,15 +1,6 @@ - blob = @snippet.blob .js-file-title.file-title-flex-parent - .file-header-content - = blob_icon blob.mode, blob.path - - %strong.file-title-name - = blob.path - - = copy_file_path_button(blob.path) - - %small - = number_to_human_size(blob.raw_size) + = render 'projects/blob/header_content', blob: blob .file-actions.hidden-xs = render 'projects/blob/viewer_switcher', blob: blob -- cgit v1.2.1 From d02e7226c47db88549f54d0fed0e2b33a726908d Mon Sep 17 00:00:00 2001 From: Mark Fletcher Date: Mon, 1 May 2017 13:22:11 +0800 Subject: Detect already enabled DeployKeys in EnableDeployKeyService Ensures deploy keys can't be re-added, which causes a validation error --- app/services/projects/enable_deploy_key_service.rb | 5 ++++- ...r-when-enabling-a-deploy-key-more-than-once-through-api.yml | 4 ++++ spec/services/projects/enable_deploy_key_service_spec.rb | 10 ++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/29673-500-internal-server-error-when-enabling-a-deploy-key-more-than-once-through-api.yml diff --git a/app/services/projects/enable_deploy_key_service.rb b/app/services/projects/enable_deploy_key_service.rb index 3cf4264ce9b..121385afca3 100644 --- a/app/services/projects/enable_deploy_key_service.rb +++ b/app/services/projects/enable_deploy_key_service.rb @@ -4,7 +4,10 @@ module Projects key = accessible_keys.find_by(id: params[:key_id] || params[:id]) return unless key - project.deploy_keys << key + unless project.deploy_keys.include?(key) + project.deploy_keys << key + end + key end diff --git a/changelogs/unreleased/29673-500-internal-server-error-when-enabling-a-deploy-key-more-than-once-through-api.yml b/changelogs/unreleased/29673-500-internal-server-error-when-enabling-a-deploy-key-more-than-once-through-api.yml new file mode 100644 index 00000000000..3e62ede1521 --- /dev/null +++ b/changelogs/unreleased/29673-500-internal-server-error-when-enabling-a-deploy-key-more-than-once-through-api.yml @@ -0,0 +1,4 @@ +--- +title: Detect already enabled DeployKeys in EnableDeployKeyService +merge_request: +author: diff --git a/spec/services/projects/enable_deploy_key_service_spec.rb b/spec/services/projects/enable_deploy_key_service_spec.rb index a37510cf159..78626fbad4b 100644 --- a/spec/services/projects/enable_deploy_key_service_spec.rb +++ b/spec/services/projects/enable_deploy_key_service_spec.rb @@ -21,6 +21,16 @@ describe Projects::EnableDeployKeyService, services: true do end end + context 'add the same key twice' do + before do + project.deploy_keys << deploy_key + end + + it 'returns existing key' do + expect(service.execute).to eq(deploy_key) + end + end + def service Projects::EnableDeployKeyService.new(project, user, params) end -- cgit v1.2.1 From c99075e0d30560d347423a6de7b5f45e9ab73f3a Mon Sep 17 00:00:00 2001 From: Mark Fletcher Date: Mon, 1 May 2017 13:57:58 +0800 Subject: Fix label creation from issuable for subgroup projects --- app/views/shared/issuable/_sidebar.html.haml | 2 +- .../unreleased/30667-creating-new-label-on-new-issue-causing-bug.yml | 4 ++++ spec/features/issues/issue_sidebar_spec.rb | 3 ++- 3 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/30667-creating-new-label-on-new-issue-causing-bug.yml diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index 2e0d6a129fb..4150ec374d7 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -136,7 +136,7 @@ - selected_labels.each do |label| = hidden_field_tag "#{issuable.to_ability_name}[label_names][]", label.id, id: nil .dropdown - %button.dropdown-menu-toggle.js-label-select.js-multiselect.js-label-sidebar-dropdown{ type: "button", data: {toggle: "dropdown", default_label: "Labels", field_name: "#{issuable.to_ability_name}[label_names][]", ability_name: issuable.to_ability_name, show_no: "true", show_any: "true", namespace_path: @project.try(:namespace).try(:path), project_path: @project.try(:path), issue_update: issuable_json_path(issuable), labels: (namespace_project_labels_path(@project.namespace, @project, :json) if @project) } } + %button.dropdown-menu-toggle.js-label-select.js-multiselect.js-label-sidebar-dropdown{ type: "button", data: {toggle: "dropdown", default_label: "Labels", field_name: "#{issuable.to_ability_name}[label_names][]", ability_name: issuable.to_ability_name, show_no: "true", show_any: "true", namespace_path: @project.try(:namespace).try(:full_path), project_path: @project.try(:path), issue_update: issuable_json_path(issuable), labels: (namespace_project_labels_path(@project.namespace, @project, :json) if @project) } } %span.dropdown-toggle-text{ class: ("is-default" if selected_labels.empty?) } = multi_label_name(selected_labels, "Labels") = icon('chevron-down', 'aria-hidden': 'true') diff --git a/changelogs/unreleased/30667-creating-new-label-on-new-issue-causing-bug.yml b/changelogs/unreleased/30667-creating-new-label-on-new-issue-causing-bug.yml new file mode 100644 index 00000000000..ce0ea69211e --- /dev/null +++ b/changelogs/unreleased/30667-creating-new-label-on-new-issue-causing-bug.yml @@ -0,0 +1,4 @@ +--- +title: Fix label creation from issuable for subgroup projects +merge_request: +author: diff --git a/spec/features/issues/issue_sidebar_spec.rb b/spec/features/issues/issue_sidebar_spec.rb index baacd7edb86..6eed13f0774 100644 --- a/spec/features/issues/issue_sidebar_spec.rb +++ b/spec/features/issues/issue_sidebar_spec.rb @@ -3,7 +3,8 @@ require 'rails_helper' feature 'Issue Sidebar', feature: true do include MobileHelpers - let(:project) { create(:project, :public) } + let(:group) { create(:group, :nested) } + let(:project) { create(:project, :public, namespace: group) } let(:issue) { create(:issue, project: project) } let!(:user) { create(:user)} let!(:label) { create(:label, project: project, title: 'bug') } -- cgit v1.2.1 From d6da528b458942cf66b18b834840e0a6dbba0fc9 Mon Sep 17 00:00:00 2001 From: Mark Fletcher Date: Mon, 1 May 2017 14:25:52 +0800 Subject: Note Ghost user and refer to user deletion documentation + Add a partial for displaying user deletion guidance --- app/views/admin/users/show.html.haml | 6 +----- app/views/profiles/accounts/show.html.haml | 6 +----- app/views/users/_deletion_guidance.html.haml | 10 ++++++++++ .../unreleased/31383-admin-remove-user-text-incorrect.yml | 4 ++++ 4 files changed, 16 insertions(+), 10 deletions(-) create mode 100644 app/views/users/_deletion_guidance.html.haml create mode 100644 changelogs/unreleased/31383-admin-remove-user-text-incorrect.yml diff --git a/app/views/admin/users/show.html.haml b/app/views/admin/users/show.html.haml index 840d843f069..89d0bbb7126 100644 --- a/app/views/admin/users/show.html.haml +++ b/app/views/admin/users/show.html.haml @@ -175,11 +175,7 @@ .panel-body - if @user.can_be_removed? && can?(current_user, :destroy_user, @user) %p Deleting a user has the following effects: - %ul - %li All user content like authored issues, snippets, comments will be removed - - rp = @user.personal_projects.count - - unless rp.zero? - %li #{pluralize rp, 'personal project'} will be removed and cannot be restored + = render 'users/deletion_guidance', user: @user %br = link_to 'Remove user', [:admin, @user], data: { confirm: "USER #{@user.name} WILL BE REMOVED! Are you sure?" }, method: :delete, class: "btn btn-remove" - else diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index d843cacd52d..73f33e69d68 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -118,11 +118,7 @@ - if @user.can_be_removed? && can?(current_user, :destroy_user, @user) %p Deleting an account has the following effects: - %ul - %li All user content like authored issues, snippets, comments will be removed - - rp = current_user.personal_projects.count - - unless rp.zero? - %li #{pluralize rp, 'personal project'} will be removed and cannot be restored + = render 'users/deletion_guidance', user: current_user = link_to 'Delete account', user_registration_path, data: { confirm: "REMOVE #{current_user.name}? Are you sure?" }, method: :delete, class: "btn btn-remove" - else - if @user.solo_owned_groups.present? diff --git a/app/views/users/_deletion_guidance.html.haml b/app/views/users/_deletion_guidance.html.haml new file mode 100644 index 00000000000..0545cab538c --- /dev/null +++ b/app/views/users/_deletion_guidance.html.haml @@ -0,0 +1,10 @@ +- user = local_assigns.fetch(:user) + +%ul + %li + %p + Certain user content will be moved to a system-wide "Ghost User" in order to maintain content for posterity. For further information, please refer to the + = link_to 'user account deletion documentation.', help_page_path("user/profile/account/delete_account", anchor: "associated-records") + - personal_projects_count = user.personal_projects.count + - unless personal_projects_count.zero? + %li #{pluralize(personal_projects_count, 'personal project')} will be removed and cannot be restored diff --git a/changelogs/unreleased/31383-admin-remove-user-text-incorrect.yml b/changelogs/unreleased/31383-admin-remove-user-text-incorrect.yml new file mode 100644 index 00000000000..a2a2c0c42bd --- /dev/null +++ b/changelogs/unreleased/31383-admin-remove-user-text-incorrect.yml @@ -0,0 +1,4 @@ +--- +title: Note Ghost user and refer to user deletion documentation +merge_request: +author: -- cgit v1.2.1 From ba446b86b00f3493a446e4167f850b4f44c70805 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 3 May 2017 12:09:47 +0200 Subject: Elaborate on the usage of Spring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- doc/development/rake_tasks.md | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/doc/development/rake_tasks.md b/doc/development/rake_tasks.md index ec9e4dcc59d..fdaaa65fa28 100644 --- a/doc/development/rake_tasks.md +++ b/doc/development/rake_tasks.md @@ -31,16 +31,26 @@ files it can find, also the ones in `/tmp` To run a single test file you can use: -- `bundle exec rspec spec/controllers/commit_controller_spec.rb` for a rspec test -- `bundle exec spinach features/project/issues/milestones.feature` for a spinach test +- `bin/rspec spec/controllers/commit_controller_spec.rb` for a rspec test +- `bin/spinach features/project/issues/milestones.feature` for a spinach test To run several tests inside one directory: -- `bundle exec rspec spec/requests/api/` for the rspec tests if you want to test API only -- `bundle exec spinach features/profile/` for the spinach tests if you want to test only profile pages +- `bin/rspec spec/requests/api/` for the rspec tests if you want to test API only +- `bin/spinach features/profile/` for the spinach tests if you want to test only profile pages -If you want to use [Spring](https://github.com/rails/spring) set -`ENABLE_SPRING=1` in your environment. +### Speed-up tests, rake tasks, and migrations + +[Spring](https://github.com/rails/spring) is a Rails application preloader. It +speeds up development by keeping your application running in the background so +you don't need to boot it every time you run a test, rake task or migration. + +If you want to use it, you'll need to export the `ENABLE_SPRING` environment +variable to `1`: + +``` +export ENABLE_SPRING=1 +``` ## Compile Frontend Assets -- cgit v1.2.1 From f327bf01e52a7936c3b7fa8aa69eb28bc455f1cd Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Wed, 3 May 2017 12:29:59 +0100 Subject: find and match first dropdown before clicking --- spec/features/merge_requests/create_new_mr_spec.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/features/merge_requests/create_new_mr_spec.rb b/spec/features/merge_requests/create_new_mr_spec.rb index f0fec625108..f1b3e7f158c 100644 --- a/spec/features/merge_requests/create_new_mr_spec.rb +++ b/spec/features/merge_requests/create_new_mr_spec.rb @@ -20,7 +20,6 @@ feature 'Create New Merge Request', feature: true, js: true do expect(page).to have_content('Target branch') first('.js-source-branch').click - first('.dropdown-source-branch .dropdown-content') find('.dropdown-source-branch .dropdown-content a', match: :first).click expect(page).to have_content "b83d6e3" @@ -35,8 +34,7 @@ feature 'Create New Merge Request', feature: true, js: true do expect(page).to have_content('Target branch') first('.js-target-branch').click - first('.dropdown-target-branch .dropdown-content') - first('.dropdown-target-branch .dropdown-content a', text: 'v1.1.0').click + find('.dropdown-target-branch .dropdown-content a', text: 'v1.1.0', match: :first).click expect(page).to have_content "b83d6e3" end -- cgit v1.2.1 From 308e2d3ff213a5d5d75090ad7ddd1cb4812f1ef1 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Wed, 3 May 2017 13:18:28 +0000 Subject: Fixed issue_sidebar_spec.rb click as true click cannot hit the right element and removed sleep --- spec/features/issues/issue_sidebar_spec.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/spec/features/issues/issue_sidebar_spec.rb b/spec/features/issues/issue_sidebar_spec.rb index 8589945ab74..0f76cdc35d9 100644 --- a/spec/features/issues/issue_sidebar_spec.rb +++ b/spec/features/issues/issue_sidebar_spec.rb @@ -151,9 +151,7 @@ feature 'Issue Sidebar', feature: true do end def open_issue_sidebar - page.within('aside.right-sidebar.right-sidebar-collapsed') do - find('.js-sidebar-toggle').click - sleep 1 - end + find('aside.right-sidebar.right-sidebar-collapsed .js-sidebar-toggle').trigger('click') + find('aside.right-sidebar.right-sidebar-expanded') end end -- cgit v1.2.1 From d6f07ce41dd6b078840abadc31dc26d81bb33bcf Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Wed, 3 May 2017 13:24:10 +0000 Subject: Revert to real click seeing as that was a bug with only the original branch --- spec/features/issues/issue_sidebar_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/issues/issue_sidebar_spec.rb b/spec/features/issues/issue_sidebar_spec.rb index 0f76cdc35d9..2002eff7995 100644 --- a/spec/features/issues/issue_sidebar_spec.rb +++ b/spec/features/issues/issue_sidebar_spec.rb @@ -151,7 +151,7 @@ feature 'Issue Sidebar', feature: true do end def open_issue_sidebar - find('aside.right-sidebar.right-sidebar-collapsed .js-sidebar-toggle').trigger('click') + find('aside.right-sidebar.right-sidebar-collapsed .js-sidebar-toggle').click find('aside.right-sidebar.right-sidebar-expanded') end end -- cgit v1.2.1 From 35f14904087a087d287cd56652f17d9030f798c8 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 3 May 2017 16:22:05 +0200 Subject: Use gitlab-workhorse 2.0.0 --- GITLAB_WORKHORSE_VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index 428b770e3e2..227cea21564 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -1.4.3 +2.0.0 -- cgit v1.2.1 From 2fbcaaafcd40a2d0752cd2d40ba6abd15f702bf9 Mon Sep 17 00:00:00 2001 From: Dosuken shinya Date: Wed, 3 May 2017 15:23:20 +0000 Subject: Fix lazy error handling of cron parser --- ...uses-http-500-when-accessing-settings-ci_cd.yml | 4 + lib/gitlab/ci/cron_parser.rb | 19 ++++- spec/features/triggers_spec.rb | 18 +++++ spec/lib/gitlab/ci/cron_parser_spec.rb | 88 +++++++++++++++++++--- spec/models/ci/trigger_schedule_spec.rb | 32 ++++++++ 5 files changed, 150 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/31274-creating-schedule-trigger--causes-http-500-when-accessing-settings-ci_cd.yml diff --git a/changelogs/unreleased/31274-creating-schedule-trigger--causes-http-500-when-accessing-settings-ci_cd.yml b/changelogs/unreleased/31274-creating-schedule-trigger--causes-http-500-when-accessing-settings-ci_cd.yml new file mode 100644 index 00000000000..b0c33ab3fa4 --- /dev/null +++ b/changelogs/unreleased/31274-creating-schedule-trigger--causes-http-500-when-accessing-settings-ci_cd.yml @@ -0,0 +1,4 @@ +--- +title: Fix error on CI/CD Settings page related to invalid pipeline trigger +merge_request: 10948 +author: dosuken123 diff --git a/lib/gitlab/ci/cron_parser.rb b/lib/gitlab/ci/cron_parser.rb index a3cc350ef22..dad8c3cdf5b 100644 --- a/lib/gitlab/ci/cron_parser.rb +++ b/lib/gitlab/ci/cron_parser.rb @@ -6,7 +6,7 @@ module Gitlab def initialize(cron, cron_timezone = 'UTC') @cron = cron - @cron_timezone = cron_timezone + @cron_timezone = ActiveSupport::TimeZone.find_tzinfo(cron_timezone).name end def next_time_from(time) @@ -24,8 +24,23 @@ module Gitlab private + # NOTE: + # cron_timezone can only accept timezones listed in TZInfo::Timezone. + # Aliases of Timezones from ActiveSupport::TimeZone are NOT accepted, + # because Rufus::Scheduler only supports TZInfo::Timezone. + # + # For example, those codes have the same effect. + # Time.zone = 'Pacific Time (US & Canada)' (ActiveSupport::TimeZone) + # Time.zone = 'America/Los_Angeles' (TZInfo::Timezone) + # + # However, try_parse_cron only accepts the latter format. + # try_parse_cron('* * * * *', 'Pacific Time (US & Canada)') -> Doesn't work + # try_parse_cron('* * * * *', 'America/Los_Angeles') -> Works + # If you want to know more, please take a look + # https://github.com/rails/rails/blob/master/activesupport/lib/active_support/values/time_zone.rb def try_parse_cron(cron, cron_timezone) - Rufus::Scheduler.parse("#{cron} #{cron_timezone}") + cron_line = Rufus::Scheduler.parse("#{cron} #{cron_timezone}") + cron_line if cron_line.is_a?(Rufus::Scheduler::CronLine) rescue # noop end diff --git a/spec/features/triggers_spec.rb b/spec/features/triggers_spec.rb index 81fa2de1cc3..783f330221c 100644 --- a/spec/features/triggers_spec.rb +++ b/spec/features/triggers_spec.rb @@ -104,6 +104,24 @@ feature 'Triggers', feature: true, js: true do expect(page).to have_content 'The form contains the following errors' end + + context 'when GitLab time_zone is ActiveSupport::TimeZone format' do + before do + allow(Time).to receive(:zone) + .and_return(ActiveSupport::TimeZone['Eastern Time (US & Canada)']) + end + + scenario 'do fill form with valid data and save' do + find('#trigger_trigger_schedule_attributes_active').click + fill_in 'trigger_trigger_schedule_attributes_cron', with: '1 * * * *' + fill_in 'trigger_trigger_schedule_attributes_cron_timezone', with: 'UTC' + fill_in 'trigger_trigger_schedule_attributes_ref', with: 'master' + click_button 'Save trigger' + + expect(page.find('.flash-notice')) + .to have_content 'Trigger was successfully updated.' + end + end end context 'disabling schedule' do diff --git a/spec/lib/gitlab/ci/cron_parser_spec.rb b/spec/lib/gitlab/ci/cron_parser_spec.rb index 0864bc7258d..809fda11879 100644 --- a/spec/lib/gitlab/ci/cron_parser_spec.rb +++ b/spec/lib/gitlab/ci/cron_parser_spec.rb @@ -60,14 +60,60 @@ describe Gitlab::Ci::CronParser do end end - context 'when cron_timezone is US/Pacific' do - let(:cron) { '0 0 * * *' } - let(:cron_timezone) { 'US/Pacific' } + context 'when cron_timezone is TZInfo format' do + before do + allow(Time).to receive(:zone) + .and_return(ActiveSupport::TimeZone['UTC']) + end - it_behaves_like "returns time in the future" + let(:hour_in_utc) do + ActiveSupport::TimeZone[cron_timezone] + .now.change(hour: 0).in_time_zone('UTC').hour + end + + context 'when cron_timezone is US/Pacific' do + let(:cron) { '* 0 * * *' } + let(:cron_timezone) { 'US/Pacific' } + + it_behaves_like "returns time in the future" + + it 'converts time in server time zone' do + expect(subject.hour).to eq(hour_in_utc) + end + end + end + + context 'when cron_timezone is ActiveSupport::TimeZone format' do + before do + allow(Time).to receive(:zone) + .and_return(ActiveSupport::TimeZone['UTC']) + end + + let(:hour_in_utc) do + ActiveSupport::TimeZone[cron_timezone] + .now.change(hour: 0).in_time_zone('UTC').hour + end + + context 'when cron_timezone is Berlin' do + let(:cron) { '* 0 * * *' } + let(:cron_timezone) { 'Berlin' } + + it_behaves_like "returns time in the future" + + it 'converts time in server time zone' do + expect(subject.hour).to eq(hour_in_utc) + end + end - it 'converts time in server time zone' do - expect(subject.hour).to eq((Time.zone.now.in_time_zone(cron_timezone).utc_offset / 60 / 60).abs) + context 'when cron_timezone is Eastern Time (US & Canada)' do + let(:cron) { '* 0 * * *' } + let(:cron_timezone) { 'Eastern Time (US & Canada)' } + + it_behaves_like "returns time in the future" + + it 'converts time in server time zone' do + expect(subject.hour).to eq(hour_in_utc) + end end end end @@ -76,9 +122,21 @@ describe Gitlab::Ci::CronParser do let(:cron) { 'invalid_cron' } let(:cron_timezone) { 'invalid_cron_timezone' } - it 'returns nil' do - is_expected.to be_nil - end + it { is_expected.to be_nil } + end + + context 'when cron syntax is quoted' do + let(:cron) { "'0 * * * *'" } + let(:cron_timezone) { 'UTC' } + + it { expect(subject).to be_nil } + end + + context 'when cron syntax is rufus-scheduler syntax' do + let(:cron) { 'every 3h' } + let(:cron_timezone) { 'UTC' } + + it { expect(subject).to be_nil } end end @@ -96,6 +154,12 @@ describe Gitlab::Ci::CronParser do it { is_expected.to eq(false) } end + + context 'when cron syntax is quoted' do + let(:cron) { "'0 * * * *'" } + + it { is_expected.to eq(false) } + end end describe '#cron_timezone_valid?' do @@ -112,5 +176,11 @@ describe Gitlab::Ci::CronParser do it { is_expected.to eq(false) } end + + context 'when cron_timezone is ActiveSupport::TimeZone format' do + let(:cron_timezone) { 'Eastern Time (US & Canada)' } + + it { is_expected.to eq(true) } + end end end diff --git a/spec/models/ci/trigger_schedule_spec.rb b/spec/models/ci/trigger_schedule_spec.rb index 75d21541cee..92447564d7c 100644 --- a/spec/models/ci/trigger_schedule_spec.rb +++ b/spec/models/ci/trigger_schedule_spec.rb @@ -73,4 +73,36 @@ describe Ci::TriggerSchedule, models: true do end end end + + describe '#real_next_run' do + subject do + Ci::TriggerSchedule.last.real_next_run(worker_cron: worker_cron, + worker_time_zone: worker_time_zone) + end + + context 'when GitLab time_zone is UTC' do + before do + allow(Time).to receive(:zone) + .and_return(ActiveSupport::TimeZone[worker_time_zone]) + end + + let(:worker_time_zone) { 'UTC' } + + context 'when cron_timezone is Eastern Time (US & Canada)' do + before do + create(:ci_trigger_schedule, :nightly, + cron_timezone: 'Eastern Time (US & Canada)') + end + + let(:worker_cron) { '0 1 2 3 *' } + + it 'returns the next time worker executes' do + expect(subject.min).to eq(0) + expect(subject.hour).to eq(1) + expect(subject.day).to eq(2) + expect(subject.month).to eq(3) + end + end + end + end end -- cgit v1.2.1 From cf3990cfbfe878f8b4f867042ee0967f947718ee Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 3 May 2017 16:36:01 +0000 Subject: Fix project tree saver and fork spec failures --- lib/gitlab/import_export/import_export.yml | 1 + spec/lib/gitlab/import_export/project_tree_saver_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index b95cea371b9..3aac731e844 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -84,6 +84,7 @@ excluded_attributes: - :import_jid - :id - :star_count + - :last_activity_at snippets: - :expired_at merge_request_diff: diff --git a/spec/lib/gitlab/import_export/project_tree_saver_spec.rb b/spec/lib/gitlab/import_export/project_tree_saver_spec.rb index 6e145947104..1035428b2e7 100644 --- a/spec/lib/gitlab/import_export/project_tree_saver_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_saver_spec.rb @@ -6,7 +6,7 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do let(:project_tree_saver) { described_class.new(project: project, current_user: user, shared: shared) } let(:export_path) { "#{Dir.tmpdir}/project_tree_saver_spec" } let(:user) { create(:user) } - let(:project) { setup_project } + let!(:project) { setup_project } before do project.team << [user, :master] @@ -219,7 +219,7 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do releases: [release], group: group ) - project.update(description_html: 'description') + project.update_column(:description_html, 'description') project_label = create(:label, project: project) group_label = create(:group_label, group: group) create(:label_link, label: project_label, target: issue) -- cgit v1.2.1 From c5bca70d550434001e544824ce182cce68a58677 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Wed, 3 May 2017 16:40:43 +0000 Subject: Move api lint out of static analysis job --- .gitlab-ci.yml | 7 ++++++- scripts/static-analysis | 1 - 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index aa62a86d31d..7f665f19132 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -264,11 +264,15 @@ spinach mysql 9 10: *spinach-knapsack-mysql static-analysis: <<: *ruby-static-analysis <<: *dedicated-runner + <<: *except-docs stage: test script: - scripts/static-analysis -docs:check:links: +# Documentation checks: +# - Check validity of relative links +# - Make sure cURL examples in API docs use the full switches +docs lint: image: "registry.gitlab.com/gitlab-org/gitlab-build-images:nanoc-bootstrap-ruby-2.4-alpine" stage: test <<: *dedicated-runner @@ -276,6 +280,7 @@ docs:check:links: dependencies: [] before_script: [] script: + - scripts/lint-doc.sh - mv doc/ /nanoc/content/ - cd /nanoc # Build HTML from Markdown diff --git a/scripts/static-analysis b/scripts/static-analysis index 192d9d4c3ba..1bd6b339830 100755 --- a/scripts/static-analysis +++ b/scripts/static-analysis @@ -9,7 +9,6 @@ tasks = [ %w[bundle exec rake scss_lint], %w[bundle exec rake brakeman], %w[bundle exec license_finder], - %w[scripts/lint-doc.sh], %w[yarn run eslint], %w[bundle exec rubocop --require rubocop-rspec] ] -- cgit v1.2.1