When it comes to ORM's, ActiveRecord is personal favourite of mine especially considering features such as AREL an AR Callbacks. In solving challenges, there's always multiple ways of skinning a cat, and here I will show you some interesting issues I've tackled today — and how I approached this from a troubleshooting aspect, and the steps I took literally in my head.
Come on Short Round, join me on a riveting adventure!
With great responsibility, comes great use of the around filter
Today I was troubleshooting an issue seen on a client's staging server, which pulls in Agents over an API. One particular issue was that emails were being sent out with invalid login credentials, when I was pretty sure those records were being persisted — even though I did not write the original implementation of this feature.
before_validation :generate_password
def generate_password
if self.password.nil?
self.password = SecureRandom.hex(4)
self.send_login_email
end
end
At first glance this seemed alright, but I later noticed that a bespoke create_or_update method was being leveraged by relying on instance#update_attributes, which will in fact save any instances of a #new_record?. There was also an underlying issue due to the update nature as well, but I will come back to that.
As you may have figured out by now, this callback runs prior to validation, and the email is sent out without any confirmation of whether the instance was actually persisted to disk or not. One approach may consider use of ActiveModel::Dirty but I decided to take a different route. One should also note that there is a presence validation on the password attribute, so any new instances with an empty password would fail the call to #update_attributes. It was really hard to get a sense of what was going on, even with the excellent pry gem, so I decided that exceptions should be raised rather than failing silently and replaced the call with #update_attributes!, which was a good move. This made me see exactly why instances were not being persisted.
I basically needed away to check that if a new password is generated for an Agent, and only then send out the email notification after persisting the agent. To the rescue we have the awesome around_filter. This is simply a method that yields to a block, and the method's binding context is the record itself; the block is the actual save operation.
around_save :generate_password
private
def generate_password
return if self.persisted?
pass_generated = false
if self.new_record?
password = SecureRandom.hex(4)
self.password = password
self.password_confirmation = password
pass_generated = true
end
yield
send_login_email if pass_generated
end
I decided to also throw an assumption into the mix to keep this approach sane, and that is the DB should never contain any Agents with a nil password attribute — which means that we are really only bothered about new Agents. This allowed me to easily update the password columns (for Devise) on a new instance of Agent, evaluate the block, and then send the notification making sure that a new password was in fact generated.
However, you'll realise that our around_save call would never be called as our #update_attributes call will fail at the validation step. Why? We forgot to ensure the password attribute as non-nil during the validation stage. Let's tweak things a bit
before_validation :generate_password_for_new_record
around_save :generate_password
private
def generate_password_for_new_record
self.password = SecureRandom.hex(4) if self.new_record?
end
def generate_password
return if self.persisted?
pass_generated = false
if self.new_record?
password = SecureRandom.hex(4)
self.password = password
self.password_confirmation = password
pass_generated = true
end
yield
send_login_email if pass_generated
end
def send_login_email
if Rails.env.production? || non_production_agent_email?
Spree::MyAccount::AgentMailer.new(self.email, self.password).deliver
end
end
Pragmatic Matching, when you don't need a Match object
I'm sharing a particular trick I've come to use on many an occasion when I've found myself needing to verify if a Regex match has taken place, but really don't care for a Match object. Why burden the GC more than we really need to?
def send_login_email
if Rails.env.production? || non_production_agent_email?
Spree::MyAccount::AgentMailer.new(self.email, self.password).deliver
end
end
def non_production_agent_email?
self.email.match(/clientdomain.co.uk/i).length > 0
end
Whilst looking at this potential conundrum with my trusty crowbar, I noticed a NoMethodError being raised on calling #length on nil. It was clear that this is lacking some nil-guards.
Sure, we could also proceed with the approach of calling #match but let's look at away to avoid that by attempting self.email =~ /clientdomain.co.uk/i. This approach returns the index of where the match occurred or nil and we can take advantage of that.
def non_production_agent_email?
if self.email && self.email.present?
return true if self.email =~ /clientdomain.co.uk/i
end
false
end
I also like using what I've come to called the short circuit, where a method exits early on for a certain condition; if that condition is not met, the default behaviour follows — the predicate method #non_production_agent_email? above returns false by default, and true only if our conditional is met.
However, as Antonis correctly pointed out, since we are in the context of Rails, there is no need for combining both a nil-guard and #present? as, this returns false on nil.
It is also good practice to use double-negation for predicate methods as this inherently implies that we are returning a boolean rather than an object (and in Ruby, objects are truthy as well). So how do we convert and object to a boolean? Yup, break out the double-bang... you know you want to!
def non_production_agent_email?
!!(self.email.present? && self.email =~ /clientdomain.co.uk/i)
end
I hope you've enjoyed our little adventure and I plan on continuing this series of 'Master Classes' in future. Do tell me what you think in the comments below and if you'd like to discuss this further hit me up on Twitter (@bsodmike).
Contributions
Thanks to Antonis Tzorvas for pointing out an improvement for the #non_production_agent_email? predicate method.