Add public criteria for message event access methods

get_last_response_event and get_last_outgoing_event are used in various
places to determine which events to link to, use in queries etc.
Restrict them to refer to the last publicly visible event of the
relevant type, and rename them to make that clear.

10 files changed, 145 insertions, 83 deletions

diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index a09939509..ac92801b9 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -919,7 +919,7 @@ class RequestController < ApplicationController
         @last_info_request_event_id = info_request.last_event_id_needing_description
         @new_responses_count = info_request.events_needing_description.select {|i| i.event_type == 'response'}.size
         # For send followup link at bottom
-        @last_response = info_request.get_last_response
+        @last_response = info_request.get_last_public_response
     end
 
     def make_request_zip(info_request, file_path)
diff --git a/app/helpers/link_to_helper.rb b/app/helpers/link_to_helper.rb
index 5533402c5..8df28f350 100755
--- a/app/helpers/link_to_helper.rb
+++ b/app/helpers/link_to_helper.rb
@@ -53,7 +53,7 @@ module LinkToHelper
 
     # Respond to request
     def respond_to_last_url(info_request, options = {})
-        last_response = info_request.get_last_response
+        last_response = info_request.get_last_public_response
         if last_response.nil?
             show_response_no_followup_url(options.merge(:id => info_request.id))
         else
diff --git a/app/mailers/request_mailer.rb b/app/mailers/request_mailer.rb
index 4dbce6738..13b3bc4a1 100644
--- a/app/mailers/request_mailer.rb
+++ b/app/mailers/request_mailer.rb
@@ -347,8 +347,8 @@ class RequestMailer < ApplicationMailer
                                                           :age_in_days => days_since)
 
         for info_request in info_requests
-            alert_event_id = info_request.get_last_response_event_id
-            last_response_message = info_request.get_last_response
+            alert_event_id = info_request.get_last_public_response_event_id
+            last_response_message = info_request.get_last_public_response
             if alert_event_id.nil?
                 raise "internal error, no last response while making alert new response reminder, request id " + info_request.id.to_s
             end
@@ -373,8 +373,8 @@ class RequestMailer < ApplicationMailer
     def self.alert_not_clarified_request()
         info_requests = InfoRequest.find(:all, :conditions => [ "awaiting_description = ? and described_state = 'waiting_clarification' and info_requests.updated_at < ?", false, Time.now() - 3.days ], :include => [ :user ], :order => "info_requests.id" )
         for info_request in info_requests
-            alert_event_id = info_request.get_last_response_event_id
-            last_response_message = info_request.get_last_response
+            alert_event_id = info_request.get_last_public_response_event_id
+            last_response_message = info_request.get_last_public_response
             if alert_event_id.nil?
                 raise "internal error, no last response while making alert not clarified reminder, request id " + info_request.id.to_s
             end
diff --git a/app/models/info_request.rb b/app/models/info_request.rb
index fe0c94056..847a57ef4 100644
--- a/app/models/info_request.rb
+++ b/app/models/info_request.rb
@@ -733,28 +733,30 @@ public
         self.info_request_events.create!(:event_type => type, :params => params)
     end
 
-    def response_events
-        self.info_request_events.select{|e| e.response?}
+    def public_response_events
+        self.info_request_events.select{|e| e.response? && e.incoming_message.all_can_view? }
     end
 
-    # The last response is the default one people might want to reply to
-    def get_last_response_event_id
-        get_last_response_event.id if get_last_response_event
+    # The last public response is the default one people might want to reply to
+    def get_last_public_response_event_id
+        get_last_public_response_event.id if get_last_public_response_event
     end
-    def get_last_response_event
-        response_events.last
+
+    def get_last_public_response_event
+        public_response_events.last
     end
-    def get_last_response
-        get_last_response_event.incoming_message if get_last_response_event
+
+    def get_last_public_response
+        get_last_public_response_event.incoming_message if get_last_public_response_event
     end
 
-    def outgoing_events
-        info_request_events.select{|e| e.outgoing? }
+    def public_outgoing_events
+        info_request_events.select{|e| e.outgoing? && e.outgoing_message.all_can_view? }
     end
 
