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
end

Maybe 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
end

Again, 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
end

We 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' }
end

We 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
end

Voilà! 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_email attribute
  • 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
end

And 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
end

Notice 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
end

If 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