#50 ✓resolved
Kieran P

500 Error for unknown zoom class when should be 404

Reported by Kieran P | August 29th, 2008 @ 10:32 PM

When you go to a url which basket doesn't exist ( http://kete.net.nz/sitea for example ), or an action that doesn't exist ( http://kete.net.nz/site/every/to... for example) you get the appropriate 404's.

But when you go to an existing basket and action but unknown Zoom class (item type), OR an existing basket and zoom class but unknown action, you get an Error 500's when you expect a 404's.

http://kete.net.nz/site/all/movies/

http://kete.net.nz/site/account/...

http://kete.net.nz/about/topics/...

These arn't linked anywhere, but it would still be nice to catch them and display 404's so they get the layout, rather than the empty 500.

Comments and changes to this ticket

  • James
  • James

    James September 5th, 2008 @ 02:00 PM

    • Tag set to 404, 500, accessibility, browse, error, inconsistent, search

    I've taken a look at the branch (see commit at http://github.com/kete/kete/comm... ) and in general it looks good. Here are some notes:

    In SearchController#all (line 75), you added:

    
    # if zoom class isn't valid, @results is nil,
    # so lets rescue with a 404 in this case
    rescue_404 if @results.nil?
    

    Should this not also be added to SearchController#for? There is a possibility this kind of event could take please on searches as well. I.e. /site/search/a/for/b?search_terms=b

    Also, in search_controller.rb on line 897, you've added:

    
    return if @result_sets[@current_class].nil?
    

    What is the purpose of this line in the flow of a search request? Shouldn't this event be handled by the error handling you've added (see above).

    A couple of other things I noticed that could be improved are:

    • The 500 error template has some issues with the rounded corners (see attachment)

    • Errors that aren't explicitly handled right now (e.g. SyntaxError, NoMethodError, RuntimeError, etc) render the 'failsafe' template (see log included below). It would be cool if these were also handled in using the 500 error template used for Zebra errors, etc.

  • James

    James September 5th, 2008 @ 02:00 PM

    Kieran, I've updated my comment above as there were some inaccuracies.

    James

  • Kieran P

    Kieran P September 8th, 2008 @ 11:13 AM

    Should this not also be added to SearchController#for? There is a possibility this kind of event could take please on searches as well. I.e. /site/search/a/for/b?search_terms=b

    Yes, will add it.

    Also, in search_controller.rb on line 897, you've added: return if @result_sets[@current_class].nil? What is the purpose of this line in the flow of a search request? Shouldn't this event be handled by the error handling you've added (see above).

    I'd thought so, but apparently the after_filter still runs even if rendering a 404, so when the slideshow var tries to populate it runs into a nil.[] error or something related. That line fixes the issue.

    A couple of other things I noticed that could be improved are: * The 500 error template has some issues with the rounded corners (see attachment)

    Hmm, it happens occasionally. I'll take a look.

    • Errors that aren't explicitly handled right now (e.g. SyntaxError, NoMethodError, RuntimeError, etc) render the 'failsafe' template (see log included below). It would be cool if these were also handled in using the 500 error template used for Zebra errors, etc.

    I didn't add these because when it comes down to it, a 500 suffices (internal server error) for them. It should be easy to add them if needed, but IMHO, its not worth it (500 aren't seen that often).

  • James

    James September 8th, 2008 @ 11:25 AM

    Also, in search_controller.rb on line 897, you've added: return if @result_sets[@current_class].nil? What is the purpose of this line in the flow of a search request? Shouldn't this event be handled by the error handling you've added (see above).

    I'd thought so, but apparently the after_filter still runs even if rendering a 404, so when the slideshow var tries to populate it runs into a nil.[] error or something related. That line fixes the issue.

    Ok, that makes sense. You might want to add a quick comment in the code so it's obvious to anyone else in the future.

    • Errors that aren't explicitly handled right now (e.g. SyntaxError, NoMethodError, RuntimeError, etc) render the 'failsafe' template (see log included below). It would be cool if these were also handled in using the 500 error template used for Zebra errors, etc.

    I didn't add these because when it comes down to it, a 500 suffices (internal server error) for them. It should be easy to add them if needed, but IMHO, its not worth it (500 aren't seen that often).

    It's more a matter of having the rescue_action_in_public default to the 500 error template with some nice text if it doesn't already capture the error. Right now it doesn't render anything unless the error is specified, which means it ends up rendering the fail-safe template.

    The advantage of rendering the error_500 template would be that it fits with the Kete theme and therefore is more consistent in terms of interface. The fail-safe template takes extra manual work to make look like the rest of the Kete template, right?

    I'm not 100% convinced that this work should be done, but more that it should be considered.

    Cheers, James

  • Kieran P

    Kieran P September 8th, 2008 @ 12:08 PM

    You might want to add a quick comment in the code so it's obvious to anyone else in the future.

    Done.

    It's more a matter of having the rescue_action_in_public default to the 500 error template with some nice text if it doesn't already capture the error. Right now it doesn't render anything unless the error is specified, which means it ends up rendering the fail-safe template.

    Ah, I see what you mean now. Easy enough. Have completed and pushed all changes mentioned above to the branch bugfix_lh_num_50_missed_404_exceptions

    The only time failsafe should be shown is when the rescue_500 causes a 500 (lol). Basically, whenever something errors before the before_filter in application.rb for load_related_themes. Any errors before will stop it assigning the theme the application layout needs, causing a 500 (which should be rare).

    Any last cases not currently being captured?

  • James

    James September 8th, 2008 @ 01:12 PM

    Looks good.

    Please merge this in.

    Cheers, James

  • Kieran P

    Kieran P September 8th, 2008 @ 01:29 PM

    • State changed from “new” to “resolved”

    Merged to master. Resolving.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

Kete was developed by Horowhenua Library Trust and Katipo Communications Ltd. to build a digital library of Horowhenua material.

People watching this ticket

Attachments

Pages