Remove duplication from new correspondence urls

5 files changed, 42 insertions, 19 deletions

diff --git a/app/helpers/link_to_helper.rb b/app/helpers/link_to_helper.rb
index 9f63ec583..e5ef2ca39 100755
--- a/app/helpers/link_to_helper.rb
+++ b/app/helpers/link_to_helper.rb
@@ -28,23 +28,19 @@ module LinkToHelper
 
     # Incoming / outgoing messages
     def incoming_message_url(incoming_message, options = {})
-        default_options = { :anchor => "incoming-#{ incoming_message.id }",
-                            :nocache => "incoming-#{ incoming_message.id }" }
-        request_url(incoming_message.info_request, options.merge(default_options))
+        message_url(incoming_message, options)
     end
 
     def incoming_message_path(incoming_message)
-        incoming_message_url(incoming_message, :only_path => true)
+        message_path(incoming_message)
     end
 
     def outgoing_message_url(outgoing_message, options = {})
-        default_options = { :anchor => "outgoing-#{ outgoing_message.id }",
-                            :nocache => "outgoing-#{ outgoing_message.id }" }
-        request_url(outgoing_message.info_request, options.merge(default_options))
+        message_url(outgoing_message, options)
     end
 
     def outgoing_message_path(outgoing_message)
-        outgoing_message_url(outgoing_message, :only_path => true)
+        message_path(outgoing_message)
     end
 
     def comment_url(comment, options = {})
@@ -352,5 +348,24 @@ module LinkToHelper
         return url_for(params)
     end
 
+    private
+
+    # Private: Generate a request_url linking to the new correspondence
+    def message_url(message, options = {})
+        message_type = message.class.to_s.gsub('Message', '').downcase
+
+        default_options = { :anchor => "#{ message_type }-#{ message.id }" }
+
+        if options.delete(:cachebust)
+            default_options.merge!(:nocache => "#{ message_type }-#{ message.id }")
+        end
+
+        request_url(message.info_request, options.merge(default_options))
+    end
+
+    def message_path(message)
+      message_url(message, :only_path => true)
+    end
+
 end
 
diff --git a/app/mailers/request_mailer.rb b/app/mailers/request_mailer.rb
index 1fd5b9ba7..dd6472c1d 100644
--- a/app/mailers/request_mailer.rb
+++ b/app/mailers/request_mailer.rb
@@ -71,7 +71,7 @@ class RequestMailer < ApplicationMailer
     def new_response(info_request, incoming_message)
         # Don't use login link here, just send actual URL. This is
         # because people tend to forward these emails amongst themselves.
-        @url = incoming_message_url(incoming_message)
+        @url = incoming_message_url(incoming_message, :cachebust => true)
         @incoming_message, @info_request = incoming_message, info_request
 
         headers('Return-Path' => blackhole_email,
diff --git a/app/views/track_mailer/event_digest.text.erb b/app/views/track_mailer/event_digest.text.erb
index b83c184f0..a154f430f 100644
--- a/app/views/track_mailer/event_digest.text.erb
+++ b/app/views/track_mailer/event_digest.text.erb
@@ -17,14 +17,14 @@
 
                 # e.g. Julian Burgess sent a request to Royal Mail Group (15 May 2008)
                 if event.event_type == 'response'
-                    url = incoming_message_url(event.incoming_message)
+                    url = incoming_message_url(event.incoming_message, :cachebust => true)
                     main_text += _("{{public_body}} sent a response to {{user_name}}", :public_body => event.info_request.public_body.name, :user_name => event.info_request.user_name)
                 elsif event.event_type == 'followup_sent'
-                    url = outgoing_message_url(event.outgoing_message)
+                    url = outgoing_message_url(event.outgoing_message, :cachebust => true)
                     main_text += _("{{user_name}} sent a follow up message to {{public_body}}", :user_name => event.info_request.user_name, :public_body => event.info_request.public_body.name)
                 elsif event.event_type == 'sent'
                     # this is unlikely to happen in real life, but happens in the test code
-                    url = outgoing_message_url(event.outgoing_message)
+                    url = outgoing_message_url(event.outgoing_message, :cachebust => true)
                     main_text += _("{{user_name}} sent a request to {{public_body}}", :user_name => event.info_request.user_name, :public_body => event.info_request.public_body.name)
                 elsif event.event_type == 'comment'
                     url = comment_url(event.comment)
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index bde1c136a..070511fb0 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -567,7 +567,7 @@ describe RequestController, "when showing one request" do
             get :get_attachment_as_html, :incoming_message_id => im.id, :id => ir.id, :part => 5, :file_name => 'hello world.txt', :skip_cache => 1
             response.status.should == 303
             new_location = response.header['Location']
-            new_location.should match(/request\/#{ir.url_title}\?nocache=incoming-#{im.id}#incoming-#{im.id}/)
+            new_location.should match(/request\/#{ir.url_title}#incoming-#{im.id}/)
         end
 
         it "should find a uniquely named filename even if the URL part number was wrong" do
diff --git a/spec/helpers/link_to_helper_spec.rb b/spec/helpers/link_to_helper_spec.rb
index 08362da6e..f7be9eab0 100644
--- a/spec/helpers/link_to_helper_spec.rb
+++ b/spec/helpers/link_to_helper_spec.rb
@@ -37,8 +37,12 @@ describe LinkToHelper do
                 incoming_message_url(@incoming_message).should include('#incoming-32')
             end
 
-            it 'includes a cache busting parameter' do
-                incoming_message_url(@incoming_message).should include('nocache=incoming-32')
+            it 'includes does not cache by default' do
+                incoming_message_url(@incoming_message).should_not include('nocache=incoming-32')
+            end
+
+            it 'includes a cache busting parameter if set' do
+                incoming_message_url(@incoming_message, :cachebust => true).should include('nocache=incoming-32')
             end
 
         end
@@ -46,7 +50,7 @@ describe LinkToHelper do
         context 'for internal links' do
 
             it 'generates the incoming_message_url with the path only' do
-                expected = '/request/test_title?nocache=incoming-32#incoming-32'
+                expected = '/request/test_title#incoming-32'
                 incoming_message_path(@incoming_message).should == expected
             end
 
@@ -71,8 +75,12 @@ describe LinkToHelper do
                 outgoing_message_url(@outgoing_message).should include('#outgoing-32')
             end
 
-            it 'includes a cache busting parameter' do
-                outgoing_message_url(@outgoing_message).should include('nocache=outgoing-32')
+            it 'includes does not cache by default' do
+                outgoing_message_url(@outgoing_message).should_not include('nocache=outgoing-32')
+            end
+
+            it 'includes a cache busting parameter if set' do
+                outgoing_message_url(@outgoing_message, :cachebust => true).should include('nocache=outgoing-32')
             end
 
         end
@@ -80,7 +88,7 @@ describe LinkToHelper do
         context 'for internal links' do
 
             it 'generates the outgoing_message_url with the path only' do
-                expected = '/request/test_title?nocache=outgoing-32#outgoing-32'
+                expected = '/request/test_title#outgoing-32'
                 outgoing_message_path(@outgoing_message).should == expected
             end