Refactoring ActiveRecord custom validations

Question

I have a model called PaymentNotifications. It's used to record payments only if they are valid from Paypal. I need to check that the transaction code they give me is the same one that I get back from them after I post a form.

All of that works. What I do then is check if it's valid based on some criteria as follows:

In the controller I have the following:

tx = params[:tx] 
paypal_data = get_data_from_paypal(tx)
res_hash = create_hash(paypal_data) 
@payment_notification = PaymentNotification.new(:params => res_hash, :quotation_id => res_hash['invoice'],:status => res_hash["payment_status"],:transaction_id => res_hash["txn_id"])
if paypal_data["SUCCESS"] && @payment_notification.is_valid?(tx) && @payment_notification.save
  redirect_to thankyou_path(:id => @payment_notification.quotation_id)
else
  render '/pages/error'
end

Then in the model I run my method is_valid?

validates :params, :quotation_id, :status, :transaction_id, presence: true
validates :transaction_id, :uniqueness => true

def is_valid?(tx)
  amount_paid_valid?(params["payment_gross"]) && transaction_valid?(tx) && is_quotation_unpaid? 
end

def transaction_valid?(tx)
  if tx != transaction_id
    errors.add(:transaction_id, "This transaction is not valid")
    return false
  else
    return true
  end
end

def is_quotation_unpaid?
  if quotation.unpaid?
    return true
  else
    errors.add(:quotation_paid, "This quotation has already been paid.")
    return false
  end
end

def amount_paid_valid?(amount_paid)
  if amount_paid.to_i == quotation.price.to_i
    return true
  else
    errors.add(:amount_paid, "The amount paid does not match the price quoted.")
    return false
  end
end

NOTE: :amount_paid and :quotation_paid are not attributes. They are just keys for the error messages.

I think I'm missing the boat here as there must be a way to do this with the validations built into Rails, but I'm not so good at Rails yet. Could someone help me to refactor this so that it's easier to maintain and in line with best practices?

Answer 1

The main problem here is that you're reimplementing something that Rails already has -- namely, a method to check if an AR object is valid. If you use your method rather than the built-in #valid? your objects will keep passing such actions as #save and #create even when they shouldn't.

In order to bring your custom methods into the fold and include them when calling the built-in validation, just use them as custom validations in your model, like so:

validates :params, :quotation_id, :status, :transaction_id, presence: true
validates :transaction_id, :uniqueness => true
validate :amount_paid_should_match_quote, :quotation_should_be_unpaid
validates_associated :transaction

private

def amount_paid_should_match_quote
  if amount.to_i != quotation.price.to_i
    errors.add(:amount, "does not match the price quoted")
  end
end

def quotation_should_be_unpaid
  if quotation.paid?
    errors.add(:quotation, "has already been paid")
  end
end

A few items to pay attention to:

1. Validation methods shouldn't take arguments, because they're instance methods that are testing existing attributes.
2. Avoid referencing params in your models. Handling requests is the job of controllers.
3. You just need to handle the non-passing scenarios in your methods. Don't worry about returning true when objects are valid, that's up to Rails.
4. Don't write methods to validate associations. Just use validates_associated for that.
5. It helps if you rename your custom methods to be more descriptive of the actual behavior they're trying to enforce. I tried to give you a suggestion, but you can use anything you like.

You can learn more about custom validations at the Rails Guides Validations documentation.