How to refactor controller method with several different failure messages

I have a controller "destroy" method that checks for various dependent objects and fails with corresponding alert messages. The deletion is also prevented on the database side with has_many :invoices, dependent: :restrict_with_error but that fails silently for the user.

Rubocop thinks "Assignment Branch Condition size is too high" and I would like to refactor this also because it looks kind of repetitive. However, I struggle with the returns. When I move checks into a private function the return resturns from the function but I also need it to end the "destroy" method. I kind of need a double return.

def destroy
    unless Current.user.default_entity.owner == Current.user ||
           Current.user.default_entity.user_privileged?('manage_contacts')

      redirect_back(fallback_location: contacts_path, alert: t('errors.no_access_rights')) and return
    end

    unless @contact.invoices.count.zero?
      redirect_back(fallback_location: contacts_path,
                    alert: t('errors.cant_delete_contact_with_invoices')) and return
    end

    unless @contact.offers.count.zero?
      redirect_back(fallback_location: contacts_path,
                    alert: t('errors.cant_delete_contact_with_offers')) and return
    end

    @contact.destroy

    redirect_to contacts_path, status: :see_other
  end

I have similar methods in other controllers throughout my application. Is there maybe a pattern to refactoring such types of methods?

Solution

Rubucop is right to complain. The cyclic complexity is actually indicating a few issues with this code. In general if your method ever contains more than one explicit return stop and redo it.

1. Authorization is important. Treat it accordingly.

Doing "inline" authorization like this is a pretty bad idea:

unless Current.user.default_entity.owner == Current.user || Current.user.default_entity.user_privileged?('manage_contacts')
    alert = t('errors.no_access_rights')
end

It's doesn't properly bail and prevent further execution when unauthorized.
You're repeating the authorization logic.
You're returning the wrong status code so the client thinks the request was fine. The only indicator is the flash message. This is really bad for testability.
There are way to many parts dangling out of the model and so many violations of the law of demeter in these three lines.

While the best solution would be to add an authorization layer such as Pundit, you should at least take a leaf from the playbook of Pundit/CanCan and raise an exception when the request is not authorized so that you ensured that it bails and any further callbacks are halted.

# app/errors/authorization_error.rb
class AuthorizationError < StandardError
end

class ContactsController
  # you probably want to reuse this for creating and editing
  before_action :authorize_contact, only: [:destroy]

  private

  def authorize_contact
    # This still stinks but fixing it completely is out of scope
    if entity_owner? || manage_contacts?
      @contact = Contact.find(params[:id])
    else
      raise AuthorizationError      
    end
  end

  def default_entity
    Current.user.default_entity
  end

  def entity_owner?
    default_entity.owner == Current.user
  end

  def manage_contacts?
   default_entity.user_privileged?('manage_contacts')
  end
end

class ApplicationController < ActionController::Base
  rescue_from AuthorizationError, with: :deny_access

  def deny_access
    # either redirect to the login or render an error page
    render plain: "Oh uh. You can't do that silly bear!",
      status: :forbidden
  end
end

As an added bonus you can bypass rescue_from in your tests / specs to assert / expect that an exception is raised.

Better yet would be to extract Current.user.default_entity.owner == Current.user || Current.user.default_entity.user_privileged?('manage_contacts') so that your controller doesn't have too much knowledge of the models. Pundit does this with Policy classes which you can emulate with a PORO.

2. You're making it harder than it should be

Instead of just assuming that @contact.destroy will succeed just check the return value. If destroying fails associations with the :restrict_with_error will populate the errors object - much like how validations work.

def handle_dependency
  case options[:dependent]
  when :restrict_with_exception
    raise ActiveRecord::DeleteRestrictionError.new(reflection.name) if load_target

  when :restrict_with_error
    if load_target
      record = owner.class.human_attribute_name(reflection.name).downcase
      owner.errors.add(:base, :'restrict_dependent_destroy.has_one', record: record)
      throw(:abort)
    end
  else
    delete
  end
end

And just like when creating / editing the object you can use these to provide feedback to the user. With the difference here being that you want to shove them into the flash ("alert" is really just a key in the flash) instead of displaying them over a form.

class ContactsController < ApplicationController
  before_action :authorize_contact, only: [:destroy]

  def destroy
    if @contact.destroy
      redirect_to contacts_path, status: :see_other
    else
      flash[:alert] = @contact.errors.full_messages.join(', ')
      redirect_back(fallback_location: contacts_path)
    end
  end
end

Alternatively instead of the wonky string concatenation of the flash message you could use an array.

class ContactsController < ApplicationController
  before_action :authorize_contact, only: [:destroy]

  def destroy
    if @contact.destroy
      redirect_to contacts_path, status: :see_other
    else
      flash[:alert] = @contact.errors.full_messages
      redirect_back(fallback_location: contacts_path)
    end
  end

  # ...
end

This will however require some changes in your layout to handle displaying array flash messages which I leave up to the reader.