From d515ec4df908fc65496904f4705ae903db54b6a8 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 18 Aug 2017 09:54:53 +0100 Subject: [PATCH 01/70] Fixes the breadcrumb container when in issue boards Closes #36561 --- app/views/layouts/nav/_breadcrumbs.html.haml | 3 ++- app/views/projects/boards/_show.html.haml | 1 + changelogs/unreleased/issue-boards-breadcrumbs-container.yml | 5 +++++ 3 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/issue-boards-breadcrumbs-container.yml diff --git a/app/views/layouts/nav/_breadcrumbs.html.haml b/app/views/layouts/nav/_breadcrumbs.html.haml index 4db84771f4e..653452871a0 100644 --- a/app/views/layouts/nav/_breadcrumbs.html.haml +++ b/app/views/layouts/nav/_breadcrumbs.html.haml @@ -1,8 +1,9 @@ - breadcrumb_link = breadcrumb_title_link +- container = @no_breadcrumb_container ? 'container-fluid' : container_class - hide_top_links = @hide_top_links || false %nav.breadcrumbs{ role: "navigation" } - .breadcrumbs-container{ class: [container_class, @content_class] } + .breadcrumbs-container{ class: [container, @content_class] } - if defined?(@new_sidebar) = button_tag class: 'toggle-mobile-nav', type: 'button' do %span.sr-only Open sidebar diff --git a/app/views/projects/boards/_show.html.haml b/app/views/projects/boards/_show.html.haml index 2076e46fde8..5354ec8522e 100644 --- a/app/views/projects/boards/_show.html.haml +++ b/app/views/projects/boards/_show.html.haml @@ -1,3 +1,4 @@ +- @no_breadcrumb_container = true - @no_container = true - @content_class = "issue-boards-content" - page_title "Boards" diff --git a/changelogs/unreleased/issue-boards-breadcrumbs-container.yml b/changelogs/unreleased/issue-boards-breadcrumbs-container.yml new file mode 100644 index 00000000000..5e042de7000 --- /dev/null +++ b/changelogs/unreleased/issue-boards-breadcrumbs-container.yml @@ -0,0 +1,5 @@ +--- +title: Fix breadcrumbs container in issue boards +merge_request: +author: +type: fixed From c13f712c77e0837760e79293b6fb41c734741e77 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Fri, 18 Aug 2017 17:43:23 -0700 Subject: [PATCH 02/70] implement Repository#== so that with_repo_branch_commit can properly short-circuit --- app/models/repository.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/models/repository.rb b/app/models/repository.rb index c1e4fcf94a4..1c3080c0efd 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -60,6 +60,12 @@ class Repository @project = project end + def equals(other) + @disk_path == other.disk_path + end + + alias_method :==, :equals + def raw_repository return nil unless full_path From 00b440443808fba04fc55f7e77c61f79e29c21ea Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Mon, 21 Aug 2017 15:20:44 -0700 Subject: [PATCH 03/70] skip the branch fetch if we already have the sha --- app/models/repository.rb | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 1c3080c0efd..61fab9764e8 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -991,23 +991,27 @@ class Repository end def with_repo_branch_commit(start_repository, start_branch_name) - return yield(nil) if start_repository.empty_repo? + tmp_ref = nil + return yield nil if start_repository.empty_repo? - branch_name_or_sha = + branch_commit = if start_repository == self - start_branch_name + commit(start_branch_name) else - tmp_ref = fetch_ref( - start_repository.path_to_repo, - "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", - "refs/tmp/#{SecureRandom.hex}/head" - ) + sha = start_repository.find_branch(start_branch_name).target + commit(sha) || + begin + tmp_ref = fetch_ref( + start_repository.path_to_repo, + "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", + "refs/tmp/#{SecureRandom.hex}/head" + ) - start_repository.commit(start_branch_name).sha + commit(start_repository.commit(start_branch_name).sha) + end end - yield(commit(branch_name_or_sha)) - + yield branch_commit ensure rugged.references.delete(tmp_ref) if tmp_ref end From 9e203582b367a1b84035572261a79b62e22bfeaa Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Wed, 23 Aug 2017 01:51:53 +0900 Subject: [PATCH 04/70] Improve AutocompleteController#user.json performance --- app/models/user.rb | 2 +- .../improve-autocomplete-user-performance.yml | 5 +++ lib/gitlab/sql/pattern.rb | 29 +++++++++++++++++ .../filtered_search/dropdown_author_spec.rb | 12 +++---- spec/lib/gitlab/sql/pattern_spec.rb | 31 +++++++++++++++++++ spec/models/user_spec.rb | 17 ++++++++++ 6 files changed, 89 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/improve-autocomplete-user-performance.yml create mode 100644 lib/gitlab/sql/pattern.rb create mode 100644 spec/lib/gitlab/sql/pattern_spec.rb diff --git a/app/models/user.rb b/app/models/user.rb index fbd08bc4d0a..e5a84ce4080 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -303,7 +303,7 @@ class User < ActiveRecord::Base # Returns an ActiveRecord::Relation. def search(query) table = arel_table - pattern = "%#{query}%" + pattern = Gitlab::SQL::Pattern.new(query).to_sql order = <<~SQL CASE diff --git a/changelogs/unreleased/improve-autocomplete-user-performance.yml b/changelogs/unreleased/improve-autocomplete-user-performance.yml new file mode 100644 index 00000000000..5a7153771ff --- /dev/null +++ b/changelogs/unreleased/improve-autocomplete-user-performance.yml @@ -0,0 +1,5 @@ +--- +title: Improve performance for AutocompleteController#users.json +merge_request: 13754 +author: Hiroyuki Sato +type: changed diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb new file mode 100644 index 00000000000..47ea19994a2 --- /dev/null +++ b/lib/gitlab/sql/pattern.rb @@ -0,0 +1,29 @@ +module Gitlab + module SQL + class Pattern + MIN_CHARS_FOR_PARTIAL_MATCHING = 3 + + attr_reader :query + + def initialize(query) + @query = query + end + + def to_sql + if exact_matching? + query + else + "%#{query}%" + end + end + + def exact_matching? + !partial_matching? + end + + def partial_matching? + @query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING + end + end + end +end diff --git a/spec/features/issues/filtered_search/dropdown_author_spec.rb b/spec/features/issues/filtered_search/dropdown_author_spec.rb index 975dc035f2d..3cec59050ab 100644 --- a/spec/features/issues/filtered_search/dropdown_author_spec.rb +++ b/spec/features/issues/filtered_search/dropdown_author_spec.rb @@ -6,7 +6,7 @@ describe 'Dropdown author', js: true do let!(:project) { create(:project) } let!(:user) { create(:user, name: 'administrator', username: 'root') } let!(:user_john) { create(:user, name: 'John', username: 'th0mas') } - let!(:user_jacob) { create(:user, name: 'Jacob', username: 'otter32') } + let!(:user_jacob) { create(:user, name: 'Jacob', username: 'ooter32') } let(:filtered_search) { find('.filtered-search') } let(:js_dropdown_author) { '#js-dropdown-author' } @@ -82,31 +82,31 @@ describe 'Dropdown author', js: true do end it 'filters by name' do - send_keys_to_filtered_search('ja') + send_keys_to_filtered_search('jac') expect(dropdown_author_size).to eq(1) end it 'filters by case insensitive name' do - send_keys_to_filtered_search('Ja') + send_keys_to_filtered_search('Jac') expect(dropdown_author_size).to eq(1) end it 'filters by username with symbol' do - send_keys_to_filtered_search('@ot') + send_keys_to_filtered_search('@oot') expect(dropdown_author_size).to eq(2) end it 'filters by username without symbol' do - send_keys_to_filtered_search('ot') + send_keys_to_filtered_search('oot') expect(dropdown_author_size).to eq(2) end it 'filters by case insensitive username without symbol' do - send_keys_to_filtered_search('OT') + send_keys_to_filtered_search('OOT') expect(dropdown_author_size).to eq(2) end diff --git a/spec/lib/gitlab/sql/pattern_spec.rb b/spec/lib/gitlab/sql/pattern_spec.rb new file mode 100644 index 00000000000..cbafe36de06 --- /dev/null +++ b/spec/lib/gitlab/sql/pattern_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe Gitlab::SQL::Pattern do + describe '#to_sql' do + subject(:to_sql) { described_class.new(query).to_sql } + + context 'when a query is shorter than 3 chars' do + let(:query) { '12' } + + it 'returns exact matching pattern' do + expect(to_sql).to eq('12') + end + end + + context 'when a query is equal to 3 chars' do + let(:query) { '123' } + + it 'returns partial matching pattern' do + expect(to_sql).to eq('%123%') + end + end + + context 'when a query is longer than 3 chars' do + let(:query) { '1234' } + + it 'returns partial matching pattern' do + expect(to_sql).to eq('%1234%') + end + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 9a9e255f874..50abd7af429 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -789,6 +789,7 @@ describe User do describe '.search' do let!(:user) { create(:user, name: 'user', username: 'usern', email: 'email@gmail.com') } let!(:user2) { create(:user, name: 'user name', username: 'username', email: 'someemail@gmail.com') } + let!(:user3) { create(:user, name: 'us', username: 'se', email: 'foo@gmail.com') } describe 'name matching' do it 'returns users with a matching name with exact match first' do @@ -802,6 +803,14 @@ describe User do it 'returns users with a matching name regardless of the casing' do expect(described_class.search(user2.name.upcase)).to eq([user2]) end + + it 'returns users with a exact matching name shorter than 3 chars' do + expect(described_class.search(user3.name)).to eq([user3]) + end + + it 'returns users with a exact matching name shorter than 3 chars regardless of the casing' do + expect(described_class.search(user3.name.upcase)).to eq([user3]) + end end describe 'email matching' do @@ -830,6 +839,14 @@ describe User do it 'returns users with a matching username regardless of the casing' do expect(described_class.search(user2.username.upcase)).to eq([user2]) end + + it 'returns users with a exact matching username shorter than 3 chars' do + expect(described_class.search(user3.username)).to eq([user3]) + end + + it 'returns users with a exact matching username shorter than 3 chars regardless of the casing' do + expect(described_class.search(user3.username.upcase)).to eq([user3]) + end end end From 866aab7f2a92f9929a5c5811d3d3c23c11184b26 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Sat, 26 Aug 2017 22:32:55 +0900 Subject: [PATCH 05/70] Fix escape characters was not sanitized --- lib/gitlab/sql/pattern.rb | 9 +++++++-- spec/lib/gitlab/sql/pattern_spec.rb | 24 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb index 47ea19994a2..46c973d8a11 100644 --- a/lib/gitlab/sql/pattern.rb +++ b/lib/gitlab/sql/pattern.rb @@ -11,9 +11,9 @@ module Gitlab def to_sql if exact_matching? - query + sanitized_query else - "%#{query}%" + "%#{sanitized_query}%" end end @@ -24,6 +24,11 @@ module Gitlab def partial_matching? @query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING end + + def sanitized_query + # Note: ActiveRecord::Base.sanitize_sql_like is a protected method + ActiveRecord::Base.__send__(:sanitize_sql_like, query) + end end end end diff --git a/spec/lib/gitlab/sql/pattern_spec.rb b/spec/lib/gitlab/sql/pattern_spec.rb index cbafe36de06..d0412f37098 100644 --- a/spec/lib/gitlab/sql/pattern_spec.rb +++ b/spec/lib/gitlab/sql/pattern_spec.rb @@ -12,6 +12,14 @@ describe Gitlab::SQL::Pattern do end end + context 'when a query with a escape character is shorter than 3 chars' do + let(:query) { '_2' } + + it 'returns sanitized exact matching pattern' do + expect(to_sql).to eq('\_2') + end + end + context 'when a query is equal to 3 chars' do let(:query) { '123' } @@ -20,6 +28,14 @@ describe Gitlab::SQL::Pattern do end end + context 'when a query with a escape character is equal to 3 chars' do + let(:query) { '_23' } + + it 'returns partial matching pattern' do + expect(to_sql).to eq('%\_23%') + end + end + context 'when a query is longer than 3 chars' do let(:query) { '1234' } @@ -27,5 +43,13 @@ describe Gitlab::SQL::Pattern do expect(to_sql).to eq('%1234%') end end + + context 'when a query with a escape character is longer than 3 chars' do + let(:query) { '_234' } + + it 'returns sanitized partial matching pattern' do + expect(to_sql).to eq('%\_234%') + end + end end end From 54b3908cd1fac68318007e6800305ece6013727e Mon Sep 17 00:00:00 2001 From: winh Date: Thu, 17 Aug 2017 10:42:18 +0200 Subject: [PATCH 06/70] Make notification dropdowns consistent --- app/assets/stylesheets/pages/notifications.scss | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/assets/stylesheets/pages/notifications.scss b/app/assets/stylesheets/pages/notifications.scss index bdf07a99daf..c28b1e68008 100644 --- a/app/assets/stylesheets/pages/notifications.scss +++ b/app/assets/stylesheets/pages/notifications.scss @@ -14,3 +14,7 @@ font-size: 18px; } } + +.notification-form { + @include new-style-dropdown; +} From 53fd93220c19aec495212e848979293da41164ba Mon Sep 17 00:00:00 2001 From: winh Date: Tue, 15 Aug 2017 22:39:19 +0200 Subject: [PATCH 07/70] Make file template dropdowns consistent --- app/assets/stylesheets/pages/editor.scss | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/assets/stylesheets/pages/editor.scss b/app/assets/stylesheets/pages/editor.scss index f6b8c8ee2bc..d3cd4d507de 100644 --- a/app/assets/stylesheets/pages/editor.scss +++ b/app/assets/stylesheets/pages/editor.scss @@ -204,6 +204,8 @@ .gitlab-ci-yml-selector, .dockerfile-selector, .template-type-selector { + @include new-style-dropdown; + display: inline-block; vertical-align: top; font-family: $regular_font; From b7316ffdff561cc16a003c5540f2c6e627818a2e Mon Sep 17 00:00:00 2001 From: Kai Kontio Date: Mon, 28 Aug 2017 11:47:09 +0000 Subject: [PATCH 08/70] Update backup_restore.md documentation regarding S3 and IAM profiles. https://gitlab.com/gitlab-org/gitlab-ce/commit/7f8ef19c411eceec207fef5d8459fbeaa40bf464 most likely broke the old configuration format where empty string aws_access_key_id & aws_secret_access_key were still OK. From 9.5 onwards moving backups to S3 using IAM profiles will fail if they are configured. --- doc/raketasks/backup_restore.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/doc/raketasks/backup_restore.md b/doc/raketasks/backup_restore.md index 10f5ab3370d..ae69d7f92f2 100644 --- a/doc/raketasks/backup_restore.md +++ b/doc/raketasks/backup_restore.md @@ -144,9 +144,8 @@ gitlab_rails['backup_upload_connection'] = { 'region' => 'eu-west-1', 'aws_access_key_id' => 'AKIAKIAKI', 'aws_secret_access_key' => 'secret123' - # If using an IAM Profile, leave aws_access_key_id & aws_secret_access_key empty - # ie. 'aws_access_key_id' => '', - # 'use_iam_profile' => 'true' + # If using an IAM Profile, don't configure aws_access_key_id & aws_secret_access_key + # 'use_iam_profile' => true } gitlab_rails['backup_upload_remote_directory'] = 'my.s3.bucket' ``` From 998afa5f74558be215a924d95aa131a69831ca43 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Wed, 1 Mar 2017 14:35:48 +0100 Subject: [PATCH 09/70] API: Respect the 'If-Unmodified-Since' for delete endpoints --- lib/api/access_requests.rb | 3 +++ lib/api/award_emoji.rb | 1 + lib/api/boards.rb | 1 + lib/api/broadcast_messages.rb | 1 + lib/api/deploy_keys.rb | 2 ++ lib/api/environments.rb | 1 + lib/api/groups.rb | 2 ++ lib/api/helpers.rb | 8 ++++++++ lib/api/issues.rb | 2 ++ lib/api/labels.rb | 2 ++ lib/api/members.rb | 3 ++- lib/api/merge_requests.rb | 2 ++ lib/api/notes.rb | 2 ++ lib/api/project_hooks.rb | 2 ++ lib/api/project_snippets.rb | 2 ++ lib/api/projects.rb | 2 ++ lib/api/runners.rb | 2 ++ lib/api/services.rb | 2 ++ lib/api/snippets.rb | 1 + lib/api/system_hooks.rb | 2 ++ lib/api/triggers.rb | 2 ++ lib/api/users.rb | 28 ++++++++++++++++++++++++++++ 22 files changed, 72 insertions(+), 1 deletion(-) diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb index cdacf9839e5..0c5b8862d79 100644 --- a/lib/api/access_requests.rb +++ b/lib/api/access_requests.rb @@ -67,6 +67,9 @@ module API end delete ":id/access_requests/:user_id" do source = find_source(source_type, params[:id]) + member = source.public_send(:requesters).find_by!(user_id: params[:user_id]) + + check_unmodified_since(member.updated_at) status 204 ::Members::DestroyService.new(source, current_user, params) diff --git a/lib/api/award_emoji.rb b/lib/api/award_emoji.rb index 5a028fc9d0b..51a8587d26e 100644 --- a/lib/api/award_emoji.rb +++ b/lib/api/award_emoji.rb @@ -85,6 +85,7 @@ module API end delete "#{endpoint}/:award_id" do award = awardable.award_emoji.find(params[:award_id]) + check_unmodified_since(award.updated_at) unauthorized! unless award.user == current_user || current_user.admin? diff --git a/lib/api/boards.rb b/lib/api/boards.rb index 5a2d7a681e3..d36df77dc6c 100644 --- a/lib/api/boards.rb +++ b/lib/api/boards.rb @@ -124,6 +124,7 @@ module API authorize!(:admin_list, user_project) list = board_lists.find(params[:list_id]) + check_unmodified_since(list.updated_at) service = ::Boards::Lists::DestroyService.new(user_project, current_user) diff --git a/lib/api/broadcast_messages.rb b/lib/api/broadcast_messages.rb index 9980aec4752..352972b584a 100644 --- a/lib/api/broadcast_messages.rb +++ b/lib/api/broadcast_messages.rb @@ -90,6 +90,7 @@ module API end delete ':id' do message = find_message + check_unmodified_since(message.updated_at) status 204 message.destroy diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index 42e7c1486b0..971cc816454 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -125,6 +125,8 @@ module API key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id]) not_found!('Deploy Key') unless key + check_unmodified_since(key.updated_at) + status 204 key.destroy end diff --git a/lib/api/environments.rb b/lib/api/environments.rb index c774a5c6685..3fc423ae79a 100644 --- a/lib/api/environments.rb +++ b/lib/api/environments.rb @@ -78,6 +78,7 @@ module API authorize! :update_environment, user_project environment = user_project.environments.find(params[:environment_id]) + check_unmodified_since(environment.updated_at) status 204 environment.destroy diff --git a/lib/api/groups.rb b/lib/api/groups.rb index e56427304a6..c9b32a85487 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -117,6 +117,8 @@ module API delete ":id" do group = find_group!(params[:id]) authorize! :admin_group, group + + check_unmodified_since(group.updated_at) status 204 ::Groups::DestroyService.new(group, current_user).execute diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index b56fd2388b3..1c74a14d91c 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -11,6 +11,14 @@ module API declared(params, options).to_h.symbolize_keys end + def check_unmodified_since(last_modified) + if_unmodified_since = Time.parse(headers['If-Unmodified-Since']) if headers.key?('If-Unmodified-Since') rescue nil + + if if_unmodified_since && if_unmodified_since < last_modified + render_api_error!('412 Precondition Failed', 412) + end + end + def current_user return @current_user if defined?(@current_user) diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 4cec1145f3a..cee9898d3a6 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -230,6 +230,8 @@ module API not_found!('Issue') unless issue authorize!(:destroy_issue, issue) + check_unmodified_since(issue.updated_at) + status 204 issue.destroy end diff --git a/lib/api/labels.rb b/lib/api/labels.rb index 4520c98d951..45fa57fdf55 100644 --- a/lib/api/labels.rb +++ b/lib/api/labels.rb @@ -56,6 +56,8 @@ module API label = user_project.labels.find_by(title: params[:name]) not_found!('Label') unless label + check_unmodified_since(label.updated_at) + status 204 label.destroy end diff --git a/lib/api/members.rb b/lib/api/members.rb index bb970b7cd54..5634f123eca 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -94,7 +94,8 @@ module API delete ":id/members/:user_id" do source = find_source(source_type, params[:id]) # Ensure that memeber exists - source.members.find_by!(user_id: params[:user_id]) + member = source.members.find_by!(user_id: params[:user_id]) + check_unmodified_since(member.updated_at) status 204 ::Members::DestroyService.new(source, current_user, declared_params).execute diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 8810d4e441d..c6fecc1aa6c 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -164,6 +164,8 @@ module API merge_request = find_project_merge_request(params[:merge_request_iid]) authorize!(:destroy_merge_request, merge_request) + check_unmodified_since(merge_request.updated_at) + status 204 merge_request.destroy end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 4e4e473994b..58d71787aca 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -129,7 +129,9 @@ module API end delete ":id/#{noteables_str}/:noteable_id/notes/:note_id" do note = user_project.notes.find(params[:note_id]) + authorize! :admin_note, note + check_unmodified_since(note.updated_at) status 204 ::Notes::DestroyService.new(user_project, current_user).execute(note) diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb index 649dd891f56..74d736fda59 100644 --- a/lib/api/project_hooks.rb +++ b/lib/api/project_hooks.rb @@ -96,6 +96,8 @@ module API delete ":id/hooks/:hook_id" do hook = user_project.hooks.find(params.delete(:hook_id)) + check_unmodified_since(hook.updated_at) + status 204 hook.destroy end diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb index f3d905b0068..645162d564d 100644 --- a/lib/api/project_snippets.rb +++ b/lib/api/project_snippets.rb @@ -116,6 +116,8 @@ module API not_found!('Snippet') unless snippet authorize! :admin_project_snippet, snippet + check_unmodified_since(snippet.updated_at) + status 204 snippet.destroy end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 15c3832b032..eab0ca0b3c9 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -334,6 +334,8 @@ module API desc 'Remove a project' delete ":id" do authorize! :remove_project, user_project + check_unmodified_since(user_project.updated_at) + ::Projects::DestroyService.new(user_project, current_user, {}).async_execute accepted! diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 31f940fe96b..e3b2eb904b7 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -77,7 +77,9 @@ module API end delete ':id' do runner = get_runner(params[:id]) + authenticate_delete_runner!(runner) + check_unmodified_since(runner.updated_at) status 204 runner.destroy! diff --git a/lib/api/services.rb b/lib/api/services.rb index 843c05ae32e..4fef3383e5e 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -655,6 +655,8 @@ module API end delete ":id/services/:service_slug" do service = user_project.find_or_initialize_service(params[:service_slug].underscore) + # Todo, not sure + check_unmodified_since(service.updated_at) attrs = service_attributes(service).inject({}) do |hash, key| hash.merge!(key => nil) diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb index 35ece56c65c..7107b3d669c 100644 --- a/lib/api/snippets.rb +++ b/lib/api/snippets.rb @@ -122,6 +122,7 @@ module API return not_found!('Snippet') unless snippet authorize! :destroy_personal_snippet, snippet + check_unmodified_since(snippet.updated_at) status 204 snippet.destroy diff --git a/lib/api/system_hooks.rb b/lib/api/system_hooks.rb index c0179037440..64066a75b15 100644 --- a/lib/api/system_hooks.rb +++ b/lib/api/system_hooks.rb @@ -66,6 +66,8 @@ module API hook = SystemHook.find_by(id: params[:id]) not_found!('System hook') unless hook + check_unmodified_since(hook.updated_at) + status 204 hook.destroy end diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index edfdb63d183..4ae70c65759 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -140,6 +140,8 @@ module API trigger = user_project.triggers.find(params.delete(:trigger_id)) return not_found!('Trigger') unless trigger + check_unmodified_since(trigger.updated_at) + status 204 trigger.destroy end diff --git a/lib/api/users.rb b/lib/api/users.rb index e2019d6d512..942bb72cf97 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -230,7 +230,12 @@ module API key = user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key +<<<<<<< HEAD status 204 +======= + check_unmodified_since(key.updated_at) + +>>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints key.destroy end @@ -287,7 +292,14 @@ module API email = user.emails.find_by(id: params[:email_id]) not_found!('Email') unless email +<<<<<<< HEAD Emails::DestroyService.new(user, email: email.email).execute +======= + check_unmodified_since(email.updated_at) + + email.destroy + user.update_secondary_emails! +>>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints end desc 'Delete a user. Available only for admins.' do @@ -299,11 +311,18 @@ module API end delete ":id" do authenticated_as_admin! + user = User.find_by(id: params[:id]) not_found!('User') unless user +<<<<<<< HEAD status 204 user.delete_async(deleted_by: current_user, params: params) +======= + check_unmodified_since(user.updated_at) + + ::Users::DestroyService.new(current_user).execute(user) +>>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints end desc 'Block a user. Available only for admins.' @@ -481,6 +500,8 @@ module API key = current_user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key + check_unmodified_since(key.updated_at) + status 204 key.destroy end @@ -533,6 +554,7 @@ module API email = current_user.emails.find_by(id: params[:email_id]) not_found!('Email') unless email +<<<<<<< HEAD status 204 Emails::DestroyService.new(current_user, email: email.email).execute end @@ -550,6 +572,12 @@ module API .reorder(last_activity_on: :asc) present paginate(activities), with: Entities::UserActivity +======= + check_unmodified_since(email.updated_at) + + email.destroy + current_user.update_secondary_emails! +>>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints end end end From e80313f9ee5b3495a8713e6ddae111bc8106155b Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Thu, 2 Mar 2017 13:14:13 +0100 Subject: [PATCH 10/70] Conditionally destroy a ressource --- lib/api/access_requests.rb | 11 ++++---- lib/api/award_emoji.rb | 4 +-- lib/api/boards.rb | 11 ++++---- lib/api/broadcast_messages.rb | 4 +-- lib/api/deploy_keys.rb | 5 +--- lib/api/environments.rb | 4 +-- lib/api/groups.rb | 7 +++-- lib/api/helpers.rb | 17 +++++++++--- lib/api/issues.rb | 4 +-- lib/api/labels.rb | 5 +--- lib/api/members.rb | 7 +++-- lib/api/merge_requests.rb | 4 +-- lib/api/notes.rb | 6 ++--- lib/api/project_hooks.rb | 5 +--- lib/api/project_snippets.rb | 4 +-- lib/api/projects.rb | 5 ++-- lib/api/runner.rb | 6 +++-- lib/api/runners.rb | 7 ++--- lib/api/services.rb | 4 +-- lib/api/snippets.rb | 4 +-- lib/api/system_hooks.rb | 5 +--- lib/api/triggers.rb | 5 +--- lib/api/users.rb | 47 ++++++++++------------------------ spec/requests/api/tags_spec.rb | 4 +-- 24 files changed, 71 insertions(+), 114 deletions(-) diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb index 0c5b8862d79..4fa9b2b2494 100644 --- a/lib/api/access_requests.rb +++ b/lib/api/access_requests.rb @@ -67,13 +67,12 @@ module API end delete ":id/access_requests/:user_id" do source = find_source(source_type, params[:id]) - member = source.public_send(:requesters).find_by!(user_id: params[:user_id]) + member = source.requesters.find_by!(user_id: params[:user_id]) - check_unmodified_since(member.updated_at) - - status 204 - ::Members::DestroyService.new(source, current_user, params) - .execute(:requesters) + destroy_conditionally!(member) do + ::Members::DestroyService.new(source, current_user, params) + .execute(:requesters) + end end end end diff --git a/lib/api/award_emoji.rb b/lib/api/award_emoji.rb index 51a8587d26e..8e3851640da 100644 --- a/lib/api/award_emoji.rb +++ b/lib/api/award_emoji.rb @@ -85,12 +85,10 @@ module API end delete "#{endpoint}/:award_id" do award = awardable.award_emoji.find(params[:award_id]) - check_unmodified_since(award.updated_at) unauthorized! unless award.user == current_user || current_user.admin? - status 204 - award.destroy + destroy_conditionally!(award) end end end diff --git a/lib/api/boards.rb b/lib/api/boards.rb index d36df77dc6c..0d11c5fc971 100644 --- a/lib/api/boards.rb +++ b/lib/api/boards.rb @@ -122,14 +122,13 @@ module API end delete "/lists/:list_id" do authorize!(:admin_list, user_project) - list = board_lists.find(params[:list_id]) - check_unmodified_since(list.updated_at) - service = ::Boards::Lists::DestroyService.new(user_project, current_user) - - unless service.execute(list) - render_api_error!({ error: 'List could not be deleted!' }, 400) + destroy_conditionally!(list) do |list| + service = ::Boards::Lists::DestroyService.new(user_project, current_user) + unless service.execute(list) + render_api_error!({ error: 'List could not be deleted!' }, 400) + end end end end diff --git a/lib/api/broadcast_messages.rb b/lib/api/broadcast_messages.rb index 352972b584a..0b45621ce7b 100644 --- a/lib/api/broadcast_messages.rb +++ b/lib/api/broadcast_messages.rb @@ -90,10 +90,8 @@ module API end delete ':id' do message = find_message - check_unmodified_since(message.updated_at) - status 204 - message.destroy + destroy_conditionally!(message) end end end diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index 971cc816454..f405c341398 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -125,10 +125,7 @@ module API key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id]) not_found!('Deploy Key') unless key - check_unmodified_since(key.updated_at) - - status 204 - key.destroy + destroy_conditionally!(key) end end end diff --git a/lib/api/environments.rb b/lib/api/environments.rb index 3fc423ae79a..e33269f9483 100644 --- a/lib/api/environments.rb +++ b/lib/api/environments.rb @@ -78,10 +78,8 @@ module API authorize! :update_environment, user_project environment = user_project.environments.find(params[:environment_id]) - check_unmodified_since(environment.updated_at) - status 204 - environment.destroy + destroy_conditionally!(environment) end desc 'Stops an existing environment' do diff --git a/lib/api/groups.rb b/lib/api/groups.rb index c9b32a85487..ee2ad27837b 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -117,11 +117,10 @@ module API delete ":id" do group = find_group!(params[:id]) authorize! :admin_group, group - - check_unmodified_since(group.updated_at) - status 204 - ::Groups::DestroyService.new(group, current_user).execute + destroy_conditionally!(group) do |group| + ::Groups::DestroyService.new(group, current_user).execute + end end desc 'Get a list of projects in this group.' do diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 1c74a14d91c..8d4f8c01903 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -11,14 +11,25 @@ module API declared(params, options).to_h.symbolize_keys end - def check_unmodified_since(last_modified) - if_unmodified_since = Time.parse(headers['If-Unmodified-Since']) if headers.key?('If-Unmodified-Since') rescue nil + def check_unmodified_since!(last_modified) + if_unmodified_since = Time.parse(headers['If-Unmodified-Since']) rescue nil - if if_unmodified_since && if_unmodified_since < last_modified + if if_unmodified_since && last_modified > if_unmodified_since render_api_error!('412 Precondition Failed', 412) end end + def destroy_conditionally!(resource, last_update_field: :updated_at) + check_unmodified_since!(resource.public_send(last_update_field)) + + status 204 + if block_given? + yield resource + else + resource.destroy + end + end + def current_user return @current_user if defined?(@current_user) diff --git a/lib/api/issues.rb b/lib/api/issues.rb index cee9898d3a6..6503629e2a2 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -230,10 +230,8 @@ module API not_found!('Issue') unless issue authorize!(:destroy_issue, issue) - check_unmodified_since(issue.updated_at) - status 204 - issue.destroy + destroy_conditionally!(issue) end desc 'List merge requests closing issue' do diff --git a/lib/api/labels.rb b/lib/api/labels.rb index 45fa57fdf55..c0cf618ee8d 100644 --- a/lib/api/labels.rb +++ b/lib/api/labels.rb @@ -56,10 +56,7 @@ module API label = user_project.labels.find_by(title: params[:name]) not_found!('Label') unless label - check_unmodified_since(label.updated_at) - - status 204 - label.destroy + destroy_conditionally!(label) end desc 'Update an existing label. At least one optional parameter is required.' do diff --git a/lib/api/members.rb b/lib/api/members.rb index 5634f123eca..a5d3d7f25a0 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -93,12 +93,11 @@ module API end delete ":id/members/:user_id" do source = find_source(source_type, params[:id]) - # Ensure that memeber exists member = source.members.find_by!(user_id: params[:user_id]) - check_unmodified_since(member.updated_at) - status 204 - ::Members::DestroyService.new(source, current_user, declared_params).execute + destroy_conditionally!(member) do + ::Members::DestroyService.new(source, current_user, declared_params).execute + end end end end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index c6fecc1aa6c..969c6064662 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -164,10 +164,8 @@ module API merge_request = find_project_merge_request(params[:merge_request_iid]) authorize!(:destroy_merge_request, merge_request) - check_unmodified_since(merge_request.updated_at) - status 204 - merge_request.destroy + destroy_conditionally!(merge_request) end params do diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 58d71787aca..e116448c15b 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -131,10 +131,10 @@ module API note = user_project.notes.find(params[:note_id]) authorize! :admin_note, note - check_unmodified_since(note.updated_at) - status 204 - ::Notes::DestroyService.new(user_project, current_user).execute(note) + destroy_conditionally!(note) do |note| + ::Notes::DestroyService.new(user_project, current_user).execute(note) + end end end end diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb index 74d736fda59..5b457bbe639 100644 --- a/lib/api/project_hooks.rb +++ b/lib/api/project_hooks.rb @@ -96,10 +96,7 @@ module API delete ":id/hooks/:hook_id" do hook = user_project.hooks.find(params.delete(:hook_id)) - check_unmodified_since(hook.updated_at) - - status 204 - hook.destroy + destroy_conditionally!(hook) end end end diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb index 645162d564d..704e8c6718d 100644 --- a/lib/api/project_snippets.rb +++ b/lib/api/project_snippets.rb @@ -116,10 +116,8 @@ module API not_found!('Snippet') unless snippet authorize! :admin_project_snippet, snippet - check_unmodified_since(snippet.updated_at) - status 204 - snippet.destroy + destroy_conditionally!(snippet) end desc 'Get a raw project snippet' diff --git a/lib/api/projects.rb b/lib/api/projects.rb index eab0ca0b3c9..36fe3f243b9 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -334,9 +334,10 @@ module API desc 'Remove a project' delete ":id" do authorize! :remove_project, user_project - check_unmodified_since(user_project.updated_at) - ::Projects::DestroyService.new(user_project, current_user, {}).async_execute + destroy_conditionally!(user_project) do + ::Projects::DestroyService.new(user_project, current_user, {}).async_execute + end accepted! end diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 88fc62d33df..daa8ddbe251 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -45,8 +45,10 @@ module API end delete '/' do authenticate_runner! - status 204 - Ci::Runner.find_by_token(params[:token]).destroy + + runner = Ci::Runner.find_by_token(params[:token]) + + destroy_conditionally!(runner) end desc 'Validates authentication credentials' do diff --git a/lib/api/runners.rb b/lib/api/runners.rb index e3b2eb904b7..68c2120cc15 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -79,10 +79,8 @@ module API runner = get_runner(params[:id]) authenticate_delete_runner!(runner) - check_unmodified_since(runner.updated_at) - status 204 - runner.destroy! + destroy_conditionally!(runner) end end @@ -137,8 +135,7 @@ module API runner = runner_project.runner forbidden!("Only one project associated with the runner. Please remove the runner instead") if runner.projects.count == 1 - status 204 - runner_project.destroy + destroy_conditionally!(runner_project) end end diff --git a/lib/api/services.rb b/lib/api/services.rb index 4fef3383e5e..2b5ef75b6bf 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -655,8 +655,8 @@ module API end delete ":id/services/:service_slug" do service = user_project.find_or_initialize_service(params[:service_slug].underscore) - # Todo, not sure - check_unmodified_since(service.updated_at) + # Todo: Check if this done the right way + check_unmodified_since!(service.updated_at) attrs = service_attributes(service).inject({}) do |hash, key| hash.merge!(key => nil) diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb index 7107b3d669c..00eb7c60f16 100644 --- a/lib/api/snippets.rb +++ b/lib/api/snippets.rb @@ -122,10 +122,8 @@ module API return not_found!('Snippet') unless snippet authorize! :destroy_personal_snippet, snippet - check_unmodified_since(snippet.updated_at) - status 204 - snippet.destroy + destroy_conditionally!(snippet) end desc 'Get a raw snippet' do diff --git a/lib/api/system_hooks.rb b/lib/api/system_hooks.rb index 64066a75b15..6b6a03e3300 100644 --- a/lib/api/system_hooks.rb +++ b/lib/api/system_hooks.rb @@ -66,10 +66,7 @@ module API hook = SystemHook.find_by(id: params[:id]) not_found!('System hook') unless hook - check_unmodified_since(hook.updated_at) - - status 204 - hook.destroy + destroy_conditionally!(hook) end end end diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 4ae70c65759..c9fee7e5193 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -140,10 +140,7 @@ module API trigger = user_project.triggers.find(params.delete(:trigger_id)) return not_found!('Trigger') unless trigger - check_unmodified_since(trigger.updated_at) - - status 204 - trigger.destroy + destroy_conditionally!(trigger) end end end diff --git a/lib/api/users.rb b/lib/api/users.rb index 942bb72cf97..d7c7b9ae9c1 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -230,13 +230,7 @@ module API key = user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key -<<<<<<< HEAD - status 204 -======= - check_unmodified_since(key.updated_at) - ->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints - key.destroy + destroy_conditionally!(key) end desc 'Add an email address to a specified user. Available only for admins.' do @@ -292,14 +286,11 @@ module API email = user.emails.find_by(id: params[:email_id]) not_found!('Email') unless email -<<<<<<< HEAD - Emails::DestroyService.new(user, email: email.email).execute -======= - check_unmodified_since(email.updated_at) + destroy_conditionally!(email) do |email| + Emails::DestroyService.new(current_user, email: email.email).execute + end - email.destroy user.update_secondary_emails! ->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints end desc 'Delete a user. Available only for admins.' do @@ -315,14 +306,9 @@ module API user = User.find_by(id: params[:id]) not_found!('User') unless user -<<<<<<< HEAD - status 204 - user.delete_async(deleted_by: current_user, params: params) -======= - check_unmodified_since(user.updated_at) - - ::Users::DestroyService.new(current_user).execute(user) ->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints + destroy_conditionally!(user) do + user.delete_async(deleted_by: current_user, params: params) + end end desc 'Block a user. Available only for admins.' @@ -500,10 +486,7 @@ module API key = current_user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key - check_unmodified_since(key.updated_at) - - status 204 - key.destroy + destroy_conditionally!(key) end desc "Get the currently authenticated user's email addresses" do @@ -554,9 +537,11 @@ module API email = current_user.emails.find_by(id: params[:email_id]) not_found!('Email') unless email -<<<<<<< HEAD - status 204 - Emails::DestroyService.new(current_user, email: email.email).execute + destroy_conditionally!(email) do |email| + Emails::DestroyService.new(current_user, email: email.email).execute + end + + current_user.update_secondary_emails! end desc 'Get a list of user activities' @@ -572,12 +557,6 @@ module API .reorder(last_activity_on: :asc) present paginate(activities), with: Entities::UserActivity -======= - check_unmodified_since(email.updated_at) - - email.destroy - current_user.update_secondary_emails! ->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints end end end diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb index 9884c1ec206..54900c75eb8 100644 --- a/spec/requests/api/tags_spec.rb +++ b/spec/requests/api/tags_spec.rb @@ -283,7 +283,7 @@ describe API::Tags do it_behaves_like '404 response' do let(:request) { delete api(route, current_user) } - let(:message) { 'No such tag' } + let(:message) { '404 Tag Not Found' } end end @@ -394,7 +394,7 @@ describe API::Tags do it_behaves_like '404 response' do let(:request) { put api(route, current_user), description: new_description } - let(:message) { 'Tag does not exist' } + let(:message) { '404 Tag Not Found' } end end From f0f3f38576c0691e6d0e751c962382beea998afb Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Thu, 2 Mar 2017 14:21:36 +0100 Subject: [PATCH 11/70] Use commit date for branches and tags --- lib/api/branches.rb | 15 +++++++++++---- lib/api/tags.rb | 15 +++++++++++---- spec/requests/api/tags_spec.rb | 2 +- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/lib/api/branches.rb b/lib/api/branches.rb index d3dbf941298..b87f7cdbad1 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -125,11 +125,18 @@ module API delete ':id/repository/branches/:branch', requirements: BRANCH_ENDPOINT_REQUIREMENTS do authorize_push_project - result = DeleteBranchService.new(user_project, current_user) - .execute(params[:branch]) + branch = user_project.repository.find_branch(params[:branch]) + not_found!('Branch') unless branch - if result[:status] != :success - render_api_error!(result[:message], result[:return_code]) + commit = user_project.repository.commit(branch.dereferenced_target) + + destroy_conditionally!(commit, last_update_field: :authored_date) do + result = DeleteBranchService.new(user_project, current_user) + .execute(params[:branch]) + + if result[:status] != :success + render_api_error!(result[:message], result[:return_code]) + end end end diff --git a/lib/api/tags.rb b/lib/api/tags.rb index 1333747cced..81b17935b81 100644 --- a/lib/api/tags.rb +++ b/lib/api/tags.rb @@ -65,11 +65,18 @@ module API delete ':id/repository/tags/:tag_name', requirements: TAG_ENDPOINT_REQUIREMENTS do authorize_push_project - result = ::Tags::DestroyService.new(user_project, current_user) - .execute(params[:tag_name]) + tag = user_project.repository.find_tag(params[:tag_name]) + not_found!('Tag') unless tag - if result[:status] != :success - render_api_error!(result[:message], result[:return_code]) + commit = user_project.repository.commit(tag.dereferenced_target) + + destroy_conditionally!(commit, last_update_field: :authored_date) do + result = ::Tags::DestroyService.new(user_project, current_user) + .execute(params[:tag_name]) + + if result[:status] != :success + render_api_error!(result[:message], result[:return_code]) + end end end diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb index 54900c75eb8..6ac0caa7fe8 100644 --- a/spec/requests/api/tags_spec.rb +++ b/spec/requests/api/tags_spec.rb @@ -394,7 +394,7 @@ describe API::Tags do it_behaves_like '404 response' do let(:request) { put api(route, current_user), description: new_description } - let(:message) { '404 Tag Not Found' } + let(:message) { 'Tag does not exist' } end end From dcd4ea473cab20eee05995ecaca043da98ca5a8d Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Thu, 24 Aug 2017 10:41:54 +0200 Subject: [PATCH 12/70] Update remaining endpoints --- lib/api/group_variables.rb | 3 +-- lib/api/helpers.rb | 2 +- lib/api/pipeline_schedules.rb | 3 +-- lib/api/projects.rb | 1 - lib/api/protected_branches.rb | 4 +--- lib/api/services.rb | 14 +++++++------- lib/api/users.rb | 7 +++++-- lib/api/variables.rb | 1 + spec/requests/api/pipeline_schedules_spec.rb | 3 +-- 9 files changed, 18 insertions(+), 20 deletions(-) diff --git a/lib/api/group_variables.rb b/lib/api/group_variables.rb index f64da4ab77b..25152f30998 100644 --- a/lib/api/group_variables.rb +++ b/lib/api/group_variables.rb @@ -88,8 +88,7 @@ module API variable = user_group.variables.find_by(key: params[:key]) not_found!('GroupVariable') unless variable - status 204 - variable.destroy + destroy_conditionally!(variable) end end end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 8d4f8c01903..84980864151 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -14,7 +14,7 @@ module API def check_unmodified_since!(last_modified) if_unmodified_since = Time.parse(headers['If-Unmodified-Since']) rescue nil - if if_unmodified_since && last_modified > if_unmodified_since + if if_unmodified_since && last_modified && last_modified > if_unmodified_since render_api_error!('412 Precondition Failed', 412) end end diff --git a/lib/api/pipeline_schedules.rb b/lib/api/pipeline_schedules.rb index dbeaf9e17ef..e3123ef4e2d 100644 --- a/lib/api/pipeline_schedules.rb +++ b/lib/api/pipeline_schedules.rb @@ -117,8 +117,7 @@ module API not_found!('PipelineSchedule') unless pipeline_schedule authorize! :admin_pipeline_schedule, pipeline_schedule - status :accepted - present pipeline_schedule.destroy, with: Entities::PipelineScheduleDetails + destroy_conditionally!(pipeline_schedule) end end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 36fe3f243b9..78e25b6da03 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -1,7 +1,6 @@ require_dependency 'declarative_policy' module API - # Projects API class Projects < Grape::API include PaginationParams diff --git a/lib/api/protected_branches.rb b/lib/api/protected_branches.rb index dccf4fa27a7..15fcb9e8e27 100644 --- a/lib/api/protected_branches.rb +++ b/lib/api/protected_branches.rb @@ -76,9 +76,7 @@ module API delete ':id/protected_branches/:name', requirements: BRANCH_ENDPOINT_REQUIREMENTS do protected_branch = user_project.protected_branches.find_by!(name: params[:name]) - protected_branch.destroy - - status 204 + destroy_conditionally!(protected_branch) end end end diff --git a/lib/api/services.rb b/lib/api/services.rb index 2b5ef75b6bf..ff9ddd44439 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -655,15 +655,15 @@ module API end delete ":id/services/:service_slug" do service = user_project.find_or_initialize_service(params[:service_slug].underscore) - # Todo: Check if this done the right way - check_unmodified_since!(service.updated_at) - attrs = service_attributes(service).inject({}) do |hash, key| - hash.merge!(key => nil) - end + destroy_conditionally!(service) do + attrs = service_attributes(service).inject({}) do |hash, key| + hash.merge!(key => nil) + end - unless service.update_attributes(attrs.merge(active: false)) - render_api_error!('400 Bad Request', 400) + unless service.update_attributes(attrs.merge(active: false)) + render_api_error!('400 Bad Request', 400) + end end end diff --git a/lib/api/users.rb b/lib/api/users.rb index d7c7b9ae9c1..96f47bb618a 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -408,8 +408,11 @@ module API requires :impersonation_token_id, type: Integer, desc: 'The ID of the impersonation token' end delete ':impersonation_token_id' do - status 204 - find_impersonation_token.revoke! + token = find_impersonation_token + + destroy_conditionally!(token) do + token.revoke! + end end end end diff --git a/lib/api/variables.rb b/lib/api/variables.rb index 7c0fdd3d1be..da71787abab 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -88,6 +88,7 @@ module API variable = user_project.variables.find_by(key: params[:key]) not_found!('Variable') unless variable + # Variables don't have any timestamp. Therfore, destroy unconditionally. status 204 variable.destroy end diff --git a/spec/requests/api/pipeline_schedules_spec.rb b/spec/requests/api/pipeline_schedules_spec.rb index 1fc0ec528b9..150a391bb8d 100644 --- a/spec/requests/api/pipeline_schedules_spec.rb +++ b/spec/requests/api/pipeline_schedules_spec.rb @@ -267,8 +267,7 @@ describe API::PipelineSchedules do delete api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", master) end.to change { project.pipeline_schedules.count }.by(-1) - expect(response).to have_http_status(:accepted) - expect(response).to match_response_schema('pipeline_schedule') + expect(response).to have_http_status(204) end it 'responds with 404 Not Found if requesting non-existing pipeline_schedule' do From 915dd57fe26fb228f30ae08edc4905b24d4c4339 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Thu, 24 Aug 2017 18:03:39 +0200 Subject: [PATCH 13/70] Add tests for the unmodified header --- spec/requests/api/award_emoji_spec.rb | 16 +++++ spec/requests/api/boards_spec.rb | 4 ++ spec/requests/api/branches_spec.rb | 4 ++ spec/requests/api/broadcast_messages_spec.rb | 4 ++ spec/requests/api/deploy_keys_spec.rb | 4 ++ spec/requests/api/environments_spec.rb | 4 ++ spec/requests/api/group_variables_spec.rb | 4 ++ spec/requests/api/groups_spec.rb | 4 ++ spec/requests/api/issues_spec.rb | 4 ++ spec/requests/api/labels_spec.rb | 5 ++ spec/requests/api/members_spec.rb | 4 ++ spec/requests/api/merge_requests_spec.rb | 4 ++ spec/requests/api/notes_spec.rb | 12 ++++ spec/requests/api/pipeline_schedules_spec.rb | 4 ++ spec/requests/api/project_hooks_spec.rb | 4 ++ spec/requests/api/project_snippets_spec.rb | 7 +- spec/requests/api/projects_spec.rb | 69 +++++++++++++------ spec/requests/api/protected_branches_spec.rb | 4 ++ spec/requests/api/runner_spec.rb | 5 ++ spec/requests/api/runners_spec.rb | 12 ++++ spec/requests/api/snippets_spec.rb | 4 ++ spec/requests/api/system_hooks_spec.rb | 4 ++ spec/requests/api/tags_spec.rb | 4 ++ spec/requests/api/triggers_spec.rb | 4 ++ spec/requests/api/users_spec.rb | 24 +++++++ .../requests/api/status_shared_examples.rb | 11 +++ 26 files changed, 206 insertions(+), 23 deletions(-) diff --git a/spec/requests/api/award_emoji_spec.rb b/spec/requests/api/award_emoji_spec.rb index 1dd9f3f6ddc..593068b8cd7 100644 --- a/spec/requests/api/award_emoji_spec.rb +++ b/spec/requests/api/award_emoji_spec.rb @@ -253,6 +253,10 @@ describe API::AwardEmoji do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/issues/#{issue.iid}/award_emoji/#{award_emoji.id}", user) } + end end context 'when the awardable is a Merge Request' do @@ -269,6 +273,10 @@ describe API::AwardEmoji do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/award_emoji/#{downvote.id}", user) } + end end context 'when the awardable is a Snippet' do @@ -282,6 +290,10 @@ describe API::AwardEmoji do expect(response).to have_http_status(204) end.to change { snippet.award_emoji.count }.from(1).to(0) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/snippets/#{snippet.id}/award_emoji/#{award.id}", user) } + end end end @@ -295,5 +307,9 @@ describe API::AwardEmoji do expect(response).to have_http_status(204) end.to change { note.award_emoji.count }.from(1).to(0) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{note.id}/award_emoji/#{rocket.id}", user) } + end end end diff --git a/spec/requests/api/boards_spec.rb b/spec/requests/api/boards_spec.rb index 43b381c2219..f698d5dddb3 100644 --- a/spec/requests/api/boards_spec.rb +++ b/spec/requests/api/boards_spec.rb @@ -195,6 +195,10 @@ describe API::Boards do expect(response).to have_http_status(204) end + + it_behaves_like '412 response' do + let(:request) { api("#{base_url}/#{dev_list.id}", owner) } + end end end end diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb index 5a2e1b2cf2d..b1e011de604 100644 --- a/spec/requests/api/branches_spec.rb +++ b/spec/requests/api/branches_spec.rb @@ -499,6 +499,10 @@ describe API::Branches do expect(response).to have_gitlab_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/repository/branches/#{branch_name}", user) } + end end describe 'DELETE /projects/:id/repository/merged_branches' do diff --git a/spec/requests/api/broadcast_messages_spec.rb b/spec/requests/api/broadcast_messages_spec.rb index 67989689799..b043a333d33 100644 --- a/spec/requests/api/broadcast_messages_spec.rb +++ b/spec/requests/api/broadcast_messages_spec.rb @@ -171,6 +171,10 @@ describe API::BroadcastMessages do expect(response).to have_http_status(403) end + it_behaves_like '412 response' do + let(:request) { api("/broadcast_messages/#{message.id}", admin) } + end + it 'deletes the broadcast message for admins' do expect do delete api("/broadcast_messages/#{message.id}", admin) diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb index e497ec333a2..684877c33c0 100644 --- a/spec/requests/api/deploy_keys_spec.rb +++ b/spec/requests/api/deploy_keys_spec.rb @@ -190,6 +190,10 @@ describe API::DeployKeys do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin) } + end end describe 'POST /projects/:id/deploy_keys/:key_id/enable' do diff --git a/spec/requests/api/environments_spec.rb b/spec/requests/api/environments_spec.rb index 87716c6fe3a..2361809e0e1 100644 --- a/spec/requests/api/environments_spec.rb +++ b/spec/requests/api/environments_spec.rb @@ -138,6 +138,10 @@ describe API::Environments do expect(response).to have_http_status(404) expect(json_response['message']).to eq('404 Not found') end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/environments/#{environment.id}", user) } + end end context 'a non member' do diff --git a/spec/requests/api/group_variables_spec.rb b/spec/requests/api/group_variables_spec.rb index 2179790d098..93b9cf85c1d 100644 --- a/spec/requests/api/group_variables_spec.rb +++ b/spec/requests/api/group_variables_spec.rb @@ -200,6 +200,10 @@ describe API::GroupVariables do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/groups/#{group.id}/variables/#{variable.key}", user) } + end end context 'authorized user with invalid permissions' do diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index a7557c7fb22..39d76cdbc74 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -488,6 +488,10 @@ describe API::Groups do expect(response).to have_http_status(204) end + it_behaves_like '412 response' do + let(:request) { api("/groups/#{group1.id}", user1) } + end + it "does not remove a group if not an owner" do user4 = create(:user) group1.add_master(user4) diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 7d120e4a234..58d89451073 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -1304,6 +1304,10 @@ describe API::Issues, :mailer do expect(response).to have_http_status(204) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/issues/#{issue.iid}", owner) } + end end context 'when issue does not exist' do diff --git a/spec/requests/api/labels_spec.rb b/spec/requests/api/labels_spec.rb index 5a4257d1009..b231fdea2a3 100644 --- a/spec/requests/api/labels_spec.rb +++ b/spec/requests/api/labels_spec.rb @@ -189,6 +189,11 @@ describe API::Labels do delete api("/projects/#{project.id}/labels", user) expect(response).to have_http_status(400) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/labels", user) } + let(:params) { { name: 'label1' } } + end end describe 'PUT /projects/:id/labels' do diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index 06aca698c91..d3bae8d2888 100644 --- a/spec/requests/api/members_spec.rb +++ b/spec/requests/api/members_spec.rb @@ -284,6 +284,10 @@ describe API::Members do expect(response).to have_http_status(204) end.to change { source.members.count }.by(-1) end + + it_behaves_like '412 response' do + let(:request) { api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master) } + end end it 'returns 404 if member does not exist' do diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 0db645863fb..9027090aabd 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -698,6 +698,10 @@ describe API::MergeRequests do expect(response).to have_gitlab_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user) } + end end end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 75e5062a99c..f5882c0c74a 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -390,6 +390,10 @@ describe API::Notes do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{issue_note.id}", user) } + end end context 'when noteable is a Snippet' do @@ -410,6 +414,10 @@ describe API::Notes do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/snippets/#{snippet.id}/notes/#{snippet_note.id}", user) } + end end context 'when noteable is a Merge Request' do @@ -430,6 +438,10 @@ describe API::Notes do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes/#{merge_request_note.id}", user) } + end end end end diff --git a/spec/requests/api/pipeline_schedules_spec.rb b/spec/requests/api/pipeline_schedules_spec.rb index 150a391bb8d..b6a5a7ffbb5 100644 --- a/spec/requests/api/pipeline_schedules_spec.rb +++ b/spec/requests/api/pipeline_schedules_spec.rb @@ -275,6 +275,10 @@ describe API::PipelineSchedules do expect(response).to have_http_status(:not_found) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", master) } + end end context 'authenticated user with invalid permissions' do diff --git a/spec/requests/api/project_hooks_spec.rb b/spec/requests/api/project_hooks_spec.rb index 2829c243af3..ac3bab09c4c 100644 --- a/spec/requests/api/project_hooks_spec.rb +++ b/spec/requests/api/project_hooks_spec.rb @@ -212,5 +212,9 @@ describe API::ProjectHooks, 'ProjectHooks' do expect(response).to have_http_status(404) expect(WebHook.exists?(hook.id)).to be_truthy end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/hooks/#{hook.id}", user) } + end end end diff --git a/spec/requests/api/project_snippets_spec.rb b/spec/requests/api/project_snippets_spec.rb index 2b541f5719e..0ea95f487ac 100644 --- a/spec/requests/api/project_snippets_spec.rb +++ b/spec/requests/api/project_snippets_spec.rb @@ -228,9 +228,6 @@ describe API::ProjectSnippets do let(:snippet) { create(:project_snippet, author: admin) } it 'deletes snippet' do - admin = create(:admin) - snippet = create(:project_snippet, author: admin) - delete api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin) expect(response).to have_http_status(204) @@ -242,6 +239,10 @@ describe API::ProjectSnippets do expect(response).to have_http_status(404) expect(json_response['message']).to eq('404 Snippet Not Found') end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin) } + end end describe 'GET /projects/:project_id/snippets/:id/raw' do diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index a89a58ff713..46550243d06 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1029,6 +1029,10 @@ describe API::Projects do delete api("/projects/#{project.id}/snippets/1234", user) expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/snippets/#{snippet.id}", user) } + end end describe 'GET /projects/:id/snippets/:snippet_id/raw' do @@ -1104,25 +1108,33 @@ describe API::Projects do project_fork_target.group.add_developer user2 end + context 'for a forked project' do + before do + post api("/projects/#{project_fork_target.id}/fork/#{project_fork_source.id}", admin) + project_fork_target.reload + expect(project_fork_target.forked_from_project).not_to be_nil + expect(project_fork_target.forked?).to be_truthy + end + + it 'makes forked project unforked' do + delete api("/projects/#{project_fork_target.id}/fork", admin) + + expect(response).to have_http_status(204) + project_fork_target.reload + expect(project_fork_target.forked_from_project).to be_nil + expect(project_fork_target.forked?).not_to be_truthy + end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project_fork_target.id}/fork", admin) } + end + end + it 'is forbidden to non-owner users' do delete api("/projects/#{project_fork_target.id}/fork", user2) expect(response).to have_http_status(403) end - it 'makes forked project unforked' do - post api("/projects/#{project_fork_target.id}/fork/#{project_fork_source.id}", admin) - project_fork_target.reload - expect(project_fork_target.forked_from_project).not_to be_nil - expect(project_fork_target.forked?).to be_truthy - - delete api("/projects/#{project_fork_target.id}/fork", admin) - - expect(response).to have_http_status(204) - project_fork_target.reload - expect(project_fork_target.forked_from_project).to be_nil - expect(project_fork_target.forked?).not_to be_truthy - end - it 'is idempotent if not forked' do expect(project_fork_target.forked_from_project).to be_nil delete api("/projects/#{project_fork_target.id}/fork", admin) @@ -1188,14 +1200,23 @@ describe API::Projects do end describe 'DELETE /projects/:id/share/:group_id' do - it 'returns 204 when deleting a group share' do - group = create(:group, :public) - create(:project_group_link, group: group, project: project) + context 'for a valid group' do + let(:group) { create(:group, :public) } - delete api("/projects/#{project.id}/share/#{group.id}", user) + before do + create(:project_group_link, group: group, project: project) + end - expect(response).to have_http_status(204) - expect(project.project_group_links).to be_empty + it 'returns 204 when deleting a group share' do + delete api("/projects/#{project.id}/share/#{group.id}", user) + + expect(response).to have_http_status(204) + expect(project.project_group_links).to be_empty + end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/share/#{group.id}", user) } + end end it 'returns a 400 when group id is not an integer' do @@ -1519,6 +1540,10 @@ describe API::Projects do expect(json_response['message']).to eql('202 Accepted') end + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}", user) } + end + it 'does not remove a project if not an owner' do user3 = create(:user) project.team << [user3, :developer] @@ -1549,6 +1574,10 @@ describe API::Projects do delete api('/projects/1328', admin) expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}", admin) } + end end end diff --git a/spec/requests/api/protected_branches_spec.rb b/spec/requests/api/protected_branches_spec.rb index 1aa8a95780e..07d7f96bd70 100644 --- a/spec/requests/api/protected_branches_spec.rb +++ b/spec/requests/api/protected_branches_spec.rb @@ -213,6 +213,10 @@ describe API::ProtectedBranches do expect(response).to have_gitlab_http_status(204) end + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/protected_branches/#{branch_name}", user) } + end + it "returns 404 if branch does not exist" do delete api("/projects/#{project.id}/protected_branches/barfoo", user) diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index e9ee3dd679d..993164aa8fe 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -149,6 +149,11 @@ describe API::Runner do expect(response).to have_http_status 204 expect(Ci::Runner.count).to eq(0) end + + it_behaves_like '412 response' do + let(:request) { api('/runners') } + let(:params) { { token: runner.token } } + end end end diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index c8ff25f70fa..244895a417e 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -279,6 +279,10 @@ describe API::Runners do expect(response).to have_http_status(204) end.to change { Ci::Runner.shared.count }.by(-1) end + + it_behaves_like '412 response' do + let(:request) { api("/runners/#{shared_runner.id}", admin) } + end end context 'when runner is not shared' do @@ -332,6 +336,10 @@ describe API::Runners do expect(response).to have_http_status(204) end.to change { Ci::Runner.specific.count }.by(-1) end + + it_behaves_like '412 response' do + let(:request) { api("/runners/#{specific_runner.id}", user) } + end end end @@ -463,6 +471,10 @@ describe API::Runners do expect(response).to have_http_status(204) end.to change { project.runners.count }.by(-1) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/runners/#{two_projects_runner.id}", user) } + end end context 'when runner have one associated projects' do diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb index d09b8bc42f1..351dd3276d0 100644 --- a/spec/requests/api/snippets_spec.rb +++ b/spec/requests/api/snippets_spec.rb @@ -270,6 +270,10 @@ describe API::Snippets do expect(response).to have_http_status(404) expect(json_response['message']).to eq('404 Snippet Not Found') end + + it_behaves_like '412 response' do + let(:request) { api("/snippets/#{public_snippet.id}", user) } + end end describe "GET /snippets/:id/user_agent_detail" do diff --git a/spec/requests/api/system_hooks_spec.rb b/spec/requests/api/system_hooks_spec.rb index f65b475fe44..216d278ad21 100644 --- a/spec/requests/api/system_hooks_spec.rb +++ b/spec/requests/api/system_hooks_spec.rb @@ -102,5 +102,9 @@ describe API::SystemHooks do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/hooks/#{hook.id}", admin) } + end end end diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb index 6ac0caa7fe8..0bf7863bdc8 100644 --- a/spec/requests/api/tags_spec.rb +++ b/spec/requests/api/tags_spec.rb @@ -278,6 +278,10 @@ describe API::Tags do expect(response).to have_gitlab_http_status(204) end + it_behaves_like '412 response' do + let(:request) { api(route, current_user) } + end + context 'when tag does not exist' do let(:tag_name) { 'unknown' } diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index 1e206fd2a9e..d1ed57b1825 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -293,6 +293,10 @@ describe API::Triggers do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/triggers/#{trigger.id}", user) } + end end context 'authenticated user with invalid permissions' do diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 49739a1601a..5fef4437997 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -733,6 +733,10 @@ describe API::Users do end.to change { user.keys.count }.by(-1) end + it_behaves_like '412 response' do + let(:request) { api("/users/#{user.id}/keys/#{key.id}", admin) } + end + it 'returns 404 error if user not found' do user.keys << key user.save @@ -838,6 +842,10 @@ describe API::Users do end.to change { user.emails.count }.by(-1) end + it_behaves_like '412 response' do + let(:request) { api("/users/#{user.id}/emails/#{email.id}", admin) } + end + it 'returns 404 error if user not found' do user.emails << email user.save @@ -876,6 +884,10 @@ describe API::Users do expect { Namespace.find(namespace.id) }.to raise_error ActiveRecord::RecordNotFound end + it_behaves_like '412 response' do + let(:request) { api("/users/#{user.id}", admin) } + end + it "does not delete for unauthenticated user" do Sidekiq::Testing.inline! { delete api("/users/#{user.id}") } expect(response).to have_http_status(401) @@ -1116,6 +1128,10 @@ describe API::Users do end.to change { user.keys.count}.by(-1) end + it_behaves_like '412 response' do + let(:request) { api("/user/keys/#{key.id}", user) } + end + it "returns 404 if key ID not found" do delete api("/user/keys/42", user) @@ -1239,6 +1255,10 @@ describe API::Users do end.to change { user.emails.count}.by(-1) end + it_behaves_like '412 response' do + let(:request) { api("/user/emails/#{email.id}", user) } + end + it "returns 404 if email ID not found" do delete api("/user/emails/42", user) @@ -1551,6 +1571,10 @@ describe API::Users do expect(json_response['message']).to eq('403 Forbidden') end + it_behaves_like '412 response' do + let(:request) { api("/users/#{user.id}/impersonation_tokens/#{impersonation_token.id}", admin) } + end + it 'revokes a impersonation token' do delete api("/users/#{user.id}/impersonation_tokens/#{impersonation_token.id}", admin) diff --git a/spec/support/shared_examples/requests/api/status_shared_examples.rb b/spec/support/shared_examples/requests/api/status_shared_examples.rb index 226277411d6..1bffd1e49db 100644 --- a/spec/support/shared_examples/requests/api/status_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/status_shared_examples.rb @@ -40,3 +40,14 @@ shared_examples_for '404 response' do end end end + +shared_examples_for '412 response' do + let(:params) { nil } + before do + delete request, params, { 'HTTP_IF_UNMODIFIED_SINCE' => '1990-01-12T00:00:48-0600' } + end + + it 'returns 412' do + expect(response).to have_gitlab_http_status(412) + end +end From 8bd9fb4cc4f8689a23c68e1b7f893ee59907069f Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Fri, 25 Aug 2017 14:31:09 +0200 Subject: [PATCH 14/70] Add changelog and doc --- changelogs/unreleased/api-delete-respect-headers.yml | 5 +++++ doc/api/README.md | 1 + 2 files changed, 6 insertions(+) create mode 100644 changelogs/unreleased/api-delete-respect-headers.yml diff --git a/changelogs/unreleased/api-delete-respect-headers.yml b/changelogs/unreleased/api-delete-respect-headers.yml new file mode 100644 index 00000000000..cfc8fbfdf91 --- /dev/null +++ b/changelogs/unreleased/api-delete-respect-headers.yml @@ -0,0 +1,5 @@ +--- +title: 'API: Respect the "If-Unmodified-Since" header when delting a resource' +merge_request: 9621 +author: Robert Schilling +type: added diff --git a/doc/api/README.md b/doc/api/README.md index 266b5f018d9..c2a08dcff07 100644 --- a/doc/api/README.md +++ b/doc/api/README.md @@ -263,6 +263,7 @@ The following table shows the possible return codes for API requests. | `404 Not Found` | A resource could not be accessed, e.g., an ID for a resource could not be found. | | `405 Method Not Allowed` | The request is not supported. | | `409 Conflict` | A conflicting resource already exists, e.g., creating a project with a name that already exists. | +| `412` | Indicates the request was denied. May happen if the `If-Unmodified-Since` header is provided when trying to delete a resource, which was modified in between. | | `422 Unprocessable` | The entity could not be processed. | | `500 Server Error` | While handling the request something went wrong server-side. | From b870ae1f59ab0b3f4a10f9dbb2d64324b90ba38b Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 28 Aug 2017 17:35:10 +0200 Subject: [PATCH 15/70] Eager load head pipeline projects for MRs index This ensures the project of an MR's head pipeline is eager loaded, preventing an N+1 query problem from occurring when viewing the list of MRs of a project. --- app/controllers/concerns/issuable_collections.rb | 12 +++++++++++- changelogs/unreleased/mr-index-eager-load.yml | 5 +++++ 2 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/mr-index-eager-load.yml diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index b43b2c5621f..a34a82b7ba6 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -15,7 +15,17 @@ module IssuableCollections end def merge_requests_collection - merge_requests_finder.execute.preload(:source_project, :target_project, :author, :assignee, :labels, :milestone, :head_pipeline, target_project: :namespace, merge_request_diff: :merge_request_diff_commits) + merge_requests_finder.execute.preload( + :source_project, + :target_project, + :author, + :assignee, + :labels, + :milestone, + head_pipeline: :project, + target_project: :namespace, + merge_request_diff: :merge_request_diff_commits + ) end def issues_finder diff --git a/changelogs/unreleased/mr-index-eager-load.yml b/changelogs/unreleased/mr-index-eager-load.yml new file mode 100644 index 00000000000..11c33055b17 --- /dev/null +++ b/changelogs/unreleased/mr-index-eager-load.yml @@ -0,0 +1,5 @@ +--- +title: Eager load head pipeline projects for MRs index +merge_request: +author: +type: other From 87b51c5981db3b1b9831b01ca6e74127d57dc2d9 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Tue, 29 Aug 2017 07:14:41 +0900 Subject: [PATCH 16/70] Move the logic to a concern --- app/models/user.rb | 3 ++- lib/gitlab/sql/pattern.rb | 39 ++++++++++++----------------- spec/lib/gitlab/sql/pattern_spec.rb | 16 ++++++------ 3 files changed, 26 insertions(+), 32 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index e5a84ce4080..70787de4b40 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -5,6 +5,7 @@ class User < ActiveRecord::Base include Gitlab::ConfigHelper include Gitlab::CurrentSettings + include Gitlab::SQL::Pattern include Avatarable include Referable include Sortable @@ -303,7 +304,7 @@ class User < ActiveRecord::Base # Returns an ActiveRecord::Relation. def search(query) table = arel_table - pattern = Gitlab::SQL::Pattern.new(query).to_sql + pattern = User.to_pattern(query) order = <<~SQL CASE diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb index 46c973d8a11..26bfeeeee67 100644 --- a/lib/gitlab/sql/pattern.rb +++ b/lib/gitlab/sql/pattern.rb @@ -1,33 +1,26 @@ module Gitlab module SQL - class Pattern + module Pattern + extend ActiveSupport::Concern + MIN_CHARS_FOR_PARTIAL_MATCHING = 3 - attr_reader :query - - def initialize(query) - @query = query - end - - def to_sql - if exact_matching? - sanitized_query - else - "%#{sanitized_query}%" + class_methods do + def to_pattern(query) + if exact_matching?(query) + sanitize_sql_like(query) + else + "%#{sanitize_sql_like(query)}%" + end end - end - def exact_matching? - !partial_matching? - end + def exact_matching?(query) + query.length < MIN_CHARS_FOR_PARTIAL_MATCHING + end - def partial_matching? - @query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING - end - - def sanitized_query - # Note: ActiveRecord::Base.sanitize_sql_like is a protected method - ActiveRecord::Base.__send__(:sanitize_sql_like, query) + def partial_matching?(query) + query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING + end end end end diff --git a/spec/lib/gitlab/sql/pattern_spec.rb b/spec/lib/gitlab/sql/pattern_spec.rb index d0412f37098..224336421be 100644 --- a/spec/lib/gitlab/sql/pattern_spec.rb +++ b/spec/lib/gitlab/sql/pattern_spec.rb @@ -1,14 +1,14 @@ require 'spec_helper' describe Gitlab::SQL::Pattern do - describe '#to_sql' do - subject(:to_sql) { described_class.new(query).to_sql } + describe '#to_pattern' do + subject(:to_pattern) { User.to_pattern(query) } context 'when a query is shorter than 3 chars' do let(:query) { '12' } it 'returns exact matching pattern' do - expect(to_sql).to eq('12') + expect(to_pattern).to eq('12') end end @@ -16,7 +16,7 @@ describe Gitlab::SQL::Pattern do let(:query) { '_2' } it 'returns sanitized exact matching pattern' do - expect(to_sql).to eq('\_2') + expect(to_pattern).to eq('\_2') end end @@ -24,7 +24,7 @@ describe Gitlab::SQL::Pattern do let(:query) { '123' } it 'returns partial matching pattern' do - expect(to_sql).to eq('%123%') + expect(to_pattern).to eq('%123%') end end @@ -32,7 +32,7 @@ describe Gitlab::SQL::Pattern do let(:query) { '_23' } it 'returns partial matching pattern' do - expect(to_sql).to eq('%\_23%') + expect(to_pattern).to eq('%\_23%') end end @@ -40,7 +40,7 @@ describe Gitlab::SQL::Pattern do let(:query) { '1234' } it 'returns partial matching pattern' do - expect(to_sql).to eq('%1234%') + expect(to_pattern).to eq('%1234%') end end @@ -48,7 +48,7 @@ describe Gitlab::SQL::Pattern do let(:query) { '_234' } it 'returns sanitized partial matching pattern' do - expect(to_sql).to eq('%\_234%') + expect(to_pattern).to eq('%\_234%') end end end From ee4820a5268d02fb7ed142511d1a194f06629a1e Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Mon, 28 Aug 2017 17:13:22 +0200 Subject: [PATCH 17/70] Add a spec when ressource is not modified --- lib/api/projects.rb | 6 ++--- spec/requests/api/projects_spec.rb | 2 ++ .../requests/api/status_shared_examples.rb | 22 +++++++++++++++---- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 78e25b6da03..78d900984ac 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -365,8 +365,7 @@ module API authorize! :remove_fork_project, user_project if user_project.forked? - status 204 - user_project.forked_project_link.destroy + destroy_conditionally!(user_project.forked_project_link) else not_modified! end @@ -410,8 +409,7 @@ module API link = user_project.project_group_links.find_by(group_id: params[:group_id]) not_found!('Group Link') unless link - status 204 - link.destroy + destroy_conditionally!(link) end desc 'Upload a file' diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 46550243d06..4490e50702b 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1541,6 +1541,7 @@ describe API::Projects do end it_behaves_like '412 response' do + let(:success_status) { 202 } let(:request) { api("/projects/#{project.id}", user) } end @@ -1576,6 +1577,7 @@ describe API::Projects do end it_behaves_like '412 response' do + let(:success_status) { 202 } let(:request) { api("/projects/#{project.id}", admin) } end end diff --git a/spec/support/shared_examples/requests/api/status_shared_examples.rb b/spec/support/shared_examples/requests/api/status_shared_examples.rb index 1bffd1e49db..7d7f66adeab 100644 --- a/spec/support/shared_examples/requests/api/status_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/status_shared_examples.rb @@ -43,11 +43,25 @@ end shared_examples_for '412 response' do let(:params) { nil } - before do - delete request, params, { 'HTTP_IF_UNMODIFIED_SINCE' => '1990-01-12T00:00:48-0600' } + let(:success_status) { 204 } + + context 'for a modified ressource' do + before do + delete request, params, { 'HTTP_IF_UNMODIFIED_SINCE' => '1990-01-12T00:00:48-0600' } + end + + it 'returns 412' do + expect(response).to have_gitlab_http_status(412) + end end - it 'returns 412' do - expect(response).to have_gitlab_http_status(412) + context 'for an unmodified ressource' do + before do + delete request, params, { 'HTTP_IF_UNMODIFIED_SINCE' => Time.now } + end + + it 'returns accepted' do + expect(response).to have_gitlab_http_status(success_status) + end end end From 48d3e5fac63ab7514f05b14a8645c344ba3d5f2b Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 19:26:24 +0300 Subject: [PATCH 18/70] replace `is_edited?` with `edited?` --- .rubocop_todo.yml | 4 +++- app/controllers/projects/issues_controller.rb | 2 +- app/helpers/application_helper.rb | 2 +- app/helpers/issuables_helper.rb | 2 +- app/models/concerns/editable.rb | 2 +- spec/models/concerns/editable_spec.rb | 6 +++--- 6 files changed, 10 insertions(+), 8 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 4b4f14efea4..4141d221039 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -243,7 +243,9 @@ Style/PerlBackrefs: # NamePrefixBlacklist: is_, has_, have_ # NameWhitelist: is_a? Style/PredicateName: - Enabled: false + Enabled: true + # NamePrefix: is_ + NamePrefixBlacklist: is_ # Offense count: 58 # Cop supports --auto-correct. diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 8893a514207..1afaceac567 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -202,7 +202,7 @@ class Projects::IssuesController < Projects::ApplicationController task_status: @issue.task_status } - if @issue.is_edited? + if @issue.edited? response[:updated_at] = @issue.updated_at response[:updated_by_name] = @issue.last_edited_by.name response[:updated_by_path] = user_path(@issue.last_edited_by) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index bcee81bdc15..07775a8b159 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -178,7 +178,7 @@ module ApplicationHelper end def edited_time_ago_with_tooltip(object, placement: 'top', html_class: 'time_ago', exclude_author: false) - return unless object.is_edited? + return unless object.edited? content_tag :small, class: 'edited-text' do output = content_tag(:span, 'Edited ') diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 197c90c4081..2a748ce0a75 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -229,7 +229,7 @@ module IssuablesHelper end def updated_at_by(issuable) - return {} unless issuable.is_edited? + return {} unless issuable.edited? { updatedAt: issuable.updated_at.to_time.iso8601, diff --git a/app/models/concerns/editable.rb b/app/models/concerns/editable.rb index 28623d257a6..c0a3099f676 100644 --- a/app/models/concerns/editable.rb +++ b/app/models/concerns/editable.rb @@ -1,7 +1,7 @@ module Editable extend ActiveSupport::Concern - def is_edited? + def edited? last_edited_at.present? && last_edited_at != created_at end diff --git a/spec/models/concerns/editable_spec.rb b/spec/models/concerns/editable_spec.rb index cd73af3b480..49a9a8ebcbc 100644 --- a/spec/models/concerns/editable_spec.rb +++ b/spec/models/concerns/editable_spec.rb @@ -1,11 +1,11 @@ require 'spec_helper' describe Editable do - describe '#is_edited?' do + describe '#edited?' do let(:issue) { create(:issue, last_edited_at: nil) } let(:edited_issue) { create(:issue, created_at: 3.days.ago, last_edited_at: 2.days.ago) } - it { expect(issue.is_edited?).to eq(false) } - it { expect(edited_issue.is_edited?).to eq(true) } + it { expect(issue.edited?).to eq(false) } + it { expect(edited_issue.edited?).to eq(true) } end end From 5f72aaa303aabdb2258959e84b446074bf8a6915 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 19:31:20 +0300 Subject: [PATCH 19/70] replace `is_legacy_group_milestone?` with `legacy_group_milestone?` --- app/controllers/groups/milestones_controller.rb | 6 +++--- app/helpers/milestones_helper.rb | 2 +- app/models/concerns/milestoneish.rb | 2 +- app/models/group_milestone.rb | 2 +- app/views/groups/milestones/show.html.haml | 2 +- app/views/shared/milestones/_milestone.html.haml | 2 +- app/views/shared/milestones/_top.html.haml | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb index 5c10d7bc261..7a7bcb1a3d2 100644 --- a/app/controllers/groups/milestones_controller.rb +++ b/app/controllers/groups/milestones_controller.rb @@ -35,13 +35,13 @@ class Groups::MilestonesController < Groups::ApplicationController end def edit - render_404 if @milestone.is_legacy_group_milestone? + render_404 if @milestone.legacy_group_milestone? end def update # Keep this compatible with legacy group milestones where we have to update # all projects milestones states at once. - if @milestone.is_legacy_group_milestone? + if @milestone.legacy_group_milestone? update_params = milestone_params.select { |key| key == "state_event" } milestones = @milestone.milestones else @@ -67,7 +67,7 @@ class Groups::MilestonesController < Groups::ApplicationController end def milestone_path - if @milestone.is_legacy_group_milestone? + if @milestone.legacy_group_milestone? group_milestone_path(group, @milestone.safe_title, title: @milestone.title) else group_milestone_path(group, @milestone.iid) diff --git a/app/helpers/milestones_helper.rb b/app/helpers/milestones_helper.rb index 86666022a2a..446a59030a6 100644 --- a/app/helpers/milestones_helper.rb +++ b/app/helpers/milestones_helper.rb @@ -164,7 +164,7 @@ module MilestonesHelper def group_milestone_route(milestone, params = {}) params = nil if params.empty? - if milestone.is_legacy_group_milestone? + if milestone.legacy_group_milestone? group_milestone_path(@group, milestone.safe_title, title: milestone.title, milestone: params) else group_milestone_path(@group, milestone.iid, milestone: params) diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb index f0998465822..dad92413950 100644 --- a/app/models/concerns/milestoneish.rb +++ b/app/models/concerns/milestoneish.rb @@ -78,7 +78,7 @@ module Milestoneish false end - def is_legacy_group_milestone? + def legacy_group_milestone? false end diff --git a/app/models/group_milestone.rb b/app/models/group_milestone.rb index 65249bd7bfc..98135ee3c8b 100644 --- a/app/models/group_milestone.rb +++ b/app/models/group_milestone.rb @@ -17,7 +17,7 @@ class GroupMilestone < GlobalMilestone { group_id: group.id } end - def is_legacy_group_milestone? + def legacy_group_milestone? true end end diff --git a/app/views/groups/milestones/show.html.haml b/app/views/groups/milestones/show.html.haml index 54b1b7a734a..23b1a22240f 100644 --- a/app/views/groups/milestones/show.html.haml +++ b/app/views/groups/milestones/show.html.haml @@ -1,4 +1,4 @@ = render "header_title" = render 'shared/milestones/top', milestone: @milestone, group: @group -= render 'shared/milestones/tabs', milestone: @milestone, show_project_name: true if @milestone.is_legacy_group_milestone? += render 'shared/milestones/tabs', milestone: @milestone, show_project_name: true if @milestone.legacy_group_milestone? = render 'shared/milestones/sidebar', milestone: @milestone, affix_offset: 102 diff --git a/app/views/shared/milestones/_milestone.html.haml b/app/views/shared/milestones/_milestone.html.haml index 6a85f7d0564..168c7f911d4 100644 --- a/app/views/shared/milestones/_milestone.html.haml +++ b/app/views/shared/milestones/_milestone.html.haml @@ -21,7 +21,7 @@ - if milestone.is_a?(GlobalMilestone) || milestone.is_group_milestone? .row .col-sm-6 - - if milestone.is_legacy_group_milestone? + - if milestone.legacy_group_milestone? .expiration= render('shared/milestone_expired', milestone: milestone) .projects - milestone.milestones.each do |milestone| diff --git a/app/views/shared/milestones/_top.html.haml b/app/views/shared/milestones/_top.html.haml index 3014300fbe7..2a7b8dbb12a 100644 --- a/app/views/shared/milestones/_top.html.haml +++ b/app/views/shared/milestones/_top.html.haml @@ -44,7 +44,7 @@ - close_msg = group ? 'You may close the milestone now.' : 'Navigate to the project to close the milestone.' %span All issues for this milestone are closed. #{close_msg} -- if @milestone.is_legacy_group_milestone? || @milestone.is_dashboard_milestone? +- if @milestone.legacy_group_milestone? || @milestone.is_dashboard_milestone? .table-holder %table.table %thead From 87467127b6e0927d56e532f4d6adc4091ff9ef6f Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 19:35:06 +0300 Subject: [PATCH 20/70] replace `is_ancestor?` with `ancestor?` --- app/models/deployment.rb | 2 +- app/models/repository.rb | 6 +++--- lib/gitlab/git/repository.rb | 2 +- spec/models/repository_spec.rb | 22 +++++++++++----------- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 056c49e7162..7bcded5b5e1 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -49,7 +49,7 @@ class Deployment < ActiveRecord::Base # created before then could have a `sha` referring to a commit that no # longer exists in the repository, so just ignore those. begin - project.repository.is_ancestor?(commit.id, sha) + project.repository.ancestor?(commit.id, sha) rescue Rugged::OdbError false end diff --git a/app/models/repository.rb b/app/models/repository.rb index 9fb2e2aa306..cb7aba89020 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -944,7 +944,7 @@ class Repository if branch_commit same_head = branch_commit.id == root_ref_commit.id - !same_head && is_ancestor?(branch_commit.id, root_ref_commit.id) + !same_head && ancestor?(branch_commit.id, root_ref_commit.id) else nil end @@ -958,12 +958,12 @@ class Repository nil end - def is_ancestor?(ancestor_id, descendant_id) + def ancestor?(ancestor_id, descendant_id) return false if ancestor_id.nil? || descendant_id.nil? Gitlab::GitalyClient.migrate(:is_ancestor) do |is_enabled| if is_enabled - raw_repository.is_ancestor?(ancestor_id, descendant_id) + raw_repository.ancestor?(ancestor_id, descendant_id) else rugged_is_ancestor?(ancestor_id, descendant_id) end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index b835dec24eb..dce3a9b2d37 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -439,7 +439,7 @@ module Gitlab end # Returns true is +from+ is direct ancestor to +to+, otherwise false - def is_ancestor?(from, to) + def ancestor?(from, to) gitaly_commit_client.is_ancestor(from, to) end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 462e92b8b62..3151649b64e 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -2036,23 +2036,23 @@ describe Repository, models: true do end end - describe '#is_ancestor?' do + describe '#ancestor?' do let(:commit) { repository.commit } let(:ancestor) { commit.parents.first } context 'with Gitaly enabled' do it 'it is an ancestor' do - expect(repository.is_ancestor?(ancestor.id, commit.id)).to eq(true) + expect(repository.ancestor?(ancestor.id, commit.id)).to eq(true) end it 'it is not an ancestor' do - expect(repository.is_ancestor?(commit.id, ancestor.id)).to eq(false) + expect(repository.ancestor?(commit.id, ancestor.id)).to eq(false) end it 'returns false on nil-values' do - expect(repository.is_ancestor?(nil, commit.id)).to eq(false) - expect(repository.is_ancestor?(ancestor.id, nil)).to eq(false) - expect(repository.is_ancestor?(nil, nil)).to eq(false) + expect(repository.ancestor?(nil, commit.id)).to eq(false) + expect(repository.ancestor?(ancestor.id, nil)).to eq(false) + expect(repository.ancestor?(nil, nil)).to eq(false) end end @@ -2063,17 +2063,17 @@ describe Repository, models: true do end it 'it is an ancestor' do - expect(repository.is_ancestor?(ancestor.id, commit.id)).to eq(true) + expect(repository.ancestor?(ancestor.id, commit.id)).to eq(true) end it 'it is not an ancestor' do - expect(repository.is_ancestor?(commit.id, ancestor.id)).to eq(false) + expect(repository.ancestor?(commit.id, ancestor.id)).to eq(false) end it 'returns false on nil-values' do - expect(repository.is_ancestor?(nil, commit.id)).to eq(false) - expect(repository.is_ancestor?(ancestor.id, nil)).to eq(false) - expect(repository.is_ancestor?(nil, nil)).to eq(false) + expect(repository.ancestor?(nil, commit.id)).to eq(false) + expect(repository.ancestor?(ancestor.id, nil)).to eq(false) + expect(repository.ancestor?(nil, nil)).to eq(false) end end end From ac3e5a58549c3f88659cedb47df68e5e719d68f1 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 19:37:37 +0300 Subject: [PATCH 21/70] replace `is_project_milestone?` with `project_milestone?` --- app/helpers/milestones_routing_helper.rb | 4 ++-- app/models/concerns/milestoneish.rb | 2 +- app/models/milestone.rb | 2 +- app/services/milestones/close_service.rb | 2 +- app/services/milestones/create_service.rb | 2 +- app/services/milestones/destroy_service.rb | 2 +- app/services/milestones/reopen_service.rb | 2 +- 7 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/helpers/milestones_routing_helper.rb b/app/helpers/milestones_routing_helper.rb index 766d5262018..fa05b081634 100644 --- a/app/helpers/milestones_routing_helper.rb +++ b/app/helpers/milestones_routing_helper.rb @@ -2,7 +2,7 @@ module MilestonesRoutingHelper def milestone_path(milestone, *args) if milestone.is_group_milestone? group_milestone_path(milestone.group, milestone, *args) - elsif milestone.is_project_milestone? + elsif milestone.project_milestone? project_milestone_path(milestone.project, milestone, *args) end end @@ -10,7 +10,7 @@ module MilestonesRoutingHelper def milestone_url(milestone, *args) if milestone.is_group_milestone? group_milestone_url(milestone.group, milestone, *args) - elsif milestone.is_project_milestone? + elsif milestone.project_milestone? project_milestone_url(milestone.project, milestone, *args) end end diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb index dad92413950..6c6ceaa35c7 100644 --- a/app/models/concerns/milestoneish.rb +++ b/app/models/concerns/milestoneish.rb @@ -74,7 +74,7 @@ module Milestoneish false end - def is_project_milestone? + def project_milestone? false end diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 01e0d0155a3..dd1f3824a98 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -211,7 +211,7 @@ class Milestone < ActiveRecord::Base group_id.present? end - def is_project_milestone? + def project_milestone? project_id.present? end diff --git a/app/services/milestones/close_service.rb b/app/services/milestones/close_service.rb index 776ec4b287b..5b06c4b601d 100644 --- a/app/services/milestones/close_service.rb +++ b/app/services/milestones/close_service.rb @@ -1,7 +1,7 @@ module Milestones class CloseService < Milestones::BaseService def execute(milestone) - if milestone.close && milestone.is_project_milestone? + if milestone.close && milestone.project_milestone? event_service.close_milestone(milestone, current_user) end diff --git a/app/services/milestones/create_service.rb b/app/services/milestones/create_service.rb index aef3124c7e3..ed2e833d833 100644 --- a/app/services/milestones/create_service.rb +++ b/app/services/milestones/create_service.rb @@ -3,7 +3,7 @@ module Milestones def execute milestone = parent.milestones.new(params) - if milestone.save && milestone.is_project_milestone? + if milestone.save && milestone.project_milestone? event_service.open_milestone(milestone, current_user) end diff --git a/app/services/milestones/destroy_service.rb b/app/services/milestones/destroy_service.rb index 600ebcfbecb..b18651476a8 100644 --- a/app/services/milestones/destroy_service.rb +++ b/app/services/milestones/destroy_service.rb @@ -1,7 +1,7 @@ module Milestones class DestroyService < Milestones::BaseService def execute(milestone) - return unless milestone.is_project_milestone? + return unless milestone.project_milestone? Milestone.transaction do update_params = { milestone: nil } diff --git a/app/services/milestones/reopen_service.rb b/app/services/milestones/reopen_service.rb index 5b8b682caaf..3efb33157c5 100644 --- a/app/services/milestones/reopen_service.rb +++ b/app/services/milestones/reopen_service.rb @@ -1,7 +1,7 @@ module Milestones class ReopenService < Milestones::BaseService def execute(milestone) - if milestone.activate && milestone.is_project_milestone? + if milestone.activate && milestone.project_milestone? event_service.reopen_milestone(milestone, current_user) end From fab7fdcf2f8b040c345d1cb0b05acd053c9af963 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 19:41:44 +0300 Subject: [PATCH 22/70] replace `is_group_milestone?` with `group_milestone?` --- app/helpers/milestones_routing_helper.rb | 4 ++-- app/models/concerns/milestoneish.rb | 2 +- app/models/milestone.rb | 4 ++-- app/services/system_note_service.rb | 2 +- app/views/shared/milestones/_milestone.html.haml | 6 +++--- app/views/shared/milestones/_top.html.haml | 6 +++--- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/helpers/milestones_routing_helper.rb b/app/helpers/milestones_routing_helper.rb index fa05b081634..a0b2616f224 100644 --- a/app/helpers/milestones_routing_helper.rb +++ b/app/helpers/milestones_routing_helper.rb @@ -1,6 +1,6 @@ module MilestonesRoutingHelper def milestone_path(milestone, *args) - if milestone.is_group_milestone? + if milestone.group_milestone? group_milestone_path(milestone.group, milestone, *args) elsif milestone.project_milestone? project_milestone_path(milestone.project, milestone, *args) @@ -8,7 +8,7 @@ module MilestonesRoutingHelper end def milestone_url(milestone, *args) - if milestone.is_group_milestone? + if milestone.group_milestone? group_milestone_url(milestone.group, milestone, *args) elsif milestone.project_milestone? project_milestone_url(milestone.project, milestone, *args) diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb index 6c6ceaa35c7..c4b79d51fe7 100644 --- a/app/models/concerns/milestoneish.rb +++ b/app/models/concerns/milestoneish.rb @@ -70,7 +70,7 @@ module Milestoneish due_date && due_date.past? end - def is_group_milestone? + def group_milestone? false end diff --git a/app/models/milestone.rb b/app/models/milestone.rb index dd1f3824a98..a3070a12b7c 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -163,7 +163,7 @@ class Milestone < ActiveRecord::Base # Milestone.first.to_reference(same_namespace_project) # => "gitlab-ce%1" # def to_reference(from_project = nil, format: :iid, full: false) - return if is_group_milestone? && format != :name + return if group_milestone? && format != :name format_reference = milestone_format_reference(format) reference = "#{self.class.reference_prefix}#{format_reference}" @@ -207,7 +207,7 @@ class Milestone < ActiveRecord::Base group || project end - def is_group_milestone? + def group_milestone? group_id.present? end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 1763f64a4e4..1f66a2668f9 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -142,7 +142,7 @@ module SystemNoteService # # Returns the created Note object def change_milestone(noteable, project, author, milestone) - format = milestone&.is_group_milestone? ? :name : :iid + format = milestone&.group_milestone? ? :name : :iid body = milestone.nil? ? 'removed milestone' : "changed milestone to #{milestone.to_reference(project, format: format)}" create_note(NoteSummary.new(noteable, project, author, body, action: 'milestone')) diff --git a/app/views/shared/milestones/_milestone.html.haml b/app/views/shared/milestones/_milestone.html.haml index 168c7f911d4..305e2542281 100644 --- a/app/views/shared/milestones/_milestone.html.haml +++ b/app/views/shared/milestones/_milestone.html.haml @@ -5,7 +5,7 @@ .row .col-sm-6 %strong= link_to truncate(milestone.title, length: 100), milestone_path - - if milestone.is_group_milestone? + - if milestone.group_milestone? %span - Group Milestone - else %span - Project Milestone @@ -18,7 +18,7 @@ · = link_to pluralize(milestone.merge_requests.size, 'Merge Request'), merge_requests_path .col-sm-6= milestone_progress_bar(milestone) - - if milestone.is_a?(GlobalMilestone) || milestone.is_group_milestone? + - if milestone.is_a?(GlobalMilestone) || milestone.group_milestone? .row .col-sm-6 - if milestone.legacy_group_milestone? @@ -31,7 +31,7 @@ - if @group .col-sm-6.milestone-actions - if can?(current_user, :admin_milestones, @group) - - if milestone.is_group_milestone? + - if milestone.group_milestone? = link_to edit_group_milestone_path(@group, milestone), class: "btn btn-xs btn-grouped" do Edit \ diff --git a/app/views/shared/milestones/_top.html.haml b/app/views/shared/milestones/_top.html.haml index 2a7b8dbb12a..05b607db488 100644 --- a/app/views/shared/milestones/_top.html.haml +++ b/app/views/shared/milestones/_top.html.haml @@ -22,7 +22,7 @@ - if group .pull-right - if can?(current_user, :admin_milestones, group) - - if milestone.is_group_milestone? + - if milestone.group_milestone? = link_to edit_group_milestone_path(group, milestone), class: "btn btn btn-grouped" do Edit - if milestone.active? @@ -33,7 +33,7 @@ .detail-page-description.milestone-detail %h2.title = markdown_field(milestone, :title) - - if @milestone.is_group_milestone? && @milestone.description.present? + - if @milestone.group_milestone? && @milestone.description.present? %div .description .wiki @@ -67,7 +67,7 @@ Open %td = ms.expires_at -- elsif @milestone.is_group_milestone? +- elsif @milestone.group_milestone? %br View = link_to 'Issues', issues_group_path(@group, milestone_title: milestone.title) From bd406ca3503a032dc8e1ff0bd88a6657292b3fff Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 19:42:40 +0300 Subject: [PATCH 23/70] replace `is_overlap?` with `overlap?` --- app/models/network/graph.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/network/graph.rb b/app/models/network/graph.rb index 0e5acb22d50..3845e485413 100644 --- a/app/models/network/graph.rb +++ b/app/models/network/graph.rb @@ -152,14 +152,14 @@ module Network end def find_free_parent_space(range, space_base, space_step, space_default) - if is_overlap?(range, space_default) + if overlap?(range, space_default) find_free_space(range, space_step, space_base, space_default) else space_default end end - def is_overlap?(range, overlap_space) + def overlap?(range, overlap_space) range.each do |i| if i != range.first && i != range.last && From edb14511aa9d96c41fc2dc1f6f7b0be7779149fd Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 19:43:47 +0300 Subject: [PATCH 24/70] replace `is_update?` with `update?` --- app/models/project_services/chat_notification_service.rb | 6 +++--- app/models/project_services/hipchat_service.rb | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/project_services/chat_notification_service.rb b/app/models/project_services/chat_notification_service.rb index 7b15a5dd04d..818cfb01b14 100644 --- a/app/models/project_services/chat_notification_service.rb +++ b/app/models/project_services/chat_notification_service.rb @@ -101,9 +101,9 @@ class ChatNotificationService < Service when "push", "tag_push" ChatMessage::PushMessage.new(data) when "issue" - ChatMessage::IssueMessage.new(data) unless is_update?(data) + ChatMessage::IssueMessage.new(data) unless update?(data) when "merge_request" - ChatMessage::MergeMessage.new(data) unless is_update?(data) + ChatMessage::MergeMessage.new(data) unless update?(data) when "note" ChatMessage::NoteMessage.new(data) when "pipeline" @@ -136,7 +136,7 @@ class ChatNotificationService < Service project.web_url end - def is_update?(data) + def update?(data) data[:object_attributes][:action] == 'update' end diff --git a/app/models/project_services/hipchat_service.rb b/app/models/project_services/hipchat_service.rb index f422e0ea036..976d85246a8 100644 --- a/app/models/project_services/hipchat_service.rb +++ b/app/models/project_services/hipchat_service.rb @@ -85,9 +85,9 @@ class HipchatService < Service when "push", "tag_push" create_push_message(data) when "issue" - create_issue_message(data) unless is_update?(data) + create_issue_message(data) unless update?(data) when "merge_request" - create_merge_request_message(data) unless is_update?(data) + create_merge_request_message(data) unless update?(data) when "note" create_note_message(data) when "pipeline" @@ -282,7 +282,7 @@ class HipchatService < Service "#{project_name}" end - def is_update?(data) + def update?(data) data[:object_attributes][:action] == 'update' end From 9226804bf32bcecd826818eb626714b52285c5e8 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 19:44:36 +0300 Subject: [PATCH 25/70] replace `is_runner_queue_value_latest?` with `runner_queue_value_latest?` --- app/models/ci/runner.rb | 2 +- lib/api/runner.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index c6d23898560..906a76ec560 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -142,7 +142,7 @@ module Ci expire: RUNNER_QUEUE_EXPIRY_TIME, overwrite: false) end - def is_runner_queue_value_latest?(value) + def runner_queue_value_latest?(value) ensure_runner_queue_value == value if value.present? end diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 88fc62d33df..1a7ded31c91 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -78,7 +78,7 @@ module API no_content! unless current_runner.active? update_runner_info - if current_runner.is_runner_queue_value_latest?(params[:last_update]) + if current_runner.runner_queue_value_latest?(params[:last_update]) header 'X-GitLab-Last-Update', params[:last_update] Gitlab::Metrics.add_event(:build_not_found_cached) return no_content! From 48389b4e5ec251441d1b1e135fdb28fe742289d8 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 19:46:42 +0300 Subject: [PATCH 26/70] replace `is_dashboard_milestone?` with `dashboard_milestone?` --- app/models/concerns/milestoneish.rb | 2 +- app/models/dashboard_milestone.rb | 2 +- app/views/shared/milestones/_top.html.haml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb index c4b79d51fe7..710fc1ed647 100644 --- a/app/models/concerns/milestoneish.rb +++ b/app/models/concerns/milestoneish.rb @@ -82,7 +82,7 @@ module Milestoneish false end - def is_dashboard_milestone? + def dashboard_milestone? false end diff --git a/app/models/dashboard_milestone.rb b/app/models/dashboard_milestone.rb index fac7c5e5c85..86eb4ec76fc 100644 --- a/app/models/dashboard_milestone.rb +++ b/app/models/dashboard_milestone.rb @@ -3,7 +3,7 @@ class DashboardMilestone < GlobalMilestone { authorized_only: true } end - def is_dashboard_milestone? + def dashboard_milestone? true end end diff --git a/app/views/shared/milestones/_top.html.haml b/app/views/shared/milestones/_top.html.haml index 05b607db488..fd0760d83a5 100644 --- a/app/views/shared/milestones/_top.html.haml +++ b/app/views/shared/milestones/_top.html.haml @@ -44,7 +44,7 @@ - close_msg = group ? 'You may close the milestone now.' : 'Navigate to the project to close the milestone.' %span All issues for this milestone are closed. #{close_msg} -- if @milestone.legacy_group_milestone? || @milestone.is_dashboard_milestone? +- if @milestone.legacy_group_milestone? || @milestone.dashboard_milestone? .table-holder %table.table %thead From 6a56bec735c7434c85e3b8776b8413d3bdcb93ec Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 19:58:09 +0300 Subject: [PATCH 27/70] replace `is_gitlab_user?` with `gitlab_user?` --- lib/system_check/app/git_config_check.rb | 2 +- lib/tasks/gitlab/task_helpers.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/system_check/app/git_config_check.rb b/lib/system_check/app/git_config_check.rb index 198867f7ac6..d08a81639e3 100644 --- a/lib/system_check/app/git_config_check.rb +++ b/lib/system_check/app/git_config_check.rb @@ -20,7 +20,7 @@ module SystemCheck # Returns true if all subcommands were successful (according to their exit code) # Returns false if any or all subcommands failed. def repair! - return false unless is_gitlab_user? + return false unless gitlab_user? command_success = OPTIONS.map do |name, value| system(*%W(#{Gitlab.config.git.bin_path} config --global #{name} #{value})) diff --git a/lib/tasks/gitlab/task_helpers.rb b/lib/tasks/gitlab/task_helpers.rb index d85b810ac66..8a63f486fa3 100644 --- a/lib/tasks/gitlab/task_helpers.rb +++ b/lib/tasks/gitlab/task_helpers.rb @@ -104,7 +104,7 @@ module Gitlab Gitlab.config.gitlab.user end - def is_gitlab_user? + def gitlab_user? return @is_gitlab_user unless @is_gitlab_user.nil? current_user = run_command(%w(whoami)).chomp @@ -114,7 +114,7 @@ module Gitlab def warn_user_is_not_gitlab return if @warned_user_not_gitlab - unless is_gitlab_user? + unless gitlab_user? current_user = run_command(%w(whoami)).chomp puts " Warning ".color(:black).background(:yellow) From 78c5d4ddc2af74f352d4871689c7b8451b612b13 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 20:01:22 +0300 Subject: [PATCH 28/70] replace `is_multi_check?` with `multi_check?` --- lib/system_check/base_check.rb | 2 +- lib/system_check/simple_executor.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/system_check/base_check.rb b/lib/system_check/base_check.rb index 5dcb3f0886b..7f9e2ffffc2 100644 --- a/lib/system_check/base_check.rb +++ b/lib/system_check/base_check.rb @@ -73,7 +73,7 @@ module SystemCheck self.class.instance_methods(false).include?(:skip?) end - def is_multi_check? + def multi_check? self.class.instance_methods(false).include?(:multi_check) end diff --git a/lib/system_check/simple_executor.rb b/lib/system_check/simple_executor.rb index e5986612908..6604b1078cf 100644 --- a/lib/system_check/simple_executor.rb +++ b/lib/system_check/simple_executor.rb @@ -53,7 +53,7 @@ module SystemCheck end # When implements a multi check, we don't control the output - if check.is_multi_check? + if check.multi_check? check.multi_check return end From 622c912d5b896380a0d0a1db61bbf9d1cb990c80 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 20:02:27 +0300 Subject: [PATCH 29/70] replace `is_successful?` with `successful?` --- lib/gitlab/health_checks/db_check.rb | 2 +- lib/gitlab/health_checks/redis/cache_check.rb | 2 +- lib/gitlab/health_checks/redis/queues_check.rb | 2 +- lib/gitlab/health_checks/redis/redis_check.rb | 2 +- lib/gitlab/health_checks/redis/shared_state_check.rb | 2 +- lib/gitlab/health_checks/simple_abstract_check.rb | 8 ++++---- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/gitlab/health_checks/db_check.rb b/lib/gitlab/health_checks/db_check.rb index fd94984f8a2..e27e16ddaf6 100644 --- a/lib/gitlab/health_checks/db_check.rb +++ b/lib/gitlab/health_checks/db_check.rb @@ -10,7 +10,7 @@ module Gitlab 'db_ping' end - def is_successful?(result) + def successful?(result) result == '1' end diff --git a/lib/gitlab/health_checks/redis/cache_check.rb b/lib/gitlab/health_checks/redis/cache_check.rb index a28658d42d4..0eb9b77634a 100644 --- a/lib/gitlab/health_checks/redis/cache_check.rb +++ b/lib/gitlab/health_checks/redis/cache_check.rb @@ -15,7 +15,7 @@ module Gitlab 'redis_cache_ping' end - def is_successful?(result) + def successful?(result) result == 'PONG' end diff --git a/lib/gitlab/health_checks/redis/queues_check.rb b/lib/gitlab/health_checks/redis/queues_check.rb index f97d50d3947..f322fe831b8 100644 --- a/lib/gitlab/health_checks/redis/queues_check.rb +++ b/lib/gitlab/health_checks/redis/queues_check.rb @@ -15,7 +15,7 @@ module Gitlab 'redis_queues_ping' end - def is_successful?(result) + def successful?(result) result == 'PONG' end diff --git a/lib/gitlab/health_checks/redis/redis_check.rb b/lib/gitlab/health_checks/redis/redis_check.rb index fe4e3c4a3ab..8ceb0a0aa46 100644 --- a/lib/gitlab/health_checks/redis/redis_check.rb +++ b/lib/gitlab/health_checks/redis/redis_check.rb @@ -11,7 +11,7 @@ module Gitlab 'redis_ping' end - def is_successful?(result) + def successful?(result) result == 'PONG' end diff --git a/lib/gitlab/health_checks/redis/shared_state_check.rb b/lib/gitlab/health_checks/redis/shared_state_check.rb index e3244392902..07e6f707998 100644 --- a/lib/gitlab/health_checks/redis/shared_state_check.rb +++ b/lib/gitlab/health_checks/redis/shared_state_check.rb @@ -15,7 +15,7 @@ module Gitlab 'redis_shared_state_ping' end - def is_successful?(result) + def successful?(result) result == 'PONG' end diff --git a/lib/gitlab/health_checks/simple_abstract_check.rb b/lib/gitlab/health_checks/simple_abstract_check.rb index f5026171ba4..96945ce5b20 100644 --- a/lib/gitlab/health_checks/simple_abstract_check.rb +++ b/lib/gitlab/health_checks/simple_abstract_check.rb @@ -5,7 +5,7 @@ module Gitlab def readiness check_result = check - if is_successful?(check_result) + if successful?(check_result) HealthChecks::Result.new(true) elsif check_result.is_a?(Timeout::Error) HealthChecks::Result.new(false, "#{human_name} check timed out") @@ -16,10 +16,10 @@ module Gitlab def metrics result, elapsed = with_timing(&method(:check)) - Rails.logger.error("#{human_name} check returned unexpected result #{result}") unless is_successful?(result) + Rails.logger.error("#{human_name} check returned unexpected result #{result}") unless successful?(result) [ metric("#{metric_prefix}_timeout", result.is_a?(Timeout::Error) ? 1 : 0), - metric("#{metric_prefix}_success", is_successful?(result) ? 1 : 0), + metric("#{metric_prefix}_success", successful?(result) ? 1 : 0), metric("#{metric_prefix}_latency_seconds", elapsed) ] end @@ -30,7 +30,7 @@ module Gitlab raise NotImplementedError end - def is_successful?(result) + def successful?(result) raise NotImplementedError end From 1c0def2a769befa3f0e3c8654e723645b8625117 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 20:03:50 +0300 Subject: [PATCH 30/70] replace `is_default_branch?` with `default_branch?` --- app/services/git_push_service.rb | 10 +++++----- spec/services/git_push_service_spec.rb | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index e81a56672e2..bb61136e33b 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -30,7 +30,7 @@ class GitPushService < BaseService @project.repository.after_create_branch # Re-find the pushed commits. - if is_default_branch? + if default_branch? # Initial push to the default branch. Take the full history of that branch as "newly pushed". process_default_branch else @@ -50,7 +50,7 @@ class GitPushService < BaseService # Update the bare repositories info/attributes file using the contents of the default branches # .gitattributes file - update_gitattributes if is_default_branch? + update_gitattributes if default_branch? end execute_related_hooks @@ -66,7 +66,7 @@ class GitPushService < BaseService end def update_caches - if is_default_branch? + if default_branch? if push_to_new_branch? # If this is the initial push into the default branch, the file type caches # will already be reset as a result of `Project#change_head`. @@ -108,7 +108,7 @@ class GitPushService < BaseService # Schedules processing of commit messages. def process_commit_messages - default = is_default_branch? + default = default_branch? @push_commits.last(PROCESS_COMMIT_LIMIT).each do |commit| if commit.matches_cross_reference_regex? @@ -202,7 +202,7 @@ class GitPushService < BaseService Gitlab::Git.branch_ref?(params[:ref]) end - def is_default_branch? + def default_branch? Gitlab::Git.branch_ref?(params[:ref]) && (Gitlab::Git.ref_name(params[:ref]) == project.default_branch || project.default_branch.nil?) end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index e3c1bdce300..cc3d4e7da49 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -617,7 +617,7 @@ describe GitPushService, services: true do context 'on the default branch' do before do - allow(service).to receive(:is_default_branch?).and_return(true) + allow(service).to receive(:default_branch?).and_return(true) end it 'flushes the caches of any special files that have been changed' do @@ -638,7 +638,7 @@ describe GitPushService, services: true do context 'on a non-default branch' do before do - allow(service).to receive(:is_default_branch?).and_return(false) + allow(service).to receive(:default_branch?).and_return(false) end it 'does not flush any conditional caches' do From fa030cbc85470edbff8e8c225a9fc7d6e52bfa9b Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 20:05:02 +0300 Subject: [PATCH 31/70] replace `is_spam?` with `spam?` --- app/services/akismet_service.rb | 2 +- app/services/spam_service.rb | 2 +- spec/controllers/projects/issues_controller_spec.rb | 8 ++++---- spec/controllers/projects/snippets_controller_spec.rb | 4 ++-- spec/controllers/snippets_controller_spec.rb | 4 ++-- spec/requests/api/issues_spec.rb | 4 ++-- spec/requests/api/project_snippets_spec.rb | 4 ++-- spec/requests/api/snippets_spec.rb | 4 ++-- spec/requests/api/v3/issues_spec.rb | 4 ++-- spec/requests/api/v3/project_snippets_spec.rb | 4 ++-- spec/requests/api/v3/snippets_spec.rb | 2 +- spec/services/issues/create_service_spec.rb | 4 ++-- spec/services/spam_service_spec.rb | 6 +++--- 13 files changed, 26 insertions(+), 26 deletions(-) diff --git a/app/services/akismet_service.rb b/app/services/akismet_service.rb index 59153cbbc0a..7b5482b3cd1 100644 --- a/app/services/akismet_service.rb +++ b/app/services/akismet_service.rb @@ -7,7 +7,7 @@ class AkismetService @options = options end - def is_spam? + def spam? return false unless akismet_enabled? params = { diff --git a/app/services/spam_service.rb b/app/services/spam_service.rb index 3e65b7d31a3..73ea3018fbd 100644 --- a/app/services/spam_service.rb +++ b/app/services/spam_service.rb @@ -45,7 +45,7 @@ class SpamService def check(api) return false unless request && check_for_spam? - return false unless akismet.is_spam? + return false unless akismet.spam? create_spam_log(api) true diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index b571b11dcac..da8f9e8376e 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -268,7 +268,7 @@ describe Projects::IssuesController do context 'when an issue is not identified as spam' do before do allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false) - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(false) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(false) end it 'normally updates the issue' do @@ -278,7 +278,7 @@ describe Projects::IssuesController do context 'when an issue is identified as spam' do before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end context 'when captcha is not verified' do @@ -672,7 +672,7 @@ describe Projects::IssuesController do context 'when an issue is not identified as spam' do before do allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false) - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(false) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(false) end it 'does not create an issue' do @@ -682,7 +682,7 @@ describe Projects::IssuesController do context 'when an issue is identified as spam' do before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end context 'when captcha is not verified' do diff --git a/spec/controllers/projects/snippets_controller_spec.rb b/spec/controllers/projects/snippets_controller_spec.rb index cc444f31797..3a1550aa730 100644 --- a/spec/controllers/projects/snippets_controller_spec.rb +++ b/spec/controllers/projects/snippets_controller_spec.rb @@ -98,7 +98,7 @@ describe Projects::SnippetsController do context 'when the snippet is spam' do before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end context 'when the snippet is private' do @@ -176,7 +176,7 @@ describe Projects::SnippetsController do context 'when the snippet is spam' do before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end context 'when the snippet is private' do diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index 7c5d059760f..be273acb69b 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -217,7 +217,7 @@ describe SnippetsController do context 'when the snippet is spam' do before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end context 'when the snippet is private' do @@ -289,7 +289,7 @@ describe SnippetsController do context 'when the snippet is spam' do before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end context 'when the snippet is private' do diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 7d120e4a234..47f781eab4d 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -984,7 +984,7 @@ describe API::Issues, :mailer do describe 'POST /projects/:id/issues with spam filtering' do before do allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) - allow_any_instance_of(AkismetService).to receive_messages(is_spam?: true) + allow_any_instance_of(AkismetService).to receive_messages(spam?: true) end let(:params) do @@ -1114,7 +1114,7 @@ describe API::Issues, :mailer do it "does not create a new project issue" do allow_any_instance_of(SpamService).to receive_messages(check_for_spam?: true) - allow_any_instance_of(AkismetService).to receive_messages(is_spam?: true) + allow_any_instance_of(AkismetService).to receive_messages(spam?: true) put api("/projects/#{project.id}/issues/#{issue.iid}", user), params diff --git a/spec/requests/api/project_snippets_spec.rb b/spec/requests/api/project_snippets_spec.rb index 2b541f5719e..b64d8b1bb63 100644 --- a/spec/requests/api/project_snippets_spec.rb +++ b/spec/requests/api/project_snippets_spec.rb @@ -117,7 +117,7 @@ describe API::ProjectSnippets do end before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end context 'when the snippet is private' do @@ -179,7 +179,7 @@ describe API::ProjectSnippets do end before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end context 'when the snippet is private' do diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb index d09b8bc42f1..68ab406770c 100644 --- a/spec/requests/api/snippets_spec.rb +++ b/spec/requests/api/snippets_spec.rb @@ -137,7 +137,7 @@ describe API::Snippets do end before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end context 'when the snippet is private' do @@ -209,7 +209,7 @@ describe API::Snippets do end before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end context 'when the snippet is private' do diff --git a/spec/requests/api/v3/issues_spec.rb b/spec/requests/api/v3/issues_spec.rb index 9eb538c4b09..9a0e6647ebf 100644 --- a/spec/requests/api/v3/issues_spec.rb +++ b/spec/requests/api/v3/issues_spec.rb @@ -884,7 +884,7 @@ describe API::V3::Issues, :mailer do describe 'POST /projects/:id/issues with spam filtering' do before do allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) - allow_any_instance_of(AkismetService).to receive_messages(is_spam?: true) + allow_any_instance_of(AkismetService).to receive_messages(spam?: true) end let(:params) do @@ -1016,7 +1016,7 @@ describe API::V3::Issues, :mailer do it "does not create a new project issue" do allow_any_instance_of(SpamService).to receive_messages(check_for_spam?: true) - allow_any_instance_of(AkismetService).to receive_messages(is_spam?: true) + allow_any_instance_of(AkismetService).to receive_messages(spam?: true) put v3_api("/projects/#{project.id}/issues/#{issue.id}", user), params diff --git a/spec/requests/api/v3/project_snippets_spec.rb b/spec/requests/api/v3/project_snippets_spec.rb index 3963924a066..7e88489082a 100644 --- a/spec/requests/api/v3/project_snippets_spec.rb +++ b/spec/requests/api/v3/project_snippets_spec.rb @@ -80,7 +80,7 @@ describe API::ProjectSnippets do end before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end context 'when the snippet is private' do @@ -140,7 +140,7 @@ describe API::ProjectSnippets do end before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end context 'when the snippet is private' do diff --git a/spec/requests/api/v3/snippets_spec.rb b/spec/requests/api/v3/snippets_spec.rb index 9ead3cad8bb..79860725634 100644 --- a/spec/requests/api/v3/snippets_spec.rb +++ b/spec/requests/api/v3/snippets_spec.rb @@ -107,7 +107,7 @@ describe API::V3::Snippets do end before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end context 'when the snippet is private' do diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 78b11cd7991..cc3d648c340 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -370,7 +370,7 @@ describe Issues::CreateService do context 'when recaptcha was not verified' do context 'when akismet detects spam' do before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) end it 'marks an issue as a spam ' do @@ -392,7 +392,7 @@ describe Issues::CreateService do context 'when akismet does not detect spam' do before do - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(false) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(false) end it 'does not mark an issue as a spam ' do diff --git a/spec/services/spam_service_spec.rb b/spec/services/spam_service_spec.rb index a14dfa3f01f..61312d55b84 100644 --- a/spec/services/spam_service_spec.rb +++ b/spec/services/spam_service_spec.rb @@ -23,7 +23,7 @@ describe SpamService do before do issue.closed_at = Time.zone.now - allow(AkismetService).to receive(:new).and_return(double(is_spam?: true)) + allow(AkismetService).to receive(:new).and_return(double(spam?: true)) end it 'returns false' do @@ -43,7 +43,7 @@ describe SpamService do context 'when indicated as spam by akismet' do before do - allow(AkismetService).to receive(:new).and_return(double(is_spam?: true)) + allow(AkismetService).to receive(:new).and_return(double(spam?: true)) end it 'doesnt check as spam when request is missing' do @@ -71,7 +71,7 @@ describe SpamService do context 'when not indicated as spam by akismet' do before do - allow(AkismetService).to receive(:new).and_return(double(is_spam?: false)) + allow(AkismetService).to receive(:new).and_return(double(spam?: false)) end it 'returns false' do From eb0a17ba4bf05b7bcefb31755381d08fd689b818 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 20:06:04 +0300 Subject: [PATCH 32/70] replace `is_team_member?` with `team_member?` --- app/policies/project_policy.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 0133091db57..a925fac7d3e 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -17,13 +17,13 @@ class ProjectPolicy < BasePolicy desc "Project has public builds enabled" condition(:public_builds, scope: :subject) { project.public_builds? } - # For guest access we use #is_team_member? so we can use + # For guest access we use #team_member? so we can use # project.members, which gets cached in subject scope. # This is safe because team_access_level is guaranteed # by ProjectAuthorization's validation to be at minimum # GUEST desc "User has guest access" - condition(:guest) { is_team_member? } + condition(:guest) { team_member? } desc "User has reporter access" condition(:reporter) { team_access_level >= Gitlab::Access::REPORTER } @@ -293,7 +293,7 @@ class ProjectPolicy < BasePolicy private - def is_team_member? + def team_member? return false if @user.nil? greedy_load_subject = false From e6c7c11a5bb2656c2be997368b62aca61bb1f485 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 20:16:31 +0300 Subject: [PATCH 33/70] replace `has_matching_label` with `has_matching_label?` --- .rubocop_todo.yml | 3 +++ lib/gitlab/prometheus/queries/matched_metrics_query.rb | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 4141d221039..f51228180c8 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -244,8 +244,11 @@ Style/PerlBackrefs: # NameWhitelist: is_a? Style/PredicateName: Enabled: true + Exclude: + - 'features/*' # NamePrefix: is_ NamePrefixBlacklist: is_ + NameWhitelist: has_n_stars # Offense count: 58 # Cop supports --auto-correct. diff --git a/lib/gitlab/prometheus/queries/matched_metrics_query.rb b/lib/gitlab/prometheus/queries/matched_metrics_query.rb index d4894c87f8d..4c3edccc71a 100644 --- a/lib/gitlab/prometheus/queries/matched_metrics_query.rb +++ b/lib/gitlab/prometheus/queries/matched_metrics_query.rb @@ -42,13 +42,13 @@ module Gitlab lookup = series.each_slice(MAX_QUERY_ITEMS).flat_map do |batched_series| client_series(*batched_series, start: timeframe_start, stop: timeframe_end) - .select(&method(:has_matching_label)) + .select(&method(:has_matching_label?)) .map { |series_info| [series_info['__name__'], true] } end lookup.to_h end - def has_matching_label(series_info) + def has_matching_label?(series_info) series_info.key?('environment') end From 81061aef62e603aa122c5c3a58ca8cfd9341a59b Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 20:17:21 +0300 Subject: [PATCH 34/70] replace `has_n_stars` with `has_n_stars?` --- features/steps/project/star.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/features/steps/project/star.rb b/features/steps/project/star.rb index 9f7c748a3b7..2e7eb65901a 100644 --- a/features/steps/project/star.rb +++ b/features/steps/project/star.rb @@ -9,15 +9,15 @@ class Spinach::Features::ProjectStar < Spinach::FeatureSteps end step "The project has 0 stars" do - has_n_stars(0) + has_n_stars?(0) end step "The project has 1 star" do - has_n_stars(1) + has_n_stars?(1) end step "The project has 2 stars" do - has_n_stars(2) + has_n_stars?(2) end # Requires @javascript @@ -31,7 +31,7 @@ class Spinach::Features::ProjectStar < Spinach::FeatureSteps protected - def has_n_stars(n) + def has_n_stars?(n) expect(page).to have_css(".star-count", text: n, visible: true) end end From 90112f57d26a21a6c219e24367b7ef91d93858ad Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 20:46:41 +0300 Subject: [PATCH 35/70] replace `is_member_of` with `member_of?` --- .rubocop_todo.yml | 6 +++--- features/steps/shared/group.rb | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index f51228180c8..9186501ca96 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -244,11 +244,11 @@ Style/PerlBackrefs: # NameWhitelist: is_a? Style/PredicateName: Enabled: true - Exclude: - - 'features/*' + # Exclude: + # - 'features/*' # NamePrefix: is_ NamePrefixBlacklist: is_ - NameWhitelist: has_n_stars + NameWhitelist: have_visible_content # Offense count: 58 # Cop supports --auto-correct. diff --git a/features/steps/shared/group.rb b/features/steps/shared/group.rb index de119f2c6c0..45fad83e8bb 100644 --- a/features/steps/shared/group.rb +++ b/features/steps/shared/group.rb @@ -2,27 +2,27 @@ module SharedGroup include Spinach::DSL step 'current user is developer of group "Owned"' do - is_member_of(current_user.name, "Owned", Gitlab::Access::DEVELOPER) + member_of?(current_user.name, "Owned", Gitlab::Access::DEVELOPER) end step '"John Doe" is owner of group "Owned"' do - is_member_of("John Doe", "Owned", Gitlab::Access::OWNER) + member_of?("John Doe", "Owned", Gitlab::Access::OWNER) end step '"John Doe" is guest of group "Guest"' do - is_member_of("John Doe", "Guest", Gitlab::Access::GUEST) + member_of?("John Doe", "Guest", Gitlab::Access::GUEST) end step '"Mary Jane" is owner of group "Owned"' do - is_member_of("Mary Jane", "Owned", Gitlab::Access::OWNER) + member_of?("Mary Jane", "Owned", Gitlab::Access::OWNER) end step '"Mary Jane" is guest of group "Owned"' do - is_member_of("Mary Jane", "Owned", Gitlab::Access::GUEST) + member_of?("Mary Jane", "Owned", Gitlab::Access::GUEST) end step '"Mary Jane" is guest of group "Guest"' do - is_member_of("Mary Jane", "Guest", Gitlab::Access::GUEST) + member_of?("Mary Jane", "Guest", Gitlab::Access::GUEST) end step 'I should see group "TestGroup"' do @@ -35,7 +35,7 @@ module SharedGroup protected - def is_member_of(username, groupname, role) + def member_of?(username, groupname, role) @project_count ||= 0 user = User.find_by(name: username) || create(:user, name: username) group = Group.find_by(name: groupname) || create(:group, name: groupname) From 10ae0d8316bd9ae5aa1ce46fcea3ec8f01a3b336 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 20:52:43 +0300 Subject: [PATCH 36/70] replace `is_ancestor` with `ancestor?` --- lib/gitlab/checks/force_push.rb | 2 +- lib/gitlab/git/repository.rb | 2 +- lib/gitlab/gitaly_client/commit_service.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/checks/force_push.rb b/lib/gitlab/checks/force_push.rb index 714464fd5e7..dc5d285ea65 100644 --- a/lib/gitlab/checks/force_push.rb +++ b/lib/gitlab/checks/force_push.rb @@ -12,7 +12,7 @@ module Gitlab !project .repository .gitaly_commit_client - .is_ancestor(oldrev, newrev) + .ancestor?(oldrev, newrev) else Gitlab::Git::RevList.new( path_to_repo: project.repository.path_to_repo, diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index dce3a9b2d37..03e2bec84dd 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -440,7 +440,7 @@ module Gitlab # Returns true is +from+ is direct ancestor to +to+, otherwise false def ancestor?(from, to) - gitaly_commit_client.is_ancestor(from, to) + gitaly_commit_client.ancestor?(from, to) end # Return an array of Diff objects that represent the diff diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index 57f42bd35ee..21a32a7e0db 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -22,7 +22,7 @@ module Gitlab end end - def is_ancestor(ancestor_id, child_id) + def ancestor?(ancestor_id, child_id) request = Gitaly::CommitIsAncestorRequest.new( repository: @gitaly_repo, ancestor_id: ancestor_id, From e637f802431bc3d9b289299c1d5d4d2a6127cf6b Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 24 Aug 2017 21:02:37 +0300 Subject: [PATCH 37/70] add changelog --- .rubocop_todo.yml | 3 --- changelogs/unreleased/35793_fix_predicate_names.yml | 5 +++++ 2 files changed, 5 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/35793_fix_predicate_names.yml diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 9186501ca96..9e6412a436d 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -244,9 +244,6 @@ Style/PerlBackrefs: # NameWhitelist: is_a? Style/PredicateName: Enabled: true - # Exclude: - # - 'features/*' - # NamePrefix: is_ NamePrefixBlacklist: is_ NameWhitelist: have_visible_content diff --git a/changelogs/unreleased/35793_fix_predicate_names.yml b/changelogs/unreleased/35793_fix_predicate_names.yml new file mode 100644 index 00000000000..d4da177dc2e --- /dev/null +++ b/changelogs/unreleased/35793_fix_predicate_names.yml @@ -0,0 +1,5 @@ +--- +title: Remove `is_` prefix from predicate method names +merge_request: 13810 +author: Maxim Rydkin +type: other From 380dff622f83e199a08f37692395bfe3a3e5b437 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Tue, 29 Aug 2017 11:51:15 +0300 Subject: [PATCH 38/70] exclude spec/ and features/ from `Style/PredicateName` cop --- .rubocop.yml | 12 ++++++++++++ .rubocop_todo.yml | 10 ---------- features/steps/project/star.rb | 8 ++++---- features/steps/shared/group.rb | 14 +++++++------- 4 files changed, 23 insertions(+), 21 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index abdda90a33e..e96f4273c6a 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -606,6 +606,18 @@ Style/YodaCondition: Style/Proc: Enabled: true +# Use `spam?` instead of `is_spam?` +# Configuration parameters: NamePrefix, NamePrefixBlacklist, NameWhitelist. +# NamePrefix: is_, has_, have_ +# NamePrefixBlacklist: is_, has_, have_ +# NameWhitelist: is_a? +Style/PredicateName: + Enabled: true + NamePrefixBlacklist: is_ + Exclude: + - 'spec/**/*' + - 'features/**/*' + # Metrics ##################################################################### # A calculated magnitude based on number of assignments, diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 9e6412a436d..cdf97d1d842 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -237,16 +237,6 @@ Style/PercentLiteralDelimiters: Style/PerlBackrefs: Enabled: false -# Offense count: 105 -# Configuration parameters: NamePrefix, NamePrefixBlacklist, NameWhitelist. -# NamePrefix: is_, has_, have_ -# NamePrefixBlacklist: is_, has_, have_ -# NameWhitelist: is_a? -Style/PredicateName: - Enabled: true - NamePrefixBlacklist: is_ - NameWhitelist: have_visible_content - # Offense count: 58 # Cop supports --auto-correct. # Configuration parameters: EnforcedStyle, SupportedStyles. diff --git a/features/steps/project/star.rb b/features/steps/project/star.rb index 2e7eb65901a..9f7c748a3b7 100644 --- a/features/steps/project/star.rb +++ b/features/steps/project/star.rb @@ -9,15 +9,15 @@ class Spinach::Features::ProjectStar < Spinach::FeatureSteps end step "The project has 0 stars" do - has_n_stars?(0) + has_n_stars(0) end step "The project has 1 star" do - has_n_stars?(1) + has_n_stars(1) end step "The project has 2 stars" do - has_n_stars?(2) + has_n_stars(2) end # Requires @javascript @@ -31,7 +31,7 @@ class Spinach::Features::ProjectStar < Spinach::FeatureSteps protected - def has_n_stars?(n) + def has_n_stars(n) expect(page).to have_css(".star-count", text: n, visible: true) end end diff --git a/features/steps/shared/group.rb b/features/steps/shared/group.rb index 45fad83e8bb..de119f2c6c0 100644 --- a/features/steps/shared/group.rb +++ b/features/steps/shared/group.rb @@ -2,27 +2,27 @@ module SharedGroup include Spinach::DSL step 'current user is developer of group "Owned"' do - member_of?(current_user.name, "Owned", Gitlab::Access::DEVELOPER) + is_member_of(current_user.name, "Owned", Gitlab::Access::DEVELOPER) end step '"John Doe" is owner of group "Owned"' do - member_of?("John Doe", "Owned", Gitlab::Access::OWNER) + is_member_of("John Doe", "Owned", Gitlab::Access::OWNER) end step '"John Doe" is guest of group "Guest"' do - member_of?("John Doe", "Guest", Gitlab::Access::GUEST) + is_member_of("John Doe", "Guest", Gitlab::Access::GUEST) end step '"Mary Jane" is owner of group "Owned"' do - member_of?("Mary Jane", "Owned", Gitlab::Access::OWNER) + is_member_of("Mary Jane", "Owned", Gitlab::Access::OWNER) end step '"Mary Jane" is guest of group "Owned"' do - member_of?("Mary Jane", "Owned", Gitlab::Access::GUEST) + is_member_of("Mary Jane", "Owned", Gitlab::Access::GUEST) end step '"Mary Jane" is guest of group "Guest"' do - member_of?("Mary Jane", "Guest", Gitlab::Access::GUEST) + is_member_of("Mary Jane", "Guest", Gitlab::Access::GUEST) end step 'I should see group "TestGroup"' do @@ -35,7 +35,7 @@ module SharedGroup protected - def member_of?(username, groupname, role) + def is_member_of(username, groupname, role) @project_count ||= 0 user = User.find_by(name: username) || create(:user, name: username) group = Group.find_by(name: groupname) || create(:group, name: groupname) From 12633b46b6884dda4ffd87b14b4b52725acd6ec1 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Tue, 29 Aug 2017 18:00:03 +0900 Subject: [PATCH 39/70] Refactor --- lib/gitlab/sql/pattern.rb | 10 +++------- spec/lib/gitlab/sql/pattern_spec.rb | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb index 26bfeeeee67..b42bc67ccfc 100644 --- a/lib/gitlab/sql/pattern.rb +++ b/lib/gitlab/sql/pattern.rb @@ -7,17 +7,13 @@ module Gitlab class_methods do def to_pattern(query) - if exact_matching?(query) - sanitize_sql_like(query) - else + if partial_matching?(query) "%#{sanitize_sql_like(query)}%" + else + sanitize_sql_like(query) end end - def exact_matching?(query) - query.length < MIN_CHARS_FOR_PARTIAL_MATCHING - end - def partial_matching?(query) query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING end diff --git a/spec/lib/gitlab/sql/pattern_spec.rb b/spec/lib/gitlab/sql/pattern_spec.rb index 224336421be..9d7b2136dab 100644 --- a/spec/lib/gitlab/sql/pattern_spec.rb +++ b/spec/lib/gitlab/sql/pattern_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::SQL::Pattern do - describe '#to_pattern' do + describe '.to_pattern' do subject(:to_pattern) { User.to_pattern(query) } context 'when a query is shorter than 3 chars' do From 11da029edb0bf09eb906301bd0692ed1d74d25eb Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Mon, 28 Aug 2017 13:50:21 +0200 Subject: [PATCH 40/70] Move GPG signed commits docs to new location Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/36804 --- app/views/profiles/gpg_keys/index.html.haml | 2 +- .../commit/_signature_badge.html.haml | 2 +- doc/README.md | 3 +- doc/user/project/gpg_signed_commits/index.md | 246 +----------------- .../profile_settings_gpg_keys_paste_pub.png | Bin .../profile_settings_gpg_keys_single_key.png | Bin .../project_signed_and_unsigned_commits.png | Bin ...ect_signed_commit_unverified_signature.png | Bin ...oject_signed_commit_verified_signature.png | Bin .../repository/gpg_signed_commits/index.md | 245 +++++++++++++++++ doc/user/project/repository/index.md | 4 +- 11 files changed, 253 insertions(+), 249 deletions(-) rename doc/user/project/{ => repository}/gpg_signed_commits/img/profile_settings_gpg_keys_paste_pub.png (100%) rename doc/user/project/{ => repository}/gpg_signed_commits/img/profile_settings_gpg_keys_single_key.png (100%) rename doc/user/project/{ => repository}/gpg_signed_commits/img/project_signed_and_unsigned_commits.png (100%) rename doc/user/project/{ => repository}/gpg_signed_commits/img/project_signed_commit_unverified_signature.png (100%) rename doc/user/project/{ => repository}/gpg_signed_commits/img/project_signed_commit_verified_signature.png (100%) create mode 100644 doc/user/project/repository/gpg_signed_commits/index.md diff --git a/app/views/profiles/gpg_keys/index.html.haml b/app/views/profiles/gpg_keys/index.html.haml index 720a97cddb7..8dbb8aef31b 100644 --- a/app/views/profiles/gpg_keys/index.html.haml +++ b/app/views/profiles/gpg_keys/index.html.haml @@ -12,7 +12,7 @@ Add a GPG key %p.profile-settings-content Before you can add a GPG key you need to - = link_to 'generate it.', help_page_path('user/project/gpg_signed_commits/index.md') + = link_to 'generate it.', help_page_path('user/project/repository/gpg_signed_commits/index.md') = render 'form' %hr %h5 diff --git a/app/views/projects/commit/_signature_badge.html.haml b/app/views/projects/commit/_signature_badge.html.haml index a3783b31b86..d06b29db838 100644 --- a/app/views/projects/commit/_signature_badge.html.haml +++ b/app/views/projects/commit/_signature_badge.html.haml @@ -12,7 +12,7 @@ %span.monospace= signature.gpg_key_primary_keyid - = link_to('Learn more about signing commits', help_page_path('user/project/gpg_signed_commits/index.md'), class: 'gpg-popover-help-link') + = link_to('Learn more about signing commits', help_page_path('user/project/repository/gpg_signed_commits/index.md'), class: 'gpg-popover-help-link') %button{ class: css_classes, data: { toggle: 'popover', html: 'true', placement: 'auto top', title: title, content: content } } = label diff --git a/doc/README.md b/doc/README.md index 76d4c3e51fe..63ba8ff03e9 100644 --- a/doc/README.md +++ b/doc/README.md @@ -77,6 +77,8 @@ Manage your [repositories](user/project/repository/index.md) from the UI (user i - [Create a branch](user/project/repository/web_editor.md#create-a-new-branch) - [Protected branches](user/project/protected_branches.md#protected-branches) - [Delete merged branches](user/project/repository/branches/index.md#delete-merged-branches) +- Commits + - [Signing commits](user/project/repository/gpg_signed_commits/index.md): use GPG to sign your commits. ### Issues and Merge Requests (MRs) @@ -98,7 +100,6 @@ Manage your [repositories](user/project/repository/index.md) from the UI (user i - [Git](topics/git/index.md): Getting started with Git, branching strategies, Git LFS, advanced use. - [Git cheatsheet](https://gitlab.com/gitlab-com/marketing/raw/master/design/print/git-cheatsheet/print-pdf/git-cheatsheet.pdf): Download a PDF describing the most used Git operations. - [GitLab Flow](workflow/gitlab_flow.md): explore the best of Git with the GitLab Flow strategy. -- [Signing commits](user/project/gpg_signed_commits/index.md): use GPG to sign your commits. ### Migrate and import your projects from other platforms diff --git a/doc/user/project/gpg_signed_commits/index.md b/doc/user/project/gpg_signed_commits/index.md index 3ea2203c895..261eedeb412 100644 --- a/doc/user/project/gpg_signed_commits/index.md +++ b/doc/user/project/gpg_signed_commits/index.md @@ -1,245 +1 @@ -# Signing commits with GPG - -> [Introduced][ce-9546] in GitLab 9.5. - -GitLab can show whether a commit is verified or not when signed with a GPG key. -All you need to do is upload the public GPG key in your profile settings. - -GPG verified tags are not supported yet. - -## Getting started with GPG - -Here are a few guides to get you started with GPG: - -- [Git Tools - Signing Your Work](https://git-scm.com/book/en/v2/Git-Tools-Signing-Your-Work) -- [Managing OpenPGP Keys](https://riseup.net/en/security/message-security/openpgp/gpg-keys) -- [OpenPGP Best Practices](https://riseup.net/en/security/message-security/openpgp/best-practices) -- [Creating a new GPG key with subkeys](https://www.void.gr/kargig/blog/2013/12/02/creating-a-new-gpg-key-with-subkeys/) (advanced) - -## How GitLab handles GPG - -GitLab uses its own keyring to verify the GPG signature. It does not access any -public key server. - -In order to have a commit verified on GitLab the corresponding public key needs -to be uploaded to GitLab. For a signature to be verified two prerequisites need -to be met: - -1. The public key needs to be added your GitLab account -1. One of the emails in the GPG key matches your **primary** email - -## Generating a GPG key - -If you don't already have a GPG key, the following steps will help you get -started: - -1. [Install GPG](https://www.gnupg.org/download/index.html) for your operating system -1. Generate the private/public key pair with the following command: - - ```sh - gpg --full-gen-key - ``` - - This will spawn a series of questions. - -1. The first question is which algorithm can be used. Select the kind you want - or press Enter to choose the default (RSA and RSA): - - ``` - Please select what kind of key you want: - (1) RSA and RSA (default) - (2) DSA and Elgamal - (3) DSA (sign only) - (4) RSA (sign only) - Your selection? 1 - ``` - -1. The next question is key length. We recommend to choose the highest value - which is `4096`: - - ``` - RSA keys may be between 1024 and 4096 bits long. - What keysize do you want? (2048) 4096 - Requested keysize is 4096 bits - ``` -1. Next, you need to specify the validity period of your key. This is something - subjective, and you can use the default value which is to never expire: - - ``` - Please specify how long the key should be valid. - 0 = key does not expire - = key expires in n days - w = key expires in n weeks - m = key expires in n months - y = key expires in n years - Key is valid for? (0) 0 - Key does not expire at all - ``` - -1. Confirm that the answers you gave were correct by typing `y`: - - ``` - Is this correct? (y/N) y - ``` - -1. Enter you real name, the email address to be associated with this key (should - match the primary email address you use in GitLab) and an optional comment - (press Enter to skip): - - ``` - GnuPG needs to construct a user ID to identify your key. - - Real name: Mr. Robot - Email address: mr@robot.sh - Comment: - You selected this USER-ID: - "Mr. Robot " - - Change (N)ame, (C)omment, (E)mail or (O)kay/(Q)uit? O - ``` - -1. Pick a strong password when asked and type it twice to confirm. -1. Use the following command to list the private GPG key you just created: - - ``` - gpg --list-secret-keys mr@robot.sh - ``` - - Replace `mr@robot.sh` with the email address you entered above. - -1. Copy the GPG key ID that starts with `sec`. In the following example, that's - `0x30F2B65B9246B6CA`: - - ``` - sec rsa4096/0x30F2B65B9246B6CA 2017-08-18 [SC] - D5E4F29F3275DC0CDA8FFC8730F2B65B9246B6CA - uid [ultimate] Mr. Robot - ssb rsa4096/0xB7ABC0813E4028C0 2017-08-18 [E] - ``` - -1. Export the public key of that ID (replace your key ID from the previous step): - - ``` - gpg --armor --export 0x30F2B65B9246B6CA - ``` - -1. Finally, copy the public key and [add it in your profile settings](#adding-a-gpg-key-to-your-account) - -## Adding a GPG key to your account - ->**Note:** -Once you add a key, you cannot edit it, only remove it. In case the paste -didn't work, you'll have to remove the offending key and re-add it. - -You can add a GPG key in your profile's settings: - -1. On the upper right corner, click on your avatar and go to your **Settings**. - - ![Settings dropdown](../../profile/img/profile_settings_dropdown.png) - -1. Navigate to the **GPG keys** tab and paste your _public_ key in the 'Key' - box. - - ![Paste GPG public key](img/profile_settings_gpg_keys_paste_pub.png) - -1. Finally, click on **Add key** to add it to GitLab. You will be able to see - its fingerprint, the corresponding email address and creation date. - - ![GPG key single page](img/profile_settings_gpg_keys_single_key.png) - -## Associating your GPG key with Git - -After you have [created your GPG key](#generating-a-gpg-key) and [added it to -your account](#adding-a-gpg-key-to-your-account), it's time to tell Git which -key to use. - -1. Use the following command to list the private GPG key you just created: - - ``` - gpg --list-secret-keys mr@robot.sh - ``` - - Replace `mr@robot.sh` with the email address you entered above. - -1. Copy the GPG key ID that starts with `sec`. In the following example, that's - `0x30F2B65B9246B6CA`: - - ``` - sec rsa4096/0x30F2B65B9246B6CA 2017-08-18 [SC] - D5E4F29F3275DC0CDA8FFC8730F2B65B9246B6CA - uid [ultimate] Mr. Robot - ssb rsa4096/0xB7ABC0813E4028C0 2017-08-18 [E] - ``` - -1. Tell Git to use that key to sign the commits: - - ``` - git config --global user.signingkey 0x30F2B65B9246B6CA - ``` - - Replace `0x30F2B65B9246B6CA` with your GPG key ID. - -## Signing commits - -After you have [created your GPG key](#generating-a-gpg-key) and [added it to -your account](#adding-a-gpg-key-to-your-account), you can start signing your -commits: - -1. Commit like you used to, the only difference is the addition of the `-S` flag: - - ``` - git commit -S -m "My commit msg" - ``` - -1. Enter the passphrase of your GPG key when asked. -1. Push to GitLab and check that your commits [are verified](#verifying-commits). - -If you don't want to type the `-S` flag every time you commit, you can tell Git -to sign your commits automatically: - -``` -git config --global commit.gpgsign true -``` - -## Verifying commits - -1. Within a project or [merge request](../merge_requests/index.md), navigate to - the **Commits** tab. Signed commits will show a badge containing either - "Verified" or "Unverified", depending on the verification status of the GPG - signature. - - ![Signed and unsigned commits](img/project_signed_and_unsigned_commits.png) - -1. By clicking on the GPG badge, details of the signature are displayed. - - ![Signed commit with verified signature](img/project_signed_commit_verified_signature.png) - - ![Signed commit with verified signature](img/project_signed_commit_unverified_signature.png) - -## Revoking a GPG key - -Revoking a key **unverifies** already signed commits. Commits that were -verified by using this key will change to an unverified state. Future commits -will also stay unverified once you revoke this key. This action should be used -in case your key has been compromised. - -To revoke a GPG key: - -1. On the upper right corner, click on your avatar and go to your **Settings**. -1. Navigate to the **GPG keys** tab. -1. Click on **Revoke** besides the GPG key you want to delete. - -## Removing a GPG key - -Removing a key **does not unverify** already signed commits. Commits that were -verified by using this key will stay verified. Only unpushed commits will stay -unverified once you remove this key. To unverify already signed commits, you need -to [revoke the associated GPG key](#revoking-a-gpg-key) from your account. - -To remove a GPG key from your account: - -1. On the upper right corner, click on your avatar and go to your **Settings**. -1. Navigate to the **GPG keys** tab. -1. Click on the trash icon besides the GPG key you want to delete. - -[ce-9546]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546 +This document was moved to [another location](../repository/gpg_signed_commits/index.md). diff --git a/doc/user/project/gpg_signed_commits/img/profile_settings_gpg_keys_paste_pub.png b/doc/user/project/repository/gpg_signed_commits/img/profile_settings_gpg_keys_paste_pub.png similarity index 100% rename from doc/user/project/gpg_signed_commits/img/profile_settings_gpg_keys_paste_pub.png rename to doc/user/project/repository/gpg_signed_commits/img/profile_settings_gpg_keys_paste_pub.png diff --git a/doc/user/project/gpg_signed_commits/img/profile_settings_gpg_keys_single_key.png b/doc/user/project/repository/gpg_signed_commits/img/profile_settings_gpg_keys_single_key.png similarity index 100% rename from doc/user/project/gpg_signed_commits/img/profile_settings_gpg_keys_single_key.png rename to doc/user/project/repository/gpg_signed_commits/img/profile_settings_gpg_keys_single_key.png diff --git a/doc/user/project/gpg_signed_commits/img/project_signed_and_unsigned_commits.png b/doc/user/project/repository/gpg_signed_commits/img/project_signed_and_unsigned_commits.png similarity index 100% rename from doc/user/project/gpg_signed_commits/img/project_signed_and_unsigned_commits.png rename to doc/user/project/repository/gpg_signed_commits/img/project_signed_and_unsigned_commits.png diff --git a/doc/user/project/gpg_signed_commits/img/project_signed_commit_unverified_signature.png b/doc/user/project/repository/gpg_signed_commits/img/project_signed_commit_unverified_signature.png similarity index 100% rename from doc/user/project/gpg_signed_commits/img/project_signed_commit_unverified_signature.png rename to doc/user/project/repository/gpg_signed_commits/img/project_signed_commit_unverified_signature.png diff --git a/doc/user/project/gpg_signed_commits/img/project_signed_commit_verified_signature.png b/doc/user/project/repository/gpg_signed_commits/img/project_signed_commit_verified_signature.png similarity index 100% rename from doc/user/project/gpg_signed_commits/img/project_signed_commit_verified_signature.png rename to doc/user/project/repository/gpg_signed_commits/img/project_signed_commit_verified_signature.png diff --git a/doc/user/project/repository/gpg_signed_commits/index.md b/doc/user/project/repository/gpg_signed_commits/index.md new file mode 100644 index 00000000000..ff419d714f9 --- /dev/null +++ b/doc/user/project/repository/gpg_signed_commits/index.md @@ -0,0 +1,245 @@ +# Signing commits with GPG + +> [Introduced][ce-9546] in GitLab 9.5. + +GitLab can show whether a commit is verified or not when signed with a GPG key. +All you need to do is upload the public GPG key in your profile settings. + +GPG verified tags are not supported yet. + +## Getting started with GPG + +Here are a few guides to get you started with GPG: + +- [Git Tools - Signing Your Work](https://git-scm.com/book/en/v2/Git-Tools-Signing-Your-Work) +- [Managing OpenPGP Keys](https://riseup.net/en/security/message-security/openpgp/gpg-keys) +- [OpenPGP Best Practices](https://riseup.net/en/security/message-security/openpgp/best-practices) +- [Creating a new GPG key with subkeys](https://www.void.gr/kargig/blog/2013/12/02/creating-a-new-gpg-key-with-subkeys/) (advanced) + +## How GitLab handles GPG + +GitLab uses its own keyring to verify the GPG signature. It does not access any +public key server. + +In order to have a commit verified on GitLab the corresponding public key needs +to be uploaded to GitLab. For a signature to be verified two prerequisites need +to be met: + +1. The public key needs to be added your GitLab account +1. One of the emails in the GPG key matches your **primary** email + +## Generating a GPG key + +If you don't already have a GPG key, the following steps will help you get +started: + +1. [Install GPG](https://www.gnupg.org/download/index.html) for your operating system +1. Generate the private/public key pair with the following command: + + ```sh + gpg --full-gen-key + ``` + + This will spawn a series of questions. + +1. The first question is which algorithm can be used. Select the kind you want + or press Enter to choose the default (RSA and RSA): + + ``` + Please select what kind of key you want: + (1) RSA and RSA (default) + (2) DSA and Elgamal + (3) DSA (sign only) + (4) RSA (sign only) + Your selection? 1 + ``` + +1. The next question is key length. We recommend to choose the highest value + which is `4096`: + + ``` + RSA keys may be between 1024 and 4096 bits long. + What keysize do you want? (2048) 4096 + Requested keysize is 4096 bits + ``` +1. Next, you need to specify the validity period of your key. This is something + subjective, and you can use the default value which is to never expire: + + ``` + Please specify how long the key should be valid. + 0 = key does not expire + = key expires in n days + w = key expires in n weeks + m = key expires in n months + y = key expires in n years + Key is valid for? (0) 0 + Key does not expire at all + ``` + +1. Confirm that the answers you gave were correct by typing `y`: + + ``` + Is this correct? (y/N) y + ``` + +1. Enter you real name, the email address to be associated with this key (should + match the primary email address you use in GitLab) and an optional comment + (press Enter to skip): + + ``` + GnuPG needs to construct a user ID to identify your key. + + Real name: Mr. Robot + Email address: mr@robot.sh + Comment: + You selected this USER-ID: + "Mr. Robot " + + Change (N)ame, (C)omment, (E)mail or (O)kay/(Q)uit? O + ``` + +1. Pick a strong password when asked and type it twice to confirm. +1. Use the following command to list the private GPG key you just created: + + ``` + gpg --list-secret-keys mr@robot.sh + ``` + + Replace `mr@robot.sh` with the email address you entered above. + +1. Copy the GPG key ID that starts with `sec`. In the following example, that's + `0x30F2B65B9246B6CA`: + + ``` + sec rsa4096/0x30F2B65B9246B6CA 2017-08-18 [SC] + D5E4F29F3275DC0CDA8FFC8730F2B65B9246B6CA + uid [ultimate] Mr. Robot + ssb rsa4096/0xB7ABC0813E4028C0 2017-08-18 [E] + ``` + +1. Export the public key of that ID (replace your key ID from the previous step): + + ``` + gpg --armor --export 0x30F2B65B9246B6CA + ``` + +1. Finally, copy the public key and [add it in your profile settings](#adding-a-gpg-key-to-your-account) + +## Adding a GPG key to your account + +>**Note:** +Once you add a key, you cannot edit it, only remove it. In case the paste +didn't work, you'll have to remove the offending key and re-add it. + +You can add a GPG key in your profile's settings: + +1. On the upper right corner, click on your avatar and go to your **Settings**. + + ![Settings dropdown](../../../profile/img/profile_settings_dropdown.png) + +1. Navigate to the **GPG keys** tab and paste your _public_ key in the 'Key' + box. + + ![Paste GPG public key](img/profile_settings_gpg_keys_paste_pub.png) + +1. Finally, click on **Add key** to add it to GitLab. You will be able to see + its fingerprint, the corresponding email address and creation date. + + ![GPG key single page](img/profile_settings_gpg_keys_single_key.png) + +## Associating your GPG key with Git + +After you have [created your GPG key](#generating-a-gpg-key) and [added it to +your account](#adding-a-gpg-key-to-your-account), it's time to tell Git which +key to use. + +1. Use the following command to list the private GPG key you just created: + + ``` + gpg --list-secret-keys mr@robot.sh + ``` + + Replace `mr@robot.sh` with the email address you entered above. + +1. Copy the GPG key ID that starts with `sec`. In the following example, that's + `0x30F2B65B9246B6CA`: + + ``` + sec rsa4096/0x30F2B65B9246B6CA 2017-08-18 [SC] + D5E4F29F3275DC0CDA8FFC8730F2B65B9246B6CA + uid [ultimate] Mr. Robot + ssb rsa4096/0xB7ABC0813E4028C0 2017-08-18 [E] + ``` + +1. Tell Git to use that key to sign the commits: + + ``` + git config --global user.signingkey 0x30F2B65B9246B6CA + ``` + + Replace `0x30F2B65B9246B6CA` with your GPG key ID. + +## Signing commits + +After you have [created your GPG key](#generating-a-gpg-key) and [added it to +your account](#adding-a-gpg-key-to-your-account), you can start signing your +commits: + +1. Commit like you used to, the only difference is the addition of the `-S` flag: + + ``` + git commit -S -m "My commit msg" + ``` + +1. Enter the passphrase of your GPG key when asked. +1. Push to GitLab and check that your commits [are verified](#verifying-commits). + +If you don't want to type the `-S` flag every time you commit, you can tell Git +to sign your commits automatically: + +``` +git config --global commit.gpgsign true +``` + +## Verifying commits + +1. Within a project or [merge request](../../merge_requests/index.md), navigate to + the **Commits** tab. Signed commits will show a badge containing either + "Verified" or "Unverified", depending on the verification status of the GPG + signature. + + ![Signed and unsigned commits](img/project_signed_and_unsigned_commits.png) + +1. By clicking on the GPG badge, details of the signature are displayed. + + ![Signed commit with verified signature](img/project_signed_commit_verified_signature.png) + + ![Signed commit with verified signature](img/project_signed_commit_unverified_signature.png) + +## Revoking a GPG key + +Revoking a key **unverifies** already signed commits. Commits that were +verified by using this key will change to an unverified state. Future commits +will also stay unverified once you revoke this key. This action should be used +in case your key has been compromised. + +To revoke a GPG key: + +1. On the upper right corner, click on your avatar and go to your **Settings**. +1. Navigate to the **GPG keys** tab. +1. Click on **Revoke** besides the GPG key you want to delete. + +## Removing a GPG key + +Removing a key **does not unverify** already signed commits. Commits that were +verified by using this key will stay verified. Only unpushed commits will stay +unverified once you remove this key. To unverify already signed commits, you need +to [revoke the associated GPG key](#revoking-a-gpg-key) from your account. + +To remove a GPG key from your account: + +1. On the upper right corner, click on your avatar and go to your **Settings**. +1. Navigate to the **GPG keys** tab. +1. Click on the trash icon besides the GPG key you want to delete. + +[ce-9546]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546 diff --git a/doc/user/project/repository/index.md b/doc/user/project/repository/index.md index 5e5ae880518..235af83353d 100644 --- a/doc/user/project/repository/index.md +++ b/doc/user/project/repository/index.md @@ -22,7 +22,7 @@ you to [connect with GitLab via SSH](../../../ssh/README.md). ## Files -## Create and edit files +### Create and edit files Host your codebase in GitLab repositories by pushing your files to GitLab. You can either use the user interface (UI), or connect your local computer @@ -111,6 +111,8 @@ right from the UI. - **Revert a commit:** Easily [revert a commit](../merge_requests/revert_changes.md#reverting-a-commit) from the UI to a selected branch. +- **Sign a commit:** +Use GPG to [sign your commits](gpg_signed_commits/index.md). ## Repository size From ab3c8302afdbbcd608e2768f9db5e53ec4358512 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 29 Aug 2017 13:34:12 +0100 Subject: [PATCH 41/70] Fixes the diff changes buttons from toggling when scrolling Closes #36698 --- app/assets/javascripts/lib/utils/sticky.js | 2 +- app/assets/javascripts/merge_request_tabs.js | 1 + changelogs/unreleased/changes-bar-sticky-fix.yml | 5 +++++ spec/javascripts/merge_request_tabs_spec.js | 11 +++++++++++ 4 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/changes-bar-sticky-fix.yml diff --git a/app/assets/javascripts/lib/utils/sticky.js b/app/assets/javascripts/lib/utils/sticky.js index ff2b66046b4..283c0ec0410 100644 --- a/app/assets/javascripts/lib/utils/sticky.js +++ b/app/assets/javascripts/lib/utils/sticky.js @@ -1,5 +1,5 @@ export const isSticky = (el, scrollY, stickyTop) => { - const top = el.offsetTop - scrollY; + const top = Math.floor(el.offsetTop - scrollY); if (top <= stickyTop) { el.classList.add('is-stuck'); diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 5a9b3d19f84..3b3620fe61b 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -253,6 +253,7 @@ import bp from './breakpoints'; loadDiff(source) { if (this.diffsLoaded) { + document.dispatchEvent(new CustomEvent('scroll')); return; } diff --git a/changelogs/unreleased/changes-bar-sticky-fix.yml b/changelogs/unreleased/changes-bar-sticky-fix.yml new file mode 100644 index 00000000000..7d62773ef7a --- /dev/null +++ b/changelogs/unreleased/changes-bar-sticky-fix.yml @@ -0,0 +1,5 @@ +--- +title: Fixed diff changes bar buttons from showing/hiding whilst scrolling +merge_request: +author: +type: fixed diff --git a/spec/javascripts/merge_request_tabs_spec.js b/spec/javascripts/merge_request_tabs_spec.js index dc40244c20e..8830a2d29e5 100644 --- a/spec/javascripts/merge_request_tabs_spec.js +++ b/spec/javascripts/merge_request_tabs_spec.js @@ -295,6 +295,17 @@ import 'vendor/jquery.scrollTo'; this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); }); + it('triggers scroll event when diff already loaded', function () { + spyOn(document, 'dispatchEvent'); + + this.class.diffsLoaded = true; + this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); + + expect( + document.dispatchEvent, + ).toHaveBeenCalledWith(new CustomEvent('scroll')); + }); + describe('with inline diff', () => { let noteId; let noteLineNumId; From 573f73f22a53ac6d72d4670f925eceb28f3cc8fc Mon Sep 17 00:00:00 2001 From: winh Date: Wed, 16 Aug 2017 13:15:40 +0200 Subject: [PATCH 42/70] Make issuable dashboard dropdowns consistent --- app/assets/stylesheets/framework/dropdowns.scss | 5 +++++ app/assets/stylesheets/framework/filters.scss | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index a45d5a6dca0..4cf2f46c6d3 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -771,6 +771,11 @@ &::before { top: 16px; } + + &.dropdown-menu-user-link::before { + top: 50%; + transform: translateY(-50%); + } } } } diff --git a/app/assets/stylesheets/framework/filters.scss b/app/assets/stylesheets/framework/filters.scss index a5d33d410fb..e4e095eaabf 100644 --- a/app/assets/stylesheets/framework/filters.scss +++ b/app/assets/stylesheets/framework/filters.scss @@ -478,3 +478,7 @@ padding: 8px 16px; text-align: center; } + +.issues-details-filters { + @include new-style-dropdown; +} From e2caee527f8619c48535b4d7535c18044ee330f0 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Tue, 29 Aug 2017 13:58:38 +0100 Subject: [PATCH 43/70] Add Geo to the list of team labels --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 12fb34b24be..6cc34f1de08 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -164,7 +164,7 @@ Assigning a team label makes sure issues get the attention of the appropriate people. The current team labels are ~Build, ~CI, ~Discussion, ~Documentation, ~Edge, -~Gitaly, ~Platform, ~Prometheus, ~Release, and ~"UX". +~Geo, ~Gitaly, ~Platform, ~Prometheus, ~Release, and ~"UX". The descriptions on the [labels page][labels-page] explain what falls under the responsibility of each team. From efd5e533b6d45abb258501356496e1af0c112e7d Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Tue, 29 Aug 2017 13:20:58 +0100 Subject: [PATCH 44/70] Prevents rendering empty badge when pipeline request fails --- .../pipelines/components/navigation_tabs.vue | 54 +++++--- changelogs/unreleased/35048-empty-badges.yml | 5 + .../helpers/vue_mount_component_helper.js | 4 + .../pipelines/navigation_tabs_spec.js | 127 ++++++++++++++++++ 4 files changed, 170 insertions(+), 20 deletions(-) create mode 100644 changelogs/unreleased/35048-empty-badges.yml create mode 100644 spec/javascripts/helpers/vue_mount_component_helper.js create mode 100644 spec/javascripts/pipelines/navigation_tabs_spec.js diff --git a/app/assets/javascripts/pipelines/components/navigation_tabs.vue b/app/assets/javascripts/pipelines/components/navigation_tabs.vue index d2f6d47f043..73f7e3a0cad 100644 --- a/app/assets/javascripts/pipelines/components/navigation_tabs.vue +++ b/app/assets/javascripts/pipelines/components/navigation_tabs.vue @@ -1,23 +1,29 @@