From 629b1461a1cf59cd9f768a16be9271e344addfec Mon Sep 17 00:00:00 2001 From: edward Date: Tue, 5 Jul 2016 23:30:36 +0100 Subject: [PATCH] fix for fp importer seems to complain if plot already exists #47 --- .../importers/forest_plots_importer.rb | 3 + app/classes/importers/row_importer.rb | 14 ++++- app/controllers/batches_controller.rb | 2 +- app/models/concerns/batch_import.rb | 20 +++++- ...20160705085200_add_duplicate_to_batches.rb | 5 ++ db/misc_sql/delete_fp_import.sql | 2 +- db/schema.rb | 3 +- .../importers/forest_plots_importer_spec.rb | 62 +++++++++++++------ spec/support/database_cleaner.rb | 8 +++ 9 files changed, 95 insertions(+), 24 deletions(-) create mode 100644 db/migrate/20160705085200_add_duplicate_to_batches.rb create mode 100644 spec/support/database_cleaner.rb diff --git a/app/classes/importers/forest_plots_importer.rb b/app/classes/importers/forest_plots_importer.rb index ab43d89..f19dc00 100644 --- a/app/classes/importers/forest_plots_importer.rb +++ b/app/classes/importers/forest_plots_importer.rb @@ -37,10 +37,13 @@ def read_row(values, logger) sub_plot = @sub_plots_cache[sub_plot_code] + puts "sub_plot_code in forest_place is #{sub_plot}" + if sub_plot.nil? # Sub-plots have other columns which are not being imported here # type, area and all that lot. sub_plot = find_or_create(SubPlot, :plot_id => plot.id, :sub_plot_code => sub_plot_code) + attempt_to_overwrite!(sub_plot) @sub_plots_cache[sub_plot_code] = sub_plot end diff --git a/app/classes/importers/row_importer.rb b/app/classes/importers/row_importer.rb index ee66095..3ba392f 100644 --- a/app/classes/importers/row_importer.rb +++ b/app/classes/importers/row_importer.rb @@ -60,8 +60,9 @@ def save_with_status! end def find_or_new(ar_class = nil, unique_identifiers) + batch_id = use_batch_if_it_exists(unique_identifiers) ar_class ||= self.class.ar_class - ar_class.batch_find_or_initialize_by(@batch_id, unique_identifiers) + ar_class.batch_find_or_initialize_by(batch_id, unique_identifiers) end def find_or_create(ar_class = nil, unique_identifiers) @@ -69,8 +70,17 @@ def find_or_create(ar_class = nil, unique_identifiers) ar_class.batch_find_or_create_by!(@batch_id, unique_identifiers) end + def use_batch_if_it_exists(unique_identifiers) + batch_id = unique_identifiers[:sub_plot][:batch_id] || unique_identifiers[:plot][:batch_id] + if batch_id + batch_id + else + @batch_id + end + end + def attempt_to_overwrite!(object) - unless object.can_overwrite(@batch_id, @overwrite_batch_id) + unless object.can_overwrite(object.batch_id, @overwrite_batch_id) raise Gemdata::NoPermissionToOverwrite, "No permission to override #{object.class.name} from a different batch: #{object.to_json}" end end diff --git a/app/controllers/batches_controller.rb b/app/controllers/batches_controller.rb index 597839a..7755f30 100644 --- a/app/controllers/batches_controller.rb +++ b/app/controllers/batches_controller.rb @@ -78,6 +78,6 @@ def set_batch # Never trust parameters from the scary internet, only allow the white list through. def batch_params - params.require(:batch).permit(:import_address, :started, :finished, :transaction_passed) + params.require(:batch).permit(:import_address, :started, :finished, :transaction_passed, :duplicate) end end diff --git a/app/models/concerns/batch_import.rb b/app/models/concerns/batch_import.rb index a5eec05..e3da618 100644 --- a/app/models/concerns/batch_import.rb +++ b/app/models/concerns/batch_import.rb @@ -20,9 +20,27 @@ def batch_find_or_initialize_by(batch_id, params) def batch_find_or_create_by!(batch_id, params) self.find_or_create_by!(params) do |new_object| + update_batch_as_duplicate(batch_id, params) new_object.batch_id = batch_id end end - end + + def update_batch_as_duplicate(batch_id, params) + updated_object = use_batch_from_association(params) + if updated_object.try(:batch_id).present? && updated_object.try(:batch_id) != batch_id + batch_object = Batch.find(batch_id) + batch_object.update(duplicate: true) if batch_object.duplicate == false + end + end + + def use_batch_from_association(params) + if params[:plot_code].present? && params[:fp_id].present? + Plot.where(params).first + elsif params[:tree].present? + params[:tree] + end + end + + end end diff --git a/db/migrate/20160705085200_add_duplicate_to_batches.rb b/db/migrate/20160705085200_add_duplicate_to_batches.rb new file mode 100644 index 0000000..f691545 --- /dev/null +++ b/db/migrate/20160705085200_add_duplicate_to_batches.rb @@ -0,0 +1,5 @@ +class AddDuplicateToBatches < ActiveRecord::Migration + def change + add_column :batches, :duplicate, :boolean, default: false + end +end diff --git a/db/misc_sql/delete_fp_import.sql b/db/misc_sql/delete_fp_import.sql index 01f6723..9c009e4 100644 --- a/db/misc_sql/delete_fp_import.sql +++ b/db/misc_sql/delete_fp_import.sql @@ -11,4 +11,4 @@ WHERE t.sub_plot_id = sp.id AND sp.plot_id = p.id AND p.plot_code = 'INSERT_PLOTCODE_HERE'; -COMMIT; \ No newline at end of file +COMMIT; diff --git a/db/schema.rb b/db/schema.rb index fb334d1..7ec27c2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160319145213) do +ActiveRecord::Schema.define(version: 20160705085200) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -23,6 +23,7 @@ t.boolean "transaction_passed" t.datetime "created_at" t.datetime "updated_at" + t.boolean "duplicate", default: false end create_table "branch_architectures", force: true do |t| diff --git a/spec/classes/importers/forest_plots_importer_spec.rb b/spec/classes/importers/forest_plots_importer_spec.rb index b756cc3..22ae554 100644 --- a/spec/classes/importers/forest_plots_importer_spec.rb +++ b/spec/classes/importers/forest_plots_importer_spec.rb @@ -14,7 +14,6 @@ end it 'can read CSV' do - importer = ForestPlotsImporter.new(1, 2) status = importer.read_row(@values, logger) expect(status).to eq(Lookup::ImportStatus.inserted) @@ -25,7 +24,7 @@ plot = Plot.find(tree.sub_plot.plot_id) expect(plot.fp_id).to eq(90) - expect(plot.plot_code).to eq('TAM04') + expect(plot.plot_code).to eq('TAM-04') expect(plot.plot_desc).to eq('Tambopata plot two swamp edge clay') species = tree.fp_species @@ -52,33 +51,28 @@ end it 'selects existing plots and subplots' do - - plot = Plot.create!(:plot_code => 'TAM04', :fp_id => 90, batch_id: 1) - subplot = SubPlot.create!(:plot => plot, :sub_plot_code => '1', batch_id: 1) + plot = Plot.find_or_create_by(:plot_code => 'TAM-04', :fp_id => 90, batch_id: 1) + subplot = SubPlot.find_or_create_by!(:plot => plot, :sub_plot_code => '1', batch_id: 1) importer = ForestPlotsImporter.new(1, 1) importer.read_row(@values, logger) expect(importer.object.reload.sub_plot.plot).to eq(plot) expect(importer.object.reload.sub_plot).to eq(subplot) - end it 'selects existing species complex' do - - fp_family = FpFamily.create!(apg_id: 377, name: 'Sapotaceae', batch: Batch.new) - fp_genus = FpGenus.create!(fp_id: 24801, name: 'Pouteria', fp_family: fp_family, batch: Batch.new) - fp_species = FpSpecies.create!(fp_id: 653110, name: 'Pouteria indet', fp_genus: fp_genus, batch: Batch.new) + fp_family = FpFamily.find_or_create_by!(apg_id: 377, name: 'Sapotaceae', batch_id: 1) + fp_genus = FpGenus.create!(fp_id: 24801, name: 'Pouteria', fp_family: fp_family, batch_id: 1) + fp_species = FpSpecies.create!(fp_id: 653110, name: 'Pouteria indet', fp_genus: fp_genus, batch_id: 1) importer = ForestPlotsImporter.new(1, 2) importer.read_row(@values, logger) expect(importer.object.reload.fp_species).to eq(fp_species) - end it 'creates selects an existing census' do - - plot = Plot.create!(:plot_code => 'TAM04', :fp_id => 90, batch_id: 1) - sub_plot = SubPlot.create!(:plot_id => plot.id, :sub_plot_code => '1', batch_id: 1) + plot = Plot.find_or_create_by(:plot_code => 'TAM-04', :fp_id => 90, batch_id: 1) + sub_plot = SubPlot.find_or_create_by!(:plot_id => plot.id, :sub_plot_code => '1', batch_id: 1) fp_species = FpSpecies.new tree = Tree.create!(:tree_code => 'T2', :sub_plot => sub_plot, :fp_species => fp_species, :fp_id => 54832, batch_id: 1) census = Census.create!(number: 1, mean_date: '1983.67', plot: plot, batch_id: 1) @@ -86,15 +80,13 @@ importer = ForestPlotsImporter.new(1, 1) importer.read_row(@values, logger) expect(importer.object.reload.censuses).to include(census) - end it 'should not incorrectly flag duplicates' do - first_importer = ForestPlotsImporter.new(1, 2) first_status = first_importer.read_row(@values, logger) - second_values = CSV.parse_line '90,TAM-04,Tambopata plot two swamp edge clay,PERU,Oliver Phillips,1990.755,3,90,Main Plot View,,54832,377,Sapotaceae,24801,Pouteria,653110,Pouteria indet,,,2,121,121,121,121,1300,a,1,5,0,,,' + second_values = CSV.parse_line '90,TAM-04,Tambopata plot two swamp edge clay,PERU,Oliver Phillips,1990.755,3,90,Main Plot View,1,54832,377,Sapotaceae,24801,Pouteria,653110,Pouteria indet,,,2,121,121,121,121,1300,a,1,5,0,,,' second_importer = ForestPlotsImporter.new(1, 1) second_status = second_importer.read_row(second_values, logger) expect(second_status).to eq(Lookup::ImportStatus.skipped) @@ -106,7 +98,7 @@ first_importer = ForestPlotsImporter.new(1, 2) first_status = first_importer.read_row(@values, logger) - second_values = CSV.parse_line '90,TAM-04,Tambopata plot two swamp edge clay,PERU,Oliver Phillips,1983.67,1,90,Main Plot View,,12345,377,Sapotaceae,24801,Pouteria,653110,Pouteria indet,,,2,105,105,105,105,1300,a,1,5,0,,,' + second_values = CSV.parse_line '90,TAM-04,Tambopata plot two swamp edge clay,PERU,Oliver Phillips,1983.67,1,90,Main Plot View,1,12345,377,Sapotaceae,24801,Pouteria,653110,Pouteria indet,,,2,105,105,105,105,1300,a,1,5,0,,,' second_importer = ForestPlotsImporter.new(1, 2) second_status = second_importer.read_row(second_values, logger) expect(second_status).to eq(Lookup::ImportStatus.inserted) @@ -115,6 +107,40 @@ expect(second_importer.object.reload).to_not eq(first_importer.object.reload) end + it 'should be able to delete forest_import record using sql' do + batch = Batch.find_or_create_by!(id:2) + importer = ForestPlotsImporter.new(batch.id, 1) + status = importer.read_row(@values, logger) + tree = importer.object.reload + + custom_sql = " + DELETE from dbh_measurements d USING trees t, sub_plots sp, plots p + WHERE d.tree_id = #{tree.id} + AND t.sub_plot_id = #{tree.sub_plot_id} + AND sp.plot_id = #{tree.sub_plot.plot_id} + AND p.plot_code = 'TAM-04'; + + DELETE from trees t USING sub_plots sp, plots p + WHERE t.sub_plot_id = #{tree.sub_plot_id} + AND sp.plot_id = #{tree.sub_plot.plot_id} + AND p.plot_code = 'TAM-04'; + COMMIT; + " + + ActiveRecord::Base.connection.execute(custom_sql) + expect(status).to eq(Lookup::ImportStatus.inserted) + expect{Tree.find(tree.id)}.to raise_exception(ActiveRecord::RecordNotFound) + end + + it 'should be able to save forest_import record after an sql delete' do + batch = Batch.find_or_create_by!(id:2) + importer = ForestPlotsImporter.new(batch.id, 1) + status = importer.read_row(@values, logger) + tree = importer.object.reload + tree_from_db = Tree.find(tree.id) + expect(tree_from_db.id).to eq(tree.id) + end + it 'should trim imports damn it!' diff --git a/spec/support/database_cleaner.rb b/spec/support/database_cleaner.rb new file mode 100644 index 0000000..5d1099f --- /dev/null +++ b/spec/support/database_cleaner.rb @@ -0,0 +1,8 @@ +require "rake" + +RSpec.configure do |config| + config.after :all do + Gemdata::Application.load_tasks + Rake::Task['db:reset'].invoke + end +end