diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..52683d9 --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,43 @@ +name: Test + +on: + - push + - pull_request + +jobs: + build: + runs-on: ubuntu-latest + + strategy: + matrix: + ruby_version: + - '2.6' + - '2.7' + - '3.0' + rails_version: + - '5.2.6' + - '6.0.4' + - '6.1.4' + - '7.0.0' + exclude: + - ruby_version: '3.0' + rails_version: '5.2.6' + - ruby_version: '2.6' + rails_version: '7.0.0' + + name: Ruby ${{ matrix.ruby_version }} / Rails ${{ matrix.rails_version }} + + env: + RAILS: ${{ matrix.rails_version }} + + steps: + - uses: actions/checkout@v2 + + - name: Setup Ruby + uses: ruby/setup-ruby@v1 + with: + ruby-version: ${{ matrix.ruby_version }} + bundler-cache: true + + - name: Test + run: bundle exec rspec spec diff --git a/.gitignore b/.gitignore index 54cb8bb..b4fb8da 100644 --- a/.gitignore +++ b/.gitignore @@ -13,4 +13,7 @@ capybara-*.html /spec/tmp/* **.orig rerun.txt -pickle-email-*.html \ No newline at end of file +pickle-email-*.html +pkg +spec/rails +Gemfile.lock diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index 7b00bc4..0000000 --- a/.travis.yml +++ /dev/null @@ -1,8 +0,0 @@ -script: bundle exec rspec spec -env: - matrix: - - RAILS=5.0.0 - - RAILS=5.1.0 -rvm: - - 2.2.5 - - 2.3.1 diff --git a/CHANGELOG.md b/CHANGELOG.md index 63495ef..458a8a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,21 @@ # Changelog +## [5.0.0] - 2021-11-16 +- Ruby 3 compatibility added #190 | @clinejj +- Support for a non UTF-8 file when zip uploading #185| @naokirin +- Rails 6 supported #183 | @pnghai +- Drop ruby 2.4 support #192 | @Fivell + + +## [4.2.0] - 2020-02-05 +- generic exception for import added #175 | @linqueta + +## [4.1.2] - 2019-12-16 +- allow application/octet-stream content-type #172 | @dmitry-sinina +- Allow activerecord-import >= 0.27 #171 | @sagium + +## [4.1.1] - 2019-09-20 +- Fix column slicing #168 | @doredesign +- Handle errors on base #163 ## [4.1.0] - 2019-01-15 - Upgrade dependencies: `activerecord-import` to >=0.27.1 | @jkowens diff --git a/Gemfile b/Gemfile index 57ec576..8dd493e 100644 --- a/Gemfile +++ b/Gemfile @@ -6,12 +6,13 @@ gemspec group :test do - default_rails_version = "~> 5.1" + default_rails_version = "~> 5.2.4" rails_version = ENV['RAILS'] || default_rails_version + gem 'sassc-rails' gem 'rails', rails_version gem 'rspec-rails' gem 'coveralls', require: false # Test coverage website. Go to https://coveralls.io - gem 'sqlite3' + gem "sqlite3", "~> 1.4.0" gem 'launchy' gem 'database_cleaner' gem 'capybara' diff --git a/README.md b/README.md index c492ee5..357db1b 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # ActiveAdminImport -[![Travis Build ][build_badge]][build_link] +[![Build Status ][build_badge]][build_link] [![Coverage Status][coveralls_badge]][coveralls_link] [![Code Climate ][codeclimate_badge]][codeclimate_link] [![Gem Version ][rubygems_badge]][rubygems_link] @@ -91,8 +91,8 @@ Tool | Description [rchardet]: https://github.com/jmhodges/rchardet [activerecord-import]: https://github.com/zdennis/activerecord-import -[build_badge]: https://travis-ci.org/activeadmin-plugins/active_admin_import.svg?branch=master -[build_link]: https://travis-ci.org/activeadmin-plugins/active_admin_import +[build_badge]: https://github.com/activeadmin-plugins/active_admin_import/actions/workflows/test.yml/badge.svg +[build_link]: https://github.com/activeadmin-plugins/active_admin_import/actions [coveralls_badge]: https://coveralls.io/repos/activeadmin-plugins/active_admin_import/badge.svg [coveralls_link]: https://coveralls.io/github/activeadmin-plugins/active_admin_import [codeclimate_badge]: https://codeclimate.com/github/activeadmin-plugins/active_admin_import/badges/gpa.svg diff --git a/active_admin_import.gemspec b/active_admin_import.gemspec index fb2ffce..252c83d 100644 --- a/active_admin_import.gemspec +++ b/active_admin_import.gemspec @@ -15,8 +15,8 @@ Gem::Specification.new do |gem| gem.name = 'active_admin_import' gem.require_paths = ['lib'] gem.version = ActiveAdminImport::VERSION - gem.add_runtime_dependency 'activerecord-import', '~> 0.27' - gem.add_runtime_dependency 'rchardet', '~> 1.6' - gem.add_runtime_dependency 'rubyzip', '~> 1.2' - gem.add_dependency 'activeadmin', '>= 1.0.0.pre2' + gem.add_runtime_dependency 'activerecord-import', '>= 0.27' + gem.add_runtime_dependency 'rchardet', '>= 1.6' + gem.add_runtime_dependency 'rubyzip', '>= 1.2' + gem.add_dependency 'activeadmin', '>= 1.0.0' end diff --git a/lib/active_admin_import.rb b/lib/active_admin_import.rb index 40adf2b..8654626 100644 --- a/lib/active_admin_import.rb +++ b/lib/active_admin_import.rb @@ -3,6 +3,7 @@ require 'active_admin' require 'active_admin_import/version' require 'active_admin_import/engine' +require 'active_admin_import/exception' require 'active_admin_import/import_result' require 'active_admin_import/options' require 'active_admin_import/dsl' diff --git a/lib/active_admin_import/dsl.rb b/lib/active_admin_import/dsl.rb index 6d4698f..ccfd4ab 100644 --- a/lib/active_admin_import/dsl.rb +++ b/lib/active_admin_import/dsl.rb @@ -54,11 +54,14 @@ def active_admin_import(options = {}, &block) options.assert_valid_keys(*Options::VALID_OPTIONS) options = Options.options_for(config, options) - params_key = ActiveModel::Naming.param_key(options[:template_object]) collection_action :import, method: :get do authorize!(ActiveAdminImport::Auth::IMPORT, active_admin_config.resource_class) - @active_admin_import_model = options[:template_object] + @active_admin_import_model = if options[:template_object].is_a?(Proc) + options[:template_object].call + else + options[:template_object] + end render template: options[:template] end @@ -73,9 +76,15 @@ def active_admin_import(options = {}, &block) collection_action :do_import, method: :post do authorize!(ActiveAdminImport::Auth::IMPORT, active_admin_config.resource_class) + options[:controller_context] = self _params = params.respond_to?(:to_unsafe_h) ? params.to_unsafe_h : params params = ActiveSupport::HashWithIndifferentAccess.new _params - @active_admin_import_model = options[:template_object] + @active_admin_import_model = if options[:template_object].is_a?(Proc) + options[:template_object].call + else + options[:template_object] + end + params_key = ActiveModel::Naming.param_key(@active_admin_import_model.class) @active_admin_import_model.assign_attributes(params[params_key].try(:deep_symbolize_keys) || {}) # go back to form return render template: options[:template] unless @active_admin_import_model.valid? @@ -92,7 +101,12 @@ def active_admin_import(options = {}, &block) else instance_exec result, options, &DEFAULT_RESULT_PROC end - rescue ActiveRecord::Import::MissingColumnError, NoMethodError, ActiveRecord::StatementInvalid, CSV::MalformedCSVError => e + rescue ActiveRecord::Import::MissingColumnError, + NoMethodError, + ArgumentError, + ActiveRecord::StatementInvalid, + CSV::MalformedCSVError, + ActiveAdminImport::Exception => e Rails.logger.error(I18n.t('active_admin_import.file_error', message: e.message)) Rails.logger.error(e.backtrace.join("\n")) flash[:error] = I18n.t('active_admin_import.file_error', message: e.message[0..200]) diff --git a/lib/active_admin_import/exception.rb b/lib/active_admin_import/exception.rb new file mode 100644 index 0000000..7d70a07 --- /dev/null +++ b/lib/active_admin_import/exception.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true +module ActiveAdminImport + class Exception < StandardError + end +end \ No newline at end of file diff --git a/lib/active_admin_import/import_result.rb b/lib/active_admin_import/import_result.rb index 2556a95..9abcc6d 100644 --- a/lib/active_admin_import/import_result.rb +++ b/lib/active_admin_import/import_result.rb @@ -33,8 +33,21 @@ def failed_message(options = {}) limit = options[:limit] || failed.count failed.first(limit).map do |record| errors = record.errors - (errors.full_messages.zip errors.keys.map { |k| record.send k }).map { |ms| ms.join(' - ') }.join(', ') + failed_values = attribute_names_for(errors).map do |key| + key == :base ? nil : record.public_send(key) + end + errors.full_messages.zip(failed_values).map { |ms| ms.compact.join(' - ') }.join(', ') end.join(' ; ') end + + private + + def attribute_names_for(errors) + if Gem::Version.new(Rails.version) >= Gem::Version.new('7.0') + errors.attribute_names + else + errors.keys + end + end end end diff --git a/lib/active_admin_import/importer.rb b/lib/active_admin_import/importer.rb index 9d18e4e..ab337f6 100644 --- a/lib/active_admin_import/importer.rb +++ b/lib/active_admin_import/importer.rb @@ -18,7 +18,8 @@ class Importer :headers_rewrites, :batch_size, :batch_transaction, - :csv_options + :csv_options, + :controller_context ].freeze def initialize(resource, model, options) @@ -37,7 +38,7 @@ def file end def cycle(lines) - @csv_lines = CSV.parse(lines.join, @csv_options) + @csv_lines = CSV.parse(lines.join, **@csv_options) import_result.add(batch_import, lines.count) end @@ -70,7 +71,7 @@ def batch_replace(header_key, options) end end - # Use it when CSV file contain redundant columns + # Use this method when CSV file contains unnecessary columns # # Example: # @@ -81,16 +82,22 @@ def batch_replace(header_key, options) # end # def batch_slice_columns(slice_columns) - use_indexes = [] - headers.values.each_with_index do |val, index| - use_indexes << index if val.in?(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 - return csv_lines if use_indexes.empty? - # slice CSV headers - @headers = headers.to_a.values_at(*use_indexes).to_h + # slice CSV values csv_lines.map! do |line| - line.values_at(*use_indexes) + line.values_at(*@use_indexes) end end @@ -109,7 +116,7 @@ def process_file batch_size = options[:batch_size].to_i File.open(file.path) do |f| # capture headers if not exist - prepare_headers { CSV.parse(f.readline, @csv_options).first } + prepare_headers { CSV.parse(f.readline, **@csv_options).first } f.each_line do |line| lines << line if line.present? if lines.size == batch_size || f.eof? diff --git a/lib/active_admin_import/model.rb b/lib/active_admin_import/model.rb index 5927996..f3fde01 100644 --- a/lib/active_admin_import/model.rb +++ b/lib/active_admin_import/model.rb @@ -21,6 +21,7 @@ module CONST application/csv application/vnd.ms-excel application/vnd.msexcel + application/octet-stream text/tsv text/x-tsv text/tab-separated-values @@ -102,6 +103,8 @@ def file_path def encode_file data = File.read(file_path) + return if data.empty? + File.open(file_path, 'w') do |f| f.write(encode(data)) end @@ -109,7 +112,7 @@ def encode_file def unzip_file Zip::File.open(file_path) do |zip_file| - self.file = Tempfile.new(CONST::TMP_FILE) + self.file = Tempfile.new(CONST::TMP_FILE, binmode: true) data = zip_file.entries.select(&:file?).first.get_input_stream.read file << data file.close diff --git a/lib/active_admin_import/options.rb b/lib/active_admin_import/options.rb index 53f4a78..3ed1ce8 100644 --- a/lib/active_admin_import/options.rb +++ b/lib/active_admin_import/options.rb @@ -26,7 +26,7 @@ module Options def self.options_for(config, options = {}) unless options.key? :template_object - options[:template_object] = ActiveAdminImport::Model.new + options[:template_object] = -> { ActiveAdminImport::Model.new } end { diff --git a/lib/active_admin_import/version.rb b/lib/active_admin_import/version.rb index 5f9317a..6affade 100644 --- a/lib/active_admin_import/version.rb +++ b/lib/active_admin_import/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module ActiveAdminImport - VERSION = '4.1.0' + VERSION = '5.1.0' end diff --git a/spec/fixtures/files/authors.csv b/spec/fixtures/files/authors.csv index b523140..a79bff3 100644 --- a/spec/fixtures/files/authors.csv +++ b/spec/fixtures/files/authors.csv @@ -1,3 +1,3 @@ -Name,Last name,Birthday -John,Doe,1986-05-01 -Jane,Roe,1988-11-16 \ No newline at end of file +Birthday,Name,Last name +1986-05-01,John,Doe +1988-11-16,Jane,Roe diff --git a/spec/fixtures/files/authors_values_exceeded_headers.csv b/spec/fixtures/files/authors_values_exceeded_headers.csv new file mode 100644 index 0000000..e3ee481 --- /dev/null +++ b/spec/fixtures/files/authors_values_exceeded_headers.csv @@ -0,0 +1,3 @@ +Birthday,Name,Last name +1986-05-01,John,Doe +1988-11-16,Jane,Roe, exceeded value \ No newline at end of file diff --git a/spec/import_result_spec.rb b/spec/import_result_spec.rb index 918eab4..266ceed 100644 --- a/spec/import_result_spec.rb +++ b/spec/import_result_spec.rb @@ -5,17 +5,18 @@ context 'failed_message' do let(:import_result) { ActiveAdminImport::ImportResult.new } - before do - Author.create(name: 'John', last_name: 'Doe') - Author.create(name: 'Jane', last_name: 'Roe') + let(:failed_instances) do + [ + Author.new(last_name: 'Doe').tap {|r| r.errors.add(:last_name, :taken) }, + Author.new(name: "", last_name: 'Doe').tap {|r| r.errors.add(:name, :blank); r.errors.add(:last_name, :taken) }, + Author.new.tap {|r| r.errors.add(:base, 'custom') } + ] + end + + before do @result = double \ - failed_instances: [ - # {:last_name=>["has already been taken"]} - Author.create(name: 'Jim', last_name: 'Doe'), - # {:name=>["can't be blank"], :last_name=>["has already been taken"]} - Author.create(name: nil, last_name: 'Doe') - ] + failed_instances: failed_instances end it 'should work without any failed instances' do @@ -26,7 +27,7 @@ import_result.add(@result, 4) expect(import_result.failed_message) .to eq( - "Last name has already been taken - Doe ; Name can't be blank - , Last name has already been taken - Doe" + "Last name has already been taken - Doe ; Name can't be blank - , Last name has already been taken - Doe ; custom" ) end diff --git a/spec/import_spec.rb b/spec/import_spec.rb index 390edef..cc4db50 100644 --- a/spec/import_spec.rb +++ b/spec/import_spec.rb @@ -96,6 +96,38 @@ def upload_file!(name, ext = 'csv') end include_examples 'successful inserts for author' end + + context 'when template object passed like proc' do + before do + add_post_resource(template_object: -> { ActiveAdminImport::Model.new(author_id: author.id) }, + validate: true, + before_batch_import: lambda do |importer| + importer.csv_lines.map! { |row| row << importer.model.author_id } + importer.headers.merge!(:'Author Id' => :author_id) + end + ) + + visit '/admin/posts/import' + upload_file!(:posts_for_author) + end + + include_examples 'successful inserts for author' + + context 'after successful import try without file' do + let(:after_successful_import_do!) do + # reload page + visit '/admin/posts/import' + # submit form without file + find_button('Import').click + end + + it 'should render validation error' do + after_successful_import_do! + + expect(page).to have_content I18n.t('active_admin_import.no_file_error') + end + end + end end context 'for csv with author name' do @@ -371,6 +403,20 @@ def upload_file!(name, ext = 'csv') end end end + + context 'when zipped with Win1251 file' do + let(:options) do + attributes = { force_encoding: :auto } + { template_object: ActiveAdminImport::Model.new(attributes) } + end + it 'should import file' do + with_zipped_csv(:authors_win1251_win_endline) do + upload_file!(:authors_win1251_win_endline, :zip) + expect(page).to have_content 'Successfully imported 2 authors' + expect(Author.count).to eq(2) + end + end + end end context 'with different header attribute names' do @@ -410,6 +456,36 @@ def upload_file!(name, ext = 'csv') expect(Author.count).to eq(2) end end + + context 'with empty csv and auto detect encoding' do + let(:options) do + attributes = { force_encoding: :auto } + { template_object: ActiveAdminImport::Model.new(attributes) } + end + + before do + upload_file!(:empty) + end + + it 'should render warning' do + expect(page).to have_content I18n.t('active_admin_import.file_empty_error') + expect(Author.count).to eq(0) + end + end + + context 'with csv which has exceeded values' do + before do + upload_file!(:authors_values_exceeded_headers) + end + + it 'should render warning' do + # 5 columns: 'birthday, name, last_name, created_at, updated_at' + # 6 values: '"1988-11-16", "Jane", "Roe", " exceeded value", datetime, datetime' + msg = 'Number of values (6) exceeds number of columns (5)' + expect(page).to have_content I18n.t('active_admin_import.file_error', message: msg) + expect(Author.count).to eq(0) + end + end end context 'with callback procs options' do @@ -430,15 +506,40 @@ def upload_file!(name, ext = 'csv') upload_file!(:authors) expect(Author.count).to eq(2) end + + context 'when the option before_import raises a ActiveAdminImport::Exception' do + let(:options) { { before_import: ->(_) { raise ActiveAdminImport::Exception, 'error message' } } } + + before { upload_file!(:authors) } + + it 'should show error' do + expect(page).to have_content I18n.t('active_admin_import.file_error', message: 'error message') + expect(Author.count).to eq(0) + end + end + + context 'when the option before_batch_import raises a ActiveAdminImport::Exception' do + let(:options) { { before_batch_import: ->(_) { raise ActiveAdminImport::Exception, 'error message' } } } + + before { upload_file!(:authors) } + + it 'should show error' do + expect(page).to have_content I18n.t('active_admin_import.file_error', message: 'error message') + expect(Author.count).to eq(0) + end + end end end context "with slice_columns option" do + let(:batch_size) { 2 } + before do add_author_resource template_object: ActiveAdminImport::Model.new, before_batch_import: lambda { |importer| importer.batch_slice_columns(slice_columns) - } + }, + batch_size: batch_size visit "/admin/authors/import" upload_file!(:authors) end @@ -446,13 +547,23 @@ def upload_file!(name, ext = 'csv') context "slice last column and superfluous column" do let(:slice_columns) { %w(name last_name not_existing_column) } - it "should not fill `birthday` column" do - expect(Author.pluck(:name, :last_name, :birthday)).to match_array( - [ - ["Jane", "Roe", nil], - ["John", "Doe", nil] - ] - ) + shared_examples_for "birthday column removed" do + it "should not fill `birthday` column" do + expect(Author.pluck(:name, :last_name, :birthday)).to match_array( + [ + ["Jane", "Roe", nil], + ["John", "Doe", nil] + ] + ) + end + end + + it_behaves_like "birthday column removed" + + context "when doing more than one batch" do + let(:batch_size) { 1 } + + it_behaves_like "birthday column removed" end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index aded259..c8fd58e 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -20,6 +20,7 @@ require 'active_model' # require ActiveRecord to ensure that Ransack loads correctly require 'active_record' +require 'action_view' require 'active_admin' ActiveAdmin.application.load_paths = [ENV['RAILS_ROOT'] + '/app/admin'] require ENV['RAILS_ROOT'] + '/config/environment.rb' diff --git a/tasks/test.rake b/tasks/test.rake index 8def1e9..6699224 100644 --- a/tasks/test.rake +++ b/tasks/test.rake @@ -3,5 +3,5 @@ desc 'Creates a test rails app for the specs to run against' task :setup do require 'rails/version' system('mkdir spec/rails') unless File.exist?('spec/rails') - system "bundle exec rails new spec/rails/rails-#{Rails::VERSION::STRING} -m spec/support/rails_template.rb --skip-spring --skip-turbolinks" + system "bundle exec rails new spec/rails/rails-#{Rails::VERSION::STRING} -m spec/support/rails_template.rb --skip-spring --skip-turbolinks --skip-bootsnap" end