Rework the temporary storing of a csv file of public body info between a dry run and real load. Use a temp file, not the session for the data. Also fixes #503.

3 files changed, 175 insertions, 49 deletions

diff --git a/app/controllers/admin_public_body_controller.rb b/app/controllers/admin_public_body_controller.rb
index 25a8d1e08..7bd794d23 100644
--- a/app/controllers/admin_public_body_controller.rb
+++ b/app/controllers/admin_public_body_controller.rb
@@ -139,7 +139,9 @@ class AdminPublicBodyController < AdminController
     end
 
     def import_csv
-        if params[:csv_file]
+        @notes = ""
+        @errors = ""
+        if request.post?
             if params['commit'] == 'Dry run'
                 dry_run_only = true
             elsif params['commit'] == 'Upload'
@@ -147,40 +149,76 @@ class AdminPublicBodyController < AdminController
             else
                 raise "internal error, unknown button label"
             end
-            csv_contents = params[:csv_file].read
-        else
-            csv_contents = session.delete(:previous_csv)
-        end
-        if !csv_contents.nil?
-            # Try with dry run first
-            en = PublicBody.import_csv(csv_contents, params[:tag], params[:tag_behaviour], true, admin_http_auth_user(), I18n.available_locales)
-            errors = en[0]
-            notes = en[1]
-
-            if errors.size == 0
-                if dry_run_only
-                    notes.push("Dry run was successful, real run would do as above.")
-                    session[:previous_csv] = csv_contents
-                else
-                    # And if OK, with real run
-                    en = PublicBody.import_csv(csv_contents, params[:tag], params[:tag_behaviour], false, admin_http_auth_user(), I18n.available_locales)
-                    errors = en[0]
-                    notes = en[1]
-                    if errors.size != 0
-                        raise "dry run mismatched real run"
+            # Read file from params
+            if params[:csv_file]
+                csv_contents = params[:csv_file].read
+                @original_csv_file = params[:csv_file].original_filename
+            # or from previous dry-run temporary file
+            elsif params[:temporary_csv_file] && params[:original_csv_file]
+                csv_contents = retrieve_csv_data(params[:temporary_csv_file])
+                @original_csv_file = params[:original_csv_file]
+            end
+
+            if !csv_contents.nil?
+                # Try with dry run first
+                errors, notes = PublicBody.import_csv(csv_contents,
+                                                      params[:tag],
+                                                      params[:tag_behaviour],
+                                                      true,
+                                                      admin_http_auth_user(),
+                                                      I18n.available_locales)
+
+                if errors.size == 0
+                    if dry_run_only
+                        notes.push("Dry run was successful, real run would do as above.")
+                        # Store the csv file for ease of performing the real run
+                        @temporary_csv_file = store_csv_data(csv_contents)
+                    else
+                        # And if OK, with real run
+                        errors, notes = PublicBody.import_csv(csv_contents,
+                                                              params[:tag],
+                                                              params[:tag_behaviour],
+                                                              false,
+                                                              admin_http_auth_user(),
+                                                              I18n.available_locales)
+                        if errors.size != 0
+                            raise "dry run mismatched real run"
+                        end
+                        notes.push("Import was successful.")
                     end
-                    notes.push("Import was successful.")
                 end
+                @errors = errors.join("\n")
+                @notes = notes.join("\n")
             end
-            @errors = errors.join("\n")
-            @notes = notes.join("\n")
-        else
-            @errors = ""
-            @notes = ""
         end
-
     end
 
     private
 
+    # Save the contents to a temporary file - not using Tempfile as we need
+    # the file to persist between requests. Return the name of the file.
+    def store_csv_data(csv_contents)
+        tempfile_name = "csv_upload-#{Time.now.strftime("%Y%m%d")}-#{SecureRandom.random_number(10000)}"
+        tempfile = File.new(File.join(Dir::tmpdir, tempfile_name), 'w')
+        tempfile.write(csv_contents)
+        tempfile.close
+        return tempfile_name
+    end
+
+    # Get csv contents from the file whose name is passed, as long as the
+    # name is of the expected form.
+    # Delete the file, return the contents.
+    def retrieve_csv_data(tempfile_name)
+        if not /csv_upload-\d{8}-\d{1,5}/.match(tempfile_name)
+            raise "Invalid filename in upload_csv: #{tempfile_name}"
+        end
+        tempfile_path = File.join(Dir::tmpdir, tempfile_name)
+        if ! File.exist?(tempfile_path)
+            raise "Missing file in upload_csv: #{tempfile_name}"
+        end
+        csv_contents = File.read(tempfile_path)
+        File.delete(tempfile_path)
+        return csv_contents
+    end
+
 end
