1
\$\begingroup\$

I have a site where user accounts can be created in two ways: by new users (sign up) and by admin users. Since the creation is handled differently in each case, I decided to split the logic into two separate functions. I have a working implementation but I'm pretty new to Rails so I don't know if it is robust and secure. I am looking for feedback on how to improve it.

users_controller.rb

before_action :signed_in_user, only: [:home, :edit, :update, :destroy]
before_action :correct_user, only: [:edit, :update]
before_action :admin_user, only: [:create_by_admin, :destroy]

.
.
.

def new
    if !signed_in?
        @user = User.new
        @user.confirmation_code = User.new_confirmation_code
    elsif signed_in? && current_user.admin?
        @user = User.new
        render 'new_by_admin' # basically a new.html.erb but with a dropdown for the admin setting
    else
        redirect_to root_url
    end
end

def create
    if signed_in? && !current_user.admin?
        redirect_to root_url
    elsif signed_in? && current_user.admin?
        create_by_admin
    else
        create_by_user
    end
end

def create_by_user
    @user = User.new(user_params)
    if @user.save
        UserMailer.confirmation_email(@user).deliver
        flash[:success] = "Great! Now check your email for activation instructions."
        redirect_to root_url
    else
        render 'new'
    end
end

def create_by_admin
    @user = User.new(admin_params)
    if @user.save
        @user.update_attributes(confirmed: true)
        flash[:success] = "User for " + @user.name + " created."
        redirect_to users_path
    else
        render 'new_by_admin'
    end
end

.
.
.

private
    def user_params
        params.require(:user).permit(:name, :email, :password, :password_confirmation)
    end

    def admin_params
        params.require(:user).permit(:name, :email, :password, :password_confirmation, :admin)
    end

Essentially, all create requests are routed to the create action, and from there sent to the appropriate method (depending on whether the requesting user is a new user or an existing user who is an admin). The differences between the methods is that new users receive an email to activate their accounts upon signup and cannot set themselves as admin, whereas admin users create accounts that are already confirmed and can set them as admin.

Is this a silly way to do it? Should I create an admin_controller that handles all administrative requests?

\$\endgroup\$
  • \$\begingroup\$ You have a bug in your #create method: The elsif branch will never be reached. If you're signed in, you'll trigger the first branch immediately; if you're not signed in, it'll jump to the else branch. It'll never check if you're an admin. \$\endgroup\$ – Flambino Apr 22 '14 at 19:00
  • \$\begingroup\$ Thanks, I actually fixed that last night after posting it - I'll make the edit. :) \$\endgroup\$ – enraged camel Apr 22 '14 at 19:51
2
\$\begingroup\$

It's fine to create 2 methods to break up the logic a bit - however, there are other things I'd look into before tackling the user vs admin stuff:

Firstly: Ruby style prescribes 2-spaces indentation - not tabs, not 4 spaces.

Second, if you're going to use them, make those create_by* methods private. Don't mix 'em with the regular controller actions.

Third: String interpolation:

flash[:success] = "User for #{@user.name} created."

But success is an uncommon flash key. You can use the default notice message type directly in your redirect

redirect_to root_url, notice: "User for #{@user.name} created."

If you absolutely want to use success, you can use add_flash_types to add it, and use redirect_to ..., success: '...'.

And then you have this:

render 'new_by_admin' # basically a new.html.erb but with a dropdown for the admin setting

Don't use two templates - that's a giant hassle to maintain. Use a conditional in the view itself to show or not show the dropdown. I'd also add an admin? helper like this

# in application_controller.rb
def admin?
  current_user.try(:admin?)
end
helper_method :admin?

(I'm assuming that current_user is nil if you're not signed in).

So now you can check for admins in one go rather than both check signed_in? and then check current_user.admin?

In the new view you get this:

<% if admin? %>
  your dropdown
<% end %>

And you don't need to 2 templates. Your if..elsif..else in #create can also become the more readable:

if admin?
  ...
elsif signed_in? # regular user
  ...
else
  ...
end

Your code also makes me wonder how you handle passwords. If an admin creates a user, I imagine you'll want to send login instructions and password to the new user.

Right now, it looks like the admin picks a password for other people, but you should already have a "reset password" method (for users who've forgotten their password) that will create a random password and email it to the user. It shouldn't be the admin's job to make up passwords.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your comment. Your suggestions make sense - I implemented them and they work great. To answer your questions... 1. I have a flash_class method inside application_helpers.rb that assigns a class to the flash div, which Bootstrap then uses to style it appropriately. In this case, :success equates to 'alert alert-success'. 2. passwords: currently there isn't a mechanism to reset them, although that's what I will work on next. Admins won't always create user accounts - that's just a fail-safe in case the regular method (sign-up) fails. :) \$\endgroup\$ – enraged camel Apr 22 '14 at 21:25
  • 1
    \$\begingroup\$ @enragedcamel I figured the success flash key was to do with Bootstrap. All the more reason to use the add_flash_types trick, and make success an "official" flash type (or use the helper to remap notice to the alert-success class). \$\endgroup\$ – Flambino Apr 23 '14 at 9:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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