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