Changing a user status flag, with validation

Question

I am writing a function which on update of any attribute of model sets the status of variable is_kyc_verified to false.

Here is the code of the User model and the method which changes the status:

class User < ActiveRecord::Base
  # Include default devise modules. Others available are:
  # :confirmable, :lockable, :timeoutable and :omniauthable
  devise :database_authenticatable, :registerable,
         :recoverable, :rememberable, :trackable, :validatable
  before_update :change_kyc_status, unless: :is_kyc_verified_changed?

  #Associations
  has_one  :address, dependent: :destroy
  has_one  :kyc, dependent: :destroy
  has_one  :pan_detail, dependent: :destroy
  has_one  :document, dependent: :destroy
  has_many :nominee_details, dependent: :destroy
  has_many :bank_details, dependent: :destroy

  #Accept Attributes for associated model

  accepts_nested_attributes_for :address, :kyc, :pan_detail,
                                :document, :nominee_details, :bank_details,
                                allow_destroy: true, reject_if: :all_blank


  #validates
  validates :name, :mobile_no, :gender, :dob,  presence: true
  validates :mobile_no, numericality: true, length: { is: 10 }

  private
  ##
  # Check if is_kyc_verified is set to true
  # if 'yes' then alert user and set is_kyc_verified to false
  def change_kyc_status
    self.is_kyc_verified = false if self.valid? and self.is_kyc_verified.present?
    true
  end

end

As you can see the method initially used to return the self.is_kyc_verified which was false, which in turn resulted in "rollback of transaction" so I explicitly added a true at the end so it won't "rollback the transaction"

However I feel, that this is not the right way to implement this function. Can you please review my code and suggest me the right way to do so?

Answer 1

Maybe just change the before_update to execute when #changed? is true?

http://api.rubyonrails.org/classes/ActiveModel/Dirty.html#method-i-changed-3F

EDIT: After looking at the question again, I'd change my answer. I think it's reasonable to return true. There's not really anyway around it, considering the way callbacks handle the return value. I suppose you could invert the values of the is_kyc_verified to be something like is_kyc_unverified?. But, that already seems confusing.

I do however think you should move the unless: :is_kyc_verified_changed? into the change_kyc_status method. I find the current flow a little hard to follow, as you must check in two places if is_kyc_verified will be set.

I might change it to something like:

before_update :confirm_is_kyc_verified
...
def confirm_is_kyc_verified
  if !is_kyc_verified.changed? && is_kyc_verified && valid?
    is_kyc_verified = false
  end

  true
end