Commit 50f8366a authored by Douwe Maan's avatar Douwe Maan

Merge branch 'complexity/rubocop-metrics' into 'master'

Enable rubocop metrics

This enables rubocop metrics like CyclomaticComplexity and ABCSize.
Initial threshold values are high, should be probably decreased.

See merge request !1802
parents eaecd397 cbeb06eb
......@@ -735,23 +735,39 @@ Metrics/AbcSize:
Description: >-
A calculated magnitude based on number of assignments,
branches, and conditions.
Enabled: false
Enabled: true
Max: 70
Metrics/CyclomaticComplexity:
Description: >-
A complexity metric that is strongly correlated to the number
of test cases needed to validate a method.
Enabled: true
Max: 17
Metrics/PerceivedComplexity:
Description: >-
A complexity metric geared towards measuring complexity for a
human reader.
Enabled: true
Max: 17
Metrics/ParameterLists:
Description: 'Avoid parameter lists longer than three or four parameters.'
StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#too-many-params'
Enabled: true
Max: 8
Metrics/BlockNesting:
Description: 'Avoid excessive block nesting'
StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#three-is-the-number-thou-shalt-count'
Enabled: false
Enabled: true
Max: 4
Metrics/ClassLength:
Description: 'Avoid classes longer than 100 lines of code.'
Enabled: false
Metrics/CyclomaticComplexity:
Description: >-
A complexity metric that is strongly correlated to the number
of test cases needed to validate a method.
Enabled: false
Metrics/LineLength:
Description: 'Limit lines to 80 characters.'
StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#80-character-limits'
......@@ -762,17 +778,6 @@ Metrics/MethodLength:
StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#short-methods'
Enabled: false
Metrics/ParameterLists:
Description: 'Avoid parameter lists longer than three or four parameters.'
StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#too-many-params'
Enabled: false
Metrics/PerceivedComplexity:
Description: >-
A complexity metric geared towards measuring complexity for a
human reader.
Enabled: false
#################### Lint ################################
### Warnings
......
......@@ -59,85 +59,17 @@ module Emails
subject: subject("Project was moved"))
end
def repository_push_email(project_id, recipient, author_id: nil,
ref: nil,
action: nil,
compare: nil,
reverse_compare: false,
send_from_committer_email: false,
disable_diffs: false)
unless author_id && ref && action
raise ArgumentError, "missing keywords: author_id, ref, action"
end
def repository_push_email(project_id, recipient, opts = {})
@message =
Gitlab::Email::Message::RepositoryPush.new(self, project_id, recipient, opts)
@project = Project.find(project_id)
@current_user = @author = User.find(author_id)
@reverse_compare = reverse_compare
@compare = compare
@ref_name = Gitlab::Git.ref_name(ref)
@ref_type = Gitlab::Git.tag_ref?(ref) ? "tag" : "branch"
@action = action
@disable_diffs = disable_diffs
if @compare
@commits = Commit.decorate(compare.commits, @project)
@diffs = compare.diffs
end
@action_name =
case action
when :create
"pushed new"
when :delete
"deleted"
else
"pushed to"
end
@subject = "[Git]"
@subject << "[#{@project.path_with_namespace}]"
@subject << "[#{@ref_name}]" if action == :push
@subject << " "
if action == :push
if @commits.length > 1
@target_url = namespace_project_compare_url(@project.namespace,
@project,
from: Commit.new(@compare.base, @project),
to: Commit.new(@compare.head, @project))
@subject << "Deleted " if @reverse_compare
@subject << "#{@commits.length} commits: #{@commits.first.title}"
else
@target_url = namespace_project_commit_url(@project.namespace,
@project, @commits.first)
@subject << "Deleted 1 commit: " if @reverse_compare
@subject << @commits.first.title
end
else
unless action == :delete
@target_url = namespace_project_tree_url(@project.namespace,
@project, @ref_name)
end
subject_action = @action_name.dup
subject_action[0] = subject_action[0].capitalize
@subject << "#{subject_action} #{@ref_type} #{@ref_name}"
end
@disable_footer = true
reply_to =
if send_from_committer_email && can_send_from_user_email?(@author)
@author.email
else
Gitlab.config.gitlab.email_reply_to
end
mail(from: sender(author_id, send_from_committer_email),
reply_to: reply_to,
to: recipient,
subject: @subject)
# used in notify layout
@target_url = @message.target_url
mail(from: sender(@message.author_id, @message.send_from_committer_email?),
reply_to: @message.reply_to,
to: @message.recipient,
subject: @message.subject)
end
end
end
......@@ -33,13 +33,13 @@ class Notify < BaseMailer
allowed_domains
end
private
def can_send_from_user_email?(sender)
sender_domain = sender.email.split("@").last
self.class.allowed_email_domains.include?(sender_domain)
end
private
# Return an email address that displays the name of the sender.
# Only the displayed name changes; the actual email address is always the same.
def sender(sender_id, send_from_user_email = false)
......
%h3 #{@author.name} #{@action_name} #{@ref_type} #{@ref_name} at #{link_to @project.name_with_namespace, namespace_project_url(@project.namespace, @project)}
%h3
#{@message.author_name} #{@message.action_name} #{@message.ref_type} #{@message.ref_name}
at #{link_to(@message.project_name_with_namespace, namespace_project_url(@message.project_namespace, @message.project))}
- if @compare
- if @reverse_compare
- if @message.compare
- if @message.reverse_compare?
%p
%strong WARNING:
The push did not contain any new commits, but force pushed to delete the commits and changes below.
%h4
= @reverse_compare ? "Deleted commits:" : "Commits:"
= @message.reverse_compare? ? "Deleted commits:" : "Commits:"
%ul
- @commits.each do |commit|
- @message.commits.each do |commit|
%li
%strong #{link_to commit.short_id, namespace_project_commit_url(@project.namespace, @project, commit)}
%strong #{link_to(commit.short_id, namespace_project_commit_url(@message.project_namespace, @message.project, commit))}
%div
%span by #{commit.author_name}
%i at #{commit.committed_date.strftime("%Y-%m-%dT%H:%M:%SZ")}
%pre.commit-message
= commit.safe_message
%h4 #{pluralize @diffs.count, "changed file"}:
%h4 #{pluralize @message.diffs_count, "changed file"}:
%ul
- @diffs.each_with_index do |diff, i|
- @message.diffs.each_with_index do |diff, i|
%li.file-stats
%a{href: "#{@target_url if @disable_diffs}#diff-#{i}" }
%a{href: "#{@message.target_url if @message.disable_diffs?}#diff-#{i}" }
- if diff.deleted_file
%span.deleted-file
&minus;
......@@ -40,11 +42,11 @@
- else
= diff.new_path
- unless @disable_diffs
- unless @message.disable_diffs?
%h4 Changes:
- @diffs.each_with_index do |diff, i|
- @message.diffs.each_with_index do |diff, i|
%li{id: "diff-#{i}"}
%a{href: @target_url + "#diff-#{i}"}
%a{href: @message.target_url + "#diff-#{i}"}
- if diff.deleted_file
%strong
= diff.old_path
......@@ -62,5 +64,5 @@
= color_email_diff(diff.diff)
%br
- if @compare.timeout
- if @message.compare_timeout
%h5 Huge diff. To prevent performance issues changes are hidden
#{@author.name} #{@action_name} #{@ref_type} #{@ref_name} at #{@project.name_with_namespace}
- if @compare
#{@message.author_name} #{@message.action_name} #{@message.ref_type} #{@message.ref_name} at #{@message.project_name_with_namespace}
- if @message.compare
\
\
- if @reverse_compare
- if @message.reverse_compare?
WARNING: The push did not contain any new commits, but force pushed to delete the commits and changes below.
\
\
= @reverse_compare ? "Deleted commits:" : "Commits:"
- @commits.each do |commit|
= @message.reverse_compare? ? "Deleted commits:" : "Commits:"
- @message.commits.each do |commit|
#{commit.short_id} by #{commit.author_name} at #{commit.committed_date.strftime("%Y-%m-%dT%H:%M:%SZ")}
#{commit.safe_message}
\- - - - -
\
\
#{pluralize @diffs.count, "changed file"}:
#{pluralize @message.diffs_count, "changed file"}:
\
- @diffs.each do |diff|
- @message.diffs.each do |diff|
- if diff.deleted_file
\- − #{diff.old_path}
- elsif diff.renamed_file
......@@ -24,11 +24,11 @@
\- + #{diff.new_path}
- else
\- #{diff.new_path}
- unless @disable_diffs
- unless @message.disable_diffs?
\
\
Changes:
- @diffs.each do |diff|
- @message.diffs.each do |diff|
\
\=====================================
- if diff.deleted_file
......@@ -39,11 +39,11 @@
= diff.new_path
\=====================================
!= diff.diff
- if @compare.timeout
- if @message.compare_timeout
\
\
Huge diff. To prevent performance issues it was hidden
- if @target_url
- if @message.target_url
\
\
View it on GitLab: #{@target_url}
View it on GitLab: #{@message.target_url}
......@@ -132,26 +132,36 @@ module Ci
end
def validate_job!(name, job)
validate_job_name!(name)
validate_job_keys!(name, job)
validate_job_types!(name, job)
validate_job_stage!(name, job) if job[:stage]
validate_job_cache!(name, job) if job[:cache]
validate_job_artifacts!(name, job) if job[:artifacts]
end
private
def validate_job_name!(name)
if name.blank? || !validate_string(name)
raise ValidationError, "job name should be non-empty string"
end
end
def validate_job_keys!(name, job)
job.keys.each do |key|
unless ALLOWED_JOB_KEYS.include? key
raise ValidationError, "#{name} job: unknown parameter #{key}"
end
end
end
def validate_job_types!(name, job)
if !validate_string(job[:script]) && !validate_array_of_strings(job[:script])
raise ValidationError, "#{name} job: script should be a string or an array of a strings"
end
if job[:stage]
unless job[:stage].is_a?(String) && job[:stage].in?(stages)
raise ValidationError, "#{name} job: stage parameter should be #{stages.join(", ")}"
end
end
if job[:image] && !validate_string(job[:image])
raise ValidationError, "#{name} job: image should be a string"
end
......@@ -172,36 +182,40 @@ module Ci
raise ValidationError, "#{name} job: except parameter should be an array of strings"
end
if job[:cache]
if job[:cache][:untracked] && !validate_boolean(job[:cache][:untracked])
raise ValidationError, "#{name} job: cache:untracked parameter should be an boolean"
end
if job[:cache][:paths] && !validate_array_of_strings(job[:cache][:paths])
raise ValidationError, "#{name} job: cache:paths parameter should be an array of strings"
end
if job[:allow_failure] && !validate_boolean(job[:allow_failure])
raise ValidationError, "#{name} job: allow_failure parameter should be an boolean"
end
if job[:artifacts]
if job[:artifacts][:untracked] && !validate_boolean(job[:artifacts][:untracked])
raise ValidationError, "#{name} job: artifacts:untracked parameter should be an boolean"
end
if job[:when] && !job[:when].in?(%w(on_success on_failure always))
raise ValidationError, "#{name} job: when parameter should be on_success, on_failure or always"
end
end
if job[:artifacts][:paths] && !validate_array_of_strings(job[:artifacts][:paths])
raise ValidationError, "#{name} job: artifacts:paths parameter should be an array of strings"
end
def validate_job_stage!(name, job)
unless job[:stage].is_a?(String) && job[:stage].in?(stages)
raise ValidationError, "#{name} job: stage parameter should be #{stages.join(", ")}"
end
end
if job[:allow_failure] && !validate_boolean(job[:allow_failure])
raise ValidationError, "#{name} job: allow_failure parameter should be an boolean"
def validate_job_cache!(name, job)
if job[:cache][:untracked] && !validate_boolean(job[:cache][:untracked])
raise ValidationError, "#{name} job: cache:untracked parameter should be an boolean"
end
if job[:when] && !job[:when].in?(%w(on_success on_failure always))
raise ValidationError, "#{name} job: when parameter should be on_success, on_failure or always"
if job[:cache][:paths] && !validate_array_of_strings(job[:cache][:paths])
raise ValidationError, "#{name} job: cache:paths parameter should be an array of strings"
end
end
private
def validate_job_artifacts!(name, job)
if job[:artifacts][:untracked] && !validate_boolean(job[:artifacts][:untracked])
raise ValidationError, "#{name} job: artifacts:untracked parameter should be an boolean"
end
if job[:artifacts][:paths] && !validate_array_of_strings(job[:artifacts][:paths])
raise ValidationError, "#{name} job: artifacts:paths parameter should be an array of strings"
end
end
def validate_array_of_strings(values)
values.is_a?(Array) && values.all? { |value| validate_string(value) }
......
module Gitlab
module Email
module Message
class RepositoryPush
attr_accessor :recipient
attr_reader :author_id, :ref, :action
include Gitlab::Application.routes.url_helpers
delegate :namespace, :name_with_namespace, to: :project, prefix: :project
delegate :name, to: :author, prefix: :author
def initialize(notify, project_id, recipient, opts = {})
raise ArgumentError, 'Missing options: author_id, ref, action' unless
opts[:author_id] && opts[:ref] && opts[:action]
@notify = notify
@project_id = project_id
@recipient = recipient
@opts = opts.dup
@author_id = @opts.delete(:author_id)
@ref = @opts.delete(:ref)
@action = @opts.delete(:action)
end
def project
@project ||= Project.find(@project_id)
end
def author
@author ||= User.find(@author_id)
end
def commits
@commits ||= (Commit.decorate(compare.commits, project) if compare)
end
def diffs
@diffs ||= (compare.diffs if compare)
end
def diffs_count
diffs.count if diffs
end
def compare
@opts[:compare]
end
def compare_timeout
compare.timeout if compare
end
def reverse_compare?
@opts[:reverse_compare] || false
end
def disable_diffs?
@opts[:disable_diffs] || false
end
def send_from_committer_email?
@opts[:send_from_committer_email] || false
end
def action_name
@action_name ||=
case @action
when :create
'pushed new'
when :delete
'deleted'
else
'pushed to'
end
end
def ref_name
@ref_name ||= Gitlab::Git.ref_name(@ref)
end
def ref_type
@ref_type ||= Gitlab::Git.tag_ref?(@ref) ? 'tag' : 'branch'
end
def target_url
if @action == :push && commits
if commits.length > 1
namespace_project_compare_url(project_namespace,
project,
from: Commit.new(compare.base, project),
to: Commit.new(compare.head, project))
else
namespace_project_commit_url(project_namespace,
project, commits.first)
end
else
unless @action == :delete
namespace_project_tree_url(project_namespace,
project, ref_name)
end
end
end
def reply_to
if send_from_committer_email? && @notify.can_send_from_user_email?(author)
author.email
else
Gitlab.config.gitlab.email_reply_to
end
end
def subject
subject_text = '[Git]'
subject_text << "[#{project.path_with_namespace}]"
subject_text << "[#{ref_name}]" if @action == :push
subject_text << ' '
if @action == :push && commits
if commits.length > 1
subject_text << "Deleted " if reverse_compare?
subject_text << "#{commits.length} commits: #{commits.first.title}"
else
subject_text << "Deleted 1 commit: " if reverse_compare?
subject_text << commits.first.title
end
else
subject_action = action_name.dup
subject_action[0] = subject_action[0].capitalize
subject_text << "#{subject_action} #{ref_type} #{ref_name}"
end
end
end
end
end
end
......@@ -532,21 +532,21 @@ module Ci
end
it "returns errors if job stage is not a string" do
config = YAML.dump({ rspec: { script: "test", type: 1, allow_failure: "string" } })
config = YAML.dump({ rspec: { script: "test", type: 1 } })
expect do
GitlabCiYamlProcessor.new(config, path)
end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: stage parameter should be build, test, deploy")
end
it "returns errors if job stage is not a pre-defined stage" do
config = YAML.dump({ rspec: { script: "test", type: "acceptance", allow_failure: "string" } })
config = YAML.dump({ rspec: { script: "test", type: "acceptance" } })
expect do
GitlabCiYamlProcessor.new(config, path)
end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: stage parameter should be build, test, deploy")
end
it "returns errors if job stage is not a defined stage" do
config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", type: "acceptance", allow_failure: "string" } })
config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", type: "acceptance" } })
expect do
GitlabCiYamlProcessor.new(config, path)
end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: stage parameter should be build, test")
......
require 'spec_helper'
describe Gitlab::Email::Message::RepositoryPush do
include RepoHelpers
let!(:group) { create(:group, name: 'my_group') }
let!(:project) { create(:project, name: 'my_project', namespace: group) }
let!(:author) { create(:author, name: 'Author') }
let(:message) do
described_class.new(Notify, project.id, 'recipient@example.com', opts)
end
context 'new commits have been pushed to repository' do
let(:opts) do
{ author_id: author.id, ref: 'master', action: :push, compare: compare,
send_from_committer_email: true }
end
let(:compare) do
Gitlab::Git::Compare.new(project.repository.raw_repository,
sample_image_commit.id, sample_commit.id)
end
describe '#project' do
subject { message.project }
it { is_expected.to eq project }
it { is_expected.to be_an_instance_of Project }
end
describe '#project_namespace' do
subject { message.project_namespace }
it { is_expected.to eq group }
it { is_expected.to be_kind_of Namespace }
end
describe '#project_name_with_namespace' do
subject { message.project_name_with_namespace }
it { is_expected.to eq 'my_group / my_project' }
end
describe '#author' do
subject { message.author }
it { is_expected.to eq author }
it { is_expected.to be_an_instance_of User }
end
describe '#author_name' do
subject { message.author_name }
it { is_expected.to eq 'Author' }
end
describe '#commits' do
subject { message.commits }
it { is_expected.to be_kind_of Array }
it { is_expected.to all(be_instance_of Commit) }
end
describe '#diffs' do
subject { message.diffs }
it { is_expected.to all(be_an_instance_of Gitlab::Git::Diff) }
end
describe '#diffs_count' do
subject { message.diffs_count }
it { is_expected.to eq compare.diffs.count }
end
describe '#compare' do
subject { message.compare }
it { is_expected.to be_an_instance_of Gitlab::Git::Compare }
end
describe '#compare_timeout' do
subject { message.compare_timeout }
it { is_expected.to eq compare.timeout }
end
describe '#reverse_compare?' do
subject { message.reverse_compare? }
it { is_expected.to eq false }
end
describe '#disable_diffs?' do
subject { message.disable_diffs? }
it { is_expected.to eq false }
end
describe '#send_from_committer_email?' do
subject { message.send_from_committer_email? }
it { is_expected.to eq true }
end
describe '#action_name' do
subject { message.action_name }
it { is_expected.to eq 'pushed to' }
end
describe '#ref_name' do
subject { message.ref_name }
it { is_expected.to eq 'master' }
end
describe '#ref_type' do
subject { message.ref_type }
it { is_expected.to eq 'branch' }
end
describe '#target_url' do
subject { message.target_url }
it { is_expected.to include 'compare' }
it { is_expected.to include compare.commits.first.parents.first.id }
it { is_expected.to include compare.commits.last.id }
end
describe '#subject' do
subject { message.subject }
it { is_expected.to include "[Git][#{project.path_with_namespace}]" }
it { is_expected.to include "#{compare.commits.length} commits" }
it { is_expected.to include compare.commits.first.message.split("\n").first }
end
end
end
......@@ -4,7 +4,7 @@
# - let(:backref_text) { "the way that +subject+ should refer to itself in backreferences " }
# - let(:set_mentionable_text) { lambda { |txt| "block that assigns txt to the subject's mentionable_text" } }
def common_mentionable_setup
shared_context 'mentionable context' do
let(:project) { subject.project }
let(:author) { subject.author }
......@@ -56,7 +56,7 @@ def common_mentionable_setup
end
shared_examples 'a mentionable' do
common_mentionable_setup
include_context 'mentionable context'
it 'generates a descriptive back-reference' do
expect(subject.gfm_reference).to eq(backref_text)
......@@ -88,7 +88,7 @@ shared_examples 'a mentionable' do
end
shared_examples 'an editable mentionable' do
common_mentionable_setup
include_context 'mentionable context'
it_behaves_like 'a mentionable'
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment