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.

Conflicts:

	spec/controllers/api_controller_spec.rb

2 files changed, 96 insertions, 3 deletions

diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb
index 524aa44b7..26950aaf3 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 1f65576b6..f9296e7e1 100644
--- a/spec/controllers/api_controller_spec.rb
+++ b/spec/controllers/api_controller_spec.rb
@@ -260,4 +260,90 @@ describe ApiController, "when using the API" do
         # assigns them and changing assignment to an equality
         # check, which does not really test anything at all.
     end
+    
+    it "should show an Atom feed of new request events" do
+        get :body_request_events,
+            :id => public_bodies(:geraldine_public_body).id,
+            :k => public_bodies(:geraldine_public_body).api_key,
+            :feed_type => "atom"
+        
+        response.should be_success
+        response.should render_template("api/request_events.atom")
+        assigns[:events].size.should > 0
+        assigns[:events].each do |event|
+            event.info_request.public_body.should == public_bodies(:geraldine_public_body)
+            event.outgoing_message.should_not be_nil
+            event.event_type.should satisfy {|x| ['sent', 'followup_sent', 'resent', 'followup_resent'].include?(x)}
+        end
+    end
+
+    it "should show a JSON feed of new request events" do
+        get :body_request_events,
+            :id => public_bodies(:geraldine_public_body).id,
+            :k => public_bodies(:geraldine_public_body).api_key,
+            :feed_type => "json"
+        
+        response.should be_success
+        assigns[:events].size.should > 0
+        assigns[:events].each do |event|
+            event.info_request.public_body.should == public_bodies(:geraldine_public_body)
+            event.outgoing_message.should_not be_nil
+            event.event_type.should satisfy {|x| ['sent', 'followup_sent', 'resent', 'followup_resent'].include?(x)}
+        end
+        
+        assigns[:event_data].size.should == assigns[:events].size
+        assigns[:event_data].each do |event_record|
+            event_record[:event_type].should satisfy {|x| ['sent', 'followup_sent', 'resent', 'followup_resent'].include?(x)}
+        end
+    end
+    
+    it "should honour the since_event_id parameter" do
+        get :body_request_events,
+            :id => public_bodies(:geraldine_public_body).id,
+            :k => public_bodies(:geraldine_public_body).api_key,
+            :feed_type => "json"
+        response.should be_success
+        first_event = assigns[:event_data][0]
+        second_event_id = assigns[:event_data][1][:event_id]
+        
+        get :body_request_events,
+            :id => public_bodies(:geraldine_public_body).id,
+            :k => public_bodies(:geraldine_public_body).api_key,
+            :feed_type => "json",
+            :since_event_id => second_event_id
+        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