Additional changes omitted from commit 9d8388c03d0faeaca29d233a340c58bd65f28a97 (distinguish 404s and 500s), fixes #161.

6 files changed, 42 insertions, 31 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index cb64cb922..49b7fd7f1 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -8,6 +8,7 @@
 #
 # $Id: application.rb,v 1.59 2009-09-17 13:01:56 francis Exp $
 
+require 'open-uri'
 
 class ApplicationController < ActionController::Base
     # Standard headers, footers and navigation for whole site
@@ -101,11 +102,17 @@ class ApplicationController < ActionController::Base
         # Make sure expiry time for session is set (before_filters are
         # otherwise missed by this override) 
         session_remember_me
-
+        case exception
+        when ActiveRecord::RecordNotFound, ActionController::UnknownAction, ActionController::RoutingError
+            @status = 404
+        else
+            @status = 500
+        end
         # Display user appropriate error message
         @exception_backtrace = exception.backtrace.join("\n")
         @exception_class = exception.class.to_s
-        render :template => "general/exception_caught.rhtml", :status => 404
+        @exception_message = exception.message
+        render :template => "general/exception_caught.rhtml", :status => @status
     end
 
     # For development sites.
diff --git a/app/controllers/public_body_controller.rb b/app/controllers/public_body_controller.rb
index 830f37855..ce70e1ab0 100644
--- a/app/controllers/public_body_controller.rb
+++ b/app/controllers/public_body_controller.rb
@@ -19,7 +19,7 @@ class PublicBodyController < ApplicationController
         @locale = self.locale_from_params()
         PublicBody.with_locale(@locale) do 
             @public_body = PublicBody.find_by_url_name_with_historic(params[:url_name])
-            raise "None found" if @public_body.nil? # XXX proper 404
+            raise ActiveRecord::RecordNotFound.new("None found") if @public_body.nil? # XXX proper 404
             if @public_body.url_name.nil?
                 redirect_to :back
                 return
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index 79b8e4816..a76418e4e 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -37,7 +37,7 @@ class RequestController < ApplicationController
             # Look up by new style text names 
             @info_request = InfoRequest.find_by_url_title(params[:url_title])
             if @info_request.nil?
-                raise "Request not found" 
+                raise ActiveRecord::RecordNotFound.new("Request not found")
             end
             set_last_request(@info_request)
 
@@ -189,7 +189,7 @@ class RequestController < ApplicationController
                     params[:info_request][:public_body_id] = params[:url_name]
                 else
                     public_body = PublicBody.find_by_url_name_with_historic(params[:url_name])
-                    raise "None found" if public_body.nil? # XXX proper 404
+                    raise ActiveRecord::RecordNotFound.new("None found") if public_body.nil? # XXX proper 404
                     params[:info_request][:public_body_id] = public_body.id
                 end
             elsif params[:public_body_id]
@@ -672,10 +672,10 @@ class RequestController < ApplicationController
         raise "internal error, pre-auth filter should have caught this" if !@info_request.user_can_view?(authenticated_user)
   
         @attachment = IncomingMessage.get_attachment_by_url_part_number(@incoming_message.get_attachments_for_display, @part_number)
-        raise "attachment not found part number " + @part_number.to_s + " incoming_message " + @incoming_message.id.to_s if @attachment.nil?
+        raise ActiveRecord::RecordNotFound.new("attachment not found part number " + @part_number.to_s + " incoming_message " + @incoming_message.id.to_s) if @attachment.nil?
 
         # check filename in URL matches that in database (use a censor rule if you want to change a filename)
