Merge remote-tracking branch 'origin/release/0.6.4' into wdtk

Conflicts:
  app/views/request/show.rhtml
  spec/models/info_request_spec.rb

1 files changed, 524 insertions, 288 deletions

diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index 8cadbd238..0ff795029 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -114,6 +114,7 @@ describe RequestController, "when listing recent requests" do
         assigns[:list_results].size.should == 25
         assigns[:show_no_more_than].should == RequestController::MAX_RESULTS
     end
+
     it "should return 404 for pages we don't want to serve up" do
         xap_results = mock_model(ActsAsXapian::Search,
                    :results => (1..25).to_a.map { |m| { :model => m } },
@@ -123,6 +124,12 @@ describe RequestController, "when listing recent requests" do
         }.should raise_error(ActiveRecord::RecordNotFound)
     end
 
+    it 'should not raise an error for a page param of less than zero, but should treat it as
+        a param of 1' do
+        lambda{ get :list, :view => 'all', :page => "-1" }.should_not raise_error
+        assigns[:page].should == 1
+    end
+
 end
 
 describe RequestController, "when changing things that appear on the request page" do
@@ -226,8 +233,177 @@ describe RequestController, "when showing one request" do
         response.should redirect_to(:action => 'show', :url_title => info_requests(:naughty_chicken_request).url_title)
     end
 
+    it 'should show actions the request owner can take' do
+        get :show, :url_title => 'why_do_you_have_such_a_fancy_dog'
+        response.should have_tag('div#owner_actions')
+    end
+
+    describe 'when the request is being viewed by an admin' do
+
+        describe 'if the request is awaiting description' do
+
+            before do
+                dog_request = info_requests(:fancy_dog_request)
+                dog_request.awaiting_description = true
+                dog_request.save!
+            end
+
+            it 'should show the describe state form' do
+                get :show, { :url_title => 'why_do_you_have_such_a_fancy_dog' },
+                           { :user_id => users(:admin_user).id }
+                response.should have_tag('div.describe_state_form')
+            end
+
+            it 'should ask the user to use the describe state from' do
+                get :show, { :url_title => 'why_do_you_have_such_a_fancy_dog' },
+                           { :user_id => users(:admin_user).id }
+                response.should have_tag('p#request_status', :text => /answer the question above/)
+            end
+
+        end
+
+        describe 'if the request is waiting for a response and very overdue' do
+
+            before do
+                dog_request = info_requests(:fancy_dog_request)
+                dog_request.awaiting_description = false
+                dog_request.described_state = 'waiting_response'
+                dog_request.save!
+                dog_request.calculate_status.should == 'waiting_response_very_overdue'
+            end
+
+            it 'should give a link to requesting an internal review' do
+                get :show, { :url_title => 'why_do_you_have_such_a_fancy_dog' },
+                           { :user_id => users(:admin_user).id }
+                response.should have_tag('p#request_status', :text =>/requesting an internal review/)
+            end
+
+        end
+
+        describe 'if the request is waiting clarification' do
+
+            before do
+                dog_request = info_requests(:fancy_dog_request)
+                dog_request.awaiting_description = false
+                dog_request.described_state = 'waiting_clarification'
+                dog_request.save!
+                dog_request.calculate_status.should == 'waiting_clarification'
+            end
+
+            it 'should give a link to make a followup' do
+                get :show, { :url_title => 'why_do_you_have_such_a_fancy_dog' },
+                           { :user_id => users(:admin_user).id }
+                response.should have_tag('p#request_status a', :text =>/send a follow up message/)
+            end
+        end
+
+    end
+
+    describe 'when showing an external request' do
+
+        describe 'when viewing with no logged in user' do
+
+            it 'should be successful' do
+                get :show, { :url_title => 'balalas' }, { :user_id => nil }
+                response.should be_success
+            end
+
+            it 'should not display actions the request owner can take' do
+                get :show, :url_title => 'balalas'
+                response.should_not have_tag('div#owner_actions')
+            end
+
+        end
+
+        describe 'when the request is being viewed by an admin' do
+
+            def make_request
+                get :show, { :url_title => 'balalas' }, { :user_id => users(:admin_user).id }
+            end
+
+            it 'should be successful' do
+                make_request
+                response.should be_success
+            end
+
+            describe 'if the request is awaiting description' do
+
+                before do
+                    external_request = info_requests(:external_request)
+                    external_request.awaiting_description = true
+                    external_request.save!
+                end
+
+                it 'should not show the describe state form' do
+                    make_request
+                    response.should_not have_tag('div.describe_state_form')
+                end
+
+                it 'should not ask the user to use the describe state form' do
+                    make_request
+                    response.should_not have_tag('p#request_status', :text => /answer the question above/)
+                end
+
+            end
+
+            describe 'if the request is waiting for a response and very overdue' do
+
+                before do
+                    external_request = info_requests(:external_request)
+                    external_request.awaiting_description = false
+                    external_request.described_state = 'waiting_response'
+                    external_request.save!
+                    external_request.calculate_status.should == 'waiting_response_very_overdue'
+                end
+
+                it 'should not give a link to requesting an internal review' do
+                    make_request
+                    response.should_not have_tag('p#request_status', :text =>/requesting an internal review/)
+                end
+            end
+
+            describe 'if the request is waiting clarification' do
+
+                before do
+                    external_request = info_requests(:external_request)
+                    external_request.awaiting_description = false
+                    external_request.described_state = 'waiting_clarification'
+                    external_request.save!
+                    external_request.calculate_status.should == 'waiting_clarification'
+                end
+
+                it 'should not give a link to make a followup' do
+                    make_request
+                    response.should_not have_tag('p#request_status a', :text =>/send a follow up message/)
+                end
+
+                it 'should not give a link to sign in (in the request status paragraph)' do
+                    make_request
+                    response.should_not have_tag('p#request_status a', :text => /sign in/)
+                end
+
+            end
+
+        end
+
+    end
 
     describe 'when handling an update_status parameter' do
