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:


  def mailboxer_email(object)

  def self.search(search)
    where("email ILIKE ?", "%#{search}%") 

  after_create :build_profile


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/

Users controller

class UsersController < ApplicationController
  def index
    @users = User.all
    if params[:search]
      @users = User.search(params[:search]).order("created_at DESC")
      @users = User.all.order('created_at DESC')

  def show
    @user = User.find(params[:id])

  def watchlist
    @watched_listings = current_user.watched_listings.all

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])

  def edit
    @profile = Profile.find_by user_id: current_user.id

  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'
      flash[:notices] = ["Your profile could not be updated"]
      render 'edit'


  def profile_params
    params.require(:profile).permit(:city, :country, :avatar)

  def correct_user
    @profile = Profile.find_by(user_id: params[:user_id])
    redirect_to(root_path) unless current_user?(@profile)

I have also defined:

Profiles helper

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

Within my Application Controller:

class ApplicationController < ActionController::Base
  protect_from_forgery with: :exception

  def after_sign_in_path_for(resource)

  def after_sign_up_path_for(resource)

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.


1 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

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.


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.

  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – Ben Hawker
    Dec 2, 2015 at 13:58
  • \$\begingroup\$ I'm glad you found it useful! Cheers :) \$\endgroup\$ Dec 2, 2015 at 14:18
  • \$\begingroup\$ 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. \$\endgroup\$ Dec 2, 2015 at 17:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.