-        raise "please use same filename as original file has, display: '" + @attachment.display_filename + "' old_display: '" + @attachment.old_display_filename + "' original: '" + @original_filename + "'" if @attachment.display_filename != @original_filename && @attachment.old_display_filename != @original_filename
+        raise ActiveRecord::RecordNotFound.new("please use same filename as original file has, display: '" + @attachment.display_filename + "' old_display: '" + @attachment.old_display_filename + "' original: '" + @original_filename + "'") if @attachment.display_filename != @original_filename && @attachment.old_display_filename != @original_filename
 
         @attachment_url = get_attachment_url(:id => @incoming_message.info_request_id,
                 :incoming_message_id => @incoming_message.id, :part => @part_number,
diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb
index d3c42c7f1..8e4fb29ef 100644
--- a/app/controllers/user_controller.rb
+++ b/app/controllers/user_controller.rb
@@ -24,7 +24,7 @@ class UserController < ApplicationController
 
         @display_user = User.find(:first, :conditions => [ "url_name = ? and email_confirmed = ?", params[:url_name], true ])
         if not @display_user
-            raise "user not found, url_name=" + params[:url_name]
+            raise ActiveRecord::RecordNotFound.new("user not found, url_name=" + params[:url_name])
         end
         @same_name_users = User.find(:all, :conditions => [ "name ilike ? and email_confirmed = ? and id <> ?", @display_user.name, true, @display_user.id ], :order => "created_at")
 
@@ -133,7 +133,6 @@ class UserController < ApplicationController
                 # New unconfirmed user
                 @user_signup.email_confirmed = false
                 @user_signup.save!
-
                 send_confirmation_mail @user_signup
                 return
             end
@@ -454,7 +453,7 @@ class UserController < ApplicationController
     def get_profile_photo
         @display_user = User.find(:first, :conditions => [ "url_name = ? and email_confirmed = ?", params[:url_name], true ])
         if !@display_user
-            raise "user not found, url_name=" + params[:url_name]
+            raise ActiveRecord::RecordNotFound.new("user not found, url_name=" + params[:url_name])
         end
         if !@display_user.profile_photo
             raise "user has no profile photo, url_name=" + params[:url_name]
diff --git a/app/views/general/exception_caught.rhtml b/app/views/general/exception_caught.rhtml
index ca36b592b..b266b53a1 100644
--- a/app/views/general/exception_caught.rhtml
+++ b/app/views/general/exception_caught.rhtml
@@ -1,17 +1,24 @@
-<h1><%= _("Sorry, we couldn't find that page") %></h1>
+<div id="error-page">
+ <% if @status == 404 %>
+  <h1><%= _("Sorry, we couldn't find that page") %></h1>
 
-<p><%= _("The page either doesn't exist, or is broken. Things you can try now:")%></p>
+  <p><%= _("The page doesn't exist. Things you can try now:")%></p>
 
-<ul>
-<li><%= _("Check for mistakes if you typed or copied the address.")%></li>
-<li><%= _("Search the site to find what you were looking for.")%>
-    <% form_tag({:controller => "general", :action => "search_redirect"}, {:id => "search_form"}) do %>
-           <%= text_field_tag 'query', params[:query], { :size => 30 } %>
-           <%= submit_tag _("Search") %>
-    <% end %>
-</li>
-<li><%= _('<a href="%s">Contact us</a> to tell us about the problem</li>') % [help_contact_path] %>
-<li><%= _('Go to our <a href="%s">front page</a></li>') % ["/"] %>
-</ul>
+  <ul>
+  <li><%= _("Check for mistakes if you typed or copied the address.")%></li>
+  <li><%= _("Search the site to find what you were looking for.")%>
+      <% form_tag({:controller => "general", :action => "search_redirect"}, {:id => "search_form"}) do %>
+             <%= text_field_tag 'query', params[:query], { :size => 30 } %>
+             <%= submit_tag _("Search") %>
+      <% end %>
+  </li>
+  </ul>  
+ <% else %>
+  <h1><%= _("Sorry, there was a problem processing this page") %></h1>
+  <p><%= _('You have found a bug.  Please <a href="{{contact_url}}">contact us</a> to tell us about the problem', :contact_url => help_contact_path) %></p>
 
-<p id="error_technical_details"><%= _("<strong>Technical details:</strong>")%> <%=@exception_class ? @exception_class : _("Unknown")%></p>
+ <% end %>
+  <h2><%= _('Technical details') %></h2>
+  <p><strong><%=@exception_class ? @exception_class : _("Unknown")%></strong></p>
+  <p><strong><%=@exception_message %></strong></p>
+</div>
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index 2b153ac7a..f3084af12 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -161,7 +161,7 @@ describe RequestController, "when showing one request" do
             lambda {
                 get :get_attachment, :incoming_message_id => ir.incoming_messages[1].id, :id => ir.id, :part => 2, 
                     :file_name => ['http://trying.to.hack']
-            }.should raise_error(RuntimeError)
+            }.should raise_error(ActiveRecord::RecordNotFound)
         end
 
         it "should censor attachments downloaded as binary" do
@@ -747,18 +747,16 @@ describe RequestController, "when classifying an information request" do
             response.should redirect_to(:controller => 'help', :action => 'unhappy', :url_title => @dog_request.url_title)
         end
 
-        describe "when using custom statuses from the theme" do
+        it "knows about extended states" do
             InfoRequest.send(:require, File.expand_path(File.join(File.dirname(__FILE__), '..', 'models', 'customstates')))
             InfoRequest.send(:include, InfoRequestCustomStates)
             InfoRequest.class_eval('@@custom_states_loaded = true')
             RequestController.send(:require, File.expand_path(File.join(File.dirname(__FILE__), '..', 'models', 'customstates')))
             RequestController.send(:include, RequestControllerCustomStates)
             RequestController.class_eval('@@custom_states_loaded = true')
-            it "knows about extended states" do
-                Time.stub!(:now).and_return(Time.utc(2007, 11, 10, 00, 01)) 
-                post_status('deadline_extended')
-                flash[:notice].should == 'Authority has requested extension of the deadline.'
-            end
+            Time.stub!(:now).and_return(Time.utc(2007, 11, 10, 00, 01)) 
+            post_status('deadline_extended')
+            flash[:notice].should == 'Authority has requested extension of the deadline.'
         end
     end