Rails5: Transaction + how to test it

Question

I have a class with following transaction:

# frozen_string_literal: true

class InactivateEmployee
  include ServiceResult

  def call(id)
    begin
      ActiveRecord::Base.transaction do
        employee = Employee.find(id)
        employee.update(is_active: false)

        if employee.tasks.any?
          employee.tasks.delete_all
        end

        response(code: 204, value: employee)
      rescue ActiveRecord::ActiveRecordError
        raise ActiveRecord::Rollback
      end
    rescue ActiveRecord::Rollback => e
      response(code: 422, errors: e)
    end
  end
end

where ServiceResult is:

# frozen_string_literal: true

# ServiceResult should be included in each Service Class to have a unified returned object from each service

ServiceResultResponse = Struct.new(:success?, :response_code, :errors, :value, keyword_init: true)

module ServiceResult
  def response(code:, errors: nil, value: nil )
    ServiceResultResponse.new(
      success?: code.to_s[0] == '2',
      response_code: code,
      errors: errors,
      value: value
    )
  end
end

Question 1: Is this code ok? what could be improved?

Question 2 How to test this transaction with use of Rspec? how to simulate in my test that destroy_all raise and error? i tried sth like that - but it does not work....

   before do
        allow(ActiveRecord::Associations::CollectionAssociation).to receive(:delete_all).and_return(ActiveRecord::ActiveRecordError.new)
      end

Answer 1

Question 1: Is this code ok? what could be improved?

First and foremost, call should not be determining response codes. That wields together making the call with a specific context. That's someone else's responsibility. For example, 422 seems inappropriate, the only possible errors here are not finding the Employee (404) or an internal error (500). In general if you're rescuing ActiveRecordError you could probably be rescuing something more specific.

Does this need to be an entire service object? It's not using a service. It's only acting on Employee. If it's a method of Employee it can be used on any existing Employee object.

class Employee
  def deactivate!
    # There's no need for the find to be inside the transaction.
    transaction do
      # Use update! so it will throw an exception if it fails.
      update!(is_active: false)

      # Don't check first, it's an extra query and a race condition.
      tasks.delete_all
    end
  end
end

Something else is responsible for catching errors and determining response codes. Probably the controller. Generic errors like a database failure should be handled higher up, probably by a default template.

begin
  employee = Employee.find(id)
  employee.deactivate!
rescue ActiveRecord::RecordNotFound
  render status: :not_found
end

render status: :no_content

---

In ServiceResult you're checking success with code.to_s[0] == '2', use math or a Range instead. The caller should not be doing that at all, but it does because you have a module returning a Struct which can't do anything for itself.

ServiceResult should a class with a success? method. It's more flexible, more obvious what's happening, and doesn't pollute the caller's namespace.

class ServiceResult
  # This makes it act like a Model.
  include ActiveModel::Model

  # These will be accepted by `new`
  # You had "errors" but it takes a single error.
  attr_accessor :code, :error, :value

  def success?
    (200...300).include?(code)
  end
end

result = ServiceResult.new(code: 204, error: e)
puts "Huzzah!" if result.success?

I question if it's needed at all. It seems to be usurping the functionality of render. Is it an artifact of InactivateEmployee trying to do too much and having to pass its interpretation of what happened around?

---

Question 2 How to test this transaction with use of Rspec? how to simulate in my test that destroy_all raise and error?

Now that you're not doing too much in a single method, it's much simpler.

describe '#deactivate!' do
  context 'with an active employee' do
    # I'm assuming you're using FactoryBot.
    let(:employee) { create(:employee, is_active: true) }

    context 'when there is an error deleting tasks' do
      before do
        allow(employee.tasks).to receive(:delete_all)
          # Exceptions are raised, not returned.
          .and_raise(ActiveRecord::ActiveRecordError)
      end

      # I'm assuming there's an Employee#active?
      it 'remains active' do
        # same as `expect(employee.active?).to be true` with better diagnostics.
        expect(employee).to be_active
      end
    end
  end
end