-    # The last outgoing message
-    def get_last_outgoing_event
-        outgoing_events.last
+    # The last public outgoing message
+    def get_last_public_outgoing_event
+        public_outgoing_events.last
     end
 
     # Text from the the initial request, for use in summary display
@@ -989,8 +991,8 @@ public
     end
 
     def is_old_unclassified?
-        !is_external? && awaiting_description && url_title != 'holding_pen' && get_last_response_event &&
-            Time.now > get_last_response_event.created_at + OLD_AGE_IN_DAYS
+        !is_external? && awaiting_description && url_title != 'holding_pen' && get_last_public_response_event &&
+            Time.now > get_last_public_response_event.created_at + OLD_AGE_IN_DAYS
     end
 
     # List of incoming messages to followup, by unique email
diff --git a/app/views/admin_general/index.html.erb b/app/views/admin_general/index.html.erb
index b239a2b3f..976860fa7 100644
--- a/app/views/admin_general/index.html.erb
+++ b/app/views/admin_general/index.html.erb
@@ -164,7 +164,7 @@
                     <%= request_both_links(@request) %>
                   </td>
                   <td class="span2">
-                    <%=simple_date(@request.get_last_response_event.created_at)%>
+                    <%=simple_date(@request.get_last_public_response_event.created_at)%>
                   </td>
                 </tr>
             <% end %>
diff --git a/spec/controllers/admin_request_controller_spec.rb b/spec/controllers/admin_request_controller_spec.rb
index 42b4bcbc1..c374ff90d 100644
--- a/spec/controllers/admin_request_controller_spec.rb
+++ b/spec/controllers/admin_request_controller_spec.rb
@@ -77,7 +77,7 @@ describe AdminRequestController, "when administering the holding pen" do
         ir.handle_rejected_responses = 'holding_pen'
         ir.save!
         receive_incoming_mail('incoming-request-plain.email', ir.incoming_email, "frob@nowhere.com")
-        get :show_raw_email, :id => InfoRequest.holding_pen_request.get_last_response.raw_email.id
+        get :show_raw_email, :id => InfoRequest.holding_pen_request.get_last_public_response.raw_email.id
         response.should contain "Only the authority can reply to this request"
     end
 
@@ -88,7 +88,8 @@ describe AdminRequestController, "when administering the holding pen" do
         ir.save!
         mail_to = "request-#{ir.id}-asdfg@example.com"
         receive_incoming_mail('incoming-request-plain.email', mail_to)
-        interesting_email = InfoRequest.holding_pen_request.get_last_response.raw_email.id
+        interesting_email = InfoRequest.holding_pen_request.get_last_public_response
+.raw_email.id
         # now we add another message to the queue, which we're not interested in
         receive_incoming_mail('incoming-request-plain.email', ir.incoming_email, "")
         InfoRequest.holding_pen_request.incoming_messages.length.should == 2
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index c5ee8cbf7..ec10d99d8 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -1596,7 +1596,7 @@ describe RequestController, "when classifying an information request" do
                 @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_public_response_event.should == info_request_events(:useless_incoming_message_event)
                 @dog_request.info_request_events.last.event_type.should == "status_update"
                 @dog_request.info_request_events.last.calculated_state.should == 'rejected'
             end
@@ -1749,13 +1749,13 @@ describe RequestController, "when classifying an information request" do
 
                 it 'should redirect to the "response url" when there is a last response' do
                     incoming_message = mock_model(IncomingMessage)