diff --git a/app/views/admin_public_body/import_csv.rhtml b/app/views/admin_public_body/import_csv.rhtml
index 8c64fede2..4eb83cc7e 100644
--- a/app/views/admin_public_body/import_csv.rhtml
+++ b/app/views/admin_public_body/import_csv.rhtml
@@ -11,8 +11,16 @@
 
 <% form_tag 'import_csv', :multipart => true do %>
     <p>
-    <label for="csv_file">CSV file:</label>
-    <%= file_field_tag :csv_file, :size => 40 %>
+    <% if @original_csv_file && @temporary_csv_file %>
+        CSV file:
+        <%= @original_csv_file %>
+        <%= hidden_field_tag :original_csv_file, @original_csv_file %>
+        <%= hidden_field_tag :temporary_csv_file, @temporary_csv_file %>
+        <%= link_to 'Clear current file', 'import_csv', :class => "btn btn-warning" %>
+    <% else %>
+        <label for="csv_file">CSV file:</label>
+        <%= file_field_tag :csv_file, :size => 40 %>
+    <% end %>
     </p>
 
     <p>
diff --git a/spec/controllers/admin_public_body_controller_spec.rb b/spec/controllers/admin_public_body_controller_spec.rb
index 55a6649b2..be33802c5 100644
--- a/spec/controllers/admin_public_body_controller_spec.rb
+++ b/spec/controllers/admin_public_body_controller_spec.rb
@@ -50,7 +50,7 @@ describe AdminPublicBodyController, "when administering public bodies" do
         response.should redirect_to(:controller=>'admin_public_body', :action=>'show', :id => id)
         PublicBody.count.should == n
     end
-    
+
     it "destroys a public body" do
         n = PublicBody.count
         post :destroy, { :id => public_bodies(:forlorn_public_body).id }
@@ -70,6 +70,86 @@ describe AdminPublicBodyController, "when administering public bodies" do
         response.should redirect_to(:action=>'list')
         PublicBody.find_by_tag("department").count.should == n
     end
