Today we will relive a recent refactor completed on our Ruby on Rails project which took some very old code and refactored it to allow for easier reuse. This change deals with user management; specifically users changing their email addresses. We’ll take an interface which required intimate knowledge of the inner workings of a model and encapsulate our business logic into an easy to use service object.
A few weeks ago, a bug was filed in which users were able to change their email address to an address already taken by another user. This can not be allowed since we uniquely identify users by their email address. My first thought was to make sure we were validating new email addresses against existing email addresses.
# snippet from app/models/user.rb
attr_accessor :changed_email
alias_attribute :changed_email?, :changed_email
validate :new_email_cannot_be_taken, if: :changed_email?
validates :new_email, presence: true
def new_email_cannot_be_taken
if User.where(email: new_email).exists?
errors.add(:new_email, I18n.t('activerecord.errors.models.users.attributes.new_email.taken'))
end
endMaybe not exactly how I’d implement this today, but this seems reasonable. So next, I checked the controller.
# from app/controllers/manage_accounts_controller.rb in the update action
if authenticated_from_password && params[:user][:new_email].present?
@user.new_email = params[:user][:new_email]
@user.save!
@user.make_email_verification_code
@user.device_events << EmailChangedEvent.new
UserMailer.new_email_verification(@user).deliver
endAgain, nothing immediately jumps out. We ensure the user is authenticated, and then update the new email attribute which hold the new email address until the user verifies it.
With no fruit from that initial research, we will begin where every good bug squashing journey starts with a failing test reproducing the bug.
# from spec/controllers/manage_account_controller_spec.rb
context 'when the new email is already taken' do
let(:user) { User.make! }
let(:other_user) { User.make! }
let(:request_params) do
{
id: user.id,
user: {
password: user.password,
new_email: other_user.email
}
}
end
it 'does not save the new email' do
expect { put :update, request_params, format: :js }.not_to \
change { user.new_email }.to(other_user.email)
end
endWe are using machinist for our test data generation. User.make! will persist a new user record with some reasonable but dummy data. Specifically in our case we use a serial number within each user’s email address to ensure uniqueness for each invocation of make.
# from spec/support/blueprints.rb
User.blueprint do
email { "user-#{sn}@example.com" }
password { 'b3rry$3cr3t' }
endWe use the two user records to try and change the email address of the first to that of the second. This test failed which validates that we do have a problem.
Failures:
1) ManageAccountController#update when email is already taken does not save the new email
Failure/Error: expect(@user.new_email).not_to eq(other_user.email)
expected: value != "user-0005@example.com"
got: "user-0005@example.com"The one line fix
Looking back at the model code, we see that a virtual attribute called changed_email? is used to determine if the validation is run. We need to set that to true in order for the validation to run.
# from app/controllers/manage_accounts_controller.rb in the update action
if authenticated_from_password && params[:user][:new_email].present?
@user.new_email = params[:user][:new_email]
@user.changed_email = true
@user.save!
@user.make_email_verification_code
@user.device_events << EmailChangedEvent.new
UserMailer.new_email_verification(@user).deliver
endVoilà! A simple one line change and we’re done.
Not so fast. When fixing bugs, it’s good to also think about the circumstances which introduced the issue and try to mitigate recurrances or regressions. A good first line of defense against regressions are tests. As we’ve done above, we’ve reproduced the bug in tests and then verified its fix through the same test. But new tests don’t explain how a bug was introduced in the first place.
We can imagine a previous developer seeing that this validation queries the database in order to check if the new email is already taken by an existing user. Not wanting to run this query everytime a user record is updated and assuming users aren’t changing their email address very often, a possible solution is to toggle a flag when this validation should be run. In most situations, the validation won’t be run and the database query is not run, while still allowing explicit invocation of the validation when necessary.
A subsequent developer came along and didn’t understand this explicit requirement to manually toggle the flag to run the validation and the bug was introduced.
Leave the code better than you found it
From the controller snippets above, we can see that when a user changes their email four things need to happen. We need to:
- Set the
new_emailattribute - Make a verification code
- Create an
EmailChangedEvent - Send a verification email
When faced with a business process like this, the service object pattern is a good fit to encapsulate and clean up your controller code.
Service objects are plain old Ruby objects which are neither models nor controllers. They aim to encapsulate a business operation, as a service. In our case, we will be using a change email service to change an email address for a user. We are using “change email” in the sense that this is what it takes to perform the process of changing an email; not simply changing the email attribute. Once you’ve defined a service in your application, you can inoke the service whenever you need the process performed.
Being good TDDers, we’ll start with tests for this new service object. I’ve left out the implementation here. If you outline you tests’ contexts and examples well, filling in the implementation is straight forward.
Rspec.describe ChangeEmail do
describe '.for_user' do
context 'when a unique email is provided' do
it 'sets the new_email attribute'
it 'creates an EmailChangedEvent for the user'
it 'creates a new verification code'
it 'delivers a new email verification email'
it 'is true'
end
context 'when an existing email is provided' do
it 'does not create an EmailChangedEvent'
it 'does not create a new verification code'
it 'does not set the new_email attribute'
it 'does not send a new email verification email'
it 'is false'
end
endAnd the service object implementation:
class ChangeEmail
def self.for_user(user, new_email)
user.new_email = new_email
if user.valid?
user.device_events << EmailChangedEvent.new
user.make_email_verification_code
UserMailer.new_email_verification_email(user).deliver!
end
user.save
end
endNotice how we are still using the validations to determine if the new_email is duplicate (or any other user issues). ActiveRecord already implements dirty attribute tracking for us. We don’t need to use our own virtual attribute. We can remove the virtual attribute and its alias in favor of the built in ActiveRecord <attr>_changed? method:
validate :new_email_cannot_be_taken, if: :new_email_changed?Finally, by returning the result of user.save, we can still return proper error messages (or successful responses) in our controllers. For example:
# from app/controllers/manage_accounts_controller.rb in the update action
if ChangeEmail.for_user(@user, params[:user][:new_email])
redirect_to manage_account_path(@user)
else
render :edit
endIf we re-run our controller test(s), we’ll see they are still passing, including the duplication validations. It can be very tempting to take the quick and easy route when squashing a bug by only looking at the symptom and not considering the incorrect assumptions that caused the bug in the first place. The quickest fix is not always the correct one. Do not be afraid to think a little bigger and always strive to leave the codebase better for the developers coming behind you. You might very well be that developer and your future self will be grateful.
Happy refactoring!
– Chris