Nov 25, 2018

We are all legacy code - Part 5

In Part 4 we were able to deploy our updated code to a Heroku instance and we also came up with a list of remaining issues to tackle:

  • Fix / investigate our production email configuration issues.
  • Rip out our Twitter authentication functionality.
  • Update our email change functionality to update the name field in the user table.
  • Test the password change functionality on a Heroku test instance.
  • Update our sign-up functionality to prevent new sign-ups. As mentionned in part 1, I don’t really want anyone else using the application. We’ll have to think how we want to handle this.
  • Update our footer to display the correct year.

I think the most impactful change at this point is going to be getting our email configuration issues sorted. This issue impacts various aspects of the application. Fixing it is going to result in a number of issues going away. So let’s start with the email configuration and see how it goes.

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 3 (remember we made no code changes in part 4). Or you can clone the code as it stood after part 3.

Clone the Repo

If cloning the code from GitHub:

Terminal
git clone -b part-03-get-tests-running 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

As mentionned above, we’re going to start by examining our email configuration issue. If it goes well, we might tackle a few more issues.

Creating a branch for our fixes

We’ll start off by creating a new branch.

Terminal
git checkout -b fix-email-configuration

With that out of the way, let’s get to it!

Email configuration

In order to trouble-shoot the email issue, we’ll create a new Heroku test instance. If you recall in part 4 we went through the steps for getting this set-up, we’ll briefly re-iterate those steps below.

Setting up a Heroku test instance

First we need to create the instance.

Terminal
heroku create temp-wmcgy -s=cedar-14

Then deploy our code.

Terminal
git push heroku fix-email-configuration:master

Then run our migrations.

Terminal
heroku run rake db:migrate

And now we can open our app.

Terminal
heroku open

With that you should see our application running in a browser.

Configuring email

We are using the SendGrid Heroku add-on to handle email for our application.

Let’s install the add-on into our test Heroku instance.

Installing and testing the add-on in our test instance

Terminal
heroku addons:create sendgrid:starter

Now let’s see if our contact page works.

After clicking send we get:

And in about 30 seconds the email shows up in my inbox. So it appears email is working fine on our https://temp-wmcgy.herokuapp.com/ test instance.

Maybe we just need to uninstall and re-install the add-on in production?

Re-installing the add-on in production

So I removed the instance from my production Heroku instance.

Terminal
heroku addons:destroy sendgrid

And then re-added it.

Terminal
heroku addons:create sendgrid:starter

And guess what? Email is now working fine on the production instance. So perhaps something got messed up with the SendGrid installation on production and a simple drop / re-add of the add-on seems to have fixed things.

I’ve confirmed that this also fixes the issues we had with the various account and registration steps that were broken. Sweet… an easy win!

A review of our remaing issues

Now that email is working, our updated list of outstanding issues looks like:

  • Registration / sign in
    • Sign up - works
    • Sign in with Twitter - errors out with “We’re sorry, but something went wrong.” - not going to fix, going to remove this functionality
  • Accounts
    • Change email - mostly works - main functionality works, but the User.Name field is not updated on an email change
    • Change password - unable to test until have sign up working. - works… need to test on a Heroku instance just to be sure however
    • Forgot password - works
  • Contact page
    • Sending an email - works

I validated that the change password functionality works on our test Heroku instance so we can also be done with that one… this leaves:

  • Registration / sign in
    • Sign in with Twitter - errors out with “We’re sorry, but something went wrong.” - not going to fix, going to remove this functionality
  • Accounts
    • Change email - mostly works - main functionality works, but the User.Name field is not updated on an email change

So let’s rip out our Twitter authentication functionality… this should be easy, after all tearing things down is always easier than building them up!

We’re down with our Heroku test instance for today, so we might as well clean in up before moving on.

Terminal
heroku apps:destroy temp-wmcgy --confirm temp-wmcgy

Removing Twitter authentication

