I’ve been adding stuff a bit “blindly” to my controller and currently everything is working as it should, but the controller is gotten very messy. I’m using the gem rails_best_practices to tidy up my code, but it’s currently not giving me any other fixes than moving some of the methods to the model from the controller.

Few questions

Is there other gems like rails_best_practices that I could use to figure out how to tidy up the controller?

and secondly

Any advice on what kind of changes I should do here?

I’m doing some authorisation here for which I think I should use a gem to handle, but currently not using any authorisation gems.

Here’s the mess

class OffersController < ApplicationController
  before_action :authenticate_user!
  before_action :offer_authorised, only: [:show]
  before_action :set_offer, only: [:accept, :reject, :send_signature_requests, :make_payment]
  before_action :is_authorised, only: [:accept, :reject]

  def create
    session[:return_to] ||= request.referer
    if !current_user.stripe_id?
      return redirect_to payment_path, alert: "Connect your bank accout before payments."
    end
    rental = Rental.find(offer_params[:rental_id])

    if rental && rental.user_id == current_user.id
      redirect_to(request.referrer, alert: "You cannot make an offer from your own property.") && return
    end

    if current_user.offers.accepted.any?
      redirect_to(request.referrer, alert: "You've already rented an apartment.") && return
    end

    if Offer.exists?(user_id: current_user.id, rental_id: offer_params[:rental_id])
      redirect_to(request.referrer, alert: "You can only make one offer at a time.") && return
    end

    @offer = current_user.offers.build(offer_params)
    @offer.landlord_id = rental.user.id
    if @offer.save
      redirect_to applications_path, notice: "Offer sent."
    else
      redirect_to request.referrer, flash: {error: @offer.errors.full_messages.join(", ")}
    end
  end

  def destroy
    @offer = Offer.find(params[:id])
    @offer.destroy
    redirect_to request.referrer, notice: "Offer deleted."
  end

  def accept
    if @offer.waiting?
      @offer.accepted!
      @offer.rental.update(active: !@offer.rental.active?)
      Offer.where(landlord_id: current_user.id, rental_id: @offer.rental.id).where.not(id: @offer.id).update_all(status: 2)
      @offer.rental.tours.destroy_all
      @offer.user.tours.destroy_all
      OfferMailer.with(offer: @offer, tenant: @offer.user_id, landlord: @offer.landlord_id).offer_accepted.deliver_now
      flash[:notice] = "Offer accepted."
    end
    redirect_to offer_accepted_path(@offer)
  end

  def reject
    if @offer.waiting?
      @offer.rejected!
      flash[:notice] = "Offer rejected."
    end
    redirect_to request.referrer
  end

  def update
    @offer = Offer.find(params[:id])
    if params[:offer][:due_date]
      @offer.update(due_date: params[:offer][:due_date])
    end
    if params[:offer][:rental_agreement]
      @offer.rental_agreement.purge
      @offer.rental_agreement.attach(params[:offer][:rental_agreement])
    end
    flash[:notice] = "Saved."
    redirect_to request.referrer
  end

  def show
    @offer = Offer.find(params[:id])
    @rental = @offer.rental_id ? Rental.find(@offer.rental_id) : nil
    @comments = Comment.where(offer_id: params[:id])
    @has_attachment_file_sent = Comment.where(user_id: @offer.landlord_id).any? { |obj| obj.attachment_file.attached? }
    respond_to do |format|
      format.html
      format.js { render layout: false }
    end
  end

  def pay_rent
    @offer = Offer.find(params[:id])
    @rental = @offer.rental_id ? Rental.find(@offer.rental_id) : nil
    if @rental.user_referral_reward_count > 0
      referral_rent_payment(@rental, @offer)
    else
      make_payment(@rental, @offer)
    end
    flash[:notice] = "Payment succeeded."
    redirect_to request.referrer
  end

  def send_signature_requests
    landlord_mail = User.where(id: @offer.landlord_id)[0].email
    tenant_mail = User.where(id: @offer.user_id)[0].email
    if  @offer.rental_agreement.attached?
      attachment_file = @offer.rental_agreement
      blob_path = rails_blob_path(attachment_file, disposition: "attachment", only_path: true)
      response = HTTParty.get("URL", body: {
        landlord_mail: landlord_mail,
        tenant_mail: tenant_mail,
        agreement_url: blob_path
      }.to_json,
      headers: {"Content-Type" => "application/json"})
      @offer.update(invitation_uuid: response.parsed_response["invitation_uuid"], document_uuid: response.parsed_response["document_uuid"])
      flash[:notice] = "Signature requests sent."
      redirect_to request.referrer
    else
      flash[:alert] = "Add aggreement."
      redirect_to request.referrer
    end
  rescue => e
    flash[:alert] = "Failed to send signature requests."
    redirect_to request.referrer
  end

  private

  def make_payment(rental, offer)
    unless offer.user_stripe_id.blank?
      payment_methods = Stripe::PaymentMethod.list(
        customer: offer.user_stripe_id,
        type: "sepa_debit"
      )
      payment_intent = Stripe::PaymentIntent.create({
        payment_method_types: ["sepa_debit"],
        payment_method: payment_methods.data[0].id,
        confirm: true,
        off_session: true,
        customer: offer.user_stripe_id,
        amount: offer.amount * 100,
        currency: "eur",
        application_fee_amount: offer.amount * 4,
        metadata: {"name" => User.where(id: offer.user_id).first.full_name, "offer" => offer.id},
        transfer_data: {
          destination: rental.user_merchant_id
        }
      })
    end
  rescue Stripe::CardError => e
    flash[:alert] = e.message
  end

  def referral_rent_payment(rental, offer)
    unless offer.user_stripe_id.blank?
      referrer = User.find_by(id: rental.user.id)
      customer = Stripe::Customer.retrieve(offer.user_stripe_id)
      charge = Stripe::Charge.create(
        customer: customer.id,
        amount: offer.amount * 100,
        description: "Rent for #{rental.listing_name}",
        currency: "eur",
        application_fee_amount: offer.amount * 2,
        metadata: {"name" => User.where(id: offer.user_id).first.full_name, "offer" => offer.id},
        transfer_data: {
          destination: rental.user_merchant_id
        }
      )
      referrer.update_attribute(:referral_reward_count, referrer.referral_reward_count -= 1)
    end
  rescue Stripe::CardError => e
    flash[:alert] = e.message
  end

  def offer_authorised
    unless Offer.exists?(["id = ? AND (landlord_id = ? OR user_id = ?) AND status = ?", params[:id], current_user.id, current_user.id, 1])
      redirect_to dashboard_path,
        alert: "You're not authorized."
    end
  end

  def set_offer
    @offer = Offer.find(params[:id])
  end

  def is_authorised
    redirect_to root_path, alert: "You're not authorized." unless current_user.id == @offer.rental.user_id
  end

  def offer_params
    params.require(:offer).permit(:amount, :rental_id, :status, :priority, :rental_agreement)
  end
