diff options
| -rw-r--r-- | app/controllers/api_controller.rb | 88 | ||||
| -rw-r--r-- | app/models/info_request.rb | 10 | ||||
| -rw-r--r-- | spec/controllers/api_controller_spec.rb | 22 |
3 files changed, 59 insertions, 61 deletions
diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 355584288..6c0788d49 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -1,5 +1,9 @@ class ApiController < ApplicationController before_filter :check_api_key + before_filter :check_external_request, + :only => [:add_correspondence, :update_state] + before_filter :check_request_ownership, + :only => [:add_correspondence, :update_state] def show_request @request = InfoRequest.find(params[:id]) @@ -77,12 +81,6 @@ class ApiController < ApplicationController end def add_correspondence - 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] @@ -93,16 +91,6 @@ class ApiController < ApplicationController errors = [] - if !request.is_external? - render :json => { "errors" => ["Request #{params[:id]} cannot be updated using the API"] }, :status => 500 - return - end - - if request.public_body_id != @public_body.id - render :json => { "errors" => ["You do not own request #{params[:id]}"] }, :status => 500 - return - end - if !["request", "response"].include?(direction) errors << "The direction parameter must be 'request' or 'response'" end @@ -126,16 +114,16 @@ class ApiController < ApplicationController # In the 'request' direction, i.e. what we (Alaveteli) regard as outgoing outgoing_message = OutgoingMessage.new( - :info_request => request, + :info_request => @request, :status => 'ready', :message_type => 'followup', :body => body, :last_sent_at => sent_at, :what_doing => 'normal_sort' ) - request.outgoing_messages << outgoing_message - request.save! - request.log_event("followup_sent", + @request.outgoing_messages << outgoing_message + @request.save! + @request.log_event("followup_sent", :api => true, :email => nil, :outgoing_message_id => outgoing_message.id, @@ -155,50 +143,33 @@ class ApiController < ApplicationController ) end - mail = RequestMailer.external_response(request, body, sent_at, attachment_hashes) + mail = RequestMailer.external_response(@request, body, sent_at, attachment_hashes) - request.receive(mail, mail.encoded, true) + @request.receive(mail, mail.encoded, true) if new_state && InfoRequest.enumerate_states.include?(new_state) - request.set_described_state(new_state) + @request.set_described_state(new_state) end end render :json => { - 'url' => make_url("request", request.url_title), + 'url' => make_url("request", @request.url_title), } end def update_state - request = InfoRequest.find_by_id(params[:id]) - if request.nil? - render :json => { - "errors" => ["Could not find request #{params[:id]}"] - }, - :status => 404 - return - end new_state = params["state"] errors = [] - if !request.is_external? - render :json => { - "errors" => ["Request #{params[:id]} cannot be updated using the API"] - }, - :status => 500 - return - end - - if request.public_body_id != @public_body.id - render :json => { - "errors" => ["You do not own request #{params[:id]}"] - }, - :status => 500 - return - end - - if InfoRequest.enumerate_states.include?(new_state) - request.set_described_state(new_state) + if InfoRequest.allowed_incoming_states.include?(new_state) + ActiveRecord::Base.transaction do + event = @request.log_event("status_update", + { :script => "#{@public_body.name} on behalf of requester via API", + :old_described_state => @request.described_state, + :described_state => new_state, + }) + @request.set_described_state(new_state) + end else render :json => { "errors" => ["'#{new_state}' is not a valid request state" ] @@ -208,7 +179,7 @@ class ApiController < ApplicationController end render :json => { - 'url' => make_url("request", request.url_title), + 'url' => make_url("request", @request.url_title), } end @@ -295,6 +266,21 @@ class ApiController < ApplicationController raise PermissionDenied if @public_body.nil? end + def check_external_request + @request = InfoRequest.find_by_id(params[:id]) + if @request.nil? + render :json => { "errors" => ["Could not find request #{params[:id]}"] }, :status => 404 + elsif !@request.is_external? + render :json => { "errors" => ["Request #{params[:id]} cannot be updated using the API"] }, :status => 403 + end + end + + def check_request_ownership + if @request.public_body_id != @public_body.id + render :json => { "errors" => ["You do not own request #{params[:id]}"] }, :status => 403 + end + end + private def make_url(*args) "http://" + AlaveteliConfiguration::domain + "/" + args.join("/") diff --git a/app/models/info_request.rb b/app/models/info_request.rb index a2112a210..aed651ad3 100644 --- a/app/models/info_request.rb +++ b/app/models/info_request.rb @@ -115,6 +115,16 @@ class InfoRequest < ActiveRecord::Base states end + # Subset of states accepted via the API + def self.allowed_incoming_states + [ + 'waiting_response', + 'rejected', + 'successful', + 'partially_successful' + ] + end + # Possible reasons that a request could be reported for administrator attention def report_reasons [_("Contains defamatory material"), diff --git a/spec/controllers/api_controller_spec.rb b/spec/controllers/api_controller_spec.rb index 07c2ca6e0..b6023089a 100644 --- a/spec/controllers/api_controller_spec.rb +++ b/spec/controllers/api_controller_spec.rb @@ -225,7 +225,7 @@ describe ApiController, "when using the API" do "body" => "xxx" }.to_json - response.status.should == 500 + response.status.should == 403 ActiveSupport::JSON.decode(response.body)["errors"].should == [ "Request #{request_id} cannot be updated using the API"] @@ -247,7 +247,7 @@ describe ApiController, "when using the API" do "body" => "xxx" }.to_json - response.status.should == 500 + response.status.should == 403 ActiveSupport::JSON.decode(response.body)["errors"].should == [ "You do not own request #{request_id}"] @@ -256,7 +256,8 @@ describe ApiController, "when using the API" do end it "should return a JSON 404 error for non-existent requests" do - request_id = 123459876 # Let's hope this doesn't exist! + request_id = "123459876" + InfoRequest.stub(:find_by_id).with(request_id).and_return(nil) 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, @@ -271,7 +272,7 @@ describe ApiController, "when using the API" do 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 + it "should return a JSON 403 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" @@ -283,7 +284,7 @@ describe ApiController, "when using the API" do "sent_at" => sent_at, "body" => response_body }.to_json - response.status.should == 500 + response.status.should == 403 ActiveSupport::JSON.decode(response.body)["errors"].should == ["Request #{request_id} cannot be updated using the API"] end @@ -360,11 +361,11 @@ describe ApiController, "when using the API" do post :update_state, :k => public_bodies(:geraldine_public_body).api_key, :id => request_id, - :state => "not_held" + :state => "partially_successful" # It should have updated the status request = InfoRequest.find_by_id(request_id) - request.described_state.should == "not_held" + request.described_state.should == "partially_successful" end it "should return a JSON 500 error if an invalid state is sent" do @@ -391,7 +392,8 @@ describe ApiController, "when using the API" do end it "should return a JSON 404 error for non-existent requests" do - request_id = 123459876 # Let's hope this doesn't exist! + request_id = "123459876" + InfoRequest.stub(:find_by_id).with(request_id).and_return(nil) post :update_state, :k => public_bodies(:geraldine_public_body).api_key, @@ -402,7 +404,7 @@ describe ApiController, "when using the API" do 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 + it "should return a JSON 403 error if we try to add correspondence to a request we don't own" do request_id = info_requests(:naughty_chicken_request).id post :update_state, @@ -410,7 +412,7 @@ describe ApiController, "when using the API" do :id => request_id, :state => "successful" - response.status.should == 500 + response.status.should == 403 ActiveSupport::JSON.decode(response.body)["errors"].should == ["Request #{request_id} cannot be updated using the API"] end end |