Let’s start by removing the omniauth-twitter Gem from our Gemfile.

Removing the Gem

/Gemfile
...
...
gem 'newrelic_rpm'
#gem 'omniauth-twitter'
gem 'omniauth-google-oauth2'
...
...

Next we’ll run bundle install.

Terminal
bundle install

Now I’m hoping our tests will point us to any additional code we need to remove.

Terminal
bundle exec rspec spec/

It turns out not really… instead of failing tests we get an error when trying to run the tests.

So instead the approach we’ll take is to search for any reference to twitter in our code.

If we search our code base for twitter, we see a few areas we can start to rip some code out of. Let’s start with the UI. Our current view code is:

/app/views/sessions/_oauth_sign_in.html.erb
<section class="span8 offset2 auth-providers">
  <header class="auth-provider-header">Or sign in through Google or Twitter.</header>
  <a href="/auth/google_oauth2" class="auth-provider">
    <%= image_tag "google_apps_64.png", size: "64x64", alt: "Google" %>
    Google
  </a>
  <a href="/auth/twitter" class="auth-provider">
    <%= image_tag "twitter_64.png", size: "64x64", alt: "Twitter" %>
    Twitter
  </a>
</section>

We need to remove the Twitter link, so it becomes:

/app/views/sessions/_oauth_sign_in.html.erb
<section class="span8 offset2 auth-providers">
  <header class="auth-provider-header">Or sign in through Google.</header>
  <a href="/auth/google_oauth2" class="auth-provider">
    <%= image_tag "google_apps_64.png", size: "64x64", alt: "Google" %>
    Google
  </a>
</section>

Note we’ve also updated the <header> text to remove the reference to Twitter.

Next we can update our user model, we have some Twitter specific logic in the create_with_omniauth method.

/app/models/user.rb …line 277
def self.create_with_omniauth(auth)
  User.create do |user|
    user.provider = auth["provider"]
    user.uid = auth["uid"]
    user.name = auth["info"]["name"]
    if user.provider == 'twitter'
      # twitter does not provide the user's email
      user.email = "#{user.uid}.no.email.for.twitter@example.com"
    else
      user.email = auth["info"]["email"]
    end
    ...
    ...

We just need to remove the if statement and logic.

/app/models/user.rb …line 277
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 also have some configuration code to alter.

/config/initializers/omniauth.rb
Rails.application.config.middleware.use OmniAuth::Builder do
  provider :twitter, ENV['TWITTER_KEY'], ENV['TWITTER_SECRET']
  provider :google_oauth2, ENV['GOOGLE_KEY'], ENV['GOOGLE_SECRET']
end
...
...

The reference to the Twitter provider needs to be removed.

/config/initializers/omniauth.rb
Rails.application.config.middleware.use OmniAuth::Builder do
  provider :google_oauth2, ENV['GOOGLE_KEY'], ENV['GOOGLE_SECRET']
end
...
...

And while we are at it, we should also remove the TWITTER_KEY and TWITTER_SECRET environment variables from our production Heroku instance. Since this functionality is currently broken, we won’t be causing any problems by removing the keys now. I’ve also verified we have no current users that authenticate via Twitter, thus it is safe to remove this functionality and the keys.

Removing the production environment variables

So to remove the keys we just use the Heroku CLI.

First we’ll verify that we have these keys set.

Terminal
heroku config:get TWITTER_KEY

Terminal
heroku config:get TWITTER_SECRET

Now we’ll unset the values and confirm they are gone.

Terminal
heroku config:unset TWITTER_KEY
heroku config:unset TWITTER_SECRET

Terminal
heroku config:get TWITTER_KEY
Terminal
heroku config:get TWITTER_SECRET

Done!

Running and updating the tests

Now that we’ve removed our Twitter related code, let’s see where our tests are at.

Terminal
bundle exec rspec spec/

