diff options
| -rw-r--r-- | app/controllers/projects/branches_controller.rb | 8 | ||||
| -rw-r--r-- | app/services/delete_branch_service.rb | 24 | ||||
| -rw-r--r-- | app/services/service_response.rb | 31 | ||||
| -rw-r--r-- | lib/api/branches.rb | 4 | ||||
| -rw-r--r-- | spec/services/delete_branch_service_spec.rb | 6 | ||||
| -rw-r--r-- | spec/services/service_response_spec.rb | 57 |
6 files changed, 108 insertions, 22 deletions
diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb index e14abbf7c78..fc708400657 100644 --- a/app/controllers/projects/branches_controller.rb +++ b/app/controllers/projects/branches_controller.rb @@ -100,14 +100,14 @@ class Projects::BranchesController < Projects::ApplicationController respond_to do |format| format.html do - flash_type = result[:status] == :error ? :alert : :notice - flash[flash_type] = result[:message] + flash_type = result.error? ? :alert : :notice + flash[flash_type] = result.message redirect_to project_branches_path(@project), status: :see_other end - format.js { head result[:return_code] } - format.json { render json: { message: result[:message] }, status: result[:return_code] } + format.js { head result.http_status } + format.json { render json: { message: result.message }, status: result.http_status } end end diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb index 4c3ac19f754..fd41ce54486 100644 --- a/app/services/delete_branch_service.rb +++ b/app/services/delete_branch_service.rb @@ -6,27 +6,25 @@ class DeleteBranchService < BaseService branch = repository.find_branch(branch_name) unless current_user.can?(:push_code, project) - return error('You dont have push access to repo', 405) + return ServiceResponse.error( + message: 'You dont have push access to repo', + http_status: 405) end unless branch - return error('No such branch', 404) + return ServiceResponse.error( + message: 'No such branch', + http_status: 404) end if repository.rm_branch(current_user, branch_name) - success('Branch was deleted') + ServiceResponse.success(message: 'Branch was deleted') else - error('Failed to remove branch') + ServiceResponse.error( + message: 'Failed to remove branch', + http_status: 400) end rescue Gitlab::Git::PreReceiveError => ex - error(ex.message) - end - - def error(message, return_code = 400) - super(message).merge(return_code: return_code) - end - - def success(message) - super().merge(message: message) + ServiceResponse.error(message: ex.message, http_status: 400) end end diff --git a/app/services/service_response.rb b/app/services/service_response.rb new file mode 100644 index 00000000000..1de30e68d87 --- /dev/null +++ b/app/services/service_response.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class ServiceResponse + def self.success(message: nil) + new(status: :success, message: message) + end + + def self.error(message:, http_status: nil) + new(status: :error, message: message, http_status: http_status) + end + + attr_reader :status, :message, :http_status + + def initialize(status:, message: nil, http_status: nil) + self.status = status + self.message = message + self.http_status = http_status + end + + def success? + status == :success + end + + def error? + status == :error + end + + private + + attr_writer :status, :message, :http_status +end diff --git a/lib/api/branches.rb b/lib/api/branches.rb index 5c98b0ad56c..65d7f68bbf9 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -162,8 +162,8 @@ module API result = DeleteBranchService.new(user_project, current_user) .execute(params[:branch]) - if result[:status] != :success - render_api_error!(result[:message], result[:return_code]) + if result.error? + render_api_error!(result.message, result.http_status) end end end diff --git a/spec/services/delete_branch_service_spec.rb b/spec/services/delete_branch_service_spec.rb index 43a70d33d2d..6bcb67c972c 100644 --- a/spec/services/delete_branch_service_spec.rb +++ b/spec/services/delete_branch_service_spec.rb @@ -19,7 +19,7 @@ describe DeleteBranchService do result = service.execute('feature') - expect(result[:status]).to eq :success + expect(result.status).to eq :success expect(branch_exists?('feature')).to be false end end @@ -30,8 +30,8 @@ describe DeleteBranchService do result = service.execute('feature') - expect(result[:status]).to eq :error - expect(result[:message]).to eq 'You dont have push access to repo' + expect(result.status).to eq :error + expect(result.message).to eq 'You dont have push access to repo' expect(branch_exists?('feature')).to be true end end diff --git a/spec/services/service_response_spec.rb b/spec/services/service_response_spec.rb new file mode 100644 index 00000000000..30bd4d6820b --- /dev/null +++ b/spec/services/service_response_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +ActiveSupport::Dependencies.autoload_paths << 'app/services' + +describe ServiceResponse do + describe '.success' do + it 'creates a successful response without a message' do + expect(described_class.success).to be_success + end + + it 'creates a successful response with a message' do + response = described_class.success(message: 'Good orange') + + expect(response).to be_success + expect(response.message).to eq('Good orange') + end + end + + describe '.error' do + it 'creates a failed response without HTTP status' do + response = described_class.error(message: 'Bad apple') + + expect(response).to be_error + expect(response.message).to eq('Bad apple') + end + + it 'creates a failed response with HTTP status' do + response = described_class.error(message: 'Bad apple', http_status: 400) + + expect(response).to be_error + expect(response.message).to eq('Bad apple') + expect(response.http_status).to eq(400) + end + end + + describe '#success?' do + it 'returns true for a successful response' do + expect(described_class.success.success?).to eq(true) + end + + it 'returns false for a failed response' do + expect(described_class.error(message: 'Bad apple').success?).to eq(false) + end + end + + describe '#error?' do + it 'returns false for a successful response' do + expect(described_class.success.error?).to eq(false) + end + + it 'returns true for a failed response' do + expect(described_class.error(message: 'Bad apple').error?).to eq(true) + end + end +end |