Change attachment caching code to lower level functions.

Make mime types the same for cached and uncached images.
Store images from PDF/DOC etc. extraction in correct cache place.
Make sure such images are authenticated with permissions to view request.

app/controllers/request_controller.rb

@@ -4,7 +4,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: request_controller.rb,v 1.164 2009-06-30 14:28:25 francis Exp $
+# $Id: request_controller.rb,v 1.165 2009-07-01 11:07:19 francis Exp $
 
 class RequestController < ApplicationController
 
@@ -507,32 +507,37 @@ def authenticate_attachment
         end
     end
 
-    # use :cache_path here so domain doesn't appear in filename, as /admin
-    # interface runs on separate domain (so can use HTTPS without its own
-    # certificate) in mySociety server configuration.
-    caches_action :get_attachment, :cache_path => { :only_path => true }
-    def get_attachment
-        if !get_attachment_internal
+    # special caching code so mime types are handled right
+    around_filter :cache_attachments, :only => [ :get_attachment, :get_attachment_as_html ]
+    def cache_attachments
+        key = params.merge(:only_path => true)
+        if cached = read_fragment(key)
+            IncomingMessage # load global filename_to_mimetype XXX should move filename_to_mimetype to proper namespace
+            response.content_type = filename_to_mimetype(params[:file_name].join("/")) or 'application/octet-stream'
+            render_for_text(cached)
             return
         end
 
-        response.content_type = 'application/octet-stream'
-        if !@attachment.content_type.nil?
-            # Hmm, this is a bit rubbish as when cached won't cache the content
-            # type. We try to overcome it by setting the file extension right
-            # in FOIAttachment.
-            response.content_type = @attachment.content_type
-        end
+        yield
+
+        write_fragment(key, response.body)
+    end
+
+    def get_attachment
+        get_attachment_internal
+
+        # we don't use @attachment.content_type here, as we want same mime type when cached in cache_attachments above
+        response.content_type = filename_to_mimetype(params[:file_name].join("/")) or 'application/octet-stream'
+
         render :text => @attachment.body
     end
 
-    caches_action :get_attachment_as_html, :cache_path => { :only_path => true }
     def get_attachment_as_html
-        if !get_attachment_internal
-            return
-        end
+        get_attachment_internal
 
-        image_dir = File.dirname(Rails.public_path + url_for(params.merge(:only_path => true)))
+        # images made during conversion (e.g. images in PDF files) are put in the cache directory, so
+        # the same cache code in cache_attachments above will display them.
+        image_dir = File.dirname(ActionController::Base.cache_store.cache_path + "/views" + url_for(params.merge(:only_path => true)))
         FileUtils.mkdir_p(image_dir)
         html = @attachment.body_as_html(image_dir)
 
@@ -556,7 +561,8 @@ def get_attachment_internal
             raise sprintf("Incoming message %d does not belong to request %d", @incoming_message.info_request_id, params[:id])
         end
         @part_number = params[:part].to_i
-        @filename = params[:file_name]
+        @filename = params[:file_name].join("/")
+        @original_filename = @filename.gsub(/\.html$/, "")
 
         # check permissions
         raise "internal error, pre-auth filter should have caught this" if !@info_request.user_can_view?(authenticated_user)
@@ -569,9 +575,7 @@ def get_attachment_internal
 
         @attachment_url = get_attachment_url(:id => @incoming_message.info_request_id,
                 :incoming_message_id => @incoming_message.id, :part => @part_number,
-                :file_name => @filename )
-
-        return true
+                :file_name => @original_filename )
     end
 
     # FOI officers can upload a response

app/views/request/_bubble.rhtml

@@ -10,7 +10,7 @@
                                     :file_name => a.display_filename) 
                             attachment_as_html_url = get_attachment_as_html_url(:id => incoming_message.info_request_id,
                                     :incoming_message_id => incoming_message.id, :part => a.url_part_number, 
-                                    :file_name => a.display_filename) 
+                                    :file_name => a.display_filename + '.html') 
                         %>
                         <% img_filename = "icon_" + a.content_type.sub('/', '_') + "_large.png"
                         full_filename = File.join(File.dirname(__FILE__), "../../../public/images", img_filename)

config/environment.rb

@@ -125,6 +125,7 @@ def page_link(page, text, attributes = {})
 
 # Load monkey patches from lib/
 require 'tmail_extensions'
+require 'activesupport_cache_extensions.rb'
 
 # XXX temp debug for SQL logging production sites
 #ActiveRecord::Base.logger = Logger.new(STDOUT)

config/routes.rb

@@ -4,7 +4,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: routes.rb,v 1.88 2009-06-22 12:54:45 francis Exp $
+# $Id: routes.rb,v 1.89 2009-07-01 11:07:19 francis Exp $
 
 ActionController::Routing::Routes.draw do |map|
 
@@ -39,7 +39,7 @@
         request.describe_state   '/request/:id/describe', :action => 'describe_state'
         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.html', :action => 'get_attachment_as_html'
+        request.get_attachment_as_html   '/request/:id/response/:incoming_message_id/attach/html/:part/*file_name', :action => 'get_attachment_as_html'
         request.get_attachment   '/request/:id/response/:incoming_message_id/attach/:part/*file_name', :action => 'get_attachment'
 
         request.info_request_event '/request_event/:info_request_event_id', :action => 'show_request_event'

lib/tmail_extensions.rb

@@ -1,10 +1,10 @@
-# models/request_mailer.rb:
+# lib/tmail_extensions.rb:
 # Extensions / fixes to TMail.
 #
 # Copyright (c) 2009 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: tmail_extensions.rb,v 1.4 2009-05-21 01:18:45 francis Exp $
+# $Id: tmail_extensions.rb,v 1.5 2009-07-01 11:07:20 francis Exp $
 
 # Monkeypatch!
 

todo.txt

@@ -428,6 +428,9 @@ Have a single button to sign up to alerts on authorities for your postcode
 NHS postcode database: 
     http://www.ons.gov.uk/about-statistics/geography/products/geog-products-postcode/nhspd/index.html
 
+Make request preview have a URL so you can show it to someone else before
+sending it :)
+
 Proposed request submission queue with comments - new requests don't get sent straight
     away, but are delayed while people help improve them.
 

vendor/rails-2.1.0/activesupport/lib/active_support/cache/file_store.rb

@@ -67,4 +67,4 @@ def search_dir(dir, &callback)
         end
       end
   end
-end
+end