From 22bf844869bde4e480d981b2f267bc692e701eb4 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Tue, 15 Sep 2015 13:50:24 +0300 Subject: [PATCH] fix specs. Stage 3 --- lib/api/helpers.rb | 38 +++++++++++++ lib/api/projects.rb | 36 ------------ lib/ci/api/entities.rb | 6 +- lib/ci/api/projects.rb | 27 ++++----- spec/requests/ci/api/projects_spec.rb | 81 ++++++++++++++++----------- 5 files changed, 106 insertions(+), 82 deletions(-) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index ef0f897a2fb..7fada98fcdc 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -245,6 +245,44 @@ module API error!({ 'message' => message }, status) end + # Projects helpers + + def filter_projects(projects) + # If the archived parameter is passed, limit results accordingly + if params[:archived].present? + projects = projects.where(archived: parse_boolean(params[:archived])) + end + + if params[:search].present? + projects = projects.search(params[:search]) + end + + if params[:ci_enabled_first].present? + projects.includes(:gitlab_ci_service). + reorder("services.active DESC, projects.#{project_order_by} #{project_sort}") + else + projects.reorder(project_order_by => project_sort) + end + end + + def project_order_by + order_fields = %w(id name path created_at updated_at last_activity_at) + + if order_fields.include?(params['order_by']) + params['order_by'] + else + 'created_at' + end + end + + def project_sort + if params["sort"] == 'asc' + :asc + else + :desc + end + end + private def add_pagination_headers(paginated, per_page) diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 1f2251c9b9c..c2fb36b4143 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -11,42 +11,6 @@ module API attrs[:visibility_level] = Gitlab::VisibilityLevel::PUBLIC if !attrs[:visibility_level].present? && publik == true attrs end - - def filter_projects(projects) - # If the archived parameter is passed, limit results accordingly - if params[:archived].present? - projects = projects.where(archived: parse_boolean(params[:archived])) - end - - if params[:search].present? - projects = projects.search(params[:search]) - end - - if params[:ci_enabled_first].present? - projects.includes(:gitlab_ci_service). - reorder("services.active DESC, projects.#{project_order_by} #{project_sort}") - else - projects.reorder(project_order_by => project_sort) - end - end - - def project_order_by - order_fields = %w(id name path created_at updated_at last_activity_at) - - if order_fields.include?(params['order_by']) - params['order_by'] - else - 'created_at' - end - end - - def project_sort - if params["sort"] == 'asc' - :asc - else - :desc - end - end end # Get a projects list for authenticated user diff --git a/lib/ci/api/entities.rb b/lib/ci/api/entities.rb index 1277d68a364..07f25129663 100644 --- a/lib/ci/api/entities.rb +++ b/lib/ci/api/entities.rb @@ -34,8 +34,12 @@ module Ci end class Project < Grape::Entity - expose :id, :name, :timeout, :token, :default_ref, :gitlab_url, :path, + expose :id, :name, :token, :default_ref, :gitlab_url, :path, :always_build, :polling_interval, :public, :ssh_url_to_repo, :gitlab_id + + expose :timeout do |model| + model.timeout + end end class RunnerProject < Grape::Entity diff --git a/lib/ci/api/projects.rb b/lib/ci/api/projects.rb index 556de3bff9f..138667c980f 100644 --- a/lib/ci/api/projects.rb +++ b/lib/ci/api/projects.rb @@ -17,7 +17,7 @@ module Ci project = Ci::Project.find(params[:project_id]) - unauthorized! unless current_user.can_manage_project?(project.gitlab_id) + unauthorized! unless can?(current_user, :manage_project, project.gl_project) web_hook = project.web_hooks.new({ url: params[:web_hook] }) @@ -34,9 +34,10 @@ module Ci # Example Request: # GET /projects get do - gitlab_projects = Ci::Project.from_gitlab( - current_user, :authorized, { page: params[:page], per_page: params[:per_page], ci_enabled_first: true } - ) + gitlab_projects = current_user.authorized_projects + gitlab_projects = filter_projects(gitlab_projects) + gitlab_projects = paginate gitlab_projects + ids = gitlab_projects.map { |project| project.id } projects = Ci::Project.where("gitlab_id IN (?)", ids).load @@ -48,9 +49,10 @@ module Ci # Example Request: # GET /projects/owned get "owned" do - gitlab_projects = Ci::Project.from_gitlab( - current_user, :owned, { page: params[:page], per_page: params[:per_page], ci_enabled_first: true } - ) + gitlab_projects = current_user.owned_projects + gitlab_projects = filter_projects(gitlab_projects) + gitlab_projects = paginate gitlab_projects + ids = gitlab_projects.map { |project| project.id } projects = Ci::Project.where("gitlab_id IN (?)", ids).load @@ -65,8 +67,7 @@ module Ci # GET /projects/:id get ":id" do project = Ci::Project.find(params[:id]) - - unauthorized! unless can?(current_user, :read_project, gl_project) + unauthorized! unless can?(current_user, :read_project, project.gl_project) present project, with: Entities::Project end @@ -118,7 +119,7 @@ module Ci put ":id" do project = Ci::Project.find(params[:id]) - unauthorized! unless can?(current_user, :manage_project, gl_project) + unauthorized! unless can?(current_user, :manage_project, project.gl_project) attrs = attributes_for_keys [:name, :gitlab_id, :path, :gitlab_url, :default_ref, :ssh_url_to_repo] @@ -144,7 +145,7 @@ module Ci delete ":id" do project = Ci::Project.find(params[:id]) - unauthorized! unless can?(current_user, :manage_project, gl_project) + unauthorized! unless can?(current_user, :manage_project, project.gl_project) project.destroy end @@ -160,7 +161,7 @@ module Ci project = Ci::Project.find(params[:id]) runner = Ci::Runner.find(params[:runner_id]) - unauthorized! unless can?(current_user, :manage_project, gl_project) + unauthorized! unless can?(current_user, :manage_project, project.gl_project) options = { project_id: project.id, @@ -188,7 +189,7 @@ module Ci project = Ci::Project.find(params[:id]) runner = Ci::Runner.find(params[:runner_id]) - unauthorized! unless can?(current_user, :manage_project, gl_project) + unauthorized! unless can?(current_user, :manage_project, project.gl_project) options = { project_id: project.id, diff --git a/spec/requests/ci/api/projects_spec.rb b/spec/requests/ci/api/projects_spec.rb index 05f6bd5f4f3..02ac58dbfc3 100644 --- a/spec/requests/ci/api/projects_spec.rb +++ b/spec/requests/ci/api/projects_spec.rb @@ -3,8 +3,9 @@ require 'spec_helper' describe Ci::API::API do include ApiHelpers - let(:gitlab_url) { GitlabCi.config.gitlab_server.url } - let(:private_token) { Network.new.authenticate(access_token: "some_token")["private_token"] } + let(:gitlab_url) { GitlabCi.config.gitlab_ci.url } + let(:user) { create(:user) } + let(:private_token) { user.private_token } let(:options) do { @@ -20,11 +21,16 @@ describe Ci::API::API do context "requests for scoped projects" do # NOTE: These ids are tied to the actual projects on demo.gitlab.com describe "GET /projects" do - let!(:project1) { FactoryGirl.create(:ci_project, name: "gitlabhq", gitlab_id: 3) } - let!(:project2) { FactoryGirl.create(:ci_project, name: "gitlab-ci", gitlab_id: 4) } + let!(:project1) { FactoryGirl.create(:ci_project) } + let!(:project2) { FactoryGirl.create(:ci_project) } + + before do + project1.gl_project.team << [user, :developer] + project2.gl_project.team << [user, :developer] + end it "should return all projects on the CI instance" do - get api("/projects"), options + get ci_api("/projects"), options expect(response.status).to eq(200) expect(json_response.count).to eq(2) expect(json_response.first["id"]).to eq(project1.id) @@ -33,15 +39,21 @@ describe Ci::API::API do end describe "GET /projects/owned" do - # NOTE: This user doesn't own any of these projects on demo.gitlab.com - let!(:project1) { FactoryGirl.create(:ci_project, name: "gitlabhq", gitlab_id: 3) } - let!(:project2) { FactoryGirl.create(:ci_project, name: "random-project", gitlab_id: 9898) } + let!(:gl_project1) {FactoryGirl.create(:empty_project, namespace: user.namespace)} + let!(:gl_project2) {FactoryGirl.create(:empty_project, namespace: user.namespace)} + let!(:project1) { FactoryGirl.create(:ci_project, gl_project: gl_project1) } + let!(:project2) { FactoryGirl.create(:ci_project, gl_project: gl_project2) } + + before do + project1.gl_project.team << [user, :developer] + project2.gl_project.team << [user, :developer] + end it "should return all projects on the CI instance" do - get api("/projects/owned"), options + get ci_api("/projects/owned"), options expect(response.status).to eq(200) - expect(json_response.count).to eq(0) + expect(json_response.count).to eq(2) end end end @@ -54,22 +66,23 @@ describe Ci::API::API do before do options.merge!(webhook) + project.gl_project.team << [user, :master] end it "should create webhook for specified project" do - post api("/projects/#{project.id}/webhooks"), options + post ci_api("/projects/#{project.id}/webhooks"), options expect(response.status).to eq(201) expect(json_response["url"]).to eq(webhook[:web_hook]) end it "fails to create webhook for non existsing project" do - post api("/projects/non-existant-id/webhooks"), options + post ci_api("/projects/non-existant-id/webhooks"), options expect(response.status).to eq(404) end it "non-manager is not authorized" do allow_any_instance_of(User).to receive(:can_manage_project?).and_return(false) - post api("/projects/#{project.id}/webhooks"), options + post ci_api("/projects/#{project.id}/webhooks"), options expect(response.status).to eq(401) end end @@ -82,14 +95,14 @@ describe Ci::API::API do end it "fails to create webhook for not valid url" do - post api("/projects/#{project.id}/webhooks"), options + post ci_api("/projects/#{project.id}/webhooks"), options expect(response.status).to eq(400) end end context "Missed web_hook parameter" do it "fails to create webhook for not provided url" do - post api("/projects/#{project.id}/webhooks"), options + post ci_api("/projects/#{project.id}/webhooks"), options expect(response.status).to eq(400) end end @@ -98,9 +111,13 @@ describe Ci::API::API do describe "GET /projects/:id" do let!(:project) { FactoryGirl.create(:ci_project) } + before do + project.gl_project.team << [user, :developer] + end + context "with an existing project" do it "should retrieve the project info" do - get api("/projects/#{project.id}"), options + get ci_api("/projects/#{project.id}"), options expect(response.status).to eq(200) expect(json_response['id']).to eq(project.id) end @@ -108,7 +125,7 @@ describe Ci::API::API do context "with a non-existing project" do it "should return 404 error if project not found" do - get api("/projects/non_existent_id"), options + get ci_api("/projects/non_existent_id"), options expect(response.status).to eq(404) end end @@ -123,19 +140,19 @@ describe Ci::API::API do end it "should update a specific project's information" do - put api("/projects/#{project.id}"), options + put ci_api("/projects/#{project.id}"), options expect(response.status).to eq(200) expect(json_response["name"]).to eq(project_info[:name]) end it "fails to update a non-existing project" do - put api("/projects/non-existant-id"), options + put ci_api("/projects/non-existant-id"), options expect(response.status).to eq(404) end it "non-manager is not authorized" do allow_any_instance_of(User).to receive(:can_manage_project?).and_return(false) - put api("/projects/#{project.id}"), options + put ci_api("/projects/#{project.id}"), options expect(response.status).to eq(401) end end @@ -144,7 +161,7 @@ describe Ci::API::API do let!(:project) { FactoryGirl.create(:ci_project) } it "should delete a specific project" do - delete api("/projects/#{project.id}"), options + delete ci_api("/projects/#{project.id}"), options expect(response.status).to eq(200) expect { project.reload }.to raise_error @@ -152,12 +169,12 @@ describe Ci::API::API do it "non-manager is not authorized" do allow_any_instance_of(User).to receive(:can_manage_project?).and_return(false) - delete api("/projects/#{project.id}"), options + delete ci_api("/projects/#{project.id}"), options expect(response.status).to eq(401) end it "is getting not found error" do - delete api("/projects/not-existing_id"), options + delete ci_api("/projects/not-existing_id"), options expect(response.status).to eq(404) end end @@ -180,7 +197,7 @@ describe Ci::API::API do end it "should create a project with valid data" do - post api("/projects"), options + post ci_api("/projects"), options expect(response.status).to eq(201) expect(json_response['name']).to eq(project_info[:name]) end @@ -192,7 +209,7 @@ describe Ci::API::API do end it "should error with invalid data" do - post api("/projects"), options + post ci_api("/projects"), options expect(response.status).to eq(400) end end @@ -202,7 +219,7 @@ describe Ci::API::API do let(:runner) { FactoryGirl.create(:ci_runner) } it "should add the project to the runner" do - post api("/projects/#{project.id}/runners/#{runner.id}"), options + post ci_api("/projects/#{project.id}/runners/#{runner.id}"), options expect(response.status).to eq(201) project.reload @@ -210,16 +227,16 @@ describe Ci::API::API do end it "should fail if it tries to link a non-existing project or runner" do - post api("/projects/#{project.id}/runners/non-existing"), options + post ci_api("/projects/#{project.id}/runners/non-existing"), options expect(response.status).to eq(404) - post api("/projects/non-existing/runners/#{runner.id}"), options + post ci_api("/projects/non-existing/runners/#{runner.id}"), options expect(response.status).to eq(404) end it "non-manager is not authorized" do allow_any_instance_of(User).to receive(:can_manage_project?).and_return(false) - post api("/projects/#{project.id}/runners/#{runner.id}"), options + post ci_api("/projects/#{project.id}/runners/#{runner.id}"), options expect(response.status).to eq(401) end end @@ -229,12 +246,12 @@ describe Ci::API::API do let(:runner) { FactoryGirl.create(:ci_runner) } before do - post api("/projects/#{project.id}/runners/#{runner.id}"), options + post ci_api("/projects/#{project.id}/runners/#{runner.id}"), options end it "should remove the project from the runner" do expect(project.runners).to be_present - delete api("/projects/#{project.id}/runners/#{runner.id}"), options + delete ci_api("/projects/#{project.id}/runners/#{runner.id}"), options expect(response.status).to eq(200) project.reload @@ -243,7 +260,7 @@ describe Ci::API::API do it "non-manager is not authorized" do allow_any_instance_of(User).to receive(:can_manage_project?).and_return(false) - post api("/projects/#{project.id}/runners/#{runner.id}"), options + post ci_api("/projects/#{project.id}/runners/#{runner.id}"), options expect(response.status).to eq(401) end end