Skip to content

Commit

Permalink
Fix various issues with domain block import
Browse files Browse the repository at this point in the history
- stop using Paperclip for processing domain allow/block imports
- stop leaving temporary files
- better error handling
- assume CSV files are UTF-8-encoded
  • Loading branch information
ClearlyClaire committed Nov 16, 2022
1 parent ad84fd2 commit cad824d
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 45 deletions.
6 changes: 3 additions & 3 deletions app/controllers/admin/export_domain_allows_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ class ExportDomainAllowsController < BaseController

before_action :set_dummy_import!, only: [:new]

ROWS_PROCESSING_LIMIT = 20_000

def new
authorize :domain_allow, :create?
end
Expand All @@ -23,9 +21,11 @@ def import
authorize :domain_allow, :create?
begin
@import = Admin::Import.new(import_params)
return render :new unless @import.validate

parse_import_data!(export_headers)

@data.take(ROWS_PROCESSING_LIMIT).each do |row|
@data.take(Admin::Import::ROWS_PROCESSING_LIMIT).each do |row|
domain = row['#domain'].strip
next if DomainAllow.allowed?(domain)

Expand Down
6 changes: 3 additions & 3 deletions app/controllers/admin/export_domain_blocks_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ class ExportDomainBlocksController < BaseController

before_action :set_dummy_import!, only: [:new]

ROWS_PROCESSING_LIMIT = 20_000

def new
authorize :domain_block, :create?
end
Expand All @@ -23,12 +21,14 @@ def import
authorize :domain_block, :create?

@import = Admin::Import.new(import_params)
return render :new unless @import.validate

parse_import_data!(export_headers)

@global_private_comment = I18n.t('admin.export_domain_blocks.import.private_comment_template', source: @import.data_file_name, date: I18n.l(Time.now.utc))

@form = Form::DomainBlockBatch.new
@domain_blocks = @data.take(ROWS_PROCESSING_LIMIT).filter_map do |row|
@domain_blocks = @data.take(Admin::Import::ROWS_PROCESSING_LIMIT).filter_map do |row|
domain = row['#domain'].strip
next if DomainBlock.rule_for(domain).present?

Expand Down
8 changes: 4 additions & 4 deletions app/controllers/concerns/admin_export_controller_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ def import_params
params.require(:admin_import).permit(:data)
end

def import_data
Paperclip.io_adapters.for(@import.data).read
def import_data_path
params[:admin_import][:data].path
end

def parse_import_data!(default_headers)
data = CSV.parse(import_data, headers: true)
data = CSV.parse(import_data, headers: default_headers) unless data.headers&.first&.strip&.include?(default_headers[0])
data = CSV.read(import_data_path, headers: true, encoding: 'UTF-8')
data = CSV.read(import_data_path, headers: default_headers, encoding: 'UTF-8') unless data.headers&.first&.strip&.include?(default_headers[0])
@data = data.reject(&:blank?)
end
end
35 changes: 19 additions & 16 deletions app/models/admin/import.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,31 @@

# A non-activerecord helper class for csv upload
class Admin::Import
extend ActiveModel::Callbacks
include ActiveModel::Model
include Paperclip::Glue

FILE_TYPES = %w(text/plain text/csv application/csv).freeze
ROWS_PROCESSING_LIMIT = 20_000

# Paperclip required callbacks
define_model_callbacks :save, only: [:after]
define_model_callbacks :destroy, only: [:before, :after]
attr_accessor :data

attr_accessor :data_file_name, :data_content_type
validates :data, presence: true
validate :validate_data

has_attached_file :data
validates_attachment_content_type :data, content_type: FILE_TYPES
validates_attachment_presence :data
validates_with AdminImportValidator, on: :create

def save
run_callbacks :save
def data_file_name
data.original_filename
end

def destroy
run_callbacks :destroy
private

def validate_data
return if data.blank?

csv_data = CSV.read(data.path, encoding: 'UTF-8')

row_count = csv_data.size
row_count -= 1 if csv_data.first&.first == '#domain'

errors.add(:data, I18n.t('imports.errors.over_rows_processing_limit', count: ROWS_PROCESSING_LIMIT)) if row_count > ROWS_PROCESSING_LIMIT
rescue CSV::MalformedCSVError => e
errors.add(:data, I18n.t('imports.errors.invalid_csv_file', error: e.message))
end
end
19 changes: 0 additions & 19 deletions app/validators/admin_import_validator.rb

This file was deleted.

1 change: 1 addition & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1159,6 +1159,7 @@ en:
invalid_markup: 'contains invalid HTML markup: %{error}'
imports:
errors:
invalid_csv_file: 'Invalid CSV file. Error: %{error}'
over_rows_processing_limit: contains more than %{count} rows
modes:
merge: Merge
Expand Down

0 comments on commit cad824d

Please sign in to comment.