From f99d6b5ae831a09ed2feb8e11770b4e045f9716b Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Mon, 17 Jul 2023 20:45:24 +0200 Subject: [PATCH 01/22] Adding CSV importer with basic import and validation functions, and rspecs (refs #108) --- config/application.rb | 2 + lib/imports/csv_importer.rb | 77 +++++++++++++++++++++++++++ spec/lib/csv_importer_spec.rb | 38 +++++++++++++ spec/support/files/places.csv | 3 ++ spec/support/files/places_invalid.csv | 2 + 5 files changed, 122 insertions(+) create mode 100644 lib/imports/csv_importer.rb create mode 100644 spec/lib/csv_importer_spec.rb create mode 100644 spec/support/files/places.csv create mode 100644 spec/support/files/places_invalid.csv diff --git a/config/application.rb b/config/application.rb index 81bcf161..b6f89947 100644 --- a/config/application.rb +++ b/config/application.rb @@ -19,6 +19,8 @@ class Application < Rails::Application # config.time_zone = "Central Time (US & Canada)" # config.eager_load_paths << Rails.root.join("extras") + + config.autoload_paths += %W(#{config.root}/lib) config.action_cable.mount_path = '/cable' diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb new file mode 100644 index 00000000..d0bb9439 --- /dev/null +++ b/lib/imports/csv_importer.rb @@ -0,0 +1,77 @@ +require 'csv' + +class Imports::CsvImporter + attr_reader :invalid_rows + + + def initialize(file,layer_id) + @file = file + @invalid_rows = [] + @layer = Layer.find(layer_id) + @existing_titles = [] + end + + def import + CSV.foreach(@file.path, headers: true) do |row| + unless valid_row?(row) + @invalid_rows << row + next + end + + title = sanitize(row['title']) + if @existing_titles.include?(title) + @invalid_rows << row + else + @existing_titles << title + end + end + + if @invalid_rows.empty? + process_valid_rows + Rails.logger.info('CSV import completed successfully!') + else + handle_invalid_rows + end + end + + private + + def valid_row?(row) + place = Place.new(title: sanitize(row['title']), teaser: sanitize(row['teaser']), layer_id: @layer.id) + place.valid? + end + + def process_valid_rows + CSV.foreach(@file.path, headers: true) do |row| + title = sanitize(row['title']) + unless @existing_titles.include?(title) + place = Place.new(title: title, teaser: sanitize(row['teaser']), layer_id: @layer.id) + place.save! + @existing_titles << title + else + Rails.logger.error("Place already exists! #{title}") + puts "Place already exists" + end + end + end + + def handle_invalid_rows + error_messages = @invalid_rows.map do |row| + place = Place.new(title: sanitize(row['title']), teaser: sanitize(row['teaser'])) + place.valid? + place.errors.full_messages + end + + # Implement how you want to handle the invalid rows + # For example, you can log the error messages or store them in a separate file + error_messages.each_with_index do |messages, index| + Rails.logger.error("Invalid row #{index + 1}: #{@invalid_rows[index]} - Errors: #{messages.join(', ')}") + end + end + + def sanitize(value) + # Implement any sanitization logic you require + # For example, you can strip leading/trailing whitespace: value.strip + value + end +end diff --git a/spec/lib/csv_importer_spec.rb b/spec/lib/csv_importer_spec.rb new file mode 100644 index 00000000..a8849a8e --- /dev/null +++ b/spec/lib/csv_importer_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Imports::CsvImporter do + describe '#import' do + let(:file) { Rack::Test::UploadedFile.new('spec/support/files/places.csv', 'text/csv') } + + let(:layer) { create(:layer) } + + context 'with valid CSV' do + it 'creates new Place records from valid rows' do + importer = Imports::CsvImporter.new(file,layer.id) + + expect { + importer.import + }.to change(Place, :count).by(3) # Assuming the CSV file has 3 valid rows + + # Additional assertions if needed + expect(Place.pluck(:title)).to contain_exactly('Place 1', 'Place 2', 'Place 3') + end + end + + context 'with invalid CSV' do + it 'handles invalid rows and does not create Place records' do + invalid_file = Rack::Test::UploadedFile.new('spec/support/files/places_invalid.csv', 'text/csv') + + importer = Imports::CsvImporter.new(invalid_file,layer.id) + + expect { + importer.import + }.not_to change(Place, :count) + + expect(importer.invalid_rows.count).to eq(1) + end + end + end +end diff --git a/spec/support/files/places.csv b/spec/support/files/places.csv new file mode 100644 index 00000000..20bfff9e --- /dev/null +++ b/spec/support/files/places.csv @@ -0,0 +1,3 @@ +id,title,teaser,text,annotations,startdate,enddate,lat,lon,location,address,zip,city,country +239,August Lütgens-Park,"Ein etwas versteckt gelegenes und daher wohl eher unbekanntes Fleckchen Grün befindet sich auf dem ehemaligen Gelände des früheren Altonaer Krankenhauses. Alteingesessene Hamburger werden keine Probleme haben, es zu finden. Für alle anderen weist ein Willkommensschild den Weg in die kleine grüne Oase des August-Lütgens-Parks.","",,,,53.55676430365751,9.947492480278017,"","",22765,Hamburg (Altona-Nord), +240,Wohlerspark,"Der ehemalige ""Begräbnisplatz"" der 'evangelisch-lutherischen Kirchengemeinde' St. Johannis >FOO%lt; wurde 1831 eröffnet und bis 1979 als Friedhof genutzt. Allerdings wurden bereits seit 1897 keine neuen Grabstellen mehr zugelassen. Durch den Zweiten Weltkrieg erlitt der Friedhof im Sommer 1943 große Schäden an seiner Substanz. Zusätzlich wurden in der Nachkriegszeit zum Zweck der Feuerholzgewinnung zahlreiche der alten Alleebäume abgeholzt. Noch bis 1948 bauten die Bürger Hamburgs zwischen den Gräbern Kartoffeln und Gemüse an. Um 1979 wurde der Friedhof schließlich zur öffentlichen Grünfläche umgewidmet, indem man ihn umgestaltete und als Wohlerspark der Bevölkerung übergab.","",,,,53.557909553720144,9.953098297119142, ,Norderreihe 2,22767,Hamburg (Altona-Altstadt),"" diff --git a/spec/support/files/places_invalid.csv b/spec/support/files/places_invalid.csv new file mode 100644 index 00000000..ba30926d --- /dev/null +++ b/spec/support/files/places_invalid.csv @@ -0,0 +1,2 @@ +id,title,teaser,text,annotations,startdate,enddate,lat,lon,location,address,zip,city,country +"","","",,,,,"","","" From 35b783f8c0171be30fd6f227b42837f82333bb21 Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Wed, 19 Jul 2023 19:24:49 +0200 Subject: [PATCH 02/22] Rubocop. Adding frozen_string_literal --- lib/imports/csv_importer.rb | 17 +++++++++-------- spec/lib/csv_importer_spec.rb | 14 +++++++------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb index d0bb9439..a245b353 100644 --- a/lib/imports/csv_importer.rb +++ b/lib/imports/csv_importer.rb @@ -1,10 +1,11 @@ +# frozen_string_literal: true + require 'csv' - + class Imports::CsvImporter attr_reader :invalid_rows - - def initialize(file,layer_id) + def initialize(file, layer_id) @file = file @invalid_rows = [] @layer = Layer.find(layer_id) @@ -44,13 +45,13 @@ def valid_row?(row) def process_valid_rows CSV.foreach(@file.path, headers: true) do |row| title = sanitize(row['title']) - unless @existing_titles.include?(title) - place = Place.new(title: title, teaser: sanitize(row['teaser']), layer_id: @layer.id) + if @existing_titles.include?(title) + Rails.logger.error("Place already exists! #{title}") + puts 'Place already exists' + else + place = Place.new(title: title, teaser: sanitize(row['teaser']), layer_id: @layer.id) place.save! @existing_titles << title - else - Rails.logger.error("Place already exists! #{title}") - puts "Place already exists" end end end diff --git a/spec/lib/csv_importer_spec.rb b/spec/lib/csv_importer_spec.rb index a8849a8e..3d6bfdec 100644 --- a/spec/lib/csv_importer_spec.rb +++ b/spec/lib/csv_importer_spec.rb @@ -10,12 +10,12 @@ context 'with valid CSV' do it 'creates new Place records from valid rows' do - importer = Imports::CsvImporter.new(file,layer.id) + importer = Imports::CsvImporter.new(file, layer.id) - expect { + # Assuming the CSV file has 3 valid rows + expect do importer.import - }.to change(Place, :count).by(3) # Assuming the CSV file has 3 valid rows - + end.to change(Place, :count).by(3) # Additional assertions if needed expect(Place.pluck(:title)).to contain_exactly('Place 1', 'Place 2', 'Place 3') end @@ -25,11 +25,11 @@ it 'handles invalid rows and does not create Place records' do invalid_file = Rack::Test::UploadedFile.new('spec/support/files/places_invalid.csv', 'text/csv') - importer = Imports::CsvImporter.new(invalid_file,layer.id) + importer = Imports::CsvImporter.new(invalid_file, layer.id) - expect { + expect do importer.import - }.not_to change(Place, :count) + end.not_to change(Place, :count) expect(importer.invalid_rows.count).to eq(1) end From 15eed71df4970b15722f77da193d37f5cc25e860 Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Wed, 19 Jul 2023 19:30:30 +0200 Subject: [PATCH 03/22] Rubocop/2 --- app/helpers/icons_helper.rb | 2 +- spec/controllers/public/submissions_controller_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/helpers/icons_helper.rb b/app/helpers/icons_helper.rb index 7813ec42..74238713 100644 --- a/app/helpers/icons_helper.rb +++ b/app/helpers/icons_helper.rb @@ -8,7 +8,7 @@ def icon_url(file) end def icon_linktag(file) - return unless file.attached? + return unless file.attached? "".html_safe end diff --git a/spec/controllers/public/submissions_controller_spec.rb b/spec/controllers/public/submissions_controller_spec.rb index 55fc06f1..6e1de8d1 100644 --- a/spec/controllers/public/submissions_controller_spec.rb +++ b/spec/controllers/public/submissions_controller_spec.rb @@ -236,7 +236,7 @@ session[:submission_id] = submission.id image_attributes = FactoryBot.attributes_for(:image, :with_file) expect do - post :create_image, params: { image: image_attributes, layer_id: @layer.id, place_id: submission.place.id, submission_id: submission.id, locale: 'de' }, session: valid_session + post :create_image, params: { image: image_attributes, layer_id: @layer.id, place_id: submission.place.id, submission_id: submission.id, locale: 'de' }, session: valid_session end.to change(Image, :count).by(0) expect(response).to have_http_status(302) expect(response).to redirect_to(submission_finished_url(layer_id: @layer.id, place_id: @place.id, submission_id: submission.id)) From 1dded04b6472661f67664c3d83eae6491c2b75fc Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Fri, 21 Jul 2023 11:24:22 +0200 Subject: [PATCH 04/22] Adding CSV header validation, define required headers and fields, adding some validations (refs #108) --- app/models/place.rb | 5 +++ lib/imports/csv_importer.rb | 31 ++++++++++++---- spec/lib/csv_importer_spec.rb | 35 ++++++++++++++++--- spec/support/files/places.csv | 4 +-- spec/support/files/places_invalid_header.csv | 2 ++ spec/support/files/places_invalid_lat.csv | 2 ++ .../{places_invalid.csv => places_nodata.csv} | 0 7 files changed, 66 insertions(+), 13 deletions(-) create mode 100644 spec/support/files/places_invalid_header.csv create mode 100644 spec/support/files/places_invalid_lat.csv rename spec/support/files/{places_invalid.csv => places_nodata.csv} (100%) diff --git a/app/models/place.rb b/app/models/place.rb index b65b5f7a..99073f09 100644 --- a/app/models/place.rb +++ b/app/models/place.rb @@ -29,6 +29,11 @@ class Place < ApplicationRecord validates :title, presence: true validate :check_audio_format + validates :lat, presence: true, format: { with: /\A-?\d+(\.\d+)?\z/, message: 'should be a valid latitude value' } + validates :lon, presence: true, format: { with: /\A-?\d+(\.\d+)?\z/, message: 'should be a valid longitude value' } + validates :lat, presence: true, numericality: { greater_than_or_equal_to: -90, less_than_or_equal_to: 90 } + validates :lon, presence: true, numericality: { greater_than_or_equal_to: -180, less_than_or_equal_to: 180 } + scope :sorted_by_startdate, -> { order(startdate: :asc) } scope :sorted_by_title, -> { order(title: :asc) } diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb index a245b353..00620152 100644 --- a/lib/imports/csv_importer.rb +++ b/lib/imports/csv_importer.rb @@ -5,6 +5,12 @@ class Imports::CsvImporter attr_reader :invalid_rows + REQUIRED_FIELDS = %w[title lat lon].freeze + + ALLOWED_FIELDS = %[title subtitle teaser text link startdate startdate_date startdate_time enddate enddate_date enddate_time lat lon location address zip city country published featured sensitive sensitive_radius shy imagelink layer_id icon_id relations_tos relations_froms].freeze + + # TODO: param: update or skip existing place? + def initialize(file, layer_id) @file = file @invalid_rows = [] @@ -13,6 +19,9 @@ def initialize(file, layer_id) end def import + + validate_header + CSV.foreach(@file.path, headers: true) do |row| unless valid_row?(row) @invalid_rows << row @@ -23,7 +32,8 @@ def import if @existing_titles.include?(title) @invalid_rows << row else - @existing_titles << title + # FIXME + # @existing_titles << title end end @@ -37,19 +47,28 @@ def import private + def validate_header + headers = CSV.read(@file.path, headers: true).headers + missing_fields = REQUIRED_FIELDS - headers + + raise StandardError, "Missing required fields: #{missing_fields.join(', ')}" if missing_fields.any? + + end + def valid_row?(row) - place = Place.new(title: sanitize(row['title']), teaser: sanitize(row['teaser']), layer_id: @layer.id) + place = Place.new(title: sanitize(row['title']), lat: sanitize(row['lat']), lon: sanitize(row['lon']), layer_id: @layer.id) place.valid? end def process_valid_rows CSV.foreach(@file.path, headers: true) do |row| title = sanitize(row['title']) + puts "Process #{title}" if @existing_titles.include?(title) Rails.logger.error("Place already exists! #{title}") puts 'Place already exists' else - place = Place.new(title: title, teaser: sanitize(row['teaser']), layer_id: @layer.id) + place = Place.new(title: title, lat: sanitize(row['lat']), lon: sanitize(row['lon']), layer_id: @layer.id) place.save! @existing_titles << title end @@ -58,7 +77,7 @@ def process_valid_rows def handle_invalid_rows error_messages = @invalid_rows.map do |row| - place = Place.new(title: sanitize(row['title']), teaser: sanitize(row['teaser'])) + place = Place.new(title: sanitize(row['title']), lat: sanitize(row['lat']), lon: sanitize(row['lon']), layer_id: @layer.id) place.valid? place.errors.full_messages end @@ -72,7 +91,7 @@ def handle_invalid_rows def sanitize(value) # Implement any sanitization logic you require - # For example, you can strip leading/trailing whitespace: value.strip - value + # strip leading/trailing whitespace: value.strip + value.strip end end diff --git a/spec/lib/csv_importer_spec.rb b/spec/lib/csv_importer_spec.rb index 3d6bfdec..bef8dd0b 100644 --- a/spec/lib/csv_importer_spec.rb +++ b/spec/lib/csv_importer_spec.rb @@ -10,20 +10,45 @@ context 'with valid CSV' do it 'creates new Place records from valid rows' do + + Place.destroy_all importer = Imports::CsvImporter.new(file, layer.id) # Assuming the CSV file has 3 valid rows expect do importer.import - end.to change(Place, :count).by(3) - # Additional assertions if needed - expect(Place.pluck(:title)).to contain_exactly('Place 1', 'Place 2', 'Place 3') + end.to change(Place, :count).by(2) + + expect(Place.pluck(:title)).to contain_exactly('Place 1', 'Place 2') end end context 'with invalid CSV' do - it 'handles invalid rows and does not create Place records' do - invalid_file = Rack::Test::UploadedFile.new('spec/support/files/places_invalid.csv', 'text/csv') + it 'handles empty rows and does not create Place records' do + invalid_file = Rack::Test::UploadedFile.new('spec/support/files/places_nodata.csv', 'text/csv') + + importer = Imports::CsvImporter.new(invalid_file,layer.id) + + expect { + importer.import + }.not_to change(Place, :count) + + expect(importer.invalid_rows.count).to eq(1) + end + + it 'handles wrong csv header and does not create Place records' do + invalid_file = Rack::Test::UploadedFile.new('spec/support/files/places_invalid_header.csv', 'text/csv') + + importer = Imports::CsvImporter.new(invalid_file,layer.id) + + expect { + importer.import + }.to raise_error(StandardError) + + end + + it 'handles invalid row with wrong lat value and does not create Place records' do + invalid_file = Rack::Test::UploadedFile.new('spec/support/files/places_invalid_lat.csv', 'text/csv') importer = Imports::CsvImporter.new(invalid_file, layer.id) diff --git a/spec/support/files/places.csv b/spec/support/files/places.csv index 20bfff9e..7b688e2c 100644 --- a/spec/support/files/places.csv +++ b/spec/support/files/places.csv @@ -1,3 +1,3 @@ id,title,teaser,text,annotations,startdate,enddate,lat,lon,location,address,zip,city,country -239,August Lütgens-Park,"Ein etwas versteckt gelegenes und daher wohl eher unbekanntes Fleckchen Grün befindet sich auf dem ehemaligen Gelände des früheren Altonaer Krankenhauses. Alteingesessene Hamburger werden keine Probleme haben, es zu finden. Für alle anderen weist ein Willkommensschild den Weg in die kleine grüne Oase des August-Lütgens-Parks.","",,,,53.55676430365751,9.947492480278017,"","",22765,Hamburg (Altona-Nord), -240,Wohlerspark,"Der ehemalige ""Begräbnisplatz"" der 'evangelisch-lutherischen Kirchengemeinde' St. Johannis >FOO%lt; wurde 1831 eröffnet und bis 1979 als Friedhof genutzt. Allerdings wurden bereits seit 1897 keine neuen Grabstellen mehr zugelassen. Durch den Zweiten Weltkrieg erlitt der Friedhof im Sommer 1943 große Schäden an seiner Substanz. Zusätzlich wurden in der Nachkriegszeit zum Zweck der Feuerholzgewinnung zahlreiche der alten Alleebäume abgeholzt. Noch bis 1948 bauten die Bürger Hamburgs zwischen den Gräbern Kartoffeln und Gemüse an. Um 1979 wurde der Friedhof schließlich zur öffentlichen Grünfläche umgewidmet, indem man ihn umgestaltete und als Wohlerspark der Bevölkerung übergab.","",,,,53.557909553720144,9.953098297119142, ,Norderreihe 2,22767,Hamburg (Altona-Altstadt),"" +239,Place 1,"Ein etwas versteckt gelegenes und daher wohl eher unbekanntes Fleckchen Grün.","",,,,53.55,9.94,"","",22765,Hamburg (Altona-Nord), +240,Place 2,"Zusätzlich wurden zum Zweck der Feuerholzgewinnung zahlreiche der alten Alleebäume abgeholzt.","",,,,53.55,9.95, ,Norderreihe 2,22767,Hamburg (Altona-Altstadt),"" diff --git a/spec/support/files/places_invalid_header.csv b/spec/support/files/places_invalid_header.csv new file mode 100644 index 00000000..b43d1220 --- /dev/null +++ b/spec/support/files/places_invalid_header.csv @@ -0,0 +1,2 @@ +id,titleX,latX,lonX +"","","","" diff --git a/spec/support/files/places_invalid_lat.csv b/spec/support/files/places_invalid_lat.csv new file mode 100644 index 00000000..8c0bd5f3 --- /dev/null +++ b/spec/support/files/places_invalid_lat.csv @@ -0,0 +1,2 @@ +id,title,teaser,text,annotations,startdate,enddate,lat,lon,location,address,zip,city,country +"id","title","teaser","text","annotations","startdate","enddate","1,2","","" diff --git a/spec/support/files/places_invalid.csv b/spec/support/files/places_nodata.csv similarity index 100% rename from spec/support/files/places_invalid.csv rename to spec/support/files/places_nodata.csv From 53bc39a5bb192b40bb68007444e3a10ef5d7f224 Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Fri, 21 Jul 2023 11:30:37 +0200 Subject: [PATCH 05/22] Rubocop'd --- app/models/place.rb | 3 +-- lib/imports/csv_importer.rb | 8 ++------ spec/lib/csv_importer_spec.rb | 14 ++++++-------- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/app/models/place.rb b/app/models/place.rb index 99073f09..a4c0865e 100644 --- a/app/models/place.rb +++ b/app/models/place.rb @@ -29,12 +29,11 @@ class Place < ApplicationRecord validates :title, presence: true validate :check_audio_format - validates :lat, presence: true, format: { with: /\A-?\d+(\.\d+)?\z/, message: 'should be a valid latitude value' } + validates :lat, presence: true, format: { with: /\A-?\d+(\.\d+)?\z/, message: 'should be a valid latitude value' } validates :lon, presence: true, format: { with: /\A-?\d+(\.\d+)?\z/, message: 'should be a valid longitude value' } validates :lat, presence: true, numericality: { greater_than_or_equal_to: -90, less_than_or_equal_to: 90 } validates :lon, presence: true, numericality: { greater_than_or_equal_to: -180, less_than_or_equal_to: 180 } - scope :sorted_by_startdate, -> { order(startdate: :asc) } scope :sorted_by_title, -> { order(title: :asc) } diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb index 00620152..1d421fce 100644 --- a/lib/imports/csv_importer.rb +++ b/lib/imports/csv_importer.rb @@ -7,7 +7,7 @@ class Imports::CsvImporter REQUIRED_FIELDS = %w[title lat lon].freeze - ALLOWED_FIELDS = %[title subtitle teaser text link startdate startdate_date startdate_time enddate enddate_date enddate_time lat lon location address zip city country published featured sensitive sensitive_radius shy imagelink layer_id icon_id relations_tos relations_froms].freeze + ALLOWED_FIELDS = %(title subtitle teaser text link startdate startdate_date startdate_time enddate enddate_date enddate_time lat lon location address zip city country published featured sensitive sensitive_radius shy imagelink layer_id icon_id relations_tos relations_froms) # TODO: param: update or skip existing place? @@ -19,7 +19,6 @@ def initialize(file, layer_id) end def import - validate_header CSV.foreach(@file.path, headers: true) do |row| @@ -31,7 +30,7 @@ def import title = sanitize(row['title']) if @existing_titles.include?(title) @invalid_rows << row - else + # else # FIXME # @existing_titles << title end @@ -52,7 +51,6 @@ def validate_header missing_fields = REQUIRED_FIELDS - headers raise StandardError, "Missing required fields: #{missing_fields.join(', ')}" if missing_fields.any? - end def valid_row?(row) @@ -63,10 +61,8 @@ def valid_row?(row) def process_valid_rows CSV.foreach(@file.path, headers: true) do |row| title = sanitize(row['title']) - puts "Process #{title}" if @existing_titles.include?(title) Rails.logger.error("Place already exists! #{title}") - puts 'Place already exists' else place = Place.new(title: title, lat: sanitize(row['lat']), lon: sanitize(row['lon']), layer_id: @layer.id) place.save! diff --git a/spec/lib/csv_importer_spec.rb b/spec/lib/csv_importer_spec.rb index bef8dd0b..4df18f0d 100644 --- a/spec/lib/csv_importer_spec.rb +++ b/spec/lib/csv_importer_spec.rb @@ -10,7 +10,6 @@ context 'with valid CSV' do it 'creates new Place records from valid rows' do - Place.destroy_all importer = Imports::CsvImporter.new(file, layer.id) @@ -27,11 +26,11 @@ it 'handles empty rows and does not create Place records' do invalid_file = Rack::Test::UploadedFile.new('spec/support/files/places_nodata.csv', 'text/csv') - importer = Imports::CsvImporter.new(invalid_file,layer.id) + importer = Imports::CsvImporter.new(invalid_file, layer.id) - expect { + expect do importer.import - }.not_to change(Place, :count) + end.not_to change(Place, :count) expect(importer.invalid_rows.count).to eq(1) end @@ -39,12 +38,11 @@ it 'handles wrong csv header and does not create Place records' do invalid_file = Rack::Test::UploadedFile.new('spec/support/files/places_invalid_header.csv', 'text/csv') - importer = Imports::CsvImporter.new(invalid_file,layer.id) + importer = Imports::CsvImporter.new(invalid_file, layer.id) - expect { + expect do importer.import - }.to raise_error(StandardError) - + end.to raise_error(StandardError) end it 'handles invalid row with wrong lat value and does not create Place records' do From f5c7f2f8ab90e7895eebe94919622f1cee885ab7 Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Sat, 22 Jul 2023 15:21:10 +0200 Subject: [PATCH 06/22] Sanitize/strip HTML from inputs --- lib/imports/csv_importer.rb | 5 ++++- spec/lib/csv_importer_spec.rb | 13 ++++++++++++- spec/support/files/places_with_html.csv | 2 ++ 3 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 spec/support/files/places_with_html.csv diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb index 1d421fce..52daac89 100644 --- a/lib/imports/csv_importer.rb +++ b/lib/imports/csv_importer.rb @@ -2,6 +2,8 @@ require 'csv' +include ActionView::Helpers::SanitizeHelper + class Imports::CsvImporter attr_reader :invalid_rows @@ -64,7 +66,8 @@ def process_valid_rows if @existing_titles.include?(title) Rails.logger.error("Place already exists! #{title}") else - place = Place.new(title: title, lat: sanitize(row['lat']), lon: sanitize(row['lon']), layer_id: @layer.id) + # TODO: write all fields within ALLOWED_FIELDS array + place = Place.new(title: title, teaser: strip_tags(row['teaser']).strip, lat: sanitize(row['lat']), lon: sanitize(row['lon']), layer_id: @layer.id) place.save! @existing_titles << title end diff --git a/spec/lib/csv_importer_spec.rb b/spec/lib/csv_importer_spec.rb index 4df18f0d..34a4cc55 100644 --- a/spec/lib/csv_importer_spec.rb +++ b/spec/lib/csv_importer_spec.rb @@ -10,7 +10,6 @@ context 'with valid CSV' do it 'creates new Place records from valid rows' do - Place.destroy_all importer = Imports::CsvImporter.new(file, layer.id) # Assuming the CSV file has 3 valid rows @@ -56,6 +55,18 @@ expect(importer.invalid_rows.count).to eq(1) end + + it 'sanitizes a title with js and html', focus: true do + file_with_html = Rack::Test::UploadedFile.new('spec/support/files/places_with_html.csv', 'text/csv') + + importer = Imports::CsvImporter.new(file_with_html, layer.id) + + expect do + importer.import + end.to change(Place, :count).by(1) + + expect(Place.first.teaser).to eq('Ein etwas versteckt gelegenes Fleckchen') + end end end end diff --git a/spec/support/files/places_with_html.csv b/spec/support/files/places_with_html.csv new file mode 100644 index 00000000..dd1059dc --- /dev/null +++ b/spec/support/files/places_with_html.csv @@ -0,0 +1,2 @@ +id,title,teaser,text,annotations,startdate,enddate,lat,lon,location,address,zip,city,country +239,"Place 1","Ein etwas versteckt gelegenes Fleckchen ","",,,,53.55,9.94,"","",22765,Hamburg (Altona-Nord) From 6e920a87bf18d288cae9280ccc9523ebe9609612 Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Sat, 22 Jul 2023 15:21:10 +0200 Subject: [PATCH 07/22] Add allowed_fields routine and create all allowed fields with Place.new --- lib/imports/csv_importer.rb | 48 +++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb index 52daac89..19aae732 100644 --- a/lib/imports/csv_importer.rb +++ b/lib/imports/csv_importer.rb @@ -9,7 +9,9 @@ class Imports::CsvImporter REQUIRED_FIELDS = %w[title lat lon].freeze - ALLOWED_FIELDS = %(title subtitle teaser text link startdate startdate_date startdate_time enddate enddate_date enddate_time lat lon location address zip city country published featured sensitive sensitive_radius shy imagelink layer_id icon_id relations_tos relations_froms) + ALLOWED_FIELDS = %w[title subtitle teaser text link startdate startdate_date startdate_time enddate enddate_date enddate_time lat lon location address zip city country published featured sensitive sensitive_radius shy imagelink layer_id icon_id relations_tos relations_froms].freeze + + TEXT_FIELDS = %w[title subtitle teaser text location address zip city country].freeze # TODO: param: update or skip existing place? @@ -24,22 +26,30 @@ def import validate_header CSV.foreach(@file.path, headers: true) do |row| - unless valid_row?(row) - @invalid_rows << row + processed_row = row.to_hash.slice(*ALLOWED_FIELDS) + + unless !processed_row.empty? && valid_row?(processed_row) + @invalid_rows << processed_row next end - title = sanitize(row['title']) + # dupe handling + title = sanitize(processed_row['title']) + if @existing_titles.include?(title) - @invalid_rows << row + # TODO! + # skip existing rows with this title + @invalid_rows << processed_row # else - # FIXME + # update existing row # @existing_titles << title + else + @existing_titles << title + create_place(processed_row) end end if @invalid_rows.empty? - process_valid_rows Rails.logger.info('CSV import completed successfully!') else handle_invalid_rows @@ -48,11 +58,19 @@ def import private + def required_fields_present?(row) + REQUIRED_FIELDS.all? { |field| row[field].present? } + end + def validate_header headers = CSV.read(@file.path, headers: true).headers missing_fields = REQUIRED_FIELDS - headers raise StandardError, "Missing required fields: #{missing_fields.join(', ')}" if missing_fields.any? + + processable_fields = headers - ALLOWED_FIELDS + + raise StandardError, 'Now allowed fields found if missing_fields.any?' if processable_fields.empty? end def valid_row?(row) @@ -74,6 +92,22 @@ def process_valid_rows end end + def create_place(row) + place_attrs = {} + ALLOWED_FIELDS.each do |field| + puts "#{field} #{row[field]}" + if TEXT_FIELDS.include?(field) + place_attrs[field.to_sym] = strip_tags(row[field]).strip if row[field] + elsif row[field] + place_attrs[field.to_sym] = sanitize(row[field]) + end + end + place_attrs[:layer_id] = @layer.id unless place_attrs[:layer_id] + + place = Place.new(place_attrs) + place.save + end + def handle_invalid_rows error_messages = @invalid_rows.map do |row| place = Place.new(title: sanitize(row['title']), lat: sanitize(row['lat']), lon: sanitize(row['lon']), layer_id: @layer.id) From 5bd60c2e6bb63eef5fd1d28a54ae3ced5ed37b08 Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Mon, 24 Jul 2023 14:56:51 +0200 Subject: [PATCH 08/22] Rspec for nonallowed fields, improve sanitzer --- lib/imports/csv_importer.rb | 42 +++++++------------ spec/lib/csv_importer_spec.rb | 16 ++++++- ...places_valid_but_with_not_allowed_data.csv | 3 ++ 3 files changed, 31 insertions(+), 30 deletions(-) create mode 100644 spec/support/files/places_valid_but_with_not_allowed_data.csv diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb index 19aae732..6292e58e 100644 --- a/lib/imports/csv_importer.rb +++ b/lib/imports/csv_importer.rb @@ -5,7 +5,7 @@ include ActionView::Helpers::SanitizeHelper class Imports::CsvImporter - attr_reader :invalid_rows + attr_reader :invalid_rows, :unprocessable_fields # values needed for testing REQUIRED_FIELDS = %w[title lat lon].freeze @@ -20,6 +20,7 @@ def initialize(file, layer_id) @invalid_rows = [] @layer = Layer.find(layer_id) @existing_titles = [] + @unprocessable_fields = [] end def import @@ -34,7 +35,7 @@ def import end # dupe handling - title = sanitize(processed_row['title']) + title = do_sanitize(processed_row['title']) if @existing_titles.include?(title) # TODO! @@ -64,34 +65,22 @@ def required_fields_present?(row) def validate_header headers = CSV.read(@file.path, headers: true).headers - missing_fields = REQUIRED_FIELDS - headers + missing_fields = REQUIRED_FIELDS - headers raise StandardError, "Missing required fields: #{missing_fields.join(', ')}" if missing_fields.any? - processable_fields = headers - ALLOWED_FIELDS + @unprocessable_fields = headers - ALLOWED_FIELDS + Rails.logger.error('Not allowed fields found (and skipped)') unless @unprocessable_fields.empty? - raise StandardError, 'Now allowed fields found if missing_fields.any?' if processable_fields.empty? + processable_fields = headers - @unprocessable_fields + raise StandardError, 'No allowed fields found' if processable_fields.empty? end def valid_row?(row) - place = Place.new(title: sanitize(row['title']), lat: sanitize(row['lat']), lon: sanitize(row['lon']), layer_id: @layer.id) + place = Place.new(title: do_sanitize(row['title']), lat: do_sanitize(row['lat']), lon: do_sanitize(row['lon']), layer_id: @layer.id) place.valid? end - def process_valid_rows - CSV.foreach(@file.path, headers: true) do |row| - title = sanitize(row['title']) - if @existing_titles.include?(title) - Rails.logger.error("Place already exists! #{title}") - else - # TODO: write all fields within ALLOWED_FIELDS array - place = Place.new(title: title, teaser: strip_tags(row['teaser']).strip, lat: sanitize(row['lat']), lon: sanitize(row['lon']), layer_id: @layer.id) - place.save! - @existing_titles << title - end - end - end - def create_place(row) place_attrs = {} ALLOWED_FIELDS.each do |field| @@ -99,7 +88,7 @@ def create_place(row) if TEXT_FIELDS.include?(field) place_attrs[field.to_sym] = strip_tags(row[field]).strip if row[field] elsif row[field] - place_attrs[field.to_sym] = sanitize(row[field]) + place_attrs[field.to_sym] = do_sanitize(row[field]) end end place_attrs[:layer_id] = @layer.id unless place_attrs[:layer_id] @@ -110,21 +99,18 @@ def create_place(row) def handle_invalid_rows error_messages = @invalid_rows.map do |row| - place = Place.new(title: sanitize(row['title']), lat: sanitize(row['lat']), lon: sanitize(row['lon']), layer_id: @layer.id) + place = Place.new(title: do_sanitize(row['title']), lat: do_sanitize(row['lat']), lon: do_sanitize(row['lon']), layer_id: @layer.id) place.valid? place.errors.full_messages end - # Implement how you want to handle the invalid rows - # For example, you can log the error messages or store them in a separate file error_messages.each_with_index do |messages, index| Rails.logger.error("Invalid row #{index + 1}: #{@invalid_rows[index]} - Errors: #{messages.join(', ')}") end end - def sanitize(value) - # Implement any sanitization logic you require - # strip leading/trailing whitespace: value.strip - value.strip + def do_sanitize(value) + # call rails sanitizer + sanitize(value).strip end end diff --git a/spec/lib/csv_importer_spec.rb b/spec/lib/csv_importer_spec.rb index 34a4cc55..ac1a8ed7 100644 --- a/spec/lib/csv_importer_spec.rb +++ b/spec/lib/csv_importer_spec.rb @@ -12,13 +12,25 @@ it 'creates new Place records from valid rows' do importer = Imports::CsvImporter.new(file, layer.id) - # Assuming the CSV file has 3 valid rows expect do importer.import end.to change(Place, :count).by(2) expect(Place.pluck(:title)).to contain_exactly('Place 1', 'Place 2') end + it 'creates new Place records from valid rows except non-allowed columns' do + file = Rack::Test::UploadedFile.new('spec/support/files/places_valid_but_with_not_allowed_data.csv', 'text/csv') + + importer = Imports::CsvImporter.new(file, layer.id) + + expect do + importer.import + end.to change(Place, :count).by(2) + + expect(importer.unprocessable_fields).to contain_exactly('id', 'annotations', 'unknown') + + # TODO: test outcome + end end context 'with invalid CSV' do @@ -56,7 +68,7 @@ expect(importer.invalid_rows.count).to eq(1) end - it 'sanitizes a title with js and html', focus: true do + it 'sanitizes a title with js and html' do file_with_html = Rack::Test::UploadedFile.new('spec/support/files/places_with_html.csv', 'text/csv') importer = Imports::CsvImporter.new(file_with_html, layer.id) diff --git a/spec/support/files/places_valid_but_with_not_allowed_data.csv b/spec/support/files/places_valid_but_with_not_allowed_data.csv new file mode 100644 index 00000000..aa2983f6 --- /dev/null +++ b/spec/support/files/places_valid_but_with_not_allowed_data.csv @@ -0,0 +1,3 @@ +id,title,teaser,text,annotations,startdate,enddate,lat,lon,location,address,zip,city,country,unknown +239,Place 1,"Ein etwas versteckt gelegenes und daher wohl eher unbekanntes Fleckchen Grün.","a annotation",,,,53.55,9.94,"","",22765,Hamburg (Altona-Nord),"Something" +240,Place 2,"Zusätzlich wurden zum Zweck der Feuerholzgewinnung zahlreiche der alten Alleebäume abgeholzt.","another annotation",,,,53.55,9.95, ,Norderreihe 2,22767,Hamburg (Altona-Altstadt),"","Some other thing" From d9cdd1740e22a09da2b2883fcb769918360370bd Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Sat, 29 Jul 2023 19:12:03 +0200 Subject: [PATCH 09/22] Adding controller logic, form template --- app/controllers/layers_controller.rb | 17 ++++++++++++- app/views/layers/import.html.haml | 38 ++++++++++++++++++++++++++++ config/routes.rb | 2 ++ lib/imports/csv_importer.rb | 3 +-- 4 files changed, 57 insertions(+), 3 deletions(-) create mode 100644 app/views/layers/import.html.haml diff --git a/app/controllers/layers_controller.rb b/app/controllers/layers_controller.rb index 31897181..206fc7f6 100644 --- a/app/controllers/layers_controller.rb +++ b/app/controllers/layers_controller.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class LayersController < ApplicationController - before_action :set_layer, only: %i[images show edit update destroy annotations relations pack build] + before_action :set_layer, only: %i[images import show edit update destroy annotations relations pack build] before_action :redirect_to_friendly_id, only: %i[show] @@ -20,6 +20,21 @@ def images @map = Map.sorted.by_user(current_user).friendly.find(params[:map_id]) end + def import; end + + def importing + file = params[:file] + + if file + importer = Imports::CsvImporter.new(file, @layer.id) + importer.import + + redirect_to map_layer_path(@map, @layer), notice: 'CSV import completed successfully!' + else + redirect_to map_layer_path(@map, @layer), notice: 'No CSV file provided to import' + end + end + def search @map = Map.sorted.by_user(current_user).friendly.find(params[:map_id]) @layers = @map.layers diff --git a/app/views/layers/import.html.haml b/app/views/layers/import.html.haml new file mode 100644 index 00000000..9bbcf4a8 --- /dev/null +++ b/app/views/layers/import.html.haml @@ -0,0 +1,38 @@ +- content_for(:title) { "Import for #{@layer.title}" } + +#table-wrapper + %p.text-right.close_link_wrapper + =link_to map_path(@map), :class=>'close_link' do + %i.fi.fi-x + %h1 + Import: + = link_to map_layer_path(@map,@layer) do + = @layer.title + .grid-x.grid-padding-x + .large-12.medium-12.small-12.cell + %hr + %p Here you can mass create places for this layer by importing a CSV file. + %p.hint You can use this importer to reimport an exported CSV (as a backup) or to set up a set of new places. + %p + The CSV file must contain the following named columns: + %code + title; lat; lon + %p.hint + The following columns are optional (and correspond to the datastructure of the Place model): + %code + subtitle teaser text link startdate startdate_date startdate_time enddate enddate_date enddate_time location address zip city country published featured sensitive sensitive_radius shy imagelink layer_id icon_id relations_tos relations_froms + + = simple_form_for :import, url: importing_map_layer_path(@map), method: "POST", html: { class: 'form-inline' } do |f| + %hr + %p + = f.input :file, :as => "file", :label => 'CSV File' + %span.hint The format of the file must be CSV. + %hr + %p + = f.input :overwrite, :as => :boolean, :input_html => { :checked => "" }, :label => 'Overwrite' + %span.hint + Overwrite existing places with the same title + %hr + .form-actions + %p + = f.submit "Import CSV File", class: "button" diff --git a/config/routes.rb b/config/routes.rb index be4ed3ee..7efcdd6f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -43,6 +43,8 @@ get :annotations get :relations get :images + get :import + post :importing end resources :places do resources :images diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb index 6292e58e..89fbee3a 100644 --- a/lib/imports/csv_importer.rb +++ b/lib/imports/csv_importer.rb @@ -84,7 +84,6 @@ def valid_row?(row) def create_place(row) place_attrs = {} ALLOWED_FIELDS.each do |field| - puts "#{field} #{row[field]}" if TEXT_FIELDS.include?(field) place_attrs[field.to_sym] = strip_tags(row[field]).strip if row[field] elsif row[field] @@ -110,7 +109,7 @@ def handle_invalid_rows end def do_sanitize(value) - # call rails sanitizer + # call rails sanitizer + strip string sanitize(value).strip end end From d34daa486e7f69a1c0587bab386e3b207c5c920c Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Sat, 29 Jul 2023 20:00:34 +0200 Subject: [PATCH 10/22] Rspec for controller and routing --- app/controllers/layers_controller.rb | 6 +-- spec/controllers/layers_controller_spec.rb | 48 ++++++++++++++++++++-- spec/routing/layers_routing_spec.rb | 8 ++++ 3 files changed, 56 insertions(+), 6 deletions(-) diff --git a/app/controllers/layers_controller.rb b/app/controllers/layers_controller.rb index 206fc7f6..6d12c8ec 100644 --- a/app/controllers/layers_controller.rb +++ b/app/controllers/layers_controller.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class LayersController < ApplicationController - before_action :set_layer, only: %i[images import show edit update destroy annotations relations pack build] + before_action :set_layer, only: %i[images importing import show edit update destroy annotations relations pack build] before_action :redirect_to_friendly_id, only: %i[show] @@ -29,9 +29,9 @@ def importing importer = Imports::CsvImporter.new(file, @layer.id) importer.import - redirect_to map_layer_path(@map, @layer), notice: 'CSV import completed successfully!' + redirect_to import_map_layer_path(@map, @layer), notice: 'CSV import completed successfully!' else - redirect_to map_layer_path(@map, @layer), notice: 'No CSV file provided to import' + redirect_to import_map_layer_path(@map, @layer), notice: 'No CSV file provided to import' end end diff --git a/spec/controllers/layers_controller_spec.rb b/spec/controllers/layers_controller_spec.rb index de545075..b826b0f2 100644 --- a/spec/controllers/layers_controller_spec.rb +++ b/spec/controllers/layers_controller_spec.rb @@ -26,9 +26,6 @@ FactoryBot.attributes_for(:layer, :invalid, map_id: @map.id) end - # This should return the minimal set of values that should be in the session - # in order to pass any filters (e.g. authentication) defined in - # LayersController. Be sure to keep this updated too. let(:valid_session) { {} } describe 'GET #index' do @@ -47,6 +44,51 @@ end end + describe 'GET #import' do + it 'renders the import form' do + get :import, params: { map_id: @map.id, id: layer.friendly_id }, session: valid_session + expect(response).to render_template(:import) + end + end + + describe 'POST #importing' do + let(:file) { Rack::Test::UploadedFile.new('spec/support/files/places.csv', 'text/csv') } + let(:layer) { create(:layer) } + + context 'with valid CSV' do + it 'imports the CSV and redirects to map_layer_path' do + post :importing, params: { map_id: @map.id, id: layer.friendly_id, file: file }, session: valid_session + expect(response).to redirect_to(import_map_layer_path(@map, layer)) + expect(flash[:notice]).to eq('CSV import completed successfully!') + end + + it 'creates new place records from the CSV' do + expect do + post :importing, params: { map_id: @map.id, id: layer.friendly_id, file: file }, session: valid_session + end.to change(Place, :count).by(2) + + expect(Place.pluck(:title)).to contain_exactly('Place 1', 'Place 2') + end + end + + context 'with invalid CSV' do + let(:invalid_file) { Rack::Test::UploadedFile.new('spec/support/files/places_nodata.csv', 'text/csv') } + + xit 'does not import the CSV and shows an error message' do + post :importing, params: { map_id: @map.id, id: layer.friendly_id, file: invalid_file }, session: valid_session + + expect(response).to render_template(:import) + expect(flash[:alert]).to include('Invalid row') + end + + it 'does not create book records from the invalid CSV' do + expect do + post :importing, params: { map_id: @map.id, id: layer.friendly_id, file: invalid_file }, session: valid_session + end.not_to change(Place, :count) + end + end + end + describe 'GET #pack' do it 'returns a success response' do layer = Layer.create! valid_attributes diff --git a/spec/routing/layers_routing_spec.rb b/spec/routing/layers_routing_spec.rb index 692dae05..a0a9f1cc 100644 --- a/spec/routing/layers_routing_spec.rb +++ b/spec/routing/layers_routing_spec.rb @@ -8,6 +8,14 @@ expect(get: '/maps/1/layers').to route_to('layers#index', map_id: '1') end + it 'routes to #import' do + expect(get: '/maps/1/layers/1/import').to route_to('layers#import', id: '1', map_id: '1') + end + + it 'routes to #importing' do + expect(post: '/maps/1/layers/1/importing').to route_to('layers#importing', id: '1', map_id: '1') + end + it 'routes to #new' do expect(get: '/maps/1/layers/new').to route_to('layers#new', map_id: '1') end From 534ca124cb29c473cf24c9ff9ff36d691a19c452 Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Mon, 31 Jul 2023 13:17:16 +0200 Subject: [PATCH 11/22] Import preview --- app/controllers/layers_controller.rb | 14 +++++++- app/views/layers/import.html.haml | 2 +- app/views/layers/import_preview.html.haml | 43 +++++++++++++++++++++++ config/routes.rb | 1 + lib/imports/csv_importer.rb | 14 +++++--- 5 files changed, 68 insertions(+), 6 deletions(-) create mode 100644 app/views/layers/import_preview.html.haml diff --git a/app/controllers/layers_controller.rb b/app/controllers/layers_controller.rb index 6d12c8ec..4e73de69 100644 --- a/app/controllers/layers_controller.rb +++ b/app/controllers/layers_controller.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class LayersController < ApplicationController - before_action :set_layer, only: %i[images importing import show edit update destroy annotations relations pack build] + before_action :set_layer, only: %i[images import import_preview importing show edit update destroy annotations relations pack build] before_action :redirect_to_friendly_id, only: %i[show] @@ -22,6 +22,18 @@ def images def import; end + def import_preview + file = params[:file] + file = params[:import][:file] + + return unless file + + importer = Imports::CsvImporter.new(file, @layer.id) + importer.import + @valid_rows = importer.valid_rows + @invalid_rows = importer.invalid_rows + end + def importing file = params[:file] diff --git a/app/views/layers/import.html.haml b/app/views/layers/import.html.haml index 9bbcf4a8..43abde13 100644 --- a/app/views/layers/import.html.haml +++ b/app/views/layers/import.html.haml @@ -22,7 +22,7 @@ %code subtitle teaser text link startdate startdate_date startdate_time enddate enddate_date enddate_time location address zip city country published featured sensitive sensitive_radius shy imagelink layer_id icon_id relations_tos relations_froms - = simple_form_for :import, url: importing_map_layer_path(@map), method: "POST", html: { class: 'form-inline' } do |f| + = simple_form_for :import, url: import_preview_map_layer_path(@map), multipart: true, method: "POST", html: { class: 'form-inline' } do |f| %hr %p = f.input :file, :as => "file", :label => 'CSV File' diff --git a/app/views/layers/import_preview.html.haml b/app/views/layers/import_preview.html.haml new file mode 100644 index 00000000..25d8a4d2 --- /dev/null +++ b/app/views/layers/import_preview.html.haml @@ -0,0 +1,43 @@ +- content_for(:title) { "Import for #{@layer.title}" } + +#table-wrapper + %p.text-right.close_link_wrapper + =link_to map_path(@map), :class=>'close_link' do + %i.fi.fi-x + %h1 + Import preview: + = link_to map_layer_path(@map,@layer) do + = @layer.title + .grid-x.grid-padding-x + .large-12.medium-12.small-12.cell + %hr + - if flash[:alert] + .alert.alert-danger + %p= flash[:alert] + + - if @invalid_rows.any? + .alert.alert-danger + %p Some rows in the CSV are invalid and cannot be imported. Please review the errors below. + + - if @valid_rows.any? + %h2 Preview: + %table.table.table-striped + %thead + %tr + - Imports::CsvImporter::ALLOWED_FIELDS.each do |field| + %th= field.capitalize + %tbody + - @valid_rows.each do |row| + %tr + - Imports::CsvImporter::ALLOWED_FIELDS.each do |field| + %td= row[field] + + = form_tag importing_map_layer_path(@map,@layer), method: :post do + = hidden_field_tag :file, params[:file] + = hidden_field_tag :dry_run, false + = button_tag(type: 'submit', class: 'button') do + + Confirm Import + + - else + %p No valid rows found. diff --git a/config/routes.rb b/config/routes.rb index 7efcdd6f..da297d0c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -44,6 +44,7 @@ get :relations get :images get :import + post :import_preview post :importing end resources :places do diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb index 89fbee3a..7e76cbb5 100644 --- a/lib/imports/csv_importer.rb +++ b/lib/imports/csv_importer.rb @@ -5,7 +5,7 @@ include ActionView::Helpers::SanitizeHelper class Imports::CsvImporter - attr_reader :invalid_rows, :unprocessable_fields # values needed for testing + attr_reader :invalid_rows, :valid_rows, :unprocessable_fields # values needed for testing REQUIRED_FIELDS = %w[title lat lon].freeze @@ -15,10 +15,12 @@ class Imports::CsvImporter # TODO: param: update or skip existing place? - def initialize(file, layer_id) + def initialize(file, layer_id, dry_run: true) @file = file - @invalid_rows = [] @layer = Layer.find(layer_id) + @dry_run = dry_run + @invalid_rows = [] + @valid_rows = [] @existing_titles = [] @unprocessable_fields = [] end @@ -46,7 +48,11 @@ def import # @existing_titles << title else @existing_titles << title - create_place(processed_row) + if @dry_run + @valid_rows << processed_row + else + create_place(processed_row) + end end end From f28f175114bf761b3b457e5bfe88874ef4279983 Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Mon, 31 Jul 2023 20:48:24 +0200 Subject: [PATCH 12/22] get import file by param --- app/controllers/layers_controller.rb | 4 +--- app/views/layers/import_preview.html.haml | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/controllers/layers_controller.rb b/app/controllers/layers_controller.rb index 4e73de69..2060e045 100644 --- a/app/controllers/layers_controller.rb +++ b/app/controllers/layers_controller.rb @@ -23,9 +23,7 @@ def images def import; end def import_preview - file = params[:file] file = params[:import][:file] - return unless file importer = Imports::CsvImporter.new(file, @layer.id) @@ -35,8 +33,8 @@ def import_preview end def importing - file = params[:file] + file = params[:import][:file] if file importer = Imports::CsvImporter.new(file, @layer.id) importer.import diff --git a/app/views/layers/import_preview.html.haml b/app/views/layers/import_preview.html.haml index 25d8a4d2..b7da7363 100644 --- a/app/views/layers/import_preview.html.haml +++ b/app/views/layers/import_preview.html.haml @@ -32,8 +32,8 @@ - Imports::CsvImporter::ALLOWED_FIELDS.each do |field| %td= row[field] - = form_tag importing_map_layer_path(@map,@layer), method: :post do - = hidden_field_tag :file, params[:file] + = simple_form_for :import, url: importing_map_layer_path(@map), multipart: true, method: "POST", html: { class: 'form-inline' } do |f| + = f.input :file, :input_html => { :value => params[:import][:file] }, label: false = hidden_field_tag :dry_run, false = button_tag(type: 'submit', class: 'button') do From edaf4df99c2c85fe860f46f2b7f3140ef474fbdd Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Mon, 31 Jul 2023 22:38:01 +0200 Subject: [PATCH 13/22] Swith to session based datatransfer between steps --- app/controllers/layers_controller.rb | 18 +++++++++++------- app/views/layers/import_preview.html.haml | 1 - 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/app/controllers/layers_controller.rb b/app/controllers/layers_controller.rb index 2060e045..b46304d7 100644 --- a/app/controllers/layers_controller.rb +++ b/app/controllers/layers_controller.rb @@ -29,17 +29,21 @@ def import_preview importer = Imports::CsvImporter.new(file, @layer.id) importer.import @valid_rows = importer.valid_rows + session[:importing_rows] = @valid_rows @invalid_rows = importer.invalid_rows end def importing - - file = params[:import][:file] - if file - importer = Imports::CsvImporter.new(file, @layer.id) - importer.import - - redirect_to import_map_layer_path(@map, @layer), notice: 'CSV import completed successfully!' + importing_rows_data = session[:importing_rows] + # if file + # importer = Imports::CsvImporter.new(file, @layer.id) + # importer.import + + if importing_rows_data + @importing_rows = importing_rows_data.map { |place_data| Place.new(place_data) } + @importing_rows.each(&:save) + session.delete(:importing_rows) + redirect_to import_map_layer_path(@map, @layer), notice: "CSV import completed successfully! (#{@importing_rows.count} places has been imported)" else redirect_to import_map_layer_path(@map, @layer), notice: 'No CSV file provided to import' end diff --git a/app/views/layers/import_preview.html.haml b/app/views/layers/import_preview.html.haml index b7da7363..a7d20e76 100644 --- a/app/views/layers/import_preview.html.haml +++ b/app/views/layers/import_preview.html.haml @@ -33,7 +33,6 @@ %td= row[field] = simple_form_for :import, url: importing_map_layer_path(@map), multipart: true, method: "POST", html: { class: 'form-inline' } do |f| - = f.input :file, :input_html => { :value => params[:import][:file] }, label: false = hidden_field_tag :dry_run, false = button_tag(type: 'submit', class: 'button') do From f2eda215519ce16e74194667d2b986c1d023f35f Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Tue, 22 Aug 2023 21:12:52 +0200 Subject: [PATCH 14/22] Show data errors w/error types and AR messages --- app/assets/stylesheets/6_tables.css.scss | 8 ++++ app/assets/stylesheets/application.css.scss | 1 + app/controllers/layers_controller.rb | 14 +++--- app/views/layers/import_preview.html.haml | 49 +++++++++++++++++---- lib/imports/csv_importer.rb | 23 ++++++---- 5 files changed, 71 insertions(+), 24 deletions(-) create mode 100644 app/assets/stylesheets/6_tables.css.scss diff --git a/app/assets/stylesheets/6_tables.css.scss b/app/assets/stylesheets/6_tables.css.scss new file mode 100644 index 00000000..19dee5ac --- /dev/null +++ b/app/assets/stylesheets/6_tables.css.scss @@ -0,0 +1,8 @@ +.table-wrapper { + overflow: auto; + margin-bottom: 5px; +} + +span.nobr { + white-space: nowrap; +} \ No newline at end of file diff --git a/app/assets/stylesheets/application.css.scss b/app/assets/stylesheets/application.css.scss index f92efb4c..1908db67 100644 --- a/app/assets/stylesheets/application.css.scss +++ b/app/assets/stylesheets/application.css.scss @@ -24,6 +24,7 @@ *= require 3_forms *= require 4_map *= require 5_header + *= require 6_tables *= require public/submissions */ diff --git a/app/controllers/layers_controller.rb b/app/controllers/layers_controller.rb index b46304d7..811a4d63 100644 --- a/app/controllers/layers_controller.rb +++ b/app/controllers/layers_controller.rb @@ -31,21 +31,19 @@ def import_preview @valid_rows = importer.valid_rows session[:importing_rows] = @valid_rows @invalid_rows = importer.invalid_rows + @duplicate_rows = importer.duplicate_rows + @errored_rows = importer.errored_rows end def importing importing_rows_data = session[:importing_rows] - # if file - # importer = Imports::CsvImporter.new(file, @layer.id) - # importer.import - if importing_rows_data - @importing_rows = importing_rows_data.map { |place_data| Place.new(place_data) } - @importing_rows.each(&:save) + @importing_rows = importing_rows_data.map { |place_data| Place.new(place_data.merge(layer_id: @layer.id)) } + @importing_rows.each(&:save!) session.delete(:importing_rows) - redirect_to import_map_layer_path(@map, @layer), notice: "CSV import completed successfully! (#{@importing_rows.count} places has been imported)" + redirect_to map_layer_path(@map, @layer), notice: "CSV import completed successfully! (#{@importing_rows.count} places has been imported to #{@layer.title})" else - redirect_to import_map_layer_path(@map, @layer), notice: 'No CSV file provided to import' + redirect_to import_map_layer_path(@map, @layer), notice: 'No data provided to import' end end diff --git a/app/views/layers/import_preview.html.haml b/app/views/layers/import_preview.html.haml index a7d20e76..60ebea2a 100644 --- a/app/views/layers/import_preview.html.haml +++ b/app/views/layers/import_preview.html.haml @@ -8,6 +8,10 @@ Import preview: = link_to map_layer_path(@map,@layer) do = @layer.title + %p + Map: #{@layer.map.title} + \| + Places: #{@layer.places.count} .grid-x.grid-padding-x .large-12.medium-12.small-12.cell %hr @@ -18,25 +22,54 @@ - if @invalid_rows.any? .alert.alert-danger %p Some rows in the CSV are invalid and cannot be imported. Please review the errors below. + - if @duplicate_rows.any? + .alert.alert-danger + %p Some rows in the CSV are duplicates and cannot be imported: + %table.table.table-striped + %tbody + - @duplicate_rows.each do |row| + %tr + %td + = row['title'] - - if @valid_rows.any? - %h2 Preview: + %p.hint If you want to replace duplicates, go back, tick the "Overwrite?" option and try again. + + - if @errored_rows %table.table.table-striped - %thead - %tr - - Imports::CsvImporter::ALLOWED_FIELDS.each do |field| - %th= field.capitalize %tbody - - @valid_rows.each do |row| + - @errored_rows.each do |error_hash| + %tr + %td= error_hash[:data]['title'] + %td= error_hash[:type] + %td= error_hash[:messages].join(', ') + + - if @valid_rows.any? + %h2 Preview: + %p Entries: #{@valid_rows.count} + .table-wrapper + %table.table.table-striped + %thead %tr - Imports::CsvImporter::ALLOWED_FIELDS.each do |field| - %td= row[field] + %th= field.capitalize + %tbody + - @valid_rows.each do |row| + %tr + - Imports::CsvImporter::ALLOWED_FIELDS.each do |field| + %td + - if field == 'title' + %span.nobr + = row[field] + - elsif row[field] + = row[field].html_safe = simple_form_for :import, url: importing_map_layer_path(@map), multipart: true, method: "POST", html: { class: 'form-inline' } do |f| = hidden_field_tag :dry_run, false + = link_to 'Cancel', :back, class: 'button secondary large' = button_tag(type: 'submit', class: 'button') do Confirm Import + - else %p No valid rows found. diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb index 7e76cbb5..5e2b0e0a 100644 --- a/lib/imports/csv_importer.rb +++ b/lib/imports/csv_importer.rb @@ -5,7 +5,7 @@ include ActionView::Helpers::SanitizeHelper class Imports::CsvImporter - attr_reader :invalid_rows, :valid_rows, :unprocessable_fields # values needed for testing + attr_reader :invalid_rows, :valid_rows, :duplicate_rows, :errored_rows, :unprocessable_fields # values needed for testing REQUIRED_FIELDS = %w[title lat lon].freeze @@ -20,8 +20,11 @@ def initialize(file, layer_id, dry_run: true) @layer = Layer.find(layer_id) @dry_run = dry_run @invalid_rows = [] + @duplicate_rows = [] @valid_rows = [] - @existing_titles = [] + @errored_rows = [] + # @existing_titles = [] + @existing_titles = @layer.places.pluck(:title) @unprocessable_fields = [] end @@ -42,7 +45,9 @@ def import if @existing_titles.include?(title) # TODO! # skip existing rows with this title - @invalid_rows << processed_row + @duplicate_rows << processed_row + error_hash = { data: processed_row, type: 'Duplicate', messages: ['Title already exists'] } + @errored_rows << error_hash # else # update existing row # @existing_titles << title @@ -103,14 +108,16 @@ def create_place(row) end def handle_invalid_rows - error_messages = @invalid_rows.map do |row| + @invalid_rows.map do |row| place = Place.new(title: do_sanitize(row['title']), lat: do_sanitize(row['lat']), lon: do_sanitize(row['lon']), layer_id: @layer.id) - place.valid? - place.errors.full_messages + unless place.valid? + error_hash = { data: row, type: 'Invalid data', messages: place.errors.full_messages } + @errored_rows << error_hash + end end - error_messages.each_with_index do |messages, index| - Rails.logger.error("Invalid row #{index + 1}: #{@invalid_rows[index]} - Errors: #{messages.join(', ')}") + @errored_rows.each_with_index do |row, index| + Rails.logger.error("Invalid row #{index + 1}: #{@invalid_rows[index]} - Errors: #{row['messages']}") end end From 3e1cb8e1e4b9d85edaa3f698b2dfc99435cd59c7 Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Wed, 23 Aug 2023 11:58:53 +0200 Subject: [PATCH 15/22] Improve import preview output/UI. Adapt rspecs (tbc) --- app/assets/stylesheets/6_tables.css.scss | 13 +++- app/controllers/layers_controller.rb | 2 +- app/views/layers/import_preview.html.haml | 66 ++++++++++-------- lib/imports/csv_importer.rb | 33 ++------- spec/controllers/layers_controller_spec.rb | 80 +++++++++++++++++----- spec/lib/csv_importer_spec.rb | 48 ++++--------- spec/support/files/places_invalid_lat.csv | 4 +- spec/support/files/places_with_html.csv | 2 +- 8 files changed, 134 insertions(+), 114 deletions(-) diff --git a/app/assets/stylesheets/6_tables.css.scss b/app/assets/stylesheets/6_tables.css.scss index 19dee5ac..f78167c1 100644 --- a/app/assets/stylesheets/6_tables.css.scss +++ b/app/assets/stylesheets/6_tables.css.scss @@ -3,6 +3,17 @@ margin-bottom: 5px; } -span.nobr { +th.nobr, td.nobr, span.nobr { white-space: nowrap; +} + +div.alert { + background-color: rgba(200,0,0,0.2); + padding: 15px; + margin-bottom: 10px; +} +div.feedback { + background-color: rgba(0,200,150,0.2); + padding: 15px; + margin-bottom: 10px; } \ No newline at end of file diff --git a/app/controllers/layers_controller.rb b/app/controllers/layers_controller.rb index 811a4d63..84869d2a 100644 --- a/app/controllers/layers_controller.rb +++ b/app/controllers/layers_controller.rb @@ -43,7 +43,7 @@ def importing session.delete(:importing_rows) redirect_to map_layer_path(@map, @layer), notice: "CSV import completed successfully! (#{@importing_rows.count} places has been imported to #{@layer.title})" else - redirect_to import_map_layer_path(@map, @layer), notice: 'No data provided to import' + redirect_to import_map_layer_path(@map, @layer), notice: 'No data provided to import!' end end diff --git a/app/views/layers/import_preview.html.haml b/app/views/layers/import_preview.html.haml index 60ebea2a..c0967343 100644 --- a/app/views/layers/import_preview.html.haml +++ b/app/views/layers/import_preview.html.haml @@ -19,11 +19,29 @@ .alert.alert-danger %p= flash[:alert] - - if @invalid_rows.any? - .alert.alert-danger + + + - if @errored_rows + .alert + %h3 Invalid entries %p Some rows in the CSV are invalid and cannot be imported. Please review the errors below. + + %table.table.table-striped + %thead + %tr + %th Title + %th Reason + %th Messages + %tbody + - @errored_rows.each do |error_hash| + %tr + %td= error_hash[:data]['title'] + %td.nobr= error_hash[:type] + %td + %tt= error_hash[:messages].join(', ') - if @duplicate_rows.any? - .alert.alert-danger + .alert + %h3 Duplicate entries %p Some rows in the CSV are duplicates and cannot be imported: %table.table.table-striped %tbody @@ -34,34 +52,26 @@ %p.hint If you want to replace duplicates, go back, tick the "Overwrite?" option and try again. - - if @errored_rows - %table.table.table-striped - %tbody - - @errored_rows.each do |error_hash| - %tr - %td= error_hash[:data]['title'] - %td= error_hash[:type] - %td= error_hash[:messages].join(', ') - - if @valid_rows.any? - %h2 Preview: - %p Entries: #{@valid_rows.count} - .table-wrapper - %table.table.table-striped - %thead - %tr - - Imports::CsvImporter::ALLOWED_FIELDS.each do |field| - %th= field.capitalize - %tbody - - @valid_rows.each do |row| + .feedback + %h3 Entries to be imported + %p These #{@valid_rows.count} rows are validated and will be imported: + .table-wrapper + %table.table.table-striped + %thead %tr - Imports::CsvImporter::ALLOWED_FIELDS.each do |field| - %td - - if field == 'title' - %span.nobr - = row[field] - - elsif row[field] - = row[field].html_safe + %th= field.capitalize + %tbody + - @valid_rows.each do |row| + %tr + - Imports::CsvImporter::ALLOWED_FIELDS.each do |field| + %td + - if field == 'title' + %span.nobr + = row[field] + - elsif row[field] + = row[field].html_safe = simple_form_for :import, url: importing_map_layer_path(@map), multipart: true, method: "POST", html: { class: 'form-inline' } do |f| = hidden_field_tag :dry_run, false diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb index 5e2b0e0a..011b965d 100644 --- a/lib/imports/csv_importer.rb +++ b/lib/imports/csv_importer.rb @@ -5,7 +5,7 @@ include ActionView::Helpers::SanitizeHelper class Imports::CsvImporter - attr_reader :invalid_rows, :valid_rows, :duplicate_rows, :errored_rows, :unprocessable_fields # values needed for testing + attr_reader :invalid_rows, :valid_rows, :duplicate_rows, :errored_rows, :unprocessable_fields REQUIRED_FIELDS = %w[title lat lon].freeze @@ -15,22 +15,19 @@ class Imports::CsvImporter # TODO: param: update or skip existing place? - def initialize(file, layer_id, dry_run: true) + def initialize(file, layer_id) @file = file @layer = Layer.find(layer_id) - @dry_run = dry_run @invalid_rows = [] @duplicate_rows = [] @valid_rows = [] @errored_rows = [] - # @existing_titles = [] @existing_titles = @layer.places.pluck(:title) @unprocessable_fields = [] end def import validate_header - CSV.foreach(@file.path, headers: true) do |row| processed_row = row.to_hash.slice(*ALLOWED_FIELDS) @@ -53,15 +50,12 @@ def import # @existing_titles << title else @existing_titles << title - if @dry_run - @valid_rows << processed_row - else - create_place(processed_row) - end + @valid_rows << processed_row end end - - if @invalid_rows.empty? + if @valid_rows.empty? + Rails.logger.info('CSV import returns no valid rows!') + elsif @invalid_rows.empty? Rails.logger.info('CSV import completed successfully!') else handle_invalid_rows @@ -92,21 +86,6 @@ def valid_row?(row) place.valid? end - def create_place(row) - place_attrs = {} - ALLOWED_FIELDS.each do |field| - if TEXT_FIELDS.include?(field) - place_attrs[field.to_sym] = strip_tags(row[field]).strip if row[field] - elsif row[field] - place_attrs[field.to_sym] = do_sanitize(row[field]) - end - end - place_attrs[:layer_id] = @layer.id unless place_attrs[:layer_id] - - place = Place.new(place_attrs) - place.save - end - def handle_invalid_rows @invalid_rows.map do |row| place = Place.new(title: do_sanitize(row['title']), lat: do_sanitize(row['lat']), lon: do_sanitize(row['lon']), layer_id: @layer.id) diff --git a/spec/controllers/layers_controller_spec.rb b/spec/controllers/layers_controller_spec.rb index b826b0f2..81ac9207 100644 --- a/spec/controllers/layers_controller_spec.rb +++ b/spec/controllers/layers_controller_spec.rb @@ -51,40 +51,82 @@ end end - describe 'POST #importing' do + describe 'GET #import_preview' do let(:file) { Rack::Test::UploadedFile.new('spec/support/files/places.csv', 'text/csv') } let(:layer) { create(:layer) } context 'with valid CSV' do - it 'imports the CSV and redirects to map_layer_path' do - post :importing, params: { map_id: @map.id, id: layer.friendly_id, file: file }, session: valid_session - expect(response).to redirect_to(import_map_layer_path(@map, layer)) - expect(flash[:notice]).to eq('CSV import completed successfully!') + it 'renders the import preview' do + post :import_preview, params: { map_id: @map.id, id: layer.friendly_id, import: { file: file } }, session: valid_session + expect(response).to render_template(:import_preview) + expect(session[:importing_rows]).not_to be_nil end + end - it 'creates new place records from the CSV' do - expect do - post :importing, params: { map_id: @map.id, id: layer.friendly_id, file: file }, session: valid_session - end.to change(Place, :count).by(2) + context 'with invalid CSV' do + let(:invalid_file) { Rack::Test::UploadedFile.new('spec/support/files/places_nodata.csv', 'text/csv') } + + it 'shows an error message' do + post :import_preview, params: { map_id: @map.id, id: layer.friendly_id, import: { file: invalid_file } }, session: valid_session - expect(Place.pluck(:title)).to contain_exactly('Place 1', 'Place 2') + expect(response).to render_template(:import_preview) + expect(session[:importing_rows]).to eq([]) end end + end - context 'with invalid CSV' do - let(:invalid_file) { Rack::Test::UploadedFile.new('spec/support/files/places_nodata.csv', 'text/csv') } + describe 'POST #importing' do + let(:file) { Rack::Test::UploadedFile.new('spec/support/files/places.csv', 'text/csv') } + let(:invalid_file) { Rack::Test::UploadedFile.new('spec/support/files/places_invalid_lat.csv', 'text/csv') } + let(:layer) { create(:layer) } - xit 'does not import the CSV and shows an error message' do - post :importing, params: { map_id: @map.id, id: layer.friendly_id, file: invalid_file }, session: valid_session + context 'without session data' do + it 'redirects to map_layer_path and flashes error message' do + post :importing, params: { map_id: @map.id, id: layer.friendly_id, file: file }, session: valid_session + expect(session[:importing_rows]).to be_nil + expect(response).to redirect_to(import_map_layer_path(@map, layer)) + expect(flash[:notice]).to eq('No data provided to import!') + end + end + + context 'with session data' do + context 'with valid CSV' do + before do + importing_rows = [build(:place, title: 'Place 1', layer: layer).attributes, build(:place, title: 'Place 2', layer: layer).attributes] + allow(controller).to receive(:session).and_return(importing_rows: importing_rows) + end - expect(response).to render_template(:import) - expect(flash[:alert]).to include('Invalid row') + it 'imports the CSV and redirects to map_layer_path' do + post :importing, params: { map_id: @map.id, id: layer.friendly_id, file: file }, session: valid_session + + # session gets cleared + expect(session[:importing_rows]).to eq(nil) + expect(response).to redirect_to(map_layer_path(@map, layer)) + expect(flash[:notice]).to match('CSV import completed successfully!') + end + + it 'creates new place records from the CSV' do + expect do + post :importing, params: { map_id: @map.id, id: layer.friendly_id, file: file }, session: valid_session + end.to change(Place, :count).by(2) + + expect(Place.pluck(:title)).to contain_exactly('Place 1', 'Place 2') + end end - it 'does not create book records from the invalid CSV' do - expect do + context 'with invalid CSV' do + it 'does not import the CSV and shows an error message' do post :importing, params: { map_id: @map.id, id: layer.friendly_id, file: invalid_file }, session: valid_session - end.not_to change(Place, :count) + + expect(response).to redirect_to(import_map_layer_path(@map, layer)) + expect(flash[:notice]).to include('No data provided') + end + + it 'does not create book records from the invalid CSV' do + expect do + post :importing, params: { map_id: @map.id, id: layer.friendly_id, file: invalid_file }, session: valid_session + end.not_to change(Place, :count) + end end end end diff --git a/spec/lib/csv_importer_spec.rb b/spec/lib/csv_importer_spec.rb index ac1a8ed7..45dd66a8 100644 --- a/spec/lib/csv_importer_spec.rb +++ b/spec/lib/csv_importer_spec.rb @@ -9,28 +9,26 @@ let(:layer) { create(:layer) } context 'with valid CSV' do - it 'creates new Place records from valid rows' do + it 'returns valid rows' do importer = Imports::CsvImporter.new(file, layer.id) - - expect do - importer.import - end.to change(Place, :count).by(2) - - expect(Place.pluck(:title)).to contain_exactly('Place 1', 'Place 2') + expect(importer.valid_rows).to contain_exactly('id', 'annotations', 'unknown') end - it 'creates new Place records from valid rows except non-allowed columns' do + it 'returns valid rows except non-allowed columns' do file = Rack::Test::UploadedFile.new('spec/support/files/places_valid_but_with_not_allowed_data.csv', 'text/csv') importer = Imports::CsvImporter.new(file, layer.id) - - expect do - importer.import - end.to change(Place, :count).by(2) - expect(importer.unprocessable_fields).to contain_exactly('id', 'annotations', 'unknown') # TODO: test outcome end + + it 'sanitizes a title with js and html', focus: true do + file_with_html = Rack::Test::UploadedFile.new('spec/support/files/places_with_html.csv', 'text/csv') + + importer = Imports::CsvImporter.new(file_with_html, layer.id) + expect(importer.valid_rows.count).to eq(1) + expect(importer.valid_rows.first.teaser).to eq('Ein etwas versteckt gelegenes Fleckchen') + end end context 'with invalid CSV' do @@ -38,11 +36,6 @@ invalid_file = Rack::Test::UploadedFile.new('spec/support/files/places_nodata.csv', 'text/csv') importer = Imports::CsvImporter.new(invalid_file, layer.id) - - expect do - importer.import - end.not_to change(Place, :count) - expect(importer.invalid_rows.count).to eq(1) end @@ -60,24 +53,7 @@ invalid_file = Rack::Test::UploadedFile.new('spec/support/files/places_invalid_lat.csv', 'text/csv') importer = Imports::CsvImporter.new(invalid_file, layer.id) - - expect do - importer.import - end.not_to change(Place, :count) - - expect(importer.invalid_rows.count).to eq(1) - end - - it 'sanitizes a title with js and html' do - file_with_html = Rack::Test::UploadedFile.new('spec/support/files/places_with_html.csv', 'text/csv') - - importer = Imports::CsvImporter.new(file_with_html, layer.id) - - expect do - importer.import - end.to change(Place, :count).by(1) - - expect(Place.first.teaser).to eq('Ein etwas versteckt gelegenes Fleckchen') + expect(importer.invalid_rows.count).to eq(2) end end end diff --git a/spec/support/files/places_invalid_lat.csv b/spec/support/files/places_invalid_lat.csv index 8c0bd5f3..d8ca3e77 100644 --- a/spec/support/files/places_invalid_lat.csv +++ b/spec/support/files/places_invalid_lat.csv @@ -1,2 +1,4 @@ id,title,teaser,text,annotations,startdate,enddate,lat,lon,location,address,zip,city,country -"id","title","teaser","text","annotations","startdate","enddate","1,2","","" +"1","title1","teaser1","text1","annotations1","2025-01-01","2025-01-02","1.2","9","Main Street" +"2","title2","teaser2","text2","annotations2","2025-01-02","2025-01-03","1,2","","" +"3","","teaser2","text2","annotations2","2025-01-02","2025-01-03","1.2","9","" diff --git a/spec/support/files/places_with_html.csv b/spec/support/files/places_with_html.csv index dd1059dc..ea0485b0 100644 --- a/spec/support/files/places_with_html.csv +++ b/spec/support/files/places_with_html.csv @@ -1,2 +1,2 @@ id,title,teaser,text,annotations,startdate,enddate,lat,lon,location,address,zip,city,country -239,"Place 1","Ein etwas versteckt gelegenes Fleckchen ","",,,,53.55,9.94,"","",22765,Hamburg (Altona-Nord) +239,"Place 1 HTML","Ein etwas versteckt gelegenes Fleckchen ","",,,,53.55,9.94,"","",22765,Hamburg (Altona-Nord) From 181f208e171a743017e1218c32721079ef17a07e Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Wed, 23 Aug 2023 17:22:42 +0200 Subject: [PATCH 16/22] Minor improvements, better comments,... --- app/views/layers/import_preview.html.haml | 19 ++---- db/schema.rb | 74 +++++++++++------------ lib/imports/csv_importer.rb | 7 ++- spec/lib/csv_importer_spec.rb | 11 ++-- spec/support/files/places_invalid_lat.csv | 6 +- 5 files changed, 52 insertions(+), 65 deletions(-) diff --git a/app/views/layers/import_preview.html.haml b/app/views/layers/import_preview.html.haml index c0967343..422357c1 100644 --- a/app/views/layers/import_preview.html.haml +++ b/app/views/layers/import_preview.html.haml @@ -21,7 +21,7 @@ - - if @errored_rows + - if !@errored_rows.empty? .alert %h3 Invalid entries %p Some rows in the CSV are invalid and cannot be imported. Please review the errors below. @@ -39,18 +39,9 @@ %td.nobr= error_hash[:type] %td %tt= error_hash[:messages].join(', ') - - if @duplicate_rows.any? - .alert - %h3 Duplicate entries - %p Some rows in the CSV are duplicates and cannot be imported: - %table.table.table-striped - %tbody - - @duplicate_rows.each do |row| - %tr - %td - = row['title'] - - %p.hint If you want to replace duplicates, go back, tick the "Overwrite?" option and try again. + - if @duplicate_rows.any? + %p.small If you want to replace duplicates, go back, tick the "Overwrite?" option and try again. + = link_to 'Back', :back, class: 'button secondary' - if @valid_rows.any? .feedback @@ -75,7 +66,7 @@ = simple_form_for :import, url: importing_map_layer_path(@map), multipart: true, method: "POST", html: { class: 'form-inline' } do |f| = hidden_field_tag :dry_run, false - = link_to 'Cancel', :back, class: 'button secondary large' + = link_to 'Back', :back, class: 'button secondary' = button_tag(type: 'submit', class: 'button') do Confirm Import diff --git a/db/schema.rb b/db/schema.rb index 25979c04..67498c78 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -12,17 +12,17 @@ ActiveRecord::Schema.define(version: 2023_02_27_174811) do - create_table "active_storage_attachments", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "active_storage_attachments", charset: "utf8mb3", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false - t.bigint "record_id", null: false - t.bigint "blob_id", null: false + t.integer "record_id", null: false + t.integer "blob_id", null: false t.datetime "created_at", null: false t.index ["blob_id"], name: "index_active_storage_attachments_on_blob_id" t.index ["record_type", "record_id", "name", "blob_id"], name: "index_active_storage_attachments_uniqueness", unique: true end - create_table "active_storage_blobs", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "active_storage_blobs", charset: "utf8mb3", force: :cascade do |t| t.string "key", null: false t.string "filename", null: false t.string "content_type" @@ -34,13 +34,13 @@ t.index ["key"], name: "index_active_storage_blobs_on_key", unique: true end - create_table "active_storage_variant_records", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "active_storage_variant_records", charset: "utf8mb3", force: :cascade do |t| t.bigint "blob_id", null: false t.string "variation_digest", null: false t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true end - create_table "annotations", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "annotations", charset: "utf8mb3", force: :cascade do |t| t.string "title" t.text "text" t.bigint "place_id" @@ -54,7 +54,7 @@ t.index ["place_id"], name: "fk_rails_51dbcfe977" end - create_table "build_logs", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "build_logs", charset: "utf8mb3", force: :cascade do |t| t.bigint "map_id" t.bigint "layer_id" t.string "output" @@ -66,7 +66,7 @@ t.index ["map_id"], name: "index_build_logs_on_map_id" end - create_table "friendly_id_slugs", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "friendly_id_slugs", charset: "utf8mb3", force: :cascade do |t| t.string "slug", null: false t.integer "sluggable_id", null: false t.string "sluggable_type", limit: 50 @@ -77,7 +77,7 @@ t.index ["sluggable_type", "sluggable_id"], name: "index_friendly_id_slugs_on_sluggable_type_and_sluggable_id" end - create_table "groups", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "groups", charset: "utf8mb3", force: :cascade do |t| t.string "title" t.datetime "created_at", null: false t.datetime "updated_at", null: false @@ -85,15 +85,15 @@ t.text "message" end - create_table "icons", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "icons", charset: "utf8mb3", force: :cascade do |t| t.string "title" - t.bigint "iconset_id" + t.integer "iconset_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false t.index ["iconset_id"], name: "index_icons_on_iconset_id" end - create_table "iconsets", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "iconsets", charset: "utf8mb3", force: :cascade do |t| t.string "title" t.text "text" t.datetime "created_at", null: false @@ -104,12 +104,12 @@ t.string "class_name" end - create_table "images", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "images", charset: "utf8mb3", force: :cascade do |t| t.string "title" t.string "licence" t.text "source" t.string "creator" - t.bigint "place_id" + t.integer "place_id" t.string "alt" t.string "caption" t.integer "sorting" @@ -120,11 +120,11 @@ t.index ["place_id"], name: "index_images_on_place_id" end - create_table "layers", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "layers", charset: "utf8mb3", force: :cascade do |t| t.string "title" t.string "subtitle" t.boolean "published", default: false - t.bigint "map_id" + t.integer "map_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false t.string "color" @@ -157,11 +157,11 @@ t.index ["slug"], name: "index_layers_on_slug", unique: true end - create_table "maps", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "maps", charset: "utf8mb3", force: :cascade do |t| t.string "title" t.string "subtitle" t.boolean "published", default: false - t.bigint "group_id" + t.integer "group_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false t.text "script" @@ -192,7 +192,7 @@ t.index ["slug"], name: "index_maps_on_slug", unique: true end - create_table "mobility_string_translations", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "mobility_string_translations", charset: "utf8mb3", force: :cascade do |t| t.string "locale", null: false t.string "key", null: false t.string "value" @@ -205,7 +205,7 @@ t.index ["translatable_type", "key", "value", "locale"], name: "index_mobility_string_translations_on_query_keys" end - create_table "mobility_text_translations", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "mobility_text_translations", charset: "utf8mb3", force: :cascade do |t| t.string "locale", null: false t.string "key", null: false t.text "value" @@ -217,7 +217,7 @@ t.index ["translatable_id", "translatable_type", "locale", "key"], name: "index_mobility_text_translations_on_keys", unique: true end - create_table "people", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "people", charset: "utf8mb3", force: :cascade do |t| t.string "name" t.text "info" t.datetime "created_at", null: false @@ -226,7 +226,7 @@ t.index ["map_id"], name: "index_people_on_map_id" end - create_table "places", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "places", charset: "utf8mb3", force: :cascade do |t| t.string "title" t.text "teaser" t.text "text" @@ -241,7 +241,7 @@ t.string "city" t.string "country" t.boolean "published", default: false - t.bigint "layer_id" + t.integer "layer_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false t.string "imagelink" @@ -255,7 +255,7 @@ t.index ["layer_id"], name: "index_places_on_layer_id" end - create_table "relations", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "relations", charset: "utf8mb3", force: :cascade do |t| t.integer "relation_from_id" t.integer "relation_to_id" t.string "rtype" @@ -263,7 +263,7 @@ t.datetime "updated_at", null: false end - create_table "submission_configs", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "submission_configs", charset: "utf8mb3", force: :cascade do |t| t.string "title_intro" t.string "subtitle_intro" t.text "intro" @@ -279,7 +279,7 @@ t.index ["layer_id"], name: "index_submission_configs_on_layer_id" end - create_table "submissions", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "submissions", charset: "utf8mb3", force: :cascade do |t| t.string "name" t.string "email" t.boolean "rights" @@ -292,7 +292,7 @@ t.index ["place_id"], name: "index_submissions_on_place_id" end - create_table "taggings", id: :integer, charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "taggings", id: :integer, charset: "utf8mb3", force: :cascade do |t| t.integer "tag_id" t.string "taggable_type" t.integer "taggable_id" @@ -311,15 +311,15 @@ t.index ["tagger_id"], name: "index_taggings_on_tagger_id" end - create_table "tags", id: :integer, charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| - t.string "name", collation: "utf8_bin" + create_table "tags", id: :integer, charset: "utf8mb3", force: :cascade do |t| + t.string "name", collation: "utf8mb3_bin" t.datetime "created_at" t.datetime "updated_at" t.integer "taggings_count", default: 0 t.index ["name"], name: "index_tags_on_name", unique: true end - create_table "users", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "users", charset: "utf8mb3", force: :cascade do |t| t.string "email", default: "", null: false t.string "encrypted_password", default: "", null: false t.string "reset_password_token" @@ -331,15 +331,15 @@ t.string "current_sign_in_ip" t.string "last_sign_in_ip" t.string "role", default: "user" - t.bigint "group_id" - t.datetime "created_at", default: "2021-09-03 18:29:24", null: false - t.datetime "updated_at", default: "2021-09-03 18:29:24", null: false + t.integer "group_id" + t.datetime "created_at", default: "2021-06-28 17:11:21", null: false + t.datetime "updated_at", default: "2021-06-28 17:11:21", null: false t.index ["email"], name: "index_users_on_email", unique: true t.index ["group_id"], name: "index_users_on_group_id" t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true end - create_table "videos", charset: "utf8", collation: "utf8_general_ci", force: :cascade do |t| + create_table "videos", charset: "utf8mb3", force: :cascade do |t| t.string "title" t.string "licence" t.text "source" @@ -359,15 +359,9 @@ add_foreign_key "annotations", "places" add_foreign_key "build_logs", "layers" add_foreign_key "build_logs", "maps" - add_foreign_key "icons", "iconsets" - add_foreign_key "images", "places" - add_foreign_key "layers", "maps" - add_foreign_key "maps", "groups" add_foreign_key "people", "maps" - add_foreign_key "places", "layers" add_foreign_key "submission_configs", "layers" add_foreign_key "submissions", "places" add_foreign_key "taggings", "tags" - add_foreign_key "users", "groups" add_foreign_key "videos", "places" end diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb index 011b965d..c1405566 100644 --- a/lib/imports/csv_importer.rb +++ b/lib/imports/csv_importer.rb @@ -13,7 +13,7 @@ class Imports::CsvImporter TEXT_FIELDS = %w[title subtitle teaser text location address zip city country].freeze - # TODO: param: update or skip existing place? + # TODO: update existing place as an option def initialize(file, layer_id) @file = file @@ -22,7 +22,7 @@ def initialize(file, layer_id) @duplicate_rows = [] @valid_rows = [] @errored_rows = [] - @existing_titles = @layer.places.pluck(:title) + @existing_titles = @layer.places ? @layer.places.pluck(:title) : [] @unprocessable_fields = [] end @@ -30,6 +30,7 @@ def import validate_header CSV.foreach(@file.path, headers: true) do |row| processed_row = row.to_hash.slice(*ALLOWED_FIELDS) + Rails.logger.info("Process row") unless !processed_row.empty? && valid_row?(processed_row) @invalid_rows << processed_row @@ -40,11 +41,11 @@ def import title = do_sanitize(processed_row['title']) if @existing_titles.include?(title) - # TODO! # skip existing rows with this title @duplicate_rows << processed_row error_hash = { data: processed_row, type: 'Duplicate', messages: ['Title already exists'] } @errored_rows << error_hash + # TODO: # else # update existing row # @existing_titles << title diff --git a/spec/lib/csv_importer_spec.rb b/spec/lib/csv_importer_spec.rb index 45dd66a8..b4024612 100644 --- a/spec/lib/csv_importer_spec.rb +++ b/spec/lib/csv_importer_spec.rb @@ -9,23 +9,24 @@ let(:layer) { create(:layer) } context 'with valid CSV' do - it 'returns valid rows' do + it 'returns valid rows', focus:true do importer = Imports::CsvImporter.new(file, layer.id) - expect(importer.valid_rows).to contain_exactly('id', 'annotations', 'unknown') + expect(importer.valid_rows.count).to eq(1) end + it 'returns valid rows except non-allowed columns' do file = Rack::Test::UploadedFile.new('spec/support/files/places_valid_but_with_not_allowed_data.csv', 'text/csv') importer = Imports::CsvImporter.new(file, layer.id) expect(importer.unprocessable_fields).to contain_exactly('id', 'annotations', 'unknown') - - # TODO: test outcome + expect(importer.valid_rows).to contain_exactly('id', 'annotations') end - it 'sanitizes a title with js and html', focus: true do + it 'sanitizes a title with js and html' do file_with_html = Rack::Test::UploadedFile.new('spec/support/files/places_with_html.csv', 'text/csv') importer = Imports::CsvImporter.new(file_with_html, layer.id) + puts importer.valid_rows.inspect expect(importer.valid_rows.count).to eq(1) expect(importer.valid_rows.first.teaser).to eq('Ein etwas versteckt gelegenes Fleckchen') end diff --git a/spec/support/files/places_invalid_lat.csv b/spec/support/files/places_invalid_lat.csv index d8ca3e77..cdc3c1dd 100644 --- a/spec/support/files/places_invalid_lat.csv +++ b/spec/support/files/places_invalid_lat.csv @@ -1,4 +1,4 @@ id,title,teaser,text,annotations,startdate,enddate,lat,lon,location,address,zip,city,country -"1","title1","teaser1","text1","annotations1","2025-01-01","2025-01-02","1.2","9","Main Street" -"2","title2","teaser2","text2","annotations2","2025-01-02","2025-01-03","1,2","","" -"3","","teaser2","text2","annotations2","2025-01-02","2025-01-03","1.2","9","" +"1","title1","row with valid data","text1","annotations1","2025-01-01","2025-01-02","1.2","9","Main Street" +"2","title2","row with wrong lat","text2","annotations2","2025-01-02","2025-01-03","1,2","","" +"3","","row without title","text2","annotations2","2025-01-02","2025-01-03","1.2","9","" From 456569e688cf5c562f73af82f2107016d25ad22a Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Wed, 23 Aug 2023 20:57:11 +0200 Subject: [PATCH 17/22] Fix/complete RSpecs --- lib/imports/csv_importer.rb | 11 ++++++----- spec/lib/csv_importer_spec.rb | 29 ++++++++++++++++++++++------- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb index c1405566..fe0e5f73 100644 --- a/lib/imports/csv_importer.rb +++ b/lib/imports/csv_importer.rb @@ -30,7 +30,7 @@ def import validate_header CSV.foreach(@file.path, headers: true) do |row| processed_row = row.to_hash.slice(*ALLOWED_FIELDS) - Rails.logger.info("Process row") + Rails.logger.info('Process row') unless !processed_row.empty? && valid_row?(processed_row) @invalid_rows << processed_row @@ -45,12 +45,13 @@ def import @duplicate_rows << processed_row error_hash = { data: processed_row, type: 'Duplicate', messages: ['Title already exists'] } @errored_rows << error_hash - # TODO: - # else + # TODO: else # update existing row # @existing_titles << title else @existing_titles << title + processed_row['title'] = title + processed_row['teaser'] = do_sanitize(processed_row['teaser']) if processed_row['teaser'].present? @valid_rows << processed_row end end @@ -83,13 +84,13 @@ def validate_header end def valid_row?(row) - place = Place.new(title: do_sanitize(row['title']), lat: do_sanitize(row['lat']), lon: do_sanitize(row['lon']), layer_id: @layer.id) + place = Place.new(title: row['title'], lat: row['lat'], lon: row['lon'], layer_id: @layer.id) place.valid? end def handle_invalid_rows @invalid_rows.map do |row| - place = Place.new(title: do_sanitize(row['title']), lat: do_sanitize(row['lat']), lon: do_sanitize(row['lon']), layer_id: @layer.id) + place = Place.new(title: row['title'], lat: row['lat'], lon: row['lon'], layer_id: @layer.id) unless place.valid? error_hash = { data: row, type: 'Invalid data', messages: place.errors.full_messages } @errored_rows << error_hash diff --git a/spec/lib/csv_importer_spec.rb b/spec/lib/csv_importer_spec.rb index b4024612..c8eeb4ba 100644 --- a/spec/lib/csv_importer_spec.rb +++ b/spec/lib/csv_importer_spec.rb @@ -9,26 +9,40 @@ let(:layer) { create(:layer) } context 'with valid CSV' do - it 'returns valid rows', focus:true do + it 'returns valid rows' do importer = Imports::CsvImporter.new(file, layer.id) - expect(importer.valid_rows.count).to eq(1) + importer.import + expect(importer.valid_rows.count).to eq(2) end - + it 'returns valid rows except non-allowed columns' do file = Rack::Test::UploadedFile.new('spec/support/files/places_valid_but_with_not_allowed_data.csv', 'text/csv') importer = Imports::CsvImporter.new(file, layer.id) + importer.import expect(importer.unprocessable_fields).to contain_exactly('id', 'annotations', 'unknown') - expect(importer.valid_rows).to contain_exactly('id', 'annotations') + expect(importer.valid_rows.count).to eq(2) + expect(importer.valid_rows.first['teaser']).to match('Ein etwas versteckt gelegenes und ') + end + + it 'returns valid rows except duplicate and invalid rows' do + place = create(:place, title: 'title1', layer: layer) + file = Rack::Test::UploadedFile.new('spec/support/files/places_invalid_lat.csv', 'text/csv') + + importer = Imports::CsvImporter.new(file, layer.id) + importer.import + expect(importer.valid_rows.count).to eq(0) + expect(importer.duplicate_rows.count).to eq(1) + expect(importer.invalid_rows.count).to eq(2) end it 'sanitizes a title with js and html' do file_with_html = Rack::Test::UploadedFile.new('spec/support/files/places_with_html.csv', 'text/csv') importer = Imports::CsvImporter.new(file_with_html, layer.id) - puts importer.valid_rows.inspect + importer.import expect(importer.valid_rows.count).to eq(1) - expect(importer.valid_rows.first.teaser).to eq('Ein etwas versteckt gelegenes Fleckchen') + expect(importer.valid_rows.first['teaser']).to match('Ein etwas versteckt gelegenes') end end @@ -37,6 +51,7 @@ invalid_file = Rack::Test::UploadedFile.new('spec/support/files/places_nodata.csv', 'text/csv') importer = Imports::CsvImporter.new(invalid_file, layer.id) + importer.import expect(importer.invalid_rows.count).to eq(1) end @@ -44,7 +59,6 @@ invalid_file = Rack::Test::UploadedFile.new('spec/support/files/places_invalid_header.csv', 'text/csv') importer = Imports::CsvImporter.new(invalid_file, layer.id) - expect do importer.import end.to raise_error(StandardError) @@ -54,6 +68,7 @@ invalid_file = Rack::Test::UploadedFile.new('spec/support/files/places_invalid_lat.csv', 'text/csv') importer = Imports::CsvImporter.new(invalid_file, layer.id) + importer.import expect(importer.invalid_rows.count).to eq(2) end end From 2b420607fee765878a6bb96913fed12a41cffe6a Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Fri, 25 Aug 2023 16:09:20 +0200 Subject: [PATCH 18/22] Link import, improve descriptions --- app/views/layers/import.html.haml | 8 ++++++-- app/views/layers/import_preview.html.haml | 3 ++- app/views/places/index.html.haml | 19 ++++++++++++------- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/app/views/layers/import.html.haml b/app/views/layers/import.html.haml index 43abde13..d3871bb7 100644 --- a/app/views/layers/import.html.haml +++ b/app/views/layers/import.html.haml @@ -11,8 +11,12 @@ .grid-x.grid-padding-x .large-12.medium-12.small-12.cell %hr - %p Here you can mass create places for this layer by importing a CSV file. - %p.hint You can use this importer to reimport an exported CSV (as a backup) or to set up a set of new places. + %p You can create many places for this layer by importing a CSV file. + %p.hint + You can use this importer also to re-import an earlier exported + = link_to map_layer_path(@map,@layer,:format => :csv), :title => "Export this layer as CSV" do + Layer-CSV (as a backup) + or to quickly fill a layer with places defined elsewhere. %p The CSV file must contain the following named columns: %code diff --git a/app/views/layers/import_preview.html.haml b/app/views/layers/import_preview.html.haml index 422357c1..1a1aceac 100644 --- a/app/views/layers/import_preview.html.haml +++ b/app/views/layers/import_preview.html.haml @@ -39,8 +39,9 @@ %td.nobr= error_hash[:type] %td %tt= error_hash[:messages].join(', ') - - if @duplicate_rows.any? + - if @duplicate_rows.count > 0 %p.small If you want to replace duplicates, go back, tick the "Overwrite?" option and try again. + %p.small Otherwise edit your CSV file and remove those rows. = link_to 'Back', :back, class: 'button secondary' - if @valid_rows.any? diff --git a/app/views/places/index.html.haml b/app/views/places/index.html.haml index 6f6360d9..70da949d 100644 --- a/app/views/places/index.html.haml +++ b/app/views/places/index.html.haml @@ -15,7 +15,7 @@ %span.shy (with #{@places.count} Places) .grid-x.grid-padding-x - .medium-12.cell + .medium-12.cell.card %p.text-left.hint-simple - if @layer.published %i.fi-eye.fi-18 @@ -24,7 +24,7 @@ %i.fi-lock.fi-18 The content of this layer is not public .grid-x.grid-padding-x - .medium-7.cell + .medium-7.cell.card %p.text-left.hint-simple Privacy settings for this layer: %ul.hint @@ -46,19 +46,24 @@ You can change these settings = link_to edit_map_layer_path(@map,@layer) do in the edit mode + %br + %br .medium-5.cell - %p.text-right.hint-simple - See also: %ul.hint %li = link_to annotations_map_layer_path(@map,@layer) do - Annotations (about Places) + All Annotations (about Places) %li = link_to relations_map_layer_path(@map,@layer) do - Relations (between places) + All relations (between places) %li = link_to images_map_layer_path(@map,@layer) do - All Images of this layer + List of all images + %hr + %ul.hint + %li + = link_to import_map_layer_path(@map,@layer) do + Import CSV to create places from %p   %table From 7b97f62146c4ba364092e4cb619cd57506dc42ef Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Sat, 26 Aug 2023 15:51:45 +0200 Subject: [PATCH 19/22] Rubocop settings --- .rubocop.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.rubocop.yml b/.rubocop.yml index d97cc9d9..399d8f09 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,5 +1,11 @@ #inherit_from: ../.rubocop.yml +AllCops: + NewCops: enable + +require: + # - rubocop-rspec + Style/AsciiComments: Enabled: false From e25e00cc4cbe3253c4427654a3da4fcec96753c4 Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Sat, 26 Aug 2023 16:40:43 +0200 Subject: [PATCH 20/22] Enable overwrite feature. Capture wrong/invalid CSV format error --- app/controllers/layers_controller.rb | 10 ++++++++-- app/views/layers/import.html.haml | 2 +- app/views/layers/import_preview.html.haml | 4 +++- lib/imports/csv_importer.rb | 16 +++++++--------- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/app/controllers/layers_controller.rb b/app/controllers/layers_controller.rb index 84869d2a..ab1286c3 100644 --- a/app/controllers/layers_controller.rb +++ b/app/controllers/layers_controller.rb @@ -24,10 +24,16 @@ def import; end def import_preview file = params[:import][:file] + @overwrite = params[:import][:overwrite] == '1' return unless file - importer = Imports::CsvImporter.new(file, @layer.id) - importer.import + begin + importer = Imports::CsvImporter.new(file, @layer.id, @overwrite) + importer.import + flash[:notice] = 'CSV read successfully!' + rescue CSV::MalformedCSVError => e + flash[:error] = "Malformed CSV: #{e.message}. (Maybe the file does not contain CSV?)" + end @valid_rows = importer.valid_rows session[:importing_rows] = @valid_rows @invalid_rows = importer.invalid_rows diff --git a/app/views/layers/import.html.haml b/app/views/layers/import.html.haml index d3871bb7..718dfab8 100644 --- a/app/views/layers/import.html.haml +++ b/app/views/layers/import.html.haml @@ -33,7 +33,7 @@ %span.hint The format of the file must be CSV. %hr %p - = f.input :overwrite, :as => :boolean, :input_html => { :checked => "" }, :label => 'Overwrite' + = f.input :overwrite, :as => :boolean, :label => 'Overwrite' %span.hint Overwrite existing places with the same title %hr diff --git a/app/views/layers/import_preview.html.haml b/app/views/layers/import_preview.html.haml index 1a1aceac..55d32b05 100644 --- a/app/views/layers/import_preview.html.haml +++ b/app/views/layers/import_preview.html.haml @@ -19,6 +19,9 @@ .alert.alert-danger %p= flash[:alert] + - if @overwrite + %p.hint + Overwriting places with the same title is enabled. - if !@errored_rows.empty? @@ -43,7 +46,6 @@ %p.small If you want to replace duplicates, go back, tick the "Overwrite?" option and try again. %p.small Otherwise edit your CSV file and remove those rows. = link_to 'Back', :back, class: 'button secondary' - - if @valid_rows.any? .feedback %h3 Entries to be imported diff --git a/lib/imports/csv_importer.rb b/lib/imports/csv_importer.rb index fe0e5f73..44b3442f 100644 --- a/lib/imports/csv_importer.rb +++ b/lib/imports/csv_importer.rb @@ -5,7 +5,7 @@ include ActionView::Helpers::SanitizeHelper class Imports::CsvImporter - attr_reader :invalid_rows, :valid_rows, :duplicate_rows, :errored_rows, :unprocessable_fields + attr_reader :valid_rows, :invalid_rows, :duplicate_rows, :errored_rows, :unprocessable_fields, :error REQUIRED_FIELDS = %w[title lat lon].freeze @@ -15,9 +15,10 @@ class Imports::CsvImporter # TODO: update existing place as an option - def initialize(file, layer_id) + def initialize(file, layer_id, overwrite: false) @file = file @layer = Layer.find(layer_id) + @overwrite = overwrite @invalid_rows = [] @duplicate_rows = [] @valid_rows = [] @@ -40,14 +41,11 @@ def import # dupe handling title = do_sanitize(processed_row['title']) - if @existing_titles.include?(title) + if @existing_titles.include?(title) && @overwrite == false # skip existing rows with this title @duplicate_rows << processed_row error_hash = { data: processed_row, type: 'Duplicate', messages: ['Title already exists'] } @errored_rows << error_hash - # TODO: else - # update existing row - # @existing_titles << title else @existing_titles << title processed_row['title'] = title @@ -59,9 +57,10 @@ def import Rails.logger.info('CSV import returns no valid rows!') elsif @invalid_rows.empty? Rails.logger.info('CSV import completed successfully!') - else - handle_invalid_rows end + return if @invalid_rows.empty? + + handle_invalid_rows end private @@ -72,7 +71,6 @@ def required_fields_present?(row) def validate_header headers = CSV.read(@file.path, headers: true).headers - missing_fields = REQUIRED_FIELDS - headers raise StandardError, "Missing required fields: #{missing_fields.join(', ')}" if missing_fields.any? From 2411af23b5f4f4df1506be55775b24c40e285323 Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Sat, 26 Aug 2023 16:52:24 +0200 Subject: [PATCH 21/22] Fix missing keyword param in controller --- app/controllers/layers_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/layers_controller.rb b/app/controllers/layers_controller.rb index ab1286c3..2f3af4a3 100644 --- a/app/controllers/layers_controller.rb +++ b/app/controllers/layers_controller.rb @@ -28,7 +28,7 @@ def import_preview return unless file begin - importer = Imports::CsvImporter.new(file, @layer.id, @overwrite) + importer = Imports::CsvImporter.new(file, @layer.id, overwrite: @overwrite) importer.import flash[:notice] = 'CSV read successfully!' rescue CSV::MalformedCSVError => e From 708a60ba8c5e178b6a6aff67a2453771cbf8fed9 Mon Sep 17 00:00:00 2001 From: Ulf Treger Date: Mon, 8 Jul 2024 11:52:51 +0200 Subject: [PATCH 22/22] Merged main manually --- app/assets/javascripts/helpers/cluster.js | 77 +++++++++++++++++++ .../{6_tables.css.scss => 7_tables.css.scss} | 0 2 files changed, 77 insertions(+) create mode 100644 app/assets/javascripts/helpers/cluster.js rename app/assets/stylesheets/{6_tables.css.scss => 7_tables.css.scss} (100%) diff --git a/app/assets/javascripts/helpers/cluster.js b/app/assets/javascripts/helpers/cluster.js new file mode 100644 index 00000000..72128a00 --- /dev/null +++ b/app/assets/javascripts/helpers/cluster.js @@ -0,0 +1,77 @@ +var markerclusterSettings = { + + // TODO: involve cluster_large, cluster_medium, cluster_small, cluster_xlarge + + create: function(marker_display_mode, data_id, cluster) { + return { + maxClusterRadius: 30, + showCoverageOnHover: false, + animate: true, + iconCreateFunction: function(cluster) { + if ( marker_display_mode == 'cluster' ) { + return L.divIcon({ + html: '
' + cluster.getChildCount() + '
', + className: "leaflet-data-markercluster", + iconAnchor : [15, 15], + iconSize : [30, 30], + popupAnchor : [0, -28] + }); + } else { + console.log("Cluster: Zigzgag",cluster.getChildCount()); + + let cluster_viz = ( marker_display_mode === 'zigzag cluster' ? cluster_small : cluster_small_with_gradient ); + if ( cluster.getChildCount() >= 10 ) { + cluster_viz = ( marker_display_mode === 'zigzag cluster' ? cluster_xlarge : cluster_xlarge_with_gradient ); + } else if ( cluster.getChildCount() >= 6 ) { + cluster_viz = ( marker_display_mode === 'zigzag cluster' ? cluster_large : cluster_large_with_gradient ); + } else if ( cluster.getChildCount() >= 3 ) { + cluster_viz = ( marker_display_mode === 'zigzag cluster' ? cluster_medium : cluster_medium_with_gradient ); + } + if ( layer_color && layer_color.length > 0 ) { + cluster_viz = cluster_viz.replace(/rgba\(255, 0, 153, 0.8\)/g, layer_color); + } + + let rnd_rotate = Math.floor(Math.random()*25) * 15; + return L.divIcon({ + html: '
' + cluster_viz + '
', + className: "leaflet-data-markercluster", + iconAnchor : [15, 15], + iconSize : [30, 30], + popupAnchor : [0, -28] + }); + } + }, + spiderLegPolylineOptions: { weight: 0, color: '#efefef', opacity: 0.5 }, + elementsPlacementStrategy: "clock", + helpingCircles: true, + clockHelpingCircleOptions: { + weight: 40, + opacity: 0, + color: "#000000", + fill: "#333", + fillOpacity: 0.2 + }, + spiderfyDistanceSurplus: 28, + spiderfyDistanceMultiplier: 1, + spiderfiedClassName: 'spiderfied-places', + elementsMultiplier: 1.4, + firstCircleElements: 10, + spiderfyShapePositions: function(count, centerPt) { + var distanceFromCenter = 50, + markerDistance = 105, + lineLength = markerDistance * (count - 1), + lineStart = centerPt.y - lineLength / 4, + res = [], + i; + + res.length = count; + + for (i = count - 1; i >= 0; i--) { + res[i] = new Point(centerPt.x + distanceFromCenter, lineStart + markerDistance * i); + } + + return res; + } + } + } +}; diff --git a/app/assets/stylesheets/6_tables.css.scss b/app/assets/stylesheets/7_tables.css.scss similarity index 100% rename from app/assets/stylesheets/6_tables.css.scss rename to app/assets/stylesheets/7_tables.css.scss