Make tag optional when importing public bodies through CSV (issue mys…

…ociety#60)

app/controllers/admin_public_body_controller.rb

@@ -138,41 +138,36 @@ def destroy
 
     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 = ""

app/models/public_body.rb

@@ -357,7 +357,11 @@ def self.import_csv(csv, tag, dry_run, editor, available_locales = [])
                 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 @@ def self.import_csv(csv, tag, dry_run, editor, available_locales = [])
 
                     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 @@ def self.import_csv(csv, tag, dry_run, editor, available_locales = [])
                 # 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

app/views/admin_public_body/import_csv.rhtml

@@ -11,16 +11,16 @@
 
 
 <% 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

spec/models/public_body_spec.rb

@@ -227,6 +227,12 @@
 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 @@
         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