-                    @dog_request.stub!(:get_last_response).and_return(incoming_message)
+                    @dog_request.stub!(:get_last_public_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 there are no events
                     needing description' do
-                    @dog_request.stub!(:get_last_response).and_return(nil)
+                    @dog_request.stub!(:get_last_public_response).and_return(nil)
                     expect_redirect('waiting_clarification', "request/#{@dog_request.id}/response")
                 end
 
@@ -1794,7 +1794,7 @@ describe RequestController, "when classifying an information request" do
             context 'when status is updated to "gone postal"' do
 
                 it 'should redirect to the "respond to last url"' do
-                    expect_redirect('gone_postal', "request/#{@dog_request.id}/response/#{@dog_request.get_last_response.id}?gone_postal=1")
+                    expect_redirect('gone_postal', "request/#{@dog_request.id}/response/#{@dog_request.get_last_public_response.id}?gone_postal=1")
                 end
 
             end
@@ -1836,7 +1836,7 @@ describe RequestController, "when classifying an information request" do
             context 'when status is updated to "user_withdrawn"' do
 
                 it 'should redirect to the "respond to last url url" ' do
-                    expect_redirect('user_withdrawn', "request/#{@dog_request.id}/response/#{@dog_request.get_last_response.id}")
+                    expect_redirect('user_withdrawn', "request/#{@dog_request.id}/response/#{@dog_request.get_last_public_response.id}")
                 end
 
             end
@@ -1889,7 +1889,7 @@ describe RequestController, "when sending a followup message" do
         # fake that this is a clarification
         info_requests(:fancy_dog_request).set_described_state('waiting_clarification')
         info_requests(:fancy_dog_request).described_state.should == 'waiting_clarification'
-        info_requests(:fancy_dog_request).get_last_response_event.calculated_state.should == 'waiting_clarification'
+        info_requests(:fancy_dog_request).get_last_public_response_event.calculated_state.should == 'waiting_clarification'
 
         # make the followup
         session[:user_id] = users(:bob_smith_user).id
@@ -1907,7 +1907,7 @@ describe RequestController, "when sending a followup message" do
         # and that the status changed
         info_requests(:fancy_dog_request).reload
         info_requests(:fancy_dog_request).described_state.should == 'waiting_response'
-        info_requests(:fancy_dog_request).get_last_response_event.calculated_state.should == 'waiting_clarification'
+        info_requests(:fancy_dog_request).get_last_public_response_event.calculated_state.should == 'waiting_clarification'
     end
 
     it "should give an error if the same followup is submitted twice" do
diff --git a/spec/mailers/request_mailer_spec.rb b/spec/mailers/request_mailer_spec.rb
index 23806b35b..f30beae82 100644
--- a/spec/mailers/request_mailer_spec.rb
+++ b/spec/mailers/request_mailer_spec.rb
@@ -204,10 +204,10 @@ describe RequestMailer, "when sending reminders to requesters to classify a resp
     before do
         Time.stub!(:now).and_return(Time.utc(2007, 11, 12, 23, 59))
         @mock_event = mock_model(InfoRequestEvent)
-        @mock_response = mock_model(IncomingMessage)
+        @mock_response = mock_model(IncomingMessage, :user_can_view? => true)
         @mock_user = mock_model(User)
-        @mock_request = mock_model(InfoRequest, :get_last_response_event_id => @mock_event.id,
-                                                :get_last_response => @mock_response,
+        @mock_request = mock_model(InfoRequest, :get_last_public_response_event_id => @mock_event.id,
+                                                :get_last_public_response => @mock_response,
                                                 :user_id => 2,
                                                 :url_title => 'test_title',
                                                 :user => @mock_user)
@@ -252,7 +252,7 @@ describe RequestMailer, "when sending reminders to requesters to classify a resp
     end
 
     it 'should raise an error if a request does not have a last response event id' do
-        @mock_request.stub!(:get_last_response_event_id).and_return(nil)
+        @mock_request.stub!(:get_last_public_response_event_id).and_return(nil)
         expected_message = "internal error, no last response while making alert new response reminder, request id #{@mock_request.id}"
         lambda{ send_alerts }.should raise_error(expected_message)
     end
@@ -289,7 +289,7 @@ describe RequestMailer, "when sending reminders to requesters to classify a resp
             mock_sent_alert.should_receive(:info_request=).with(@mock_request)
             mock_sent_alert.should_receive(:user=).with(@mock_user)
             mock_sent_alert.should_receive(:alert_type=).with('new_response_reminder_1')
-            mock_sent_alert.should_receive(:info_request_event_id=).with(@mock_request.get_last_response_event_id)
+            mock_sent_alert.should_receive(:info_request_event_id=).with(@mock_request.get_last_public_response_event_id)
             mock_sent_alert.should_receive(:save!)
             send_alerts
         end
diff --git a/spec/models/info_request_spec.rb b/spec/models/info_request_spec.rb
index 2846fa6dc..a6f877b20 100644
--- a/spec/models/info_request_spec.rb
+++ b/spec/models/info_request_spec.rb
@@ -451,8 +451,14 @@ describe InfoRequest do
 
         before do
             Time.stub!(:now).and_return(Time.utc(2007, 11, 9, 23, 59))
-            @mock_comment_event = mock_model(InfoRequestEvent, :created_at => Time.now - 23.days, :event_type => 'comment', :response? => false)
-            @mock_response_event = mock_model(InfoRequestEvent, :created_at => Time.now - 22.days, :event_type => 'response', :response? => true)
+            @mock_comment_event = mock_model(InfoRequestEvent, :created_at => Time.now - 23.days,
+                                                               :event_type => 'comment',
+                                                               :response? => false)
+            mock_incoming_message = mock_model(IncomingMessage, :all_can_view? => true)
+            @mock_response_event = mock_model(InfoRequestEvent, :created_at => Time.now - 22.days,
+                                                                :event_type => 'response',
+                                                                :response? => true,
+                                                                :incoming_message => mock_incoming_message)
             @info_request = InfoRequest.new(:prominence => 'normal',
                                             :awaiting_description => true,
                                             :info_request_events => [@mock_response_event, @mock_comment_event])
@@ -589,6 +595,59 @@ describe InfoRequest do
         end
     end
 
+    describe 'when asked for the last public response event' do
+
+        before do
+            @info_request = FactoryGirl.create(:info_request_with_incoming)
+            @incoming_message = @info_request.incoming_messages.first
+        end
+
+        it 'should not return an event with a hidden prominence message' do
+            @incoming_message.prominence = 'hidden'
+            @incoming_message.save!
+            @info_request.get_last_public_response_event.should == nil
+        end
+
+        it 'should not return an event with a requester_only prominence message' do
+            @incoming_message.prominence = 'requester_only'
+            @incoming_message.save!
+            @info_request.get_last_public_response_event.should == nil
+        end
+
+        it 'should return an event with a normal prominence message' do
+            @incoming_message.prominence = 'normal'
+            @incoming_message.save!
+            @info_request.get_last_public_response_event.should == @incoming_message.response_event
+        end
+    end
+
+    describe 'when asked for the last public outgoing event' do
+
+        before do
+            @info_request = FactoryGirl.create(:info_request)
+            @outgoing_message = @info_request.outgoing_messages.first
+        end
+
+        it 'should not return an event with a hidden prominence message' do
+            @outgoing_message.prominence = 'hidden'
+            @outgoing_message.save!
+            @info_request.get_last_public_outgoing_event.should == nil
+        end
+
+        it 'should not return an event with a requester_only prominence message' do
+            @outgoing_message.prominence = 'requester_only'
+            @outgoing_message.save!
+            @info_request.get_last_public_outgoing_event.should == nil
+        end
+
+        it 'should return an event with a normal prominence message' do
+            @outgoing_message.prominence = 'normal'
+            @outgoing_message.save!
+            @info_request.get_last_public_outgoing_event.should == @outgoing_message.info_request_events.first
+        end
+
+    end
+
     describe  'when generating json for the api' do
 
         before do
diff --git a/spec/views/request/show.html.erb_spec.rb b/spec/views/request/show.html.erb_spec.rb
index 4578268b2..6e63b9b43 100644
--- a/spec/views/request/show.html.erb_spec.rb
+++ b/spec/views/request/show.html.erb_spec.rb
@@ -1,8 +1,8 @@
 require File.expand_path(File.join('..', '..', '..', 'spec_helper'), __FILE__)
 
 describe 'request/show' do
-    
-    before do 
+
+    before do
         @mock_body = mock_model(PublicBody, :name => 'test body',
                                             :url_name => 'test_body',
                                             :is_school? => false)
@@ -10,101 +10,101 @@ describe 'request/show' do
                                       :url_name => 'test_user',
                                       :profile_photo => nil)
         @mock_request = mock_model(InfoRequest, :title => 'test request',
-                                                :awaiting_description => false, 
+                                                :awaiting_description => false,
                                                 :law_used_with_a => 'A Freedom of Information request',
                                                 :law_used_full => 'Freedom of Information',
                                                 :public_body => @mock_body,
-                                                :user => @mock_user, 
-                                                :user_name => @mock_user.name, 
+                                                :user => @mock_user,
+                                                :user_name => @mock_user.name,
                                                 :is_external? => false,
-                                                :calculate_status => 'waiting_response', 
+                                                :calculate_status => 'waiting_response',
                                                 :date_response_required_by => Date.today,
                                                 :prominence => 'normal',
                                                 :comments_allowed? => true,
                                                 :all_can_view? => true,
                                                 :url_title => 'test_request')
     end
-    
+
     def request_page
         assign :info_request, @mock_request
         assign :info_request_events, []
         assign :status, @mock_request.calculate_status
         render
     end
-    
-    describe 'when a status update has been requested' do 
-        
-        before do 
+
+    describe 'when a status update has been requested' do
+
+        before do
             assign :update_status, true
         end
-        
+
         it 'should show the first form for describing the state of the request' do
             request_page
             response.should have_selector("div.describe_state_form#describe_state_form_1")
-        end    
-        
+        end
+
     end
-    
-    describe 'when it is awaiting a description' do 
-    
-        before do 
+
+    describe 'when it is awaiting a description' do
+
+        before do
             @mock_request.stub!(:awaiting_description).and_return(true)
         end
-        
+
         it 'should show the first form for describing the state of the request' do
             request_page
             response.should have_selector("div.describe_state_form#describe_state_form_1")
         end
-        
-        it 'should show the second form for describing the state of the request' do 
+
+        it 'should show the second form for describing the state of the request' do
             request_page
             response.should have_selector("div.describe_state_form#describe_state_form_2")
         end
-    
+
     end
-    
-    describe 'when the user is the request owner' do 
-    
-        before do 
+
+    describe 'when the user is the request owner' do
+
+        before do
             assign :is_owning_user, true
         end
-        
-        describe 'when the request status is "waiting clarification"' do 
-    
-            before do 
+
+        describe 'when the request status is "waiting clarification"' do
+
+            before do
                 @mock_request.stub!(:calculate_status).and_return('waiting_clarification')
             end
-        
-            describe 'when there is a last response' do 
-            
+
+            describe 'when there is a last response' do
+
                 before do
                     @mock_response = mock_model(IncomingMessage)
-                    @mock_request.stub!(:get_last_response).and_return(@mock_response)
+                    @mock_request.stub!(:get_last_public_response).and_return(@mock_response)
                 end
 
-            
-                it 'should show a link to follow up the last response with clarification' do 
+
+                it 'should show a link to follow up the last response with clarification' do
                     request_page
                     expected_url = "/en/request/#{@mock_request.id}/response/#{@mock_response.id}#followup"
                     response.should have_selector("a", :href => expected_url, :content => 'send a follow up message')
                 end
-            
+
             end
-    
+
             describe 'when there is no last response' do
-        
-                before do 
-                    @mock_request.stub!(:get_last_response).and_return(nil)
+
+                before do
+                    @mock_request.stub!(:get_last_public_response).and_return(nil)
                 end
 
-            
-                it 'should show a link to follow up the request without reference to a specific response' do 
+
+                it 'should show a link to follow up the request without reference to a specific response' do
                     request_page
                     expected_url = "/en/request/#{@mock_request.id}/response#followup"
                     response.should have_selector("a", :href => expected_url, :content => 'send a follow up message')
                 end
             end
         end
-    
+
     end
 end