improve resilience of csv import of partners

Author: McEileen

app/controllers/concerns/importable.rb

@@ -25,7 +25,7 @@ module Importable
   def import_csv
     if params[:file].present?
       data = File.read(params[:file].path, encoding: "BOM|UTF-8")
-      csv = CSV.parse(data, headers: true)
+      csv = CSV.parse(data, headers: true, skip_blanks: true)
       if csv.count.positive? && csv.first.headers.all? { |header| !header.nil? }
         errors = resource_model.import_csv(csv, current_organization.id)
         if errors.empty?

app/models/partner.rb

@@ -57,6 +57,7 @@ class Partner < ApplicationRecord
   validate :default_storage_location_belongs_to_organization
 
   before_save { email&.downcase! }
+  before_create :default_send_reminders_to_false, if: :send_reminders_nil?
   before_update :invite_new_partner, if: :should_invite_because_email_changed?
 
   scope :alphabetized, -> { order(:name) }
@@ -110,7 +111,9 @@ def self.import_csv(csv, organization_id)
 
       svc = PartnerCreateService.new(organization: organization, partner_attrs: hash_rows)
       svc.call
-      if svc.errors.present?
+      if svc.errors.present? && svc.partner.errors.blank?
+        errors << "#{svc.partner.name}: #{svc.errors.full_messages.to_sentence}"
+      elsif svc.errors.present?
         errors << "#{svc.partner.name}: #{svc.partner.errors.full_messages.to_sentence}"
       end
     end
@@ -172,6 +175,14 @@ def correct_document_mime_type
     end
   end
 
+  def default_send_reminders_to_false
+    self.send_reminders = false
+  end
+
+  def send_reminders_nil?
+    send_reminders.nil?
+  end
+
   def invite_new_partner
     UserInviteService.invite(email: email, roles: [Role::PARTNER], resource: self)
   end

app/services/partner_create_service.rb

@@ -9,9 +9,14 @@ def initialize(organization:, partner_attrs:)
   end
 
   def call
-    if partner_attrs["default_storage_location"]
-      default_storage_location_name = partner_attrs["default_storage_location"].titlecase
-      default_storage_location_id = StorageLocation.find_by(name: default_storage_location_name)&.id
+    if partner_attrs.has_key?("default_storage_location") && partner_attrs["default_storage_location"].blank?
+      partner_attrs.delete("default_storage_location")
+    elsif partner_attrs.has_key?("default_storage_location") && !partner_attrs["default_storage_location"].blank?
+      default_storage_location_name = partner_attrs["default_storage_location"]&.titlecase
+      default_storage_location_id = StorageLocation.find_by(name: default_storage_location_name, organization: organization.id)&.id
+      if default_storage_location_id.nil?
+        errors.add(:default_storage_location, "The default storage location is not a storage location for this partner's organization")
+      end
       partner_attrs.delete("default_storage_location")
       partner_attrs["default_storage_location_id"] = default_storage_location_id
     end

spec/fixtures/files/partners_missing_default_storage_location_field.csv

@@ -0,0 +1,3 @@
+name,email,default_storage_location,send_reminders,quota,notes
+Partner 51,[email protected],,false,50,"great partner"
+

spec/fixtures/files/partners_missing_default_storage_location_field_and_header.csv

@@ -0,0 +1,3 @@
+name,email,send_reminders,quota,notes
+Partner 71,[email protected],false,50,"great partner"
+

spec/fixtures/files/partners_missing_send_reminders_field.csv

@@ -0,0 +1,3 @@
+name,email,default_storage_location,send_reminders,quota,notes
+Partner 51,[email protected],"Smithsonian Conservation Center",,50,"great partner"
+

spec/fixtures/files/partners_missing_send_reminders_field_and_header.csv

@@ -0,0 +1,3 @@
+name,email,default_storage_location,quota,notes
+Partner 51,[email protected],"Smithsonian Conservation Center",50,"great partner"
+

spec/fixtures/files/partners_with_final_line_blank.csv

@@ -0,0 +1,3 @@
+name,email,default_storage_location,send_reminders,quota,notes
+Partner 31,[email protected],"Smithsonian Conservation Center",true,50,"great partner"
+

spec/requests/partners_requests_spec.rb

@@ -478,6 +478,46 @@
       end
     end
 
