diff options
| -rw-r--r-- | app/controllers/admin_public_body_controller.rb | 4 | ||||
| -rw-r--r-- | app/models/public_body.rb | 22 | ||||
| -rw-r--r-- | app/views/admin_public_body/import_csv.rhtml | 13 | ||||
| -rw-r--r-- | spec/fixtures/files/fake-authority-add-tags.rb | 4 | ||||
| -rw-r--r-- | spec/models/public_body_spec.rb | 70 |
5 files changed, 85 insertions, 28 deletions
diff --git a/app/controllers/admin_public_body_controller.rb b/app/controllers/admin_public_body_controller.rb index d80e2937f..e249cef11 100644 --- a/app/controllers/admin_public_body_controller.rb +++ b/app/controllers/admin_public_body_controller.rb @@ -150,7 +150,7 @@ class AdminPublicBodyController < AdminController # 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) + 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] @@ -159,7 +159,7 @@ class AdminPublicBodyController < AdminController 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) + 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 diff --git a/app/models/public_body.rb b/app/models/public_body.rb index 430aa8f34..311e19001 100644 --- a/app/models/public_body.rb +++ b/app/models/public_body.rb @@ -334,7 +334,7 @@ class PublicBody < ActiveRecord::Base # Import from CSV. Just tests things and returns messages if dry_run is true. # Returns an array of [array of errors, array of notes]. If there are errors, # always rolls back (as with dry_run). - def self.import_csv(csv, tag, dry_run, editor, available_locales = []) + def self.import_csv(csv, tag, tag_behaviour, dry_run, editor, available_locales = []) errors = [] notes = [] available_locales = [I18n.default_locale] if available_locales.empty? @@ -387,13 +387,26 @@ class PublicBody < ActiveRecord::Base field_list = ['name', 'short_name', 'request_email', 'notes', 'publication_scheme', 'home_page', 'tag_string'] - if public_body = bodies_by_name[name] # Existing public body + if public_body = bodies_by_name[name] # Existing public body available_locales.each do |locale| PublicBody.with_locale(locale) do changed = {} field_list.each do |field_name| localized_field_name = (locale === I18n.default_locale) ? field_name : "#{field_name}.#{locale}" localized_value = field_names[localized_field_name] && row[field_names[localized_field_name]] + + # Tags are a special case, as we support adding to the field, not just setting a new value + if localized_field_name == 'tag_string' + if localized_value.nil? + localized_value = tag unless tag.empty? + else + if tag_behaviour == 'add' + localized_value = "#{localized_value} #{tag}" unless tag.empty? + localized_value = "#{localized_value} #{public_body.tag_string}" + end + end + end + if !localized_value.nil? and public_body.send(field_name) != localized_value changed[field_name] = "#{public_body.send(field_name)}: #{localized_value}" public_body.send("#{field_name}=", localized_value) @@ -416,6 +429,11 @@ class PublicBody < ActiveRecord::Base field_list.each do |field_name| localized_field_name = (locale === I18n.default_locale) ? field_name : "#{field_name}.#{locale}" localized_value = field_names[localized_field_name] && row[field_names[localized_field_name]] + + if localized_field_name == 'tag_string' and tag_behaviour == 'add' + localized_value = "#{localized_value} #{tag}" unless tag.empty? + end + if !localized_value.nil? and public_body.send(field_name) != localized_value changed[field_name] = localized_value public_body.send("#{field_name}=", localized_value) diff --git a/app/views/admin_public_body/import_csv.rhtml b/app/views/admin_public_body/import_csv.rhtml index a7a13f477..ecd2c38b7 100644 --- a/app/views/admin_public_body/import_csv.rhtml +++ b/app/views/admin_public_body/import_csv.rhtml @@ -20,6 +20,14 @@ <label for="tag">Optional: Tag to add entries to / alter entries for:</label> <%= text_field_tag 'tag', params[:tag] %> </p> + + <p> + <label for="tag_behaviour">What to do with existing tags?</label> + <%= select_tag 'tag_behaviour', + "<option value='add' selected>Add new tags to existing ones</option> + <option value='replace'>Replace existing tags with new ones</option>" + %> + </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' @@ -42,6 +50,11 @@ of the changes. When uploading, any changes since last import will be overwritten - e.g. email addresses changed back. </p> + + <p><strong>Note:</strong> The import tag will also be added to the imported bodies + if no tags are provided in the CSV file or if the import mode is set to + "Add new tags to existing ones". + </p> <p><%= submit_tag 'Dry run' %> <%= submit_tag 'Upload' %></p> <% end %> diff --git a/spec/fixtures/files/fake-authority-add-tags.rb b/spec/fixtures/files/fake-authority-add-tags.rb new file mode 100644 index 000000000..a5612d87f --- /dev/null +++ b/spec/fixtures/files/fake-authority-add-tags.rb @@ -0,0 +1,4 @@ +#id,request_email,name,tag_string +,north_west_foi@localhost,North West Fake Authority,aTag +,scottish_foi@localhost,Scottish Fake Authority,aTag +,ni_foi@localhost,Fake Authority of Northern Ireland,fake aTag diff --git a/spec/models/public_body_spec.rb b/spec/models/public_body_spec.rb index 9833179a7..d2b10d493 100644 --- a/spec/models/public_body_spec.rb +++ b/spec/models/public_body_spec.rb @@ -237,13 +237,14 @@ describe PublicBody, " when loading CSV files" do original_count = PublicBody.count csv_contents = load_file_fixture("fake-authority-type.csv") - errors, notes = PublicBody.import_csv(csv_contents, 'fake', true, 'someadmin') # true means dry run + errors, notes = PublicBody.import_csv(csv_contents, '', 'replace', true, 'someadmin') # true means dry run errors.should == [] - notes.size.should == 3 + 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\"\}" + "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 @@ -253,13 +254,14 @@ describe PublicBody, " when loading CSV files" do original_count = PublicBody.count csv_contents = load_file_fixture("fake-authority-type.csv") - errors, notes = PublicBody.import_csv(csv_contents, 'fake', false, 'someadmin') # false means real run + errors, notes = PublicBody.import_csv(csv_contents, '', 'replace', false, 'someadmin') # false means real run errors.should == [] - notes.size.should == 3 + 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\"\}" + "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 @@ -269,7 +271,7 @@ describe PublicBody, " when loading CSV files" 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, notes = PublicBody.import_csv(csv_contents, '', 'replace', false, 'someadmin') # false means real run errors.should == [] notes.size.should == 4 notes.should == [ @@ -285,35 +287,53 @@ describe PublicBody, " when loading CSV files" do original_count = PublicBody.count csv_contents = load_file_fixture("fake-authority-type-with-field-names.csv") - errors, notes = PublicBody.import_csv(csv_contents, 'fake', true, 'someadmin') # true means dry run + errors, notes = PublicBody.import_csv(csv_contents, '', 'replace', true, 'someadmin') # true means dry run errors.should == [] - notes.size.should == 3 + notes.size.should == 4 notes.should == [ "line 2: creating new authority 'North West Fake Authority' (locale: en):\n\t\{\"request_email\":\"north_west_foi@localhost\",\"home_page\":\"http://northwest.org\"\}", "line 3: creating new authority 'Scottish Fake Authority' (locale: en):\n\t\{\"tag_string\":\"scottish\",\"request_email\":\"scottish_foi@localhost\",\"home_page\":\"http://scottish.org\"\}", - "line 4: creating new authority 'Fake Authority of Northern Ireland' (locale: en):\n\t\{\"tag_string\":\"fake aTag\",\"request_email\":\"ni_foi@localhost\"\}" + "line 4: creating new authority 'Fake Authority of Northern Ireland' (locale: en):\n\t\{\"tag_string\":\"fake aTag\",\"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 end - it "should import tags successfully when no the import tag is not set" do + it "should import tags successfully when the import tag is not set" do csv_contents = load_file_fixture("fake-authority-type-with-field-names.csv") - errors, notes = PublicBody.import_csv(csv_contents, '', false, 'someadmin') # false means real run + errors, notes = PublicBody.import_csv(csv_contents, '', 'replace', false, 'someadmin') # false means real run PublicBody.find_by_name('North West Fake Authority').tag_array_for_search.should == [] PublicBody.find_by_name('Scottish Fake Authority').tag_array_for_search.should == ['scottish'] PublicBody.find_by_name('Fake Authority of Northern Ireland').tag_array_for_search.should == ['fake', 'aTag'] + + # Import again to check the 'add' tag functionality works + new_tags_file = load_file_fixture('fake-authority-add-tags.rb') + errors, notes = PublicBody.import_csv(new_tags_file, '', 'add', false, 'someadmin') # false means real run + + # Check tags were added successfully + PublicBody.find_by_name('North West Fake Authority').tag_array_for_search.should == ['aTag'] + PublicBody.find_by_name('Scottish Fake Authority').tag_array_for_search.should == ['scottish', 'aTag'] + PublicBody.find_by_name('Fake Authority of Northern Ireland').tag_array_for_search.should == ['fake', 'aTag'] end - it "should import tags successfully when no the import tag is set" do + it "should import tags successfully when the import tag is set" do csv_contents = load_file_fixture("fake-authority-type-with-field-names.csv") - errors, notes = PublicBody.import_csv(csv_contents, 'fake', false, 'someadmin') # false means real run - # XXX: when a 'tag' is set for the import, do we want to add it to the tag string for all bodies? - # The tests below assume we don't + errors, notes = PublicBody.import_csv(csv_contents, 'fake', 'add', false, 'someadmin') # false means real run + + # Check new bodies were imported successfully + PublicBody.find_by_name('North West Fake Authority').tag_array_for_search.should == ['fake'] + PublicBody.find_by_name('Scottish Fake Authority').tag_array_for_search.should == ['scottish', 'fake'] + PublicBody.find_by_name('Fake Authority of Northern Ireland').tag_array_for_search.should == ['fake', 'aTag'] - PublicBody.find_by_name('North West Fake Authority').tag_array_for_search.should == [] - PublicBody.find_by_name('Scottish Fake Authority').tag_array_for_search.should == ['scottish'] + # Import again to check the 'replace' tag functionality works + new_tags_file = load_file_fixture('fake-authority-add-tags.rb') + errors, notes = PublicBody.import_csv(new_tags_file, 'fake', 'replace', false, 'someadmin') # false means real run + + # Check tags were added successfully + PublicBody.find_by_name('North West Fake Authority').tag_array_for_search.should == ['aTag'] + PublicBody.find_by_name('Scottish Fake Authority').tag_array_for_search.should == ['aTag'] PublicBody.find_by_name('Fake Authority of Northern Ireland').tag_array_for_search.should == ['fake', 'aTag'] end @@ -321,16 +341,17 @@ describe PublicBody, " when loading CSV files" do original_count = PublicBody.count csv_contents = load_file_fixture("fake-authority-type-with-field-names.csv") - errors, notes = PublicBody.import_csv(csv_contents, 'fake', false, 'someadmin', [:en, :es]) + errors, notes = PublicBody.import_csv(csv_contents, '', 'replace', false, 'someadmin', [:en, :es]) errors.should == [] - notes.size.should == 6 + notes.size.should == 7 notes.should == [ "line 2: creating new authority 'North West Fake Authority' (locale: en):\n\t{\"request_email\":\"north_west_foi@localhost\",\"home_page\":\"http://northwest.org\"}", "line 2: creating new authority 'North West Fake Authority' (locale: es):\n\t{\"name\":\"Autoridad del Nordeste\"}", "line 3: creating new authority 'Scottish Fake Authority' (locale: en):\n\t{\"tag_string\":\"scottish\",\"request_email\":\"scottish_foi@localhost\",\"home_page\":\"http://scottish.org\"}", "line 3: creating new authority 'Scottish Fake Authority' (locale: es):\n\t{\"name\":\"Autoridad Escocesa\"}", "line 4: creating new authority 'Fake Authority of Northern Ireland' (locale: en):\n\t{\"tag_string\":\"fake aTag\",\"request_email\":\"ni_foi@localhost\"}", - "line 4: creating new authority 'Fake Authority of Northern Ireland' (locale: es):\n\t{\"name\":\"Autoridad Irlandesa\"}" + "line 4: creating new authority 'Fake Authority of Northern Ireland' (locale: es):\n\t{\"name\":\"Autoridad Irlandesa\"}", + "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 @@ -346,13 +367,14 @@ describe PublicBody, " when loading CSV files" do original_count = PublicBody.count csv_contents = load_file_fixture("fake-authority-type-with-field-names.csv") - errors, notes = PublicBody.import_csv(csv_contents, 'fake', true, 'someadmin', [:en, :xx]) # true means dry run + errors, notes = PublicBody.import_csv(csv_contents, '', 'replace', true, 'someadmin', [:en, :xx]) # true means dry run errors.should == [] - notes.size.should == 3 + notes.size.should == 4 notes.should == [ "line 2: creating new authority 'North West Fake Authority' (locale: en):\n\t{\"request_email\":\"north_west_foi@localhost\",\"home_page\":\"http://northwest.org\"}", "line 3: creating new authority 'Scottish Fake Authority' (locale: en):\n\t{\"tag_string\":\"scottish\",\"request_email\":\"scottish_foi@localhost\",\"home_page\":\"http://scottish.org\"}", - "line 4: creating new authority 'Fake Authority of Northern Ireland' (locale: en):\n\t{\"tag_string\":\"fake aTag\",\"request_email\":\"ni_foi@localhost\"}" + "line 4: creating new authority 'Fake Authority of Northern Ireland' (locale: en):\n\t{\"tag_string\":\"fake aTag\",\"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 |