Dec 1, 2018

We are all legacy code - Part 6

In Part 5 we were able to resolve our email configuration issues and also removed our Twitter authentication functionality. This leaves us with only 2 issues left to address from the list of tasks we originally compiled in Part 4 .

  • Update our email change functionality to update the name field in the user table.
  • Update our sign-up functionality to prevent new sign-ups.

Today we’ll be working on the first issue.

Getting started

As mentionned before, I’m not sure this series of posts is really a “follow along” type of series, but it is possible.

If you’ve been following along, you can continue with the code from part 5. Or you can clone the code as it stood after part 5.

Clone the Repo

If cloning the code from GitHub:

Terminal
git clone -b part-05-remove-twitter-auth https://github.com/riebeekn/wmcgy2 budget-app
cd budget-app

If getting the code from GitHub, we also need to repeat our steps to set our local Ruby version and run bundle install.

Terminal
rbenv local 2.3.6
Terminal
bundle install

And with that, we are good to go.

Today’s objective

We’re going to tackle the issue with our email change functionality, let’s quickly review how it is currently working.

When a user changes their email, we’re not seeing the change reflected in the UI. For instance say our user Bob Log, signed up to our application with the email bob@example.com. If he then changes his email in our application’s Account page to bob2@example.com, the UI will still display the original email. This despite him now having to sign in with bob2@example.com.

We discovered the name field is not updated in the database on an email change.

This is problematic as it is the name field which we display in the UI:

<li class="current-user"><%= link_to "Signed in as #{current_user.name}", account_path %></li>

One option is to update the name field when the user updates their email… but on further reflection why do we have a name field in the first place, considering it is just a duplicate of the user’s email? Well the reason is that when a user signs in via Google OAuth the response from Google contains the user’s name. In this scenario we use this value to populate the name field; versus when a user signs up via our custom authentication and the name field is defaulted to the user’s email.

The OAuth code is:

def self.create_with_omniauth(auth)
  User.create do |user|
    user.provider = auth["provider"]
    user.uid = auth["uid"]
    user.name = auth["info"]["name"]
    user.email = auth["info"]["email"]
    ...
    ...

We can see the user’s name is being passed back from the OAuth provider via the ["info"]["name"] key.

This contrasts with the code that gets run when a user is created via custom authentication, in this case the name is set to the user’s email:

def set_name_if_empty
  if name.nil?
    self.name = email
  end
end

The result of the divergent OAuth code path is that instead of displaying "Signed in as bob@example.com", a user signed in via Google with a Google account name of Bob Log, would see "Signed in as Bob Log". Not much of a gain for this particular pain!

Exploring further the option of updating the name field on any email updates, this should work fine as a user’s account page displays differently depending on how they signed up. In the case of OAuth users, only the Delete account functionality is available; this makes sense as there is no site specific email or password, this is all handled by the OAuth provider.

Does keeping these fields in sync really make sense however? Maybe it would be better to add logic to the UI to display the email for custom authenticated users and only use the name field for OAuth users.

Again, this all seems like a look of extra complication for not much gain. And I don’t really like having the name field in the database populated with email values in the case of user’s who signed up via the custom authentication. Having emails in a name field doesn’t make sense and could be a cause for confusion down the line. We could update the field to be nullable and add logic to insert nulls for custom auth, but again this seems like a lot of complication for no real benefit.

In retrospect I don’t know why I bothered to make use of the name value returned by the OAuth providers. I think it was a poor design decision… so let’s get rid of it altogether and always display the email value regardless of whether the user signed up via OAuth or custom authentication.

Man… that was a lot of lip flapping… I think ripping out the name field is a good decision thou, so let’s get on with it already!

Creating a branch for our fixes

We’ll start off by creating a new branch.

Terminal
git checkout -b remove-name-field-from-user-table

And now onto some code!

Removing the User.Name field

Let’s start by updating the UI.

UI

Our UI changes are very simple. We need to update a single line in the header code. It currently looks like:

/app/views/layouts/_signed_in_header.html.erb …line 13
<li class="current-user"><%= link_to "Signed in as #{current_user.name}", account_path %></li>

And we need to change it to the below, we’ve swapped out current_user.name for current_user.email.

/app/views/layouts/_signed_in_header.html.erb …line 13
<li class="current-user"><%= link_to "Signed in as #{current_user.email}", account_path %></li>

Database

Next lets update our database. We’ll use a Rails migration to remove our column.

Terminal
rails g migration remove_name_from_users name:string

This generates our migration file:

/db/migrate/(the current time stamp)_remove_name_from_users
class RemoveNameFromUsers < ActiveRecord::Migration
  def up
    remove_column :users, :name
  end

  def down
    add_column :users, :name, :string
  end
end

The migration looks good, no need to make any changes to it, so we can go straight to running the migration.

Terminal
bundle exec rake db:migrate

Looking in TablePlus we can see that the structure of the users table has been updated.

No more name column!

Code

Now we can update our code. The only file we need to touch is user.rb, and our changes all involve removing code.

The first thing to do is remove the comment at line 16.

/app/models/user.rb …line 16
#  updated_at             :datetime        not null
#  name                   :string(255)
#  provider               :string(255)

Next the before filter at line 37 needs to be removed.

/app/models/user.rb …line 37
before_create { set_name_if_empty }

And the method referred to in the before filter can also be removed.

/app/models/user.rb …line 255
def set_name_if_empty
  if name.nil?
    self.name = email
  end
end

The final change is to update the create_with_omniauth method, we just need to remove the user.name = auth["info"]["name"] line.

/app/models/user.rb …line 269
def self.create_with_omniauth(auth)
  User.create do |user|
    user.provider = auth["provider"]
    user.uid = auth["uid"]
    user.name = auth["info"]["name"]
    user.email = auth["info"]["email"]

