Managing users and profiles in Rails

Question

I am building a Rails marketplace application using TDD. I would like to get advice on the way in which I have built the User associated Profile associations and way in which these are then handled in the controllers.

User model

class User < ActiveRecord::Base
 has_one :profile, dependent: :destroy
 has_many :listings, dependent: :destroy
 has_many :watches, dependent: :destroy
 has_many :watched_listings, -> { uniq }, :through => :watches, dependent: :destroy
 # Include default devise modules. Others available are:
 acts_as_messageable
 def mailboxer_email(object)
 email
 end
 def self.search(search)
 where("email ILIKE ?", "%#{search}%") 
 end
 after_create :build_profile
end

Profile model

class Profile < ActiveRecord::Base
 belongs_to :user
 has_attached_file :avatar, :styles => { :medium => "300x300>", :thumb => "100x100>" }, :default_url => "/images/:style/missing.png"
 validates_attachment_content_type :avatar, :content_type => /\Aimage\/.*\Z/
end

Users controller

class UsersController < ApplicationController
 def index
 @users = User.all
 if params[:search]
 @users = User.search(params[:search]).order("created_at DESC")
 else
 @users = User.all.order('created_at DESC')
 end
 end
 def show
 @user = User.find(params[:id])
 end
 def watchlist
 @watched_listings = current_user.watched_listings.all
 end
end

Profiles controller

class ProfilesController < ApplicationController
 before_filter :authenticate_user!, :only [:edit, :update]
 before_filter :correct_user, :only [:edit, :update]
 def show
 @profile = Profile.find_by(user_id: params[:user_id])
 end
 def edit
 @profile = Profile.find_by user_id: current_user.id
 end
 def update
 @profile = Profile.find_by user_id: current_user.id
 if @profile.update(profile_params)
 flash[:notices] = ["Your profile was successfully updated"]
 render 'show'
 else
 flash[:notices] = ["Your profile could not be updated"]
 render 'edit'
 end
 end
 private
 def profile_params
 params.require(:profile).permit(:city, :country, :avatar)
 end
 def correct_user
 @profile = Profile.find_by(user_id: params[:user_id])
 redirect_to(root_path) unless current_user?(@profile)
 end
end

I have also defined:

Profiles helper

module ProfilesHelper
 def current_user?(user)
 user == current_user
 end
end

Within my Application Controller:

class ApplicationController < ActionController::Base
 protect_from_forgery with: :exception
 def after_sign_in_path_for(resource)
 edit_user_profile_path(resource)
 end
 def after_sign_up_path_for(resource)
 edit_user_profile_path(resource)
 end
end

I'm very keen to get any feedback on:

• If the use of the after_create :build_profile is appropriate. Does this violate SRP principles by making the user model responsible for the creation of a profile?

• The best way to validate the correct user in the profile controller - to enforce whether a user can edit a profile or not.

• Mainly whether it is required that I be finding profiles by Profile.find_by(user_id: params[:user_id]) rather than by a Profile ID.

• Any other improvements to the code!

I have RSpec and Capybara tests to back this here.

Answer 1

Here's my thoughts.

Profile Model

I'm not sure that the separate profile is pulling enough weight to justify its existence. It seems that a lot of this code could be avoided by collapsing it into the User model. I appreciate that the User and Profile can be two separate entities but it looks like it'll be much easier to keep them in one model until they grow too large to be viable, or there comes a time when a Profile has a separate lifecycle from a User.

Which then avoids your question around SRP altogether :)

But if we were to keep them separate, I don't think it would be a practical violation of SRP because your application likely depends on a profile always existing for a user. So having asserted that User should always have a Profile, you are then faced with the choice of leaving the creation in the hands of the controller, or the model. I favour leaving it in the hands of the model because the model is responsible for modelling the relationships between different business objects.

Finding the Profile and Validating Access

You could replace the find methods with a before_action that uses the user's relationship with the profile like so:

before_action :get_profile, only: [:edit, :update]
def get_profile
 @profile = current_user.profile
end

This will both reduce the amount of code required to select the profile, and ensure that the user can only access their profile to edit/update it.

However, at that point it doesn't make sense to have the profile edit and update actions available with a user ID. They're essentially now singular resources and should be handled accordingly in your routes.

You ask whether find by user_id or not is a good choice. It depends on whether the person accessing that route will be accessing it from the context of its User, or the Profile owner.

current_user?

I don't believe this code will work. You are passing it only Profile objects, and comparing it to the current_user method which returns a User object (or null if not logged in, if memory serves). This should always return false. You either need to call profile.user before passing it to the method, or have that method make the call.

• Ben Hawker: Thanks Richard for taking the time to write this feedback so clearly. I too had an uneasy feeling about the user profile association - going to refactor this back into the user model this weekend and try to tidy it up. Your point about wrapping the find methods with a before action will go along way to cleaning up my controllers in future Rails project.
• Rich Seviora: I'm glad you found it useful! Cheers :)
• Rich Seviora: Also, listen to your gut. Every time I've ignored the uneasy feeling I've had to come back to it later at much higher cost. It's really just your subconscious trying to avoid work it doesn't want right now.