From db3bc89792c5450cb93e674d621d5ea996ee01f6 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Fri, 1 Sep 2017 00:43:13 +0300 Subject: [PATCH 1/8] refactor `app/helpers/projects_helper.rb:21:3` --- .rubocop.yml | 2 +- app/helpers/projects_helper.rb | 4 ++-- spec/helpers/projects_helper_spec.rb | 10 +++++++++- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 16f2e4484fc..4640681379a 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -643,7 +643,7 @@ Metrics/ClassLength: # of test cases needed to validate a method. Metrics/CyclomaticComplexity: Enabled: true - Max: 15 + Max: 14 # Limit lines to 80 characters. Metrics/LineLength: diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 86665ea2aec..7e5b214b1c7 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -17,7 +17,7 @@ module ProjectsHelper def link_to_member_avatar(author, opts = {}) default_opts = { avatar: true, name: true, size: 16, author_class: 'author', title: ":name" } opts = default_opts.merge(opts) - image_tag(avatar_icon(author, opts[:size]), width: opts[:size], class: "avatar avatar-inline #{"s#{opts[:size]}" if opts[:size]}", alt: '') if opts[:avatar] + image_tag(avatar_icon(author, opts[:size]), width: opts[:size], class: ['avatar', 'avatar-inline', "#{"s#{opts[:size]}" if opts[:size]}", opts[:avatar_class]].reject(&:blank?), alt: '') if opts[:avatar] end def link_to_member(project, author, opts = {}, &block) @@ -29,7 +29,7 @@ module ProjectsHelper author_html = "" # Build avatar image tag - author_html << image_tag(avatar_icon(author, opts[:size]), width: opts[:size], class: "avatar avatar-inline #{"s#{opts[:size]}" if opts[:size]} #{opts[:avatar_class] if opts[:avatar_class]}", alt: '') if opts[:avatar] + author_html << link_to_member_avatar(author, opts) if opts[:avatar] # Build name span tag if opts[:by_username] diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index d1efa318d14..93a5b55e4d0 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -191,7 +191,15 @@ describe ProjectsHelper do end end - describe 'link_to_member' do + describe '#link_to_member_avatar' do + let(:user) { create(:user) } + + it 'returns image tag for member avatar' do + expect(helper.link_to_member_avatar(user)).to eq("\"\"") + end + end + + describe '#link_to_member' do let(:group) { create(:group) } let(:project) { create(:project, group: group) } let(:user) { create(:user) } From c9d76b528c837f63f1052eb3f096bea4c8308e06 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Fri, 1 Sep 2017 00:52:42 +0300 Subject: [PATCH 2/8] adds changelog --- .../31362_decrease_cyclomatic_complexity_threshold_step3.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/31362_decrease_cyclomatic_complexity_threshold_step3.yml diff --git a/changelogs/unreleased/31362_decrease_cyclomatic_complexity_threshold_step3.yml b/changelogs/unreleased/31362_decrease_cyclomatic_complexity_threshold_step3.yml new file mode 100644 index 00000000000..4a8d8097169 --- /dev/null +++ b/changelogs/unreleased/31362_decrease_cyclomatic_complexity_threshold_step3.yml @@ -0,0 +1,5 @@ +--- +title: Decrease Cyclomatic Complexity threshold to 14 +merge_request: 13972 +author: Maxim Rydkin +type: other From 0963ac363641c5552f6668705edcded4bad0259c Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Tue, 5 Sep 2017 23:17:53 +0300 Subject: [PATCH 3/8] fix helper and spec --- app/helpers/projects_helper.rb | 6 +++++- spec/helpers/projects_helper_spec.rb | 13 ++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 7e5b214b1c7..c51b27ae24f 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -17,7 +17,11 @@ module ProjectsHelper def link_to_member_avatar(author, opts = {}) default_opts = { avatar: true, name: true, size: 16, author_class: 'author', title: ":name" } opts = default_opts.merge(opts) - image_tag(avatar_icon(author, opts[:size]), width: opts[:size], class: ['avatar', 'avatar-inline', "#{"s#{opts[:size]}" if opts[:size]}", opts[:avatar_class]].reject(&:blank?), alt: '') if opts[:avatar] + classes = %w[avatar avatar-inline] + classes << "s#{opts[:size]}" + classes << opts[:avatar_class] if opts[:avatar_class] + + image_tag(avatar_icon(author, opts[:size]), width: opts[:size], class: classes, alt: '') if opts[:avatar] end def link_to_member(project, author, opts = {}, &block) diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 93a5b55e4d0..9a6fb4d6cb0 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -192,17 +192,20 @@ describe ProjectsHelper do end describe '#link_to_member_avatar' do - let(:user) { create(:user) } + let(:user) { build_stubbed(:user) } it 'returns image tag for member avatar' do - expect(helper.link_to_member_avatar(user)).to eq("\"\"") + allow(helper).to receive(:image_tag).with(nil, {width: 16, class: ["avatar", "avatar-inline", "s16"], alt: ""}) + allow(helper).to receive(:avatar_icon).with(user, 16) + + helper.link_to_member_avatar(user) end end describe '#link_to_member' do - let(:group) { create(:group) } - let(:project) { create(:project, group: group) } - let(:user) { create(:user) } + let(:group) { build_stubbed(:group) } + let(:project) { build_stubbed(:project, group: group) } + let(:user) { build_stubbed(:user) } describe 'using the default options' do it 'returns an HTML link to the user' do From 42e7e75095e7b96d495433106078aa9a7b949daa Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Tue, 5 Sep 2017 23:57:42 +0300 Subject: [PATCH 4/8] refactor app/services/ci/create_pipeline_service.rb:50:5 --- app/services/ci/create_pipeline_service.rb | 26 ++++++++++++---------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 414c01b2546..6f1f3c4d2e8 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -16,9 +16,9 @@ module Ci protected: project.protected_for?(ref) ) - result = validate(current_user, - ignore_skip_ci: ignore_skip_ci, - save_on_errors: save_on_errors) + result = validate_project_and_git_items || + validate_pipeline(ignore_skip_ci: ignore_skip_ci, + save_on_errors: save_on_errors) return result if result @@ -47,13 +47,13 @@ module Ci private - def validate(triggering_user, ignore_skip_ci:, save_on_errors:) + def validate_project_and_git_items unless project.builds_enabled? return error('Pipeline is disabled') end - unless allowed_to_trigger_pipeline?(triggering_user) - if can?(triggering_user, :create_pipeline, project) + unless allowed_to_trigger_pipeline? + if can?(current_user, :create_pipeline, project) return error("Insufficient permissions for protected ref '#{ref}'") else return error('Insufficient permissions to create a new pipeline') @@ -67,7 +67,9 @@ module Ci unless commit return error('Commit not found') end + end + def validate_pipeline(ignore_skip_ci:, save_on_errors:) unless pipeline.config_processor unless pipeline.ci_yaml_file return error("Missing #{pipeline.ci_yaml_file_path} file") @@ -85,18 +87,18 @@ module Ci end end - def allowed_to_trigger_pipeline?(triggering_user) - if triggering_user - allowed_to_create?(triggering_user) + def allowed_to_trigger_pipeline? + if current_user + allowed_to_create? else # legacy triggers don't have a corresponding user !project.protected_for?(ref) end end - def allowed_to_create?(triggering_user) - access = Gitlab::UserAccess.new(triggering_user, project: project) + def allowed_to_create? + access = Gitlab::UserAccess.new(current_user, project: project) - can?(triggering_user, :create_pipeline, project) && + can?(current_user, :create_pipeline, project) && if branch? access.can_update_branch?(ref) elsif tag? From 0a08e07303246c2129c8dfdd90101fe07f230b7c Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Wed, 6 Sep 2017 10:41:39 +0300 Subject: [PATCH 5/8] fix CI --- spec/helpers/projects_helper_spec.rb | 2 +- spec/services/ci/create_pipeline_service_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 9a6fb4d6cb0..08eec9b2f84 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -195,7 +195,7 @@ describe ProjectsHelper do let(:user) { build_stubbed(:user) } it 'returns image tag for member avatar' do - allow(helper).to receive(:image_tag).with(nil, {width: 16, class: ["avatar", "avatar-inline", "s16"], alt: ""}) + allow(helper).to receive(:image_tag).with(nil, { width: 16, class: ["avatar", "avatar-inline", "s16"], alt: "" }) allow(helper).to receive(:avatar_icon).with(user, 16) helper.link_to_member_avatar(user) diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 49d7c663128..009d67a3fbe 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -489,7 +489,7 @@ describe Ci::CreatePipelineService do subject do described_class.new(project, user, ref: ref) - .send(:allowed_to_create?, user) + .send(:allowed_to_create?) end context 'when user is a developer' do From 2a54cbcab83612991c0c61ffed1b2472e4e49047 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Wed, 6 Sep 2017 19:26:01 +0300 Subject: [PATCH 6/8] fix project_helper.rb and add couple specs to it --- app/helpers/projects_helper.rb | 8 +++++--- spec/helpers/projects_helper_spec.rb | 16 ++++++++++++++-- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index c51b27ae24f..79ff57d5a94 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -17,11 +17,13 @@ module ProjectsHelper def link_to_member_avatar(author, opts = {}) default_opts = { avatar: true, name: true, size: 16, author_class: 'author', title: ":name" } opts = default_opts.merge(opts) - classes = %w[avatar avatar-inline] - classes << "s#{opts[:size]}" + + return unless opts[:avatar] + + classes = %W[avatar avatar-inline s#{opts[:size]}] classes << opts[:avatar_class] if opts[:avatar_class] - image_tag(avatar_icon(author, opts[:size]), width: opts[:size], class: classes, alt: '') if opts[:avatar] + image_tag(avatar_icon(author, opts[:size]), width: opts[:size], class: classes, alt: '') end def link_to_member(project, author, opts = {}, &block) diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 08eec9b2f84..73ade811165 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -193,13 +193,25 @@ describe ProjectsHelper do describe '#link_to_member_avatar' do let(:user) { build_stubbed(:user) } + let(:expected) { double } it 'returns image tag for member avatar' do - allow(helper).to receive(:image_tag).with(nil, { width: 16, class: ["avatar", "avatar-inline", "s16"], alt: "" }) - allow(helper).to receive(:avatar_icon).with(user, 16) + expect(helper).to receive(:avatar_icon).with(user, 16).and_return(expected) + expect(helper).to receive(:image_tag).with(expected, { width: 16, class: ["avatar", "avatar-inline", "s16"], alt: "" }) helper.link_to_member_avatar(user) end + + it 'returns image tag with avatar class' do + expect(helper).to receive(:avatar_icon).with(user, 16).and_return(expected) + expect(helper).to receive(:image_tag).with(expected, { width: 16, class: ["avatar", "avatar-inline", "s16", "any-avatar-class"], alt: "" }) + + helper.link_to_member_avatar(user, avatar_class: "any-avatar-class") + end + + it 'returns no image tag if avatar is nil' do + expect(helper.link_to_member_avatar(user, avatar: nil)).to eq(nil) + end end describe '#link_to_member' do From 9151ca7bb37b7c0f36b0a1c6ba4ba1c37fd3660b Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Wed, 6 Sep 2017 19:35:28 +0300 Subject: [PATCH 7/8] refactor Ci::CreatePipelineService#allowed_to_create? --- app/services/ci/create_pipeline_service.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 6f1f3c4d2e8..d20de9b16a4 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -96,16 +96,16 @@ module Ci end def allowed_to_create? - access = Gitlab::UserAccess.new(current_user, project: project) + return unless can?(current_user, :create_pipeline, project) - can?(current_user, :create_pipeline, project) && - if branch? - access.can_update_branch?(ref) - elsif tag? - access.can_create_tag?(ref) - else - true # Allow it for now and we'll reject when we check ref existence - end + access = Gitlab::UserAccess.new(current_user, project: project) + if branch? + access.can_update_branch?(ref) + elsif tag? + access.can_create_tag?(ref) + else + true # Allow it for now and we'll reject when we check ref existence + end end def update_merge_requests_head_pipeline From 091b1c5ed59b5c505d29c8905c78a867b12d5641 Mon Sep 17 00:00:00 2001 From: Maxim Rydkin Date: Thu, 7 Sep 2017 13:27:23 +0300 Subject: [PATCH 8/8] remove unnecessary args from `link_to_member_avatar` method in `app/helpers/projects_helper.rb` --- app/helpers/projects_helper.rb | 4 +--- spec/helpers/projects_helper_spec.rb | 10 ++++------ 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 79ff57d5a94..51c625ede4b 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -15,11 +15,9 @@ module ProjectsHelper end def link_to_member_avatar(author, opts = {}) - default_opts = { avatar: true, name: true, size: 16, author_class: 'author', title: ":name" } + default_opts = { size: 16 } opts = default_opts.merge(opts) - return unless opts[:avatar] - classes = %W[avatar avatar-inline s#{opts[:size]}] classes << opts[:avatar_class] if opts[:avatar_class] diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 73ade811165..49cb7c954b4 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -195,23 +195,21 @@ describe ProjectsHelper do let(:user) { build_stubbed(:user) } let(:expected) { double } - it 'returns image tag for member avatar' do + before do expect(helper).to receive(:avatar_icon).with(user, 16).and_return(expected) + end + + it 'returns image tag for member avatar' do expect(helper).to receive(:image_tag).with(expected, { width: 16, class: ["avatar", "avatar-inline", "s16"], alt: "" }) helper.link_to_member_avatar(user) end it 'returns image tag with avatar class' do - expect(helper).to receive(:avatar_icon).with(user, 16).and_return(expected) expect(helper).to receive(:image_tag).with(expected, { width: 16, class: ["avatar", "avatar-inline", "s16", "any-avatar-class"], alt: "" }) helper.link_to_member_avatar(user, avatar_class: "any-avatar-class") end - - it 'returns no image tag if avatar is nil' do - expect(helper.link_to_member_avatar(user, avatar: nil)).to eq(nil) - end end describe '#link_to_member' do