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.