From f4e7a8893d71bcbc92a5ecd16c95593f17d1ddf5 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 24 Dec 2015 19:05:57 +0100 Subject: [PATCH 001/218] Add builds API (listing, showing trace) --- lib/api/api.rb | 2 ++ lib/api/builds.rb | 80 +++++++++++++++++++++++++++++++++++++++++++++ lib/api/entities.rb | 13 ++++++++ 3 files changed, 95 insertions(+) create mode 100644 lib/api/builds.rb diff --git a/lib/api/api.rb b/lib/api/api.rb index 7834262d61..266b5f48f8 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -54,5 +54,7 @@ module API mount Keys mount Tags mount Triggers + + mount Builds end end diff --git a/lib/api/builds.rb b/lib/api/builds.rb new file mode 100644 index 0000000000..ce4892b5ee --- /dev/null +++ b/lib/api/builds.rb @@ -0,0 +1,80 @@ +module API + # Projects builds API + class Builds < Grape::API + before { authenticate! } + + resource :projects do + # Get a project repository commits + # + # Parameters: + # id (required) - The ID of a project + # scope (optional) - The scope of builds to show (one of: all, finished, running) + # page (optional) - The page number for pagination (default: 1) + # per_page (ooptional) - The value of items per page to show (default 30) + # Example Request: + # GET /projects/:id/builds/all + get ':id/builds' do + all_builds = user_project.builds + builds = all_builds.order('created_at DESC') + builds = + case params[:scope] + when 'all' + builds + when 'finished' + builds.finished + when 'running' + builds.running + when 'pending' + builds.pending + when 'success' + builds.success + when 'failed' + builds.failed + else + builds.running_or_pending.reverse_order + end + + page = (params[:page] || 1).to_i + per_page = (params[:per_page] || 30).to_i + + present builds.page(page).per(per_page), with: Entities::Build + end + + # Get a specific build of a project + # + # Parameters: + # id (required) - The ID of a project + # build_id (required) - The ID of a build + # Example Request: + # GET /projects/:id/builds/:build_id + get ':id/builds/:build_id' do + present get_build(params[:build_id]), with: Entities::Build + end + + # Get a trace of a specific build of a project + # + # Parameters: + # id (required) - The ID of a project + # build_id (required) - The ID of a build + # Example Request: + # GET /projects/:id/build/:build_id/trace + get ':id/builds/:build_id/trace' do + trace = get_build(params[:build_id]).trace + trace = + unless trace.nil? + trace.split("\n") + else + [] + end + + present trace + end + end + + helpers do + def get_build(id) + user_project.builds.where(id: id).first + end + end + end +end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index f8511ac5f5..0bf50490ea 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -366,5 +366,18 @@ module API class TriggerRequest < Grape::Entity expose :id, :variables end + + class Build < Grape::Entity + expose :id + expose :status + expose :stage + expose :name + expose :ref + expose :commit + expose :runner + expose :created_at + expose :started_at + expose :finished_at + end end end From b5fef34f1e3beb60e7184cfb3420976bfa367137 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 24 Dec 2015 19:18:01 +0100 Subject: [PATCH 002/218] Fix example request url --- lib/api/builds.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/api/builds.rb b/lib/api/builds.rb index ce4892b5ee..0ddb9e98de 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -12,7 +12,7 @@ module API # page (optional) - The page number for pagination (default: 1) # per_page (ooptional) - The value of items per page to show (default 30) # Example Request: - # GET /projects/:id/builds/all + # GET /projects/:id/builds get ':id/builds' do all_builds = user_project.builds builds = all_builds.order('created_at DESC') From f39959d00a1358ba7d73ebeaccb827738c8151ba Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 28 Dec 2015 13:09:51 +0100 Subject: [PATCH 003/218] Add some fixes to builds API --- lib/api/builds.rb | 27 +++++++++------------------ lib/api/entities.rb | 20 ++++++++++++++++++-- 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/lib/api/builds.rb b/lib/api/builds.rb index 0ddb9e98de..863be0d5e4 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -15,23 +15,15 @@ module API # GET /projects/:id/builds get ':id/builds' do all_builds = user_project.builds - builds = all_builds.order('created_at DESC') + builds = all_builds.order('id DESC') builds = case params[:scope] - when 'all' - builds when 'finished' builds.finished when 'running' builds.running - when 'pending' - builds.pending - when 'success' - builds.success - when 'failed' - builds.failed else - builds.running_or_pending.reverse_order + builds end page = (params[:page] || 1).to_i @@ -59,15 +51,14 @@ module API # Example Request: # GET /projects/:id/build/:build_id/trace get ':id/builds/:build_id/trace' do - trace = get_build(params[:build_id]).trace - trace = - unless trace.nil? - trace.split("\n") - else - [] - end + build = get_build(params[:build_id]) - present trace + header 'Content-Disposition', "infile; filename=\"#{build.id}.log\"" + content_type 'text/plain' + env['api.format'] = :binary + + trace = build.trace + body trace end end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 0bf50490ea..76b5d14f20 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -367,14 +367,30 @@ module API expose :id, :variables end + class CiCommit < Grape::Entity + expose :id + expose :ref + expose :sha + expose :committed_at + end + + class CiRunner < Grape::Entity + expose :id + expose :token + expose :description + expose :active + expose :is_shared + expose :name + end + class Build < Grape::Entity expose :id expose :status expose :stage expose :name expose :ref - expose :commit - expose :runner + expose :commit, with: CiCommit + expose :runner, with: CiRunner expose :created_at expose :started_at expose :finished_at From f4cff4dcd0b11d4597efe454731838fbf5803516 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 28 Dec 2015 13:33:03 +0100 Subject: [PATCH 004/218] Modify build pagination to use 'paginate' helper --- lib/api/builds.rb | 5 +---- lib/api/helpers.rb | 6 ++++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/api/builds.rb b/lib/api/builds.rb index 863be0d5e4..f219b0f524 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -26,10 +26,7 @@ module API builds end - page = (params[:page] || 1).to_i - per_page = (params[:per_page] || 30).to_i - - present builds.page(page).per(per_page), with: Entities::Build + present paginate(builds), with: Entities::Build end # Get a specific build of a project diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index a4df810e75..8fb5cd6ab6 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -97,8 +97,10 @@ module API end def paginate(relation) - per_page = params[:per_page].to_i - paginated = relation.page(params[:page]).per(per_page) + page = (params[:page] || 1).to_i + per_page = (params[:per_page] || 30).to_i + + paginated = relation.page(page).per(per_page) add_pagination_headers(paginated, per_page) paginated From d398e78ea09c1f88800cd4c99f92dd8e5d1d0d39 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 28 Dec 2015 15:49:13 +0100 Subject: [PATCH 005/218] Add endpoint for getting builds for a specific commit --- lib/api/builds.rb | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/lib/api/builds.rb b/lib/api/builds.rb index f219b0f524..7488863cdc 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -4,7 +4,7 @@ module API before { authenticate! } resource :projects do - # Get a project repository commits + # Get a project builds # # Parameters: # id (required) - The ID of a project @@ -14,18 +14,21 @@ module API # Example Request: # GET /projects/:id/builds get ':id/builds' do - all_builds = user_project.builds - builds = all_builds.order('id DESC') - builds = - case params[:scope] - when 'finished' - builds.finished - when 'running' - builds.running - else - builds - end + builds = user_project.builds.order('id DESC') + builds = filter_builds(builds, params[:scope]) + present paginate(builds), with: Entities::Build + end + # GET builds for a specific commit of a project + # + # Parameters: + # id (required) - The ID of a project + # sha (required) - The SHA id of a commit + # Example Request: + # GET /projects/:id/builds/commit/:sha + get ':id/builds/commit/:sha' do + builds = user_project.ci_commits.find_by_sha(params[:sha]).builds.order('id DESC') + builds = filter_builds(builds, params[:scope]) present paginate(builds), with: Entities::Build end @@ -63,6 +66,17 @@ module API def get_build(id) user_project.builds.where(id: id).first end + + def filter_builds(builds, scope) + case scope + when 'finished' + builds.finished + when 'running' + builds.running + else + builds + end + end end end end From e7d0746d9319119c459581615ae4205139bee444 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 28 Dec 2015 16:38:02 +0100 Subject: [PATCH 006/218] Add 'not_found' notifications in build/trace details --- lib/api/builds.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/api/builds.rb b/lib/api/builds.rb index 7488863cdc..5ac24d0367 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -19,7 +19,7 @@ module API present paginate(builds), with: Entities::Build end - # GET builds for a specific commit of a project + # Get builds for a specific commit of a project # # Parameters: # id (required) - The ID of a project @@ -40,7 +40,10 @@ module API # Example Request: # GET /projects/:id/builds/:build_id get ':id/builds/:build_id' do - present get_build(params[:build_id]), with: Entities::Build + build = get_build(params[:build_id]) + return not_found!(build) unless build + + present build, with: Entities::Build end # Get a trace of a specific build of a project @@ -52,6 +55,7 @@ module API # GET /projects/:id/build/:build_id/trace get ':id/builds/:build_id/trace' do build = get_build(params[:build_id]) + return not_found!(build) unless build header 'Content-Disposition', "infile; filename=\"#{build.id}.log\"" content_type 'text/plain' From 8d4555037a844d21dbf56c2995cff30782af920b Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 28 Dec 2015 16:38:29 +0100 Subject: [PATCH 007/218] Add cancel/retry endpoints to build API --- lib/api/builds.rb | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/lib/api/builds.rb b/lib/api/builds.rb index 5ac24d0367..16e4549d28 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -64,6 +64,42 @@ module API trace = build.trace body trace end + + # cancel a specific build of a project + # + # parameters: + # id (required) - the id of a project + # build_id (required) - the id of a build + # example request: + # post /projects/:id/build/:build_id/cancel + post ':id/builds/:build_id/cancel' do + authorize_manage_builds! + + build = get_build(params[:build_id]) + return not_found!(build) unless build + + build.cancel + + present build, with: Entities::Build + end + + # cancel a specific build of a project + # + # parameters: + # id (required) - the id of a project + # build_id (required) - the id of a build + # example request: + # post /projects/:id/build/:build_id/retry + post ':id/builds/:build_id/retry' do + authorize_manage_builds! + + build = get_build(params[:build_id]) + return not_found!(build) unless build && build.retryable? + + build = Ci::Build.retry(build) + + present build, with: Entities::Build + end end helpers do @@ -81,6 +117,10 @@ module API builds end end + + def authorize_manage_builds! + authorize! :manage_builds, user_project + end end end end From b2fbeb377d8ba17352d40817b0b444196cb97636 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 28 Dec 2015 17:01:02 +0100 Subject: [PATCH 008/218] Remove changes in 'paginate' helper --- lib/api/helpers.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 8fb5cd6ab6..a4df810e75 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -97,10 +97,8 @@ module API end def paginate(relation) - page = (params[:page] || 1).to_i - per_page = (params[:per_page] || 30).to_i - - paginated = relation.page(page).per(per_page) + per_page = params[:per_page].to_i + paginated = relation.page(params[:page]).per(per_page) add_pagination_headers(paginated, per_page) paginated From d2601211a0c7a44666501dea82a8488b08f8faa7 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 29 Dec 2015 23:12:36 +0100 Subject: [PATCH 009/218] Add specs for build listings in API --- lib/api/builds.rb | 9 ++++-- spec/requests/api/builds_spec.rb | 52 ++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) create mode 100644 spec/requests/api/builds_spec.rb diff --git a/lib/api/builds.rb b/lib/api/builds.rb index 16e4549d28..a519224d2b 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -27,7 +27,10 @@ module API # Example Request: # GET /projects/:id/builds/commit/:sha get ':id/builds/commit/:sha' do - builds = user_project.ci_commits.find_by_sha(params[:sha]).builds.order('id DESC') + commit = user_project.ci_commits.find_by_sha(params[:sha]) + return not_found! unless commit + + builds = commit.builds.order('id DESC') builds = filter_builds(builds, params[:scope]) present paginate(builds), with: Entities::Build end @@ -65,7 +68,7 @@ module API body trace end - # cancel a specific build of a project + # Cancel a specific build of a project # # parameters: # id (required) - the id of a project @@ -83,7 +86,7 @@ module API present build, with: Entities::Build end - # cancel a specific build of a project + # Retry a specific build of a project # # parameters: # id (required) - the id of a project diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb new file mode 100644 index 0000000000..81c176c9fb --- /dev/null +++ b/spec/requests/api/builds_spec.rb @@ -0,0 +1,52 @@ +require 'spec_helper' + +describe API::API, api: true do + include ApiHelpers + + let(:user) { create(:user) } + let(:user2) { create(:user) } + let!(:project) { create(:project, creator_id: user.id) } + let!(:master) { create(:project_member, user: user, project: project, access_level: ProjectMember::MASTER) } + let!(:guest) { create(:project_member, user: user2, project: project, access_level: ProjectMember::GUEST) } + + describe 'GET /projects/:id/builds ' do + context 'authorized user' do + it 'should return project builds' do + get api("/projects/#{project.id}/builds", user) + + puts json_response + expect(response.status).to eq(200) + expect(json_response).to be_an Array + end + end + + context 'unauthorized user' do + it 'should not return project builds' do + get api("/projects/#{project.id}/builds") + + expect(response.status).to eq(401) + end + end + end + + describe 'GET /projects/:id/builds/commit/:sha' do + context 'authorized user' do + it 'should return project builds for specific commit' do + project.ensure_ci_commit(project.repository.commit.sha) + get api("/projects/#{project.id}/builds/commit/#{project.ci_commits.first.sha}", user) + + expect(response.status).to eq(200) + expect(json_response).to be_an Array + end + end + + context 'unauthorized user' do + it 'should not return project builds' do + project.ensure_ci_commit(project.repository.commit.sha) + get api("/projects/#{project.id}/builds/commit/#{project.ci_commits.first.sha}") + + expect(response.status).to eq(401) + end + end + end +end From 593d87ea54eec4d60cf7eeb404af82d9e015b066 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 30 Dec 2015 15:12:07 +0100 Subject: [PATCH 010/218] Add specs for build details/traces features in builds API --- spec/factories/ci/builds.rb | 6 +++++ spec/requests/api/builds_spec.rb | 40 ++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index f76e826f13..4551ee57d7 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -30,6 +30,7 @@ FactoryGirl.define do name 'test' ref 'master' tag false + created_at 'Di 29. Okt 09:50:00 CET 2013' started_at 'Di 29. Okt 09:51:28 CET 2013' finished_at 'Di 29. Okt 09:53:28 CET 2013' commands 'ls -a' @@ -54,5 +55,10 @@ FactoryGirl.define do factory :ci_build_tag do tag true end + + factory :ci_build_with_trace do + id 999 + trace 'BUILD TRACE' + end end end diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb index 81c176c9fb..c68ea0898b 100644 --- a/spec/requests/api/builds_spec.rb +++ b/spec/requests/api/builds_spec.rb @@ -8,6 +8,9 @@ describe API::API, api: true do let!(:project) { create(:project, creator_id: user.id) } let!(:master) { create(:project_member, user: user, project: project, access_level: ProjectMember::MASTER) } let!(:guest) { create(:project_member, user: user2, project: project, access_level: ProjectMember::GUEST) } + let(:commit) { create(:ci_commit, project: project)} + let(:build) { create(:ci_build, commit: commit) } + let(:build_with_trace) { create(:ci_build_with_trace, commit: commit) } describe 'GET /projects/:id/builds ' do context 'authorized user' do @@ -32,7 +35,7 @@ describe API::API, api: true do describe 'GET /projects/:id/builds/commit/:sha' do context 'authorized user' do it 'should return project builds for specific commit' do - project.ensure_ci_commit(project.repository.commit.sha) + project.ensure_ci_commit(commit.sha) get api("/projects/#{project.id}/builds/commit/#{project.ci_commits.first.sha}", user) expect(response.status).to eq(200) @@ -42,11 +45,44 @@ describe API::API, api: true do context 'unauthorized user' do it 'should not return project builds' do - project.ensure_ci_commit(project.repository.commit.sha) + project.ensure_ci_commit(commit.sha) get api("/projects/#{project.id}/builds/commit/#{project.ci_commits.first.sha}") expect(response.status).to eq(401) end end end + + describe 'GET /projects/:id/builds/:build_id(/trace)?' do + context 'authorized user' do + it 'should return specific build data' do + get api("/projects/#{project.id}/builds/#{build.id}", user) + + expect(response.status).to eq(200) + expect(json_response['name']).to eq('test') + expect(json_response['commit']['sha']).to eq(commit.sha) + end + + it 'should return specific build trace' do + get api("/projects/#{project.id}/builds/#{build_with_trace.id}/trace", user) + + expect(response.status).to eq(200) + expect(response.body).to eq(build_with_trace.trace) + end + end + + context 'unauthorized user' do + it 'should not return specific build data' do + get api("/projects/#{project.id}/builds/#{build.id}") + + expect(response.status).to eq(401) + end + + it 'should not return specific build trace' do + get api("/projects/#{project.id}/builds/#{build_with_trace.id}/trace") + + expect(response.status).to eq(401) + end + end + end end From a17bf380cb4c90696349f268ca4a8c2fedc1f545 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 30 Dec 2015 16:37:47 +0100 Subject: [PATCH 011/218] Add cancel/retry features to builds API --- spec/factories/ci/builds.rb | 4 +++ spec/requests/api/builds_spec.rb | 62 +++++++++++++++++++++++++++++++- 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 4551ee57d7..ce68457f86 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -60,5 +60,9 @@ FactoryGirl.define do id 999 trace 'BUILD TRACE' end + + factory :ci_build_canceled do + status 'canceled' + end end end diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb index c68ea0898b..d4af7639d4 100644 --- a/spec/requests/api/builds_spec.rb +++ b/spec/requests/api/builds_spec.rb @@ -7,10 +7,11 @@ describe API::API, api: true do let(:user2) { create(:user) } let!(:project) { create(:project, creator_id: user.id) } let!(:master) { create(:project_member, user: user, project: project, access_level: ProjectMember::MASTER) } - let!(:guest) { create(:project_member, user: user2, project: project, access_level: ProjectMember::GUEST) } + let!(:reporter) { create(:project_member, user: user2, project: project, access_level: ProjectMember::REPORTER) } let(:commit) { create(:ci_commit, project: project)} let(:build) { create(:ci_build, commit: commit) } let(:build_with_trace) { create(:ci_build_with_trace, commit: commit) } + let(:build_canceled) { create(:ci_build_canceled, commit: commit) } describe 'GET /projects/:id/builds ' do context 'authorized user' do @@ -85,4 +86,63 @@ describe API::API, api: true do end end end + + describe 'GET /projects/:id/builds/:build_id/cancel' do + context 'authorized user' do + context 'user with :manage_builds persmission' do + it 'should cancel running or pending build' do + post api("/projects/#{project.id}/builds/#{build.id}/cancel", user) + + expect(response.status).to eq(201) + expect(project.builds.first.status).to eq('canceled') + end + end + + context 'user without :manage_builds permission' do + it 'should not cancel build' do + post api("/projects/#{project.id}/builds/#{build.id}/cancel", user2) + + expect(response.status).to eq(403) + end + end + end + + context 'unauthorized user' do + it 'should not cancel build' do + post api("/projects/#{project.id}/builds/#{build.id}/cancel") + + expect(response.status).to eq(401) + end + end + end + + describe 'GET /projects/:id/builds/:build_id/retry' do + context 'authorized user' do + context 'user with :manage_builds persmission' do + it 'should retry non-running build' do + post api("/projects/#{project.id}/builds/#{build_canceled.id}/retry", user) + + expect(response.status).to eq(201) + expect(project.builds.first.status).to eq('canceled') + expect(json_response['status']).to eq('pending') + end + end + + context 'user without :manage_builds permission' do + it 'should not retry build' do + post api("/projects/#{project.id}/builds/#{build_canceled.id}/retry", user2) + + expect(response.status).to eq(403) + end + end + end + + context 'unauthorized user' do + it 'should not retry build' do + post api("/projects/#{project.id}/builds/#{build_canceled.id}/retry") + + expect(response.status).to eq(401) + end + end + end end From ea4777ff501e370a39ae30e76a955136afe3c1fa Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 31 Dec 2015 15:19:13 +0100 Subject: [PATCH 012/218] Add features for list and show details of variables in API --- app/models/ci/variable.rb | 1 + lib/api/api.rb | 2 + lib/api/entities.rb | 4 ++ lib/api/variables.rb | 43 +++++++++++++++++ spec/factories/ci/variables.rb | 25 ++++++++++ spec/requests/api/variables_spec.rb | 75 +++++++++++++++++++++++++++++ 6 files changed, 150 insertions(+) create mode 100644 lib/api/variables.rb create mode 100644 spec/factories/ci/variables.rb create mode 100644 spec/requests/api/variables_spec.rb diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb index 56759d3e50..0e2712086c 100644 --- a/app/models/ci/variable.rb +++ b/app/models/ci/variable.rb @@ -9,6 +9,7 @@ # encrypted_value :text # encrypted_value_salt :string(255) # encrypted_value_iv :string(255) +# gl_project_id :integer # module Ci diff --git a/lib/api/api.rb b/lib/api/api.rb index 7834262d61..a9e1913f0f 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -54,5 +54,7 @@ module API mount Keys mount Tags mount Triggers + + mount Variables end end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 26e7c956e8..f71d072f26 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -365,5 +365,9 @@ module API class TriggerRequest < Grape::Entity expose :id, :variables end + + class Variable < Grape::Entity + expose :id, :key, :value + end end end diff --git a/lib/api/variables.rb b/lib/api/variables.rb new file mode 100644 index 0000000000..6517150f6f --- /dev/null +++ b/lib/api/variables.rb @@ -0,0 +1,43 @@ +module API + # Projects variables API + class Variables < Grape::API + before { authenticate! } + before { authorize_admin_project } + + resource :projects do + # Get project variables + # + # Parameters: + # id (required) - The ID of a project + # page (optional) - The page number for pagination + # per_page (optional) - The value of items per page to show + # Example Request: + # GET /projects/:id/variables + get ':id/variables' do + variables = user_project.variables + present paginate(variables), with: Entities::Variable + end + + # Get specifica bariable of a project + # + # Parameters: + # id (required) - The ID of a project + # variable_id (required) - The ID OR `key` of variable to show; if variable_id contains only digits it's treated + # as ID other ways it's treated as `key` + # Example Reuest: + # GET /projects/:id/variables/:variable_id + get ':id/variables/:variable_id' do + variable_id = params[:variable_id] + variables = user_project.variables + variables = + if variable_id.match(/^\d+$/) + variables.where(id: variable_id.to_i) + else + variables.where(key: variable_id) + end + + present variables.first, with: Entities::Variable + end + end + end +end diff --git a/spec/factories/ci/variables.rb b/spec/factories/ci/variables.rb new file mode 100644 index 0000000000..c3dcb678da --- /dev/null +++ b/spec/factories/ci/variables.rb @@ -0,0 +1,25 @@ +# == Schema Information +# +# Table name: ci_variables +# +# id :integer not null, primary key +# project_id :integer not null +# key :string(255) +# value :text +# encrypted_value :text +# encrypted_value_salt :string(255) +# encrypted_value_iv :string(255) +# gl_project_id :integer +# + +# Read about factories at https://github.com/thoughtbot/factory_girl + +FactoryGirl.define do + factory :ci_variable, class: Ci::Variable do + id 1 + key 'TEST_VARIABLE_1' + value 'VALUE_1' + + project factory: :empty_project + end +end diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb new file mode 100644 index 0000000000..8f66f5432b --- /dev/null +++ b/spec/requests/api/variables_spec.rb @@ -0,0 +1,75 @@ +require 'spec_helper' + +describe API::API, api: true do + include ApiHelpers + + let(:user) { create(:user) } + let(:user2) { create(:user) } + let!(:project) { create(:project, creator_id: user.id) } + let!(:master) { create(:project_member, user: user, project: project, access_level: ProjectMember::MASTER) } + let!(:developer) { create(:project_member, user: user2, project: project, access_level: ProjectMember::DEVELOPER) } + let!(:variable) { create(:ci_variable, project: project) } + + describe 'GET /projects/:id/variables' do + context 'authorized user with proper permissions' do + it 'should return project variables' do + get api("/projects/#{project.id}/variables", user) + + expect(response.status).to eq(200) + expect(json_response).to be_a(Array) + end + end + + context 'authorized user with invalid permissions' do + it 'should not return project variables' do + get api("/projects/#{project.id}/variables", user2) + + expect(response.status).to eq(403) + end + end + + context 'unauthorized user' do + it 'should not return project variables' do + get api("/projects/#{project.id}/variables") + + expect(response.status).to eq(401) + end + end + end + + describe 'GET /projects/:id/variables/:variable_id' do + context 'authorized user with proper permissions' do + it 'should return project variable details when ID is used as :variable_id' do + get api("/projects/#{project.id}/variables/1", user) + + expect(response.status).to eq(200) + expect(json_response['key']).to eq('TEST_VARIABLE_1') + expect(json_response['value']).to eq('VALUE_1') + end + + it 'should return project variable details when `key` is used as :variable_id' do + get api("/projects/#{project.id}/variables/TEST_VARIABLE_1", user) + + expect(response.status).to eq(200) + expect(json_response['id']).to eq(1) + expect(json_response['value']).to eq('VALUE_1') + end + end + + context 'authorized user with invalid permissions' do + it 'should not return project variable details' do + get api("/projects/#{project.id}/variables/1", user2) + + expect(response.status).to eq(403) + end + end + + context 'unauthorized user' do + it 'should not return project variable details' do + get api("/projects/#{project.id}/variables/1") + + expect(response.status).to eq(401) + end + end + end +end From a692ce1c079703c4f3947e1d0a29547189e94d0f Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 31 Dec 2015 16:25:49 +0100 Subject: [PATCH 013/218] Add update feature for variables API --- lib/api/variables.rb | 21 +++++++++++- spec/requests/api/variables_spec.rb | 52 ++++++++++++++++++++++++----- 2 files changed, 64 insertions(+), 9 deletions(-) diff --git a/lib/api/variables.rb b/lib/api/variables.rb index 6517150f6f..6522ecba70 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -24,7 +24,7 @@ module API # id (required) - The ID of a project # variable_id (required) - The ID OR `key` of variable to show; if variable_id contains only digits it's treated # as ID other ways it's treated as `key` - # Example Reuest: + # Example Request: # GET /projects/:id/variables/:variable_id get ':id/variables/:variable_id' do variable_id = params[:variable_id] @@ -38,6 +38,25 @@ module API present variables.first, with: Entities::Variable end + + # Update existing variable of a project + # + # Parameters: + # id (required) - The ID of a project + # variable_id (required) - The ID of a variable + # key (optional) - new value for `key` field of variable + # value (optional) - new value for `value` field of variable + # Example Request: + # PUT /projects/:id/variables/:variable_id + put ':id/variables/:variable_id' do + variable = user_project.variables.where(id: params[:variable_id].to_i).first + + variable.key = params[:key] + variable.value = params[:value] + variable.save! + + present variable, with: Entities::Variable + end end end end diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb index 8f66f5432b..3f58277c4a 100644 --- a/spec/requests/api/variables_spec.rb +++ b/spec/requests/api/variables_spec.rb @@ -40,25 +40,25 @@ describe API::API, api: true do describe 'GET /projects/:id/variables/:variable_id' do context 'authorized user with proper permissions' do it 'should return project variable details when ID is used as :variable_id' do - get api("/projects/#{project.id}/variables/1", user) + get api("/projects/#{project.id}/variables/#{variable.id}", user) expect(response.status).to eq(200) - expect(json_response['key']).to eq('TEST_VARIABLE_1') - expect(json_response['value']).to eq('VALUE_1') + expect(json_response['key']).to eq(variable.key) + expect(json_response['value']).to eq(variable.value) end it 'should return project variable details when `key` is used as :variable_id' do - get api("/projects/#{project.id}/variables/TEST_VARIABLE_1", user) + get api("/projects/#{project.id}/variables/#{variable.key}", user) expect(response.status).to eq(200) - expect(json_response['id']).to eq(1) - expect(json_response['value']).to eq('VALUE_1') + expect(json_response['id']).to eq(variable.id) + expect(json_response['value']).to eq(variable.value) end end context 'authorized user with invalid permissions' do it 'should not return project variable details' do - get api("/projects/#{project.id}/variables/1", user2) + get api("/projects/#{project.id}/variables/#{variable.id}", user2) expect(response.status).to eq(403) end @@ -66,7 +66,43 @@ describe API::API, api: true do context 'unauthorized user' do it 'should not return project variable details' do - get api("/projects/#{project.id}/variables/1") + get api("/projects/#{project.id}/variables/#{variable.id}") + + expect(response.status).to eq(401) + end + end + end + + describe 'PUT /projects/:id/variables/:variable_id' do + context 'authorized user with proper permissions' do + it 'should update variable data' do + initial_variable = project.variables.first + key_before = initial_variable.key + value_before = initial_variable.value + + put api("/projects/#{project.id}/variables/#{variable.id}", user), key: 'TEST_VARIABLE_1_UP', value: 'VALUE_1_UP' + + updated_variable = project.variables.first + + expect(response.status).to eq(200) + expect(key_before).to eq(variable.key) + expect(value_before).to eq(variable.value) + expect(updated_variable.key).to eq('TEST_VARIABLE_1_UP') + expect(updated_variable.value).to eq('VALUE_1_UP') + end + end + + context 'authorized user with invalid permissions' do + it 'should not update variable' do + put api("/projects/#{project.id}/variables/#{variable.id}", user2) + + expect(response.status).to eq(403) + end + end + + context 'unauthorized user' do + it 'should not return project variable details' do + put api("/projects/#{project.id}/variables/#{variable.id}") expect(response.status).to eq(401) end From 0d014feb1d216e692882976f0d70c3227eaec4ca Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 31 Dec 2015 16:56:03 +0100 Subject: [PATCH 014/218] Add delete feature to variables API --- lib/api/variables.rb | 12 ++++++++++++ spec/requests/api/variables_spec.rb | 29 ++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/lib/api/variables.rb b/lib/api/variables.rb index 6522ecba70..c70c7cd9d7 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -57,6 +57,18 @@ module API present variable, with: Entities::Variable end + + # Delete existing variable of a project + # + # Parameters: + # id (required) - The ID of a project + # variable_id (required) - The ID of a variable + # Exanoke Reqyest: + # DELETE /projects/:id/variables/:variable_id + delete ':id/variables/:variable_id' do + variable = user_project.variables.where(id: params[:variable_id].to_i).first + variable.destroy + end end end end diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb index 3f58277c4a..385db2409b 100644 --- a/spec/requests/api/variables_spec.rb +++ b/spec/requests/api/variables_spec.rb @@ -101,11 +101,38 @@ describe API::API, api: true do end context 'unauthorized user' do - it 'should not return project variable details' do + it 'should not update variable' do put api("/projects/#{project.id}/variables/#{variable.id}") expect(response.status).to eq(401) end end end + + describe 'DELETE /projects/:id/variables/:variable_id' do + context 'authorized user with proper permissions' do + it 'should delete variable' do + expect do + delete api("/projects/#{project.id}/variables/#{variable.id}", user) + end.to change{project.variables.count}.by(-1) + expect(response.status).to eq(200) + end + end + + context 'authorized user with invalid permissions' do + it 'should not delete variable' do + delete api("/projects/#{project.id}/variables/#{variable.id}", user2) + + expect(response.status).to eq(403) + end + end + + context 'unauthorized user' do + it 'should not delete variable' do + delete api("/projects/#{project.id}/variables/#{variable.id}") + + expect(response.status).to eq(401) + end + end + end end From c5177dd5e2171b047a695802c979cf779522ba8a Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 31 Dec 2015 17:03:11 +0100 Subject: [PATCH 015/218] Add missing 'not_found' checks in variables API --- lib/api/variables.rb | 7 +++++++ spec/requests/api/variables_spec.rb | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/lib/api/variables.rb b/lib/api/variables.rb index c70c7cd9d7..dac2ba679c 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -36,6 +36,8 @@ module API variables.where(key: variable_id) end + return not_found!('Variable') if variables.empty? + present variables.first, with: Entities::Variable end @@ -51,6 +53,8 @@ module API put ':id/variables/:variable_id' do variable = user_project.variables.where(id: params[:variable_id].to_i).first + return not_found!('Variable') unless variable + variable.key = params[:key] variable.value = params[:value] variable.save! @@ -67,6 +71,9 @@ module API # DELETE /projects/:id/variables/:variable_id delete ':id/variables/:variable_id' do variable = user_project.variables.where(id: params[:variable_id].to_i).first + + return not_found!('Variable') unless variable + variable.destroy end end diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb index 385db2409b..b35ee2d32d 100644 --- a/spec/requests/api/variables_spec.rb +++ b/spec/requests/api/variables_spec.rb @@ -54,6 +54,12 @@ describe API::API, api: true do expect(json_response['id']).to eq(variable.id) expect(json_response['value']).to eq(variable.value) end + + it 'should responde with 404 Not Found if requesting non-existing variable' do + get api("/projects/#{project.id}/variables/9999", user) + + expect(response.status).to eq(404) + end end context 'authorized user with invalid permissions' do @@ -90,6 +96,12 @@ describe API::API, api: true do expect(updated_variable.key).to eq('TEST_VARIABLE_1_UP') expect(updated_variable.value).to eq('VALUE_1_UP') end + + it 'should responde with 404 Not Found if requesting non-existing variable' do + put api("/projects/#{project.id}/variables/9999", user) + + expect(response.status).to eq(404) + end end context 'authorized user with invalid permissions' do @@ -117,6 +129,12 @@ describe API::API, api: true do end.to change{project.variables.count}.by(-1) expect(response.status).to eq(200) end + + it 'should responde with 404 Not Found if requesting non-existing variable' do + delete api("/projects/#{project.id}/variables/9999", user) + + expect(response.status).to eq(404) + end end context 'authorized user with invalid permissions' do From 937567b767e6d7b34dcaa1d9c83fc75464638683 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 31 Dec 2015 22:30:07 +0100 Subject: [PATCH 016/218] Add create feature to variables API --- lib/api/variables.rb | 20 +++++++++++++++ spec/factories/ci/variables.rb | 2 +- spec/requests/api/variables_spec.rb | 38 +++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 1 deletion(-) diff --git a/lib/api/variables.rb b/lib/api/variables.rb index dac2ba679c..fc63ac2f56 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -41,6 +41,24 @@ module API present variables.first, with: Entities::Variable end + # Create a new variable in project + # + # Parameters: + # id (required) - The ID of a project + # key (required) - The key of variable being created + # value (required) - The value of variable being created + # Example Request: + # POST /projects/:id/variables + post ':id/variables' do + required_attributes! [:key, :value] + + variable = user_project.variables.create(key: params[:key], value: params[:value]) + return render_validation_error!(variable) unless variable.valid? + variable.save! + + present variable, with: Entities::Variable + end + # Update existing variable of a project # # Parameters: @@ -75,6 +93,8 @@ module API return not_found!('Variable') unless variable variable.destroy + + present variable, with: Entities::Variable end end end diff --git a/spec/factories/ci/variables.rb b/spec/factories/ci/variables.rb index c3dcb678da..a19b9fc72f 100644 --- a/spec/factories/ci/variables.rb +++ b/spec/factories/ci/variables.rb @@ -16,7 +16,7 @@ FactoryGirl.define do factory :ci_variable, class: Ci::Variable do - id 1 + id 10 key 'TEST_VARIABLE_1' value 'VALUE_1' diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb index b35ee2d32d..bf0dd77473 100644 --- a/spec/requests/api/variables_spec.rb +++ b/spec/requests/api/variables_spec.rb @@ -79,6 +79,44 @@ describe API::API, api: true do end end + describe 'POST /projects/:id/variables' do + context 'authorized user with proper permissions' do + it 'should create variable' do + expect do + post api("/projects/#{project.id}/variables", user), key: 'TEST_VARIABLE_2', value: 'VALUE_2' + end.to change{project.variables.count}.by(1) + + expect(response.status).to eq(201) + expect(json_response['key']).to eq('TEST_VARIABLE_2') + expect(json_response['value']).to eq('VALUE_2') + end + + it 'should not allow to duplicate variable key' do + expect do + post api("/projects/#{project.id}/variables", user), key: 'TEST_VARIABLE_1', value: 'VALUE_2' + end.to change{project.variables.count}.by(0) + + expect(response.status).to eq(400) + end + end + + context 'authorized user with invalid permissions' do + it 'should not create variable' do + post api("/projects/#{project.id}/variables", user2) + + expect(response.status).to eq(403) + end + end + + context 'unauthorized user' do + it 'should not create variable' do + post api("/projects/#{project.id}/variables") + + expect(response.status).to eq(401) + end + end + end + describe 'PUT /projects/:id/variables/:variable_id' do context 'authorized user with proper permissions' do it 'should update variable data' do From 16bd4df083135e2e4a263b2e1bdd71b78a875ef7 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 31 Dec 2015 22:59:06 +0100 Subject: [PATCH 017/218] Fix a typo in method description --- lib/api/variables.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/api/variables.rb b/lib/api/variables.rb index fc63ac2f56..b8bbcb6ce3 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -18,7 +18,7 @@ module API present paginate(variables), with: Entities::Variable end - # Get specifica bariable of a project + # Get specifica variable of a project # # Parameters: # id (required) - The ID of a project From 628297fe5692fc241c93ff34cece71132bfb9aed Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 31 Dec 2015 23:01:28 +0100 Subject: [PATCH 018/218] Remove incorrect 'default' values from method description --- lib/api/builds.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/api/builds.rb b/lib/api/builds.rb index a519224d2b..92bf849824 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -9,8 +9,8 @@ module API # Parameters: # id (required) - The ID of a project # scope (optional) - The scope of builds to show (one of: all, finished, running) - # page (optional) - The page number for pagination (default: 1) - # per_page (ooptional) - The value of items per page to show (default 30) + # page (optional) - The page number for pagination + # per_page (ooptional) - The value of items per page to show # Example Request: # GET /projects/:id/builds get ':id/builds' do From d9da81f736b770bb44c4869aef5d5c455e74ab7a Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 4 Jan 2016 16:38:32 +0100 Subject: [PATCH 019/218] Add triggers feature to API --- lib/api/entities.rb | 10 ++++++ lib/api/triggers.rb | 18 +++++++++++ spec/factories/ci/trigger_requests.rb | 2 ++ spec/requests/api/triggers_spec.rb | 45 ++++++++++++++++++++++++--- 4 files changed, 71 insertions(+), 4 deletions(-) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 26e7c956e8..bc0cd76a2b 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -365,5 +365,15 @@ module API class TriggerRequest < Grape::Entity expose :id, :variables end + + class Trigger < Grape::Entity + expose :id, :token, :created_at, :updated_at, :deleted_at + expose :last_used do |repo_obj, _options| + if repo_obj.respond_to?(:last_trigger_request) + request = repo_obj.last_trigger_request + request.created_at if request + end + end + end end end diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 2781f1cf19..9a1e3fdc97 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -43,6 +43,24 @@ module API render_api_error!(errors, 400) end end + + # Get triggers list + # + # Parameters: + # id (required) - The ID of a project + # page (optional) - The page number for pagination + # per_page (optional) - The value of items per page to show + # Example Request: + # GET /projects/:id/triggers + get ':id/triggers' do + authenticate! + authorize_admin_project + + triggers = user_project.triggers.includes(:trigger_requests) + triggers = paginate(triggers) + + present triggers, with: Entities::Trigger + end end end end diff --git a/spec/factories/ci/trigger_requests.rb b/spec/factories/ci/trigger_requests.rb index db053c610c..5298d9fa7c 100644 --- a/spec/factories/ci/trigger_requests.rb +++ b/spec/factories/ci/trigger_requests.rb @@ -3,6 +3,8 @@ FactoryGirl.define do factory :ci_trigger_request, class: Ci::TriggerRequest do factory :ci_trigger_request_with_variables do + trigger :ci_trigger + variables do { TRIGGER_KEY: 'TRIGGER_VALUE' diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index 314bd7ddc5..4b356108c8 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -3,11 +3,19 @@ require 'spec_helper' describe API::API do include ApiHelpers + let(:user) { create(:user) } + let(:user2) { create(:user) } + let!(:trigger_token) { 'secure token' } + let!(:trigger_token_2) { 'secure token 2' } + let!(:project) { create(:project, creator_id: user.id) } + let!(:master) { create(:project_member, user: user, project: project, access_level: ProjectMember::MASTER) } + let!(:developer) { create(:project_member, user: user2, project: project, access_level: ProjectMember::DEVELOPER) } + let!(:trigger) { create(:ci_trigger, project: project, token: trigger_token) } + let!(:trigger2) { create(:ci_trigger, project: project, token: trigger_token_2) } + let!(:trigger_request) { create(:ci_trigger_request, trigger: trigger, created_at: '2015-01-01 12:13:14') } + describe 'POST /projects/:project_id/trigger' do - let!(:trigger_token) { 'secure token' } - let!(:project) { FactoryGirl.create(:project) } - let!(:project2) { FactoryGirl.create(:empty_project) } - let!(:trigger) { FactoryGirl.create(:ci_trigger, project: project, token: trigger_token) } + let!(:project2) { create(:empty_project) } let(:options) do { token: trigger_token @@ -77,4 +85,33 @@ describe API::API do end end end + + describe 'GET /projects/:id/triggets' do + context 'authenticated user with valid permissions' do + it 'should return list of triggers' do + get api("/projects/#{project.id}/triggers", user) + + expect(response.status).to eq(200) + expect(json_response).to be_a(Array) + expect(json_response[0]['token']).to eq(trigger_token) + expect(json_response[1]['token']).to eq(trigger_token_2) + end + end + + context 'authenticated user with invalid permissions' do + it 'should not return triggers list' do + get api("/projects/#{project.id}/triggers", user2) + + expect(response.status).to eq(403) + end + end + + context 'unauthentikated user' do + it 'should not return triggers list' do + get api("/projects/#{project.id}/triggers") + + expect(response.status).to eq(401) + end + end + end end From 3098500835aad26748d982fa81c84a2deed97931 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 5 Jan 2016 11:01:44 +0100 Subject: [PATCH 020/218] Fix ci_trigger_request factory --- spec/factories/ci/trigger_requests.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/factories/ci/trigger_requests.rb b/spec/factories/ci/trigger_requests.rb index 5298d9fa7c..2c0d004d26 100644 --- a/spec/factories/ci/trigger_requests.rb +++ b/spec/factories/ci/trigger_requests.rb @@ -3,7 +3,7 @@ FactoryGirl.define do factory :ci_trigger_request, class: Ci::TriggerRequest do factory :ci_trigger_request_with_variables do - trigger :ci_trigger + trigger factory: :ci_trigger variables do { From f00607431cd13a952731e36701ebc3b39e64d09b Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 5 Jan 2016 11:27:38 +0100 Subject: [PATCH 021/218] Add delete feature to triggers API --- lib/api/triggers.rb | 19 +++++++++++++++++ spec/requests/api/triggers_spec.rb | 33 ++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 9a1e3fdc97..3cb7810241 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -61,6 +61,25 @@ module API present triggers, with: Entities::Trigger end + + # Delete trigger + # + # Parameters: + # id (required) - The ID of a project + # trigger_id - The ID of trigger to delete + # Example Request: + # DELETE /projects/:id/triggers/:trigger_id + delete ':id/triggers/:trigger_id' do + authenticate! + authorize_admin_project + + trigger = user_project.triggers.where(id: params[:trigger_id].to_i).first + return not_found!('Trigger') unless trigger + + trigger.destroy + + present trigger, with: Entities::Trigger + end end end end diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index 4b356108c8..4e073a55d9 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -114,4 +114,37 @@ describe API::API do end end end + + describe 'DELETE /projects/:id/triggets/:trigger_id' do + context 'authenticated user with valid permissions' do + it 'should delete trigger' do + expect do + delete api("/projects/#{project.id}/triggers/#{trigger.id}", user) + end.to change{project.triggers.count}.by(-1) + expect(response.status).to eq(200) + end + + it 'should responde with 404 Not Found if requesting non-existing trigger' do + delete api("/projects/#{project.id}/triggers/9999", user) + + expect(response.status).to eq(404) + end + end + + context 'authenticated user with invalid permissions' do + it 'should not delete trigger' do + delete api("/projects/#{project.id}/triggers/#{trigger.id}", user2) + + expect(response.status).to eq(403) + end + end + + context 'unauthentikated user' do + it 'should not delete trigger' do + delete api("/projects/#{project.id}/triggers/#{trigger.id}") + + expect(response.status).to eq(401) + end + end + end end From 49c8bf4e9b510be51859dcc301cb46b29b750cb0 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 5 Jan 2016 11:44:10 +0100 Subject: [PATCH 022/218] Add create feature to triggers API --- lib/api/triggers.rb | 16 ++++++++++++++++ spec/requests/api/triggers_spec.rb | 29 +++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 3cb7810241..38cf1e9a2e 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -62,6 +62,22 @@ module API present triggers, with: Entities::Trigger end + # Create trigger + # + # Parameters: + # id (required) - The ID of a project + # Example Request: + # POST /projects/:id/triggers + post ':id/triggers' do + authenticate! + authorize_admin_project + + trigger = user_project.triggers.new + trigger.save + + present trigger, with: Entities::Trigger + end + # Delete trigger # # Parameters: diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index 4e073a55d9..316c2ae958 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -115,6 +115,35 @@ describe API::API do end end + describe 'POST /projects/:id/triggets' do + context 'authenticated user with valid permissions' do + it 'should create trigger' do + expect do + post api("/projects/#{project.id}/triggers", user) + end.to change{project.triggers.count}.by(1) + + expect(response.status).to eq(201) + expect(json_response).to be_a(Hash) + end + end + + context 'authenticated user with invalid permissions' do + it 'should not create trigger' do + post api("/projects/#{project.id}/triggers", user2) + + expect(response.status).to eq(403) + end + end + + context 'unauthentikated user' do + it 'should not create trigger' do + post api("/projects/#{project.id}/triggers") + + expect(response.status).to eq(401) + end + end + end + describe 'DELETE /projects/:id/triggets/:trigger_id' do context 'authenticated user with valid permissions' do it 'should delete trigger' do From 8675664655c4e0f1e043afa88ff1fd75ae5a6a9e Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 5 Jan 2016 12:25:16 +0100 Subject: [PATCH 023/218] Get show details feature to triggers API --- lib/api/triggers.rb | 26 ++++++++++++++ spec/requests/api/triggers_spec.rb | 56 +++++++++++++++++++++++++++--- 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 38cf1e9a2e..0e548b936c 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -62,6 +62,32 @@ module API present triggers, with: Entities::Trigger end + # Get specific trigger of a project + # + # Parameters: + # id (required) - The ID of a project + # trigger_id (required) - The ID or `token` of a trigger to show; if trigger_id contains only digits it's + # treated as ID other ways it's reated as `key` + # Example Request: + # GET /projects/:id/triggers/:trigger_id + get ':id/triggers/:trigger_id' do + authenticate! + authorize_admin_project + + trigger_id = params[:trigger_id] + triggers = user_project.triggers + triggers = + if trigger_id.match(/^\d+$/) + triggers.where(id: trigger_id.to_i) + else + triggers.where(token: trigger_id) + end + + return not_found!('Trigger') if triggers.empty? + + present triggers.first, with: Entities::Trigger + end + # Create trigger # # Parameters: diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index 316c2ae958..c1c2bb04b2 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -5,8 +5,8 @@ describe API::API do let(:user) { create(:user) } let(:user2) { create(:user) } - let!(:trigger_token) { 'secure token' } - let!(:trigger_token_2) { 'secure token 2' } + let!(:trigger_token) { 'secure_token' } + let!(:trigger_token_2) { 'secure_token_2' } let!(:project) { create(:project, creator_id: user.id) } let!(:master) { create(:project_member, user: user, project: project, access_level: ProjectMember::MASTER) } let!(:developer) { create(:project_member, user: user2, project: project, access_level: ProjectMember::DEVELOPER) } @@ -86,7 +86,7 @@ describe API::API do end end - describe 'GET /projects/:id/triggets' do + describe 'GET /projects/:id/triggers' do context 'authenticated user with valid permissions' do it 'should return list of triggers' do get api("/projects/#{project.id}/triggers", user) @@ -115,7 +115,53 @@ describe API::API do end end - describe 'POST /projects/:id/triggets' do + describe 'GET /projects/:id/triggers/:triggers_id' do + context 'authenticated user with valid permissions' do + context 'ID is used as :trigger_id' do + it 'should return trigger details' do + get api("/projects/#{project.id}/triggers/#{trigger.id}", user) + + expect(response.status).to eq(200) + expect(json_response).to be_a(Hash) + expect(json_response['token']).to eq(trigger_token) + end + end + + context '`token` is used as :trigger_id' do + it 'should return trigger details' do + get api("/projects/#{project.id}/triggers/#{trigger.token}", user) + + expect(response.status).to eq(200) + expect(json_response).to be_a(Hash) + expect(json_response['id']).to eq(trigger.id) + end + end + + it 'should responde with 404 Not Found if requesting non-existing trigger' do + get api("/projects/#{project.id}/triggers/9999", user) + + expect(response.status).to eq(404) + end + end + + context 'authenticated user with invalid permissions' do + it 'should not return triggers list' do + get api("/projects/#{project.id}/triggers/#{trigger.id}", user2) + + expect(response.status).to eq(403) + end + end + + context 'unauthentikated user' do + it 'should not return triggers list' do + get api("/projects/#{project.id}/triggers/#{trigger.id}") + + expect(response.status).to eq(401) + end + end + end + + describe 'POST /projects/:id/triggers' do context 'authenticated user with valid permissions' do it 'should create trigger' do expect do @@ -144,7 +190,7 @@ describe API::API do end end - describe 'DELETE /projects/:id/triggets/:trigger_id' do + describe 'DELETE /projects/:id/triggers/:trigger_id' do context 'authenticated user with valid permissions' do it 'should delete trigger' do expect do From a862ade55bf68f56734538b40e02e56036f8a1bd Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 5 Jan 2016 16:36:14 +0100 Subject: [PATCH 024/218] Update ./doc/api/ --- doc/api/README.md | 1 + doc/api/builds.md | 206 ++++++++++++++++++++++++++++++++++++++++++++++ lib/api/builds.rb | 3 +- 3 files changed, 209 insertions(+), 1 deletion(-) create mode 100644 doc/api/builds.md diff --git a/doc/api/README.md b/doc/api/README.md index 25a31b235c..43ee24ddb6 100644 --- a/doc/api/README.md +++ b/doc/api/README.md @@ -23,6 +23,7 @@ - [Namespaces](namespaces.md) - [Settings](settings.md) - [Keys](keys.md) +- [Builds](builds.md) ## Clients diff --git a/doc/api/builds.md b/doc/api/builds.md new file mode 100644 index 0000000000..b716499dd3 --- /dev/null +++ b/doc/api/builds.md @@ -0,0 +1,206 @@ +# Builds API + +## List project builds + +Get a list of builds in a project. + +``` +GET /projects/:id/builds +``` + +Parameters: + +- `id` (required) - The ID of a project +- `scope` (optional) - The scope of builds to show (one of: `all`, `finished`, `running`; default: `all`) + +```json +[ + { + "commit": { + "committed_at": "2015-12-28T14:34:03.814Z", + "id": 2, + "ref": null, + "sha": "6b053ad388c531c21907f022933e5e81598db388" + }, + "created_at": "2016-01-04T15:41:23.147Z", + "finished_at": null, + "id": 65, + "name": "brakeman", + "ref": "master", + "runner": null, + "stage": "test", + "started_at": null, + "status": "pending" + }, + { + "commit": { + "committed_at": "2015-12-28T14:34:03.814Z", + "id": 2, + "ref": null, + "sha": "6b053ad388c531c21907f022933e5e81598db388" + }, + "created_at": "2016-01-04T15:41:23.046Z", + "finished_at": null, + "id": 64, + "name": "rubocop", + "ref": "master", + "runner": null, + "stage": "test", + "started_at": null, + "status": "pending" + } +] +``` + +## List commit builds + +Get a list of builds for specific commit in a project. + +``` +GET /projects/:id/builds/commit/:sha +``` + +Parameters: + +- `id` (required) - The ID of a project +- `sha` (required) - The SHA id of a commit +- `scope` (optional) - The scope of builds to show (one of: `all`, `finished`, `running`; default: `all`) + + +```json +[ + { + "commit": { + "committed_at": "2015-12-28T14:34:03.814Z", + "id": 2, + "ref": null, + "sha": "6b053ad388c531c21907f022933e5e81598db388" + }, + "created_at": "2016-01-04T15:41:23.147Z", + "finished_at": null, + "id": 65, + "name": "brakeman", + "ref": "master", + "runner": null, + "stage": "test", + "started_at": null, + "status": "pending" + }, + { + "commit": { + "committed_at": "2015-12-28T14:34:03.814Z", + "id": 2, + "ref": null, + "sha": "6b053ad388c531c21907f022933e5e81598db388" + }, + "created_at": "2016-01-04T15:41:23.046Z", + "finished_at": null, + "id": 64, + "name": "rubocop", + "ref": "master", + "runner": null, + "stage": "test", + "started_at": null, + "status": "pending" + } +] +``` + +## Get a single build + +Get a single build of a project + +``` +GET /projects/:id/builds/:build_id +``` + +Parameters: + +- `id` (required) - The ID of a project +- `build_id` (required) - The ID of a build + +```json +{ + "commit": { + "committed_at": "2015-12-28T14:34:03.814Z", + "id": 2, + "ref": null, + "sha": "6b053ad388c531c21907f022933e5e81598db388" + }, + "created_at": "2016-01-04T15:41:23.046Z", + "finished_at": null, + "id": 64, + "name": "rubocop", + "ref": "master", + "runner": null, + "stage": "test", + "started_at": null, + "status": "pending" +} +``` + +## Cancel a build + +Cancel a single build of a project + +``` +POST /projects/:id/builds/:build_id/cancel +``` + +Parameters: + +- `id` (required) - The ID of a project +- `build_id` (required) - The ID of a build + +```json +{ + "commit": { + "committed_at": "2015-12-28T14:34:03.814Z", + "id": 2, + "ref": null, + "sha": "6b053ad388c531c21907f022933e5e81598db388" + }, + "created_at": "2016-01-05T15:33:25.936Z", + "finished_at": "2016-01-05T15:33:47.553Z", + "id": 66, + "name": "rubocop", + "ref": "master", + "runner": null, + "stage": "test", + "started_at": null, + "status": "canceled" +} +``` + +## Retry a build + +Retry a single build of a project + +``` +POST /projects/:id/builds/:build_id/retry +``` + +Parameters: + +- `id` (required) - The ID of a project +- `build_id` (required) - The ID of a build + +```json +{ + "commit": { + "committed_at": "2015-12-28T14:34:03.814Z", + "id": 2, + "ref": null, + "sha": "6b053ad388c531c21907f022933e5e81598db388" + }, + "created_at": "2016-01-05T15:33:25.936Z", + "finished_at": null, + "id": 66, + "name": "rubocop", + "ref": "master", + "runner": null, + "stage": "test", + "started_at": null, + "status": "pending" +} +``` diff --git a/lib/api/builds.rb b/lib/api/builds.rb index 92bf849824..6b0edcff82 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -8,7 +8,7 @@ module API # # Parameters: # id (required) - The ID of a project - # scope (optional) - The scope of builds to show (one of: all, finished, running) + # scope (optional) - The scope of builds to show (one of: all, finished, running; default: all) # page (optional) - The page number for pagination # per_page (ooptional) - The value of items per page to show # Example Request: @@ -24,6 +24,7 @@ module API # Parameters: # id (required) - The ID of a project # sha (required) - The SHA id of a commit + # scope (optional) - The scope of builds to show (one of: all, finished, running; default: all) # Example Request: # GET /projects/:id/builds/commit/:sha get ':id/builds/commit/:sha' do From b0a77a224857ed45afaf642b26ce3ba87d9828a7 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 7 Jan 2016 11:04:25 +0100 Subject: [PATCH 025/218] Update ./doc/api --- doc/api/README.md | 1 + doc/api/triggers.md | 94 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+) create mode 100644 doc/api/triggers.md diff --git a/doc/api/README.md b/doc/api/README.md index 25a31b235c..1838bcc0bb 100644 --- a/doc/api/README.md +++ b/doc/api/README.md @@ -23,6 +23,7 @@ - [Namespaces](namespaces.md) - [Settings](settings.md) - [Keys](keys.md) +- [Triggers](triggers.md) ## Clients diff --git a/doc/api/triggers.md b/doc/api/triggers.md new file mode 100644 index 0000000000..6adcc8fe3b --- /dev/null +++ b/doc/api/triggers.md @@ -0,0 +1,94 @@ +# Triggers + +## List project triggers + +Get a list of project triggers + +``` +GET /projects/:id/triggers +``` + +Parameters: + +- `id` (required) - The ID of a project + +```json +[ + { + "created_at": "2015-12-23T16:24:34.716Z", + "deleted_at": null, + "id": 1, + "last_used": "2016-01-04T15:41:21.986Z", + "token": "fbdb730c2fbdb095a0862dbd8ab88b", + "updated_at": "2015-12-23T16:24:34.716Z" + }, + { + "created_at": "2015-12-23T16:25:56.760Z", + "deleted_at": null, + "id": 2, + "last_used": null, + "token": "7b9148c158980bbd9bcea92c17522d", + "updated_at": "2015-12-23T16:25:56.760Z" + } +] +``` + +## Get trigger details + +Get details of trigger of a project + +``` +GET /projects/:id/triggers/:trigger_id +``` + +Parameters: + +- `id` (required) - The ID of a project +- `trigger_id` (required) - The ID of a trigger + +```json +{ + "created_at": "2015-12-23T16:25:56.760Z", + "deleted_at": null, + "id": 2, + "last_used": null, + "token": "7b9148c158980bbd9bcea92c17522d", + "updated_at": "2015-12-23T16:25:56.760Z" +} +``` + +## Create a project trigger + +Create a trigger for a project + +``` +POST /projects/:id/triggers +``` + +Parameters: + +- `id` (required) - The ID of a project + +```json +{ + "created_at": "2016-01-07T09:53:58.235Z", + "deleted_at": null, + "id": 5, + "last_used": null, + "token": "6d056f63e50fe6f8c5f8f4aa10edb7", + "updated_at": "2016-01-07T09:53:58.235Z" +} +``` + +## Remove a project trigger + +Remove a trigger of a project + +``` +DELETE /projects/:id/triggers/:trigger_id +``` + +Parameters: + +- `id` (required) - The ID of a project +- `trigger_id` (required) - The ID of a trigger From b60c146267dfa8dc1c170426e1817c6b2a168d1a Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 7 Jan 2016 13:49:38 +0100 Subject: [PATCH 026/218] Change :variable_id to :key as resource ID in API --- app/models/ci/variable.rb | 6 ++++- lib/api/variables.rb | 41 +++++++++++----------------- spec/requests/api/variables_spec.rb | 42 +++++++++++------------------ 3 files changed, 36 insertions(+), 53 deletions(-) diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb index 0e2712086c..1d9ee91a31 100644 --- a/app/models/ci/variable.rb +++ b/app/models/ci/variable.rb @@ -18,8 +18,12 @@ module Ci belongs_to :project, class_name: '::Project', foreign_key: :gl_project_id - validates_presence_of :key validates_uniqueness_of :key, scope: :gl_project_id + validates :key, + presence: true, + length: { within: 0..255 }, + format: { with: /\A[a-zA-Z0-9_]+\z/, + message: "can contain only letters, digits and '_'." } attr_encrypted :value, mode: :per_attribute_iv_and_salt, key: Gitlab::Application.secrets.db_key_base end diff --git a/lib/api/variables.rb b/lib/api/variables.rb index b8bbcb6ce3..cc038e5731 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -22,19 +22,12 @@ module API # # Parameters: # id (required) - The ID of a project - # variable_id (required) - The ID OR `key` of variable to show; if variable_id contains only digits it's treated - # as ID other ways it's treated as `key` + # key (required) - The `key` of variable # Example Request: - # GET /projects/:id/variables/:variable_id - get ':id/variables/:variable_id' do - variable_id = params[:variable_id] - variables = user_project.variables - variables = - if variable_id.match(/^\d+$/) - variables.where(id: variable_id.to_i) - else - variables.where(key: variable_id) - end + # GET /projects/:id/variables/:key + get ':id/variables/:key' do + key = params[:key] + variables = user_project.variables.where(key: key) return not_found!('Variable') if variables.empty? @@ -45,8 +38,8 @@ module API # # Parameters: # id (required) - The ID of a project - # key (required) - The key of variable being created - # value (required) - The value of variable being created + # key (required) - The key of variable + # value (required) - The value of variable # Example Request: # POST /projects/:id/variables post ':id/variables' do @@ -63,17 +56,15 @@ module API # # Parameters: # id (required) - The ID of a project - # variable_id (required) - The ID of a variable - # key (optional) - new value for `key` field of variable - # value (optional) - new value for `value` field of variable + # key (optional) - The `key` of variable + # value (optional) - New value for `value` field of variable # Example Request: - # PUT /projects/:id/variables/:variable_id - put ':id/variables/:variable_id' do - variable = user_project.variables.where(id: params[:variable_id].to_i).first + # PUT /projects/:id/variables/:key + put ':id/variables/:key' do + variable = user_project.variables.where(key: params[:key]).first return not_found!('Variable') unless variable - variable.key = params[:key] variable.value = params[:value] variable.save! @@ -84,11 +75,11 @@ module API # # Parameters: # id (required) - The ID of a project - # variable_id (required) - The ID of a variable + # key (required) - The ID of a variable # Exanoke Reqyest: - # DELETE /projects/:id/variables/:variable_id - delete ':id/variables/:variable_id' do - variable = user_project.variables.where(id: params[:variable_id].to_i).first + # DELETE /projects/:id/variables/:key + delete ':id/variables/:key' do + variable = user_project.variables.where(key: params[:key]).first return not_found!('Variable') unless variable diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb index bf0dd77473..214d7d5a0c 100644 --- a/spec/requests/api/variables_spec.rb +++ b/spec/requests/api/variables_spec.rb @@ -37,26 +37,17 @@ describe API::API, api: true do end end - describe 'GET /projects/:id/variables/:variable_id' do + describe 'GET /projects/:id/variables/:key' do context 'authorized user with proper permissions' do - it 'should return project variable details when ID is used as :variable_id' do - get api("/projects/#{project.id}/variables/#{variable.id}", user) - - expect(response.status).to eq(200) - expect(json_response['key']).to eq(variable.key) - expect(json_response['value']).to eq(variable.value) - end - - it 'should return project variable details when `key` is used as :variable_id' do + it 'should return project variable details' do get api("/projects/#{project.id}/variables/#{variable.key}", user) expect(response.status).to eq(200) - expect(json_response['id']).to eq(variable.id) expect(json_response['value']).to eq(variable.value) end it 'should responde with 404 Not Found if requesting non-existing variable' do - get api("/projects/#{project.id}/variables/9999", user) + get api("/projects/#{project.id}/variables/non_existing_variable", user) expect(response.status).to eq(404) end @@ -64,7 +55,7 @@ describe API::API, api: true do context 'authorized user with invalid permissions' do it 'should not return project variable details' do - get api("/projects/#{project.id}/variables/#{variable.id}", user2) + get api("/projects/#{project.id}/variables/#{variable.key}", user2) expect(response.status).to eq(403) end @@ -72,7 +63,7 @@ describe API::API, api: true do context 'unauthorized user' do it 'should not return project variable details' do - get api("/projects/#{project.id}/variables/#{variable.id}") + get api("/projects/#{project.id}/variables/#{variable.key}") expect(response.status).to eq(401) end @@ -117,26 +108,23 @@ describe API::API, api: true do end end - describe 'PUT /projects/:id/variables/:variable_id' do + describe 'PUT /projects/:id/variables/:key' do context 'authorized user with proper permissions' do it 'should update variable data' do initial_variable = project.variables.first - key_before = initial_variable.key value_before = initial_variable.value - put api("/projects/#{project.id}/variables/#{variable.id}", user), key: 'TEST_VARIABLE_1_UP', value: 'VALUE_1_UP' + put api("/projects/#{project.id}/variables/#{variable.key}", user), value: 'VALUE_1_UP' updated_variable = project.variables.first expect(response.status).to eq(200) - expect(key_before).to eq(variable.key) expect(value_before).to eq(variable.value) - expect(updated_variable.key).to eq('TEST_VARIABLE_1_UP') expect(updated_variable.value).to eq('VALUE_1_UP') end it 'should responde with 404 Not Found if requesting non-existing variable' do - put api("/projects/#{project.id}/variables/9999", user) + put api("/projects/#{project.id}/variables/non_existing_variable", user) expect(response.status).to eq(404) end @@ -144,7 +132,7 @@ describe API::API, api: true do context 'authorized user with invalid permissions' do it 'should not update variable' do - put api("/projects/#{project.id}/variables/#{variable.id}", user2) + put api("/projects/#{project.id}/variables/#{variable.key}", user2) expect(response.status).to eq(403) end @@ -152,24 +140,24 @@ describe API::API, api: true do context 'unauthorized user' do it 'should not update variable' do - put api("/projects/#{project.id}/variables/#{variable.id}") + put api("/projects/#{project.id}/variables/#{variable.key}") expect(response.status).to eq(401) end end end - describe 'DELETE /projects/:id/variables/:variable_id' do + describe 'DELETE /projects/:id/variables/:key' do context 'authorized user with proper permissions' do it 'should delete variable' do expect do - delete api("/projects/#{project.id}/variables/#{variable.id}", user) + delete api("/projects/#{project.id}/variables/#{variable.key}", user) end.to change{project.variables.count}.by(-1) expect(response.status).to eq(200) end it 'should responde with 404 Not Found if requesting non-existing variable' do - delete api("/projects/#{project.id}/variables/9999", user) + delete api("/projects/#{project.id}/variables/non_existing_variable", user) expect(response.status).to eq(404) end @@ -177,7 +165,7 @@ describe API::API, api: true do context 'authorized user with invalid permissions' do it 'should not delete variable' do - delete api("/projects/#{project.id}/variables/#{variable.id}", user2) + delete api("/projects/#{project.id}/variables/#{variable.key}", user2) expect(response.status).to eq(403) end @@ -185,7 +173,7 @@ describe API::API, api: true do context 'unauthorized user' do it 'should not delete variable' do - delete api("/projects/#{project.id}/variables/#{variable.id}") + delete api("/projects/#{project.id}/variables/#{variable.key}") expect(response.status).to eq(401) end From b60445906849e84ff52ac6a5d7d501bb5a21eb60 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 7 Jan 2016 14:10:49 +0100 Subject: [PATCH 027/218] Update ./doc/api --- doc/api/README.md | 1 + doc/api/variables.md | 106 +++++++++++++++++++++++++++++++++++++++++++ lib/api/entities.rb | 2 +- 3 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 doc/api/variables.md diff --git a/doc/api/README.md b/doc/api/README.md index 25a31b235c..198b4ddfee 100644 --- a/doc/api/README.md +++ b/doc/api/README.md @@ -23,6 +23,7 @@ - [Namespaces](namespaces.md) - [Settings](settings.md) - [Keys](keys.md) +- [Variables](variables.md) ## Clients diff --git a/doc/api/variables.md b/doc/api/variables.md new file mode 100644 index 0000000000..cdc9ba4225 --- /dev/null +++ b/doc/api/variables.md @@ -0,0 +1,106 @@ +# Variables + +## Variables keys + +All variable keys must contains only letters, digits and '\_'. They must also be no longer than 255 characters. + +## List project variables + +Get list of variables of a project. + +``` +GET /projects/:id/variables +``` + +Parameters: + +- `id` (required) - The ID of a project + +```json +[ + { + "key": "TEST_VARIABLE_1", + "value": "TEST_1" + }, + { + "key": "TEST_VARIABLE_2", + "value": "TEST_2" + } +] +``` + +## Show variable details + +Get details of specifica variable of a project. + +``` +GET /projects/:id/variables/:key +``` + +Parameters: + +- `id` (required) - The ID of a project +- `key` (required) - The `key` of variable + +```json +{ + "key": "TEST_VARIABLE_1", + "value": "TEST_1" +} +``` + +## Create variable + +Create new variable in project. + +``` +POST /projects/:id/variables +``` + +Parameters: + +- `id` (required) - The ID of a project +- `key` (required) - The `key` for variable +- `value` (required) - The `value` for variable + +```json +{ + "key": "NEW_VARIABLE", + "value": "new value" +} +``` + +## Update variable + +Update variable. + +``` +PUT /projects/:id/variables/:key +``` + +Parameters: + +- `id` (required) - The ID of a project +- `key` (required) - The `key` for variable +- `value` (required) - The `value` for variable + +```json +{ + "key": "NEW_VARIABLE", + "value": "updated value" +} +``` + +## Remove variable + +Remove variable. + +``` +DELETE /projects/:id/variables/:key +``` + +Parameters: + +- `id` (required) - The ID of a project +- `key` (required) - The `key` for variable + diff --git a/lib/api/entities.rb b/lib/api/entities.rb index f71d072f26..db3164d9d9 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -367,7 +367,7 @@ module API end class Variable < Grape::Entity - expose :id, :key, :value + expose :key, :value end end end From e0ec69d919cb44194e76034f2324ec0d4f5f1df6 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 7 Jan 2016 18:48:33 +0100 Subject: [PATCH 028/218] Change 'trigger_id' to 'token' as resource ID in triggers API --- doc/api/triggers.md | 12 +++------ lib/api/entities.rb | 2 +- lib/api/triggers.rb | 25 ++++++------------ spec/requests/api/triggers_spec.rb | 42 ++++++++++-------------------- 4 files changed, 27 insertions(+), 54 deletions(-) diff --git a/doc/api/triggers.md b/doc/api/triggers.md index 6adcc8fe3b..4150d7b4c1 100644 --- a/doc/api/triggers.md +++ b/doc/api/triggers.md @@ -17,7 +17,6 @@ Parameters: { "created_at": "2015-12-23T16:24:34.716Z", "deleted_at": null, - "id": 1, "last_used": "2016-01-04T15:41:21.986Z", "token": "fbdb730c2fbdb095a0862dbd8ab88b", "updated_at": "2015-12-23T16:24:34.716Z" @@ -25,7 +24,6 @@ Parameters: { "created_at": "2015-12-23T16:25:56.760Z", "deleted_at": null, - "id": 2, "last_used": null, "token": "7b9148c158980bbd9bcea92c17522d", "updated_at": "2015-12-23T16:25:56.760Z" @@ -38,19 +36,18 @@ Parameters: Get details of trigger of a project ``` -GET /projects/:id/triggers/:trigger_id +GET /projects/:id/triggers/:token ``` Parameters: - `id` (required) - The ID of a project -- `trigger_id` (required) - The ID of a trigger +- `token` (required) - The `token` of a trigger ```json { "created_at": "2015-12-23T16:25:56.760Z", "deleted_at": null, - "id": 2, "last_used": null, "token": "7b9148c158980bbd9bcea92c17522d", "updated_at": "2015-12-23T16:25:56.760Z" @@ -73,7 +70,6 @@ Parameters: { "created_at": "2016-01-07T09:53:58.235Z", "deleted_at": null, - "id": 5, "last_used": null, "token": "6d056f63e50fe6f8c5f8f4aa10edb7", "updated_at": "2016-01-07T09:53:58.235Z" @@ -85,10 +81,10 @@ Parameters: Remove a trigger of a project ``` -DELETE /projects/:id/triggers/:trigger_id +DELETE /projects/:id/triggers/:token ``` Parameters: - `id` (required) - The ID of a project -- `trigger_id` (required) - The ID of a trigger +- `token` (required) - The `token` of a trigger diff --git a/lib/api/entities.rb b/lib/api/entities.rb index bc0cd76a2b..37c483b45e 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -367,7 +367,7 @@ module API end class Trigger < Grape::Entity - expose :id, :token, :created_at, :updated_at, :deleted_at + expose :token, :created_at, :updated_at, :deleted_at expose :last_used do |repo_obj, _options| if repo_obj.respond_to?(:last_trigger_request) request = repo_obj.last_trigger_request diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 0e548b936c..25bb8aef20 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -66,23 +66,14 @@ module API # # Parameters: # id (required) - The ID of a project - # trigger_id (required) - The ID or `token` of a trigger to show; if trigger_id contains only digits it's - # treated as ID other ways it's reated as `key` + # token (required) - The `token` of a trigger # Example Request: - # GET /projects/:id/triggers/:trigger_id - get ':id/triggers/:trigger_id' do + # GET /projects/:id/triggers/:token + get ':id/triggers/:token' do authenticate! authorize_admin_project - trigger_id = params[:trigger_id] - triggers = user_project.triggers - triggers = - if trigger_id.match(/^\d+$/) - triggers.where(id: trigger_id.to_i) - else - triggers.where(token: trigger_id) - end - + triggers = user_project.triggers.where(token: params[:token]) return not_found!('Trigger') if triggers.empty? present triggers.first, with: Entities::Trigger @@ -108,14 +99,14 @@ module API # # Parameters: # id (required) - The ID of a project - # trigger_id - The ID of trigger to delete + # token (required) - The `token` of a trigger # Example Request: - # DELETE /projects/:id/triggers/:trigger_id - delete ':id/triggers/:trigger_id' do + # DELETE /projects/:id/triggers/:token + delete ':id/triggers/:token' do authenticate! authorize_admin_project - trigger = user_project.triggers.where(id: params[:trigger_id].to_i).first + trigger = user_project.triggers.where(token: params[:token]).first return not_found!('Trigger') unless trigger trigger.destroy diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index c1c2bb04b2..e8d89426ec 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -93,8 +93,7 @@ describe API::API do expect(response.status).to eq(200) expect(json_response).to be_a(Array) - expect(json_response[0]['token']).to eq(trigger_token) - expect(json_response[1]['token']).to eq(trigger_token_2) + expect(json_response[0]).to have_key('token') end end @@ -115,30 +114,17 @@ describe API::API do end end - describe 'GET /projects/:id/triggers/:triggers_id' do + describe 'GET /projects/:id/triggers/:token' do context 'authenticated user with valid permissions' do - context 'ID is used as :trigger_id' do - it 'should return trigger details' do - get api("/projects/#{project.id}/triggers/#{trigger.id}", user) + it 'should return trigger details' do + get api("/projects/#{project.id}/triggers/#{trigger.token}", user) - expect(response.status).to eq(200) - expect(json_response).to be_a(Hash) - expect(json_response['token']).to eq(trigger_token) - end - end - - context '`token` is used as :trigger_id' do - it 'should return trigger details' do - get api("/projects/#{project.id}/triggers/#{trigger.token}", user) - - expect(response.status).to eq(200) - expect(json_response).to be_a(Hash) - expect(json_response['id']).to eq(trigger.id) - end + expect(response.status).to eq(200) + expect(json_response).to be_a(Hash) end it 'should responde with 404 Not Found if requesting non-existing trigger' do - get api("/projects/#{project.id}/triggers/9999", user) + get api("/projects/#{project.id}/triggers/abcdef012345", user) expect(response.status).to eq(404) end @@ -146,7 +132,7 @@ describe API::API do context 'authenticated user with invalid permissions' do it 'should not return triggers list' do - get api("/projects/#{project.id}/triggers/#{trigger.id}", user2) + get api("/projects/#{project.id}/triggers/#{trigger.token}", user2) expect(response.status).to eq(403) end @@ -154,7 +140,7 @@ describe API::API do context 'unauthentikated user' do it 'should not return triggers list' do - get api("/projects/#{project.id}/triggers/#{trigger.id}") + get api("/projects/#{project.id}/triggers/#{trigger.token}") expect(response.status).to eq(401) end @@ -190,17 +176,17 @@ describe API::API do end end - describe 'DELETE /projects/:id/triggers/:trigger_id' do + describe 'DELETE /projects/:id/triggers/:token' do context 'authenticated user with valid permissions' do it 'should delete trigger' do expect do - delete api("/projects/#{project.id}/triggers/#{trigger.id}", user) + delete api("/projects/#{project.id}/triggers/#{trigger.token}", user) end.to change{project.triggers.count}.by(-1) expect(response.status).to eq(200) end it 'should responde with 404 Not Found if requesting non-existing trigger' do - delete api("/projects/#{project.id}/triggers/9999", user) + delete api("/projects/#{project.id}/triggers/abcdef012345", user) expect(response.status).to eq(404) end @@ -208,7 +194,7 @@ describe API::API do context 'authenticated user with invalid permissions' do it 'should not delete trigger' do - delete api("/projects/#{project.id}/triggers/#{trigger.id}", user2) + delete api("/projects/#{project.id}/triggers/#{trigger.token}", user2) expect(response.status).to eq(403) end @@ -216,7 +202,7 @@ describe API::API do context 'unauthentikated user' do it 'should not delete trigger' do - delete api("/projects/#{project.id}/triggers/#{trigger.id}") + delete api("/projects/#{project.id}/triggers/#{trigger.token}") expect(response.status).to eq(401) end From 549a2fa7873366b52e9ba3caa849073b7b958b73 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Fri, 8 Jan 2016 14:01:31 +0100 Subject: [PATCH 029/218] Modify builds scope filtering in builds API --- app/models/ci/build.rb | 4 ++++ doc/api/builds.md | 5 ++--- lib/api/builds.rb | 31 +++++++++++++++++++------------ spec/requests/api/builds_spec.rb | 15 ++++++++++++++- 4 files changed, 39 insertions(+), 16 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 3e67b2771c..8fb68743a9 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -94,6 +94,10 @@ module Ci new_build.save new_build end + + def available_statuses + state_machines[:status].states.map &:value + end end state_machine :status, initial: :pending do diff --git a/doc/api/builds.md b/doc/api/builds.md index b716499dd3..c52266714d 100644 --- a/doc/api/builds.md +++ b/doc/api/builds.md @@ -11,7 +11,7 @@ GET /projects/:id/builds Parameters: - `id` (required) - The ID of a project -- `scope` (optional) - The scope of builds to show (one of: `all`, `finished`, `running`; default: `all`) +- `scope` (optional) - The scope of builds to show (one or array of: pending, running, failed, success, canceled; if none provided showing all builds) ```json [ @@ -64,8 +64,7 @@ Parameters: - `id` (required) - The ID of a project - `sha` (required) - The SHA id of a commit -- `scope` (optional) - The scope of builds to show (one of: `all`, `finished`, `running`; default: `all`) - +- `scope` (optional) - The scope of builds to show (one or array of: pending, running, failed, success, canceled; if none provided showing all builds) ```json [ diff --git a/lib/api/builds.rb b/lib/api/builds.rb index 6b0edcff82..6aae185695 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -8,9 +8,8 @@ module API # # Parameters: # id (required) - The ID of a project - # scope (optional) - The scope of builds to show (one of: all, finished, running; default: all) - # page (optional) - The page number for pagination - # per_page (ooptional) - The value of items per page to show + # scope (optional) - The scope of builds to show (one or array of: pending, running, failed, success, canceled; + # if none provided showing all builds) # Example Request: # GET /projects/:id/builds get ':id/builds' do @@ -24,7 +23,8 @@ module API # Parameters: # id (required) - The ID of a project # sha (required) - The SHA id of a commit - # scope (optional) - The scope of builds to show (one of: all, finished, running; default: all) + # scope (optional) - The scope of builds to show (one or array of: pending, running, failed, success, canceled; + # if none provided showing all builds) # Example Request: # GET /projects/:id/builds/commit/:sha get ':id/builds/commit/:sha' do @@ -112,14 +112,21 @@ module API end def filter_builds(builds, scope) - case scope - when 'finished' - builds.finished - when 'running' - builds.running - else - builds - end + available_scopes = Ci::Build.available_statuses + scope = + if scope.is_a?(String) || scope.is_a?(Symbol) + available_scopes & [scope.to_s] + elsif scope.is_a?(Array) + available_scopes & scope + elsif scope.respond_to?(:to_h) + available_scopes & scope.to_h.values + else + [] + end + + return builds if scope.empty? + + builds.where(status: scope) end def authorize_manage_builds! diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb index d4af7639d4..a953eb2fac 100644 --- a/spec/requests/api/builds_spec.rb +++ b/spec/requests/api/builds_spec.rb @@ -18,7 +18,20 @@ describe API::API, api: true do it 'should return project builds' do get api("/projects/#{project.id}/builds", user) - puts json_response + expect(response.status).to eq(200) + expect(json_response).to be_an Array + end + + it 'should filter project with one scope element' do + get api("/projects/#{project.id}/builds?scope=pending", user) + + expect(response.status).to eq(200) + expect(json_response).to be_an Array + end + + it 'should filter project with array of scope elements' do + get api("/projects/#{project.id}/builds?scope[0]=pending&scope[1]=running", user) + expect(response.status).to eq(200) expect(json_response).to be_an Array end From bc7ef8e5b7a002ca6bc2d7a5e6be11b4a59b6710 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Tue, 29 Dec 2015 17:53:55 -0200 Subject: [PATCH 030/218] Add ldap_blocked as new state to users state machine --- app/models/user.rb | 12 ++++++++++- spec/models/user_spec.rb | 44 +++++++++++++++++++++++++--------------- 2 files changed, 39 insertions(+), 17 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 46b36c605b..67b47b0f32 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -198,16 +198,26 @@ class User < ActiveRecord::Base transition active: :blocked end + event :ldap_block do + transition active: :ldap_blocked + end + event :activate do transition blocked: :active end + + state :blocked, :ldap_blocked do + def blocked? + true + end + end end mount_uploader :avatar, AvatarUploader # Scopes scope :admins, -> { where(admin: true) } - scope :blocked, -> { with_state(:blocked) } + scope :blocked, -> { with_states(:blocked, :ldap_blocked) } scope :active, -> { with_state(:active) } scope :not_in_project, ->(project) { project.users.present? ? where("id not in (:ids)", ids: project.users.map(&:id) ) : all } scope :without_projects, -> { where('id NOT IN (SELECT DISTINCT(user_id) FROM members)') } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3cd63b2b0e..0bef68e288 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -569,27 +569,39 @@ describe User, models: true do end end - describe :ldap_user? do - it "is true if provider name starts with ldap" do - user = create(:omniauth_user, provider: 'ldapmain') - expect( user.ldap_user? ).to be_truthy + context 'ldap synchronized user' do + describe :ldap_user? do + it 'is true if provider name starts with ldap' do + user = create(:omniauth_user, provider: 'ldapmain') + expect(user.ldap_user?).to be_truthy + end + + it 'is false for other providers' do + user = create(:omniauth_user, provider: 'other-provider') + expect(user.ldap_user?).to be_falsey + end + + it 'is false if no extern_uid is provided' do + user = create(:omniauth_user, extern_uid: nil) + expect(user.ldap_user?).to be_falsey + end end - it "is false for other providers" do - user = create(:omniauth_user, provider: 'other-provider') - expect( user.ldap_user? ).to be_falsey + describe :ldap_identity do + it 'returns ldap identity' do + user = create :omniauth_user + expect(user.ldap_identity.provider).not_to be_empty + end end - it "is false if no extern_uid is provided" do - user = create(:omniauth_user, extern_uid: nil) - expect( user.ldap_user? ).to be_falsey - end - end + describe '#ldap_block' do + let(:user) { create(:omniauth_user, provider: 'ldapmain', name: 'John Smith') } - describe :ldap_identity do - it "returns ldap identity" do - user = create :omniauth_user - expect(user.ldap_identity.provider).not_to be_empty + it 'blocks user flaging the action caming from ldap' do + user.ldap_block + expect(user.blocked?).to be_truthy + expect(user.ldap_blocked?).to be_truthy + end end end From ba9855d4877998e3574907cc542fcab15a9d1353 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Tue, 29 Dec 2015 18:58:38 -0200 Subject: [PATCH 031/218] Prevent ldap_blocked users from being unblocked by the Admin UI --- app/assets/stylesheets/framework/buttons.scss | 3 ++ app/controllers/admin/users_controller.rb | 4 ++- app/views/admin/users/index.html.haml | 7 +++- .../admin/users_controller_spec.rb | 33 ++++++++++++++----- 4 files changed, 37 insertions(+), 10 deletions(-) diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index 97a9463884..e237636348 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -132,6 +132,9 @@ margin-right: 0px; } } + &.disabled { + pointer-events: auto !important; + } } .btn-block { diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index d7c927d444..87f4fb455b 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -40,7 +40,9 @@ class Admin::UsersController < Admin::ApplicationController end def unblock - if user.activate + if user.ldap_blocked? + redirect_back_or_admin_user(alert: "This user cannot be unlocked manually from GitLab") + elsif user.activate redirect_back_or_admin_user(notice: "Successfully unblocked") else redirect_back_or_admin_user(alert: "Error occurred. User was not unblocked") diff --git a/app/views/admin/users/index.html.haml b/app/views/admin/users/index.html.haml index a92c9c152b..911c4d0cf1 100644 --- a/app/views/admin/users/index.html.haml +++ b/app/views/admin/users/index.html.haml @@ -90,7 +90,12 @@   = link_to 'Edit', edit_admin_user_path(user), id: "edit_#{dom_id(user)}", class: "btn btn-xs" - unless user == current_user - - if user.blocked? + - if user.ldap_blocked? + = link_to '#', title: 'Cannot unblock LDAP blocked users', data: {toggle: 'tooltip'}, class: 'btn btn-xs btn-success disabled' do + %i.fa.fa-lock + Unblock + = '' + - elsif user.blocked? = link_to 'Unblock', unblock_admin_user_path(user), method: :put, class: "btn btn-xs btn-success" - else = link_to 'Block', block_admin_user_path(user), data: {confirm: 'USER WILL BE BLOCKED! Are you sure?'}, method: :put, class: "btn btn-xs btn-warning" diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 8b7af4d3a0..5b1f65d7af 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -34,17 +34,34 @@ describe Admin::UsersController do end describe 'PUT unblock/:id' do - let(:user) { create(:user) } + context 'ldap blocked users' do + let(:user) { create(:omniauth_user, provider: 'ldapmain') } - before do - user.block + before do + user.ldap_block + end + + it 'will not unblock user' do + put :unblock, id: user.username + user.reload + expect(user.blocked?).to be_truthy + expect(flash[:alert]).to eq 'This user cannot be unlocked manually from GitLab' + end end - it 'unblocks user' do - put :unblock, id: user.username - user.reload - expect(user.blocked?).to be_falsey - expect(flash[:notice]).to eq 'Successfully unblocked' + context 'manually blocked users' do + let(:user) { create(:user) } + + before do + user.block + end + + it 'unblocks user' do + put :unblock, id: user.username + user.reload + expect(user.blocked?).to be_falsey + expect(flash[:notice]).to eq 'Successfully unblocked' + end end end From 6e7db8e23e169bcbf0847ece27b9e44e00ae572b Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 30 Dec 2015 16:52:02 -0200 Subject: [PATCH 032/218] Prevent ldap_blocked users from being blocked/unblocked by the API --- doc/api/users.md | 6 ++++-- lib/api/users.rb | 12 ++++++++---- spec/requests/api/users_spec.rb | 23 ++++++++++++++++++----- 3 files changed, 30 insertions(+), 11 deletions(-) diff --git a/doc/api/users.md b/doc/api/users.md index 773fe36d27..b7fc903825 100644 --- a/doc/api/users.md +++ b/doc/api/users.md @@ -558,7 +558,8 @@ Parameters: - `uid` (required) - id of specified user -Will return `200 OK` on success, or `404 User Not Found` is user cannot be found. +Will return `200 OK` on success, `404 User Not Found` is user cannot be found or +`403 Forbidden` when trying to block an already blocked user by LDAP synchronization. ## Unblock user @@ -572,4 +573,5 @@ Parameters: - `uid` (required) - id of specified user -Will return `200 OK` on success, or `404 User Not Found` is user cannot be found. +Will return `200 OK` on success, `404 User Not Found` is user cannot be found or +`403 Forbidden` when trying to unblock a user blocked by LDAP synchronization. diff --git a/lib/api/users.rb b/lib/api/users.rb index 0d7813428e..01fd90139b 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -284,10 +284,12 @@ module API authenticated_as_admin! user = User.find_by(id: params[:id]) - if user + if !user + not_found!('User') + elsif !user.ldap_blocked? user.block else - not_found!('User') + forbidden!('LDAP blocked users cannot be modified by the API') end end @@ -299,10 +301,12 @@ module API authenticated_as_admin! user = User.find_by(id: params[:id]) - if user + if !user + not_found!('User') + elsif !user.ldap_blocked? user.activate else - not_found!('User') + forbidden!('LDAP blocked users cannot be unblocked by the API') end end end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 4f278551d0..b82c5c7685 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -8,6 +8,8 @@ describe API::API, api: true do let(:key) { create(:key, user: user) } let(:email) { create(:email, user: user) } let(:omniauth_user) { create(:omniauth_user) } + let(:ldap_user) { create(:omniauth_user, provider: 'ldapmain') } + let(:ldap_blocked_user) { create(:omniauth_user, provider: 'ldapmain', state: 'ldap_blocked') } describe "GET /users" do context "when unauthenticated" do @@ -783,6 +785,12 @@ describe API::API, api: true do expect(user.reload.state).to eq('blocked') end + it 'should not re-block ldap blocked users' do + put api("/users/#{ldap_blocked_user.id}/block", admin) + expect(response.status).to eq(403) + expect(ldap_blocked_user.reload.state).to eq('ldap_blocked') + end + it 'should not be available for non admin users' do put api("/users/#{user.id}/block", user) expect(response.status).to eq(403) @@ -797,7 +805,9 @@ describe API::API, api: true do end describe 'PUT /user/:id/unblock' do + let(:blocked_user) { create(:user, state: 'blocked') } before { admin } + it 'should unblock existing user' do put api("/users/#{user.id}/unblock", admin) expect(response.status).to eq(200) @@ -805,12 +815,15 @@ describe API::API, api: true do end it 'should unblock a blocked user' do - put api("/users/#{user.id}/block", admin) + put api("/users/#{blocked_user.id}/unblock", admin) expect(response.status).to eq(200) - expect(user.reload.state).to eq('blocked') - put api("/users/#{user.id}/unblock", admin) - expect(response.status).to eq(200) - expect(user.reload.state).to eq('active') + expect(blocked_user.reload.state).to eq('active') + end + + it 'should not unblock ldap blocked users' do + put api("/users/#{ldap_blocked_user.id}/unblock", admin) + expect(response.status).to eq(403) + expect(ldap_blocked_user.reload.state).to eq('ldap_blocked') end it 'should not be available for non admin users' do From d6dc088affeee4568e771e1d7894e0bcdb955af8 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 30 Dec 2015 20:56:26 -0200 Subject: [PATCH 033/218] LDAP synchronization block/unblock new states --- lib/gitlab/ldap/access.rb | 6 ++--- spec/lib/gitlab/ldap/access_spec.rb | 34 ++++++++++++----------------- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index c438a3d167..76cb48d7aa 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -37,15 +37,15 @@ module Gitlab # Block user in GitLab if he/she was blocked in AD if Gitlab::LDAP::Person.disabled_via_active_directory?(user.ldap_identity.extern_uid, adapter) - user.block + user.ldap_block false else - user.activate if user.blocked? && !ldap_config.block_auto_created_users + user.activate if (user.blocked? && !ldap_config.block_auto_created_users) || user.ldap_blocked? true end else # Block the user if they no longer exist in LDAP/AD - user.block + user.ldap_block false end rescue diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index a628d0c015..f58d70e809 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -13,64 +13,59 @@ describe Gitlab::LDAP::Access, lib: true do end it { is_expected.to be_falsey } - + it 'should block user in GitLab' do access.allowed? expect(user).to be_blocked + expect(user).to be_ldap_blocked end end context 'when the user is found' do before do - allow(Gitlab::LDAP::Person). - to receive(:find_by_dn).and_return(:ldap_user) + allow(Gitlab::LDAP::Person).to receive(:find_by_dn).and_return(:ldap_user) end context 'and the user is disabled via active directory' do before do - allow(Gitlab::LDAP::Person). - to receive(:disabled_via_active_directory?).and_return(true) + allow(Gitlab::LDAP::Person).to receive(:disabled_via_active_directory?).and_return(true) end it { is_expected.to be_falsey } - it "should block user in GitLab" do + it 'should block user in GitLab' do access.allowed? expect(user).to be_blocked + expect(user).to be_ldap_blocked end end context 'and has no disabled flag in active diretory' do before do user.block - - allow(Gitlab::LDAP::Person). - to receive(:disabled_via_active_directory?).and_return(false) + allow(Gitlab::LDAP::Person).to receive(:disabled_via_active_directory?).and_return(false) end it { is_expected.to be_truthy } context 'when auto-created users are blocked' do - before do - allow_any_instance_of(Gitlab::LDAP::Config). - to receive(:block_auto_created_users).and_return(true) + allow_any_instance_of(Gitlab::LDAP::Config).to receive(:block_auto_created_users).and_return(true) end - it "does not unblock user in GitLab" do + it 'does not unblock user in GitLab' do access.allowed? expect(user).to be_blocked + expect(user).not_to be_ldap_blocked # this block is handled by omniauth not by our internal logic end end - context "when auto-created users are not blocked" do - + context 'when auto-created users are not blocked' do before do - allow_any_instance_of(Gitlab::LDAP::Config). - to receive(:block_auto_created_users).and_return(false) + allow_any_instance_of(Gitlab::LDAP::Config).to receive(:block_auto_created_users).and_return(false) end - it "should unblock user in GitLab" do + it 'should unblock user in GitLab' do access.allowed? expect(user).not_to be_blocked end @@ -80,8 +75,7 @@ describe Gitlab::LDAP::Access, lib: true do context 'without ActiveDirectory enabled' do before do allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true) - allow_any_instance_of(Gitlab::LDAP::Config). - to receive(:active_directory).and_return(false) + allow_any_instance_of(Gitlab::LDAP::Config).to receive(:active_directory).and_return(false) end it { is_expected.to be_truthy } From ec67e9be1d7486199b47e19c766202a8bfdefe93 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 6 Jan 2016 05:38:52 -0200 Subject: [PATCH 034/218] Repair ldap_blocked state when no ldap identity exist anymore --- .../admin/identities_controller.rb | 2 + app/models/identity.rb | 4 ++ app/models/user.rb | 1 + .../repair_ldap_blocked_user_service.rb | 15 ++++++++ .../admin/identities_controller_spec.rb | 26 +++++++++++++ spec/models/identity_spec.rb | 38 +++++++++++++++++++ .../repair_ldap_blocked_user_service_spec.rb | 23 +++++++++++ 7 files changed, 109 insertions(+) create mode 100644 app/services/repair_ldap_blocked_user_service.rb create mode 100644 spec/controllers/admin/identities_controller_spec.rb create mode 100644 spec/models/identity_spec.rb create mode 100644 spec/services/repair_ldap_blocked_user_service_spec.rb diff --git a/app/controllers/admin/identities_controller.rb b/app/controllers/admin/identities_controller.rb index e383fe38ea..9ba1048751 100644 --- a/app/controllers/admin/identities_controller.rb +++ b/app/controllers/admin/identities_controller.rb @@ -26,6 +26,7 @@ class Admin::IdentitiesController < Admin::ApplicationController def update if @identity.update_attributes(identity_params) + RepairLdapBlockedUserService.new(@user, @identity).execute redirect_to admin_user_identities_path(@user), notice: 'User identity was successfully updated.' else render :edit @@ -34,6 +35,7 @@ class Admin::IdentitiesController < Admin::ApplicationController def destroy if @identity.destroy + RepairLdapBlockedUserService.new(@user, @identity).execute redirect_to admin_user_identities_path(@user), notice: 'User identity was successfully removed.' else redirect_to admin_user_identities_path(@user), alert: 'Failed to remove user identity.' diff --git a/app/models/identity.rb b/app/models/identity.rb index 8bcdc19495..830b99fa3f 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -18,4 +18,8 @@ class Identity < ActiveRecord::Base validates :provider, presence: true validates :extern_uid, allow_blank: true, uniqueness: { scope: :provider } validates :user_id, uniqueness: { scope: :provider } + + def is_ldap? + provider.starts_with?('ldap') + end end diff --git a/app/models/user.rb b/app/models/user.rb index 67b47b0f32..5eed9cf91c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -196,6 +196,7 @@ class User < ActiveRecord::Base state_machine :state, initial: :active do event :block do transition active: :blocked + transition ldap_blocked: :blocked end event :ldap_block do diff --git a/app/services/repair_ldap_blocked_user_service.rb b/app/services/repair_ldap_blocked_user_service.rb new file mode 100644 index 0000000000..ceca15414e --- /dev/null +++ b/app/services/repair_ldap_blocked_user_service.rb @@ -0,0 +1,15 @@ +class RepairLdapBlockedUserService + attr_accessor :user, :identity + + def initialize(user, identity) + @user, @identity = user, identity + end + + def execute + if identity.destroyed? + user.block if identity.is_ldap? && user.ldap_blocked? && !user.ldap_user? + else + user.block if !identity.is_ldap? && user.ldap_blocked? && !user.ldap_user? + end + end +end diff --git a/spec/controllers/admin/identities_controller_spec.rb b/spec/controllers/admin/identities_controller_spec.rb new file mode 100644 index 0000000000..c131d22a30 --- /dev/null +++ b/spec/controllers/admin/identities_controller_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +describe Admin::IdentitiesController do + let(:admin) { create(:admin) } + before { sign_in(admin) } + + describe 'UPDATE identity' do + let(:user) { create(:omniauth_user, provider: 'ldapmain', extern_uid: 'uid=myuser,ou=people,dc=example,dc=com') } + + it 'repairs ldap blocks' do + expect_any_instance_of(RepairLdapBlockedUserService).to receive(:execute) + + put :update, user_id: user.username, id: user.ldap_identity.id, identity: { provider: 'twitter' } + end + end + + describe 'DELETE identity' do + let(:user) { create(:omniauth_user, provider: 'ldapmain', extern_uid: 'uid=myuser,ou=people,dc=example,dc=com') } + + it 'repairs ldap blocks' do + expect_any_instance_of(RepairLdapBlockedUserService).to receive(:execute) + + delete :destroy, user_id: user.username, id: user.ldap_identity.id + end + end +end diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb new file mode 100644 index 0000000000..107bfc1778 --- /dev/null +++ b/spec/models/identity_spec.rb @@ -0,0 +1,38 @@ +# == Schema Information +# +# Table name: identities +# +# id :integer not null, primary key +# extern_uid :string(255) +# provider :string(255) +# user_id :integer +# created_at :datetime +# updated_at :datetime +# + +require 'spec_helper' + +RSpec.describe Identity, models: true do + + describe 'relations' do + it { is_expected.to belong_to(:user) } + end + + describe 'fields' do + it { is_expected.to respond_to(:provider) } + it { is_expected.to respond_to(:extern_uid) } + end + + describe '#is_ldap?' do + let(:ldap_identity) { create(:identity, provider: 'ldapmain') } + let(:other_identity) { create(:identity, provider: 'twitter') } + + it 'returns true if it is a ldap identity' do + expect(ldap_identity.is_ldap?).to be_truthy + end + + it 'returns false if it is not a ldap identity' do + expect(other_identity.is_ldap?).to be_falsey + end + end +end diff --git a/spec/services/repair_ldap_blocked_user_service_spec.rb b/spec/services/repair_ldap_blocked_user_service_spec.rb new file mode 100644 index 0000000000..2a2114d038 --- /dev/null +++ b/spec/services/repair_ldap_blocked_user_service_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +describe RepairLdapBlockedUserService, services: true do + let(:user) { create(:omniauth_user, provider: 'ldapmain', state: 'ldap_blocked') } + let(:identity) { user.ldap_identity } + subject(:service) { RepairLdapBlockedUserService.new(user, identity) } + + describe '#execute' do + it 'change to normal block after destroying last ldap identity' do + identity.destroy + service.execute + + expect(user.reload).not_to be_ldap_blocked + end + + it 'change to normal block after changing last ldap identity to another provider' do + identity.update_attribute(:provider, 'twitter') + service.execute + + expect(user.reload).not_to be_ldap_blocked + end + end +end From 47e4613f4adc2d6ef4b066a87ec772ef8044bdd5 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Thu, 7 Jan 2016 14:01:01 -0200 Subject: [PATCH 035/218] Code style fixes and some code simplified --- app/controllers/admin/identities_controller.rb | 4 ++-- .../repair_ldap_blocked_user_service.rb | 18 ++++++++++-------- lib/gitlab/ldap/access.rb | 4 +++- .../repair_ldap_blocked_user_service_spec.rb | 2 +- 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/app/controllers/admin/identities_controller.rb b/app/controllers/admin/identities_controller.rb index 9ba1048751..79a53556f0 100644 --- a/app/controllers/admin/identities_controller.rb +++ b/app/controllers/admin/identities_controller.rb @@ -26,7 +26,7 @@ class Admin::IdentitiesController < Admin::ApplicationController def update if @identity.update_attributes(identity_params) - RepairLdapBlockedUserService.new(@user, @identity).execute + RepairLdapBlockedUserService.new(@user).execute redirect_to admin_user_identities_path(@user), notice: 'User identity was successfully updated.' else render :edit @@ -35,7 +35,7 @@ class Admin::IdentitiesController < Admin::ApplicationController def destroy if @identity.destroy - RepairLdapBlockedUserService.new(@user, @identity).execute + RepairLdapBlockedUserService.new(@user).execute redirect_to admin_user_identities_path(@user), notice: 'User identity was successfully removed.' else redirect_to admin_user_identities_path(@user), alert: 'Failed to remove user identity.' diff --git a/app/services/repair_ldap_blocked_user_service.rb b/app/services/repair_ldap_blocked_user_service.rb index ceca15414e..863cef7ff6 100644 --- a/app/services/repair_ldap_blocked_user_service.rb +++ b/app/services/repair_ldap_blocked_user_service.rb @@ -1,15 +1,17 @@ class RepairLdapBlockedUserService - attr_accessor :user, :identity + attr_accessor :user - def initialize(user, identity) - @user, @identity = user, identity + def initialize(user) + @user = user end def execute - if identity.destroyed? - user.block if identity.is_ldap? && user.ldap_blocked? && !user.ldap_user? - else - user.block if !identity.is_ldap? && user.ldap_blocked? && !user.ldap_user? - end + user.block if ldap_hard_blocked? + end + + private + + def ldap_hard_blocked? + user.ldap_blocked? && !user.ldap_user? end end diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 76cb48d7aa..ebd9260ad5 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -40,7 +40,9 @@ module Gitlab user.ldap_block false else - user.activate if (user.blocked? && !ldap_config.block_auto_created_users) || user.ldap_blocked? + if (user.blocked? && !ldap_config.block_auto_created_users) || user.ldap_blocked? + user.activate + end true end else diff --git a/spec/services/repair_ldap_blocked_user_service_spec.rb b/spec/services/repair_ldap_blocked_user_service_spec.rb index 2a2114d038..ce7d145597 100644 --- a/spec/services/repair_ldap_blocked_user_service_spec.rb +++ b/spec/services/repair_ldap_blocked_user_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe RepairLdapBlockedUserService, services: true do let(:user) { create(:omniauth_user, provider: 'ldapmain', state: 'ldap_blocked') } let(:identity) { user.ldap_identity } - subject(:service) { RepairLdapBlockedUserService.new(user, identity) } + subject(:service) { RepairLdapBlockedUserService.new(user) } describe '#execute' do it 'change to normal block after destroying last ldap identity' do From 1eb7b5ee8d5afeeea74ccbd5627e5a235dffe9fd Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Fri, 8 Jan 2016 22:57:42 +0100 Subject: [PATCH 036/218] Modify entities for builds API --- lib/api/builds.rb | 16 +++++++++----- lib/api/entities.rb | 37 ++++++++++++++++---------------- spec/requests/api/builds_spec.rb | 1 - 3 files changed, 29 insertions(+), 25 deletions(-) diff --git a/lib/api/builds.rb b/lib/api/builds.rb index 6aae185695..33e6ed2410 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -15,7 +15,9 @@ module API get ':id/builds' do builds = user_project.builds.order('id DESC') builds = filter_builds(builds, params[:scope]) - present paginate(builds), with: Entities::Build + + present paginate(builds), with: Entities::Build, + user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project) end # Get builds for a specific commit of a project @@ -33,7 +35,8 @@ module API builds = commit.builds.order('id DESC') builds = filter_builds(builds, params[:scope]) - present paginate(builds), with: Entities::Build + present paginate(builds), with: Entities::Build, + user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project) end # Get a specific build of a project @@ -47,7 +50,8 @@ module API build = get_build(params[:build_id]) return not_found!(build) unless build - present build, with: Entities::Build + present build, with: Entities::Build, + user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project) end # Get a trace of a specific build of a project @@ -84,7 +88,8 @@ module API build.cancel - present build, with: Entities::Build + present build, with: Entities::Build, + user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project) end # Retry a specific build of a project @@ -102,7 +107,8 @@ module API build = Ci::Build.retry(build) - present build, with: Entities::Build + present build, with: Entities::Build, + user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project) end end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index f21da54b8f..cb00b392db 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -366,16 +366,8 @@ module API expose :id, :variables end - class CiCommit < Grape::Entity + class Runner < Grape::Entity expose :id - expose :ref - expose :sha - expose :committed_at - end - - class CiRunner < Grape::Entity - expose :id - expose :token expose :description expose :active expose :is_shared @@ -383,16 +375,23 @@ module API end class Build < Grape::Entity - expose :id - expose :status - expose :stage - expose :name - expose :ref - expose :commit, with: CiCommit - expose :runner, with: CiRunner - expose :created_at - expose :started_at - expose :finished_at + expose :id, :status, :stage, :name, :ref, :tag, :coverage, :user + expose :created_at, :started_at, :finished_at + expose :download_url do |repo_obj, options| + if options[:user_can_download_artifacts] + repo_obj.download_url + else + nil + end + end + expose :commit, with: RepoCommit do |repo_obj, _options| + if repo_obj.respond_to?(:commit) + repo_obj.commit.commit_data + else + nil + end + end + expose :runner, with: Runner end end end diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb index a953eb2fac..1e58e18fad 100644 --- a/spec/requests/api/builds_spec.rb +++ b/spec/requests/api/builds_spec.rb @@ -74,7 +74,6 @@ describe API::API, api: true do expect(response.status).to eq(200) expect(json_response['name']).to eq('test') - expect(json_response['commit']['sha']).to eq(commit.sha) end it 'should return specific build trace' do From d54bff2a770c1030056866a47097aee9937390ef Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Fri, 8 Jan 2016 23:04:44 +0100 Subject: [PATCH 037/218] Change test access level from MASTER to DEVELOPER This should help us detect potential regressions in the future. --- spec/requests/api/builds_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb index 1e58e18fad..b6a1154cf7 100644 --- a/spec/requests/api/builds_spec.rb +++ b/spec/requests/api/builds_spec.rb @@ -6,7 +6,7 @@ describe API::API, api: true do let(:user) { create(:user) } let(:user2) { create(:user) } let!(:project) { create(:project, creator_id: user.id) } - let!(:master) { create(:project_member, user: user, project: project, access_level: ProjectMember::MASTER) } + let!(:developer) { create(:project_member, user: user, project: project, access_level: ProjectMember::DEVELOPER) } let!(:reporter) { create(:project_member, user: user2, project: project, access_level: ProjectMember::REPORTER) } let(:commit) { create(:ci_commit, project: project)} let(:build) { create(:ci_build, commit: commit) } From 4eb27d7c72d57015c7551a00e34a54cefc2d3db9 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Fri, 8 Jan 2016 23:33:45 +0100 Subject: [PATCH 038/218] Add some modifications to builds API and specs --- lib/api/builds.rb | 2 +- spec/requests/api/builds_spec.rb | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/api/builds.rb b/lib/api/builds.rb index 33e6ed2410..05d1b8d92e 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -103,7 +103,7 @@ module API authorize_manage_builds! build = get_build(params[:build_id]) - return not_found!(build) unless build && build.retryable? + return forbidden!('Build is not retryable') unless build && build.retryable? build = Ci::Build.retry(build) diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb index b6a1154cf7..799558d1bd 100644 --- a/spec/requests/api/builds_spec.rb +++ b/spec/requests/api/builds_spec.rb @@ -50,7 +50,7 @@ describe API::API, api: true do context 'authorized user' do it 'should return project builds for specific commit' do project.ensure_ci_commit(commit.sha) - get api("/projects/#{project.id}/builds/commit/#{project.ci_commits.first.sha}", user) + get api("/projects/#{project.id}/builds/commit/#{commit.sha}", user) expect(response.status).to eq(200) expect(json_response).to be_an Array @@ -60,7 +60,7 @@ describe API::API, api: true do context 'unauthorized user' do it 'should not return project builds' do project.ensure_ci_commit(commit.sha) - get api("/projects/#{project.id}/builds/commit/#{project.ci_commits.first.sha}") + get api("/projects/#{project.id}/builds/commit/#{commit.sha}") expect(response.status).to eq(401) end @@ -99,7 +99,7 @@ describe API::API, api: true do end end - describe 'GET /projects/:id/builds/:build_id/cancel' do + describe 'POST /projects/:id/builds/:build_id/cancel' do context 'authorized user' do context 'user with :manage_builds persmission' do it 'should cancel running or pending build' do @@ -128,7 +128,7 @@ describe API::API, api: true do end end - describe 'GET /projects/:id/builds/:build_id/retry' do + describe 'POST /projects/:id/builds/:build_id/retry' do context 'authorized user' do context 'user with :manage_builds persmission' do it 'should retry non-running build' do From 58867eff46dc6886b85bfe5a787341f224d09421 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 9 Dec 2015 11:59:25 +0100 Subject: [PATCH 039/218] Unsubscribe from thread through link in email footer --- CHANGELOG | 1 + .../sent_notifications_controller.rb | 25 +++++++++ app/mailers/emails/issues.rb | 38 ++++++------- app/mailers/emails/merge_requests.rb | 55 +++++++------------ app/mailers/emails/notes.rb | 40 +++++++------- app/mailers/notify.rb | 5 +- app/models/concerns/issuable.rb | 6 ++ app/models/sent_notification.rb | 8 ++- app/views/layouts/notify.html.haml | 8 ++- config/routes.rb | 6 ++ .../sent_notification_controller_spec.rb | 26 +++++++++ spec/factories.rb | 7 +++ spec/mailers/notify_spec.rb | 34 +++++++++++- 13 files changed, 175 insertions(+), 84 deletions(-) create mode 100644 app/controllers/sent_notifications_controller.rb create mode 100644 spec/controllers/sent_notification_controller_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 921dac0126..38c0dbf093 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -59,6 +59,7 @@ v 8.3.0 - Add open_issues_count to project API (Stan Hu) - Expand character set of usernames created by Omniauth (Corey Hinshaw) - Add button to automatically merge a merge request when the build succeeds (Zeger-Jan van de Weg) + - Add unsubscribe link in the email footer (Zeger-Jan van de Weg) - Provide better diagnostic message upon project creation errors (Stan Hu) - Bump devise to 3.5.3 to fix reset token expiring after account creation (Stan Hu) - Remove api credentials from link to build_page diff --git a/app/controllers/sent_notifications_controller.rb b/app/controllers/sent_notifications_controller.rb new file mode 100644 index 0000000000..93dcc16094 --- /dev/null +++ b/app/controllers/sent_notifications_controller.rb @@ -0,0 +1,25 @@ +class SentNotificationsController < ApplicationController + skip_before_action :authenticate_user! + + def unsubscribe + @sent_notification = SentNotification.for(params[:id]) + return render_404 unless @sent_notification && !@sent_notification.for_commit? + + noteable = @sent_notification.noteable + noteable.unsubscribe(@sent_notification.recipient) + + flash[:notice] = "You have been unsubscribed from this thread." + if current_user + case @sent_notification.noteable + when Issue + redirect_to issue_path(noteable) + when MergeRequest + redirect_to merge_request_path(noteable) + else + redirect_to root_path + end + else + redirect_to new_user_session_path + end + end +end diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index abdeefed5e..4a88cb6113 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -1,31 +1,31 @@ module Emails module Issues def new_issue_email(recipient_id, issue_id) - issue_mail_with_notification(issue_id, recipient_id) do - mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id)) - end + setup_issue_mail(issue_id, recipient_id) + + mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id)) end def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id) - issue_mail_with_notification(issue_id, recipient_id) do - @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id - mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) - end + setup_issue_mail(issue_id, recipient_id) + + @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id + mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) end def closed_issue_email(recipient_id, issue_id, updated_by_user_id) - issue_mail_with_notification(issue_id, recipient_id) do - @updated_by = User.find updated_by_user_id - mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) - end + setup_issue_mail(issue_id, recipient_id) + + @updated_by = User.find updated_by_user_id + mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) end def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id) - issue_mail_with_notification(issue_id, recipient_id) do - @issue_status = status - @updated_by = User.find updated_by_user_id - mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) - end + setup_issue_mail(issue_id, recipient_id) + + @issue_status = status + @updated_by = User.find updated_by_user_id + mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) end private @@ -38,14 +38,12 @@ module Emails } end - def issue_mail_with_notification(issue_id, recipient_id) + def setup_issue_mail(issue_id, recipient_id) @issue = Issue.find(issue_id) @project = @issue.project @target_url = namespace_project_issue_url(@project.namespace, @project, @issue) - yield - - SentNotification.record(@issue, recipient_id, reply_key) + @sent_notification = SentNotification.record(@issue, recipient_id, reply_key) end end end diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index 7923fb770d..325996e2e1 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -1,77 +1,64 @@ module Emails module MergeRequests def new_merge_request_email(recipient_id, merge_request_id) - @merge_request = MergeRequest.find(merge_request_id) - @project = @merge_request.project - @target_url = namespace_project_merge_request_url(@project.namespace, - @project, - @merge_request) + setup_merge_request_mail(merge_request_id, recipient_id) + mail_new_thread(@merge_request, from: sender(@merge_request.author_id), to: recipient(recipient_id), subject: subject("#{@merge_request.title} (##{@merge_request.iid})")) - - SentNotification.record(@merge_request, recipient_id, reply_key) end def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id) - @merge_request = MergeRequest.find(merge_request_id) + setup_merge_request_mail(merge_request_id, recipient_id) + @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id - @project = @merge_request.project - @target_url = namespace_project_merge_request_url(@project.namespace, - @project, - @merge_request) mail_answer_thread(@merge_request, from: sender(updated_by_user_id), to: recipient(recipient_id), subject: subject("#{@merge_request.title} (##{@merge_request.iid})")) - - SentNotification.record(@merge_request, recipient_id, reply_key) end def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id) - @merge_request = MergeRequest.find(merge_request_id) + setup_merge_request_mail(merge_request_id, recipient_id) + @updated_by = User.find updated_by_user_id - @project = @merge_request.project - @target_url = namespace_project_merge_request_url(@project.namespace, - @project, - @merge_request) mail_answer_thread(@merge_request, from: sender(updated_by_user_id), to: recipient(recipient_id), subject: subject("#{@merge_request.title} (##{@merge_request.iid})")) - - SentNotification.record(@merge_request, recipient_id, reply_key) end def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id) - @merge_request = MergeRequest.find(merge_request_id) - @project = @merge_request.project - @target_url = namespace_project_merge_request_url(@project.namespace, - @project, - @merge_request) + setup_merge_request_mail(merge_request_id, recipient_id) + mail_answer_thread(@merge_request, from: sender(updated_by_user_id), to: recipient(recipient_id), subject: subject("#{@merge_request.title} (##{@merge_request.iid})")) - - SentNotification.record(@merge_request, recipient_id, reply_key) end def merge_request_status_email(recipient_id, merge_request_id, status, updated_by_user_id) - @merge_request = MergeRequest.find(merge_request_id) + setup_merge_request_mail(merge_request_id, recipient_id) + @mr_status = status - @project = @merge_request.project @updated_by = User.find updated_by_user_id - @target_url = namespace_project_merge_request_url(@project.namespace, - @project, - @merge_request) mail_answer_thread(@merge_request, from: sender(updated_by_user_id), to: recipient(recipient_id), subject: subject("#{@merge_request.title} (##{@merge_request.iid})")) + end - SentNotification.record(@merge_request, recipient_id, reply_key) + private + + def setup_merge_request_mail(merge_request_id, recipient_id) + @merge_request = MergeRequest.find(merge_request_id) + @project = @merge_request.project + @target_url = namespace_project_merge_request_url(@project.namespace, + @project, + @merge_request) + + @sent_notification = SentNotification.record(@merge_request, recipient_id, reply_key) end end end diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb index e1382d2da1..ccd33b880f 100644 --- a/app/mailers/emails/notes.rb +++ b/app/mailers/emails/notes.rb @@ -1,31 +1,31 @@ module Emails module Notes def note_commit_email(recipient_id, note_id) - note_mail_with_notification(note_id, recipient_id) do - @commit = @note.noteable - @target_url = namespace_project_commit_url(*note_target_url_options) + note_mail_with_notification(note_id, recipient_id) - mail_answer_thread(@commit, - from: sender(@note.author_id), - to: recipient(recipient_id), - subject: subject("#{@commit.title} (#{@commit.short_id})")) - end + @commit = @note.noteable + @target_url = namespace_project_commit_url(*note_target_url_options) + + mail_answer_thread(@commit, + from: sender(@note.author_id), + to: recipient(recipient_id), + subject: subject("#{@commit.title} (#{@commit.short_id})")) end def note_issue_email(recipient_id, note_id) - note_mail_with_notification(note_id, recipient_id) do - @issue = @note.noteable - @target_url = namespace_project_issue_url(*note_target_url_options) - mail_answer_thread(@issue, note_thread_options(recipient_id)) - end + note_mail_with_notification(note_id, recipient_id) + + @issue = @note.noteable + @target_url = namespace_project_issue_url(*note_target_url_options) + mail_answer_thread(@issue, note_thread_options(recipient_id)) end def note_merge_request_email(recipient_id, note_id) - note_mail_with_notification(note_id, recipient_id) do - @merge_request = @note.noteable - @target_url = namespace_project_merge_request_url(*note_target_url_options) - mail_answer_thread(@merge_request, note_thread_options(recipient_id)) - end + note_mail_with_notification(note_id, recipient_id) + + @merge_request = @note.noteable + @target_url = namespace_project_merge_request_url(*note_target_url_options) + mail_answer_thread(@merge_request, note_thread_options(recipient_id)) end private @@ -46,9 +46,7 @@ module Emails @note = Note.find(note_id) @project = @note.project - yield - - SentNotification.record_note(@note, recipient_id, reply_key) + @sent_notification = SentNotification.record_note(@note, recipient_id, reply_key) end end end diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 3bbdd9cee7..e1cd075a97 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -107,10 +107,9 @@ class Notify < BaseMailer end headers["X-GitLab-#{model.class.name}-ID"] = model.id + headers['X-GitLab-Reply-Key'] = reply_key - if reply_key - headers['X-GitLab-Reply-Key'] = reply_key - + if Gitlab::IncomingEmail.enabled? address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key)) address.display_name = @project.name_with_namespace diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 18a00f95b4..04650a9e67 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -119,6 +119,12 @@ module Issuable update(subscribed: !subscribed?(user)) end + def unsubscribe(user) + subscriptions. + find_or_initialize_by(user_id: user.id). + update(subscribed: false) + end + def to_hook_data(user) { object_kind: self.class.name.underscore, diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb index f36eda1531..fd12615717 100644 --- a/app/models/sent_notification.rb +++ b/app/models/sent_notification.rb @@ -25,8 +25,6 @@ class SentNotification < ActiveRecord::Base class << self def reply_key - return nil unless Gitlab::IncomingEmail.enabled? - SecureRandom.hex(16) end @@ -59,7 +57,7 @@ class SentNotification < ActiveRecord::Base def record_note(note, recipient_id, reply_key, params = {}) params[:line_code] = note.line_code - + record(note.noteable, recipient_id, reply_key, params) end end @@ -75,4 +73,8 @@ class SentNotification < ActiveRecord::Base super end end + + def to_param + self.reply_key + end end diff --git a/app/views/layouts/notify.html.haml b/app/views/layouts/notify.html.haml index 3ca4c34040..da7de41aba 100644 --- a/app/views/layouts/notify.html.haml +++ b/app/views/layouts/notify.html.haml @@ -44,6 +44,10 @@ %br -# Don't link the host is the line below, one link in the email is easier to quickly click than two. You're receiving this email because of your account on #{Gitlab.config.gitlab.host}. - If you'd like to receive fewer emails, you can adjust your notification settings. + If you'd like to receive fewer emails, you can + - if @sent_notification && !@sent_notification.for_commit? + = link_to "unsubscribe", unsubscribe_sent_notification_url(@sent_notification) + from this thread or + adjust your notification settings. - = email_action @target_url \ No newline at end of file + = email_action @target_url diff --git a/config/routes.rb b/config/routes.rb index 4fcea1185c..2739e4961f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -88,6 +88,12 @@ Rails.application.routes.draw do end end + resources :sent_notifications, only: [], constraints: { id: /[0-9a-f]{32}/ } do + member do + get :unsubscribe + end + end + # Spam reports resources :abuse_reports, only: [:new, :create] diff --git a/spec/controllers/sent_notification_controller_spec.rb b/spec/controllers/sent_notification_controller_spec.rb new file mode 100644 index 0000000000..9ced397bd4 --- /dev/null +++ b/spec/controllers/sent_notification_controller_spec.rb @@ -0,0 +1,26 @@ +require 'rails_helper' + +describe SentNotificationsController, type: :controller do + let(:user) { create(:user) } + let(:issue) { create(:issue, author: user) } + let(:sent_notification) { create(:sent_notification, noteable: issue) } + + describe 'GET #unsubscribe' do + it 'returns a 404 when calling without existing id' do + get(:unsubscribe, id: '0' * 32) + + expect(response.status).to be 404 + end + + context 'calling with id' do + it 'shows a flash message to the user' do + get(:unsubscribe, id: sent_notification.reply_key) + + expect(response.status).to be 302 + + expect(response).to redirect_to new_user_session_path + expect(controller).to set_flash[:notice].to(/unsubscribed/).now + end + end + end +end diff --git a/spec/factories.rb b/spec/factories.rb index d6b4efa9a0..2a81684dfc 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -212,4 +212,11 @@ FactoryGirl.define do provider 'ldapmain' extern_uid 'my-ldap-id' end + + factory :sent_notification do + project + recipient factory: :user + noteable factory: :issue + reply_key "0123456789abcdef" * 2 + end end diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 154901a2fb..08ade64464 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -104,6 +104,14 @@ describe Notify do it { is_expected.to have_body_text /View Commit/ } end + shared_examples 'an unsubscribeable thread' do + it { is_expected.to have_body_text /unsubscribe/ } + end + + shared_examples "a user can not unsubscribe through footer link" do + it { is_expected.not_to have_body_text /unsubscribe/ } + end + describe 'for new users, the email' do let(:example_site_path) { root_path } let(:new_user) { create(:user, email: new_user_address, created_by_id: 1) } @@ -115,6 +123,7 @@ describe Notify do it_behaves_like 'an email sent from GitLab' it_behaves_like 'a new user email', new_user_address it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like 'a user can not unsubscribe through footer link' it 'contains the password text' do is_expected.to have_body_text /Click here to set your password/ @@ -134,7 +143,6 @@ describe Notify do end end - describe 'for users that signed up, the email' do let(:example_site_path) { root_path } let(:new_user) { create(:user, email: new_user_address, password: "securePassword") } @@ -144,6 +152,7 @@ describe Notify do it_behaves_like 'an email sent from GitLab' it_behaves_like 'a new user email', new_user_address it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like 'a user can not unsubscribe through footer link' it 'should not contain the new user\'s password' do is_expected.not_to have_body_text /password/ @@ -157,6 +166,7 @@ describe Notify do it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like 'a user can not unsubscribe through footer link' it 'is sent to the new user' do is_expected.to deliver_to key.user.email @@ -181,6 +191,7 @@ describe Notify do subject { Notify.new_email_email(email.id) } it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like 'a user can not unsubscribe through footer link' it 'is sent to the new user' do is_expected.to deliver_to email.user.email @@ -227,6 +238,7 @@ describe Notify do it_behaves_like 'an assignee email' it_behaves_like 'an email starting a new thread', 'issue' it_behaves_like 'it should show Gmail Actions View Issue link' + it_behaves_like 'an unsubscribeable thread' it 'has the correct subject' do is_expected.to have_subject /#{project.name} \| #{issue.title} \(##{issue.iid}\)/ @@ -253,6 +265,7 @@ describe Notify do it_behaves_like 'a multiple recipients email' it_behaves_like 'an answer to an existing thread', 'issue' it_behaves_like 'it should show Gmail Actions View Issue link' + it_behaves_like "an unsubscribeable thread" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -283,6 +296,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread', 'issue' it_behaves_like 'it should show Gmail Actions View Issue link' + it_behaves_like 'an unsubscribeable thread' it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -319,6 +333,7 @@ describe Notify do it_behaves_like 'an assignee email' it_behaves_like 'an email starting a new thread', 'merge_request' it_behaves_like 'it should show Gmail Actions View Merge request link' + it_behaves_like "an unsubscribeable thread" it 'has the correct subject' do is_expected.to have_subject /#{merge_request.title} \(##{merge_request.iid}\)/ @@ -345,6 +360,7 @@ describe Notify do subject { Notify.new_merge_request_email(merge_request_with_description.assignee_id, merge_request_with_description.id) } it_behaves_like 'it should show Gmail Actions View Merge request link' + it_behaves_like "an unsubscribeable thread" it 'contains the description' do is_expected.to have_body_text /#{merge_request_with_description.description}/ @@ -357,6 +373,7 @@ describe Notify do it_behaves_like 'a multiple recipients email' it_behaves_like 'an answer to an existing thread', 'merge_request' it_behaves_like 'it should show Gmail Actions View Merge request link' + it_behaves_like "an unsubscribeable thread" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -387,6 +404,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread', 'merge_request' it_behaves_like 'it should show Gmail Actions View Merge request link' + it_behaves_like "an unsubscribeable thread" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -417,6 +435,7 @@ describe Notify do it_behaves_like 'a multiple recipients email' it_behaves_like 'an answer to an existing thread', 'merge_request' it_behaves_like 'it should show Gmail Actions View Merge request link' + it_behaves_like "an unsubscribeable thread" it 'is sent as the merge author' do sender = subject.header[:from].addrs[0] @@ -446,6 +465,7 @@ describe Notify do it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like "a user can not unsubscribe through footer link" it 'has the correct subject' do is_expected.to have_subject /Project was moved/ @@ -468,6 +488,7 @@ describe Notify do it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like "a user can not unsubscribe through footer link" it 'has the correct subject' do is_expected.to have_subject /Access to project was granted/ @@ -518,6 +539,7 @@ describe Notify do it_behaves_like 'a note email' it_behaves_like 'an answer to an existing thread', 'commit' it_behaves_like 'it should show Gmail Actions View Commit link' + it_behaves_like "a user can not unsubscribe through footer link" it 'has the correct subject' do is_expected.to have_subject /#{commit.title} \(#{commit.short_id}\)/ @@ -538,6 +560,7 @@ describe Notify do it_behaves_like 'a note email' it_behaves_like 'an answer to an existing thread', 'merge_request' it_behaves_like 'it should show Gmail Actions View Merge request link' + it_behaves_like 'an unsubscribeable thread' it 'has the correct subject' do is_expected.to have_subject /#{merge_request.title} \(##{merge_request.iid}\)/ @@ -558,6 +581,7 @@ describe Notify do it_behaves_like 'a note email' it_behaves_like 'an answer to an existing thread', 'issue' it_behaves_like 'it should show Gmail Actions View Issue link' + it_behaves_like 'an unsubscribeable thread' it 'has the correct subject' do is_expected.to have_subject /#{issue.title} \(##{issue.iid}\)/ @@ -579,6 +603,7 @@ describe Notify do it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like "a user can not unsubscribe through footer link" it 'has the correct subject' do is_expected.to have_subject /Access to group was granted/ @@ -607,6 +632,7 @@ describe Notify do subject { ActionMailer::Base.deliveries.last } it_behaves_like 'an email sent from GitLab' + it_behaves_like "a user can not unsubscribe through footer link" it 'is sent to the new user' do is_expected.to deliver_to 'new-email@mail.com' @@ -629,6 +655,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :create) } it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like "a user can not unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -657,6 +684,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/tags/v1.0', action: :create) } it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like "a user can not unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -684,6 +712,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :delete) } it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like "a user can not unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -707,6 +736,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/tags/v1.0', action: :delete) } it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like "a user can not unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -734,6 +764,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, send_from_committer_email: send_from_committer_email) } it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like "a user can not unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -839,6 +870,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare) } it_behaves_like 'it should show Gmail Actions View Commit link' + it_behaves_like "a user can not unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] From 36d858bcf98868426256ce51a28e7029b38e0c2d Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Sat, 9 Jan 2016 12:47:31 +0100 Subject: [PATCH 040/218] Add #can_unsubscribe? to SentNotification --- app/models/sent_notification.rb | 4 ++++ app/views/layouts/notify.html.haml | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb index fd12615717..03108da17b 100644 --- a/app/models/sent_notification.rb +++ b/app/models/sent_notification.rb @@ -62,6 +62,10 @@ class SentNotification < ActiveRecord::Base end end + def can_unsubscribe? + !for_commit? + end + def for_commit? noteable_type == "Commit" end diff --git a/app/views/layouts/notify.html.haml b/app/views/layouts/notify.html.haml index da7de41aba..07f9be12f9 100644 --- a/app/views/layouts/notify.html.haml +++ b/app/views/layouts/notify.html.haml @@ -45,7 +45,7 @@ -# Don't link the host is the line below, one link in the email is easier to quickly click than two. You're receiving this email because of your account on #{Gitlab.config.gitlab.host}. If you'd like to receive fewer emails, you can - - if @sent_notification && !@sent_notification.for_commit? + - if @sent_notification && @sent_notification.can_unsubscribe? = link_to "unsubscribe", unsubscribe_sent_notification_url(@sent_notification) from this thread or adjust your notification settings. From 26cedc7e0bd83fc488da3a4dc5265d395639215f Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Sat, 9 Jan 2016 19:32:03 +0100 Subject: [PATCH 041/218] Minor improvements, unsubscribe from email footer --- .../sent_notifications_controller.rb | 4 +-- app/mailers/emails/notes.rb | 8 ++--- config/routes.rb | 2 +- spec/mailers/notify_spec.rb | 32 +++++++++---------- 4 files changed, 23 insertions(+), 23 deletions(-) diff --git a/app/controllers/sent_notifications_controller.rb b/app/controllers/sent_notifications_controller.rb index 93dcc16094..b7008c82bf 100644 --- a/app/controllers/sent_notifications_controller.rb +++ b/app/controllers/sent_notifications_controller.rb @@ -3,14 +3,14 @@ class SentNotificationsController < ApplicationController def unsubscribe @sent_notification = SentNotification.for(params[:id]) - return render_404 unless @sent_notification && !@sent_notification.for_commit? + return render_404 unless @sent_notification && @sent_notification.can_unsubscribe? noteable = @sent_notification.noteable noteable.unsubscribe(@sent_notification.recipient) flash[:notice] = "You have been unsubscribed from this thread." if current_user - case @sent_notification.noteable + case noteable when Issue redirect_to issue_path(noteable) when MergeRequest diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb index ccd33b880f..f9650df9a7 100644 --- a/app/mailers/emails/notes.rb +++ b/app/mailers/emails/notes.rb @@ -1,7 +1,7 @@ module Emails module Notes def note_commit_email(recipient_id, note_id) - note_mail_with_notification(note_id, recipient_id) + setup_note_mail(note_id, recipient_id) @commit = @note.noteable @target_url = namespace_project_commit_url(*note_target_url_options) @@ -13,7 +13,7 @@ module Emails end def note_issue_email(recipient_id, note_id) - note_mail_with_notification(note_id, recipient_id) + setup_note_mail(note_id, recipient_id) @issue = @note.noteable @target_url = namespace_project_issue_url(*note_target_url_options) @@ -21,7 +21,7 @@ module Emails end def note_merge_request_email(recipient_id, note_id) - note_mail_with_notification(note_id, recipient_id) + setup_note_mail(note_id, recipient_id) @merge_request = @note.noteable @target_url = namespace_project_merge_request_url(*note_target_url_options) @@ -42,7 +42,7 @@ module Emails } end - def note_mail_with_notification(note_id, recipient_id) + def setup_note_mail(note_id, recipient_id) @note = Note.find(note_id) @project = @note.project diff --git a/config/routes.rb b/config/routes.rb index 2739e4961f..0fd72ae78e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -88,7 +88,7 @@ Rails.application.routes.draw do end end - resources :sent_notifications, only: [], constraints: { id: /[0-9a-f]{32}/ } do + resources :sent_notifications, only: [], constraints: { id: /\h{32}/ } do member do get :unsubscribe end diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 08ade64464..8f86c491d3 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -108,7 +108,7 @@ describe Notify do it { is_expected.to have_body_text /unsubscribe/ } end - shared_examples "a user can not unsubscribe through footer link" do + shared_examples "a user cannot unsubscribe through footer link" do it { is_expected.not_to have_body_text /unsubscribe/ } end @@ -123,7 +123,7 @@ describe Notify do it_behaves_like 'an email sent from GitLab' it_behaves_like 'a new user email', new_user_address it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like 'a user can not unsubscribe through footer link' + it_behaves_like 'a user cannot unsubscribe through footer link' it 'contains the password text' do is_expected.to have_body_text /Click here to set your password/ @@ -152,7 +152,7 @@ describe Notify do it_behaves_like 'an email sent from GitLab' it_behaves_like 'a new user email', new_user_address it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like 'a user can not unsubscribe through footer link' + it_behaves_like 'a user cannot unsubscribe through footer link' it 'should not contain the new user\'s password' do is_expected.not_to have_body_text /password/ @@ -166,7 +166,7 @@ describe Notify do it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like 'a user can not unsubscribe through footer link' + it_behaves_like 'a user cannot unsubscribe through footer link' it 'is sent to the new user' do is_expected.to deliver_to key.user.email @@ -191,7 +191,7 @@ describe Notify do subject { Notify.new_email_email(email.id) } it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like 'a user can not unsubscribe through footer link' + it_behaves_like 'a user cannot unsubscribe through footer link' it 'is sent to the new user' do is_expected.to deliver_to email.user.email @@ -465,7 +465,7 @@ describe Notify do it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like "a user can not unsubscribe through footer link" + it_behaves_like "a user cannot unsubscribe through footer link" it 'has the correct subject' do is_expected.to have_subject /Project was moved/ @@ -488,7 +488,7 @@ describe Notify do it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like "a user can not unsubscribe through footer link" + it_behaves_like "a user cannot unsubscribe through footer link" it 'has the correct subject' do is_expected.to have_subject /Access to project was granted/ @@ -539,7 +539,7 @@ describe Notify do it_behaves_like 'a note email' it_behaves_like 'an answer to an existing thread', 'commit' it_behaves_like 'it should show Gmail Actions View Commit link' - it_behaves_like "a user can not unsubscribe through footer link" + it_behaves_like "a user cannot unsubscribe through footer link" it 'has the correct subject' do is_expected.to have_subject /#{commit.title} \(#{commit.short_id}\)/ @@ -603,7 +603,7 @@ describe Notify do it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like "a user can not unsubscribe through footer link" + it_behaves_like "a user cannot unsubscribe through footer link" it 'has the correct subject' do is_expected.to have_subject /Access to group was granted/ @@ -632,7 +632,7 @@ describe Notify do subject { ActionMailer::Base.deliveries.last } it_behaves_like 'an email sent from GitLab' - it_behaves_like "a user can not unsubscribe through footer link" + it_behaves_like "a user cannot unsubscribe through footer link" it 'is sent to the new user' do is_expected.to deliver_to 'new-email@mail.com' @@ -655,7 +655,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :create) } it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like "a user can not unsubscribe through footer link" + it_behaves_like "a user cannot unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -684,7 +684,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/tags/v1.0', action: :create) } it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like "a user can not unsubscribe through footer link" + it_behaves_like "a user cannot unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -712,7 +712,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :delete) } it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like "a user can not unsubscribe through footer link" + it_behaves_like "a user cannot unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -736,7 +736,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/tags/v1.0', action: :delete) } it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like "a user can not unsubscribe through footer link" + it_behaves_like "a user cannot unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -764,7 +764,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, send_from_committer_email: send_from_committer_email) } it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like "a user can not unsubscribe through footer link" + it_behaves_like "a user cannot unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -870,7 +870,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare) } it_behaves_like 'it should show Gmail Actions View Commit link' - it_behaves_like "a user can not unsubscribe through footer link" + it_behaves_like "a user cannot unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] From 96bbc145f31ad029e080ad8903445d81d6c31968 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 11 Jan 2016 10:20:45 +0100 Subject: [PATCH 042/218] Change commit builds URL in builds API --- lib/api/builds.rb | 4 ++-- spec/requests/api/builds_spec.rb | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/api/builds.rb b/lib/api/builds.rb index 05d1b8d92e..8c21754596 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -28,8 +28,8 @@ module API # scope (optional) - The scope of builds to show (one or array of: pending, running, failed, success, canceled; # if none provided showing all builds) # Example Request: - # GET /projects/:id/builds/commit/:sha - get ':id/builds/commit/:sha' do + # GET /projects/:id/repository/commits/:sha/builds + get ':id/repository/commits/:sha/builds' do commit = user_project.ci_commits.find_by_sha(params[:sha]) return not_found! unless commit diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb index 799558d1bd..587fb74750 100644 --- a/spec/requests/api/builds_spec.rb +++ b/spec/requests/api/builds_spec.rb @@ -46,11 +46,11 @@ describe API::API, api: true do end end - describe 'GET /projects/:id/builds/commit/:sha' do + describe 'GET /projects/:id/repository/commits/:sha/builds' do context 'authorized user' do it 'should return project builds for specific commit' do project.ensure_ci_commit(commit.sha) - get api("/projects/#{project.id}/builds/commit/#{commit.sha}", user) + get api("/projects/#{project.id}/repository/commits/#{commit.sha}/builds", user) expect(response.status).to eq(200) expect(json_response).to be_an Array @@ -60,7 +60,7 @@ describe API::API, api: true do context 'unauthorized user' do it 'should not return project builds' do project.ensure_ci_commit(commit.sha) - get api("/projects/#{project.id}/builds/commit/#{commit.sha}") + get api("/projects/#{project.id}/repository/commits/#{commit.sha}/builds") expect(response.status).to eq(401) end From 4e70f2519bba83d5f9d6fd0bed80e9837e8b5fc5 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 11 Jan 2016 11:15:04 +0100 Subject: [PATCH 043/218] Update ./doc/api/builds.md --- doc/api/builds.md | 301 ++++++++++++++++++++++++++++++++++---------- lib/api/entities.rb | 3 +- 2 files changed, 234 insertions(+), 70 deletions(-) diff --git a/doc/api/builds.md b/doc/api/builds.md index c52266714d..64efe6bb3b 100644 --- a/doc/api/builds.md +++ b/doc/api/builds.md @@ -17,37 +17,97 @@ Parameters: [ { "commit": { - "committed_at": "2015-12-28T14:34:03.814Z", - "id": 2, - "ref": null, - "sha": "6b053ad388c531c21907f022933e5e81598db388" + "author_email": "admin@example.com", + "author_name": "Administrator", + "created_at": "2015-12-24T16:51:14.000+01:00", + "id": "0ff3ae198f8601a285adcf5c0fff204ee6fba5fd", + "message": "Test the CI integration.", + "short_id": "0ff3ae19", + "title": "Test the CI integration." }, - "created_at": "2016-01-04T15:41:23.147Z", - "finished_at": null, - "id": 65, - "name": "brakeman", + "coverage": null, + "created_at": "2015-12-24T15:51:21.802Z", + "download_url": null, + "finished_at": "2015-12-24T17:54:27.895Z", + "id": 7, + "name": "teaspoon", "ref": "master", "runner": null, "stage": "test", - "started_at": null, - "status": "pending" + "started_at": "2015-12-24T17:54:27.722Z", + "status": "failed", + "tag": false, + "user": { + "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", + "bio": null, + "can_create_group": true, + "can_create_project": true, + "color_scheme_id": 2, + "created_at": "2015-12-21T13:14:24.077Z", + "current_sign_in_at": "2016-01-11T09:31:40.472Z", + "email": "admin@example.com", + "id": 1, + "identities": [], + "is_admin": true, + "linkedin": "", + "name": "Administrator", + "projects_limit": 100, + "skype": "", + "state": "active", + "theme_id": 3, + "twitter": "", + "two_factor_enabled": false, + "username": "root", + "web_url": "http://gitlab.dev/u/root", + "website_url": "" + } }, { "commit": { - "committed_at": "2015-12-28T14:34:03.814Z", - "id": 2, - "ref": null, - "sha": "6b053ad388c531c21907f022933e5e81598db388" + "author_email": "admin@example.com", + "author_name": "Administrator", + "created_at": "2015-12-24T16:51:14.000+01:00", + "id": "0ff3ae198f8601a285adcf5c0fff204ee6fba5fd", + "message": "Test the CI integration.", + "short_id": "0ff3ae19", + "title": "Test the CI integration." }, - "created_at": "2016-01-04T15:41:23.046Z", - "finished_at": null, - "id": 64, - "name": "rubocop", + "coverage": null, + "created_at": "2015-12-24T15:51:21.727Z", + "download_url": null, + "finished_at": "2015-12-24T17:54:24.921Z", + "id": 6, + "name": "spinach:other", "ref": "master", "runner": null, "stage": "test", - "started_at": null, - "status": "pending" + "started_at": "2015-12-24T17:54:24.729Z", + "status": "failed", + "tag": false, + "user": { + "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", + "bio": null, + "can_create_group": true, + "can_create_project": true, + "color_scheme_id": 2, + "created_at": "2015-12-21T13:14:24.077Z", + "current_sign_in_at": "2016-01-11T09:31:40.472Z", + "email": "admin@example.com", + "id": 1, + "identities": [], + "is_admin": true, + "linkedin": "", + "name": "Administrator", + "projects_limit": 100, + "skype": "", + "state": "active", + "theme_id": 3, + "twitter": "", + "two_factor_enabled": false, + "username": "root", + "web_url": "http://gitlab.dev/u/root", + "website_url": "" + } } ] ``` @@ -57,7 +117,7 @@ Parameters: Get a list of builds for specific commit in a project. ``` -GET /projects/:id/builds/commit/:sha +GET /projects/:id/repository/commits/:sha/builds ``` Parameters: @@ -67,45 +127,104 @@ Parameters: - `scope` (optional) - The scope of builds to show (one or array of: pending, running, failed, success, canceled; if none provided showing all builds) ```json -[ - { - "commit": { - "committed_at": "2015-12-28T14:34:03.814Z", - "id": 2, - "ref": null, - "sha": "6b053ad388c531c21907f022933e5e81598db388" + +``` + +## Get a single build +mmit": { + "author_email": "admin@example.com", + "author_name": "Administrator", + "created_at": "2015-12-24T16:51:14.000+01:00", + "id": "0ff3ae198f8601a285adcf5c0fff204ee6fba5fd", + "message": "Test the CI integration.", + "short_id": "0ff3ae19", + "title": "Test the CI integration." }, - "created_at": "2016-01-04T15:41:23.147Z", - "finished_at": null, - "id": 65, + "coverage": null, + "created_at": "2015-12-24T15:51:21.957Z", + "download_url": null, + "finished_at": "2015-12-24T17:54:33.913Z", + "id": 9, "name": "brakeman", "ref": "master", "runner": null, "stage": "test", - "started_at": null, - "status": "pending" + "started_at": "2015-12-24T17:54:33.727Z", + "status": "failed", + "tag": false, + "user": { + "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", + "bio": null, + "can_create_group": true, + "can_create_project": true, + "color_scheme_id": 2, + "created_at": "2015-12-21T13:14:24.077Z", + "current_sign_in_at": "2016-01-11T09:31:40.472Z", + "email": "admin@example.com", + "id": 1, + "identities": [], + "is_admin": true, + "linkedin": "", + "name": "Administrator", + "projects_limit": 100, + "skype": "", + "state": "active", + "theme_id": 3, + "twitter": "", + "two_factor_enabled": false, + "username": "root", + "web_url": "http://gitlab.dev/u/root", + "website_url": "" + } }, { "commit": { - "committed_at": "2015-12-28T14:34:03.814Z", - "id": 2, - "ref": null, - "sha": "6b053ad388c531c21907f022933e5e81598db388" + "author_email": "admin@example.com", + "author_name": "Administrator", + "created_at": "2015-12-24T16:51:14.000+01:00", + "id": "0ff3ae198f8601a285adcf5c0fff204ee6fba5fd", + "message": "Test the CI integration.", + "short_id": "0ff3ae19", + "title": "Test the CI integration." }, - "created_at": "2016-01-04T15:41:23.046Z", - "finished_at": null, - "id": 64, + "coverage": null, + "created_at": "2015-12-24T15:51:21.880Z", + "download_url": null, + "finished_at": "2015-12-24T17:54:31.198Z", + "id": 8, "name": "rubocop", "ref": "master", "runner": null, "stage": "test", - "started_at": null, - "status": "pending" + "started_at": "2015-12-24T17:54:30.733Z", + "status": "failed", + "tag": false, + "user": { + "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", + "bio": null, + "can_create_group": true, + "can_create_project": true, + "color_scheme_id": 2, + "created_at": "2015-12-21T13:14:24.077Z", + "current_sign_in_at": "2016-01-11T09:31:40.472Z", + "email": "admin@example.com", + "id": 1, + "identities": [], + "is_admin": true, + "linkedin": "", + "name": "Administrator", + "projects_limit": 100, + "skype": "", + "state": "active", + "theme_id": 3, + "twitter": "", + "two_factor_enabled": false, + "username": "root", + "web_url": "http://gitlab.dev/u/root", + "website_url": "" + } } ] -``` - -## Get a single build Get a single build of a project @@ -121,20 +240,50 @@ Parameters: ```json { "commit": { - "committed_at": "2015-12-28T14:34:03.814Z", - "id": 2, - "ref": null, - "sha": "6b053ad388c531c21907f022933e5e81598db388" + "author_email": "admin@example.com", + "author_name": "Administrator", + "created_at": "2015-12-24T16:51:14.000+01:00", + "id": "0ff3ae198f8601a285adcf5c0fff204ee6fba5fd", + "message": "Test the CI integration.", + "short_id": "0ff3ae19", + "title": "Test the CI integration." }, - "created_at": "2016-01-04T15:41:23.046Z", - "finished_at": null, - "id": 64, + "coverage": null, + "created_at": "2015-12-24T15:51:21.880Z", + "download_url": null, + "finished_at": "2015-12-24T17:54:31.198Z", + "id": 8, "name": "rubocop", "ref": "master", "runner": null, "stage": "test", - "started_at": null, - "status": "pending" + "started_at": "2015-12-24T17:54:30.733Z", + "status": "failed", + "tag": false, + "user": { + "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", + "bio": null, + "can_create_group": true, + "can_create_project": true, + "color_scheme_id": 2, + "created_at": "2015-12-21T13:14:24.077Z", + "current_sign_in_at": "2016-01-11T09:31:40.472Z", + "email": "admin@example.com", + "id": 1, + "identities": [], + "is_admin": true, + "linkedin": "", + "name": "Administrator", + "projects_limit": 100, + "skype": "", + "state": "active", + "theme_id": 3, + "twitter": "", + "two_factor_enabled": false, + "username": "root", + "web_url": "http://gitlab.dev/u/root", + "website_url": "" + } } ``` @@ -154,20 +303,27 @@ Parameters: ```json { "commit": { - "committed_at": "2015-12-28T14:34:03.814Z", - "id": 2, - "ref": null, - "sha": "6b053ad388c531c21907f022933e5e81598db388" + "author_email": "admin@example.com", + "author_name": "Administrator", + "created_at": "2015-12-24T16:51:14.000+01:00", + "id": "0ff3ae198f8601a285adcf5c0fff204ee6fba5fd", + "message": "Test the CI integration.", + "short_id": "0ff3ae19", + "title": "Test the CI integration." }, - "created_at": "2016-01-05T15:33:25.936Z", - "finished_at": "2016-01-05T15:33:47.553Z", - "id": 66, + "coverage": null, + "created_at": "2016-01-11T10:13:33.506Z", + "download_url": null, + "finished_at": "2016-01-11T10:14:09.526Z", + "id": 69, "name": "rubocop", "ref": "master", "runner": null, "stage": "test", "started_at": null, - "status": "canceled" + "status": "canceled", + "tag": false, + "user": null } ``` @@ -187,19 +343,26 @@ Parameters: ```json { "commit": { - "committed_at": "2015-12-28T14:34:03.814Z", - "id": 2, - "ref": null, - "sha": "6b053ad388c531c21907f022933e5e81598db388" + "author_email": "admin@example.com", + "author_name": "Administrator", + "created_at": "2015-12-24T16:51:14.000+01:00", + "id": "0ff3ae198f8601a285adcf5c0fff204ee6fba5fd", + "message": "Test the CI integration.", + "short_id": "0ff3ae19", + "title": "Test the CI integration." }, - "created_at": "2016-01-05T15:33:25.936Z", + "coverage": null, + "created_at": "2016-01-11T10:13:33.506Z", + "download_url": null, "finished_at": null, - "id": 66, + "id": 69, "name": "rubocop", "ref": "master", "runner": null, "stage": "test", "started_at": null, - "status": "pending" + "status": "pending", + "tag": false, + "user": null } ``` diff --git a/lib/api/entities.rb b/lib/api/entities.rb index cb00b392db..a1a886d6fe 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -375,8 +375,9 @@ module API end class Build < Grape::Entity - expose :id, :status, :stage, :name, :ref, :tag, :coverage, :user + expose :id, :status, :stage, :name, :ref, :tag, :coverage expose :created_at, :started_at, :finished_at + expose :user, with: UserFull expose :download_url do |repo_obj, options| if options[:user_can_download_artifacts] repo_obj.download_url From be08490863b76026b8f3ffbc422cb7f5d8b4a6a4 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 11 Jan 2016 14:23:45 +0100 Subject: [PATCH 044/218] #can_unsubscribe? -> #?unsubscribable? --- app/controllers/sent_notifications_controller.rb | 2 +- app/models/sent_notification.rb | 2 +- app/views/layouts/notify.html.haml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/sent_notifications_controller.rb b/app/controllers/sent_notifications_controller.rb index b7008c82bf..7271c933b9 100644 --- a/app/controllers/sent_notifications_controller.rb +++ b/app/controllers/sent_notifications_controller.rb @@ -3,7 +3,7 @@ class SentNotificationsController < ApplicationController def unsubscribe @sent_notification = SentNotification.for(params[:id]) - return render_404 unless @sent_notification && @sent_notification.can_unsubscribe? + return render_404 unless @sent_notification && @sent_notification.unsubscribable? noteable = @sent_notification.noteable noteable.unsubscribe(@sent_notification.recipient) diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb index 03108da17b..77115597d7 100644 --- a/app/models/sent_notification.rb +++ b/app/models/sent_notification.rb @@ -62,7 +62,7 @@ class SentNotification < ActiveRecord::Base end end - def can_unsubscribe? + def unsubscribable? !for_commit? end diff --git a/app/views/layouts/notify.html.haml b/app/views/layouts/notify.html.haml index 07f9be12f9..325c68c69d 100644 --- a/app/views/layouts/notify.html.haml +++ b/app/views/layouts/notify.html.haml @@ -45,7 +45,7 @@ -# Don't link the host is the line below, one link in the email is easier to quickly click than two. You're receiving this email because of your account on #{Gitlab.config.gitlab.host}. If you'd like to receive fewer emails, you can - - if @sent_notification && @sent_notification.can_unsubscribe? + - if @sent_notification && @sent_notification.unsubscribable? = link_to "unsubscribe", unsubscribe_sent_notification_url(@sent_notification) from this thread or adjust your notification settings. From 5dcfd7d06754fecb245475cc005c112677693801 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 11 Jan 2016 15:49:25 +0100 Subject: [PATCH 045/218] Add TODO notice to build trace feature in CI API --- lib/api/builds.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/api/builds.rb b/lib/api/builds.rb index 8c21754596..d3f4e33ebb 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -61,6 +61,10 @@ module API # build_id (required) - The ID of a build # Example Request: # GET /projects/:id/build/:build_id/trace + # + # TODO: We should use `present_file!` and leave this implementation for backward compatibility (when build trace + # is saved in the DB instead of file). But before that, we need to consider how to replace the value of + # `runners_token` with some mask (like `xxxxxx`) when sending trace file directly by workhorse. get ':id/builds/:build_id/trace' do build = get_build(params[:build_id]) return not_found!(build) unless build From 5ba0232f7905a0c2a55ff80cbb97ddf2404fa22c Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 11 Jan 2016 16:30:01 +0100 Subject: [PATCH 046/218] Fix :ci_build_with_trace factory --- spec/factories/ci/builds.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index ce68457f86..558598d4d5 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -58,7 +58,9 @@ FactoryGirl.define do factory :ci_build_with_trace do id 999 - trace 'BUILD TRACE' + after(:build) do |build, evaluator| + build.trace = 'BUILD TRACE' + end end factory :ci_build_canceled do From 470b3a482a06c733665abcf2d92ad4d898b2efe0 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 12 Jan 2016 11:50:14 +0100 Subject: [PATCH 047/218] Fix doc/api/builds.md --- doc/api/builds.md | 84 +++++++++++++++++------------------------------ 1 file changed, 31 insertions(+), 53 deletions(-) diff --git a/doc/api/builds.md b/doc/api/builds.md index 64efe6bb3b..99ef0882c1 100644 --- a/doc/api/builds.md +++ b/doc/api/builds.md @@ -127,11 +127,33 @@ Parameters: - `scope` (optional) - The scope of builds to show (one or array of: pending, running, failed, success, canceled; if none provided showing all builds) ```json - -``` - -## Get a single build -mmit": { +[ + { + "commit": { + "author_email": "admin@example.com", + "author_name": "Administrator", + "created_at": "2015-12-24T16:51:14.000+01:00", + "id": "0ff3ae198f8601a285adcf5c0fff204ee6fba5fd", + "message": "Test the CI integration.", + "short_id": "0ff3ae19", + "title": "Test the CI integration." + }, + "coverage": null, + "created_at": "2016-01-11T10:13:33.506Z", + "download_url": null, + "finished_at": "2016-01-11T10:14:09.526Z", + "id": 69, + "name": "rubocop", + "ref": "master", + "runner": null, + "stage": "test", + "started_at": null, + "status": "canceled", + "tag": false, + "user": null + }, + { + "commit": { "author_email": "admin@example.com", "author_name": "Administrator", "created_at": "2015-12-24T16:51:14.000+01:00", @@ -159,54 +181,7 @@ mmit": { "can_create_project": true, "color_scheme_id": 2, "created_at": "2015-12-21T13:14:24.077Z", - "current_sign_in_at": "2016-01-11T09:31:40.472Z", - "email": "admin@example.com", - "id": 1, - "identities": [], - "is_admin": true, - "linkedin": "", - "name": "Administrator", - "projects_limit": 100, - "skype": "", - "state": "active", - "theme_id": 3, - "twitter": "", - "two_factor_enabled": false, - "username": "root", - "web_url": "http://gitlab.dev/u/root", - "website_url": "" - } - }, - { - "commit": { - "author_email": "admin@example.com", - "author_name": "Administrator", - "created_at": "2015-12-24T16:51:14.000+01:00", - "id": "0ff3ae198f8601a285adcf5c0fff204ee6fba5fd", - "message": "Test the CI integration.", - "short_id": "0ff3ae19", - "title": "Test the CI integration." - }, - "coverage": null, - "created_at": "2015-12-24T15:51:21.880Z", - "download_url": null, - "finished_at": "2015-12-24T17:54:31.198Z", - "id": 8, - "name": "rubocop", - "ref": "master", - "runner": null, - "stage": "test", - "started_at": "2015-12-24T17:54:30.733Z", - "status": "failed", - "tag": false, - "user": { - "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", - "bio": null, - "can_create_group": true, - "can_create_project": true, - "color_scheme_id": 2, - "created_at": "2015-12-21T13:14:24.077Z", - "current_sign_in_at": "2016-01-11T09:31:40.472Z", + "current_sign_in_at": "2016-01-12T10:30:48.315Z", "email": "admin@example.com", "id": 1, "identities": [], @@ -225,6 +200,9 @@ mmit": { } } ] +``` + +## Get a single build Get a single build of a project From ac6a10f3e88c5d2081b8638df63016089517a844 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Tue, 12 Jan 2016 12:29:10 -0200 Subject: [PATCH 048/218] Codestyle changes --- app/models/identity.rb | 2 +- lib/api/users.rb | 6 +++--- spec/models/identity_spec.rb | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/identity.rb b/app/models/identity.rb index 830b99fa3f..e1915b079d 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -19,7 +19,7 @@ class Identity < ActiveRecord::Base validates :extern_uid, allow_blank: true, uniqueness: { scope: :provider } validates :user_id, uniqueness: { scope: :provider } - def is_ldap? + def ldap? provider.starts_with?('ldap') end end diff --git a/lib/api/users.rb b/lib/api/users.rb index 01fd90139b..fd2128bd17 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -303,10 +303,10 @@ module API if !user not_found!('User') - elsif !user.ldap_blocked? - user.activate - else + elsif user.ldap_blocked? forbidden!('LDAP blocked users cannot be unblocked by the API') + else + user.activate end end end diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb index 107bfc1778..5afe042e15 100644 --- a/spec/models/identity_spec.rb +++ b/spec/models/identity_spec.rb @@ -28,11 +28,11 @@ RSpec.describe Identity, models: true do let(:other_identity) { create(:identity, provider: 'twitter') } it 'returns true if it is a ldap identity' do - expect(ldap_identity.is_ldap?).to be_truthy + expect(ldap_identity.ldap?).to be_truthy end it 'returns false if it is not a ldap identity' do - expect(other_identity.is_ldap?).to be_falsey + expect(other_identity.ldap?).to be_falsey end end end From ab2c6cc01ff26e07db15110e037e72159c48dc53 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 12 Jan 2016 18:32:18 +0100 Subject: [PATCH 049/218] Add some fixes --- lib/api/api.rb | 1 - lib/api/entities.rb | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/api/api.rb b/lib/api/api.rb index 266b5f48f8..1a1340c428 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -54,7 +54,6 @@ module API mount Keys mount Tags mount Triggers - mount Builds end end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index a1a886d6fe..e19f9f8d75 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -377,7 +377,7 @@ module API class Build < Grape::Entity expose :id, :status, :stage, :name, :ref, :tag, :coverage expose :created_at, :started_at, :finished_at - expose :user, with: UserFull + expose :user, with: User expose :download_url do |repo_obj, options| if options[:user_can_download_artifacts] repo_obj.download_url From efb3395b4fc0425ebbc2437ad03f0cd5fc851863 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 12 Jan 2016 19:32:44 +0100 Subject: [PATCH 050/218] Remove blank line --- lib/api/api.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/api/api.rb b/lib/api/api.rb index a9e1913f0f..098dd97584 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -54,7 +54,6 @@ module API mount Keys mount Tags mount Triggers - mount Variables end end From 9d7f88c12258e27a189e8229090920db0627e88b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 12 Jan 2016 18:10:06 +0100 Subject: [PATCH 051/218] Show referenced MRs & Issues only when the current viewer can access them --- CHANGELOG | 1 + app/controllers/projects/issues_controller.rb | 2 +- app/models/issue.rb | 4 +- app/views/projects/notes/_notes.html.haml | 1 + features/project/issues/references.feature | 31 +++++ features/steps/project/issues/references.rb | 106 ++++++++++++++++++ features/steps/shared/note.rb | 6 + features/steps/shared/project.rb | 7 ++ 8 files changed, 155 insertions(+), 3 deletions(-) create mode 100644 features/project/issues/references.feature create mode 100644 features/steps/project/issues/references.rb diff --git a/CHANGELOG b/CHANGELOG index ab34661ce0..f765899b42 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -38,6 +38,7 @@ v 8.4.0 (unreleased) - Ajax filter by message for commits page - API: Add support for deleting a tag via the API (Robert Schilling) - Allow subsequent validations in CI Linter + - Show referenced MRs & Issues only when the current viewer can access them v 8.3.3 - Preserve CE behavior with JIRA integration by only calling API if URL is set diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index b59b52291f..f476afb2d9 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -61,7 +61,7 @@ class Projects::IssuesController < Projects::ApplicationController @note = @project.notes.new(noteable: @issue) @notes = @issue.notes.nonawards.with_associations.fresh @noteable = @issue - @merge_requests = @issue.referenced_merge_requests + @merge_requests = @issue.referenced_merge_requests(current_user) respond_with(@issue) end diff --git a/app/models/issue.rb b/app/models/issue.rb index f52e47f3e6..7beba98460 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -85,10 +85,10 @@ class Issue < ActiveRecord::Base reference end - def referenced_merge_requests + def referenced_merge_requests(current_user = nil) Gitlab::ReferenceExtractor.lazily do [self, *notes].flat_map do |note| - note.all_references.merge_requests + note.all_references(current_user).merge_requests end end.sort_by(&:iid) end diff --git a/app/views/projects/notes/_notes.html.haml b/app/views/projects/notes/_notes.html.haml index ca60dd239b..a4ff947c65 100644 --- a/app/views/projects/notes/_notes.html.haml +++ b/app/views/projects/notes/_notes.html.haml @@ -8,4 +8,5 @@ - else - @notes.each do |note| - next unless note.author + - next if note.cross_reference? && note.referenced_mentionables(current_user).empty? = render note diff --git a/features/project/issues/references.feature b/features/project/issues/references.feature new file mode 100644 index 0000000000..bf7a4c6cb9 --- /dev/null +++ b/features/project/issues/references.feature @@ -0,0 +1,31 @@ +@project_issues +Feature: Project Issues References + Background: + Given I sign in as "John Doe" + And "John Doe" owns public project "Community" + And project "Community" has "Public Issue 01" open issue + And I logout + And I sign in as "Mary Jane" + And "Mary Jane" owns private project "Private Library" + And project "Private Library" has "Fix NS-01" open merge request + And project "Private Library" has "Private Issue 01" open issue + And I visit merge request page "Fix NS-01" + And I leave a comment referencing issue "Public Issue 01" from "Fix NS-01" merge request + And I visit issue page "Private Issue 01" + And I leave a comment referencing issue "Public Issue 01" from "Private Issue 01" issue + And I logout + + @javascript + Scenario: Viewing the public issue as a "John Doe" + Given I sign in as "John Doe" + When I visit issue page "Public Issue 01" + Then I should not see any related merge requests + And I should see no notes at all + + @javascript + Scenario: Viewing the public issue as "Mary Jane" + Given I sign in as "Mary Jane" + When I visit issue page "Public Issue 01" + Then I should see the "Fix NS-01" related merge request + And I should see a note linking to "Fix NS-01" merge request + And I should see a note linking to "Private Issue 01" issue diff --git a/features/steps/project/issues/references.rb b/features/steps/project/issues/references.rb new file mode 100644 index 0000000000..9c7725e8c1 --- /dev/null +++ b/features/steps/project/issues/references.rb @@ -0,0 +1,106 @@ +class Spinach::Features::ProjectIssuesReferences < Spinach::FeatureSteps + include SharedAuthentication + include SharedNote + include SharedProject + include SharedUser + + step 'project "Community" has "Public Issue 01" open issue' do + project = Project.find_by(name: 'Community') + create(:issue, + title: 'Public Issue 01', + project: project, + author: project.users.first, + description: '# Description header' + ) + end + + step 'project "Private Library" has "Fix NS-01" open merge request' do + project = Project.find_by(name: 'Private Library') + create(:merge_request, + title: 'Fix NS-01', + source_project: project, + target_project: project, + source_branch: 'fix', + target_branch: 'master', + author: project.users.first, + description: '# Description header' + ) + end + + step 'project "Private Library" has "Private Issue 01" open issue' do + project = Project.find_by(name: 'Private Library') + create(:issue, + title: 'Private Issue 01', + project: project, + author: project.users.first, + description: '# Description header' + ) + end + + step 'I leave a comment referencing issue "Public Issue 01" from "Fix NS-01" merge request' do + project = Project.find_by(name: 'Private Library') + issue = Issue.find_by!(title: 'Public Issue 01') + + page.within(".js-main-target-form") do + fill_in "note[note]", with: "##{issue.to_reference(project)}" + click_button "Add Comment" + end + end + + step 'I leave a comment referencing issue "Public Issue 01" from "Private Issue 01" issue' do + project = Project.find_by(name: 'Private Library') + issue = Issue.find_by!(title: 'Public Issue 01') + + page.within(".js-main-target-form") do + fill_in "note[note]", with: "##{issue.to_reference(project)}" + click_button "Add Comment" + end + end + + step 'I visit merge request page "Fix NS-01"' do + mr = MergeRequest.find_by(title: "Fix NS-01") + visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr) + end + + step 'I visit issue page "Private Issue 01"' do + issue = Issue.find_by(title: "Private Issue 01") + visit namespace_project_issue_path(issue.project.namespace, issue.project, issue) + end + + step 'I visit issue page "Public Issue 01"' do + issue = Issue.find_by(title: "Public Issue 01") + visit namespace_project_issue_path(issue.project.namespace, issue.project, issue) + end + + step 'I should not see any related merge requests' do + page.within '.issue-details' do + expect(page).not_to have_content('.merge-requests') + end + end + + step 'I should see the "Fix NS-01" related merge request' do + page.within '.merge-requests' do + expect(page).to have_content("1 Related Merge Request") + expect(page).to have_content('Fix NS-01') + end + end + + step 'I should see a note linking to "Fix NS-01" merge request' do + project = Project.find_by(name: 'Community') + mr = MergeRequest.find_by(title: 'Fix NS-01') + page.within('.notes') do + expect(page).to have_content('Mary Jane') + expect(page).to have_content("mentioned in merge request #{mr.to_reference(project)}") + end + end + + step 'I should see a note linking to "Private Issue 01" issue' do + project = Project.find_by(name: 'Community') + issue = Issue.find_by(title: 'Private Issue 01') + page.within('.notes') do + expect(page).to have_content('Mary Jane') + expect(page).to have_content("mentioned in issue #{issue.to_reference(project)}") + end + end + +end diff --git a/features/steps/shared/note.rb b/features/steps/shared/note.rb index f6aabfefef..6de58c6e2b 100644 --- a/features/steps/shared/note.rb +++ b/features/steps/shared/note.rb @@ -106,6 +106,12 @@ module SharedNote end end + step 'I should see no notes at all' do + page.within('.notes') do + expect(page).to_not have_css('.note') + end + end + # Markdown step 'I leave a comment with a header containing "Comment with a header"' do diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb index da643bf3ba..43a15f4347 100644 --- a/features/steps/shared/project.rb +++ b/features/steps/shared/project.rb @@ -181,6 +181,13 @@ module SharedProject project.team << [user, :master] end + step '"Mary Jane" owns private project "Private Library"' do + user = user_exists('Mary Jane', username: 'mary_jane') + project = Project.find_by(name: 'Private Library') + project ||= create(:project, name: 'Private Library', namespace: user.namespace) + project.team << [user, :master] + end + step 'public empty project "Empty Public Project"' do create :project_empty_repo, :public, name: "Empty Public Project" end From d44653da1f74c2c15fe7ec3f8aa9b16563ffebd6 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 13 Jan 2016 12:16:27 +0100 Subject: [PATCH 052/218] Add some fixes after review --- app/models/ci/trigger.rb | 4 ++++ lib/api/entities.rb | 8 +------- lib/api/triggers.rb | 11 +++++------ spec/requests/api/triggers_spec.rb | 12 ++++++------ 4 files changed, 16 insertions(+), 19 deletions(-) diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index 23516709a4..3328459c4c 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -32,6 +32,10 @@ module Ci trigger_requests.last end + def last_used + last_trigger_request.try(:created_at) + end + def short_token token[0...10] end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 37c483b45e..1108277aab 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -367,13 +367,7 @@ module API end class Trigger < Grape::Entity - expose :token, :created_at, :updated_at, :deleted_at - expose :last_used do |repo_obj, _options| - if repo_obj.respond_to?(:last_trigger_request) - request = repo_obj.last_trigger_request - request.created_at if request - end - end + expose :token, :created_at, :updated_at, :deleted_at, :last_used end end end diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 25bb8aef20..5e4964f446 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -73,10 +73,10 @@ module API authenticate! authorize_admin_project - triggers = user_project.triggers.where(token: params[:token]) - return not_found!('Trigger') if triggers.empty? + trigger = user_project.triggers.find_by(token: params[:token].to_s) + return not_found!('Trigger') unless trigger - present triggers.first, with: Entities::Trigger + present trigger, with: Entities::Trigger end # Create trigger @@ -89,8 +89,7 @@ module API authenticate! authorize_admin_project - trigger = user_project.triggers.new - trigger.save + trigger = user_project.triggers.create present trigger, with: Entities::Trigger end @@ -106,7 +105,7 @@ module API authenticate! authorize_admin_project - trigger = user_project.triggers.where(token: params[:token]).first + trigger = user_project.triggers.find_by(token: params[:token].to_s) return not_found!('Trigger') unless trigger trigger.destroy diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index e8d89426ec..2a86b60bc4 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -105,7 +105,7 @@ describe API::API do end end - context 'unauthentikated user' do + context 'unauthenticated user' do it 'should not return triggers list' do get api("/projects/#{project.id}/triggers") @@ -123,7 +123,7 @@ describe API::API do expect(json_response).to be_a(Hash) end - it 'should responde with 404 Not Found if requesting non-existing trigger' do + it 'should respond with 404 Not Found if requesting non-existing trigger' do get api("/projects/#{project.id}/triggers/abcdef012345", user) expect(response.status).to eq(404) @@ -138,7 +138,7 @@ describe API::API do end end - context 'unauthentikated user' do + context 'unauthenticated user' do it 'should not return triggers list' do get api("/projects/#{project.id}/triggers/#{trigger.token}") @@ -167,7 +167,7 @@ describe API::API do end end - context 'unauthentikated user' do + context 'unauthenticated user' do it 'should not create trigger' do post api("/projects/#{project.id}/triggers") @@ -185,7 +185,7 @@ describe API::API do expect(response.status).to eq(200) end - it 'should responde with 404 Not Found if requesting non-existing trigger' do + it 'should respond with 404 Not Found if requesting non-existing trigger' do delete api("/projects/#{project.id}/triggers/abcdef012345", user) expect(response.status).to eq(404) @@ -200,7 +200,7 @@ describe API::API do end end - context 'unauthentikated user' do + context 'unauthenticated user' do it 'should not delete trigger' do delete api("/projects/#{project.id}/triggers/#{trigger.token}") From df548285804fdc40ac7c4f36601e87a534792a4a Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 13 Jan 2016 12:47:11 +0100 Subject: [PATCH 053/218] Add some fixes after review --- lib/api/variables.rb | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/lib/api/variables.rb b/lib/api/variables.rb index cc038e5731..0c3fb5c8a7 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -27,11 +27,11 @@ module API # GET /projects/:id/variables/:key get ':id/variables/:key' do key = params[:key] - variables = user_project.variables.where(key: key) + variable = user_project.variables.find_by(key: key.to_s) - return not_found!('Variable') if variables.empty? + return not_found!('Variable') unless variable - present variables.first, with: Entities::Variable + present variable, with: Entities::Variable end # Create a new variable in project @@ -46,10 +46,12 @@ module API required_attributes! [:key, :value] variable = user_project.variables.create(key: params[:key], value: params[:value]) - return render_validation_error!(variable) unless variable.valid? - variable.save! - present variable, with: Entities::Variable + if variable.valid? + present variable, with: Entities::Variable + else + render_validation_error!(variable) + end end # Update existing variable of a project @@ -61,14 +63,16 @@ module API # Example Request: # PUT /projects/:id/variables/:key put ':id/variables/:key' do - variable = user_project.variables.where(key: params[:key]).first + variable = user_project.variables.find_by(key: params[:key].to_s) return not_found!('Variable') unless variable - variable.value = params[:value] - variable.save! - - present variable, with: Entities::Variable + attrs = attributes_for_keys [:value] + if variable.update(attrs) + present variable, with: Entities::Variable + else + render_validation_error!(variable) + end end # Delete existing variable of a project @@ -79,10 +83,9 @@ module API # Exanoke Reqyest: # DELETE /projects/:id/variables/:key delete ':id/variables/:key' do - variable = user_project.variables.where(key: params[:key]).first + variable = user_project.variables.find_by(key: params[:key].to_s) return not_found!('Variable') unless variable - variable.destroy present variable, with: Entities::Variable From b433efcf70dbafd5f76ce7140a4e6eb36008eee5 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 13 Jan 2016 14:27:36 +0100 Subject: [PATCH 054/218] Add suport for layout without container Signed-off-by: Dmitriy Zaporozhets --- app/views/layouts/_page.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/layouts/_page.html.haml b/app/views/layouts/_page.html.haml index ec7cd79bc5..2615998977 100644 --- a/app/views/layouts/_page.html.haml +++ b/app/views/layouts/_page.html.haml @@ -24,7 +24,7 @@ .content-wrapper = render "layouts/flash" = yield :flash_message - %div{ class: container_class } + %div{ class: (container_class unless @no_container) } .content .clearfix = yield From 3b3f755b5bfa5348375fffe22ec56fe0f01ac1f5 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 13 Jan 2016 14:37:25 +0100 Subject: [PATCH 055/218] New layout without background color Signed-off-by: Dmitriy Zaporozhets --- app/assets/javascripts/activities.js.coffee | 4 +-- app/assets/stylesheets/framework/blocks.scss | 12 ++++---- app/assets/stylesheets/framework/files.scss | 2 -- app/assets/stylesheets/framework/flash.scss | 2 ++ app/assets/stylesheets/framework/header.scss | 1 + app/assets/stylesheets/framework/layout.scss | 2 -- app/assets/stylesheets/framework/lists.scss | 6 ++-- app/assets/stylesheets/framework/sidebar.scss | 1 - app/assets/stylesheets/framework/tables.scss | 6 ++-- .../stylesheets/framework/timeline.scss | 4 +-- app/assets/stylesheets/pages/detail_page.scss | 5 ++-- app/assets/stylesheets/pages/events.scss | 4 +-- app/assets/stylesheets/pages/projects.scss | 8 ++--- app/helpers/events_helper.rb | 8 +++-- app/views/groups/show.html.haml | 30 +++++++++++-------- app/views/projects/_activity.html.haml | 2 +- app/views/projects/show.html.haml | 24 ++++++++------- app/views/shared/_event_filter.html.haml | 2 +- 18 files changed, 63 insertions(+), 60 deletions(-) diff --git a/app/assets/javascripts/activities.js.coffee b/app/assets/javascripts/activities.js.coffee index 6380374741..8b8e5d48aa 100644 --- a/app/assets/javascripts/activities.js.coffee +++ b/app/assets/javascripts/activities.js.coffee @@ -1,7 +1,7 @@ class @Activities constructor: -> Pager.init 20, true - $(".event-filter .btn").bind "click", (event) => + $(".event-filter a").bind "click", (event) => event.preventDefault() @toggleFilter($(event.currentTarget)) @reloadActivities() @@ -12,7 +12,7 @@ class @Activities toggleFilter: (sender) -> - sender.toggleClass "active" + sender.parent().toggleClass "active" event_filters = $.cookie("event_filter") filter = sender.attr("id").split("_")[0] if event_filters diff --git a/app/assets/stylesheets/framework/blocks.scss b/app/assets/stylesheets/framework/blocks.scss index fa0e70847f..11df5a377a 100644 --- a/app/assets/stylesheets/framework/blocks.scss +++ b/app/assets/stylesheets/framework/blocks.scss @@ -20,7 +20,8 @@ .content-block, .gray-content-block { - margin: -$gl-padding; + margin-top: -$gl-padding; + margin-bottom: -$gl-padding; background-color: $background-color; padding: $gl-padding; margin-bottom: 0px; @@ -86,10 +87,7 @@ .cover-block { text-align: center; background: $background-color; - margin: -$gl-padding; - margin-bottom: 0; - padding: 44px $gl-padding; - border-bottom: 1px solid $border-color; + padding-top: 44px; position: relative; .avatar-holder { @@ -131,6 +129,10 @@ right: auto; } } + + .cover-menu { + margin: 0; + } } .block-connector { diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss index cbfd4bc29b..f9058c8c30 100644 --- a/app/assets/stylesheets/framework/files.scss +++ b/app/assets/stylesheets/framework/files.scss @@ -3,8 +3,6 @@ * */ .file-holder { - margin-left: -$gl-padding; - margin-right: -$gl-padding; border: none; border-top: 1px solid #E7E9EE; border-bottom: 1px solid #E7E9EE; diff --git a/app/assets/stylesheets/framework/flash.scss b/app/assets/stylesheets/framework/flash.scss index 82eb50ad4b..1bfd021399 100644 --- a/app/assets/stylesheets/framework/flash.scss +++ b/app/assets/stylesheets/framework/flash.scss @@ -8,10 +8,12 @@ .flash-notice { @extend .alert; @extend .alert-info; + margin: 0; } .flash-alert { @extend .alert; @extend .alert-danger; + margin: 0; } } diff --git a/app/assets/stylesheets/framework/header.scss b/app/assets/stylesheets/framework/header.scss index 4dbbb56104..ba5e72c8c5 100644 --- a/app/assets/stylesheets/framework/header.scss +++ b/app/assets/stylesheets/framework/header.scss @@ -28,6 +28,7 @@ header { min-height: $header-height; background-color: #fff; border: none; + border-bottom: 1px solid #EEE; .container-fluid { width: 100% !important; diff --git a/app/assets/stylesheets/framework/layout.scss b/app/assets/stylesheets/framework/layout.scss index a1a9990241..e901c78d02 100644 --- a/app/assets/stylesheets/framework/layout.scss +++ b/app/assets/stylesheets/framework/layout.scss @@ -5,8 +5,6 @@ html { } body { - background-color: #F3F3F3 !important; - &.navless { background-color: white !important; } diff --git a/app/assets/stylesheets/framework/lists.scss b/app/assets/stylesheets/framework/lists.scss index bbdb1c038c..a5e1afd602 100644 --- a/app/assets/stylesheets/framework/lists.scss +++ b/app/assets/stylesheets/framework/lists.scss @@ -109,10 +109,8 @@ ul.content-list { padding: 0; > li { - padding: $gl-padding; + padding: $gl-padding 0; border-color: $table-border-color; - margin-left: -$gl-padding; - margin-right: -$gl-padding; color: $gl-gray; .avatar { @@ -148,7 +146,7 @@ ul.controls { > li { float: left; margin-right: 10px; - + &:last-child { margin-right: 0; } diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 83243dd245..06ce52e26d 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -21,7 +21,6 @@ .content-wrapper { width: 100%; - padding: 20px; .container-fluid { background: #FFF; diff --git a/app/assets/stylesheets/framework/tables.scss b/app/assets/stylesheets/framework/tables.scss index 793ab3d9bb..8ae235520d 100644 --- a/app/assets/stylesheets/framework/tables.scss +++ b/app/assets/stylesheets/framework/tables.scss @@ -1,13 +1,11 @@ .table-holder { - margin: -$gl-padding; - margin-top: 0; - margin-bottom: 0; + margin: 0; } table { &.table { margin-bottom: $gl-padding; - + .dropdown-menu a { text-decoration: none; } diff --git a/app/assets/stylesheets/framework/timeline.scss b/app/assets/stylesheets/framework/timeline.scss index ff41e26ed8..47b843e5e3 100644 --- a/app/assets/stylesheets/framework/timeline.scss +++ b/app/assets/stylesheets/framework/timeline.scss @@ -5,10 +5,8 @@ padding: 0; .timeline-entry { - padding: $gl-padding; + padding: $gl-padding 0; border-color: $table-border-color; - margin-left: -$gl-padding; - margin-right: -$gl-padding; color: $gl-gray; border-bottom: 1px solid $border-white-light; diff --git a/app/assets/stylesheets/pages/detail_page.scss b/app/assets/stylesheets/pages/detail_page.scss index deab805dbc..6844d5b421 100644 --- a/app/assets/stylesheets/pages/detail_page.scss +++ b/app/assets/stylesheets/pages/detail_page.scss @@ -1,6 +1,7 @@ .detail-page-header { - margin: -$gl-padding; - padding: 7px $gl-padding; + margin-top: -$gl-padding; + margin-bottom: -$gl-padding; + padding: 7px 0; margin-bottom: 0px; border-bottom: 1px solid $border-color; color: #5c5d5e; diff --git a/app/assets/stylesheets/pages/events.scss b/app/assets/stylesheets/pages/events.scss index 984b4b9121..8fa15b3574 100644 --- a/app/assets/stylesheets/pages/events.scss +++ b/app/assets/stylesheets/pages/events.scss @@ -4,9 +4,7 @@ */ .event-item { font-size: $gl-font-size; - padding: $gl-padding $gl-padding $gl-padding ($gl-padding + $gl-avatar-size + 15px); - margin-left: -$gl-padding; - margin-right: -$gl-padding; + padding: $gl-padding 0 $gl-padding ($gl-avatar-size + 15px); border-bottom: 1px solid $table-border-color; color: #7f8fa4; diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index f24b71963a..a8bf63539b 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -26,6 +26,8 @@ } .project-home-panel { + padding-bottom: 40px; + border-bottom: 1px solid $border-color; .cover-controls { .project-settings-dropdown { @@ -416,8 +418,6 @@ ul.nav.nav-projects-tabs { .top-area { border-bottom: 1px solid #EEE; - margin: 0 -16px; - padding: 0 $gl-padding; height: 42px; ul.left-top-menu { @@ -574,10 +574,8 @@ pre.light-well { @include basic-list; .project-row { - padding: $gl-padding; + padding: $gl-padding 0; border-color: $table-border-color; - margin-left: -$gl-padding; - margin-right: -$gl-padding; &.no-description { .project { diff --git a/app/helpers/events_helper.rb b/app/helpers/events_helper.rb index dde83ff36b..31bf45baeb 100644 --- a/app/helpers/events_helper.rb +++ b/app/helpers/events_helper.rb @@ -27,13 +27,15 @@ module EventsHelper key = key.to_s active = 'active' if @event_filter.active?(key) link_opts = { - class: "event-filter-link btn btn-default #{active}", + class: "event-filter-link", id: "#{key}_event_filter", title: "Filter by #{tooltip.downcase}", } - link_to request.path, link_opts do - content_tag(:span, ' ' + tooltip) + content_tag :li, class: active do + link_to request.path, link_opts do + content_tag(:span, ' ' + tooltip) + end end end diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 48a544fc83..3471948198 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -5,6 +5,9 @@ - if current_user = auto_discovery_link_tag(:atom, group_url(@group, format: :atom, private_token: current_user.private_token), title: "#{@group.name} activity") +- @no_container = true +- @blank_container = true + .cover-block .cover-controls - if @group && can?(current_user, :admin_group, @group) @@ -25,8 +28,8 @@ .cover-desc.description = markdown(@group.description, pipeline: :description) -- if can?(current_user, :read_group, @group) - %ul.center-top-menu.no-top + + %ul.center-top-menu.cover-menu %li.active = link_to "#activity", 'data-toggle' => 'tab' do Activity @@ -35,19 +38,22 @@ = link_to "#projects", 'data-toggle' => 'tab' do Projects - .tab-content - .tab-pane.active#activity - .gray-content-block.activity-filter-block - - if current_user - = render "events/event_last_push", event: @last_push +- if can?(current_user, :read_group, @group) - = render 'shared/event_filter' + %div{ class: container_class } + .tab-content + .tab-pane.active#activity + .activity-filter-block + - if current_user + = render "events/event_last_push", event: @last_push - .content_list - = spinner + = render 'shared/event_filter' - .tab-pane#projects - = render "projects", projects: @projects + .content_list + = spinner + + .tab-pane#projects + = render "projects", projects: @projects - else %p.center-top-menu.no-top diff --git a/app/views/projects/_activity.html.haml b/app/views/projects/_activity.html.haml index 101880bd10..6edc94e7a9 100644 --- a/app/views/projects/_activity.html.haml +++ b/app/views/projects/_activity.html.haml @@ -1,4 +1,4 @@ -.gray-content-block.activity-filter-block +.activity-filter-block - if current_user .pull-right = link_to namespace_project_path(@project.namespace, @project, format: :atom, private_token: current_user.private_token), title: "Feed", class: 'btn rss-btn' do diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index 8436be433b..25dcedf2b0 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -7,8 +7,10 @@ = render 'shared/no_ssh' = render 'shared/no_password' -= render 'projects/last_push' +- @no_container = true +- @blank_container = true += render 'projects/last_push' = render "home_panel" .project-stats.gray-content-block.second-block @@ -57,15 +59,17 @@ = link_to add_contribution_guide_path(@project) do Add Contribution guide -- if @project.archived? - .text-warning.center.prepend-top-20 - %p - = icon("exclamation-triangle fw") - Archived project! Repository is read-only - - if @repository.commit .content-block.second-block.white - = render 'projects/last_commit', commit: @repository.commit, project: @project + %div{ class: container_class } + = render 'projects/last_commit', commit: @repository.commit, project: @project -%div{class: "project-show-#{default_project_view}"} - = render default_project_view +%div{ class: container_class } + - if @project.archived? + .text-warning.center.prepend-top-20 + %p + = icon("exclamation-triangle fw") + Archived project! Repository is read-only + + %div{class: "project-show-#{default_project_view}"} + = render default_project_view diff --git a/app/views/shared/_event_filter.html.haml b/app/views/shared/_event_filter.html.haml index 8495774acc..4426096f74 100644 --- a/app/views/shared/_event_filter.html.haml +++ b/app/views/shared/_event_filter.html.haml @@ -1,4 +1,4 @@ -.btn-group.btn-group-next.event-filter +%ul.left-top-menu.event-filter.no-top = event_filter_link EventFilter.push, 'Push events' = event_filter_link EventFilter.merged, 'Merge events' = event_filter_link EventFilter.comments, 'Comments' From 8c3debb18aae2cda444d25468315bdf340cb3e9e Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 13 Jan 2016 15:03:32 +0100 Subject: [PATCH 056/218] Start moving all navigation to one class Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/framework.scss | 1 + app/assets/stylesheets/framework/blocks.scss | 4 -- app/assets/stylesheets/framework/buttons.scss | 27 -------- app/assets/stylesheets/framework/common.scss | 69 ------------------- app/assets/stylesheets/framework/mixins.scss | 35 ---------- app/assets/stylesheets/framework/nav.scss | 38 ++++++++++ app/views/groups/show.html.haml | 2 +- app/views/shared/_event_filter.html.haml | 2 +- 8 files changed, 41 insertions(+), 137 deletions(-) create mode 100644 app/assets/stylesheets/framework/nav.scss diff --git a/app/assets/stylesheets/framework.scss b/app/assets/stylesheets/framework.scss index 48a4971c8f..fa7641b167 100644 --- a/app/assets/stylesheets/framework.scss +++ b/app/assets/stylesheets/framework.scss @@ -24,6 +24,7 @@ @import "framework/lists.scss"; @import "framework/markdown_area.scss"; @import "framework/mobile.scss"; +@import "framework/nav.scss"; @import "framework/pagination.scss"; @import "framework/panels.scss"; @import "framework/selects.scss"; diff --git a/app/assets/stylesheets/framework/blocks.scss b/app/assets/stylesheets/framework/blocks.scss index 11df5a377a..7abc5955fb 100644 --- a/app/assets/stylesheets/framework/blocks.scss +++ b/app/assets/stylesheets/framework/blocks.scss @@ -129,10 +129,6 @@ right: auto; } } - - .cover-menu { - margin: 0; - } } .block-connector { diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index 97a9463884..8ee2a9c988 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -153,33 +153,6 @@ } } -.btn-group-next { - .btn { - padding: 9px 0px; - font-size: 15px; - color: #7f8fa4; - border-color: #e7e9ed; - width: 140px; - - .badge { - font-weight: normal; - background-color: #eee; - color: #78a; - } - - &.active { - border-color: $gl-info; - background: $gl-info; - color: #fff; - - .badge { - color: $gl-info; - background-color: white; - } - } - } -} - .btn-clipboard { border: none; } diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss index 11730000f8..0564511626 100644 --- a/app/assets/stylesheets/framework/common.scss +++ b/app/assets/stylesheets/framework/common.scss @@ -374,75 +374,6 @@ table { } } -.center-top-menu, .left-top-menu { - @include nav-menu; - text-align: center; - margin-top: 5px; - margin-bottom: $gl-padding; - height: auto; - margin-top: -$gl-padding; - - &.no-bottom { - margin-bottom: 0; - } - - &.no-top { - margin-top: 0; - } - - li a { - display: inline-block; - padding-top: $gl-padding; - padding-bottom: 11px; - margin-bottom: -1px; - } - - &.bottom-border { - border-bottom: 1px solid $border-color; - height: 57px; - } - - &.wide { - margin-left: -$gl-padding; - margin-right: -$gl-padding; - } -} - -.left-top-menu { - text-align: left; - border-bottom: 1px solid #EEE; -} - -.center-middle-menu { - @include nav-menu; - padding: 0; - text-align: center; - margin: -$gl-padding; - margin-top: 0; - margin-bottom: 0; - height: 58px; - border-bottom: 1px solid $border-color; - - li { - &:after { - content: "|"; - color: $border-gray-light; - } - - &:last-child { - &:after { - content: none; - } - } - - > a { - display: inline-block; - text-transform: uppercase; - font-size: 13px; - } - } -} - .dropzone .dz-preview .dz-progress { border-color: $border-color !important; } diff --git a/app/assets/stylesheets/framework/mixins.scss b/app/assets/stylesheets/framework/mixins.scss index 41fd890f14..1d5000fe38 100644 --- a/app/assets/stylesheets/framework/mixins.scss +++ b/app/assets/stylesheets/framework/mixins.scss @@ -118,38 +118,3 @@ font-size: 16px; line-height: 24px; } - -@mixin nav-menu { - padding: 0; - margin: 0; - list-style: none; - height: 56px; - - li { - display: inline-block; - - a { - padding: 14px; - font-size: 15px; - line-height: 28px; - color: #959494; - border-bottom: 2px solid transparent; - - &:hover, &:active, &:focus { - text-decoration: none; - outline: none; - } - } - - &.active a { - color: #616060; - border-bottom: 2px solid #4688f1; - } - - .badge { - font-weight: normal; - background-color: #eee; - color: #78a; - } - } -} diff --git a/app/assets/stylesheets/framework/nav.scss b/app/assets/stylesheets/framework/nav.scss new file mode 100644 index 0000000000..ffa9127898 --- /dev/null +++ b/app/assets/stylesheets/framework/nav.scss @@ -0,0 +1,38 @@ +.nav-links { + padding: 0; + margin: 0; + list-style: none; + height: 56px; + border-bottom: 1px solid $border-color; + + li { + display: inline-block; + + a { + display: inline-block; + padding-top: $gl-padding; + padding-bottom: 11px; + margin-bottom: -1px; + font-size: 15px; + line-height: 28px; + color: #959494; + border-bottom: 2px solid transparent; + + &:hover, &:active, &:focus { + text-decoration: none; + outline: none; + } + } + + &.active a { + color: #616060; + border-bottom: 2px solid #4688f1; + } + + .badge { + font-weight: normal; + background-color: #eee; + color: #78a; + } + } +} diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 3471948198..24d2f000b4 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -29,7 +29,7 @@ = markdown(@group.description, pipeline: :description) - %ul.center-top-menu.cover-menu + %ul.nav-links %li.active = link_to "#activity", 'data-toggle' => 'tab' do Activity diff --git a/app/views/shared/_event_filter.html.haml b/app/views/shared/_event_filter.html.haml index 4426096f74..c38d9313db 100644 --- a/app/views/shared/_event_filter.html.haml +++ b/app/views/shared/_event_filter.html.haml @@ -1,4 +1,4 @@ -%ul.left-top-menu.event-filter.no-top +%ul.nav-links.event-filter = event_filter_link EventFilter.push, 'Push events' = event_filter_link EventFilter.merged, 'Merge events' = event_filter_link EventFilter.comments, 'Comments' From 97338496188add9ec8d192c7e78f6a6040befffa Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 13 Jan 2016 15:17:59 +0100 Subject: [PATCH 057/218] Add some fixes after review --- doc/api/builds.md | 36 -------------------------------- lib/api/builds.rb | 24 +++++++++++---------- lib/api/entities.rb | 6 ++---- spec/requests/api/builds_spec.rb | 6 ++++++ 4 files changed, 21 insertions(+), 51 deletions(-) diff --git a/doc/api/builds.md b/doc/api/builds.md index 99ef0882c1..06ff89bea0 100644 --- a/doc/api/builds.md +++ b/doc/api/builds.md @@ -40,23 +40,14 @@ Parameters: "user": { "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", "bio": null, - "can_create_group": true, - "can_create_project": true, - "color_scheme_id": 2, "created_at": "2015-12-21T13:14:24.077Z", - "current_sign_in_at": "2016-01-11T09:31:40.472Z", - "email": "admin@example.com", "id": 1, - "identities": [], "is_admin": true, "linkedin": "", "name": "Administrator", - "projects_limit": 100, "skype": "", "state": "active", - "theme_id": 3, "twitter": "", - "two_factor_enabled": false, "username": "root", "web_url": "http://gitlab.dev/u/root", "website_url": "" @@ -87,23 +78,14 @@ Parameters: "user": { "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", "bio": null, - "can_create_group": true, - "can_create_project": true, - "color_scheme_id": 2, "created_at": "2015-12-21T13:14:24.077Z", - "current_sign_in_at": "2016-01-11T09:31:40.472Z", - "email": "admin@example.com", "id": 1, - "identities": [], "is_admin": true, "linkedin": "", "name": "Administrator", - "projects_limit": 100, "skype": "", "state": "active", - "theme_id": 3, "twitter": "", - "two_factor_enabled": false, "username": "root", "web_url": "http://gitlab.dev/u/root", "website_url": "" @@ -177,23 +159,14 @@ Parameters: "user": { "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", "bio": null, - "can_create_group": true, - "can_create_project": true, - "color_scheme_id": 2, "created_at": "2015-12-21T13:14:24.077Z", - "current_sign_in_at": "2016-01-12T10:30:48.315Z", - "email": "admin@example.com", "id": 1, - "identities": [], "is_admin": true, "linkedin": "", "name": "Administrator", - "projects_limit": 100, "skype": "", "state": "active", - "theme_id": 3, "twitter": "", - "two_factor_enabled": false, "username": "root", "web_url": "http://gitlab.dev/u/root", "website_url": "" @@ -241,23 +214,14 @@ Parameters: "user": { "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", "bio": null, - "can_create_group": true, - "can_create_project": true, - "color_scheme_id": 2, "created_at": "2015-12-21T13:14:24.077Z", - "current_sign_in_at": "2016-01-11T09:31:40.472Z", - "email": "admin@example.com", "id": 1, - "identities": [], "is_admin": true, "linkedin": "", "name": "Administrator", - "projects_limit": 100, "skype": "", "state": "active", - "theme_id": 3, "twitter": "", - "two_factor_enabled": false, "username": "root", "web_url": "http://gitlab.dev/u/root", "website_url": "" diff --git a/lib/api/builds.rb b/lib/api/builds.rb index d3f4e33ebb..1337e1bb45 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -35,6 +35,7 @@ module API builds = commit.builds.order('id DESC') builds = filter_builds(builds, params[:scope]) + present paginate(builds), with: Entities::Build, user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project) end @@ -118,25 +119,26 @@ module API helpers do def get_build(id) - user_project.builds.where(id: id).first + user_project.builds.find_by(id: id.to_i) end def filter_builds(builds, scope) - available_scopes = Ci::Build.available_statuses + return builds if scope.nil? || scope.empty? + + available_statuses = Ci::Build.available_statuses scope = - if scope.is_a?(String) || scope.is_a?(Symbol) - available_scopes & [scope.to_s] - elsif scope.is_a?(Array) - available_scopes & scope - elsif scope.respond_to?(:to_h) - available_scopes & scope.to_h.values + if scope.is_a?(String) + [scope] + elsif scope.is_a?(Hashie::Mash) + scope.values else - [] + ['unknown'] end - return builds if scope.empty? + unknown = scope - available_statuses + render_api_error!('Scope contains invalid value(s)', 400) unless unknown.empty? - builds.where(status: scope) + builds.where(status: available_statuses && scope) end def authorize_manage_builds! diff --git a/lib/api/entities.rb b/lib/api/entities.rb index e19f9f8d75..f0816a4652 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -378,18 +378,16 @@ module API expose :id, :status, :stage, :name, :ref, :tag, :coverage expose :created_at, :started_at, :finished_at expose :user, with: User + # TODO: download_url in Ci:Build model is an GitLab Web Interface URL, not API URL. We should think on some API + # for downloading of artifacts (see: https://gitlab.com/gitlab-org/gitlab-ce/issues/4255) expose :download_url do |repo_obj, options| if options[:user_can_download_artifacts] repo_obj.download_url - else - nil end end expose :commit, with: RepoCommit do |repo_obj, _options| if repo_obj.respond_to?(:commit) repo_obj.commit.commit_data - else - nil end end expose :runner, with: Runner diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb index 587fb74750..4bf3d2681d 100644 --- a/spec/requests/api/builds_spec.rb +++ b/spec/requests/api/builds_spec.rb @@ -35,6 +35,12 @@ describe API::API, api: true do expect(response.status).to eq(200) expect(json_response).to be_an Array end + + it 'should respond 400 when scope contains invalid state' do + get api("/projects/#{project.id}/builds?scope[0]=pending&scope[1]=unknown_status", user) + + expect(response.status).to eq(400) + end end context 'unauthorized user' do From 03090a88d80cbc8c4333053081882915beb721da Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 13 Jan 2016 15:58:04 +0100 Subject: [PATCH 058/218] Replace all navigation menu with nav-links class Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/framework/blocks.scss | 12 +++++- app/assets/stylesheets/framework/mobile.scss | 2 +- app/assets/stylesheets/framework/nav.scss | 5 ++- app/assets/stylesheets/framework/sidebar.scss | 2 +- app/assets/stylesheets/pages/detail_page.scss | 3 -- app/assets/stylesheets/pages/diff.scss | 2 - app/assets/stylesheets/pages/issuable.scss | 2 +- .../stylesheets/pages/merge_requests.scss | 4 +- app/assets/stylesheets/pages/projects.scss | 9 +--- app/views/admin/builds/index.html.haml | 2 +- app/views/dashboard/_activities.html.haml | 4 +- app/views/dashboard/_activity_head.html.haml | 2 +- app/views/dashboard/_groups_head.html.haml | 2 +- app/views/dashboard/_projects_head.html.haml | 2 +- app/views/dashboard/_snippets_head.html.haml | 2 +- app/views/dashboard/milestones/show.html.haml | 2 +- app/views/dashboard/snippets/index.html.haml | 42 ++++++++++--------- app/views/groups/milestones/show.html.haml | 2 +- app/views/groups/show.html.haml | 2 +- app/views/help/ui.html.haml | 4 +- app/views/projects/_activity.html.haml | 4 +- app/views/projects/_md_preview.html.haml | 2 +- app/views/projects/blob/edit.html.haml | 2 +- app/views/projects/builds/index.html.haml | 2 +- app/views/projects/builds/show.html.haml | 2 +- app/views/projects/commit/_ci_menu.html.haml | 2 +- app/views/projects/commit/show.html.haml | 4 +- app/views/projects/commits/_head.html.haml | 2 +- app/views/projects/graphs/_head.html.haml | 2 +- .../merge_requests/_new_submit.html.haml | 2 +- .../projects/merge_requests/_show.html.haml | 2 +- app/views/projects/milestones/show.html.haml | 2 +- app/views/projects/wikis/_nav.html.haml | 2 +- app/views/shared/_milestones_filter.html.haml | 2 +- app/views/shared/issuable/_filter.html.haml | 2 +- app/views/sherlock/queries/show.html.haml | 2 +- .../sherlock/transactions/show.html.haml | 2 +- app/views/users/show.html.haml | 2 +- 38 files changed, 77 insertions(+), 70 deletions(-) diff --git a/app/assets/stylesheets/framework/blocks.scss b/app/assets/stylesheets/framework/blocks.scss index 7abc5955fb..7c139d980d 100644 --- a/app/assets/stylesheets/framework/blocks.scss +++ b/app/assets/stylesheets/framework/blocks.scss @@ -20,7 +20,7 @@ .content-block, .gray-content-block { - margin-top: -$gl-padding; + margin-top: 0; margin-bottom: -$gl-padding; background-color: $background-color; padding: $gl-padding; @@ -134,3 +134,13 @@ .block-connector { margin-top: -1px; } + +.nav-block { + .controls { + float: right; + + .btn { + margin-top: 7px; + } + } +} diff --git a/app/assets/stylesheets/framework/mobile.scss b/app/assets/stylesheets/framework/mobile.scss index c00709fb6b..fdb4b42fe8 100644 --- a/app/assets/stylesheets/framework/mobile.scss +++ b/app/assets/stylesheets/framework/mobile.scss @@ -81,7 +81,7 @@ display: none; } - .center-top-menu, .left-top-menu { + .nav-links, .nav-links { li a { font-size: 14px; padding: 19px 10px; diff --git a/app/assets/stylesheets/framework/nav.scss b/app/assets/stylesheets/framework/nav.scss index ffa9127898..c537d97fb2 100644 --- a/app/assets/stylesheets/framework/nav.scss +++ b/app/assets/stylesheets/framework/nav.scss @@ -2,7 +2,7 @@ padding: 0; margin: 0; list-style: none; - height: 56px; + height: auto; border-bottom: 1px solid $border-color; li { @@ -10,6 +10,7 @@ a { display: inline-block; + padding: 14px; padding-top: $gl-padding; padding-bottom: 11px; margin-bottom: -1px; @@ -25,7 +26,7 @@ } &.active a { - color: #616060; + color: #000000; border-bottom: 2px solid #4688f1; } diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 06ce52e26d..540d0b0316 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -24,7 +24,7 @@ .container-fluid { background: #FFF; - padding: $gl-padding; + padding: 0 $gl-padding; &.container-blank { background: none; diff --git a/app/assets/stylesheets/pages/detail_page.scss b/app/assets/stylesheets/pages/detail_page.scss index 6844d5b421..a9605804fb 100644 --- a/app/assets/stylesheets/pages/detail_page.scss +++ b/app/assets/stylesheets/pages/detail_page.scss @@ -1,8 +1,5 @@ .detail-page-header { - margin-top: -$gl-padding; - margin-bottom: -$gl-padding; padding: 7px 0; - margin-bottom: 0px; border-bottom: 1px solid $border-color; color: #5c5d5e; font-size: 16px; diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index afd6fb7367..c5515bc418 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -1,7 +1,5 @@ // Common .diff-file { - margin-left: -$gl-padding; - margin-right: -$gl-padding; border: none; border-bottom: 1px solid #E7E9EE; diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index d4b44004f4..7ab7755049 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -30,7 +30,7 @@ margin-top: 7px; } - .center-top-menu { + .nav-links { text-align: left; } } diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 82effde0bf..0e95aaff65 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -3,9 +3,9 @@ * */ .mr-state-widget { - background: #F7F8FA; + background: $background-color; color: $gl-gray; - border: 1px solid #dce0e6; + border: 1px solid $border-color; @include border-radius(2px); form { diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index a8bf63539b..48fdfb1de6 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -228,7 +228,6 @@ } .projects-search-form { - .input-group .form-control { height: 42px; } @@ -418,9 +417,8 @@ ul.nav.nav-projects-tabs { .top-area { border-bottom: 1px solid #EEE; - height: 42px; - ul.left-top-menu { + ul.nav-links { display: inline-block; width: 50%; margin-bottom: 0px; @@ -523,8 +521,7 @@ pre.light-well { } .projects-search-form { - margin: -$gl-padding; - padding: $gl-padding; + padding: $gl-padding 0; padding-bottom: 0; margin-bottom: 0px; @@ -660,8 +657,6 @@ pre.light-well { } .project-show-readme .readme-holder { - margin-left: -$gl-padding; - margin-right: -$gl-padding; padding: ($gl-padding + 7px); border-top: 0; diff --git a/app/views/admin/builds/index.html.haml b/app/views/admin/builds/index.html.haml index ddd4e1481e..ebf2b7b60e 100644 --- a/app/views/admin/builds/index.html.haml +++ b/app/views/admin/builds/index.html.haml @@ -4,7 +4,7 @@ - if @all_builds.running_or_pending.any? = link_to 'Cancel all', cancel_all_admin_builds_path, data: { confirm: 'Are you sure?' }, class: 'btn btn-danger', method: :post - %ul.center-top-menu + %ul.nav-links %li{class: ('active' if @scope.nil?)} = link_to admin_builds_path do All diff --git a/app/views/dashboard/_activities.html.haml b/app/views/dashboard/_activities.html.haml index f98fd9f06b..dc76599b77 100644 --- a/app/views/dashboard/_activities.html.haml +++ b/app/views/dashboard/_activities.html.haml @@ -1,9 +1,9 @@ .hidden-xs = render "events/event_last_push", event: @last_push -.gray-content-block +.nav-block - if current_user - .pull-right + .controls = link_to dashboard_projects_path(:atom, { private_token: current_user.private_token }), class: 'btn rss-btn' do %i.fa.fa-rss = render 'shared/event_filter' diff --git a/app/views/dashboard/_activity_head.html.haml b/app/views/dashboard/_activity_head.html.haml index 9f4be025bf..b78e70ebc1 100644 --- a/app/views/dashboard/_activity_head.html.haml +++ b/app/views/dashboard/_activity_head.html.haml @@ -1,4 +1,4 @@ -%ul.center-top-menu +%ul.nav-links %li{ class: ("active" unless params[:filter]) } = link_to activity_dashboard_path, class: 'shortcuts-activity', data: {placement: 'right'} do Your Projects diff --git a/app/views/dashboard/_groups_head.html.haml b/app/views/dashboard/_groups_head.html.haml index 64bd356f54..6ca97a692b 100644 --- a/app/views/dashboard/_groups_head.html.haml +++ b/app/views/dashboard/_groups_head.html.haml @@ -1,4 +1,4 @@ -%ul.center-top-menu +%ul.nav-links = nav_link(page: dashboard_groups_path) do = link_to dashboard_groups_path, title: 'Your groups', data: {placement: 'right'} do Your Groups diff --git a/app/views/dashboard/_projects_head.html.haml b/app/views/dashboard/_projects_head.html.haml index f4a3e3162b..5c4b58cd68 100644 --- a/app/views/dashboard/_projects_head.html.haml +++ b/app/views/dashboard/_projects_head.html.haml @@ -1,7 +1,7 @@ = content_for :flash_message do = render 'shared/project_limit' .top-area - %ul.left-top-menu + %ul.nav-links = nav_link(page: [dashboard_projects_path, root_path]) do = link_to dashboard_projects_path, title: 'Home', class: 'shortcuts-activity', data: {placement: 'right'} do Your Projects diff --git a/app/views/dashboard/_snippets_head.html.haml b/app/views/dashboard/_snippets_head.html.haml index 0ae62d6f1b..b25e8ea1f0 100644 --- a/app/views/dashboard/_snippets_head.html.haml +++ b/app/views/dashboard/_snippets_head.html.haml @@ -1,4 +1,4 @@ -%ul.center-top-menu +%ul.nav-links = nav_link(page: dashboard_snippets_path, html_options: {class: 'home'}) do = link_to dashboard_snippets_path, title: 'Your snippets', data: {placement: 'right'} do Your Snippets diff --git a/app/views/dashboard/milestones/show.html.haml b/app/views/dashboard/milestones/show.html.haml index 49a558e8ac..3810267577 100644 --- a/app/views/dashboard/milestones/show.html.haml +++ b/app/views/dashboard/milestones/show.html.haml @@ -48,7 +48,7 @@ #{@milestone.open_items_count} open = milestone_progress_bar(@milestone) -%ul.center-top-menu.no-top.no-bottom +%ul.nav-links.no-top.no-bottom %li.active = link_to '#tab-issues', 'data-toggle' => 'tab' do Issues diff --git a/app/views/dashboard/snippets/index.html.haml b/app/views/dashboard/snippets/index.html.haml index 07b6d57932..d4e7862981 100644 --- a/app/views/dashboard/snippets/index.html.haml +++ b/app/views/dashboard/snippets/index.html.haml @@ -3,32 +3,36 @@ = render 'dashboard/snippets_head' -.gray-content-block - .pull-right +.nav-block + .controls = link_to new_snippet_path, class: "btn btn-new", title: "New Snippet" do = icon('plus') New Snippet - .btn-group.btn-group-next.snippet-scope-menu - = link_to dashboard_snippets_path, class: "btn btn-default #{"active" unless params[:scope]}" do - All - %span.badge - = current_user.snippets.count + .nav-links.snippet-scope-menu + %li{ class: ("active" unless params[:scope]) } + = link_to dashboard_snippets_path do + All + %span.badge + = current_user.snippets.count - = link_to dashboard_snippets_path(scope: 'are_private'), class: "btn btn-default #{"active" if params[:scope] == "are_private"}" do - Private - %span.badge - = current_user.snippets.are_private.count + %li{ class: ("active" if params[:scope] == "are_private") } + = link_to dashboard_snippets_path(scope: 'are_private') do + Private + %span.badge + = current_user.snippets.are_private.count - = link_to dashboard_snippets_path(scope: 'are_internal'), class: "btn btn-default #{"active" if params[:scope] == "are_internal"}" do - Internal - %span.badge - = current_user.snippets.are_internal.count + %li{ class: ("active" if params[:scope] == "are_internal") } + = link_to dashboard_snippets_path(scope: 'are_internal') do + Internal + %span.badge + = current_user.snippets.are_internal.count - = link_to dashboard_snippets_path(scope: 'are_public'), class: "btn btn-default #{"active" if params[:scope] == "are_public"}" do - Public - %span.badge - = current_user.snippets.are_public.count + %li{ class: ("active" if params[:scope] == "are_public") } + = link_to dashboard_snippets_path(scope: 'are_public') do + Public + %span.badge + = current_user.snippets.are_public.count = render 'snippets/snippets' diff --git a/app/views/groups/milestones/show.html.haml b/app/views/groups/milestones/show.html.haml index d063b257b5..1233da8552 100644 --- a/app/views/groups/milestones/show.html.haml +++ b/app/views/groups/milestones/show.html.haml @@ -54,7 +54,7 @@ #{@milestone.open_items_count} open = milestone_progress_bar(@milestone) -%ul.center-top-menu.no-top.no-bottom +%ul.nav-links.no-top.no-bottom %li.active = link_to '#tab-issues', 'data-toggle' => 'tab' do Issues diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 24d2f000b4..4da7035876 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -56,5 +56,5 @@ = render "projects", projects: @projects - else - %p.center-top-menu.no-top + %p.nav-links.no-top No projects to show diff --git a/app/views/help/ui.html.haml b/app/views/help/ui.html.haml index d9ffda884c..65335c708f 100644 --- a/app/views/help/ui.html.haml +++ b/app/views/help/ui.html.haml @@ -139,9 +139,9 @@ %h2#navs Navigation %h4 - %code .center-top-menu + %code .nav-links .example - %ul.center-top-menu + %ul.nav-links %li.active %a Open %li diff --git a/app/views/projects/_activity.html.haml b/app/views/projects/_activity.html.haml index 6edc94e7a9..961b61d2e7 100644 --- a/app/views/projects/_activity.html.haml +++ b/app/views/projects/_activity.html.haml @@ -1,6 +1,6 @@ -.activity-filter-block +.nav-block.activity-filter-block - if current_user - .pull-right + .controls = link_to namespace_project_path(@project.namespace, @project, format: :atom, private_token: current_user.private_token), title: "Feed", class: 'btn rss-btn' do %i.fa.fa-rss diff --git a/app/views/projects/_md_preview.html.haml b/app/views/projects/_md_preview.html.haml index 54c818baaf..1fb37ef662 100644 --- a/app/views/projects/_md_preview.html.haml +++ b/app/views/projects/_md_preview.html.haml @@ -1,6 +1,6 @@ .md-area .md-header.clearfix - %ul.center-top-menu + %ul.nav-links %li.active %a.js-md-write-button(href="#md-write-holder" tabindex="-1") Write diff --git a/app/views/projects/blob/edit.html.haml b/app/views/projects/blob/edit.html.haml index 09fa148b12..a279e6eda5 100644 --- a/app/views/projects/blob/edit.html.haml +++ b/app/views/projects/blob/edit.html.haml @@ -2,7 +2,7 @@ = render "header_title" .file-editor - %ul.center-top-menu.no-bottom.js-edit-mode + %ul.nav-links.no-bottom.js-edit-mode %li.active = link_to '#editor' do = icon('edit') diff --git a/app/views/projects/builds/index.html.haml b/app/views/projects/builds/index.html.haml index 3bbfdb1e3b..5d18c0d803 100644 --- a/app/views/projects/builds/index.html.haml +++ b/app/views/projects/builds/index.html.haml @@ -8,7 +8,7 @@ - if @all_builds.running_or_pending.any? = link_to 'Cancel running', cancel_all_namespace_project_builds_path(@project.namespace, @project), data: { confirm: 'Are you sure?' }, class: 'btn btn-danger', method: :post - %ul.center-top-menu + %ul.nav-links %li{class: ('active' if @scope.nil?)} = link_to project_builds_path(@project) do All diff --git a/app/views/projects/builds/show.html.haml b/app/views/projects/builds/show.html.haml index 5b7ecce86a..08a59d465c 100644 --- a/app/views/projects/builds/show.html.haml +++ b/app/views/projects/builds/show.html.haml @@ -14,7 +14,7 @@ #up-build-trace - if @commit.matrix_for_ref?(@build.ref) - %ul.center-top-menu.no-top.no-bottom + %ul.nav-links.no-top.no-bottom - @commit.latest_builds_for_ref(@build.ref).each do |build| %li{class: ('active' if build == @build) } = link_to namespace_project_build_path(@project.namespace, @project, build) do diff --git a/app/views/projects/commit/_ci_menu.html.haml b/app/views/projects/commit/_ci_menu.html.haml index f74f8b427e..ea33aa472a 100644 --- a/app/views/projects/commit/_ci_menu.html.haml +++ b/app/views/projects/commit/_ci_menu.html.haml @@ -1,4 +1,4 @@ -%ul.center-top-menu.no-top.no-bottom.commit-ci-menu +%ul.nav-links.no-top.no-bottom.commit-ci-menu = nav_link(path: 'commit#show') do = link_to namespace_project_commit_path(@project.namespace, @project, @commit.id) do Changes diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml index 58aa45e8d2..02297158de 100644 --- a/app/views/projects/commit/show.html.haml +++ b/app/views/projects/commit/show.html.haml @@ -2,7 +2,9 @@ - page_description @commit.description = render "projects/commits/header_title" -= render "commit_box" + +.prepend-top-default + = render "commit_box" - if @ci_commit = render "ci_menu" - else diff --git a/app/views/projects/commits/_head.html.haml b/app/views/projects/commits/_head.html.haml index fcccb002d7..498c5e05b3 100644 --- a/app/views/projects/commits/_head.html.haml +++ b/app/views/projects/commits/_head.html.haml @@ -1,4 +1,4 @@ -%ul.center-top-menu +%ul.nav-links = nav_link(controller: [:commit, :commits]) do = link_to namespace_project_commits_path(@project.namespace, @project, current_ref) do Commits diff --git a/app/views/projects/graphs/_head.html.haml b/app/views/projects/graphs/_head.html.haml index a47643bd09..79a56647c5 100644 --- a/app/views/projects/graphs/_head.html.haml +++ b/app/views/projects/graphs/_head.html.haml @@ -1,4 +1,4 @@ -%ul.center-top-menu +%ul.nav-links = nav_link(action: :show) do = link_to 'Contributors', namespace_project_graph_path = nav_link(action: :commits) do diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml index a14943b15d..dd2c59e112 100644 --- a/app/views/projects/merge_requests/_new_submit.html.haml +++ b/app/views/projects/merge_requests/_new_submit.html.haml @@ -18,7 +18,7 @@ = f.hidden_field :target_branch .mr-compare.merge-request - %ul.merge-request-tabs.center-top-menu.no-top.no-bottom + %ul.merge-request-tabs.nav-links.no-top.no-bottom %li.commits-tab = link_to url_for(params), data: {target: 'div#commits', action: 'commits', toggle: 'tab'} do Commits diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index 095876450a..200bfa5ac4 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -45,7 +45,7 @@ = link_to "command line", "#modal_merge_info", class: "how_to_merge_link vlink", title: "How To Merge", "data-toggle" => "modal" - if @commits.present? - %ul.merge-request-tabs.center-top-menu.no-top.no-bottom + %ul.merge-request-tabs.nav-links.no-top.no-bottom %li.notes-tab = link_to namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: {target: 'div#notes', action: 'notes', toggle: 'tab'} do Discussion diff --git a/app/views/projects/milestones/show.html.haml b/app/views/projects/milestones/show.html.haml index 1670ea8741..f258b75e01 100644 --- a/app/views/projects/milestones/show.html.haml +++ b/app/views/projects/milestones/show.html.haml @@ -57,7 +57,7 @@ %span.pull-right= @milestone.expires_at = milestone_progress_bar(@milestone) -%ul.center-top-menu.no-top.no-bottom +%ul.nav-links.no-top.no-bottom %li.active = link_to '#tab-issues', 'data-toggle' => 'tab' do Issues diff --git a/app/views/projects/wikis/_nav.html.haml b/app/views/projects/wikis/_nav.html.haml index e6e6ad5bc4..69ba301e23 100644 --- a/app/views/projects/wikis/_nav.html.haml +++ b/app/views/projects/wikis/_nav.html.haml @@ -7,7 +7,7 @@ = render 'projects/wikis/new' - %ul.center-top-menu + %ul.nav-links = nav_link(html_options: {class: params[:id] == 'home' ? 'active' : '' }) do = link_to 'Home', namespace_project_wiki_path(@project.namespace, @project, :home) diff --git a/app/views/shared/_milestones_filter.html.haml b/app/views/shared/_milestones_filter.html.haml index cbdecda4ff..f77feeb79c 100644 --- a/app/views/shared/_milestones_filter.html.haml +++ b/app/views/shared/_milestones_filter.html.haml @@ -1,5 +1,5 @@ .milestones-filters - %ul.center-top-menu + %ul.nav-links %li{class: ("active" if params[:state].blank? || params[:state] == 'opened')} = link_to milestones_filter_path(state: 'opened') do Open diff --git a/app/views/shared/issuable/_filter.html.haml b/app/views/shared/issuable/_filter.html.haml index 0e3e9275fc..8d6f47b38e 100644 --- a/app/views/shared/issuable/_filter.html.haml +++ b/app/views/shared/issuable/_filter.html.haml @@ -1,6 +1,6 @@ .issues-filters .issues-state-filters - %ul.center-top-menu + %ul.nav-links - if defined?(type) && type == :merge_requests - page_context_word = 'merge requests' - else diff --git a/app/views/sherlock/queries/show.html.haml b/app/views/sherlock/queries/show.html.haml index 4a84348ac8..83f61ce4b0 100644 --- a/app/views/sherlock/queries/show.html.haml +++ b/app/views/sherlock/queries/show.html.haml @@ -1,7 +1,7 @@ - page_title t('sherlock.title'), t('sherlock.transaction'), t('sherlock.query') - header_title t('sherlock.title'), sherlock_transactions_path -%ul.center-top-menu +%ul.nav-links %li.active %a(href="#tab-general" data-toggle="tab") = t('sherlock.general') diff --git a/app/views/sherlock/transactions/show.html.haml b/app/views/sherlock/transactions/show.html.haml index 3c8ffb0664..9d4b0b2724 100644 --- a/app/views/sherlock/transactions/show.html.haml +++ b/app/views/sherlock/transactions/show.html.haml @@ -1,7 +1,7 @@ - page_title t('sherlock.title'), t('sherlock.transaction') - header_title t('sherlock.title'), sherlock_transactions_path -%ul.center-top-menu +%ul.nav-links %li.active %a(href="#tab-general" data-toggle="tab") = t('sherlock.general') diff --git a/app/views/users/show.html.haml b/app/views/users/show.html.haml index ce17fc7bca..e7848bf133 100644 --- a/app/views/users/show.html.haml +++ b/app/views/users/show.html.haml @@ -74,7 +74,7 @@ .user-calendar-activities -%ul.center-top-menu.no-top.no-bottom.bottom-border.wide +%ul.nav-links.no-top.no-bottom.bottom-border.wide %li.active = link_to "#activity", 'data-toggle' => 'tab' do Activity From 96dda5f6ecba417118d80a0bdfa5c0a14e15a04c Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 13 Jan 2016 16:00:08 +0100 Subject: [PATCH 059/218] Update UI help page with only one nav-link example Signed-off-by: Dmitriy Zaporozhets --- app/views/help/ui.html.haml | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/app/views/help/ui.html.haml b/app/views/help/ui.html.haml index 65335c708f..7b45bd0905 100644 --- a/app/views/help/ui.html.haml +++ b/app/views/help/ui.html.haml @@ -147,23 +147,6 @@ %li %a Closed - %h4 - %code .btn-group.btn-group-next - .example - %div.btn-group.btn-group-next - %a.btn.active Open - %a.btn Closed - - - %h4 - %code .nav.nav-tabs - .example - %ul.nav.nav-tabs - %li.active - %a Open - %li - %a Closed - %h2#buttons Buttons From 990bd06c04bebe6319968aa619990bf4cb60483c Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 13 Jan 2016 16:05:49 +0100 Subject: [PATCH 060/218] Change :ci_build_canceled factory to :canceled trait --- spec/factories/ci/builds.rb | 8 ++++---- spec/requests/api/builds_spec.rb | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 558598d4d5..636cdbba1b 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -43,6 +43,10 @@ FactoryGirl.define do commit factory: :ci_commit + trait :canceled do + status 'canceled' + end + after(:build) do |build, evaluator| build.project = build.commit.project end @@ -62,9 +66,5 @@ FactoryGirl.define do build.trace = 'BUILD TRACE' end end - - factory :ci_build_canceled do - status 'canceled' - end end end diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb index 4bf3d2681d..e5567d4250 100644 --- a/spec/requests/api/builds_spec.rb +++ b/spec/requests/api/builds_spec.rb @@ -11,7 +11,7 @@ describe API::API, api: true do let(:commit) { create(:ci_commit, project: project)} let(:build) { create(:ci_build, commit: commit) } let(:build_with_trace) { create(:ci_build_with_trace, commit: commit) } - let(:build_canceled) { create(:ci_build_canceled, commit: commit) } + let(:build_canceled) { create(:ci_build, :canceled, commit: commit) } describe 'GET /projects/:id/builds ' do context 'authorized user' do From 132ab484d46044c4c7a88b7df02e67a29255e032 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 13 Jan 2016 16:06:59 +0100 Subject: [PATCH 061/218] Increase detail-page-header padding to match height with nav-link Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/pages/detail_page.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/stylesheets/pages/detail_page.scss b/app/assets/stylesheets/pages/detail_page.scss index a9605804fb..529a43548c 100644 --- a/app/assets/stylesheets/pages/detail_page.scss +++ b/app/assets/stylesheets/pages/detail_page.scss @@ -1,5 +1,5 @@ .detail-page-header { - padding: 7px 0; + padding: 11px 0; border-bottom: 1px solid $border-color; color: #5c5d5e; font-size: 16px; From c44b927bc00ed75b41f3951a15794c5cc6bdc60e Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 13 Jan 2016 16:10:06 +0100 Subject: [PATCH 062/218] Allign md header components Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/framework/markdown_area.scss | 7 ------- app/assets/stylesheets/framework/zen.scss | 2 +- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/app/assets/stylesheets/framework/markdown_area.scss b/app/assets/stylesheets/framework/markdown_area.scss index 4a00a197d9..f0674205c1 100644 --- a/app/assets/stylesheets/framework/markdown_area.scss +++ b/app/assets/stylesheets/framework/markdown_area.scss @@ -65,13 +65,6 @@ position: relative; } -.md-header { - ul { - float: left; - margin-bottom: 1px; - } -} - .referenced-users { color: #4c4e54; padding-top: 10px; diff --git a/app/assets/stylesheets/framework/zen.scss b/app/assets/stylesheets/framework/zen.scss index 002bd7e8ca..c3f27333fa 100644 --- a/app/assets/stylesheets/framework/zen.scss +++ b/app/assets/stylesheets/framework/zen.scss @@ -4,7 +4,7 @@ position: absolute; top: 0px; right: 4px; - line-height: 40px; + line-height: 56px; } a.js-zen-leave { From 0673fad893e2cd457424bc730b1faa146e1f6f5f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 13 Jan 2016 16:26:40 +0100 Subject: [PATCH 063/218] Apply new layout to user page Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/framework/lists.scss | 1 + app/views/groups/show.html.haml | 7 +- app/views/projects/show.html.haml | 6 +- app/views/users/show.html.haml | 127 ++++++++++---------- 4 files changed, 72 insertions(+), 69 deletions(-) diff --git a/app/assets/stylesheets/framework/lists.scss b/app/assets/stylesheets/framework/lists.scss index a5e1afd602..c6bc6fb324 100644 --- a/app/assets/stylesheets/framework/lists.scss +++ b/app/assets/stylesheets/framework/lists.scss @@ -131,6 +131,7 @@ ul.content-list { .panel > .content-list { li { margin: 0; + padding: $gl-padding; } } diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 4da7035876..5e39d3ab60 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -1,3 +1,6 @@ +- @no_container = true +- @blank_container = true + - unless can?(current_user, :read_group, @group) - @disable_search_panel = true @@ -5,9 +8,6 @@ - if current_user = auto_discovery_link_tag(:atom, group_url(@group, format: :atom, private_token: current_user.private_token), title: "#{@group.name} activity") -- @no_container = true -- @blank_container = true - .cover-block .cover-controls - if @group && can?(current_user, :admin_group, @group) @@ -39,7 +39,6 @@ Projects - if can?(current_user, :read_group, @group) - %div{ class: container_class } .tab-content .tab-pane.active#activity diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index 25dcedf2b0..65444e7e33 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -1,3 +1,6 @@ +- @no_container = true +- @blank_container = true + = content_for :meta_tags do - if current_user = auto_discovery_link_tag(:atom, namespace_project_path(@project.namespace, @project, format: :atom, private_token: current_user.private_token), title: "#{@project.name} activity") @@ -7,9 +10,6 @@ = render 'shared/no_ssh' = render 'shared/no_password' -- @no_container = true -- @blank_container = true - = render 'projects/last_push' = render "home_panel" diff --git a/app/views/users/show.html.haml b/app/views/users/show.html.haml index e7848bf133..284a62e6f4 100644 --- a/app/views/users/show.html.haml +++ b/app/views/users/show.html.haml @@ -1,6 +1,8 @@ - page_title @user.name - page_description @user.bio - header_title @user.name, user_path(@user) +- @no_container = true +- @blank_container = true = content_for :meta_tags do = auto_discovery_link_tag(:atom, user_url(@user, format: :atom), title: "#{@user.name} activity") @@ -8,6 +10,25 @@ = render 'shared/show_aside' .cover-block + .cover-controls + - if @user == current_user + = link_to profile_path, class: 'btn btn-gray' do + = icon('pencil') + - elsif current_user + %span.report-abuse + - if @user.abuse_report + %button.btn.btn-danger{ title: 'Already reported for abuse', + data: { toggle: 'tooltip', placement: 'left', container: 'body' }} + = icon('exclamation-circle') + - else + = link_to new_abuse_report_path(user_id: @user.id), class: 'btn btn-gray', + title: 'Report abuse', data: {toggle: 'tooltip', placement: 'left', container: 'body'} do + = icon('exclamation-circle') + - if current_user +   + = link_to user_path(@user, :atom, { private_token: current_user.private_token }), class: 'btn btn-gray' do + = icon('rss') + .avatar-holder = link_to avatar_icon(@user, 400), target: '_blank' do = image_tag avatar_icon(@user, 90), class: "avatar s90", alt: '' @@ -47,74 +68,56 @@ = icon('map-marker') = @user.location + %ul.nav-links.center + %li.active + = link_to "#activity", 'data-toggle' => 'tab' do + Activity + - if @groups.any? + %li + = link_to "#groups", 'data-toggle' => 'tab' do + Groups + - if @contributed_projects.present? + %li + = link_to "#contributed", 'data-toggle' => 'tab' do + Contributed projects + - if @projects.present? + %li + = link_to "#personal", 'data-toggle' => 'tab' do + Personal projects - .cover-controls - - if @user == current_user - = link_to profile_path, class: 'btn btn-gray' do - = icon('pencil') - - elsif current_user - %span.report-abuse - - if @user.abuse_report - %button.btn.btn-danger{ title: 'Already reported for abuse', - data: { toggle: 'tooltip', placement: 'left', container: 'body' }} - = icon('exclamation-circle') - - else - = link_to new_abuse_report_path(user_id: @user.id), class: 'btn btn-gray', - title: 'Report abuse', data: {toggle: 'tooltip', placement: 'left', container: 'body'} do - = icon('exclamation-circle') - - if current_user -   - = link_to user_path(@user, :atom, { private_token: current_user.private_token }), class: 'btn btn-gray' do - = icon('rss') - -.gray-content-block.second-block - .user-calendar - %h4.center.light - %i.fa.fa-spinner.fa-spin - .user-calendar-activities +%div{ class: container_class } + .tab-content + .tab-pane.active#activity + .gray-content-block.white.second-block + %div{ class: container_class } + .user-calendar + %h4.center.light + %i.fa.fa-spinner.fa-spin + .user-calendar-activities -%ul.nav-links.no-top.no-bottom.bottom-border.wide - %li.active - = link_to "#activity", 'data-toggle' => 'tab' do - Activity - - if @groups.any? - %li - = link_to "#groups", 'data-toggle' => 'tab' do - Groups - - if @contributed_projects.present? - %li - = link_to "#contributed", 'data-toggle' => 'tab' do - Contributed projects - - if @projects.present? - %li - = link_to "#personal", 'data-toggle' => 'tab' do - Personal projects + .content_list + = spinner -.tab-content - .tab-pane.active#activity - .content_list - = spinner + - if @groups.any? + .tab-pane#groups + %ul.content-list + - @groups.each do |group| + = render 'shared/groups/group', group: group - - if @groups.any? - .tab-pane#groups - %ul.content-list - - @groups.each do |group| - = render 'shared/groups/group', group: group + - if @contributed_projects.present? + .tab-pane#contributed + .contributed-projects + = render 'shared/projects/list', + projects: @contributed_projects.sort_by(&:star_count).reverse, + projects_limit: 10, stars: true, avatar: true - - if @contributed_projects.present? - .tab-pane#contributed - .contributed-projects - = render 'shared/projects/list', - projects: @contributed_projects.sort_by(&:star_count).reverse, - projects_limit: 5, stars: true, avatar: true - - - if @projects.present? - .tab-pane#personal - .personal-projects - = render 'shared/projects/list', - projects: @projects.sort_by(&:star_count).reverse, - projects_limit: 10, stars: true, avatar: true + - if @projects.present? + .tab-pane#personal + .personal-projects + = render 'shared/projects/list', + projects: @projects.sort_by(&:star_count).reverse, + projects_limit: 10, stars: true, avatar: true :javascript $(".user-calendar").load("#{user_calendar_path}"); From 5efbfa14d4666655edd6d79a3a352a134382b664 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 13 Jan 2016 16:37:17 +0100 Subject: [PATCH 064/218] Move complex view condition to a model method This is moved to a model method rather than an helper method because the API will need it too. --- app/models/note.rb | 4 ++++ app/views/projects/notes/_notes.html.haml | 5 ++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/models/note.rb b/app/models/note.rb index 3d5b663c99..3e1375e5ad 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -358,6 +358,10 @@ class Note < ActiveRecord::Base !system? && !is_award end + def cross_reference_not_visible_for?(user) + cross_reference? && referenced_mentionables(user).empty? + end + # Checks if note is an award added as a comment # # If note is an award, this method sets is_award to true diff --git a/app/views/projects/notes/_notes.html.haml b/app/views/projects/notes/_notes.html.haml index a4ff947c65..62db86fb18 100644 --- a/app/views/projects/notes/_notes.html.haml +++ b/app/views/projects/notes/_notes.html.haml @@ -2,11 +2,14 @@ - @discussions.each do |discussion_notes| - note = discussion_notes.first - if note_for_main_target?(note) + - next if note.cross_reference_not_visible_for?(current_user) + = render discussion_notes - else = render 'projects/notes/discussion', discussion_notes: discussion_notes - else - @notes.each do |note| - next unless note.author - - next if note.cross_reference? && note.referenced_mentionables(current_user).empty? + - next if note.cross_reference_not_visible_for?(current_user) + = render note From cf1a62e236bd74ce31eda3b07505664bed445a96 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 13 Jan 2016 17:01:11 +0100 Subject: [PATCH 065/218] Add top margin for page title Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/framework/typography.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss index 714369d9f1..ab4f71af03 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -177,7 +177,7 @@ body { } .page-title { - margin-top: 0px; + margin-top: $gl-padding; line-height: 1.3; font-size: 1.25em; font-weight: 600; From 5e452d3794ffa4996611ecf53c6098f4a3913d4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 13 Jan 2016 16:38:42 +0100 Subject: [PATCH 066/218] Improve & adds specs for Issue/MR references - Improve specs for private Issue/MR referenced in public Issue - Add specs for private Issue/MR referenced in public MR --- features/project/issues/references.feature | 28 ++-- .../project/merge_requests/references.feature | 31 ++++ features/steps/project/issues/references.rb | 101 +------------ .../project/merge_requests/references.rb | 7 + features/steps/shared/issuable.rb | 134 ++++++++++++++++++ features/steps/shared/note.rb | 4 +- features/steps/shared/project.rb | 48 ++++--- 7 files changed, 218 insertions(+), 135 deletions(-) create mode 100644 features/project/merge_requests/references.feature create mode 100644 features/steps/project/merge_requests/references.rb diff --git a/features/project/issues/references.feature b/features/project/issues/references.feature index bf7a4c6cb9..4ae2d65333 100644 --- a/features/project/issues/references.feature +++ b/features/project/issues/references.feature @@ -2,30 +2,32 @@ Feature: Project Issues References Background: Given I sign in as "John Doe" + And public project "Community" And "John Doe" owns public project "Community" - And project "Community" has "Public Issue 01" open issue + And project "Community" has "Community issue" open issue And I logout And I sign in as "Mary Jane" - And "Mary Jane" owns private project "Private Library" - And project "Private Library" has "Fix NS-01" open merge request - And project "Private Library" has "Private Issue 01" open issue - And I visit merge request page "Fix NS-01" - And I leave a comment referencing issue "Public Issue 01" from "Fix NS-01" merge request - And I visit issue page "Private Issue 01" - And I leave a comment referencing issue "Public Issue 01" from "Private Issue 01" issue + And private project "Enterprise" + And "Mary Jane" owns private project "Enterprise" + And project "Enterprise" has "Enterprise issue" open issue + And project "Enterprise" has "Enterprise fix" open merge request + And I visit issue page "Enterprise issue" + And I leave a comment referencing issue "Community issue" + And I visit merge request page "Enterprise fix" + And I leave a comment referencing issue "Community issue" And I logout @javascript Scenario: Viewing the public issue as a "John Doe" Given I sign in as "John Doe" - When I visit issue page "Public Issue 01" + When I visit issue page "Community issue" Then I should not see any related merge requests And I should see no notes at all @javascript Scenario: Viewing the public issue as "Mary Jane" Given I sign in as "Mary Jane" - When I visit issue page "Public Issue 01" - Then I should see the "Fix NS-01" related merge request - And I should see a note linking to "Fix NS-01" merge request - And I should see a note linking to "Private Issue 01" issue + When I visit issue page "Community issue" + Then I should see the "Enterprise fix" related merge request + And I should see a note linking to "Enterprise fix" merge request + And I should see a note linking to "Enterprise issue" issue diff --git a/features/project/merge_requests/references.feature b/features/project/merge_requests/references.feature new file mode 100644 index 0000000000..571612261a --- /dev/null +++ b/features/project/merge_requests/references.feature @@ -0,0 +1,31 @@ +@project_merge_requests +Feature: Project Merge Requests References + Background: + Given I sign in as "John Doe" + And public project "Community" + And "John Doe" owns public project "Community" + And project "Community" has "Community fix" open merge request + And I logout + And I sign in as "Mary Jane" + And private project "Enterprise" + And "Mary Jane" owns private project "Enterprise" + And project "Enterprise" has "Enterprise issue" open issue + And project "Enterprise" has "Enterprise fix" open merge request + And I visit issue page "Enterprise issue" + And I leave a comment referencing issue "Community fix" + And I visit merge request page "Enterprise fix" + And I leave a comment referencing issue "Community fix" + And I logout + + @javascript + Scenario: Viewing the public issue as a "John Doe" + Given I sign in as "John Doe" + When I visit issue page "Community fix" + Then I should see no notes at all + + @javascript + Scenario: Viewing the public issue as "Mary Jane" + Given I sign in as "Mary Jane" + When I visit issue page "Community fix" + And I should see a note linking to "Enterprise fix" merge request + And I should see a note linking to "Enterprise issue" issue diff --git a/features/steps/project/issues/references.rb b/features/steps/project/issues/references.rb index 9c7725e8c1..69e8b5cbde 100644 --- a/features/steps/project/issues/references.rb +++ b/features/steps/project/issues/references.rb @@ -1,106 +1,7 @@ class Spinach::Features::ProjectIssuesReferences < Spinach::FeatureSteps include SharedAuthentication + include SharedIssuable include SharedNote include SharedProject include SharedUser - - step 'project "Community" has "Public Issue 01" open issue' do - project = Project.find_by(name: 'Community') - create(:issue, - title: 'Public Issue 01', - project: project, - author: project.users.first, - description: '# Description header' - ) - end - - step 'project "Private Library" has "Fix NS-01" open merge request' do - project = Project.find_by(name: 'Private Library') - create(:merge_request, - title: 'Fix NS-01', - source_project: project, - target_project: project, - source_branch: 'fix', - target_branch: 'master', - author: project.users.first, - description: '# Description header' - ) - end - - step 'project "Private Library" has "Private Issue 01" open issue' do - project = Project.find_by(name: 'Private Library') - create(:issue, - title: 'Private Issue 01', - project: project, - author: project.users.first, - description: '# Description header' - ) - end - - step 'I leave a comment referencing issue "Public Issue 01" from "Fix NS-01" merge request' do - project = Project.find_by(name: 'Private Library') - issue = Issue.find_by!(title: 'Public Issue 01') - - page.within(".js-main-target-form") do - fill_in "note[note]", with: "##{issue.to_reference(project)}" - click_button "Add Comment" - end - end - - step 'I leave a comment referencing issue "Public Issue 01" from "Private Issue 01" issue' do - project = Project.find_by(name: 'Private Library') - issue = Issue.find_by!(title: 'Public Issue 01') - - page.within(".js-main-target-form") do - fill_in "note[note]", with: "##{issue.to_reference(project)}" - click_button "Add Comment" - end - end - - step 'I visit merge request page "Fix NS-01"' do - mr = MergeRequest.find_by(title: "Fix NS-01") - visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr) - end - - step 'I visit issue page "Private Issue 01"' do - issue = Issue.find_by(title: "Private Issue 01") - visit namespace_project_issue_path(issue.project.namespace, issue.project, issue) - end - - step 'I visit issue page "Public Issue 01"' do - issue = Issue.find_by(title: "Public Issue 01") - visit namespace_project_issue_path(issue.project.namespace, issue.project, issue) - end - - step 'I should not see any related merge requests' do - page.within '.issue-details' do - expect(page).not_to have_content('.merge-requests') - end - end - - step 'I should see the "Fix NS-01" related merge request' do - page.within '.merge-requests' do - expect(page).to have_content("1 Related Merge Request") - expect(page).to have_content('Fix NS-01') - end - end - - step 'I should see a note linking to "Fix NS-01" merge request' do - project = Project.find_by(name: 'Community') - mr = MergeRequest.find_by(title: 'Fix NS-01') - page.within('.notes') do - expect(page).to have_content('Mary Jane') - expect(page).to have_content("mentioned in merge request #{mr.to_reference(project)}") - end - end - - step 'I should see a note linking to "Private Issue 01" issue' do - project = Project.find_by(name: 'Community') - issue = Issue.find_by(title: 'Private Issue 01') - page.within('.notes') do - expect(page).to have_content('Mary Jane') - expect(page).to have_content("mentioned in issue #{issue.to_reference(project)}") - end - end - end diff --git a/features/steps/project/merge_requests/references.rb b/features/steps/project/merge_requests/references.rb new file mode 100644 index 0000000000..ab2ae6847a --- /dev/null +++ b/features/steps/project/merge_requests/references.rb @@ -0,0 +1,7 @@ +class Spinach::Features::ProjectMergeRequestsReferences < Spinach::FeatureSteps + include SharedAuthentication + include SharedIssuable + include SharedNote + include SharedProject + include SharedUser +end diff --git a/features/steps/shared/issuable.rb b/features/steps/shared/issuable.rb index e6d1b8b8ef..4c5f7488ef 100644 --- a/features/steps/shared/issuable.rb +++ b/features/steps/shared/issuable.rb @@ -5,6 +5,99 @@ module SharedIssuable find(:css, '.issuable-edit').click end + step 'project "Community" has "Community issue" open issue' do + create_issuable_for_project( + project_name: 'Community', + title: 'Community issue' + ) + end + + step 'project "Community" has "Community fix" open merge request' do + create_issuable_for_project( + project_name: 'Community', + type: :merge_request, + title: 'Community fix' + ) + end + + step 'project "Enterprise" has "Enterprise issue" open issue' do + create_issuable_for_project( + project_name: 'Enterprise', + title: 'Enterprise issue' + ) + end + + step 'project "Enterprise" has "Enterprise fix" open merge request' do + create_issuable_for_project( + project_name: 'Enterprise', + type: :merge_request, + title: 'Enterprise fix' + ) + end + + step 'I leave a comment referencing issue "Community issue"' do + leave_reference_comment( + issuable: Issue.find_by(title: 'Community issue'), + from_project_name: 'Enterprise' + ) + end + + step 'I leave a comment referencing issue "Community fix"' do + leave_reference_comment( + issuable: MergeRequest.find_by(title: 'Community fix'), + from_project_name: 'Enterprise' + ) + end + + step 'I visit issue page "Enterprise issue"' do + issue = Issue.find_by(title: 'Enterprise issue') + visit namespace_project_issue_path(issue.project.namespace, issue.project, issue) + end + + step 'I visit merge request page "Enterprise fix"' do + mr = MergeRequest.find_by(title: 'Enterprise fix') + visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr) + end + + step 'I visit issue page "Community issue"' do + issue = Issue.find_by(title: 'Community issue') + visit namespace_project_issue_path(issue.project.namespace, issue.project, issue) + end + + step 'I visit issue page "Community fix"' do + mr = MergeRequest.find_by(title: 'Community fix') + visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr) + end + + step 'I should not see any related merge requests' do + page.within '.issue-details' do + expect(page).not_to have_content('.merge-requests') + end + end + + step 'I should see the "Enterprise fix" related merge request' do + page.within '.merge-requests' do + expect(page).to have_content('1 Related Merge Request') + expect(page).to have_content('Enterprise fix') + end + end + + step 'I should see a note linking to "Enterprise fix" merge request' do + visible_note( + issuable: MergeRequest.find_by(title: 'Enterprise fix'), + from_project_name: 'Community', + user_name: 'Mary Jane' + ) + end + + step 'I should see a note linking to "Enterprise issue" issue' do + visible_note( + issuable: Issue.find_by(title: 'Enterprise issue'), + from_project_name: 'Community', + user_name: 'Mary Jane' + ) + end + step 'I click link "Edit" for the merge request' do edit_issuable end @@ -12,4 +105,45 @@ module SharedIssuable step 'I click link "Edit" for the issue' do edit_issuable end + + def create_issuable_for_project(project_name:, title:, type: :issue) + project = Project.find_by(name: project_name) + + attrs = { + title: title, + author: project.users.first, + description: '# Description header' + } + + case type + when :issue + attrs.merge!(project: project) + when :merge_request + attrs.merge!( + source_project: project, + target_project: project, + source_branch: 'fix', + target_branch: 'master' + ) + end + + create(type, attrs) + end + + def leave_reference_comment(issuable:, from_project_name:) + project = Project.find_by(name: from_project_name) + + page.within('.js-main-target-form') do + fill_in 'note[note]', with: "##{issuable.to_reference(project)}" + click_button 'Add Comment' + end + end + + def visible_note(issuable:, from_project_name:, user_name:) + project = Project.find_by(name: from_project_name) + + expect(page).to have_content(user_name) + expect(page).to have_content("mentioned in #{issuable.class.to_s.titleize.downcase} #{issuable.to_reference(project)}") + end + end diff --git a/features/steps/shared/note.rb b/features/steps/shared/note.rb index 6de58c6e2b..444d6726f9 100644 --- a/features/steps/shared/note.rb +++ b/features/steps/shared/note.rb @@ -107,9 +107,7 @@ module SharedNote end step 'I should see no notes at all' do - page.within('.notes') do - expect(page).to_not have_css('.note') - end + expect(page).to_not have_css('.note') end # Markdown diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb index 43a15f4347..5420c45151 100644 --- a/features/steps/shared/project.rb +++ b/features/steps/shared/project.rb @@ -161,31 +161,33 @@ module SharedProject end step '"John Doe" owns private project "Enterprise"' do - user = user_exists("John Doe", username: "john_doe") - project = Project.find_by(name: "Enterprise") - project ||= create(:empty_project, name: "Enterprise", namespace: user.namespace) - project.team << [user, :master] + user_owns_project( + user_name: 'John Doe', + project_name: 'Enterprise' + ) + end + + step '"Mary Jane" owns private project "Enterprise"' do + user_owns_project( + user_name: 'Mary Jane', + project_name: 'Enterprise' + ) end step '"John Doe" owns internal project "Internal"' do - user = user_exists("John Doe", username: "john_doe") - project = Project.find_by(name: "Internal") - project ||= create :empty_project, :internal, name: 'Internal', namespace: user.namespace - project.team << [user, :master] + user_owns_project( + user_name: 'John Doe', + project_name: 'Internal', + visibility: :internal + ) end step '"John Doe" owns public project "Community"' do - user = user_exists("John Doe", username: "john_doe") - project = Project.find_by(name: "Community") - project ||= create :empty_project, :public, name: 'Community', namespace: user.namespace - project.team << [user, :master] - end - - step '"Mary Jane" owns private project "Private Library"' do - user = user_exists('Mary Jane', username: 'mary_jane') - project = Project.find_by(name: 'Private Library') - project ||= create(:project, name: 'Private Library', namespace: user.namespace) - project.team << [user, :master] + user_owns_project( + user_name: 'John Doe', + project_name: 'Community', + visibility: :public + ) end step 'public empty project "Empty Public Project"' do @@ -220,4 +222,12 @@ module SharedProject expect(page).to have_content("skipped") end end + + def user_owns_project(user_name:, project_name:, visibility: :private) + user = user_exists(user_name, username: user_name.underscore) + project = Project.find_by(name: project_name) + project ||= create(:empty_project, visibility, name: project_name, namespace: user.namespace) + project.team << [user, :master] + end + end From 3238b0bc9fddd1a00d4926a7679c454db03d6fed Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 13 Jan 2016 17:13:36 +0100 Subject: [PATCH 067/218] Replace nav-tabs with nav-links Signed-off-by: Dmitriy Zaporozhets --- .../javascripts/merge_request_tabs.js.coffee | 2 +- .../stylesheets/framework/markdown_area.scss | 17 ---------- app/assets/stylesheets/framework/mobile.scss | 2 +- .../stylesheets/framework/tw_bootstrap.scss | 31 ------------------- app/assets/stylesheets/pages/projects.scss | 22 ------------- app/views/admin/logs/show.html.haml | 5 +-- app/views/admin/users/_head.html.haml | 3 +- app/views/devise/shared/_signin_box.html.haml | 2 +- app/views/search/_category.html.haml | 2 +- app/views/search/_results.html.haml | 2 +- app/views/search/show.html.haml | 4 ++- 11 files changed, 13 insertions(+), 79 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js.coffee b/app/assets/javascripts/merge_request_tabs.js.coffee index 9e2dc1250c..b10e1db7f3 100644 --- a/app/assets/javascripts/merge_request_tabs.js.coffee +++ b/app/assets/javascripts/merge_request_tabs.js.coffee @@ -5,7 +5,7 @@ # # ### Example Markup # -#