Returning a 404 if a user doesn't exist

Question

Why would this not generate a 404 response?

def index
  if find_user
    @documents = @client.documents
    respond_to do |format|
      format.html
      format.atom { render layout: false }
    end
  else
    flash[:error] = "#{params[:client_code]} is not a client."
    render 'error', status: '404'
  end
end

def find_user
  @client = User.find_by_client_code(params[:client_code]) if valid_user?
end

def valid_user?
  User.all.each.map(&:client_code).include?(params[:client_code])
end

Like, if the code is incorrect it should return a 404, right? And not an exception? Can't quite get it to work.

EDIT: sorry, here's the error:

An ActionView::MissingTemplate occurred in share#index:
* Parameters : {"controller"=>"share", "action"=>"index", "client_code"=>"ampNDHEDD", "format"=>"atom"}

btw, this code `User.all.each.map(&:client_code)` is terrible. one day or other, it will consume all server memory and crash the server. — Paritosh Piplewar, Aug 04 '14 at 02:08
looks like you have no `index.atom` template, also you can use `User.pluck(:client_code)` on rails > 3.x if it's an activerecord column. — Matenia Rossides, Aug 04 '14 at 02:09

score 2 · Answer 1 · answered Aug 04 '14 at 02:16

If you don't use the valid_user? or the find_user methods elsewhere, they can be removed and you could do the following

def index
  @client = User.find_by_client_code(params[:client_code]) # returns nil or a record
  if @client
    @documents = @client.documents
    respond_to do |format|
      format.html
      format.atom { render layout: false }
    end
  else
    flash[:error] = "#{params[:client_code]} is not a client."
    render status: 404
  end
end

However, your previous comment states you're getting a template error which indicates that you may not have an index.atom template available to render.

Paritosh Piplewar · Answer 2 · 2014-08-04T02:12:42.357

1

do this

def index
  if find_user
    @documents = @client.documents
    respond_to do |format|
      format.html
      format.atom { render layout: false }
    end
  else
    flash[:error] = "#{params[:client_code]} is not a client."
    raise ActionController::RoutingError.new('Not Found')
  end
end

def find_user
  @client = User.find_by_client_code(params[:client_code]) if valid_user?
end

def valid_user?
  User.where(client_code: params[:client_code]).present?
  // User.all.each.map(&:client_code).include?(params[:client_code]) // this is terrible (Read my comment )
end

edited Aug 04 '14 at 02:12

answered Aug 04 '14 at 02:06

Paritosh Piplewar

7,982
5
26
41

I like your idea, and thanks for the advice, but `raise ActionController::RoutingError.new('Not Found')` just returns a 500 error. – t56k Aug 04 '14 at 02:35
in that case, just do `render :file => "#{Rails.root}/public/404", :layout => false, :status => :not_found` – Paritosh Piplewar Aug 04 '14 at 02:41
you can refer more here http://stackoverflow.com/questions/2385799/how-to-redirect-to-a-404-in-rails – Paritosh Piplewar Aug 04 '14 at 02:41

score 1 · Answer 3 · answered Aug 04 '14 at 02:18

First, your valid_user? method is a really bad idea - it loads the entire user database just to see if the code is present... which is the same result as what User.find_by_client_code does, but without loading every record! I'd just nuc the method and the if clause. If there is no matching record, it should return nil, which should take the else path and render the error.

As for why it's not rendering the error... I'm not sure if the atom format has anything to do with it, but when code doesn't branch the way I expect, I always put a Rails.logger.debug ... before the branch I have an issue with, and/or put a bad method in the branch it's supposed to take. That helps narrow it down. :D

Returning a 404 if a user doesn't exist

3 Answers3