diff options
| -rw-r--r-- | app/models/public_body.rb | 179 | ||||
| -rw-r--r-- | spec/fixtures/files/fake-authority-add-tags.csv (renamed from spec/fixtures/files/fake-authority-add-tags.rb) | 0 | ||||
| -rw-r--r-- | spec/fixtures/files/multiple-locales-same-name.csv | 2 | ||||
| -rw-r--r-- | spec/models/public_body_spec.rb | 293 |
4 files changed, 390 insertions, 84 deletions
diff --git a/app/models/public_body.rb b/app/models/public_body.rb index 5e5b35c5b..c023b436c 100644 --- a/app/models/public_body.rb +++ b/app/models/public_body.rb @@ -453,8 +453,6 @@ class PublicBody < ActiveRecord::Base def self.import_csv_from_file(csv_filename, tag, tag_behaviour, dry_run, editor, available_locales = []) errors = [] notes = [] - available_locales = [I18n.default_locale] if available_locales.empty? - begin ActiveRecord::Base.transaction do # Use the default locale when retrieving existing bodies; otherwise @@ -475,9 +473,18 @@ class PublicBody < ActiveRecord::Base end set_of_importing = Set.new() - field_names = { 'name'=>1, 'request_email'=>2 } # Default values in case no field list is given + # Default values in case no field list is given + field_names = { 'name' => 1, 'request_email' => 2 } line = 0 + import_options = {:field_names => field_names, + :available_locales => available_locales, + :tag => tag, + :tag_behaviour => tag_behaviour, + :editor => editor, + :notes => notes, + :errors => errors } + CSV.foreach(csv_filename) do |row| line = line + 1 @@ -489,7 +496,7 @@ class PublicBody < ActiveRecord::Base end fields = {} - field_names.each{|name, i| fields[name] = row[i]} + field_names.each{ |name, i| fields[name] = row[i] } yield line, fields if block_given? @@ -505,83 +512,11 @@ class PublicBody < ActiveRecord::Base next end - field_list = [] - self.csv_import_fields.each do |field_name, field_notes| - field_list.push field_name - end - - if public_body = bodies_by_name[name] # Existing public body - available_locales.each do |locale| - I18n.with_locale(locale) do - changed = ActiveSupport::OrderedHash.new - field_list.each do |field_name| - localized_field_name = (locale.to_s == I18n.default_locale.to_s) ? 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) - end - end - - unless changed.empty? - notes.push "line #{line.to_s}: updating authority '#{name}' (locale: #{locale}):\n\t#{changed.to_json}" - public_body.last_edit_editor = editor - public_body.last_edit_comment = 'Updated from spreadsheet' - public_body.save! - end - end - end - else # New public body - public_body = PublicBody.new(:name=>"", :short_name=>"", :request_email=>"") - available_locales.each do |locale| - I18n.with_locale(locale) do - changed = ActiveSupport::OrderedHash.new - field_list.each do |field_name| - localized_field_name = (locale.to_s == I18n.default_locale.to_s) ? 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) - end - end - - unless changed.empty? - notes.push "line #{line.to_s}: creating new authority '#{name}' (locale: #{locale}):\n\t#{changed.to_json}" - public_body.publication_scheme = public_body.publication_scheme || "" - public_body.last_edit_editor = editor - public_body.last_edit_comment = 'Created from spreadsheet' - - begin - public_body.save! - rescue ActiveRecord::RecordInvalid - public_body.errors.full_messages.each do |msg| - errors.push "error: line #{ line }: #{ msg } for authority '#{ name }'" - end - next - end - end - end - end - end + public_body = bodies_by_name[name] || PublicBody.new(:name => "", + :short_name => "", + :request_email => "") + public_body.import_values_from_csv_row(row, line, name, import_options) set_of_importing.add(name) end @@ -603,6 +538,77 @@ class PublicBody < ActiveRecord::Base return [errors, notes] end + def self.localized_csv_field_name(locale, field_name) + (locale.to_s == I18n.default_locale.to_s) ? field_name : "#{field_name}.#{locale}" + end + + + # import values from a csv row (that may include localized columns) + def import_values_from_csv_row(row, line, name, options) + is_new = new_record? + edit_info = if is_new + { :action => "creating new authority", + :comment => 'Created from spreadsheet' } + else + { :action => "updating authority", + :comment => 'Updated from spreadsheet' } + end + locales = options[:available_locales] + locales = [I18n.default_locale] if locales.empty? + locales.each do |locale| + I18n.with_locale(locale) do + changed = set_locale_fields_from_csv_row(is_new, locale, row, options) + unless changed.empty? + options[:notes].push "line #{ line }: #{ edit_info[:action] } '#{ name }' (locale: #{ locale }):\n\t#{ changed.to_json }" + self.last_edit_comment = edit_info[:comment] + self.publication_scheme = publication_scheme || "" + self.last_edit_editor = options[:editor] + + begin + save! + rescue ActiveRecord::RecordInvalid + errors.full_messages.each do |msg| + options[:errors].push "error: line #{ line }: #{ msg } for authority '#{ name }'" + end + next + end + end + end + end + end + + # Sets attribute values for a locale from a csv row + def set_locale_fields_from_csv_row(is_new, locale, row, options) + changed = ActiveSupport::OrderedHash.new + csv_field_names = options[:field_names] + csv_import_fields.each do |field_name, field_notes| + localized_field_name = self.class.localized_csv_field_name(locale, field_name) + column = csv_field_names[localized_field_name] + value = column && row[column] + # Tags are a special case, as we support adding to the field, not just setting a new value + if field_name == 'tag_string' + new_tags = [value, options[:tag]].select{ |new_tag| !new_tag.blank? } + if new_tags.empty? + value = nil + else + value = new_tags.join(" ") + value = "#{value} #{tag_string}"if options[:tag_behaviour] == 'add' + end + + end + + if value and read_attribute_value(field_name, locale) != value + if is_new + changed[field_name] = value + else + changed[field_name] = "#{read_attribute_value(field_name, locale)}: #{value}" + end + assign_attributes({ field_name => value }) + end + end + changed + end + # Does this user have the power of FOI officer for this body? def is_foi_officer?(user) user_domain = user.email_domain @@ -801,6 +807,19 @@ class PublicBody < ActiveRecord::Base private + # Read an attribute value (without using locale fallbacks if the attribute is translated) + def read_attribute_value(name, locale) + if self.class.translates.include?(name.to_sym) + if globalize.stash.contains?(locale, name) + globalize.stash.read(locale, name) + else + translation_for(locale).send(name) + end + else + send(name) + end + end + def request_email_if_requestable # Request_email can be blank, meaning we don't have details if self.is_requestable? diff --git a/spec/fixtures/files/fake-authority-add-tags.rb b/spec/fixtures/files/fake-authority-add-tags.csv index a5612d87f..a5612d87f 100644 --- a/spec/fixtures/files/fake-authority-add-tags.rb +++ b/spec/fixtures/files/fake-authority-add-tags.csv diff --git a/spec/fixtures/files/multiple-locales-same-name.csv b/spec/fixtures/files/multiple-locales-same-name.csv new file mode 100644 index 000000000..43505f6a6 --- /dev/null +++ b/spec/fixtures/files/multiple-locales-same-name.csv @@ -0,0 +1,2 @@ +"#id","request_email","name","name.es","tag_string","home_page" +23842,"test@test.es","Test","Test",37,"http://www.test.es/" diff --git a/spec/models/public_body_spec.rb b/spec/models/public_body_spec.rb index 7d48fa169..b90696c25 100644 --- a/spec/models/public_body_spec.rb +++ b/spec/models/public_body_spec.rb @@ -654,7 +654,7 @@ describe PublicBody, " when loading CSV files" do PublicBody.find_by_name('Fake Authority of Northern Ireland').tag_array_for_search.should == ['aTag', 'fake'] # Import again to check the 'add' tag functionality works - new_tags_file = load_file_fixture('fake-authority-add-tags.rb') + new_tags_file = load_file_fixture('fake-authority-add-tags.csv') errors, notes = PublicBody.import_csv(new_tags_file, '', 'add', false, 'someadmin') # false means real run # Check tags were added successfully @@ -673,15 +673,284 @@ describe PublicBody, " when loading CSV files" do PublicBody.find_by_name('Fake Authority of Northern Ireland').tag_array_for_search.should == ['aTag', 'fake'] # Import again to check the 'replace' tag functionality works - new_tags_file = load_file_fixture('fake-authority-add-tags.rb') + new_tags_file = load_file_fixture('fake-authority-add-tags.csv') 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('North West Fake Authority').tag_array_for_search.should == ['aTag', 'fake'] + PublicBody.find_by_name('Scottish Fake Authority').tag_array_for_search.should == ['aTag', 'fake'] PublicBody.find_by_name('Fake Authority of Northern Ireland').tag_array_for_search.should == ['aTag', 'fake'] end + + context 'when the import tag is set' do + + context 'with a new body' do + + it 'appends the import tag when no tag_string is specified' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + ,q@localhost,Quango,,http://example.org + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, 'imported', 'add', false, 'someadmin') + + expected = %W(imported) + expect(PublicBody.find_by_name('Quango').tag_array_for_search).to eq(expected) + end + + it 'appends the import tag when a tag_string is specified' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + ,q@localhost,Quango,first_tag second_tag,http://example.org + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, 'imported', 'add', false, 'someadmin') + + expected = %W(first_tag imported second_tag) + expect(PublicBody.find_by_name('Quango').tag_array_for_search).to eq(expected) + end + + it 'replaces with the import tag when no tag_string is specified' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + ,q@localhost,Quango,,http://example.org + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, 'imported', 'replace', false, 'someadmin') + + expected = %W(imported) + expect(PublicBody.find_by_name('Quango').tag_array_for_search).to eq(expected) + end + + it 'replaces with the import tag and tag_string when a tag_string is specified' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + ,q@localhost,Quango,first_tag second_tag,http://example.org + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, 'imported', 'replace', false, 'someadmin') + + expected = %W(first_tag imported second_tag) + expect(PublicBody.find_by_name('Quango').tag_array_for_search).to eq(expected) + end + + end + + context 'an existing body without tags' do + + before do + @body = FactoryGirl.create(:public_body, :name => 'Existing Body') + end + + it 'will not import if there is an existing body without the tag' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + #{ @body.id },#{ @body.request_email },"#{ @body.name }",,#{ @body.home_page } + CSV + + # csv, tag, tag_behaviour, dry_run, editor + errors, notes = PublicBody.import_csv(csv, 'imported', 'add', false, 'someadmin') + + expected = %W(imported) + errors.should include("error: line 2: Name Name is already taken for authority 'Existing Body'") + end + + end + + context 'an existing body with tags' do + + before do + @body = FactoryGirl.create(:public_body, :tag_string => 'imported first_tag second_tag') + end + + it 'created with tags, different tags in csv, add import tag' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + #{ @body.id },#{ @body.request_email },"#{ @body.name }","first_tag new_tag",#{ @body.home_page } + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, 'imported', 'add', false, 'someadmin') + expected = %W(first_tag imported new_tag second_tag) + expect(PublicBody.find(@body.id).tag_array_for_search).to eq(expected) + end + + it 'created with tags, different tags in csv, replace import tag' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + #{ @body.id },#{ @body.request_email },"#{ @body.name }","first_tag new_tag",#{ @body.home_page } + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, 'imported', 'replace', false, 'someadmin') + + expected = %W(first_tag imported new_tag) + expect(PublicBody.find(@body.id).tag_array_for_search).to eq(expected) + end + + end + + end + + context 'when the import tag is not set' do + + context 'with a new body' do + + it 'it is empty if no tag_string is set' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + ,q@localhost,Quango,,http://example.org + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, '', 'add', false, 'someadmin') + + expected = [] + expect(PublicBody.find_by_name('Quango').tag_array_for_search).to eq(expected) + end + + it 'uses the specified tag_string' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + ,q@localhost,Quango,first_tag,http://example.org + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, '', 'add', false, 'someadmin') + + expected = %W(first_tag) + expect(PublicBody.find_by_name('Quango').tag_array_for_search).to eq(expected) + end + + it 'replaces with empty if no tag_string is set' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + ,q@localhost,Quango,,http://example.org + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, '', 'replace', false, 'someadmin') + + expected = [] + expect(PublicBody.find_by_name('Quango').tag_array_for_search).to eq(expected) + end + + it 'replaces with the specified tag_string' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + ,q@localhost,Quango,first_tag,http://example.org + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, '', 'replace', false, 'someadmin') + + expected = %W(first_tag) + expect(PublicBody.find_by_name('Quango').tag_array_for_search).to eq(expected) + end + + end + + context 'with an existing body without tags' do + + before do + @body = FactoryGirl.create(:public_body) + end + + it 'appends when no tag_string is specified' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + #{ @body.id },#{ @body.request_email },"#{ @body.name }",,#{ @body.home_page } + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, '', 'add', false, 'someadmin') + + expected = [] + expect(PublicBody.find(@body.id).tag_array_for_search).to eq(expected) + end + + it 'appends when a tag_string is specified' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + #{ @body.id },#{ @body.request_email },"#{ @body.name }",new_tag,#{ @body.home_page } + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, '', 'add', false, 'someadmin') + + expected = %W(new_tag) + expect(PublicBody.find(@body.id).tag_array_for_search).to eq(expected) + end + + it 'replaces when no tag_string is specified' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + #{ @body.id },#{ @body.request_email },"#{ @body.name }",,#{ @body.home_page } + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, '', 'replace', false, 'someadmin') + + expected = [] + expect(PublicBody.find(@body.id).tag_array_for_search).to eq(expected) + end + + it 'replaces when a tag_string is specified' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + #{ @body.id },#{ @body.request_email },"#{ @body.name }",new_tag,#{ @body.home_page } + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, '', 'replace', false, 'someadmin') + + expected = %W(new_tag) + expect(PublicBody.find(@body.id).tag_array_for_search).to eq(expected) + end + + end + + describe 'with an existing body with tags' do + + before do + @body = FactoryGirl.create(:public_body, :tag_string => 'first_tag second_tag') + end + + it 'created with tags, different tags in csv, add tags' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + #{ @body.id },#{ @body.request_email },"#{ @body.name }","first_tag new_tag",#{ @body.home_page } + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, '', 'add', false, 'someadmin') + + expected = %W(first_tag new_tag second_tag) + expect(PublicBody.find(@body.id).tag_array_for_search).to eq(expected) + end + + it 'created with tags, different tags in csv, replace' do + csv = <<-CSV.strip_heredoc + #id,request_email,name,tag_string,home_page + #{ @body.id },#{ @body.request_email },"#{ @body.name }","first_tag new_tag",#{ @body.home_page } + CSV + + # csv, tag, tag_behaviour, dry_run, editor + PublicBody.import_csv(csv, '', 'replace', false, 'someadmin') + + expected = %W(first_tag new_tag) + expect(PublicBody.find(@body.id).tag_array_for_search).to eq(expected) + end + + end + + end + it "should create bodies with names in multiple locales" do original_count = PublicBody.count @@ -806,6 +1075,22 @@ CSV PublicBody.csv_import_fields = old_csv_import_fields end + it "should import translations for fields whose values are the same as the default locale's" do + original_count = PublicBody.count + + csv_contents = load_file_fixture("multiple-locales-same-name.csv") + + errors, notes = PublicBody.import_csv(csv_contents, '', 'replace', true, 'someadmin', ['en', 'es']) # true means dry run + errors.should == [] + notes.size.should == 3 + notes[0..1].should == [ + "line 2: creating new authority 'Test' (locale: en):\n\t{\"name\":\"Test\",\"request_email\":\"test@test.es\",\"home_page\":\"http://www.test.es/\",\"tag_string\":\"37\"}", + "line 2: creating new authority 'Test' (locale: es):\n\t{\"name\":\"Test\"}", + ] + notes[2].should =~ /Notes: Some bodies are in database, but not in CSV file:\n( .+\n)*You may want to delete them manually.\n/ + + PublicBody.count.should == original_count + end end describe PublicBody do |