From db6eebbf6e82c84751a0fa00ad59da82f0466292 Mon Sep 17 00:00:00 2001 From: McEileen Date: Sun, 27 Apr 2025 20:54:16 -0400 Subject: [PATCH 1/8] enable csv import of partners with six fields --- app/models/partner.rb | 9 +++ app/services/partner_create_service.rb | 6 ++ .../_getting_started_prompt.html.erb | 12 ++++ app/views/partners/index.html.erb | 2 +- public/partners_template.csv | 4 -- public/partners_template_updated.csv | 4 ++ spec/fixtures/files/partners.csv | 4 -- .../files/partners_with_bom_encoding.csv | 42 ++++++------- .../files/partners_with_duplicates.csv | 4 -- .../files/partners_with_six_fields.csv | 4 ++ ...artners_with_six_fields_and_duplicates.csv | 4 ++ ...s_with_six_fields_capitalized_location.csv | 4 ++ ...tners_with_six_fields_invalid_location.csv | 2 + spec/models/partner_spec.rb | 31 +++++++++- spec/requests/partners_requests_spec.rb | 62 ++++++++++++++++++- 15 files changed, 156 insertions(+), 38 deletions(-) delete mode 100644 public/partners_template.csv create mode 100644 public/partners_template_updated.csv delete mode 100644 spec/fixtures/files/partners.csv delete mode 100644 spec/fixtures/files/partners_with_duplicates.csv create mode 100644 spec/fixtures/files/partners_with_six_fields.csv create mode 100644 spec/fixtures/files/partners_with_six_fields_and_duplicates.csv create mode 100644 spec/fixtures/files/partners_with_six_fields_capitalized_location.csv create mode 100644 spec/fixtures/files/partners_with_six_fields_invalid_location.csv diff --git a/app/models/partner.rb b/app/models/partner.rb index 6c646902c9..62aabce40b 100644 --- a/app/models/partner.rb +++ b/app/models/partner.rb @@ -55,6 +55,8 @@ class Partner < ApplicationRecord validate :correct_document_mime_type + validate :default_storage_location_belongs_to_organization + before_save { email&.downcase! } before_update :invite_new_partner, if: :should_invite_because_email_changed? @@ -158,6 +160,13 @@ def family_zipcodes_list families.pluck(:guardian_zip_code).uniq end + def default_storage_location_belongs_to_organization + location_ids = organization&.storage_locations&.pluck(:id) + unless location_ids&.include?(default_storage_location_id) || default_storage_location_id.nil? + errors.add(:default_storage_location_id, "The default storage location is not a storage location for this partner's organization") + end + end + def correct_document_mime_type if documents.attached? && documents.any? { |doc| !doc.content_type.in?(ALLOWED_MIME_TYPES) } errors.add(:documents, "Must be a PDF or DOC file") diff --git a/app/services/partner_create_service.rb b/app/services/partner_create_service.rb index 6b81bfae59..21e4d9fec2 100644 --- a/app/services/partner_create_service.rb +++ b/app/services/partner_create_service.rb @@ -9,6 +9,12 @@ 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 + partner_attrs.delete("default_storage_location") + partner_attrs["default_storage_location_id"] = default_storage_location_id + end @partner = organization.partners.build(partner_attrs) if @partner.valid? diff --git a/app/views/dashboard/_getting_started_prompt.html.erb b/app/views/dashboard/_getting_started_prompt.html.erb index 7c745cfef0..53f655dbcb 100644 --- a/app/views/dashboard/_getting_started_prompt.html.erb +++ b/app/views/dashboard/_getting_started_prompt.html.erb @@ -59,5 +59,17 @@ <%= button_to "Dismiss", "/manage", method: :patch, params: {organization: { bank_is_set_up: true }}, class: "btn btn-primary" %> + <%= render( + layout: "shared/csv_import_modal", + locals: { + import_type: "Partners", + csv_template_url: "/partners_template_updated.csv", + csv_import_url: import_csv_partners_path, + }, + ) do %> +
  • Open the CSV file with Excel or your favourite spreadsheet program.
    • +
  • Delete the sample data and enter your partner agency names and addresses in the appropriate columns.
    • +
  • Save the file as a CSV file.
    • + <% end %> <% end %> <% end %> diff --git a/app/views/partners/index.html.erb b/app/views/partners/index.html.erb index ce3fc6def8..7215f8ed00 100644 --- a/app/views/partners/index.html.erb +++ b/app/views/partners/index.html.erb @@ -56,7 +56,7 @@ layout: "shared/csv_import_modal", locals: { import_type: "Partners", - csv_template_url: "/partners_template.csv", + csv_template_url: "/partners_template_updated.csv", csv_import_url: import_csv_partners_path } ) do %> diff --git a/public/partners_template.csv b/public/partners_template.csv deleted file mode 100644 index bb62004176..0000000000 --- a/public/partners_template.csv +++ /dev/null @@ -1,4 +0,0 @@ -name,email -Partner 1,partner1@example.com -Partner 2,partner2@example.com -Partner 3,partner3@example.com \ No newline at end of file diff --git a/public/partners_template_updated.csv b/public/partners_template_updated.csv new file mode 100644 index 0000000000..17ac82af17 --- /dev/null +++ b/public/partners_template_updated.csv @@ -0,0 +1,4 @@ +name,email,default_storage_location,send_reminders,quota,notes +Partner 1,partner1@example.com,"Smithsonian Conservation Center",false,20,"test note 1" +Partner 2,partner2@example.com,"Smithsonian Conservation Center",false,30,"test note 2" +Partner 3,partner3@example.com,"Smithsonian Conservation Center",true,10,"test note 3" \ No newline at end of file diff --git a/spec/fixtures/files/partners.csv b/spec/fixtures/files/partners.csv deleted file mode 100644 index bb62004176..0000000000 --- a/spec/fixtures/files/partners.csv +++ /dev/null @@ -1,4 +0,0 @@ -name,email -Partner 1,partner1@example.com -Partner 2,partner2@example.com -Partner 3,partner3@example.com \ No newline at end of file diff --git a/spec/fixtures/files/partners_with_bom_encoding.csv b/spec/fixtures/files/partners_with_bom_encoding.csv index 36d82c5129..a2d24f2914 100644 --- a/spec/fixtures/files/partners_with_bom_encoding.csv +++ b/spec/fixtures/files/partners_with_bom_encoding.csv @@ -1,21 +1,21 @@ -name,email -Beaverton Police Department,krodriguez@beavertonoregon.gov -Catholic Charities,lcrombie@catholiccharitiesoregon.org -Clackamas Service Center,debramason@cscoregon.org -Healthy Familes of Clackamas County,bkersens@healthyfamiliescc.org -Dollar for Portland,jared@dollorfor.org -Emmanuel Housing Center,shauntemeyers@emmanuelpdx.org -Helensview School,hgandee@mesd.k12.or.us -JOIN,ccarroll@joinpdx.org -Job Corps (PIVOT),zutz.kayla@jobcorps.org -NARA,christman@naranw.org -NW Housing Alternatives,doty@nwhousing.org -Pregnancy Resource Center,debbie@portlandprc.org -Portland Homeless Family Solutions,emma@pdxhfs.org -Raphael House,lvold@raphaelhouse.com -The Rebecca Foundation's Cloth Diaper Closet,portland@clothforall.org -Rose Haven,adeol@rosehaven.org -Self Enhancement Inc.,stephaniep@selfenhancement.org -Teen Parent Services (PPS),lgovan@pps.net -Volunteers of America,cross@voaor.org -Central City Concern,lindsey.ramsey@ccconcern.org \ No newline at end of file +name,email,default_storage_location,send_reminders,quota,notes +Beaverton Police Department,krodriguez@beavertonoregon.gov,"Smithsonian Conservation Center",true,50,"great partner" +Catholic Charities,lcrombie@catholiccharitiesoregon.org,"Smithsonian Conservation Center",true,50,"great partner" +Clackamas Service Center,debramason@cscoregon.org,"Smithsonian Conservation Center",true,50,"great partner" +Healthy Familes of Clackamas County,bkersens@healthyfamiliescc.org,"Smithsonian Conservation Center",true,50,"great partner" +Dollar for Portland,jared@dollorfor.org,"Smithsonian Conservation Center",true,50,"great partner" +Emmanuel Housing Center,shauntemeyers@emmanuelpdx.org,"Smithsonian Conservation Center",true,50,"great partner" +Helensview School,hgandee@mesd.k12.or.us,"Smithsonian Conservation Center",true,50,"great partner" +JOIN,ccarroll@joinpdx.org,"Smithsonian Conservation Center",true,50,"great partner" +Job Corps (PIVOT),zutz.kayla@jobcorps.org,"Smithsonian Conservation Center",true,50,"great partner" +NARA,christman@naranw.org,"Smithsonian Conservation Center",true,50,"great partner" +NW Housing Alternatives,doty@nwhousing.org,"Smithsonian Conservation Center",true,50,"great partner" +Pregnancy Resource Center,debbie@portlandprc.org,"Smithsonian Conservation Center",true,50,"great partner" +Portland Homeless Family Solutions,emma@pdxhfs.org,"Smithsonian Conservation Center",true,50,"great partner" +Raphael House,lvold@raphaelhouse.com,"Smithsonian Conservation Center",true,50,"great partner" +The Rebecca Foundation's Cloth Diaper Closet,portland@clothforall.org,"Smithsonian Conservation Center",true,50,"great partner" +Rose Haven,adeol@rosehaven.org,"Smithsonian Conservation Center",true,50,"great partner" +Self Enhancement Inc.,stephaniep@selfenhancement.org,"Smithsonian Conservation Center",true,50,"great partner" +Teen Parent Services (PPS),lgovan@pps.net,"Smithsonian Conservation Center",true,50,"great partner" +Volunteers of America,cross@voaor.org,"Smithsonian Conservation Center",true,50,"great partner" +Central City Concern,lindsey.ramsey@ccconcern.org,"Smithsonian Conservation Center",true,50,"great partner" \ No newline at end of file diff --git a/spec/fixtures/files/partners_with_duplicates.csv b/spec/fixtures/files/partners_with_duplicates.csv deleted file mode 100644 index c4890e78af..0000000000 --- a/spec/fixtures/files/partners_with_duplicates.csv +++ /dev/null @@ -1,4 +0,0 @@ -name,email -Partner 1,partner1@example.com -Partner 2,partner2@example.com -Partner 4,partner4@example.com \ No newline at end of file diff --git a/spec/fixtures/files/partners_with_six_fields.csv b/spec/fixtures/files/partners_with_six_fields.csv new file mode 100644 index 0000000000..797b65f0f6 --- /dev/null +++ b/spec/fixtures/files/partners_with_six_fields.csv @@ -0,0 +1,4 @@ +name,email,default_storage_location,send_reminders,quota,notes +Partner 1,partner1@example.com,"Smithsonian Conservation Center",true,50,"great partner" +Partner 2,partner2@example.com,"Smithsonian Conservation Center",true,75,"such a great partner" +Partner 4,partner4@example.com,"Smithsonian Conservation Center",false,80,"really ten out of ten" diff --git a/spec/fixtures/files/partners_with_six_fields_and_duplicates.csv b/spec/fixtures/files/partners_with_six_fields_and_duplicates.csv new file mode 100644 index 0000000000..6250a90ae8 --- /dev/null +++ b/spec/fixtures/files/partners_with_six_fields_and_duplicates.csv @@ -0,0 +1,4 @@ +name,email,default_storage_location,send_reminders,quota,notes +Partner 1,partner1@example.com,"Smithsonian Conservation Center",true,50,"great partner" +Partner 2,partner2@example.com,"Smithsonian Conservation Center",true,75,"such a great partner" +Partner 3,partner3@example.com,"Smithsonian Conservation Center",true,80,"yay, a partner" diff --git a/spec/fixtures/files/partners_with_six_fields_capitalized_location.csv b/spec/fixtures/files/partners_with_six_fields_capitalized_location.csv new file mode 100644 index 0000000000..0f8cfd6856 --- /dev/null +++ b/spec/fixtures/files/partners_with_six_fields_capitalized_location.csv @@ -0,0 +1,4 @@ +name,email,default_storage_location,send_reminders,quota,notes +Partner 1,partner1@example.com,"SMITHSONIAN CONSERVATION CENTER",true,50,"great partner" +Partner 2,partner2@example.com,"Smithsonian Conservation Center",true,75,"such a great partner" +Partner 4,partner4@example.com,"Smithsonian Conservation Center",false,80,"really ten out of ten" diff --git a/spec/fixtures/files/partners_with_six_fields_invalid_location.csv b/spec/fixtures/files/partners_with_six_fields_invalid_location.csv new file mode 100644 index 0000000000..14daacbdd8 --- /dev/null +++ b/spec/fixtures/files/partners_with_six_fields_invalid_location.csv @@ -0,0 +1,2 @@ +name,email,default_storage_location,send_reminders,quota,notes +Partner 4,partner4@example.com,"Invalid",false,80,"really ten out of ten" \ No newline at end of file diff --git a/spec/models/partner_spec.rb b/spec/models/partner_spec.rb index f0dbc2066a..fa395e1582 100644 --- a/spec/models/partner_spec.rb +++ b/spec/models/partner_spec.rb @@ -258,15 +258,20 @@ describe "import_csv" do let(:organization) { create(:organization) } + before do + create(:storage_location, organization: organization) + create(:storage_location, organization: organization, name: "shed") + create(:storage_location, organization: organization, name: "building") + end it "imports partners from a csv file and prevents multiple imports" do before_import = Partner.count - import_file_path = Rails.root.join("spec", "fixtures", "files", "partners.csv") + import_file_path = Rails.root.join("spec", "fixtures", "files", "partners_with_six_fields.csv") data = File.read(import_file_path, encoding: "BOM|UTF-8") csv = CSV.parse(data, headers: true) Partner.import_csv(csv, organization.id) expect(Partner.count).to eq before_import + 3 - import_file_path2 = Rails.root.join("spec", "fixtures", "files", "partners_with_duplicates.csv") + import_file_path2 = Rails.root.join("spec", "fixtures", "files", "partners_with_six_fields_and_duplicates.csv") data2 = File.read(import_file_path2, encoding: "BOM|UTF-8") csv2 = CSV.parse(data2, headers: true) Partner.import_csv(csv2, organization.id) @@ -281,6 +286,28 @@ Partner.import_csv(csv, organization.id) end.to change { Partner.count }.by(20) end + + it "imports partners that have six fields" do + import_file_path = Rails.root.join("spec", "fixtures", "files", "partners_with_six_fields.csv") + data = File.read(import_file_path, encoding: "BOM|UTF-8") + csv = CSV.parse(data, headers: true) + expect do + Partner.import_csv(csv, organization.id) + end.to change { Partner.count }.by(3) + end + + it "imports partners with the correct values for fields" do + import_file_path = Rails.root.join("spec", "fixtures", "files", "partners_with_six_fields.csv") + data = File.read(import_file_path, encoding: "BOM|UTF-8") + csv = CSV.parse(data, headers: true) + Partner.import_csv(csv, organization.id) + + partner = Partner.last + expect(StorageLocation.find(partner.default_storage_location_id).name).to eq("Smithsonian Conservation Center") + expect(partner.send_reminders).to eq(false) + expect(partner.quota).to eq(80) + expect(partner.notes).to eq("really ten out of ten") + end end describe '#quantity_year_to_date' do diff --git a/spec/requests/partners_requests_spec.rb b/spec/requests/partners_requests_spec.rb index 2e4255164f..c7552ae8a3 100644 --- a/spec/requests/partners_requests_spec.rb +++ b/spec/requests/partners_requests_spec.rb @@ -240,7 +240,7 @@ enable_quantity_based_requests: true) end - it "orders partners alphaetically" do + it "orders partners alphabetically" do get partners_path(partner, format: response_format) csv = CSV.parse(response.body) @@ -433,9 +433,12 @@ describe "POST #import_csv" do let(:model_class) { Partner } + let!(:outside_organization) { create(:organization) } + let!(:invalid_storage_location) { create(:storage_location, name: 'invalid', organization: outside_organization) } + let!(:valid_storage_location) { create(:storage_location, organization: organization) } context "with a csv file" do - let(:file) { fixture_file_upload("#{model_class.name.underscore.pluralize}.csv", "text/csv") } + let(:file) { fixture_file_upload("partners_with_six_fields.csv", "text/csv") } subject { post import_csv_partners_path, params: { file: file } } it "invokes .import_csv" do @@ -499,6 +502,61 @@ expect(response).to have_error(/Partner 2: Email is invalid/) 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 } } + + 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 an invalid storage location" do + let!(:current_organization) { create(:organization) } + let!(:outside_organization) { create(:organization) } + let!(:outside_storage_location) { create(:storage_location, name: "Invalid", organization: outside_organization) } + let(:file) { fixture_file_upload("partners_with_six_fields_invalid_location.csv", "text/csv") } + before do + allow(controller).to receive(:current_organization).and_return(current_organization) + end + + subject { post import_csv_partners_path, params: { file: file } } + + 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" + end + end + + context "csv file with a valid all-caps storage location" do + let(:file) { fixture_file_upload("partners_with_six_fields.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 end describe "POST #create" do From 955d07d5c3a0b0a5abf8de5e56b243a0f0e3ca0b Mon Sep 17 00:00:00 2001 From: McEileen Date: Sat, 24 May 2025 12:56:34 -0400 Subject: [PATCH 2/8] improve resilience of csv import of partners --- app/controllers/concerns/importable.rb | 2 +- app/models/partner.rb | 13 ++- app/services/partner_create_service.rb | 11 ++- ...missing_default_storage_location_field.csv | 3 + ...ault_storage_location_field_and_header.csv | 3 + .../partners_missing_send_reminders_field.csv | 3 + ...issing_send_reminders_field_and_header.csv | 3 + .../files/partners_with_final_line_blank.csv | 3 + spec/requests/partners_requests_spec.rb | 95 ++++++++++++++++++- spec/services/partner_create_service_spec.rb | 26 +++++ 10 files changed, 156 insertions(+), 6 deletions(-) create mode 100644 spec/fixtures/files/partners_missing_default_storage_location_field.csv create mode 100644 spec/fixtures/files/partners_missing_default_storage_location_field_and_header.csv create mode 100644 spec/fixtures/files/partners_missing_send_reminders_field.csv create mode 100644 spec/fixtures/files/partners_missing_send_reminders_field_and_header.csv create mode 100644 spec/fixtures/files/partners_with_final_line_blank.csv diff --git a/app/controllers/concerns/importable.rb b/app/controllers/concerns/importable.rb index f2bd1515bb..eb3dbefa6d 100644 --- a/app/controllers/concerns/importable.rb +++ b/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? diff --git a/app/models/partner.rb b/app/models/partner.rb index 62aabce40b..2911eea518 100644 --- a/app/models/partner.rb +++ b/app/models/partner.rb @@ -58,6 +58,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) } @@ -111,7 +112,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 @@ -173,6 +176,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 diff --git a/app/services/partner_create_service.rb b/app/services/partner_create_service.rb index 21e4d9fec2..af40104eef 100644 --- a/app/services/partner_create_service.rb +++ b/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 diff --git a/spec/fixtures/files/partners_missing_default_storage_location_field.csv b/spec/fixtures/files/partners_missing_default_storage_location_field.csv new file mode 100644 index 0000000000..a3ad3a6abb --- /dev/null +++ b/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,partner51@example.com,,false,50,"great partner" + diff --git a/spec/fixtures/files/partners_missing_default_storage_location_field_and_header.csv b/spec/fixtures/files/partners_missing_default_storage_location_field_and_header.csv new file mode 100644 index 0000000000..9cd070cac5 --- /dev/null +++ b/spec/fixtures/files/partners_missing_default_storage_location_field_and_header.csv @@ -0,0 +1,3 @@ +name,email,send_reminders,quota,notes +Partner 71,partner71@example.com,false,50,"great partner" + diff --git a/spec/fixtures/files/partners_missing_send_reminders_field.csv b/spec/fixtures/files/partners_missing_send_reminders_field.csv new file mode 100644 index 0000000000..33dc94979e --- /dev/null +++ b/spec/fixtures/files/partners_missing_send_reminders_field.csv @@ -0,0 +1,3 @@ +name,email,default_storage_location,send_reminders,quota,notes +Partner 51,partner51@example.com,"Smithsonian Conservation Center",,50,"great partner" + diff --git a/spec/fixtures/files/partners_missing_send_reminders_field_and_header.csv b/spec/fixtures/files/partners_missing_send_reminders_field_and_header.csv new file mode 100644 index 0000000000..b29569b895 --- /dev/null +++ b/spec/fixtures/files/partners_missing_send_reminders_field_and_header.csv @@ -0,0 +1,3 @@ +name,email,default_storage_location,quota,notes +Partner 51,partner51@example.com,"Smithsonian Conservation Center",50,"great partner" + diff --git a/spec/fixtures/files/partners_with_final_line_blank.csv b/spec/fixtures/files/partners_with_final_line_blank.csv new file mode 100644 index 0000000000..c8a10d842b --- /dev/null +++ b/spec/fixtures/files/partners_with_final_line_blank.csv @@ -0,0 +1,3 @@ +name,email,default_storage_location,send_reminders,quota,notes +Partner 31,partner31@example.com,"Smithsonian Conservation Center",true,50,"great partner" + diff --git a/spec/requests/partners_requests_spec.rb b/spec/requests/partners_requests_spec.rb index c7552ae8a3..4708dbc6f7 100644 --- a/spec/requests/partners_requests_spec.rb +++ b/spec/requests/partners_requests_spec.rb @@ -483,6 +483,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 } } @@ -503,6 +543,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 } } @@ -535,7 +613,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 @@ -557,6 +635,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 diff --git a/spec/services/partner_create_service_spec.rb b/spec/services/partner_create_service_spec.rb index cc55d748ab..86eec31693 100644 --- a/spec/services/partner_create_service_spec.rb +++ b/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 } From 3dfc64e5290e973f1fe6df308ac3c480838be372 Mon Sep 17 00:00:00 2001 From: McEileen Date: Sun, 1 Jun 2025 13:41:03 -0400 Subject: [PATCH 3/8] enable importable concern to display flash errors or flash alerts --- app/controllers/concerns/importable.rb | 8 +++++--- app/models/concerns/provideable.rb | 2 +- app/models/donation_site.rb | 3 ++- app/models/partner.rb | 14 +++----------- app/models/storage_location.rb | 2 +- spec/models/donation_site_spec.rb | 6 +++--- spec/requests/partners_requests_spec.rb | 3 ++- 7 files changed, 17 insertions(+), 21 deletions(-) diff --git a/app/controllers/concerns/importable.rb b/app/controllers/concerns/importable.rb index eb3dbefa6d..ada17747cc 100644 --- a/app/controllers/concerns/importable.rb +++ b/app/controllers/concerns/importable.rb @@ -27,11 +27,13 @@ def import_csv data = File.read(params[:file].path, encoding: "BOM|UTF-8") 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? + errors, warnings = resource_model.import_csv(csv, current_organization.id) + if errors.empty? && warnings.empty? flash[:notice] = "#{resource_model_humanized} were imported successfully!" - else + elsif errors.present? flash[:error] = "The following #{resource_model_humanized} did not import successfully:\n#{errors.join("\n")}" + elsif warnings.present? + flash[:alert] = "The following #{resource_model_humanized} imported with warnings:\n#{warnings.join("\n")}" end else flash[:error] = "Check headers in file!" diff --git a/app/models/concerns/provideable.rb b/app/models/concerns/provideable.rb index 5f25f61810..e7be7b19c0 100644 --- a/app/models/concerns/provideable.rb +++ b/app/models/concerns/provideable.rb @@ -15,7 +15,7 @@ def self.import_csv(csv, organization) loc.save! end - [] + [[], []] end def self.csv_export_headers diff --git a/app/models/donation_site.rb b/app/models/donation_site.rb index cc79d2f46e..c06eac27f7 100644 --- a/app/models/donation_site.rb +++ b/app/models/donation_site.rb @@ -38,6 +38,7 @@ class DonationSite < ApplicationRecord def self.import_csv(csv, organization) errors = [] + warnings = [] csv.each_with_index do |row, index| loc = DonationSite.new(row.to_hash) loc.organization_id = organization @@ -47,7 +48,7 @@ def self.import_csv(csv, organization) errors << "Row #{index + 2}, #{row.to_hash["name"]} - #{loc.errors.full_messages.join(", ")}" end end - errors + [errors, warnings] end def self.csv_export_headers diff --git a/app/models/partner.rb b/app/models/partner.rb index 2911eea518..26fb7ba0ad 100644 --- a/app/models/partner.rb +++ b/app/models/partner.rb @@ -55,8 +55,6 @@ class Partner < ApplicationRecord validate :correct_document_mime_type - 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? @@ -105,6 +103,7 @@ def approvable? # better to extract this outside of the model def self.import_csv(csv, organization_id) errors = [] + warnings = [] organization = Organization.find(organization_id) csv.each do |row| @@ -113,12 +112,12 @@ def self.import_csv(csv, organization_id) svc = PartnerCreateService.new(organization: organization, partner_attrs: hash_rows) svc.call if svc.errors.present? && svc.partner.errors.blank? - errors << "#{svc.partner.name}: #{svc.errors.full_messages.to_sentence}" + warnings << "#{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 - errors + [errors, warnings] end def partials_to_show @@ -163,13 +162,6 @@ def family_zipcodes_list families.pluck(:guardian_zip_code).uniq end - def default_storage_location_belongs_to_organization - location_ids = organization&.storage_locations&.pluck(:id) - unless location_ids&.include?(default_storage_location_id) || default_storage_location_id.nil? - errors.add(:default_storage_location_id, "The default storage location is not a storage location for this partner's organization") - end - end - def correct_document_mime_type if documents.attached? && documents.any? { |doc| !doc.content_type.in?(ALLOWED_MIME_TYPES) } errors.add(:documents, "Must be a PDF or DOC file") diff --git a/app/models/storage_location.rb b/app/models/storage_location.rb index 450002fa57..822d8ec605 100644 --- a/app/models/storage_location.rb +++ b/app/models/storage_location.rb @@ -124,7 +124,7 @@ def self.import_csv(csv, organization) loc.organization_id = organization loc.save! end - [] + [[], []] end # NOTE: We should generalize this elsewhere -- Importable concern? diff --git a/spec/models/donation_site_spec.rb b/spec/models/donation_site_spec.rb index 1f1a246b1b..ce79a60ab0 100644 --- a/spec/models/donation_site_spec.rb +++ b/spec/models/donation_site_spec.rb @@ -43,7 +43,7 @@ data = File.read(duplicated_name_csv_path, encoding: "BOM|UTF-8") csv = CSV.parse(data, headers: true) - errors = DonationSite.import_csv(csv, organization.id) + errors, _ = DonationSite.import_csv(csv, organization.id) expect(errors).not_to be_empty expect(errors.first).to match(/Row/) expect(errors.first).to include("Name must be unique within the organization") @@ -55,7 +55,7 @@ data = File.read(valid_csv_path, encoding: "BOM|UTF-8") csv = CSV.parse(data, headers: true) - errors = DonationSite.import_csv(csv, organization.id) + errors, _ = DonationSite.import_csv(csv, organization.id) expect(errors).to be_empty expect(DonationSite.count).to eq 1 @@ -67,7 +67,7 @@ data = File.read(invalid_csv_path, encoding: "BOM|UTF-8") csv = CSV.parse(data, headers: true) - errors = DonationSite.import_csv(csv, organization.id) + errors, _ = DonationSite.import_csv(csv, organization.id) expect(errors).not_to be_empty expect(errors.first).to match(/Row/) expect(errors.first).to include("can't be blank") diff --git a/spec/requests/partners_requests_spec.rb b/spec/requests/partners_requests_spec.rb index 4708dbc6f7..c8db79784e 100644 --- a/spec/requests/partners_requests_spec.rb +++ b/spec/requests/partners_requests_spec.rb @@ -613,7 +613,8 @@ 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(flash[:alert]).to be_present + expect(flash[:alert]).to match(/The following Partners imported with warnings/) end end From 08f85c08fd5f0ec1541f460ee23db7d6a755f85a Mon Sep 17 00:00:00 2001 From: McEileen Date: Sat, 7 Jun 2025 13:58:03 -0400 Subject: [PATCH 4/8] make warnings display as errors when partner import hits both errors and warnings --- app/models/partner.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/partner.rb b/app/models/partner.rb index 26fb7ba0ad..1a4407699d 100644 --- a/app/models/partner.rb +++ b/app/models/partner.rb @@ -114,7 +114,7 @@ def self.import_csv(csv, organization_id) if svc.errors.present? && svc.partner.errors.blank? warnings << "#{svc.partner.name}: #{svc.errors.full_messages.to_sentence}" elsif svc.errors.present? - errors << "#{svc.partner.name}: #{svc.partner.errors.full_messages.to_sentence}" + errors << "#{svc.partner.name}: #{svc.errors.full_messages.to_sentence}" end end [errors, warnings] From 58445692590fe9c196dd9bea85a4ad027cfb1367 Mon Sep 17 00:00:00 2001 From: jonkaplan Date: Wed, 17 Sep 2025 11:30:33 -0400 Subject: [PATCH 5/8] Refactor warnings to be part of the mixin - Fix specs associated with CSV import --- app/controllers/concerns/importable.rb | 14 +++--- app/models/concerns/provideable.rb | 3 +- app/models/donation_site.rb | 5 +- app/models/partner.rb | 22 ++++++--- app/models/storage_location.rb | 3 +- app/services/partner_create_service.rb | 35 +++++++++----- app/services/service_object_errors_mixin.rb | 9 +++- ...ault_storage_location_field_and_header.csv | 1 - spec/models/donation_site_spec.rb | 13 +++-- spec/models/partner_spec.rb | 48 +++++++++++++------ spec/requests/partners_requests_spec.rb | 1 - 11 files changed, 104 insertions(+), 50 deletions(-) diff --git a/app/controllers/concerns/importable.rb b/app/controllers/concerns/importable.rb index ada17747cc..204423abf6 100644 --- a/app/controllers/concerns/importable.rb +++ b/app/controllers/concerns/importable.rb @@ -27,13 +27,15 @@ def import_csv data = File.read(params[:file].path, encoding: "BOM|UTF-8") csv = CSV.parse(data, headers: true, skip_blanks: true) if csv.count.positive? && csv.first.headers.all? { |header| !header.nil? } - errors, warnings = resource_model.import_csv(csv, current_organization.id) - if errors.empty? && warnings.empty? + results = resource_model.import_csv(csv, current_organization.id) + if results[:errors].empty? flash[:notice] = "#{resource_model_humanized} were imported successfully!" - elsif errors.present? - flash[:error] = "The following #{resource_model_humanized} did not import successfully:\n#{errors.join("\n")}" - elsif warnings.present? - flash[:alert] = "The following #{resource_model_humanized} imported with warnings:\n#{warnings.join("\n")}" + else + flash[:error] = "The following #{resource_model_humanized} did not import successfully:\n#{results[:errors].join("\n")}" + end + + if results[:warnings].present? + flash[:alert] = "The following #{resource_model_humanized} imported with warnings:\n#{results[:warnings].join("\n")}" end else flash[:error] = "Check headers in file!" diff --git a/app/models/concerns/provideable.rb b/app/models/concerns/provideable.rb index e7be7b19c0..5105c8b770 100644 --- a/app/models/concerns/provideable.rb +++ b/app/models/concerns/provideable.rb @@ -15,7 +15,8 @@ def self.import_csv(csv, organization) loc.save! end - [[], []] + + {errors: [], warnings: []} end def self.csv_export_headers diff --git a/app/models/donation_site.rb b/app/models/donation_site.rb index c06eac27f7..83ab6382be 100644 --- a/app/models/donation_site.rb +++ b/app/models/donation_site.rb @@ -38,7 +38,7 @@ class DonationSite < ApplicationRecord def self.import_csv(csv, organization) errors = [] - warnings = [] + csv.each_with_index do |row, index| loc = DonationSite.new(row.to_hash) loc.organization_id = organization @@ -48,7 +48,8 @@ def self.import_csv(csv, organization) errors << "Row #{index + 2}, #{row.to_hash["name"]} - #{loc.errors.full_messages.join(", ")}" end end - [errors, warnings] + + {errors: errors, warnings: []} end def self.csv_export_headers diff --git a/app/models/partner.rb b/app/models/partner.rb index 1a4407699d..f3f0663b8c 100644 --- a/app/models/partner.rb +++ b/app/models/partner.rb @@ -64,6 +64,7 @@ class Partner < ApplicationRecord include Filterable include Exportable + scope :by_status, ->(status) { where(status: status.to_sym) } @@ -109,15 +110,22 @@ def self.import_csv(csv, organization_id) csv.each do |row| hash_rows = Hash[row.to_hash.map { |k, v| [k.downcase, v] }] - svc = PartnerCreateService.new(organization: organization, partner_attrs: hash_rows) - svc.call - if svc.errors.present? && svc.partner.errors.blank? - warnings << "#{svc.partner.name}: #{svc.errors.full_messages.to_sentence}" - elsif svc.errors.present? - errors << "#{svc.partner.name}: #{svc.errors.full_messages.to_sentence}" + partner_create_service = PartnerCreateService.new(organization: organization, partner_attrs: hash_rows) + partner_create_service.call + if partner_create_service.partner.errors.present? || partner_create_service.errors.present? + # Show ALL errors - both partner validation and service errors + all_errors = [] + row_errors = partner_create_service.errors + all_errors.concat(partner_create_service.partner.errors.full_messages) if partner_create_service.partner.errors.present? + all_errors.concat(row_errors.map(&:message)) if partner_create_service.errors.present? + errors << "#{partner_create_service.partner.name}: #{all_errors.to_sentence}" + elsif partner_create_service.warnings.present? + row_warnings = partner_create_service.warnings + warnings << "#{partner_create_service.partner.name}: #{row_warnings.map(&:message).to_sentence}" end end - [errors, warnings] + + {errors: errors, warnings: warnings} end def partials_to_show diff --git a/app/models/storage_location.rb b/app/models/storage_location.rb index 822d8ec605..62ba6fd5c5 100644 --- a/app/models/storage_location.rb +++ b/app/models/storage_location.rb @@ -124,7 +124,8 @@ def self.import_csv(csv, organization) loc.organization_id = organization loc.save! end - [[], []] + + {errors: [], warnings: []} end # NOTE: We should generalize this elsewhere -- Importable concern? diff --git a/app/services/partner_create_service.rb b/app/services/partner_create_service.rb index af40104eef..80c63c94db 100644 --- a/app/services/partner_create_service.rb +++ b/app/services/partner_create_service.rb @@ -9,17 +9,8 @@ def initialize(organization:, partner_attrs:) end def call - 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 + process_default_storage_location + @partner = organization.partners.build(partner_attrs) if @partner.valid? @@ -48,5 +39,27 @@ def call private + def process_default_storage_location + return unless partner_attrs.has_key?("default_storage_location") + + if partner_attrs["default_storage_location"].blank? + partner_attrs.delete("default_storage_location") + else + 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? + add_warning(: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 + end + attr_reader :organization, :partner_attrs end diff --git a/app/services/service_object_errors_mixin.rb b/app/services/service_object_errors_mixin.rb index 07338a62ee..714e2450db 100644 --- a/app/services/service_object_errors_mixin.rb +++ b/app/services/service_object_errors_mixin.rb @@ -31,6 +31,14 @@ def errors @errors ||= ActiveModel::Errors.new(self) end + def warnings + @warnings ||= ActiveModel::Errors.new(self) + end + + def add_warning(attribute, message) + warnings.add(attribute, message) + end + def read_attribute_for_validation(attr) send(attr) end @@ -45,4 +53,3 @@ def lookup_ancestors end end end - diff --git a/spec/fixtures/files/partners_missing_default_storage_location_field_and_header.csv b/spec/fixtures/files/partners_missing_default_storage_location_field_and_header.csv index 9cd070cac5..986cd8d6c0 100644 --- a/spec/fixtures/files/partners_missing_default_storage_location_field_and_header.csv +++ b/spec/fixtures/files/partners_missing_default_storage_location_field_and_header.csv @@ -1,3 +1,2 @@ name,email,send_reminders,quota,notes Partner 71,partner71@example.com,false,50,"great partner" - diff --git a/spec/models/donation_site_spec.rb b/spec/models/donation_site_spec.rb index ce79a60ab0..25047cc758 100644 --- a/spec/models/donation_site_spec.rb +++ b/spec/models/donation_site_spec.rb @@ -43,7 +43,8 @@ data = File.read(duplicated_name_csv_path, encoding: "BOM|UTF-8") csv = CSV.parse(data, headers: true) - errors, _ = DonationSite.import_csv(csv, organization.id) + response = DonationSite.import_csv(csv, organization.id) + errors = response[:errors] expect(errors).not_to be_empty expect(errors.first).to match(/Row/) expect(errors.first).to include("Name must be unique within the organization") @@ -55,7 +56,8 @@ data = File.read(valid_csv_path, encoding: "BOM|UTF-8") csv = CSV.parse(data, headers: true) - errors, _ = DonationSite.import_csv(csv, organization.id) + response = DonationSite.import_csv(csv, organization.id) + errors = response[:errors] expect(errors).to be_empty expect(DonationSite.count).to eq 1 @@ -67,7 +69,8 @@ data = File.read(invalid_csv_path, encoding: "BOM|UTF-8") csv = CSV.parse(data, headers: true) - errors, _ = DonationSite.import_csv(csv, organization.id) + response = DonationSite.import_csv(csv, organization.id) + errors = response[:errors] expect(errors).not_to be_empty expect(errors.first).to match(/Row/) expect(errors.first).to include("can't be blank") @@ -79,7 +82,9 @@ import_file_path = Rails.root.join("spec", "fixtures", "files", "donation_sites.csv") data = File.read(import_file_path, encoding: "BOM|UTF-8") csv = CSV.parse(data, headers: true) - DonationSite.import_csv(csv, organization.id) + response = DonationSite.import_csv(csv, organization.id) + errors = response[:errors] + expect(errors).to be_empty expect(DonationSite.count).to eq 1 end end diff --git a/spec/models/partner_spec.rb b/spec/models/partner_spec.rb index fa395e1582..ee627cab44 100644 --- a/spec/models/partner_spec.rb +++ b/spec/models/partner_spec.rb @@ -262,20 +262,23 @@ create(:storage_location, organization: organization) create(:storage_location, organization: organization, name: "shed") create(:storage_location, organization: organization, name: "building") + create(:storage_location, organization: organization, name: "Smithsonian Conservation Center") end it "imports partners from a csv file and prevents multiple imports" do - before_import = Partner.count import_file_path = Rails.root.join("spec", "fixtures", "files", "partners_with_six_fields.csv") data = File.read(import_file_path, encoding: "BOM|UTF-8") csv = CSV.parse(data, headers: true) - Partner.import_csv(csv, organization.id) - expect(Partner.count).to eq before_import + 3 + expect do + Partner.import_csv(csv, organization.id) + end.to change { Partner.count }.by(3) + import_file_path2 = Rails.root.join("spec", "fixtures", "files", "partners_with_six_fields_and_duplicates.csv") data2 = File.read(import_file_path2, encoding: "BOM|UTF-8") csv2 = CSV.parse(data2, headers: true) - Partner.import_csv(csv2, organization.id) - expect(Partner.count).to eq before_import + 4 + expect do + Partner.import_csv(csv2, organization.id) + end.to change { Partner.count }.by(1) end it "imports partners from a csv file with BOM encodings" do @@ -287,27 +290,42 @@ end.to change { Partner.count }.by(20) end - it "imports partners that have six fields" do - import_file_path = Rails.root.join("spec", "fixtures", "files", "partners_with_six_fields.csv") - data = File.read(import_file_path, encoding: "BOM|UTF-8") - csv = CSV.parse(data, headers: true) - expect do - Partner.import_csv(csv, organization.id) - end.to change { Partner.count }.by(3) - end - it "imports partners with the correct values for fields" do import_file_path = Rails.root.join("spec", "fixtures", "files", "partners_with_six_fields.csv") data = File.read(import_file_path, encoding: "BOM|UTF-8") csv = CSV.parse(data, headers: true) - Partner.import_csv(csv, organization.id) + response = Partner.import_csv(csv, organization.id) + expect(response[:errors]).to be_empty + expect(response[:warnings]).to be_empty partner = Partner.last expect(StorageLocation.find(partner.default_storage_location_id).name).to eq("Smithsonian Conservation Center") expect(partner.send_reminders).to eq(false) expect(partner.quota).to eq(80) expect(partner.notes).to eq("really ten out of ten") end + + it "imports partners with all caps storage location" do + import_file_path = Rails.root.join("spec", "fixtures", "files", "partners_with_six_fields_capitalized_location.csv") + data = File.read(import_file_path, encoding: "BOM|UTF-8") + csv = CSV.parse(data, headers: true) + expect do + response = Partner.import_csv(csv, organization.id) + expect(response[:warnings]).to be_empty + end.to change { Partner.count }.by(3) + end + + context "when storage location is not found" do + it "returns a warning" do + import_file_path = Rails.root.join("spec", "fixtures", "files", "partners_with_six_fields_invalid_location.csv") + data = File.read(import_file_path, encoding: "BOM|UTF-8") + csv = CSV.parse(data, headers: true) + response = Partner.import_csv(csv, organization.id) + expect(response[:errors]).to be_empty + expect(response[:warnings]).to be_present + expect(response[:warnings].join).to include("The default storage location is not a storage location for this partner's organization") + end + end end describe '#quantity_year_to_date' do diff --git a/spec/requests/partners_requests_spec.rb b/spec/requests/partners_requests_spec.rb index c8db79784e..9f333eb423 100644 --- a/spec/requests/partners_requests_spec.rb +++ b/spec/requests/partners_requests_spec.rb @@ -603,7 +603,6 @@ context "csv file with an invalid storage location" do let!(:current_organization) { create(:organization) } let!(:outside_organization) { create(:organization) } - let!(:outside_storage_location) { create(:storage_location, name: "Invalid", organization: outside_organization) } let(:file) { fixture_file_upload("partners_with_six_fields_invalid_location.csv", "text/csv") } before do allow(controller).to receive(:current_organization).and_return(current_organization) From eb5c111572c201143a6fff63509b8e4c16828152 Mon Sep 17 00:00:00 2001 From: jonkaplan Date: Wed, 17 Sep 2025 11:49:42 -0400 Subject: [PATCH 6/8] Fix error display --- app/models/partner.rb | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/app/models/partner.rb b/app/models/partner.rb index f3f0663b8c..3bcf64307f 100644 --- a/app/models/partner.rb +++ b/app/models/partner.rb @@ -112,16 +112,18 @@ def self.import_csv(csv, organization_id) partner_create_service = PartnerCreateService.new(organization: organization, partner_attrs: hash_rows) partner_create_service.call - if partner_create_service.partner.errors.present? || partner_create_service.errors.present? - # Show ALL errors - both partner validation and service errors - all_errors = [] - row_errors = partner_create_service.errors - all_errors.concat(partner_create_service.partner.errors.full_messages) if partner_create_service.partner.errors.present? - all_errors.concat(row_errors.map(&:message)) if partner_create_service.errors.present? - errors << "#{partner_create_service.partner.name}: #{all_errors.to_sentence}" + + if partner_create_service.errors.present? + formatted_errors = partner_create_service.errors.map do |error| + "#{error.attribute.to_s.humanize} #{error.message.downcase}" + end + errors << "#{partner_create_service.partner.name}: #{formatted_errors.to_sentence}" + elsif partner_create_service.warnings.present? - row_warnings = partner_create_service.warnings - warnings << "#{partner_create_service.partner.name}: #{row_warnings.map(&:message).to_sentence}" + formatted_warnings = partner_create_service.warnings.map do |warning| + "#{warning.attribute.to_s.humanize} #{warning.message.downcase}" + end + warnings << "#{partner_create_service.partner.name}: #{formatted_warnings.to_sentence}" end end From e91044bbe8936c249d811792dee9c46f2b1a29e1 Mon Sep 17 00:00:00 2001 From: jonkaplan Date: Wed, 17 Sep 2025 11:53:34 -0400 Subject: [PATCH 7/8] Fix warning text --- app/services/partner_create_service.rb | 2 +- spec/models/partner_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/partner_create_service.rb b/app/services/partner_create_service.rb index 80c63c94db..bfc2b29b8c 100644 --- a/app/services/partner_create_service.rb +++ b/app/services/partner_create_service.rb @@ -53,7 +53,7 @@ def process_default_storage_location if default_storage_location_id.nil? add_warning(:default_storage_location, - "The default storage location is not a storage location for this partner's organization") + "is not a storage location for this partner's organization") end partner_attrs.delete("default_storage_location") diff --git a/spec/models/partner_spec.rb b/spec/models/partner_spec.rb index ee627cab44..d19accd69f 100644 --- a/spec/models/partner_spec.rb +++ b/spec/models/partner_spec.rb @@ -323,7 +323,7 @@ response = Partner.import_csv(csv, organization.id) expect(response[:errors]).to be_empty expect(response[:warnings]).to be_present - expect(response[:warnings].join).to include("The default storage location is not a storage location for this partner's organization") + expect(response[:warnings].join).to include("Default storage location is not a storage location for this partner's organization") end end end From 42824d4fb49a0a662fe841a6f3e0249dd7e6a667 Mon Sep 17 00:00:00 2001 From: jonkaplan Date: Mon, 6 Oct 2025 09:00:30 -0700 Subject: [PATCH 8/8] Remove skip blanks for consistency --- app/controllers/concerns/importable.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/concerns/importable.rb b/app/controllers/concerns/importable.rb index 204423abf6..67e5644584 100644 --- a/app/controllers/concerns/importable.rb +++ b/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, skip_blanks: true) + csv = CSV.parse(data, headers: true) if csv.count.positive? && csv.first.headers.all? { |header| !header.nil? } results = resource_model.import_csv(csv, current_organization.id) if results[:errors].empty?