And with that, our code changes are complete.

Tests

With the above changes, I imagine we’ll have a few tests that will now fail. Since our database schema has changed, we need to rebuild our test database prior to running the tests.

Terminal
bundle exec rake db:test:prepare

And with that we’re ready for our tests.

Terminal
bundle exec rspec spec/

4 failures, not too bad, let’s start by fixing user_spec.rb.

Based on our test output, it looks like the first issue just has to do with us checking that the name attribute is part of the user model.

We removed the name attribute so we also can remove the test!

/spec/models/user_spec.rb …line 46
it { should respond_to(:name) }

The next two failures occur in the tests at line 159 and 171 (now lines 158 and 170 after removing line 46). We need to remove the @user.name.should eq @name statements from both of these tests. The updated tests look like:

/spec/models/user_spec.rb …line 158
it "should create a new user if and return it if user does not exist" do
  expect do
    @user = User.from_omniauth(@auth)
  end.to change(User, :count).by(1)
  @user.should_not be_nil
  @user.email.should eq @email
  @user.uid.should eq @uid
  @user.provider.should eq @provider
  @user.should be_active
end

it "should return the user if the user already exists and not create a new one" do
  User.from_omniauth(@auth)
  expect do
    @user = User.from_omniauth(@auth)
  end.to_not change(User, :count).by(1)
  @user.should_not be_nil
  @user.email.should eq @email
  @user.uid.should eq @uid
  @user.provider.should eq @provider
  @user.should be_active
end

That takes care of the changes to user_spec. The test that is failing in users_spec is no longer valid so it can be removed.

/spec/requests/users_spec.rb …line 307
it "should use the email value for the name attribute" do
  click_button "Sign up"
  user = User.find_by_email("user@example.com")
  user.name.should eq "user@example.com"
end

Now we should get a fully passing test suite.

Terminal
bundle exec rspec spec/

Perfect!

We should probably do a quick manual test to make sure our OAuth authentication still works.

Manual testing that our OAuth authentication still works

So let’s fire up the server.

Terminal
rails s

And then attempt to login via Google.

Clicking the Google link results in:

So what’s going on here? The problem is that we haven’t set up our Google OAuth configuration in our development environment.

Configuring the development environment for OAuth

We need to pass a key and secret value into the Google OAuth request, we can see that these values have been set in our production environment:

These values were set using the Heroku CLI via the config:set command, i.e.

Terminal
heroku config:set GOOGLE_KEY=<the key>

In development we already have a facility for setting these Heroku / production specific variables, as can be seen in the application.rb file.

/config/application.rb
...
...
module Wmcgy
  class Application < Rails::Application
    # Load heroku vars from local file
    heroku_env = File.join(Rails.root, 'config', 'heroku_env.rb')
    load(heroku_env) if Rails.env.development? && File.exists?(heroku_env)
    ...
    ...

This code indicates that when running under development we should load a file called heroku_env.rb (if it exists). Any environment variables contained in the file will as a result be available to our development instance.

We don’t want our keys etc. in source control as these are not things we want viewable publicly, this is reflected in our .gitignore file.

/.gitignore
...
...
/config/heroku_env.rb

# text file used to keep track of enhancements / changes
*.txt

To be honest I’d completely forgotten about the heroku_env file. To make it easier to remember in the future, let’s create a template file in the root of our project.

Terminal
touch heroku_env.rb.template

The contents of the file will be:

/heroku_env.rb.template
#  This file is used to hold environment variables that are required to
#  run the application locally but that contain sensitive information so
#  should not be checked into source control.  As a result we can't add
#  these values directly to /config/environments/development.rb
#
#  Set-up steps:
#  1) Copy this file to config/heroku_env.rb, i.e. cp heroku_env.rb.template config/heroku_env.rb
#  2) Enter appropriate values for the configuration settings below

ENV['GOOGLE_KEY'] = "put the google key here"
ENV['GOOGLE_SECRET'] = "put the google secret here"

Now we’ll follow the instructions we’ve added to the file and create the heroku_env.rb file.

Terminal
cp heroku_env.rb.template config/heroku_env.rb

Our code editor visually provides us confirmation that the config file won’t be added to source control, it’s greyed out in the file tree.

We could also confirm the same by running git status.

We now need to update the values.

/config/heroku_env.rb
#  This file is used to hold environment variables that are required to
#  run the application locally but that contain sensitive information so
#  should not be checked into source control.  As a result we can't add
#  these values directly to /config/environments/development.rb
#
#  Set-up steps:
#  1) Copy this file to config/heroku_env.rb, i.e. cp heroku_env.rb.template config/heroku_env.rb
#  2) Enter appropriate values for the configuration settings below

ENV['GOOGLE_KEY'] = "10..."
ENV['GOOGLE_SECRET'] = "x..."

Giving it another go now that we’re all configured

Now we’ll restart the server.

Terminal
rails s

And if we click the Google sign in link we now see:

Signing in with out Google credentials works.

So looks like the removal of our name column is a success! The tests passed and our manual test looks good.

Let’s wrap up by merging our changes into our refactoring branch and removing our current working branch.

Terminal
git commit -am "removed name field from users"
git checkout 2018-update-and-refactor
git merge remove-name-field-from-user-table
git branch -d remove-name-field-from-user-table

Summary

This was pretty easy once we figured out the approach we wanted to take. We managed to fix our bug and also improved the design of our application.

We’re quickly running out of tasks! All we have left is:

  • Update our sign-up functionality to prevent new sign-ups.

We’ll tackle the above next time out.

Thanks for reading and I hope you enjoyed the post!

Share this post!

Comment on this post!