Sempre há (especialmente com Ruby) muitas maneiras de estruturar código. Aqui está minha abordagem e o que considero melhorias, mas suas opiniões e preferências podem ser diferentes. Além disso, você pode decidir ignorar o Rubocop. Este aviso não diz que é um código errado, apenas que talvez seja possível escrevê-lo melhor.

Código original

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

Variação 1

Vamos nos livrar da duplicação: podemos definir um alerta quando houver um problema e, se o definirmos, retornaremos e redirecionaremos.

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

  unless @contact.invoices.count.zero?
    alert = t('errors.cant_delete_contact_with_invoices')
  end

  unless @contact.offers.count.zero?
    alert = t('errors.cant_delete_contact_with_offers')
  end

  return redirect_back(fallback_location: contacts_path, alert:) if alert

  @contact.destroy

  redirect_to contacts_path, status: :see_other
end

Na verdade, não tornamos o código mais curto ou menos ramificado, mas obtivemos algumas informações: há lógica para encontrar possíveis problemas e há duas maneiras pelas quais esse método termina: redirect_back com erro ou destruição do contato.

Variação 2

Vamos mover a configuração de alerta para um método privado separado, tornando a ação mais limpa

def destroy
  return redirect_back(fallback_location: contacts_path, alert:) if alert

  @contact.destroy

  redirect_to contacts_path, status: :see_other
end

private

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

  unless @contact.invoices.count.zero?
    return t('errors.cant_delete_contact_with_invoices')
  end

  unless @contact.offers.count.zero?
    return t('errors.cant_delete_contact_with_offers')
  end
end

O nome aqui pode não ser muito bom, mas foi apenas uma ideia rápida.

Isso novamente melhorou a legibilidade do destroymétodo principal e tornou mais simples mover os detalhes, o que leva a:

Variação 3

Por que estamos verificando se o modelo pode ser excluído? O controlador não precisa saber disso, esse conhecimento possivelmente está no modelo. Além disso, os modelos ActiveRecord têm uma ótima maneira nativa de gerenciar validações e erros, o que nos ajudará a criar um ponto de extremidade Rails mais padrão. Aqui para referência - Documentação do Rails e Guia do Rails

# app/models/contact.rb
class Contact < ApplicationRecord
  ...

  before_destroy :validate_destruction, prepend: true


  private

  def validate_destruction
    unless Current.user.default_entity.owner == Current.user || Current.user.default_entity.user_privileged?('manage_contacts')
      errors.add t('errors.no_access_rights')
      return false
    end

    unless invoices.count.zero?
      errors.add t('errors.cant_delete_contact_with_invoices')
      return false
    end

    unless offers.count.zero?
      errors.add t('errors.cant_delete_contact_with_offers')
      return false
    end
  end
end

# app/controllers/contracts_controller.rb

def destroy
  if  @contact.destroy
    redirect_to contacts_path, status: :see_other
  else
    redirect_back(fallback_location: contacts_path, alert:) if alert
  end
end

Agradável e limpo.

Eu realmente não testei todo o código, então não garanto que funcione exatamente dessa maneira, estou apenas tentando apontar a direção

Algumas reflexões gerais:

• É sua decisão quanta lógica reside no controlador. No entanto, o código no controlador é muito mais difícil de testar e reutilizar. Ter código no modelo torna muito simples escrever testes unitários para Contact com estados diferentes, verificando se isso nos permitirá excluí-lo.
• Existe até uma abordagem um pouco extrema: faça com que seus controladores sempre executem apenas uma ação em um objeto e retornem uma resposta ok ou não ok.
• Pergunte "quem deve saber/fazer isso?". O controlador deve saber sobre o contato que tem faturas? Somente se for necessário. Aqui, não é.
• Você sempre pode tentar mover o código, agrupá-lo em funções/métodos/ajudantes/.. e ver como fica. Experimente, brinque e tenha uma intuição para um bom código.

Rubucop está certo em reclamar. A complexidade cíclica está, na verdade, indicando alguns problemas com esse código. Em geral, se seu método contiver mais de um return explícito, pare e refaça-o.

1. A autorização é importante. Trate-a adequadamente.

Fazer uma autorização "inline" dessa forma é uma péssima ideia:

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

• Ele não protege adequadamente e impede futuras execuções quando não autorizado.
• Você está repetindo a lógica de autorização.
• Você está retornando o código de status errado, então o cliente acha que estava OK Dory, exceto pela mensagem flash. Isso é realmente ruim para testabilidade.
• Há muitas peças penduradas para fora do modelo e muitas violações da lei de Deméter nessas três linhas.

Embora a melhor solução seja adicionar uma camada de autorização como o Pundit, você deve pelo menos seguir o exemplo do Pundit/CanCan e gerar uma exceção quando a solicitação não for autorizada, para garantir que ela seja desativada e que quaisquer retornos de chamada futuros sejam interrompidos.

# 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 current_user
    Current.user
  end 

  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

Como bônus adicional, você pode ignorar rescue_from em seus testes/especificações para afirmar/esperar que uma exceção seja gerada.

Melhor ainda seria extrair Current.user.default_entity.owner == Current.user || Current.user.default_entity.user_privileged?('manage_contacts')para que seu controlador não tenha muito conhecimento dos modelos. O Pundit faz isso com classes Policy que você pode emular com um PORO.

2. Você está tornando mais difícil do que deveria ser

Em vez de apenas assumir que @contact.destroyterá sucesso, basta verificar o valor de retorno. Se a destruição falhar, essas restrict_withassociações de erro preencherão o objeto de erros.

E assim como ao criar/editar o objeto, você pode usá-los para fornecer feedback ao usuário. Com a diferença aqui de que você quer colocá-los no flash ("alerta" é realmente apenas uma tecla no flash) em vez de exibi-los em um formulário.

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

Como alternativa, em vez da concatenação de strings da mensagem flash, você pode usar uma matriz.

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

No entanto, isso exigirá algumas alterações no seu layout para lidar com a exibição de mensagens flash do array, o que deixo para o leitor.

Você pode refatorar seu método destroy usando um before_action ou um método auxiliar que retorna um valor booleano indicando se a exclusão deve prosseguir. Você pode usar um método de cláusula guard que retorna true se a exclusão for bloqueada e executa o redirecionamento dentro dela.

before_action :authorize_destroy, only: :destroy
before_action :check_dependent_records, only: :destroy

def destroy
  @contact.destroy
  redirect_to contacts_path, status: :see_other
end

private

def authorize_destroy
  return if 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

def check_dependent_records
  if @contact.invoices.exists?
    redirect_back(fallback_location: contacts_path, alert: t('errors.cant_delete_contact_with_invoices')) and return
  end

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