Refactor massive import method into smaller instance level methods

2 files changed, 85 insertions, 91 deletions

diff --git a/app/models/public_body.rb b/app/models/public_body.rb
index 1b8512cfa..6c2e0d04b 100644
--- a/app/models/public_body.rb
+++ b/app/models/public_body.rb
@@ -422,17 +422,6 @@ class PublicBody < ActiveRecord::Base
         has_tag?('site_administration')
     end
 
-
-    # Read an attribute value (without using locale fallbacks if the attribute is translated)
-    def read_attribute_value(name, locale)
-      if PublicBody.translates.include?(name.to_sym)
-          globalize.stash.contains?(locale, name) ? globalize.stash.read(locale, name) : translation_for(locale).send(name)
-      else
-          self.send(name)
-      end
-    end
-
-
     class ImportCSVDryRun < StandardError
     end
 
@@ -456,8 +445,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
@@ -482,6 +469,14 @@ class PublicBody < ActiveRecord::Base
                 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
 
@@ -509,83 +504,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.read_attribute_value(field_name, locale) != 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.read_attribute_value(field_name, locale) != 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
 
@@ -607,6 +530,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
diff --git a/spec/models/public_body_spec.rb b/spec/models/public_body_spec.rb
index 23f3aa5d6..e43c3eb51 100644
--- a/spec/models/public_body_spec.rb
+++ b/spec/models/public_body_spec.rb
@@ -469,8 +469,8 @@ describe PublicBody, " when loading CSV files" do
         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