Merge remote-tracking branch 'openaustralia_github/one_support_email_on_admin_attention_state' into develop

Conflicts:
    Gemfile.lock
    app/controllers/request_controller.rb
    app/models/request_mailer.rb
    app/views/admin_general/index.rhtml
    spec/models/request_mailer_spec.rb

21 files changed, 419 insertions, 229 deletions

diff --git a/.gitignore b/.gitignore
index e0b1e3b21..efba35668 100644
--- a/.gitignore
+++ b/.gitignore
@@ -22,3 +22,4 @@ TAGS
 bin/
 config/aliases
 .sass-cache
+alaveteli.sublime*
diff --git a/Gemfile b/Gemfile
index 9f78b06f3..220f3a4fa 100644
--- a/Gemfile
+++ b/Gemfile
@@ -51,6 +51,8 @@ group :test do
   gem 'rspec-rails', '~> 1.3.4'
   gem 'test-unit', '~> 1.2.3', :platforms => :ruby_19
   gem 'coveralls', :require => false
+  # Using webrat because the preferred (capybara) doesn't work out of the box with rspec 1
+  gem 'webrat'
 end
 
 group :development do
diff --git a/Gemfile.lock b/Gemfile.lock
index 55b574c2c..c6575e2ea 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -96,11 +96,14 @@ GEM
     net-ssh-gateway (1.2.0)
       net-ssh (>= 2.6.5)
     newrelic_rpm (3.5.4.34)
+    nokogiri (1.5.6)
     pg (0.13.2)
     polyglot (0.3.3)
     rack (1.1.6)
     rack-ssl (1.3.3)
       rack
+    rack-test (0.6.2)
+      rack (>= 1.0)
     rake (0.9.2.2)
     rbx-require-relative (0.0.9)
     rdoc (3.12.1)
@@ -161,6 +164,10 @@ GEM
       polyglot
       polyglot (>= 0.3.1)
     vpim (0.695)
+    webrat (0.7.3)
+      nokogiri (>= 1.2.0)
+      rack (>= 1.0)
+      rack-test (>= 0.5.3)
     will_paginate (2.3.16)
     xapian-full-alaveteli (1.2.9.5)
     xml-simple (1.1.1)
@@ -206,6 +213,7 @@ DEPENDENCIES
   syslog_protocol
   test-unit (~> 1.2.3)
   vpim
+  webrat
   will_paginate (~> 2.3.11)
   xapian-full-alaveteli (~> 1.2.9.5)
   xml-simple
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index fe948db19..ec5c9d055 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -374,93 +374,77 @@ class RequestController < ApplicationController
 
     # Submitted to the describing state of messages form
     def describe_state
-        @info_request = InfoRequest.find(params[:id].to_i)
-        set_last_request(@info_request)
-
-        # If this isn't a form submit, go to the request page
-        if params[:submitted_describe_state].nil?
-            redirect_to request_url(@info_request)
-            return
-        end
+        info_request = InfoRequest.find(params[:id].to_i)
+        set_last_request(info_request)
 
         # If this is an external request, go to the request page - we don't allow
         # state change from the front end interface.
-        if @info_request.is_external?
-            redirect_to request_url(@info_request)
+        if info_request.is_external?
+            redirect_to request_url(info_request)
             return
         end
 
-        @is_owning_user = @info_request.is_owning_user?(authenticated_user)
-        @last_info_request_event_id = @info_request.last_event_id_needing_description
-        @old_unclassified = @info_request.is_old_unclassified? && !authenticated_user.nil?
-
-        # Check authenticated, and parameters set. We check is_owning_user
-        # to get admin overrides (see is_owning_user? above)
-        if !@old_unclassified && !@is_owning_user && !authenticated_as_user?(@info_request.user,
+        # Check authenticated, and parameters set.
+        unless Ability::can_update_request_state?(authenticated_user, info_request)
+            authenticated_as_user?(info_request.user,
                 :web => _("To classify the response to this FOI request"),
-                :email => _("Then you can classify the FOI response you have got from ") + @info_request.public_body.name + ".",
-                :email_subject => _("Classify an FOI response from ") + @info_request.public_body.name
-            )
+                :email => _("Then you can classify the FOI response you have got from ") + info_request.public_body.name + ".",
+                :email_subject => _("Classify an FOI response from ") + info_request.public_body.name)
             # do nothing - as "authenticated?" has done the redirect to signin page for us
             return
         end
 
         if !params[:incoming_message]
             flash[:error] = _("Please choose whether or not you got some of the information that you wanted.")
-            redirect_to request_url(@info_request)
+            redirect_to request_url(info_request)
             return
         end
 
-        if params[:last_info_request_event_id].to_i != @last_info_request_event_id
+        if params[:last_info_request_event_id].to_i != info_request.last_event_id_needing_description
             flash[:error] = _("The request has been updated since you originally loaded this page. Please check for any new incoming messages below, and try again.")
-            redirect_to request_url(@info_request)
+            redirect_to request_url(info_request)
+            return
+        end
+
+        described_state = params[:incoming_message][:described_state]
+        message = params[:incoming_message][:message]
+        # For requires_admin and error_message states we ask for an extra message to send to
+        # the administrators.
+        # If this message hasn't been included then ask for it
+        if ["error_message", "requires_admin"].include?(described_state) && message.nil?
+            redirect_to describe_state_message_url(:url_title => info_request.url_title, :described_state => described_state)
             return
         end
 
         # Make the state change
-        old_described_state = @info_request.described_state
-        @info_request.set_described_state(params[:incoming_message][:described_state])
+        info_request.set_described_state(described_state, authenticated_user, message)
 
         # If you're not the *actual* requester. e.g. you are playing the
         # classification game, or you're doing this just because you are an
         # admin user (not because you also own the request).