+
+    describe 'import_csv' do
+
+        describe 'when handling a GET request' do
+
+            it 'should get the page successfully' do
+                get :import_csv
+                response.should be_success
+            end
+
+        end
+
+        describe 'when handling a POST request' do
+
+            before do
+                PublicBody.stub!(:import_csv).and_return([[],[]])
+                @file_object = mock("a file upload", :read => 'some contents',
+                                                     :original_filename => 'contents.txt')
+            end
+
+            it 'should handle a nil csv file param' do
+                post :import_csv, { :commit => 'Dry run' }
+                response.should be_success
+            end
+
+            describe 'if there is a csv file param' do
+
+                it 'should try to get the contents and original name of a csv file param' do
+                    @file_object.should_receive(:read).and_return('some contents')
+                    post :import_csv, { :csv_file => @file_object,
+                                        :commit => 'Dry run'}
+                end
+
+                it 'should assign the original filename to the view' do
+                    post :import_csv, { :csv_file => @file_object,
+                                        :commit => 'Dry run'}
+                    assigns[:original_csv_file].should == 'contents.txt'
+                end
+
+            end
+
+            describe 'if there is no csv file param, but there are temporary_csv_file and
+                      original_csv_file params' do
+
+                it 'should try and get the file contents from a temporary file whose name
+                    is passed as a param' do
+                    @controller.should_receive(:retrieve_csv_data).with('csv_upload-2046-12-31-394')
+                    post :import_csv, { :temporary_csv_file => 'csv_upload-2046-12-31-394',
+                                        :original_csv_file => 'original_contents.txt',
+                                        :commit => 'Dry run'}
+                end
+
+                it 'should raise an error on an invalid temp file name' do
+                    params = { :temporary_csv_file => 'bad_name',
+                               :original_csv_file => 'original_contents.txt',
+                               :commit => 'Dry run'}
+                    expected_error = "Invalid filename in upload_csv: bad_name"
+                    lambda{ post :import_csv, params }.should raise_error(expected_error)
+                end
+
+                it 'should raise an error if the temp file does not exist' do
+                    temp_name = "csv_upload-20461231-394"
+                    params = { :temporary_csv_file => temp_name,
+                               :original_csv_file => 'original_contents.txt',
+                               :commit => 'Dry run'}
+                    expected_error = "Missing file in upload_csv: csv_upload-20461231-394"
+                    lambda{ post :import_csv, params }.should raise_error(expected_error)
+                end
+
+                it 'should assign the temporary filename to the view' do
+                    post :import_csv, { :csv_file => @file_object,
+                                        :commit => 'Dry run'}
+                    temporary_filename = assigns[:temporary_csv_file]
+                    temporary_filename.should match(/csv_upload-#{Time.now.strftime("%Y%m%d")}-\d{1,5}/)
+                end
+
+            end
+
+        end
+    end
 end
 
 describe AdminPublicBodyController, "when administering public bodies and paying attention to authentication" do
@@ -79,7 +159,7 @@ describe AdminPublicBodyController, "when administering public bodies and paying
     before do
         config = MySociety::Config.load_default()
         config['SKIP_ADMIN_AUTH'] = false
-        basic_auth_login @request   
+        basic_auth_login @request
     end
     after do
         config = MySociety::Config.load_default()
@@ -106,7 +186,7 @@ describe AdminPublicBodyController, "when administering public bodies and paying
         PublicBody.count.should == n - 1
         session[:using_admin].should == 1
     end
-    
+
     it "doesn't let people with bad credentials log in" do
         config = MySociety::Config.load_default()
         config['SKIP_ADMIN_AUTH'] = false
@@ -159,7 +239,7 @@ end
 
 describe AdminPublicBodyController, "when administering public bodies with i18n" do
     integrate_views
-  
+
     it "shows the index page" do
         get :index
     end
@@ -175,7 +255,7 @@ describe AdminPublicBodyController, "when administering public bodies with i18n"
 
     it "edits a public body" do
         get :edit, {:id => 3, :locale => :en}
-        
+
         # When editing a body, the controller returns all available translations
         assigns[:public_body].translation("es").name.should == 'El Department for Humpadinking'
         assigns[:public_body].name.should == 'Department for Humpadinking'
@@ -186,20 +266,20 @@ describe AdminPublicBodyController, "when administering public bodies with i18n"
         PublicBody.with_locale(:es) do
             pb = PublicBody.find(id=3)
             pb.name.should == "El Department for Humpadinking"
-            post :update, { 
-                :id => 3, 
-                :public_body => { 
-                    :name => "Department for Humpadinking", 
-                    :short_name => "", 
-                    :tag_string => "some tags", 
-                    :request_email => 'edited@localhost', 
+            post :update, {
+                :id => 3,
+                :public_body => {
+                    :name => "Department for Humpadinking",
+                    :short_name => "",
+                    :tag_string => "some tags",
+                    :request_email => 'edited@localhost',
                     :last_edit_comment => 'From test code',
                     :translated_versions => {
                         3 => {:locale => "es", :name => "Renamed",:short_name => "", :request_email => 'edited@localhost'}
                         }
                     }
                 }
-            response.flash[:notice].should include('successful') 
+            response.flash[:notice].should include('successful')
         end
 
         pb = PublicBody.find(public_bodies(:humpadink_public_body).id)
@@ -221,7 +301,7 @@ end
 
 describe AdminPublicBodyController, "when creating public bodies with i18n" do
     integrate_views
-  
+
     before do
         @old_filters = ActionController::Routing::Routes.filters
         ActionController::Routing::Routes.filters = RoutingFilter::Chain.new
@@ -242,14 +322,14 @@ describe AdminPublicBodyController, "when creating public bodies with i18n" do
 
     it "creates a new public body with multiple locales" do
         n = PublicBody.count
-        post :create, { 
-            :public_body => { 
+        post :create, {
+            :public_body => {
                 :name => "New Quango", :short_name => "", :tag_string => "blah", :request_email => 'newquango@localhost', :last_edit_comment => 'From test code',
                 :translated_versions => [{ :locale => "es", :name => "Mi Nuevo Quango", :short_name => "", :request_email => 'newquango@localhost' }]
                 }
         }
         PublicBody.count.should == n + 1
-        
+
         body = PublicBody.find_by_name("New Quango")
         body.translations.map {|t| t.locale.to_s}.sort.should == ["en", "es"]
         PublicBody.with_locale(:en) do
@@ -262,7 +342,7 @@ describe AdminPublicBodyController, "when creating public bodies with i18n" do
             body.url_name.should == "mi_nuevo_quango"
             body.first_letter.should == "M"
         end
-        
+
         response.should redirect_to(:controller=>'admin_public_body', :action=>'show', :id=>body.id)
     end
 end