+    context "csv file with send_reminders header and field missing" do
+      let(:file) { fixture_file_upload("partners_missing_send_reminders_field_and_header.csv", "text/csv") }
+      subject { post import_csv_partners_path, params: { file: file } }
+
+      it "invokes .import_csv" do
+        expect(model_class).to respond_to(:import_csv).with(2).arguments
+      end
+
+      it "redirects to :index" do
+        subject
+        expect(response).to be_redirect
+      end
+
+      it "defaults send_reminders to false" do
+        subject
+        partner = Partner.find_by(name: "Partner 51")
+        expect(partner.send_reminders).to be(false)
+      end
+    end
+
+    context "csv file with send_reminders field missing" do
+      let(:file) { fixture_file_upload("partners_missing_send_reminders_field.csv", "text/csv") }
+      subject { post import_csv_partners_path, params: { file: file } }
+
+      it "invokes .import_csv" do
+        expect(model_class).to respond_to(:import_csv).with(2).arguments
+      end
+
+      it "redirects to :index" do
+        subject
+        expect(response).to be_redirect
+      end
+
+      it "defaults send_reminders to false" do
+        subject
+        partner = Partner.find_by(name: "Partner 51")
+        expect(partner.send_reminders).to be(false)
+      end
+    end
+
     context "csv file with invalid email address" do
       let(:file) { fixture_file_upload("partners_with_invalid_email.csv", "text/csv") }
       subject { post import_csv_partners_path, params: { file: file } }
@@ -498,6 +538,44 @@
       end
     end
 
+    context "csv file with default storage location header and field missing" do
+      let(:file) { fixture_file_upload("partners_missing_default_storage_location_field_and_header.csv", "text/csv") }
+      subject { post import_csv_partners_path, params: { file: file } }
+
+      it "invokes .import_csv" do
+        expect(model_class).to respond_to(:import_csv).with(2).arguments
+      end
+
+      it "redirects to :index" do
+        subject
+        expect(response).to be_redirect
+      end
+
+      it "presents a flash notice message" do
+        subject
+        expect(response).to have_notice "#{model_class.name.underscore.humanize.pluralize} were imported successfully!"
+      end
+    end
+
+    context "csv file with default storage location field missing" do
+      let(:file) { fixture_file_upload("partners_missing_default_storage_location_field.csv", "text/csv") }
+      subject { post import_csv_partners_path, params: { file: file } }
+
+      it "invokes .import_csv" do
+        expect(model_class).to respond_to(:import_csv).with(2).arguments
+      end
+
+      it "redirects to :index" do
+        subject
+        expect(response).to be_redirect
+      end
+
+      it "presents a flash notice message" do
+        subject
+        expect(response).to have_notice "#{model_class.name.underscore.humanize.pluralize} were imported successfully!"
+      end
+    end
+
     context "csv file with default storage location, email preferences, quota, and notes" do
       let(:file) { fixture_file_upload("partners_with_six_fields.csv", "text/csv") }
       subject { post import_csv_partners_path, params: { file: file } }
@@ -530,7 +608,7 @@
 
       it "presents a flash error message" do
         subject
-        expect(response).to have_error "The following Partners did not import successfully:\nPartner 4: Default storage location The default storage location is not a storage location for this partner's organization"
+        expect(response).to have_error "The following Partners did not import successfully:\nPartner 4: default_storage_location The default storage location is not a storage location for this partner's organization"
       end
     end
 
@@ -552,6 +630,21 @@
         expect(response).to have_notice "#{model_class.name.underscore.humanize.pluralize} were imported successfully!"
       end
     end
+
+    context "csv file with a blank line at the file's bottom" do
+      let(:file) { fixture_file_upload("partners_with_final_line_blank.csv", "text/csv") }
+      subject { post import_csv_partners_path, params: { file: file } }
+
+      it "redirects to :index" do
+        subject
+        expect(response).to be_redirect
+      end
+
+      it "presents a flash notice message" do
+        subject
+        expect(response).to have_notice "#{model_class.name.underscore.humanize.pluralize} were imported successfully!"
+      end
+    end
   end
 
   describe "POST #create" do

spec/services/partner_create_service_spec.rb

@@ -43,6 +43,32 @@
         expect(query.first.enable_quantity_based_requests).to eq(true)
       end
 
+      context 'when send_reminders is nil' do
+        before do
+          partner_attrs.merge!(send_reminders: nil)
+        end
+
+        it 'defaults send_reminders to false' do
+          subject
+
+          partner = Partner.find_by(name: partner_attrs[:name])
+          expect(partner.send_reminders).to be(false)
+        end
+      end
+
+      context 'when send_reminders is missing' do
+        before do
+          partner_attrs.delete(:send_reminders)
+        end
+
+        it 'defaults send_reminders to false' do
+          subject
+
+          partner = Partner.find_by(name: partner_attrs[:name])
+          expect(partner.send_reminders).to be(false)
+        end
+      end
+
       context 'but there was an unexpected issue with saving the' do
         let(:error_message) { Faker::Games::ElderScrolls.dragon }