Merge pull request #223 from activeadmin-plugins/fix/batch-slice-columns-successive-calls

fix: compose successive batch_slice_columns calls (#186)

Author: Fivell

lib/active_admin_import/importer.rb

@@ -82,23 +82,15 @@ def batch_replace(header_key, options)
     # end
     #
     def batch_slice_columns(slice_columns)
-      # Only set @use_indexes for the first batch so that @use_indexes are in correct
-      # position for subsequent batches
-      unless defined?(@use_indexes)
-        @use_indexes = []
-        headers.values.each_with_index do |val, index|
-          @use_indexes << index if val.in?(slice_columns)
-        end
-        return csv_lines if @use_indexes.empty?
-
-        # slice CSV headers
-        @headers = headers.to_a.values_at(*@use_indexes).to_h
-      end
+      columns = headers.values
+      indexes = columns.each_index.select { |i| columns[i].in?(slice_columns) }
+      return csv_lines if indexes.empty?
 
-      # slice CSV values
-      csv_lines.map! do |line|
-        line.values_at(*@use_indexes)
-      end
+      # @headers is reset to the full set at the start of every batch (see
+      # #batch_import), so each call narrows the previous call's result and every
+      # batch slices the same way — calling this more than once now composes (#186).
+      @headers = headers.to_a.values_at(*indexes).to_h
+      csv_lines.map! { |line| line.values_at(*indexes) }
     end
 
     def values_at(header_key)
@@ -129,17 +121,18 @@ def process_file
     end
 
     def prepare_headers
-      headers = self.headers.present? ? self.headers : yield
-      blank_positions = headers.each_index.select { |i| headers[i].to_s.strip.empty? }
+      names = self.headers.present? ? self.headers : yield
+      blank_positions = names.each_index.select { |i| names[i].to_s.strip.empty? }
       unless blank_positions.empty?
         raise ActiveAdminImport::Exception,
               "blank column header at column #{blank_positions.map { |i| i + 1 }.join(', ')}"
       end
 
-      headers = headers.map(&:to_s)
-      @headers = Hash[headers.zip(headers.map { |el| el.underscore.gsub(/\s+/, '_') })].with_indifferent_access
-      @headers.merge!(options[:headers_rewrites].symbolize_keys.slice(*@headers.symbolize_keys.keys))
-      @headers
+      names = names.map(&:to_s)
+      # @source_headers is the complete parsed header row; batch_import copies it
+      # into the per-batch working @headers (see #batch_import).
+      @source_headers = Hash[names.zip(names.map { |el| el.underscore.gsub(/\s+/, '_') })].with_indifferent_access
+      @source_headers.merge!(options[:headers_rewrites].symbolize_keys.slice(*@source_headers.symbolize_keys.keys))
     end
 
     def run_callback(name)
@@ -148,6 +141,9 @@ def run_callback(name)
 
     def batch_import
       batch_result = nil
+      # Every batch re-parses full-width rows, so restore the full header set
+      # before slicing; batch_slice_columns then narrows this working copy.
+      @headers = @source_headers.dup
       @resource.transaction do
         run_callback(:before_batch_import)
         batch_result = resource.import(headers.values, csv_lines, import_options)

spec/fixtures/files/authors_many.csv

@@ -0,0 +1,6 @@
+Birthday,Name,Last name
+1986-05-01,John,Doe
+1988-11-16,Jane,Roe
+1990-01-01,Jack,Smith
+1991-02-02,Jill,Jones
+1992-03-03,Joe,Brown

spec/import_spec.rb

@@ -656,6 +656,56 @@ def upload_file!(name, ext = 'csv')
     end
   end
 
+  # Issue #186: each call must slice the result of the previous one, not re-apply
+  # the first call's indices to an already-sliced row.
+  context "with successive batch_slice_columns calls" do
+    before do
+      # validate: false isolates the slicing behaviour from the model's
+      # uniqueness validation, which would otherwise reject a later batch's
+      # author for sharing a NULL last_name with an earlier one.
+      add_author_resource template_object: ActiveAdminImport::Model.new,
+                          validate: false,
+                          before_batch_import: lambda { |importer|
+                            importer.batch_slice_columns(%w(name last_name))
+                            importer.batch_slice_columns(%w(name))
+                          },
+                          batch_size: batch_size
+      visit "/admin/authors/import"
+      upload_file!(fixture)
+    end
+
+    context "within a single batch" do
+      let(:batch_size) { 2 } # authors.csv has 2 rows -> 1 batch
+      let(:fixture) { :authors }
+
+      it "narrows the columns progressively" do
+        expect(Author.pluck(:name, :last_name, :birthday)).to match_array(
+          [
+            ["Jane", nil, nil],
+            ["John", nil, nil]
+          ]
+        )
+      end
+    end
+
+    context "across several batches" do
+      let(:batch_size) { 2 } # authors_many.csv has 5 rows -> 3 batches
+      let(:fixture) { :authors_many }
+
+      it "narrows the columns progressively in every batch" do
+        expect(Author.pluck(:name, :last_name, :birthday)).to match_array(
+          [
+            ["John", nil, nil],
+            ["Jane", nil, nil],
+            ["Jack", nil, nil],
+            ["Jill", nil, nil],
+            ["Joe", nil, nil]
+          ]
+        )
+      end
+    end
+  end
+
   context 'with invalid options' do
     let(:options) { { invalid_option: :invalid_value } }