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.
Maybe not exactly how I’d implement this today, but this seems reasonable. So next, I checked the controller.
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.
We are using machinist for our test data
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
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.
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
true in order for the validation to run.
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
- Make a verification code
- Create an
- 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.
And the service object implementation:
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
Finally, by returning the result of
user.save, we can still return proper
error messages (or successful responses) in our controllers. For example:
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.