+
+        describe 'when the request is external' do
+
+            it 'should assign the "update status" flag to the view as false if the parameter is present' do
+                get :show, :url_title => 'balalas', :update_status => 1
+                assigns[:update_status].should be_false
+            end
+
+            it 'should assign the "update status" flag to the view as false if the parameter is not present' do
+                get :show, :url_title => 'balalas'
+                assigns[:update_status].should be_false
+            end
+
+        end
+
         it 'should assign the "update status" flag to the view as true if the parameter is present' do
             get :show, :url_title => 'why_do_you_have_such_a_fancy_dog', :update_status => 1
             assigns[:update_status].should be_true
@@ -533,38 +709,48 @@ describe RequestController, "when showing one request" do
             end
         end
 
-        it "should make a zipfile available, which has a different URL when it changes" do
-            title = 'why_do_you_have_such_a_fancy_dog'
-            ir = info_requests(:fancy_dog_request)
-            session[:user_id] = ir.user.id # bob_smith_user
-            get :download_entire_request, :url_title => title
-            assigns[:url_path].should have_text(/#{title}.zip$/)
-            old_path = assigns[:url_path]
-            response.location.should have_text(/#{assigns[:url_path]}$/)
-            zipfile = Zip::ZipFile.open(File.join(File.dirname(__FILE__), "../../cache/zips", old_path)) { |zipfile|
-                zipfile.count.should == 1 # just the message
-            }
-            receive_incoming_mail('incoming-request-two-same-name.email', ir.incoming_email)
-            get :download_entire_request, :url_title => title
-            assigns[:url_path].should have_text(/#{title}.zip$/)
-            old_path = assigns[:url_path]
-            response.location.should have_text(/#{assigns[:url_path]}$/)
-            zipfile = Zip::ZipFile.open(File.join(File.dirname(__FILE__), "../../cache/zips", old_path)) { |zipfile|
-                zipfile.count.should == 3 # the message plus two "hello.txt" files
-            }
-
-            # The path of the zip file is based on the hash of the timestamp of the last request
-            # in the thread, so we wait for a second to make sure this one will have a different
-            # timestamp than the previous.
-            sleep 1
-            receive_incoming_mail('incoming-request-attachment-unknown-extension.email', ir.incoming_email)
-            get :download_entire_request, :url_title => title
-            assigns[:url_path].should have_text(/#{title}.zip$/)
-            assigns[:url_path].should_not == old_path
-            response.location.should have_text(/#{assigns[:url_path]}/)
-            zipfile = Zip::ZipFile.open(File.join(File.dirname(__FILE__), "../../cache/zips", assigns[:url_path])) { |zipfile|
-                zipfile.count.should == 5 # the message, two hello.txt, the unknown attachment, and its empty message
-            }
+        describe 'when making a zipfile available' do
+
+            it "should have a different zipfile URL when the request changes" do
+                title = 'why_do_you_have_such_a_fancy_dog'
+                ir = info_requests(:fancy_dog_request)
+                session[:user_id] = ir.user.id # bob_smith_user
+                get :download_entire_request, :url_title => title
+                assigns[:url_path].should have_text(/#{title}.zip$/)
+                old_path = assigns[:url_path]
+                response.location.should have_text(/#{assigns[:url_path]}$/)
+                zipfile = Zip::ZipFile.open(File.join(File.dirname(__FILE__), "../../cache/zips", old_path)) { |zipfile|
+                    zipfile.count.should == 1 # just the message
+                }
+                receive_incoming_mail('incoming-request-two-same-name.email', ir.incoming_email)
+                get :download_entire_request, :url_title => title
+                assigns[:url_path].should have_text(/#{title}.zip$/)
+                old_path = assigns[:url_path]
+                response.location.should have_text(/#{assigns[:url_path]}$/)
+                zipfile = Zip::ZipFile.open(File.join(File.dirname(__FILE__), "../../cache/zips", old_path)) { |zipfile|
+                    zipfile.count.should == 3 # the message plus two "hello.txt" files
+                }
+
+                # The path of the zip file is based on the hash of the timestamp of the last request
+                # in the thread, so we wait for a second to make sure this one will have a different
+                # timestamp than the previous.
+                sleep 1
+                receive_incoming_mail('incoming-request-attachment-unknown-extension.email', ir.incoming_email)
+                get :download_entire_request, :url_title => title
+                assigns[:url_path].should have_text(/#{title}.zip$/)
+                assigns[:url_path].should_not == old_path
+                response.location.should have_text(/#{assigns[:url_path]}/)
+                zipfile = Zip::ZipFile.open(File.join(File.dirname(__FILE__), "../../cache/zips", assigns[:url_path])) { |zipfile|
+                    zipfile.count.should == 5 # the message, two hello.txt, the unknown attachment, and its empty message
+                }
+            end
+
+            it 'should successfully make a zipfile for an external request' do
+                info_request = info_requests(:external_request)
+                get :download_entire_request, { :url_title => info_request.url_title },
+                                              { :user_id => users(:bob_smith_user) }
+                response.location.should have_text(/#{assigns[:url_path]}/)
+            end
         end
     end
 end
@@ -985,76 +1171,150 @@ describe RequestController, "when viewing an individual response for reply/follo
         get :show_response, :id => info_requests(:fancy_dog_request).id, :incoming_message_id => incoming_messages(:useless_incoming_message)
         response.should render_template('request/hidden')
     end
+
+    describe 'when viewing a response for an external request' do
+
+        it 'should show a message saying that external requests cannot be followed up' do
+            get :show_response, :id => info_requests(:external_request).id
+            response.should render_template('request/followup_bad')
+            assigns[:reason].should == 'external'
+        end
+
+        it 'should be successful' do
+            get :show_response, :id => info_requests(:external_request).id
+            response.should be_success
+        end
+
+    end
+
 end
 
 describe RequestController, "when classifying an information request" do
 
-    before(:each) do
-        @dog_request = info_requests(:fancy_dog_request)
-        @dog_request.stub!(:is_old_unclassified?).and_return(false)
-        InfoRequest.stub!(:find).and_return(@dog_request)
-        load_raw_emails_data
-    end
+    describe 'if the request is external' do
 
-    def post_status(status)
-        post :describe_state, :incoming_message => { :described_state => status },
-                              :id => @dog_request.id,
-                              :last_info_request_event_id => @dog_request.last_event_id_needing_description,
-                              :submitted_describe_state => 1
-    end
+        before do
+            @external_request = info_requests(:external_request)
+        end
 
-    it "should require login" do
-        post_status('rejected')
-        post_redirect = PostRedirect.get_last_post_redirect
-        response.should redirect_to(:controller => 'user', :action => 'signin', :token => post_redirect.token)
-    end
+        it 'should redirect to the request page' do
+            post :describe_state, :id => @external_request.id,
+                                  :submitted_describe_state => 1
+            response.should redirect_to(:action => 'show',
+                                        :controller => 'request',
+                                        :url_title => @external_request.url_title)
+        end
 
-    it 'should ask whether the request is old and unclassified' do
-        @dog_request.should_receive(:is_old_unclassified?)
-        post_status('rejected')
     end
 
-    it "should not classify the request if logged in as the wrong user" do
-        session[:user_id] = users(:silly_name_user).id
-        post_status('rejected')
-        response.should render_template('user/wrong_user')
-    end
+    describe 'when the request is internal' do
 
+        before(:each) do
+            @dog_request = info_requests(:fancy_dog_request)
+            @dog_request.stub!(:is_old_unclassified?).and_return(false)
+            InfoRequest.stub!(:find).and_return(@dog_request)
+            load_raw_emails_data
+        end
 
+        def post_status(status)
+            post :describe_state, :incoming_message => { :described_state => status },
+                                  :id => @dog_request.id,
+                                  :last_info_request_event_id => @dog_request.last_event_id_needing_description,
+                                  :submitted_describe_state => 1
+        end
 
-    describe 'when the request is old and unclassified' do
+        it "should require login" do
+            post_status('rejected')
+            post_redirect = PostRedirect.get_last_post_redirect
+            response.should redirect_to(:controller => 'user', :action => 'signin', :token => post_redirect.token)
+        end
 
-        before do
-            @dog_request.stub!(:is_old_unclassified?).and_return(true)
-            RequestMailer.stub!(:deliver_old_unclassified_updated)
+        it 'should ask whether the request is old and unclassified' do
+            @dog_request.should_receive(:is_old_unclassified?)
+            post_status('rejected')
         end
 
-        describe 'when the user is not logged in' do
+        it "should not classify the request if logged in as the wrong user" do
+            session[:user_id] = users(:silly_name_user).id
+            post_status('rejected')
+            response.should render_template('user/wrong_user')
+        end
+
+        describe 'when the request is old and unclassified' do
+
+            before do
+                @dog_request.stub!(:is_old_unclassified?).and_return(true)
+                RequestMailer.stub!(:deliver_old_unclassified_updated)
+            end
+
+            describe 'when the user is not logged in' do
+
+                it 'should require login' do
+                    session[:user_id] = nil
+                    post_status('rejected')
+                    post_redirect = PostRedirect.get_last_post_redirect
+                    response.should redirect_to(:controller => 'user', :action => 'signin', :token => post_redirect.token)
+                end
 
-            it 'should require login' do
-                session[:user_id] = nil
-                post_status('rejected')
-                post_redirect = PostRedirect.get_last_post_redirect
-                response.should redirect_to(:controller => 'user', :action => 'signin', :token => post_redirect.token)
             end
 
+            describe 'when the user is logged in as a different user' do
+
+                before do
+                    @other_user = mock_model(User)
+                    session[:user_id] = users(:silly_name_user).id
+                end
+
+                it 'should classify the request' do
+                    @dog_request.stub!(:calculate_status).and_return('rejected')
+                    @dog_request.should_receive(:set_described_state).with('rejected')
+                    post_status('rejected')
+                end
+
+                it 'should log a status update event' do
+                    expected_params = {:user_id => users(:silly_name_user).id,
+                                       :old_described_state => 'waiting_response',
+                                       :described_state => 'rejected'}
+                    @dog_request.should_receive(:log_event).with("status_update", expected_params)
+                    post_status('rejected')
+                end
+
+                it 'should send an email to the requester letting them know someone has updated the status of their request' do
+                    RequestMailer.should_receive(:deliver_old_unclassified_updated)
+                    post_status('rejected')
+                end
+
+                it 'should redirect to the request page' do
+                    post_status('rejected')
+                    response.should redirect_to(:action => 'show', :controller => 'request', :url_title => @dog_request.url_title)
+                end
+
+                it 'should show a message thanking the user for a good deed' do
+                    post_status('rejected')
+                    flash[:notice].should == 'Thank you for updating this request!'
+                end
+
+            end
         end
 
-        describe 'when the user is logged in as a different user' do
+        describe 'when logged in as an admin user who is not the actual requester' do
 
             before do
-                @other_user = mock_model(User)
-                session[:user_id] = users(:silly_name_user).id
+                @admin_user = users(:admin_user)
+                session[:user_id] = @admin_user.id
+                @dog_request = info_requests(:fancy_dog_request)
+                InfoRequest.stub!(:find).and_return(@dog_request)
+                @dog_request.stub!(:each).and_return([@dog_request])
             end
 
-            it 'should classify the request' do
+            it 'should update the status of the request' do
                 @dog_request.stub!(:calculate_status).and_return('rejected')
                 @dog_request.should_receive(:set_described_state).with('rejected')
                 post_status('rejected')
             end
 
             it 'should log a status update event' do
-                expected_params = {:user_id => users(:silly_name_user).id,
+                expected_params = {:user_id => @admin_user.id,
                                    :old_described_state => 'waiting_response',
                                    :described_state => 'rejected'}
                 @dog_request.should_receive(:log_event).with("status_update", expected_params)
@@ -1075,258 +1335,219 @@ describe RequestController, "when classifying an information request" do
                 post_status('rejected')
                 flash[:notice].should == 'Thank you for updating this request!'
             end
+         end
 
-        end
-    end
-
-    describe 'when logged in as an admin user who is not the actual requester' do
-
-        before do
-            @admin_user = users(:admin_user)
-            session[:user_id] = @admin_user.id
-            @dog_request = info_requests(:fancy_dog_request)
-            InfoRequest.stub!(:find).and_return(@dog_request)
-            @dog_request.stub!(:each).and_return([@dog_request])
-        end
-
-        it 'should update the status of the request' do
-            @dog_request.stub!(:calculate_status).and_return('rejected')
-            @dog_request.should_receive(:set_described_state).with('rejected')
-            post_status('rejected')
-        end
-
-        it 'should log a status update event' do
-            expected_params = {:user_id => @admin_user.id,
-                               :old_described_state => 'waiting_response',
-                               :described_state => 'rejected'}
-            @dog_request.should_receive(:log_event).with("status_update", expected_params)
-            post_status('rejected')
-        end
-
-        it 'should send an email to the requester letting them know someone has updated the status of their request' do
-            RequestMailer.should_receive(:deliver_old_unclassified_updated)
-            post_status('rejected')
-        end
-
-        it 'should redirect to the request page' do
-            post_status('rejected')
-            response.should redirect_to(:action => 'show', :controller => 'request', :url_title => @dog_request.url_title)
-        end
-
-        it 'should show a message thanking the user for a good deed' do
-            post_status('rejected')
-            flash[:notice].should == 'Thank you for updating this request!'
-        end
-     end
+        describe 'when logged in as an admin user who is also the actual requester' do
 
-    describe 'when logged in as an admin user who is also the actual requester' do
+            before do
+                @admin_user = users(:admin_user)
+                session[:user_id] = @admin_user.id
+                @dog_request = info_requests(:fancy_dog_request)
+                @dog_request.user = @admin_user
+                @dog_request.save!
+                InfoRequest.stub!(:find).and_return(@dog_request)
+                @dog_request.stub!(:each).and_return([@dog_request])
+            end
 
-        before do
-            @admin_user = users(:admin_user)
-            session[:user_id] = @admin_user.id
-            @dog_request = info_requests(:fancy_dog_request)
-            @dog_request.user = @admin_user
-            @dog_request.save!
-            InfoRequest.stub!(:find).and_return(@dog_request)
-            @dog_request.stub!(:each).and_return([@dog_request])
-        end
+            it 'should update the status of the request' do
+                @dog_request.stub!(:calculate_status).and_return('rejected')
+                @dog_request.should_receive(:set_described_state).with('rejected')
+                post_status('rejected')
+            end
 
-        it 'should update the status of the request' do
-            @dog_request.stub!(:calculate_status).and_return('rejected')
-            @dog_request.should_receive(:set_described_state).with('rejected')
-            post_status('rejected')
-        end
+            it 'should not log a status update event' do
+                @dog_request.should_not_receive(:log_event)
+                post_status('rejected')
+            end
 
-        it 'should not log a status update event' do
-            @dog_request.should_not_receive(:log_event)
-            post_status('rejected')
-        end
+            it 'should not send an email to the requester letting them know someone has updated the status of their request' do
+                RequestMailer.should_not_receive(:deliver_old_unclassified_updated)
+                post_status('rejected')
+            end
 
-        it 'should not send an email to the requester letting them know someone has updated the status of their request' do
-            RequestMailer.should_not_receive(:deliver_old_unclassified_updated)
-            post_status('rejected')
-        end
+            it 'should say it is showing advice as to what to do next' do
+                post_status('rejected')
+                flash[:notice].should match(/Here is what to do now/)
+            end
 
-        it 'should say it is showing advice as to what to do next' do
-            post_status('rejected')
-            flash[:notice].should match(/Here is what to do now/)
-        end
+            it 'should redirect to the unhappy page' do
+                post_status('rejected')
+                response.should redirect_to(:controller => 'help', :action => 'unhappy', :url_title => @dog_request.url_title)
+            end
 
-        it 'should redirect to the unhappy page' do
-            post_status('rejected')
-            response.should redirect_to(:controller => 'help', :action => 'unhappy', :url_title => @dog_request.url_title)
         end
 
-    end
+        describe 'when logged in as the requestor' do
 
-    describe 'when logged in as the requestor' do
+            before do
+                @request_owner = users(:bob_smith_user)
+                session[:user_id] = @request_owner.id
+                @dog_request.awaiting_description.should == true
+                @dog_request.stub!(:each).and_return([@dog_request])
+            end
 
-        before do
-            @request_owner = users(:bob_smith_user)
-            session[:user_id] = @request_owner.id
-            @dog_request.awaiting_description.should == true
-            @dog_request.stub!(:each).and_return([@dog_request])
-        end
+            it "should successfully classify response if logged in as user controlling request" do
+                post_status('rejected')
+                response.should redirect_to(:controller => 'help', :action => 'unhappy', :url_title => @dog_request.url_title)
+                @dog_request.reload
+                @dog_request.awaiting_description.should == false
+                @dog_request.described_state.should == 'rejected'
+                @dog_request.get_last_response_event.should == info_request_events(:useless_incoming_message_event)
+                @dog_request.get_last_response_event.calculated_state.should == 'rejected'
+            end
 
-        it "should successfully classify response if logged in as user controlling request" do
-            post_status('rejected')
-            response.should redirect_to(:controller => 'help', :action => 'unhappy', :url_title => @dog_request.url_title)
-            @dog_request.reload
-            @dog_request.awaiting_description.should == false
-            @dog_request.described_state.should == 'rejected'
-            @dog_request.get_last_response_event.should == info_request_events(:useless_incoming_message_event)
-            @dog_request.get_last_response_event.calculated_state.should == 'rejected'
-        end
+            it 'should not log a status update event' do
+                @dog_request.should_not_receive(:log_event)
+                post_status('rejected')
+            end
 
-        it 'should not log a status update event' do
-            @dog_request.should_not_receive(:log_event)
-            post_status('rejected')
-        end
+            it 'should not send an email to the requester letting them know someone has updated the status of their request' do
+                RequestMailer.should_not_receive(:deliver_old_unclassified_updated)
+                post_status('rejected')
+            end
 
-        it 'should not send an email to the requester letting them know someone has updated the status of their request' do
-            RequestMailer.should_not_receive(:deliver_old_unclassified_updated)
-            post_status('rejected')
-        end
+            it "should send email when classified as requires_admin" do
+                post :describe_state, :incoming_message => { :described_state => "requires_admin" }, :id => @dog_request.id, :incoming_message_id => incoming_messages(:useless_incoming_message), :last_info_request_event_id => @dog_request.last_event_id_needing_description, :submitted_describe_state => 1
+                response.should redirect_to(:controller => 'help', :action => 'contact')
 
-        it "should send email when classified as requires_admin" do
-            post :describe_state, :incoming_message => { :described_state => "requires_admin" }, :id => @dog_request.id, :incoming_message_id => incoming_messages(:useless_incoming_message), :last_info_request_event_id => @dog_request.last_event_id_needing_description, :submitted_describe_state => 1
-            response.should redirect_to(:controller => 'help', :action => 'contact')
+                @dog_request.reload
+                @dog_request.awaiting_description.should == false
+                @dog_request.described_state.should == 'requires_admin'
+                @dog_request.get_last_response_event.calculated_state.should == 'requires_admin'
 
-            @dog_request.reload
-            @dog_request.awaiting_description.should == false
-            @dog_request.described_state.should == 'requires_admin'
-            @dog_request.get_last_response_event.calculated_state.should == 'requires_admin'
+                deliveries = ActionMailer::Base.deliveries
+                deliveries.size.should == 1
+                mail = deliveries[0]
+                mail.body.should =~ /as needing admin/
+                mail.from_addrs.first.to_s.should == @request_owner.name_and_email
+            end
 
-            deliveries = ActionMailer::Base.deliveries
-            deliveries.size.should == 1
-            mail = deliveries[0]
-            mail.body.should =~ /as needing admin/
-            mail.from_addrs.first.to_s.should == @request_owner.name_and_email
-        end
+            it 'should say it is showing advice as to what to do next' do
+                post_status('rejected')
+                flash[:notice].should match(/Here is what to do now/)
+            end
 
-        it 'should say it is showing advice as to what to do next' do
-            post_status('rejected')
-            flash[:notice].should match(/Here is what to do now/)
-        end
+            it 'should redirect to the unhappy page' do
+                post_status('rejected')
+                response.should redirect_to(:controller => 'help', :action => 'unhappy', :url_title => @dog_request.url_title)
+            end
 
-        it 'should redirect to the unhappy page' do
-            post_status('rejected')
-            response.should redirect_to(:controller => 'help', :action => 'unhappy', :url_title => @dog_request.url_title)
+            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')
+                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
 
-        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')
-            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
+        describe 'when redirecting after a successful status update by the request owner' do
 
-    describe 'when redirecting after a successful status update by the request owner' do
+            before do
+                @request_owner = users(:bob_smith_user)
+                session[:user_id] = @request_owner.id
+                @dog_request = info_requests(:fancy_dog_request)
+                @dog_request.stub!(:each).and_return([@dog_request])
+                InfoRequest.stub!(:find).and_return(@dog_request)
+                @old_filters = ActionController::Routing::Routes.filters
+                ActionController::Routing::Routes.filters = RoutingFilter::Chain.new
+            end
+            after do
+                ActionController::Routing::Routes.filters = @old_filters
+            end
 
-        before do
-            @request_owner = users(:bob_smith_user)
-            session[:user_id] = @request_owner.id
-            @dog_request = info_requests(:fancy_dog_request)
-            @dog_request.stub!(:each).and_return([@dog_request])
-            InfoRequest.stub!(:find).and_return(@dog_request)
-            @old_filters = ActionController::Routing::Routes.filters
-            ActionController::Routing::Routes.filters = RoutingFilter::Chain.new
-        end
-        after do
-            ActionController::Routing::Routes.filters = @old_filters
-        end
+            def request_url
+                "request/#{@dog_request.url_title}"
+            end
 
-        def request_url
-            "request/#{@dog_request.url_title}"
-        end
+            def unhappy_url
+                "help/unhappy/#{@dog_request.url_title}"
+            end
 
-        def unhappy_url
-            "help/unhappy/#{@dog_request.url_title}"
-        end
+            def expect_redirect(status, redirect_path)
+                post_status(status)
+                response.should redirect_to("http://test.host/#{redirect_path}")
+            end
 
-        def expect_redirect(status, redirect_path)
-            post_status(status)
-            response.should redirect_to("http://test.host/#{redirect_path}")
-        end
+            it 'should redirect to the "request url" with a message in the right tense when status is updated to "waiting response" and the response is not overdue' do
+                @dog_request.stub!(:date_response_required_by).and_return(Time.now.to_date+1)
+                @dog_request.stub!(:date_very_overdue_after).and_return(Time.now.to_date+40)
 
-        it 'should redirect to the "request url" with a message in the right tense when status is updated to "waiting response" and the response is not overdue' do
-            @dog_request.stub!(:date_response_required_by).and_return(Time.now.to_date+1)
-            @dog_request.stub!(:date_very_overdue_after).and_return(Time.now.to_date+40)
+                expect_redirect("waiting_response", "request/#{@dog_request.url_title}")
+                flash[:notice].should match(/should get a response/)
+            end
 
-            expect_redirect("waiting_response", "request/#{@dog_request.url_title}")
-            flash[:notice].should match(/should get a response/)
-        end
+            it 'should redirect to the "request url" with a message in the right tense when status is updated to "waiting response" and the response is overdue' do
+                @dog_request.stub!(:date_response_required_by).and_return(Time.now.to_date-1)
+                @dog_request.stub!(:date_very_overdue_after).and_return(Time.now.to_date+40)
+                expect_redirect('waiting_response', request_url)
+                flash[:notice].should match(/should have got a response/)
+            end
 
-        it 'should redirect to the "request url" with a message in the right tense when status is updated to "waiting response" and the response is overdue' do
-            @dog_request.stub!(:date_response_required_by).and_return(Time.now.to_date-1)
-            @dog_request.stub!(:date_very_overdue_after).and_return(Time.now.to_date+40)
-            expect_redirect('waiting_response', request_url)
-            flash[:notice].should match(/should have got a response/)
-        end
+            it 'should redirect to the "request url" with a message in the right tense when status is updated to "waiting response" and the response is overdue' do
+                @dog_request.stub!(:date_response_required_by).and_return(Time.now.to_date-2)
+                @dog_request.stub!(:date_very_overdue_after).and_return(Time.now.to_date-1)
+                expect_redirect('waiting_response', unhappy_url)
+                flash[:notice].should match(/is long overdue/)
+                flash[:notice].should match(/by more than 40 working days/)
+                flash[:notice].should match(/within 20 working days/)
+            end
 
-        it 'should redirect to the "request url" with a message in the right tense when status is updated to "waiting response" and the response is overdue' do
-            @dog_request.stub!(:date_response_required_by).and_return(Time.now.to_date-2)
-            @dog_request.stub!(:date_very_overdue_after).and_return(Time.now.to_date-1)
-            expect_redirect('waiting_response', unhappy_url)
-            flash[:notice].should match(/is long overdue/)
-            flash[:notice].should match(/by more than 40 working days/)
-            flash[:notice].should match(/within 20 working days/)
-        end
+            it 'should redirect to the "request url" when status is updated to "not held"' do
+                expect_redirect('not_held', request_url)
+            end
 
-        it 'should redirect to the "request url" when status is updated to "not held"' do
-            expect_redirect('not_held', request_url)
-        end
+            it 'should redirect to the "request url" when status is updated to "successful"' do
+                expect_redirect('successful', request_url)
+            end
 
-        it 'should redirect to the "request url" when status is updated to "successful"' do
-            expect_redirect('successful', request_url)
-        end
+            it 'should redirect to the "unhappy url" when status is updated to "rejected"' do
+                expect_redirect('rejected', "help/unhappy/#{@dog_request.url_title}")
+            end
 
-        it 'should redirect to the "unhappy url" when status is updated to "rejected"' do
-            expect_redirect('rejected', "help/unhappy/#{@dog_request.url_title}")
-        end
+            it 'should redirect to the "unhappy url" when status is updated to "partially successful"' do
+                expect_redirect('partially_successful', "help/unhappy/#{@dog_request.url_title}")
+            end
 
-        it 'should redirect to the "unhappy url" when status is updated to "partially successful"' do
-            expect_redirect('partially_successful', "help/unhappy/#{@dog_request.url_title}")
-        end
+            it 'should redirect to the "response url" when status is updated to "waiting clarification" and there is a last response' do
+                incoming_message = mock_model(IncomingMessage)
+                @dog_request.stub!(:get_last_response).and_return(incoming_message)
+                expect_redirect('waiting_clarification', "request/#{@dog_request.id}/response/#{incoming_message.id}")
+            end
 
-        it 'should redirect to the "response url" when status is updated to "waiting clarification" and there is a last response' do
-            incoming_message = mock_model(IncomingMessage)
-            @dog_request.stub!(:get_last_response).and_return(incoming_message)
-            expect_redirect('waiting_clarification', "request/#{@dog_request.id}/response/#{incoming_message.id}")
-        end
+            it 'should redirect to the "response no followup url" when status is updated to "waiting clarification" and there are no events needing description' do
+                @dog_request.stub!(:get_last_response).and_return(nil)
+                expect_redirect('waiting_clarification', "request/#{@dog_request.id}/response")
+            end
 
-        it 'should redirect to the "response no followup url" when status is updated to "waiting clarification" and there are no events needing description' do
-            @dog_request.stub!(:get_last_response).and_return(nil)
-            expect_redirect('waiting_clarification', "request/#{@dog_request.id}/response")
-        end
+            it 'should redirect to the "respond to last url" when status is updated to "gone postal"' do
+                expect_redirect('gone_postal', "request/#{@dog_request.id}/response/#{@dog_request.get_last_response.id}?gone_postal=1")
+            end
 
-        it 'should redirect to the "respond to last url" when status is updated to "gone postal"' do
-            expect_redirect('gone_postal', "request/#{@dog_request.id}/response/#{@dog_request.get_last_response.id}?gone_postal=1")
-        end
+            it 'should redirect to the "request url" when status is updated to "internal review"' do
+                expect_redirect('internal_review', request_url)
+            end
 
-        it 'should redirect to the "request url" when status is updated to "internal review"' do
-            expect_redirect('internal_review', request_url)
-        end
+            it 'should redirect to the "help general url" when status is updated to "requires admin"' do
+                expect_redirect('requires_admin', "help/contact")
+            end
 
-        it 'should redirect to the "help general url" when status is updated to "requires admin"' do
-            expect_redirect('requires_admin', "help/contact")
-        end
+            it 'should redirect to the "help general url" when status is updated to "error message"' do
+                expect_redirect('error_message', "help/contact")
+            end
 
-        it 'should redirect to the "help general url" when status is updated to "error message"' do
-            expect_redirect('error_message', "help/contact")
-        end
+            it 'should redirect to the "respond to last url url" when status is updated to "user_withdrawn"' do
+                expect_redirect('user_withdrawn', "request/#{@dog_request.id}/response/#{@dog_request.get_last_response.id}")
+            end
 
-        it 'should redirect to the "respond to last url url" when status is updated to "user_withdrawn"' do
-            expect_redirect('user_withdrawn', "request/#{@dog_request.id}/response/#{@dog_request.get_last_response.id}")
         end
 
     end
+
 end
 
 describe RequestController, "when sending a followup message" do
@@ -1591,7 +1812,7 @@ describe RequestController, "comment alerts" do
     end
 
     it "should send an alert (once and once only)" do
-        # delete ficture comment and make new one, so is in last month (as
+        # delete fixture comment and make new one, so is in last month (as
         # alerts are only for comments in last month, see
         # RequestMailer.alert_comment_on_request)
         existing_comment = info_requests(:fancy_dog_request).comments[0]
@@ -1607,10 +1828,7 @@ describe RequestController, "comment alerts" do
         mail.to_addrs.first.to_s.should == info_requests(:fancy_dog_request).user.name_and_email
         mail.body =~ /(http:\/\/.*)/
         mail_url = $1
-
-        # XXX check mail_url here somehow, can't call comment_url like this:
-        # mail_url.should == comment_url(comments(:silly_comment))
-
+        mail_url.should match("/request/why_do_you_have_such_a_fancy_dog#comment-#{new_comment.id}")
 
         # check if we send again, no more go out
         deliveries.clear
@@ -1620,7 +1838,7 @@ describe RequestController, "comment alerts" do
     end
 
     it "should not send an alert when you comment on your own request" do
-        # delete ficture comment and make new one, so is in last month (as
+        # delete fixture comment and make new one, so is in last month (as
         # alerts are only for comments in last month, see
         # RequestMailer.alert_comment_on_request)
         existing_comment = info_requests(:fancy_dog_request).comments[0]
@@ -1635,6 +1853,16 @@ describe RequestController, "comment alerts" do
         deliveries.size.should == 0
     end
 
+    it 'should not send an alert for a comment on an external request' do
+        external_request = info_requests(:external_request)
+        external_request.add_comment("This external request is interesting", users(:silly_name_user))
+        # try to send comment alert
+        RequestMailer.alert_comment_on_request
+
+        deliveries = ActionMailer::Base.deliveries
+        deliveries.size.should == 0
+    end
+
     it "should send an alert when there are two new comments" do
         # add two comments - the second one sould be ignored, as is by the user who made the request.
         # the new comment here, will cause the one in the fixture to be picked up as a new comment by alert_comment_on_request also.
@@ -1650,9 +1878,7 @@ describe RequestController, "comment alerts" do
         mail.to_addrs.first.to_s.should == info_requests(:fancy_dog_request).user.name_and_email
         mail.body =~ /(http:\/\/.*)/
         mail_url = $1
-
-        # XXX check mail_url here somehow, can't call comment_url like this:
-        # mail_url.should == comment_url(comments(:silly_comment))
+        mail_url.should match("/request/why_do_you_have_such_a_fancy_dog#comment-#{comments(:silly_comment).id}")
 
     end
 
@@ -1736,6 +1962,10 @@ describe RequestController, "authority uploads a response from the web interface
         flash[:error].should match(/Please type a message/)
     end
 
+    it 'should 404 for non existent requests' do
+        lambda{ post :upload_response, :url_title => 'i_dont_exist'}.should raise_error(ActiveRecord::RecordNotFound)
+    end
+
     # How do I test a file upload in rails?
     # http://stackoverflow.com/questions/1178587/how-do-i-test-a-file-upload-in-rails
     it "should let the authority upload a file" do
@@ -1887,6 +2117,12 @@ describe RequestController, "when reporting a request (logged in)" do
         session[:user_id] = @user.id
     end
 
+    it "should 404 for non-existent requests" do
+      lambda {
+        post :report_request, :url_title => "hjksfdhjk_louytu_qqxxx"
+      }.should raise_error(ActiveRecord::RecordNotFound)
+    end
+
     it "should mark a request as having been reported" do
         ir = info_requests(:badger_request)
         title = ir.url_title