-        if !@info_request.is_actual_owning_user?(authenticated_user)
-            # Log the status change by someone other than the requester
-            event = @info_request.log_event("status_update",
-                { :user_id => authenticated_user.id,
-                  :old_described_state => old_described_state,
-                  :described_state => @info_request.described_state,
-                })
-            # Create a classification event for league tables
-            RequestClassification.create!(:user_id => authenticated_user.id,
-                                          :info_request_event_id => event.id)
-
+        if !info_request.is_actual_owning_user?(authenticated_user)
             # Don't give advice on what to do next, as it isn't their request
-            RequestMailer.deliver_old_unclassified_updated(@info_request) if !@info_request.is_external?
             if session[:request_game]
-                flash[:notice] = _('Thank you for updating the status of the request \'<a href="{{url}}">{{info_request_title}}</a>\'. There are some more requests below for you to classify.',:info_request_title=>CGI.escapeHTML(@info_request.title), :url=>CGI.escapeHTML(request_path(@info_request)))
+                flash[:notice] = _('Thank you for updating the status of the request \'<a href="{{url}}">{{info_request_title}}</a>\'. There are some more requests below for you to classify.',:info_request_title=>CGI.escapeHTML(info_request.title), :url=>CGI.escapeHTML(request_path(info_request)))
                 redirect_to categorise_play_url
             else
                 flash[:notice] = _('Thank you for updating this request!')
-                redirect_to request_url(@info_request)
+                redirect_to request_url(info_request)
             end
             return
         end
 
-        calculated_status = @info_request.calculate_status
         # Display advice for requester on what to do next, as appropriate