Five failures, all having to do with Twitter authentication… so we need to remove these tests. With the user_spec.rb test we can delete the entire Twitter context section which starts at line 185 and ends at line 215:

/spec/models/user_spec.rb …line 185
context "Twitter" do
  before do
    @provider = "twitter"
    @auth["provider"] = @provider
  end

  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 "#{@uid}.no.email.for.twitter@example.com"
    @user.name.should eq @name
    @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 "#{@uid}.no.email.for.twitter@example.com"
    @user.name.should eq @name
    @user.uid.should eq @uid
    @user.provider.should eq @provider
    @user.should be_active
  end
end

With the authentications_spec we need to remove the check for the Twitter link at line 24:

/spec/requests/authentications_spec.rb …line 22
context "oauth login" do
  it { should have_link('Google') }
  it { should have_link('Twitter') }
end

And also the twitter context section which starts at line 114:

/spec/requests/authentications_spec.rb …line 114
context "twitter" do
  before { visit '/auth/twitter/callback?denied=1234' }

  it "should redirect to the sign in page" do
    current_path.should eq signin_path
  end

  it "should display a message" do
    page.should have_content('Authentication failed, please try again.')
  end
end

After removing these tests, and re-running rspec spec/:

Terminal
bundle exec rspec spec/

We get:

Fantastic, no more Twitter authentication!

A review of our remaining issues and tasks

We are starting to make some good progress, the only issue we have left from the list we compiled in part 1 is:

  • Accounts
    • Change email - mostly works - main functionality works, but the User.Name field is not updated on an email change

And as far as the task list we put together in part 4 goes, we have:

  • Update our email change functionality to update the name field in the user table.
  • Update our sign-up functionality to prevent new sign-ups. As mentionned in part 1, I don’t really want anyone else using the application. We’ll have to think how we want to handle this.
  • Update our footer to display the correct year.

That last item is so minor, let’s get it done and dusted prior to wrapping things up for the day.

We need to make a very minor change to _footer.html.erb.

Before doing so, let’s add a test.

Updating our tests

We already have a test for our footer links:

/spec/requests/static_pages_spec.rb …line 20
it "should have the right links on the footer" do
  visit root_path
  click_link "About"
  page.should have_selector 'title', text: full_title('About')
  click_link "Contact"
  page.should have_selector 'title', text: full_title('Contact')
end

We can just add a new should condition to the existing test.

/spec/requests/static_pages_spec.rb …line 20
it "should have the right links on the footer" do
  visit root_path
  page.should have_content "© #{Time.current.year}"
  click_link "About"
  page.should have_selector 'title', text: full_title('About')
  click_link "Contact"
  page.should have_selector 'title', text: full_title('Contact')
end

If we run the test now:

Terminal
bundle exec rspec spec/requests/static_pages_spec.rb

We get a failure as expected:

So let’s move onto the implementation.

Implementing the change

Our footer currently looks like:

/app/views/layouts/_footer.html.erb
<footer>
  <nav>
    <ul class="nav">
      <li>&copy; 2014</li>
      <li>/</li>
      ...
      ...

It needs to become:

/app/views/layouts/_footer.html.erb
<footer>
  <nav>
    <ul class="nav">
      <li>&copy; <%= Time.current.year %></li>
      <li>/</li>
      ...
      ...

With the above change, when we run our local server and check out the UI, it looks good.

If we re-run our full test suite, everything is passing.

Terminal
bundle exec rspec spec/

Sweet!

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

Terminal
git commit -am "removed twitter authentication and fixed footer"
git checkout 2018-update-and-refactor
git merge fix-email-configuration
git branch -d fix-email-configuration

Summary

So our email configuration issue turned out to have a very simple solution! Likewise removing the Twitter authentication functionality was pretty easy, as was updating the footer code.

Next time, we’ll tackle a slightly more difficult problem when we fix our change email functionality.

Thanks for reading and I hope you enjoyed the post!

Share this post!

Comment on this post!