Commit de073cf5 authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

Merge branch 'services_refactoring1' into 'master'

Services: code style fixes, minor refactoring

## What does this MR do?

It contains code style fixes, minor refactoring and also it removes some unnecessary comments. Every comment require a support just like a regular code line and this is why we should avoid using comments everywhere unless it's really helpful. Martin Fowler said "Find comment and refactor the code around it" :) our code is not that bad so let's not spoil it with comments.

## Why was this MR needed?

Because GitLab is a live example of awesome code. Let's keep up a good job :) 


See merge request !5112
parents 52a583c1 3baed8cb
...@@ -7,7 +7,7 @@ class AuditEventService ...@@ -7,7 +7,7 @@ class AuditEventService
@details = { @details = {
with: @details[:with], with: @details[:with],
target_id: @author.id, target_id: @author.id,
target_type: "User", target_type: 'User',
target_details: @author.name, target_details: @author.name,
} }
......
...@@ -20,9 +20,11 @@ module Auth ...@@ -20,9 +20,11 @@ module Auth
token.issuer = registry.issuer token.issuer = registry.issuer
token.audience = AUDIENCE token.audience = AUDIENCE
token.expire_time = token_expire_at token.expire_time = token_expire_at
token[:access] = names.map do |name| token[:access] = names.map do |name|
{ type: 'repository', name: name, actions: %w(*) } { type: 'repository', name: name, actions: %w(*) }
end end
token.encoded token.encoded
end end
......
...@@ -3,17 +3,20 @@ require_relative 'base_service' ...@@ -3,17 +3,20 @@ require_relative 'base_service'
class CreateBranchService < BaseService class CreateBranchService < BaseService
def execute(branch_name, ref, source_project: @project) def execute(branch_name, ref, source_project: @project)
valid_branch = Gitlab::GitRefValidator.validate(branch_name) valid_branch = Gitlab::GitRefValidator.validate(branch_name)
if valid_branch == false
unless valid_branch
return error('Branch name is invalid') return error('Branch name is invalid')
end end
repository = project.repository repository = project.repository
existing_branch = repository.find_branch(branch_name) existing_branch = repository.find_branch(branch_name)
if existing_branch if existing_branch
return error('Branch already exists') return error('Branch already exists')
end end
new_branch = nil new_branch = nil
if source_project != @project if source_project != @project
repository.with_tmp_ref do |tmp_ref| repository.with_tmp_ref do |tmp_ref|
repository.fetch_ref( repository.fetch_ref(
...@@ -29,7 +32,6 @@ class CreateBranchService < BaseService ...@@ -29,7 +32,6 @@ class CreateBranchService < BaseService
end end
if new_branch if new_branch
# GitPushService handles execution of services and hooks for branch pushes
success(new_branch) success(new_branch)
else else
error('Invalid reference name') error('Invalid reference name')
...@@ -39,8 +41,6 @@ class CreateBranchService < BaseService ...@@ -39,8 +41,6 @@ class CreateBranchService < BaseService
end end
def success(branch) def success(branch)
out = super() super().merge(branch: branch)
out[:branch] = branch
out
end end
end end
...@@ -23,8 +23,6 @@ class CreateReleaseService < BaseService ...@@ -23,8 +23,6 @@ class CreateReleaseService < BaseService
end end
def success(release) def success(release)
out = super() super().merge(release: release)
out[:release] = release
out
end end
end end
class CreateSnippetService < BaseService class CreateSnippetService < BaseService
def execute def execute
if project.nil? snippet = if project
snippet = PersonalSnippet.new(params) project.snippets.build(params)
else else
snippet = project.snippets.build(params) PersonalSnippet.new(params)
end end
unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level]) unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
......
...@@ -9,6 +9,7 @@ class CreateTagService < BaseService ...@@ -9,6 +9,7 @@ class CreateTagService < BaseService
message.strip! if message message.strip! if message
new_tag = nil new_tag = nil
begin begin
new_tag = repository.add_tag(current_user, tag_name, target, message) new_tag = repository.add_tag(current_user, tag_name, target, message)
rescue Rugged::TagError rescue Rugged::TagError
...@@ -22,6 +23,7 @@ class CreateTagService < BaseService ...@@ -22,6 +23,7 @@ class CreateTagService < BaseService
CreateReleaseService.new(@project, @current_user). CreateReleaseService.new(@project, @current_user).
execute(tag_name, release_description) execute(tag_name, release_description)
end end
success.merge(tag: new_tag) success.merge(tag: new_tag)
else else
error("Target #{target} is invalid") error("Target #{target} is invalid")
......
...@@ -5,7 +5,6 @@ class DeleteBranchService < BaseService ...@@ -5,7 +5,6 @@ class DeleteBranchService < BaseService
repository = project.repository repository = project.repository
branch = repository.find_branch(branch_name) branch = repository.find_branch(branch_name)
# No such branch
unless branch unless branch
return error('No such branch', 404) return error('No such branch', 404)
end end
...@@ -14,18 +13,15 @@ class DeleteBranchService < BaseService ...@@ -14,18 +13,15 @@ class DeleteBranchService < BaseService
return error('Cannot remove HEAD branch', 405) return error('Cannot remove HEAD branch', 405)
end end
# Dont allow remove of protected branch
if project.protected_branch?(branch_name) if project.protected_branch?(branch_name)
return error('Protected branch cant be removed', 405) return error('Protected branch cant be removed', 405)
end end
# Dont allow user to remove branch if he is not allowed to push
unless current_user.can?(:push_code, project) unless current_user.can?(:push_code, project)
return error('You dont have push access to repo', 405) return error('You dont have push access to repo', 405)
end end
if repository.rm_branch(current_user, branch_name) if repository.rm_branch(current_user, branch_name)
# GitPushService handles execution of services and hooks for branch pushes
success('Branch was removed') success('Branch was removed')
else else
error('Failed to remove branch') error('Failed to remove branch')
...@@ -35,15 +31,11 @@ class DeleteBranchService < BaseService ...@@ -35,15 +31,11 @@ class DeleteBranchService < BaseService
end end
def error(message, return_code = 400) def error(message, return_code = 400)
out = super(message) super(message).merge(return_code: return_code)
out[:return_code] = return_code
out
end end
def success(message) def success(message)
out = super() super().merge(message: message)
out[:message] = message
out
end end
def build_push_data(branch) def build_push_data(branch)
......
...@@ -5,7 +5,6 @@ class DeleteTagService < BaseService ...@@ -5,7 +5,6 @@ class DeleteTagService < BaseService
repository = project.repository repository = project.repository
tag = repository.find_tag(tag_name) tag = repository.find_tag(tag_name)
# No such tag
unless tag unless tag
return error('No such tag', 404) return error('No such tag', 404)
end end
...@@ -26,15 +25,11 @@ class DeleteTagService < BaseService ...@@ -26,15 +25,11 @@ class DeleteTagService < BaseService
end end
def error(message, return_code = 400) def error(message, return_code = 400)
out = super(message) super(message).merge(return_code: return_code)
out[:return_code] = return_code
out
end end
def success(message) def success(message)
out = super() super().merge(message: message)
out[:message] = message
out
end end
def build_push_data(tag) def build_push_data(tag)
......
...@@ -15,7 +15,6 @@ module Files ...@@ -15,7 +15,6 @@ module Files
params[:file_content] params[:file_content]
end end
# Validate parameters
validate validate
# Create new branch if it different from source_branch # Create new branch if it different from source_branch
...@@ -26,7 +25,7 @@ module Files ...@@ -26,7 +25,7 @@ module Files
if commit if commit
success success
else else
error("Something went wrong. Your changes were not committed") error('Something went wrong. Your changes were not committed')
end end
rescue Repository::CommitError, Gitlab::Git::Repository::InvalidBlobName, GitHooksService::PreReceiveError, ValidationError => ex rescue Repository::CommitError, Gitlab::Git::Repository::InvalidBlobName, GitHooksService::PreReceiveError, ValidationError => ex
error(ex.message) error(ex.message)
...@@ -51,12 +50,12 @@ module Files ...@@ -51,12 +50,12 @@ module Files
unless project.empty_repo? unless project.empty_repo?
unless @source_project.repository.branch_names.include?(@source_branch) unless @source_project.repository.branch_names.include?(@source_branch)
raise_error("You can only create or edit files when you are on a branch") raise_error('You can only create or edit files when you are on a branch')
end end
if different_branch? if different_branch?
if repository.branch_names.include?(@target_branch) if repository.branch_names.include?(@target_branch)
raise_error("Branch with such name already exists. You need to switch to this branch in order to make changes") raise_error('Branch with such name already exists. You need to switch to this branch in order to make changes')
end end
end end
end end
......
...@@ -29,7 +29,7 @@ module Files ...@@ -29,7 +29,7 @@ module Files
blob = repository.blob_at_branch(@source_branch, @file_path) blob = repository.blob_at_branch(@source_branch, @file_path)
if blob if blob
raise_error("Your changes could not be committed because a file with the same name already exists") raise_error('Your changes could not be committed because a file with the same name already exists')
end end
end end
end end
......
...@@ -26,6 +26,7 @@ class GitTagPushService < BaseService ...@@ -26,6 +26,7 @@ class GitTagPushService < BaseService
unless Gitlab::Git.blank_ref?(params[:newrev]) unless Gitlab::Git.blank_ref?(params[:newrev])
tag_name = Gitlab::Git.ref_name(params[:ref]) tag_name = Gitlab::Git.ref_name(params[:ref])
tag = project.repository.find_tag(tag_name) tag = project.repository.find_tag(tag_name)
if tag && tag.target == params[:newrev] if tag && tag.target == params[:newrev]
commit = project.commit(tag.target) commit = project.commit(tag.target)
commits = [commit].compact commits = [commit].compact
......
...@@ -9,6 +9,7 @@ module Issues ...@@ -9,6 +9,7 @@ module Issues
end end
issues = Issue.where(id: issues_ids) issues = Issue.where(id: issues_ids)
issues.each do |issue| issues.each do |issue|
next unless can?(current_user, :update_issue, issue) next unless can?(current_user, :update_issue, issue)
......
...@@ -20,6 +20,7 @@ module MergeRequests ...@@ -20,6 +20,7 @@ module MergeRequests
return unless merge_request.target_branch == project.default_branch return unless merge_request.target_branch == project.default_branch
closed_issues = merge_request.closes_issues(current_user) closed_issues = merge_request.closes_issues(current_user)
closed_issues.each do |issue| closed_issues.each do |issue|
if can?(current_user, :update_issue, issue) if can?(current_user, :update_issue, issue)
Issues::CloseService.new(project, current_user, {}).execute(issue, commit: merge_request) Issues::CloseService.new(project, current_user, {}).execute(issue, commit: merge_request)
......
...@@ -153,6 +153,7 @@ class NotificationService ...@@ -153,6 +153,7 @@ class NotificationService
else else
mentioned_users mentioned_users
end end
recipients = recipients.concat(participants) recipients = recipients.concat(participants)
# Merge project watchers # Merge project watchers
...@@ -176,6 +177,7 @@ class NotificationService ...@@ -176,6 +177,7 @@ class NotificationService
# build notify method like 'note_commit_email' # build notify method like 'note_commit_email'
notify_method = "note_#{note.noteable_type.underscore}_email".to_sym notify_method = "note_#{note.noteable_type.underscore}_email".to_sym
recipients.each do |recipient| recipients.each do |recipient|
mailer.send(notify_method, recipient.id, note.id).deliver_later mailer.send(notify_method, recipient.id, note.id).deliver_later
end end
......
...@@ -3,6 +3,7 @@ module Projects ...@@ -3,6 +3,7 @@ module Projects
def execute def execute
# check that user is allowed to set specified visibility_level # check that user is allowed to set specified visibility_level
new_visibility = params[:visibility_level] new_visibility = params[:visibility_level]
if new_visibility && new_visibility.to_i != project.visibility_level if new_visibility && new_visibility.to_i != project.visibility_level
unless can?(current_user, :change_visibility_level, project) && unless can?(current_user, :change_visibility_level, project) &&
Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility) Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
......
...@@ -82,7 +82,7 @@ class SystemNoteService ...@@ -82,7 +82,7 @@ class SystemNoteService
end end
body << ' ' << 'label'.pluralize(labels_count) body << ' ' << 'label'.pluralize(labels_count)
body = "#{body.capitalize}" body = body.capitalize
create_note(noteable: noteable, project: project, author: author, note: body) create_note(noteable: noteable, project: project, author: author, note: body)
end end
...@@ -125,7 +125,7 @@ class SystemNoteService ...@@ -125,7 +125,7 @@ class SystemNoteService
# Returns the created Note object # Returns the created Note object
def self.change_status(noteable, project, author, status, source) def self.change_status(noteable, project, author, status, source)
body = "Status changed to #{status}" body = "Status changed to #{status}"
body += " by #{source.gfm_reference(project)}" if source body << " by #{source.gfm_reference(project)}" if source
create_note(noteable: noteable, project: project, author: author, note: body) create_note(noteable: noteable, project: project, author: author, note: body)
end end
...@@ -139,7 +139,7 @@ class SystemNoteService ...@@ -139,7 +139,7 @@ class SystemNoteService
# Called when 'merge when build succeeds' is canceled # Called when 'merge when build succeeds' is canceled
def self.cancel_merge_when_build_succeeds(noteable, project, author) def self.cancel_merge_when_build_succeeds(noteable, project, author)
body = "Canceled the automatic merge" body = 'Canceled the automatic merge'
create_note(noteable: noteable, project: project, author: author, note: body) create_note(noteable: noteable, project: project, author: author, note: body)
end end
...@@ -236,6 +236,7 @@ class SystemNoteService ...@@ -236,6 +236,7 @@ class SystemNoteService
else else
'deleted' 'deleted'
end end
body = "#{verb} #{branch_type.to_s} branch `#{branch}`".capitalize body = "#{verb} #{branch_type.to_s} branch `#{branch}`".capitalize
create_note(noteable: noteable, project: project, author: author, note: body) create_note(noteable: noteable, project: project, author: author, note: body)
end end
......
...@@ -21,8 +21,6 @@ class UpdateReleaseService < BaseService ...@@ -21,8 +21,6 @@ class UpdateReleaseService < BaseService
end end
def success(release) def success(release)
out = super() super().merge(release: release)
out[:release] = release
out
end end
end end
...@@ -9,6 +9,7 @@ class UpdateSnippetService < BaseService ...@@ -9,6 +9,7 @@ class UpdateSnippetService < BaseService
def execute def execute
# check that user is allowed to set specified visibility_level # check that user is allowed to set specified visibility_level
new_visibility = params[:visibility_level] new_visibility = params[:visibility_level]
if new_visibility && new_visibility.to_i != snippet.visibility_level if new_visibility && new_visibility.to_i != snippet.visibility_level
unless Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility) unless Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
deny_visibility_level(snippet, new_visibility) deny_visibility_level(snippet, new_visibility)
......
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