-        if calculated_status == 'waiting_response'
-            flash[:notice] = _("<p>Thank you! Hopefully your wait isn't too long.</p> <p>By law, you should get a response promptly, and normally before the end of <strong>
-{{date_response_required_by}}</strong>.</p>",:date_response_required_by=>simple_date(@info_request.date_response_required_by))
-            redirect_to request_url(@info_request)
-        elsif calculated_status == 'waiting_response_overdue'
-            flash[:notice] = _("<p>Thank you! Hope you don't have to wait much longer.</p> <p>By law, you should have got a response promptly, and normally before the end of <strong>{{date_response_required_by}}</strong>.</p>",:date_response_required_by=>simple_date(@info_request.date_response_required_by))
-            redirect_to request_url(@info_request)
-        elsif calculated_status == 'waiting_response_very_overdue'
-            flash[:notice] = _("<p>Thank you! Your request is long overdue, by more than {{very_late_number_of_days}} working days. Most requests should be answered within {{late_number_of_days}} working days. You might like to complain about this, see below.</p>", :very_late_number_of_days => Configuration::reply_very_late_after_days, :late_number_of_days => Configuration::reply_late_after_days)
-            redirect_to unhappy_url(@info_request)
-        elsif calculated_status == 'not_held'
-            flash[:notice] = _("<p>Thank you! Here are some ideas on what to do next:</p>
+        flash[:notice] = case info_request.calculate_status
+        when 'waiting_response'
+            _("<p>Thank you! Hopefully your wait isn't too long.</p> <p>By law, you should get a response promptly, and normally before the end of <strong>
+{{date_response_required_by}}</strong>.</p>",:date_response_required_by=>simple_date(info_request.date_response_required_by))
+        when 'waiting_response_overdue'
+            _("<p>Thank you! Hope you don't have to wait much longer.</p> <p>By law, you should have got a response promptly, and normally before the end of <strong>{{date_response_required_by}}</strong>.</p>",:date_response_required_by=>simple_date(info_request.date_response_required_by))
+        when 'waiting_response_very_overdue'
+            _("<p>Thank you! Your request is long overdue, by more than {{very_late_number_of_days}} working days. Most requests should be answered within {{late_number_of_days}} working days. You might like to complain about this, see below.</p>", :very_late_number_of_days => Configuration::reply_very_late_after_days, :late_number_of_days => Configuration::reply_late_after_days)
+        when 'not_held'
+            _("<p>Thank you! Here are some ideas on what to do next:</p>
             <ul>
             <li>To send your request to another authority, first copy the text of your request below, then <a href=\"{{find_authority_url}}\">find the other authority</a>.</li>
             <li>If you would like to contest the authority's claim that they do not hold the information, here is
@@ -471,44 +455,60 @@ class RequestController < ApplicationController
             </li>
             </ul>",
             :find_authority_url => "/new",
-            :complain_url => CGI.escapeHTML(unhappy_url(@info_request)),
-            :other_means_url => CGI.escapeHTML(unhappy_url(@info_request)) + "#other_means")
-            redirect_to request_url(@info_request)
-        elsif calculated_status == 'rejected'
-            flash[:notice] = _("Oh no! Sorry to hear that your request was refused. Here is what to do now.")
-            redirect_to unhappy_url(@info_request)
-        elsif calculated_status == 'successful'
-            flash[:notice] = _("<p>We're glad you got all the information that you wanted. If you write about or make use of the information, please come back and add an annotation below saying what you did.</p><p>If you found {{site_name}} useful, <a href=\"{{donation_url}}\">make a donation</a> to the charity which runs it.</p>", :site_name=>site_name, :donation_url => "http://www.mysociety.org/donate/")
-            redirect_to request_url(@info_request)
-        elsif calculated_status == 'partially_successful'
-            flash[:notice] = _("<p>We're glad you got some of the information that you wanted. If you found {{site_name}} useful, <a href=\"{{donation_url}}\">make a donation</a> to the charity which runs it.</p><p>If you want to try and get the rest of the information, here's what to do now.</p>", :site_name=>site_name, :donation_url=>"http://www.mysociety.org/donate/")
-            redirect_to unhappy_url(@info_request)
-        elsif calculated_status == 'waiting_clarification'
-            flash[:notice] = _("Please write your follow up message containing the necessary clarifications below.")
-            redirect_to respond_to_last_url(@info_request)
-        elsif calculated_status == 'gone_postal'
-            redirect_to respond_to_last_url(@info_request) + "?gone_postal=1"
-        elsif calculated_status == 'internal_review'
-            flash[:notice] = _("<p>Thank you! Hopefully your wait isn't too long.</p><p>You should get a response within {{late_number_of_days}} days, or be told if it will take longer (<a href=\"{{review_url}}\">details</a>).</p>",:late_number_of_days => Configuration.reply_late_after_days, :review_url => unhappy_url(@info_request) + "#internal_review")
-            redirect_to request_url(@info_request)
-        elsif calculated_status == 'error_message'
-            flash[:notice] = _("<p>Thank you! We'll look into what happened and try and fix it up.</p><p>If the error was a delivery failure, and you can find an up to date FOI email address for the authority, please tell us using the form below.</p>")
-            redirect_to help_general_url(:action => 'contact')
-        elsif calculated_status == 'requires_admin'
-            flash[:notice] = _("Please use the form below to tell us more.")
-            redirect_to help_general_url(:action => 'contact')
-        elsif calculated_status == 'user_withdrawn'
-            flash[:notice] = _("If you have not done so already, please write a message below telling the authority that you have withdrawn your request. Otherwise they will not know it has been withdrawn.")
-            redirect_to respond_to_last_url(@info_request)
+            :complain_url => CGI.escapeHTML(unhappy_url(info_request)),
+            :other_means_url => CGI.escapeHTML(unhappy_url(info_request)) + "#other_means")
+        when 'rejected'
+            _("Oh no! Sorry to hear that your request was refused. Here is what to do now.")
+        when 'successful'
+            _("<p>We're glad you got all the information that you wanted. If you write about or make use of the information, please come back and add an annotation below saying what you did.</p><p>If you found {{site_name}} useful, <a href=\"{{donation_url}}\">make a donation</a> to the charity which runs it.</p>", :site_name=>site_name, :donation_url => "http://www.mysociety.org/donate/")
+        when 'partially_successful'
+            _("<p>We're glad you got some of the information that you wanted. If you found {{site_name}} useful, <a href=\"{{donation_url}}\">make a donation</a> to the charity which runs it.</p><p>If you want to try and get the rest of the information, here's what to do now.</p>", :site_name=>site_name, :donation_url=>"http://www.mysociety.org/donate/")
+        when 'waiting_clarification'
+            _("Please write your follow up message containing the necessary clarifications below.")
+        when 'gone_postal'
+            nil
+        when 'internal_review'
+            _("<p>Thank you! Hopefully your wait isn't too long.</p><p>You should get a response within {{late_number_of_days}} days, or be told if it will take longer (<a href=\"{{review_url}}\">details</a>).</p>",:late_number_of_days => Configuration.reply_late_after_days, :review_url => unhappy_url(info_request) + "#internal_review")
+        when 'error_message', 'requires_admin'
+            _("Thank you! We'll look into what happened and try and fix it up.")
+        when 'user_withdrawn'
+            _("If you have not done so already, please write a message below telling the authority that you have withdrawn your request. Otherwise they will not know it has been withdrawn.")
+        end
+
+        case info_request.calculate_status
+        when 'waiting_response', 'waiting_response_overdue', 'not_held', 'successful',
+            'internal_review', 'error_message', 'requires_admin'
+            redirect_to request_url(info_request)
+        when 'waiting_response_very_overdue', 'rejected', 'partially_successful'
+            redirect_to unhappy_url(info_request)
+        when 'waiting_clarification', 'user_withdrawn'
+            redirect_to respond_to_last_url(info_request)
+        when 'gone_postal'
+            redirect_to respond_to_last_url(info_request) + "?gone_postal=1"
         else
             if @@custom_states_loaded
-                return self.theme_describe_state(@info_request)
+                return self.theme_describe_state(info_request)
             else
-                raise "unknown calculate_status " + calculated_status
+                raise "unknown calculate_status #{info_request.calculate_status}"
             end
         end
     end
 
+    # Collect a message to include with the change of state
+    def describe_state_message
+        @info_request = InfoRequest.find_by_url_title!(params[:url_title])
+        @described_state = params[:described_state]
+        @last_info_request_event_id = @info_request.last_event_id_needing_description
+        @title = case @described_state
+        when "error_message"
+            _("I've received an error message")
+        when "requires_admin"
+            _("This request requires administrator attention")
+        else
+            raise "Unsupported state"
+        end
+    end
+
     # Used for links from polymorphic URLs e.g. in Atom feeds - just redirect to
     # proper URL for the message the event refers to
     def show_request_event
@@ -882,7 +882,7 @@ class RequestController < ApplicationController
                               :email_subject => _("Log in to download a zip file of {{info_request_title}}",
                                            :info_request_title=>@info_request.title)
                               )
-                updated = Digest::SHA1.hexdigest(@info_request.get_last_event.created_at.to_i.to_s + @info_request.updated_at.to_i.to_s)
+                updated = Digest::SHA1.hexdigest(@info_request.info_request_events.last.created_at.to_i.to_s + @info_request.updated_at.to_i.to_s)
                 @url_path = File.join("/download",
                                        request_dirs(@info_request),
                                        updated,
diff --git a/app/models/info_request.rb b/app/models/info_request.rb
index cee9eb959..237364f56 100644
--- a/app/models/info_request.rb
+++ b/app/models/info_request.rb
@@ -543,20 +543,16 @@ public
 
     # states which require administrator action (hence email administrators
     # when they are entered, and offer state change dialog to them)
-    def InfoRequest.requires_admin_states
-        return ['requires_admin', 'error_message', 'attention_requested']
-    end
-
     def requires_admin?
-        return true if InfoRequest.requires_admin_states.include?(described_state)
-        return false
+        ['requires_admin', 'error_message', 'attention_requested'].include?(described_state)
     end
 
     # change status, including for last event for later historical purposes
-    def set_described_state(new_state, set_by = nil)
+    def set_described_state(new_state, set_by = nil, message = "")
+        old_described_state = described_state
         ActiveRecord::Base.transaction do
             self.awaiting_description = false
-            last_event = self.get_last_event
+            last_event = self.info_request_events.last
             last_event.described_state = new_state
             self.described_state = new_state
             last_event.save!
@@ -568,9 +564,23 @@ public
         if self.requires_admin?
             # Check there is someone to send the message "from"
             if !set_by.nil? || !self.user.nil?
-                RequestMailer.deliver_requires_admin(self, set_by)
+                RequestMailer.deliver_requires_admin(self, set_by, message)
             end
         end
+
+        unless set_by.nil? || is_actual_owning_user?(set_by) || described_state == 'attention_requested'
+            # Log the status change by someone other than the requester
+            event = log_event("status_update",
+                { :user_id => set_by.id,
+                  :old_described_state => old_described_state,
+                  :described_state => described_state,
+                })
+            # Create a classification event for league tables
+            RequestClassification.create!(:user_id => set_by.id,
+                                          :info_request_event_id => event.id)
+
+            RequestMailer.deliver_old_unclassified_updated(self) if !is_external?
+        end
     end
 
     # Work out what the situation of the request is. In addition to values of
@@ -719,41 +729,28 @@ public
         self.info_request_events.create!(:event_type => type, :params => params)
     end
 
+    def response_events
+        self.info_request_events.select{|e| e.response?}
+    end
+
     # The last response is the default one people might want to reply to
     def get_last_response_event_id
-        for e in self.info_request_events.reverse
-            if e.event_type == 'response'
-                return e.id
-            end
-        end
-        return nil
-
+        get_last_response_event.id if get_last_response_event
     end
     def get_last_response_event
-        for e in self.info_request_events.reverse
-            if e.event_type == 'response'
-                return e
-            end
-        end
-        return nil
+        response_events.last
     end
     def get_last_response
-        last_response_event = self.get_last_response_event
-        if last_response_event.nil?
-            return nil
-        else
-            return last_response_event.incoming_message
-        end
+        get_last_response_event.incoming_message if get_last_response_event
+    end
+
+    def outgoing_events
+        info_request_events.select{|e| e.outgoing? }
     end
 
     # The last outgoing message
     def get_last_outgoing_event
-        for e in self.info_request_events.reverse
-            if [ 'sent', 'followup_sent' ].include?(e.event_type)
-                return e
-            end
-        end
-        return nil
+        outgoing_events.last
     end
 
     # Text from the the initial request, for use in summary display
@@ -794,16 +791,6 @@ public
         end
     end
 
-    # Returns last event
-    def get_last_event
-        events = self.info_request_events
-        if events.size == 0
-            return nil
-        else
-            return events[-1]
-        end
-    end
-
     # Get previous email sent to
     def get_previous_email_sent_to(info_request_event)
         last_email = nil
@@ -821,46 +808,31 @@ public
 
     # Display version of status
     def InfoRequest.get_status_description(status)
-        if status == 'waiting_classification'
-            _("Awaiting classification.")
-        elsif status == 'waiting_response'
-            _("Awaiting response.")
-        elsif status == 'waiting_response_overdue'
-            _("Delayed.")
-        elsif status == 'waiting_response_very_overdue'
-            _("Long overdue.")
-        elsif status == 'not_held'
-            _("Information not held.")
-        elsif status == 'rejected'
-            _("Refused.")
-        elsif status == 'partially_successful'
-            _("Partially successful.")
-        elsif status == 'successful'
-            _("Successful.")
-        elsif status == 'waiting_clarification'
-            _("Waiting clarification.")
-        elsif status == 'gone_postal'
-            _("Handled by post.")
-        elsif status == 'internal_review'
-            _("Awaiting internal review.")
-        elsif status == 'error_message'
-            _("Delivery error")
-        elsif status == 'requires_admin'
-            _("Unusual response.")
-        elsif status == 'attention_requested'
-            _("Reported for administrator attention.")
-        elsif status == 'user_withdrawn'
-            _("Withdrawn by the requester.")
-        elsif status == 'vexatious'
-            _("Considered by administrators as vexatious and hidden from site.")
-        elsif status == 'not_foi'
-            _("Considered by administrators as not an FOI request and hidden from site.")
+        descriptions = {
+            'waiting_classification'        => _("Awaiting classification."),
+            'waiting_response'              => _("Awaiting response."),
+            'waiting_response_overdue'      => _("Delayed."),
+            'waiting_response_very_overdue' => _("Long overdue."),
+            'not_held'                      => _("Information not held."),
+            'rejected'                      => _("Refused."),
+            'partially_successful'          => _("Partially successful."),
+            'successful'                    => _("Successful."),
+            'waiting_clarification'         => _("Waiting clarification."),
+            'gone_postal'                   => _("Handled by post."),
+            'internal_review'               => _("Awaiting internal review."),
+            'error_message'                 => _("Delivery error"),
+            'requires_admin'                => _("Unusual response."),
+            'attention_requested'           => _("Reported for administrator attention."),
+            'user_withdrawn'                => _("Withdrawn by the requester."),
+            'vexatious'                     => _("Considered by administrators as vexatious and hidden from site."),
+            'not_foi'                       => _("Considered by administrators as not an FOI request and hidden from site."),
+        }
+        if descriptions[status]
+            descriptions[status]
+        elsif respond_to?(:theme_display_status)
+            theme_display_status(status)
         else
-            begin
-                return self.theme_display_status(status)
-            rescue NoMethodError
-                raise _("unknown status ") + status
-            end
+            raise _("unknown status ") + status
         end
     end
 
@@ -987,13 +959,8 @@ public
     end
 
     def is_old_unclassified?
-        return false if is_external?
-        return false if !awaiting_description
-        return false if url_title == 'holding_pen'
-        last_response_event = get_last_response_event
-        return false unless last_response_event
-        return false if last_response_event.created_at >= Time.now - OLD_AGE_IN_DAYS
-        return true
+        !is_external? && awaiting_description && url_title != 'holding_pen' && get_last_response_event &&
+            Time.now > get_last_response_event.created_at + OLD_AGE_IN_DAYS
     end
 
     # List of incoming messages to followup, by unique email
diff --git a/app/models/info_request_event.rb b/app/models/info_request_event.rb
index 09eba31ab..871b81b1f 100644
--- a/app/models/info_request_event.rb
+++ b/app/models/info_request_event.rb
@@ -77,17 +77,18 @@ class InfoRequestEvent < ActiveRecord::Base
     end
 
     def user_can_view?(user)
-        if !self.info_request.user_can_view?(user)
+        unless info_request.user_can_view?(user)
             raise "internal error, called user_can_view? on event when there is not permission to view entire request"
         end
 
-        if self.prominence == 'hidden'
-            return User.view_hidden_requests?(user)
-        end
-        if self.prominence == 'requester_only'
-            return self.info_request.is_owning_user?(user)
+        case prominence
+        when 'hidden'
+            User.view_hidden_requests?(user)
+        when 'requester_only'
+            info_request.is_owning_user?(user)
+        else
+            true
         end
-        return true
     end
 
 
@@ -363,16 +364,19 @@ class InfoRequestEvent < ActiveRecord::Base
     end
 
     def is_sent_sort?
-        if [ 'sent', 'resent'].include?(self.event_type)
-            return true
-        end
-        return false
+        ['sent', 'resent'].include?(event_type)
     end
+
     def is_followup_sort?
-        if [ 'followup_sent', 'followup_resent'].include?(self.event_type)
-            return true
-        end
-        return false
+        ['followup_sent', 'followup_resent'].include?(event_type)
+    end
+
+    def outgoing?
+        ['sent', 'followup_sent'].include?(event_type)
+    end
+
+    def response?
+        event_type == 'response'
     end
 
     def same_email_as_previous_send?
diff --git a/app/models/request_mailer.rb b/app/models/request_mailer.rb
index 6f8d88a1a..955f73ef4 100644
--- a/app/models/request_mailer.rb
+++ b/app/models/request_mailer.rb
@@ -62,18 +62,14 @@ class RequestMailer < ApplicationMailer
     end
 
     # An FOI response is outside the scope of the system, and needs admin attention
-    def requires_admin(info_request, set_by = nil)
-        if !set_by.nil?
-            user = set_by
-        else
-            user = info_request.user
-        end
+    def requires_admin(info_request, set_by = nil, message = "")
+        user = set_by || info_request.user
         @from = user.name_and_email
         @recipients = contact_from_name_and_email
         @subject = _("FOI response requires admin ({{reason}}) - {{title}}", :reason => info_request.described_state, :title => info_request.title)
         url = request_url(info_request)
         admin_url = admin_request_show_url(info_request)
-        @body = {:reported_by => user, :info_request => info_request, :url => url, :admin_url => admin_url }
+        @body = {:reported_by => user, :message => message, :info_request => info_request, :url => url, :admin_url => admin_url }
     end
 
     # Tell the requester that a new response has arrived
diff --git a/app/views/request/_describe_state.rhtml b/app/views/request/_describe_state.rhtml
index fde1cdfa7..0b65024fd 100644
--- a/app/views/request/_describe_state.rhtml
+++ b/app/views/request/_describe_state.rhtml
@@ -97,7 +97,6 @@
 
         <p>
         <%= hidden_field_tag 'last_info_request_event_id', @last_info_request_event_id, :id => 'last_info_request_event_id' + id_suffix %>
-        <%= hidden_field_tag 'submitted_describe_state', 1, :id => 'submitted_describe_state' + id_suffix %>
         <%= submit_tag _("Submit status") %> (<%= _('and we\'ll suggest <strong>what to do next</strong>') %>)
         </p>
     <% end %>
diff --git a/app/views/request/_other_describe_state.rhtml b/app/views/request/_other_describe_state.rhtml
index e274fe8c9..3a80823c6 100644
--- a/app/views/request/_other_describe_state.rhtml
+++ b/app/views/request/_other_describe_state.rhtml
@@ -75,7 +75,6 @@
 
     <p>
     <%= hidden_field_tag 'last_info_request_event_id', @last_info_request_event_id, :id => 'last_info_request_event_id' + id_suffix %>
-    <%= hidden_field_tag 'submitted_describe_state', 1, :id => 'submitted_describe_state' + id_suffix %>
     <%= submit_tag "Submit status" %>
     </p>
 <% end %>
diff --git a/app/views/request/describe_state_message.rhtml b/app/views/request/describe_state_message.rhtml
new file mode 100644
index 000000000..1427fea22
--- /dev/null
+++ b/app/views/request/describe_state_message.rhtml
@@ -0,0 +1,30 @@
+<h1><%= @title %></h1>
+
+
+<p>
+  <% if @described_state == "error_message" %>
+    <%= _("If the error was a delivery failure, and you can find an up to date FOI email address for the authority, please tell us using the form below.") %>
+  <% else %>
+    <%= _("Just one more thing") %>
+  <% end %>
+</p>
+
+<% form_for :incoming_message, :url => describe_state_url(:id => @info_request.id) do |f| %>
+
+    <p>
+        <label class="form_label" for="incoming_message_message">Please tell us more:</label>
+        <%= f.text_area :message, :rows => 10, :cols => 60 %>
+    </p>
+
+    <div>
+      <%= hidden_field_tag "incoming_message[described_state]", @described_state %>
+      <%= hidden_field_tag :last_info_request_event_id, @last_info_request_event_id %>
+    </div>
+
+    <div class="form_button">
+        <%= submit_tag _("Submit status and send message") %>
+    </div>
+
+<% end %>
+
+
diff --git a/app/views/request_mailer/requires_admin.rhtml b/app/views/request_mailer/requires_admin.rhtml
index e7ab53c59..b2e58295e 100644
--- a/app/views/request_mailer/requires_admin.rhtml
+++ b/app/views/request_mailer/requires_admin.rhtml
@@ -1,3 +1,5 @@
+<%= raw @message %>
+
 ---------------------------------------------------------------------
 <%= raw @reported_by.name %> <%= _('has reported an')%> <%= raw @info_request.law_used_short %>
 <%= _('response as needing administrator attention. Take a look, and reply to this
diff --git a/config/environment.rb b/config/environment.rb
index 4621c6a02..f9042857b 100644
--- a/config/environment.rb
+++ b/config/environment.rb
@@ -143,6 +143,7 @@ require 'world_foi_websites.rb'
 require 'alaveteli_external_command.rb'
 require 'quiet_opener.rb'
 require 'mail_handler'
+require "cookie_store_with_line_break_fix"
 
 if !Configuration.exception_notifications_from.blank? && !Configuration.exception_notifications_to.blank?
   ExceptionNotification::Notifier.sender_address = Configuration::exception_notifications_from
diff --git a/config/routes.rb b/config/routes.rb
index ee7ebdcac..a18295f7b 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -54,7 +54,8 @@ ActionController::Routing::Routes.draw do |map|
         request.details_request     '/details/request/:url_title', :action => 'details'
         request.similar_request     '/similar/request/:url_title', :action => 'similar'
 
-        request.describe_state   '/request/:id/describe', :action => 'describe_state'
+        request.describe_state   '/request/:id/describe', :action => 'describe_state', :conditions => {:method => :post}
+        request.describe_state_message   '/request/:url_title/describe/:described_state', :action => 'describe_state_message'
         request.show_response_no_followup    '/request/:id/response', :action => 'show_response'
         request.show_response    '/request/:id/response/:incoming_message_id', :action => 'show_response'
         request.get_attachment_as_html   '/request/:id/response/:incoming_message_id/attach/html/:part/*file_name', :action => 'get_attachment_as_html'
diff --git a/lib/ability.rb b/lib/ability.rb
new file mode 100644
index 000000000..2865ccb1c
--- /dev/null
+++ b/lib/ability.rb
@@ -0,0 +1,5 @@
+module Ability
+  def self.can_update_request_state?(user, request)
+    (user && request.is_old_unclassified?) || request.is_owning_user?(user)
+  end
+end
\ No newline at end of file
diff --git a/lib/cookie_store_with_line_break_fix.rb b/lib/cookie_store_with_line_break_fix.rb
new file mode 100644
index 000000000..dc623fbd0
--- /dev/null
+++ b/lib/cookie_store_with_line_break_fix.rb
@@ -0,0 +1,19 @@
+# See https://makandracards.com/makandra/9443-rails-2-s-cookiestore-produces-invalid-cookie-data-causing-tests-to-break
+
+# Should be able to remove this when we upgrade to Rails 3
+
+module ActionController
+  module Session
+    CookieStore.class_eval do
+
+      def call_with_line_break_fix(*args)
+        status, headers, body = call_without_line_break_fix(*args)
+        headers['Set-Cookie'].gsub! "\n\n", "\n" if headers['Set-Cookie'].present?
+        [ status, headers, body ]
+      end
+
+      alias_method_chain :call, :line_break_fix
+
+    end
+  end
+end
\ No newline at end of file
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index 21dd0853a..f35523532 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -1250,8 +1250,7 @@ describe RequestController, "when classifying an information request" do
         end
 
         it 'should redirect to the request page' do
-            post :describe_state, :id => @external_request.id,
-                                  :submitted_describe_state => 1
+            post :describe_state, :id => @external_request.id
             response.should redirect_to(:action => 'show',
                                         :controller => 'request',
                                         :url_title => @external_request.url_title)
@@ -1271,8 +1270,7 @@ describe RequestController, "when classifying an information request" 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
+                                  :last_info_request_event_id => @dog_request.last_event_id_needing_description
         end
 
         it "should require login" do
@@ -1282,6 +1280,7 @@ describe RequestController, "when classifying an information request" do
         end
 
         it 'should ask whether the request is old and unclassified' do
+            session[:user_id] = users(:silly_name_user).id
             @dog_request.should_receive(:is_old_unclassified?)
             post_status('rejected')
         end
@@ -1319,7 +1318,7 @@ describe RequestController, "when classifying an information request" do
 
                 it 'should classify the request' do
                     @dog_request.stub!(:calculate_status).and_return('rejected')
-                    @dog_request.should_receive(:set_described_state).with('rejected')
+                    @dog_request.should_receive(:set_described_state).with('rejected', users(:silly_name_user), nil)
                     post_status('rejected')
                 end
 
@@ -1347,6 +1346,26 @@ describe RequestController, "when classifying an information request" do
                     flash[:notice].should == 'Thank you for updating this request!'
                 end
 
+                context "playing the classification game" do
+                    before :each do
+                        session[:request_game] = true
+                    end
+
+                    it "should continue the game after classifying a request" do
+                        post_status("rejected")
+                        flash[:notice].should =~ /There are some more requests below for you to classify/
+                        response.should redirect_to play_url
+                    end
+                end
+
+                it "should send a mail from the user who changed the state to requires_admin" do
+                    post :describe_state, :incoming_message => { :described_state => "requires_admin", :message => "a message" }, :id => @dog_request.id, :incoming_message_id => incoming_messages(:useless_incoming_message), :last_info_request_event_id => @dog_request.last_event_id_needing_description
+
+                    deliveries = ActionMailer::Base.deliveries
+                    deliveries.size.should == 1
+                    mail = deliveries[0]
+                    mail.from_addrs.first.to_s.should == users(:silly_name_user).name_and_email
+                end
             end
         end
 
@@ -1362,7 +1381,7 @@ describe RequestController, "when classifying an information 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')
+                @dog_request.should_receive(:set_described_state).with('rejected', @admin_user, nil)
                 post_status('rejected')
             end
 
@@ -1413,7 +1432,7 @@ describe RequestController, "when classifying an information 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')
+                @dog_request.should_receive(:set_described_state).with('rejected', @admin_user, nil)
                 post_status('rejected')
             end
 
@@ -1448,6 +1467,22 @@ describe RequestController, "when classifying an information request" do
                 @dog_request.stub!(:each).and_return([@dog_request])
             end
 
+            it "should let you know when you forget to select a status" do
+                post :describe_state, :id => @dog_request.id,
+                                      :last_info_request_event_id => @dog_request.last_event_id_needing_description
+                response.should redirect_to request_url(@dog_request)
+                flash[:error].should == _("Please choose whether or not you got some of the information that you wanted.")
+            end
+
+            it "should not change the status if the request has changed while viewing it" do
+                @dog_request.stub!(:last_event_id_needing_description).and_return(2)
+
+                post :describe_state, :incoming_message => { :described_state => "rejected" },
+                    :id => @dog_request.id, :last_info_request_event_id => 1
+                response.should redirect_to request_url(@dog_request)
+                flash[:error].should =~ /The request has been updated since you originally loaded this page/
+            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)
@@ -1468,22 +1503,34 @@ describe RequestController, "when classifying an information request" do
                 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 go to the page asking for more information 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
+                response.should redirect_to describe_state_message_url(:url_title => @dog_request.url_title, :described_state => "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'
+                @dog_request.described_state.should_not == 'requires_admin'
+
+                ActionMailer::Base.deliveries.should be_empty
+            end
+
+            context "message is included when classifying as requires_admin" do
+                it "should send an email including the message" do
+                    post :describe_state,
+                        :incoming_message => {
+                            :described_state => "requires_admin",
+                            :message => "Something weird happened" },
+                        :id => @dog_request.id,
+                        :last_info_request_event_id => @dog_request.last_event_id_needing_description
 
-                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
+                    deliveries = ActionMailer::Base.deliveries
+                    deliveries.size.should == 1
+                    mail = deliveries[0]
+                    mail.body.should =~ /as needing admin/
+                    mail.body.should =~ /Something weird happened/
+                end
             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/)
@@ -1594,12 +1641,22 @@ describe RequestController, "when classifying an information request" 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")
+            it 'should redirect to the "request url" when status is updated to "requires admin"' do
+                post :describe_state, :incoming_message => {
+                                          :described_state => 'requires_admin',
+                                          :message => "A message" },
+                                      :id => @dog_request.id,
+                                      :last_info_request_event_id => @dog_request.last_event_id_needing_description
+                response.should redirect_to show_request_url(:url_title => @dog_request.url_title)
             end
 
-            it 'should redirect to the "help general url" when status is updated to "error message"' do
-                expect_redirect('error_message', "help/contact")
+            it 'should redirect to the "request url" when status is updated to "error message"' do
+                post :describe_state, :incoming_message => {
+                                          :described_state => 'error_message',
+                                          :message => "A message" },
+                                      :id => @dog_request.id,
+                                      :last_info_request_event_id => @dog_request.last_event_id_needing_description
+                response.should redirect_to show_request_url(:url_title => @dog_request.url_title)
             end
 
             it 'should redirect to the "respond to last url url" when status is updated to "user_withdrawn"' do
diff --git a/spec/integration/request_controller_spec.rb b/spec/integration/request_controller_spec.rb
new file mode 100644
index 000000000..24667bdf1
--- /dev/null
+++ b/spec/integration/request_controller_spec.rb
@@ -0,0 +1,41 @@
+# -*- coding: utf-8 -*-
+require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')
+
+describe RequestController, "when classifying an information request" do
+
+    describe 'when the request is internal' do
+
+        before(:each) do
+            @dog_request = info_requests(:fancy_dog_request)
+            # This should happen automatically before each test but doesn't with these integration
+            # tests for some reason.
+            ActionMailer::Base.deliveries = []
+        end
+
+        describe 'when logged in as the requestor' do
+
+            before :each do
+                @request_owner = @dog_request.user
+                visit signin_path
+                fill_in "Your e-mail:", :with => @request_owner.email
+                fill_in "Password:", :with => "jonespassword"
+                click_button "Sign in"
+            end
+
+            it "should send an email including the message" do
+                visit describe_state_message_path(:url_title => @dog_request.url_title,
+                    :described_state => "requires_admin")
+                fill_in "Please tell us more:", :with => "Okay. I don't quite understand."
+                click_button "Submit status and send message"
+
+                response.should contain "Thank you! We'll look into what happened and try and fix it up."
+
+                deliveries = ActionMailer::Base.deliveries
+                deliveries.size.should == 1
+                mail = deliveries[0]
+                mail.body.should =~ /as needing admin/
+                mail.body.should =~ /Okay. I don't quite understand./                
+            end
+        end
+    end
+end
diff --git a/spec/lib/ability_spec.rb b/spec/lib/ability_spec.rb
new file mode 100644
index 000000000..f075d0f32
--- /dev/null
+++ b/spec/lib/ability_spec.rb
@@ -0,0 +1,51 @@
+require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')
+
+describe Ability do
+  describe ".can_update_request_state?" do
+    context "old and unclassified request" do
+      let(:request) { mock_model(InfoRequest, :is_old_unclassified? => true) }
+
+      context "logged out" do
+        let(:user) { nil }
+        before(:each) { request.stub!(:is_owning_user?).and_return(false) }
+        it { Ability::can_update_request_state?(user, request).should be_false }
+      end
+
+      context "logged in but not owner of request" do
+        let(:user) { mock_model(User) }
+        before(:each) { request.stub!(:is_owning_user?).and_return(false) }
+
+        it { Ability::can_update_request_state?(user, request).should be_true }
+      end
+    end
+
+    context "new request" do
+      let(:request) { mock_model(InfoRequest, :is_old_unclassified? => false) }
+
+      context "logged out" do
+        let(:user) { nil }
+        before(:each) { request.stub!(:is_owning_user?).and_return(false) }
+
+        it { Ability::can_update_request_state?(user, request).should be_false }
+      end
+
+      context "logged in" do
+        let(:user) { mock_model(User) }
+
+        # An owner of a request can also be someone with admin powers
+        context "as owner of request" do
+          before(:each) { request.stub!(:is_owning_user?).and_return(true) }
+
+          it { Ability::can_update_request_state?(user, request).should be_true }
+        end
+
+        context "but not owner of request" do
+          before(:each) { request.stub!(:is_owning_user?).and_return(false) }
+
+          it { Ability::can_update_request_state?(user, request).should be_false }
+        end
+      end
+    end
+  end
+end
+
diff --git a/spec/models/info_request_spec.rb b/spec/models/info_request_spec.rb
index 544852f91..728a538f9 100644
--- a/spec/models/info_request_spec.rb
+++ b/spec/models/info_request_spec.rb
@@ -426,8 +426,8 @@ describe InfoRequest do
 
         before do
             Time.stub!(:now).and_return(Time.utc(2007, 11, 9, 23, 59))
-            @mock_comment_event = safe_mock_model(InfoRequestEvent, :created_at => Time.now - 23.days, :event_type => 'comment')
-            @mock_response_event = safe_mock_model(InfoRequestEvent, :created_at => Time.now - 22.days, :event_type => 'response')
+            @mock_comment_event = safe_mock_model(InfoRequestEvent, :created_at => Time.now - 23.days, :event_type => 'comment', :response? => false)
+            @mock_response_event = safe_mock_model(InfoRequestEvent, :created_at => Time.now - 22.days, :event_type => 'response', :response? => true)
             @info_request = InfoRequest.new(:prominence => 'normal',
                                             :awaiting_description => true,
                                             :info_request_events => [@mock_response_event, @mock_comment_event])
diff --git a/spec/models/request_mailer_spec.rb b/spec/models/request_mailer_spec.rb
index dd5a322fb..40133eedc 100644
--- a/spec/models/request_mailer_spec.rb
+++ b/spec/models/request_mailer_spec.rb
@@ -28,7 +28,7 @@ describe RequestMailer, " when receiving incoming mail" do
         receive_incoming_mail('incoming-request-plain.email', 'dummy@localhost')
         ir.incoming_messages.size.should == 1
         InfoRequest.holding_pen_request.incoming_messages.size.should == 1
-        last_event = InfoRequest.holding_pen_request.incoming_messages[0].info_request.get_last_event
+        last_event = InfoRequest.holding_pen_request.incoming_messages[0].info_request.info_request_events.last
         last_event.params[:rejected_reason].should == "Could not identify the request from the email address"
 
         deliveries = ActionMailer::Base.deliveries
@@ -48,7 +48,7 @@ describe RequestMailer, " when receiving incoming mail" do
         receive_incoming_mail('incoming-request-plain.email', ir.incoming_email, "")
         ir.incoming_messages.size.should == 1
         InfoRequest.holding_pen_request.incoming_messages.size.should == 1
-        last_event = InfoRequest.holding_pen_request.incoming_messages[0].info_request.get_last_event
+        last_event = InfoRequest.holding_pen_request.incoming_messages[0].info_request.info_request_events.last
         last_event.params[:rejected_reason].should =~ /there is no "From" address/
 
         deliveries = ActionMailer::Base.deliveries
@@ -68,7 +68,7 @@ describe RequestMailer, " when receiving incoming mail" do
         receive_incoming_mail('incoming-request-plain.email', ir.incoming_email, "frob@nowhere.com")
         ir.incoming_messages.size.should == 1
         InfoRequest.holding_pen_request.incoming_messages.size.should == 1
-        last_event = InfoRequest.holding_pen_request.incoming_messages[0].info_request.get_last_event
+        last_event = InfoRequest.holding_pen_request.incoming_messages[0].info_request.info_request_events.last
         last_event.params[:rejected_reason].should =~ /Only the authority can reply/
 
         deliveries = ActionMailer::Base.deliveries
@@ -153,7 +153,7 @@ describe RequestMailer, " when receiving incoming mail" do
         receive_incoming_mail('incoming-request-plain.email', ir.incoming_email)
         ir.incoming_messages.size.should == 1
         InfoRequest.holding_pen_request.incoming_messages.size.should == 1 # arrives in holding pen
-        last_event = InfoRequest.holding_pen_request.incoming_messages[0].info_request.get_last_event
+        last_event = InfoRequest.holding_pen_request.incoming_messages[0].info_request.info_request_events.last
         last_event.params[:rejected_reason].should =~ /allow new responses from nobody/
 
         # should be a message to admin regarding holding pen
@@ -366,4 +366,10 @@ describe RequestMailer, 'requires_admin' do
 
         mail.body.should include('http://test.host/en/admin/request/show/123')
     end
+
+    it "body should contain the message from the user" do
+        mail = RequestMailer.deliver_requires_admin(@info_request, nil, "Something has gone wrong")
+        mail.body.should include 'Something has gone wrong'
+    end
+
 end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index e05ef75dd..33e39e6e4 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -30,8 +30,9 @@ config['REPLY_LATE_AFTER_DAYS'] = 20
 require 'fakeweb'
 FakeWeb.register_uri(:purge, %r|varnish.localdomain|, :body => "OK")
 
-# Uncomment the next line to use webrat's matchers
-#require 'webrat/integrations/rspec-rails'
+Webrat.configure do |config|
+    config.mode = :rails
+end
 
 # Use test-specific translations
 FastGettext.add_text_domain 'app', :path => File.join(File.dirname(__FILE__), 'fixtures', 'locale'), :type => :po