API errors should be JSON

The API was returning Rails (HTML) errors for certain error
conditions, which is inconvenient because it makes it difficult
for the client to extract the error message.

This patch changes add_correspondence to return JSON errors
(still with suitable HTTP status codes) for two common exceptional
conditions, and adds tests.

2 files changed, 42 insertions, 3 deletions

diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb
index 718c31e6f..6c98ebeba 100644
--- a/app/controllers/api_controller.rb
+++ b/app/controllers/api_controller.rb
@@ -72,7 +72,12 @@ class ApiController < ApplicationController
     end
     
     def add_correspondence
-        request = InfoRequest.find(params[:id])
+        request = InfoRequest.find_by_id(params[:id])
+        if request.nil?
+            render :json => { "errors" => ["Could not find request #{params[:id]}"] }, :status => 404
+            return
+        end
+        
         json = ActiveSupport::JSON.decode(params[:correspondence_json])
         attachments = params[:attachments]
         
@@ -83,11 +88,13 @@ class ApiController < ApplicationController
         errors = []
         
         if !request.is_external?
-            raise ActiveRecord::RecordNotFound.new("Request #{params[:id]} cannot be updated using the API")
+            render :json => { "errors" => ["Request #{params[:id]} cannot be updated using the API"] }, :status => 500
+            return
         end
         
         if request.public_body_id != @public_body.id
-            raise ActiveRecord::RecordNotFound.new("You do not own request #{params[:id]}")
+            render :json => { "errors" => ["You do not own request #{params[:id]}"] }, :status => 500
+            return
         end
         
         if !["request", "response"].include?(direction)
diff --git a/spec/controllers/api_controller_spec.rb b/spec/controllers/api_controller_spec.rb
index 98751a93a..f9296e7e1 100644
--- a/spec/controllers/api_controller_spec.rb
+++ b/spec/controllers/api_controller_spec.rb
@@ -314,4 +314,36 @@ describe ApiController, "when using the API" do
         response.should be_success
         assigns[:event_data].should == [first_event]
     end
+    
+    it "should return a JSON 404 error for non-existent requests" do
+        request_id = 123459876 # Let's hope this doesn't exist!
+        sent_at = "2012-05-28T12:35:39+01:00"
+        response_body = "Thank you for your request for information, which we are handling in accordance with the Freedom of Information Act 2000. You will receive a response within 20 working days or before the next full moon, whichever is sooner.\n\nYours sincerely,\nJohn Gandermulch,\nExample Council FOI Officer\n"
+        post :add_correspondence,
+            :k => public_bodies(:geraldine_public_body).api_key,
+            :id => request_id,
+            :correspondence_json => {
+                "direction" => "response",
+                "sent_at" => sent_at,
+                "body" => response_body
+            }.to_json
+        response.status.should == "404 Not Found"
+        ActiveSupport::JSON.decode(response.body)["errors"].should == ["Could not find request 123459876"]
+    end
+    
+    it "should return a JSON 500 error if we try to add correspondence to a request we don't own" do
+        request_id = info_requests(:naughty_chicken_request).id
+        sent_at = "2012-05-28T12:35:39+01:00"
+        response_body = "Thank you for your request for information, which we are handling in accordance with the Freedom of Information Act 2000. You will receive a response within 20 working days or before the next full moon, whichever is sooner.\n\nYours sincerely,\nJohn Gandermulch,\nExample Council FOI Officer\n"
+        post :add_correspondence,
+            :k => public_bodies(:geraldine_public_body).api_key,
+            :id => request_id,
+            :correspondence_json => {
+                "direction" => "response",
+                "sent_at" => sent_at,
+                "body" => response_body
+            }.to_json
+        response.status.should == "500 Internal Server Error"
+        ActiveSupport::JSON.decode(response.body)["errors"].should == ["Request #{request_id} cannot be updated using the API"]
+    end
 end