Make tag optional when importing public bodies through CSV (issue #60)

4 files changed, 60 insertions, 39 deletions

diff --git a/app/controllers/admin_public_body_controller.rb b/app/controllers/admin_public_body_controller.rb
index bd85f6eed..8cb7043a0 100644
--- a/app/controllers/admin_public_body_controller.rb
+++ b/app/controllers/admin_public_body_controller.rb
@@ -138,41 +138,36 @@ class AdminPublicBodyController < AdminController
 
     def import_csv
         if params[:csv_file]
-            if !params[:tag].empty?
-                if params['commit'] == 'Dry run'
-                    dry_run_only = true
-                elsif params['commit'] == 'Upload'
-                    dry_run_only = false
+            if params['commit'] == 'Dry run'
+                dry_run_only = true
+            elsif params['commit'] == 'Upload'
+                dry_run_only = false
+            else
+                raise "internal error, unknown button label"
+            end
+            
+            # Try with dry run first
+            csv_contents = params[:csv_file].read
+            en = PublicBody.import_csv(csv_contents, params[:tag], 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.")
                 else
-                    raise "internal error, unknown button label"
-                end
-                
-                # Try with dry run first
-                csv_contents = params[:csv_file].read
-                en = PublicBody.import_csv(csv_contents, params[:tag], true, admin_http_auth_user(), 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.")
-                    else
-                        # And if OK, with real run
-                        en = PublicBody.import_csv(csv_contents, params[:tag], false, admin_http_auth_user(), I18n.available_locales)
-                        errors = en[0]
-                        notes = en[1]
-                        if errors.size != 0
-                            raise "dry run mismatched real run"
-                        end
-                        notes.push("Import was successful.")
+                    # And if OK, with real run
+                    en = PublicBody.import_csv(csv_contents, params[:tag], false, admin_http_auth_user(), I18n.available_locales)
+                    errors = en[0]
+                    notes = en[1]
+                    if errors.size != 0
+                        raise "dry run mismatched real run"
                     end
+                    notes.push("Import was successful.")
                 end
-                @errors = errors.join("\n")
-                @notes = notes.join("\n")
-            else
-                @errors = "Please enter a tag, use a singular e.g. sea_fishery_committee"
-                @notes = ""
             end
+            @errors = errors.join("\n")
+            @notes = notes.join("\n")
         else
             @errors = ""
             @notes = ""
diff --git a/app/models/public_body.rb b/app/models/public_body.rb
index 81149e3c2..9e85fb44b 100644
--- a/app/models/public_body.rb
+++ b/app/models/public_body.rb
@@ -357,7 +357,11 @@ class PublicBody < ActiveRecord::Base
                 bodies_by_name = {}
                 set_of_existing = Set.new()
                 PublicBody.with_locale(I18n.default_locale) do
-                    for existing_body in PublicBody.find_by_tag(tag)
+                    bodies = (tag.nil? || tag.empty?) ? PublicBody.find(:all) : PublicBody.find_by_tag(tag)
+                    for existing_body in bodies
+                        # Hide InternalAdminBody from import notes
+                        next if existing_body.id == PublicBody.internal_admin_body.id
+                        
                         bodies_by_name[existing_body.name] = existing_body
                         set_of_existing.add(existing_body.name)
                     end
@@ -393,7 +397,7 @@ class PublicBody < ActiveRecord::Base
                     
                     field_list = ['name', 'short_name', 'request_email', 'notes', 'publication_scheme', 'home_page']
 
-                    if public_body = bodies_by_name[name]
+                    if public_body = bodies_by_name[name]   # Existing public body
                         available_locales.each do |locale|
                             PublicBody.with_locale(locale) do
                                 changed = {}
@@ -446,7 +450,7 @@ class PublicBody < ActiveRecord::Base
                 # Give an error listing ones that are to be deleted 
                 deleted_ones = set_of_existing - set_of_importing
                 if deleted_ones.size > 0
-                    notes.push "notes: Some " + tag + " bodies are in database, but not in CSV file:\n    " + Array(deleted_ones).join("\n    ") + "\nYou may want to delete them manually.\n"
+                    notes.push "Notes: Some " + tag + " bodies are in database, but not in CSV file:\n    " + Array(deleted_ones).join("\n    ") + "\nYou may want to delete them manually.\n"
                 end
 
                 # Rollback if a dry run, or we had errors
diff --git a/app/views/admin_public_body/import_csv.rhtml b/app/views/admin_public_body/import_csv.rhtml
index 3bcc4bf41..8bdc5e3f2 100644
--- a/app/views/admin_public_body/import_csv.rhtml
+++ b/app/views/admin_public_body/import_csv.rhtml
@@ -12,15 +12,15 @@
 
 <% form_tag 'import_csv', :multipart => true do %>
     <p>
-    <label for="tag">Tag to add entries to / alter entries for:</label>
-    <%= text_field_tag 'tag', params[:tag] %>
-    </p>
-
-    <p>
     <label for="csv_file">CSV file:</label>
     <%= file_field_tag :csv_file, :size => 40 %> 
     </p>
     
+    <p>
+    <label for="tag">Optional: Tag to add entries to / alter entries for:</label>
+    <%= text_field_tag 'tag', params[:tag] %>
+    </p>
+
     <p><strong>CSV file format:</strong> A first row with the list of fields, 
     starting with '#', is optional but highly recommended. The fields 'name'
     and 'request_email' are required; additionaly, translated values are supported by
diff --git a/spec/models/public_body_spec.rb b/spec/models/public_body_spec.rb
index 3902e3662..67f1cc069 100644
--- a/spec/models/public_body_spec.rb
+++ b/spec/models/public_body_spec.rb
@@ -227,6 +227,12 @@ describe PublicBody, "when searching" do
 end
 
 describe PublicBody, " when loading CSV files" do
+    before(:each) do
+        # InternalBody is created the first time it's accessed, which happens sometimes during imports,
+        # depending on the tag used. By accessing it here before every test, it doesn't disturb our checks later on
+        PublicBody.internal_admin_body
+    end
+    
     it "should do a dry run successfully" do
         original_count = PublicBody.count
 
@@ -259,6 +265,22 @@ describe PublicBody, " when loading CSV files" do
         PublicBody.count.should == original_count + 3
     end
 
+    it "should do imports without a tag successfully" do
+        original_count = PublicBody.count
+        
+        csv_contents = load_file_fixture("fake-authority-type.csv")
+        errors, notes = PublicBody.import_csv(csv_contents, '', false, 'someadmin') # false means real run
+        errors.should == []
+        notes.size.should == 4
+        notes.should == [
+            "line 1: creating new authority 'North West Fake Authority' (locale: en):\n\t\{\"request_email\":\"north_west_foi@localhost\"\}", 
+            "line 2: creating new authority 'Scottish Fake Authority' (locale: en):\n\t\{\"request_email\":\"scottish_foi@localhost\"\}", 
+            "line 3: creating new authority 'Fake Authority of Northern Ireland' (locale: en):\n\t\{\"request_email\":\"ni_foi@localhost\"\}",
+            "Notes: Some  bodies are in database, but not in CSV file:\n    Department for Humpadinking\n    Geraldine Quango\nYou may want to delete them manually.\n"
+            ]
+        PublicBody.count.should == original_count + 3
+    end
+
     it "should handle a field list and fields out of order" do
         original_count = PublicBody.count