end

```

Only have CRUD actions in your controllers

Your controllers should only use the default CRUD actions index, show, new, edit, create, update, destroy.

So rather than doing this

class OffersController < ApplicationController
  def accept; end
end

you would have a dedicated controller

class AcceptedOffersController < ApplicationController
  def create; end
end

Have a look at this article explaining this concept http://jeromedalbert.com/how-dhh-organizes-his-rails-controllers/

Use service objects or Plain Old Ruby Objects

class CreateOffer
  class Result
    def initialize(errors: [])
      @errors = errors
    end

    def valid?
      @errors.empty?
    end
  end

  def self.run(user:, params:)
    new(user: user, params: params).run
  end

  def initialize(user:, params:)
    @user = user
    @params = params
  end

  attr_reader :user, :params

  def run
    return Result.new(errors: 'Connect your bank account') unless bank_account?
    return Result.new(errors: 'You cannot make an offer from your own property.') if own_offer?

    # ...
  end

  private

  def bank_account?
    user.stripe_id?
  end

  def rental
    @rental ||= Rental.find(offer_params[:rental_id])
  end

  def own_offer?
    rental.user_id == user.id
  end
end

class AcceptedOffersController < ApplicationController
  def create
    result = CreateOffer.run(user: current_user, params: params)
    if result.valid?
      redirect_to applications_path, notice: "Offer sent."
    else
      redirect_to(request.referrer, result.errors.join)
    end
  end
end

Here is an example on service objects https://www.toptal.com/ruby-on-rails/rails-service-objects-tutorial

Is there other gems like rails_best_practices that I could use to figure out how to tidy up the controller?

There won’t be any gem which you can just pull in which will magically clean up your code for you. You should learn about Rails and clean code best practices first.

A good book is Practical Object Oriented Design by Sandi Metz.