Merge branch '60730-service-response' into 'master'

Introduce ServiceResponse to wrap around response

Closes #60730

See merge request gitlab-org/gitlab-ce!27516

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
 

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

app/services/service_response.rb

+# 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

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

spec/services/delete_branch_service_spec.rb

@@ -8,20 +8,24 @@ describe DeleteBranchService do
   let(:user) { create(:user) }
   let(:service) { described_class.new(project, user) }
 
+  shared_examples 'a deleted branch' do |branch_name|
+    it 'removes the branch' do
+      expect(branch_exists?(branch_name)).to be true
+
+      result = service.execute(branch_name)
+
+      expect(result.status).to eq :success
+      expect(branch_exists?(branch_name)).to be false
+    end
+  end
+
   describe '#execute' do
     context 'when user has access to push to repository' do
       before do
         project.add_developer(user)
       end
 
-      it 'removes the branch' do
-        expect(branch_exists?('feature')).to be true
-
-        result = service.execute('feature')
-
-        expect(result[:status]).to eq :success
-        expect(branch_exists?('feature')).to be false
-      end
+      it_behaves_like 'a deleted branch', 'feature'
     end
 
     context 'when user does not have access to push to repository' do
@@ -30,8 +34,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

spec/services/service_response_spec.rb

+# 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