Introduce ServiceResponse to wrap around response

See https://gitlab.com/gitlab-org/gitlab-ce/issues/60730

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