From 7ddf3fa8e36ba44ccfbae8fa0f5df2b545871c06 Mon Sep 17 00:00:00 2001
From: JP <85654561+jp524@users.noreply.github.com>
Date: Sat, 5 Oct 2024 13:34:28 -0500
Subject: [PATCH 1/7] Remove duplicate validation errors form
 PartnerCreateService

---
 app/services/partner_create_service.rb | 32 +++++++++++++-------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/app/services/partner_create_service.rb b/app/services/partner_create_service.rb
index 0028155e32..6b81bfae59 100644
--- a/app/services/partner_create_service.rb
+++ b/app/services/partner_create_service.rb
@@ -11,27 +11,27 @@ def initialize(organization:, partner_attrs:)
   def call
     @partner = organization.partners.build(partner_attrs)
 
-    unless @partner.valid?
+    if @partner.valid?
+      ActiveRecord::Base.transaction do
+        @partner.save!
+
+        Partners::Profile.create!({
+                                    partner_id: @partner.id,
+                                    name: @partner.name,
+                                    enable_child_based_requests: organization.enable_child_based_requests,
+                                    enable_individual_requests: organization.enable_individual_requests,
+                                    enable_quantity_based_requests: organization.enable_quantity_based_requests
+                                  })
+      rescue StandardError => e
+        errors.add(:base, e.message)
+        raise ActiveRecord::Rollback
+      end
+    else
       @partner.errors.each do |error|
         errors.add(error.attribute, error.message)
       end
     end
 
-    ActiveRecord::Base.transaction do
-      @partner.save!
-
-      Partners::Profile.create!({
-                                  partner_id: @partner.id,
-                                  name: @partner.name,
-                                  enable_child_based_requests: organization.enable_child_based_requests,
-                                  enable_individual_requests: organization.enable_individual_requests,
-                                  enable_quantity_based_requests: organization.enable_quantity_based_requests
-                                })
-    rescue StandardError => e
-      errors.add(:base, e.message)
-      raise ActiveRecord::Rollback
-    end
-
     self
   end
 

From 5fada83f8e2807ba722c047abce61ff36b36bf83 Mon Sep 17 00:00:00 2001
From: JP <85654561+jp524@users.noreply.github.com>
Date: Sat, 5 Oct 2024 15:46:42 -0500
Subject: [PATCH 2/7] Allow Importable to display error message based on each
 model's import_csv implementation

---
 app/controllers/concerns/importable.rb | 8 ++++++--
 app/models/concerns/provideable.rb     | 1 +
 app/models/donation_site.rb            | 1 +
 app/models/partner.rb                  | 6 +++++-
 app/models/storage_location.rb         | 1 +
 5 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/app/controllers/concerns/importable.rb b/app/controllers/concerns/importable.rb
index 90152f41e4..45aa1bb630 100644
--- a/app/controllers/concerns/importable.rb
+++ b/app/controllers/concerns/importable.rb
@@ -27,8 +27,12 @@ def import_csv
       data = File.read(params[:file].path, encoding: "BOM|UTF-8")
       csv = CSV.parse(data, headers: true).reject { |row| row.to_hash.values.any?(&:nil?) }
       if csv.count.positive? && csv.first.headers.all? { |header| !header.nil? }
-        resource_model.import_csv(csv, current_organization.id)
-        flash[:notice] = "#{resource_model_humanized} were imported successfully!"
+        errors = resource_model.import_csv(csv, current_organization.id)
+        flash[:notice] = if errors.nil?
+          "#{resource_model_humanized} were imported successfully!"
+        else
+          "The following #{resource_model_humanized} did not import successfully:\n#{errors.join("\n")}"
+        end
       else
         flash[:error] = "Check headers in file!"
       end
diff --git a/app/models/concerns/provideable.rb b/app/models/concerns/provideable.rb
index 478a3869bb..5879684443 100644
--- a/app/models/concerns/provideable.rb
+++ b/app/models/concerns/provideable.rb
@@ -22,6 +22,7 @@ def self.import_csv(csv, organization)
 
         loc.save!
       end
+      nil
     end
 
     def self.csv_export_headers
diff --git a/app/models/donation_site.rb b/app/models/donation_site.rb
index 95195b5d1a..10eedfe7cc 100644
--- a/app/models/donation_site.rb
+++ b/app/models/donation_site.rb
@@ -46,6 +46,7 @@ def self.import_csv(csv, organization)
       loc.organization_id = organization
       loc.save!
     end
+    nil
   end
 
   def self.csv_export_headers
diff --git a/app/models/partner.rb b/app/models/partner.rb
index 9ac073d186..3b67835bdd 100644
--- a/app/models/partner.rb
+++ b/app/models/partner.rb
@@ -157,7 +157,7 @@ def approvable?
   end
 
   # better to extract this outside of the model
-  def self.import_csv(csv, organization_id)
+  def self.import_csv(csv, organization_id, errors = [])
     organization = Organization.find(organization_id)
 
     csv.each do |row|
@@ -165,7 +165,11 @@ def self.import_csv(csv, organization_id)
 
       svc = PartnerCreateService.new(organization: organization, partner_attrs: hash_rows)
       svc.call
+      if svc.errors.present?
+        errors << "#{svc.partner.name}: #{svc.partner.errors.full_messages.to_sentence}"
+      end
     end
+    errors.empty? ? nil : errors
   end
 
   def self.csv_export_headers
diff --git a/app/models/storage_location.rb b/app/models/storage_location.rb
index 53fcdc3d25..e8ab0f097f 100644
--- a/app/models/storage_location.rb
+++ b/app/models/storage_location.rb
@@ -121,6 +121,7 @@ def self.import_csv(csv, organization)
       loc.organization_id = organization
       loc.save!
     end
+    nil
   end
 
   # NOTE: We should generalize this elsewhere -- Importable concern?

From 5b4d22f3ee65e42339b810be1078efff7c4d0fe5 Mon Sep 17 00:00:00 2001
From: JP <85654561+jp524@users.noreply.github.com>
Date: Sat, 5 Oct 2024 16:03:24 -0500
Subject: [PATCH 3/7] Add request tests

---
 .../files/partners_with_invalid_email.csv     |  4 ++++
 spec/requests/partners_requests_spec.rb       | 20 +++++++++++++++++++
 2 files changed, 24 insertions(+)
 create mode 100644 spec/fixtures/files/partners_with_invalid_email.csv

diff --git a/spec/fixtures/files/partners_with_invalid_email.csv b/spec/fixtures/files/partners_with_invalid_email.csv
new file mode 100644
index 0000000000..e78f35d9dc
--- /dev/null
+++ b/spec/fixtures/files/partners_with_invalid_email.csv
@@ -0,0 +1,4 @@
+name,email
+Partner 1,partner1@example.com
+Partner 2,this is not an email address
+Partner 3,partner3@example.com
\ No newline at end of file
diff --git a/spec/requests/partners_requests_spec.rb b/spec/requests/partners_requests_spec.rb
index 4acd3cb63a..8064b5cc83 100644
--- a/spec/requests/partners_requests_spec.rb
+++ b/spec/requests/partners_requests_spec.rb
@@ -222,6 +222,26 @@
         expect(response).to have_error "Check headers in file!"
       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 } }
+
+      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 displaying the import errors" do
+        subject
+        expect(response).to have_notice(/The following #{model_class.name.underscore.humanize.pluralize} did not import successfully:/)
+        expect(response).to have_notice(/Partner 2: Email is invalid/)
+      end
+    end
   end
 
   describe "POST #create" do

From 5cb0ec9a659957260ff14ff7a83edabe5b632e33 Mon Sep 17 00:00:00 2001
From: JP <85654561+jp524@users.noreply.github.com>
Date: Sun, 6 Oct 2024 16:15:54 -0500
Subject: [PATCH 4/7] Display import errors with correct flash styling

---
 app/controllers/concerns/importable.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/controllers/concerns/importable.rb b/app/controllers/concerns/importable.rb
index 45aa1bb630..ec2664c391 100644
--- a/app/controllers/concerns/importable.rb
+++ b/app/controllers/concerns/importable.rb
@@ -28,10 +28,10 @@ def import_csv
       csv = CSV.parse(data, headers: true).reject { |row| row.to_hash.values.any?(&:nil?) }
       if csv.count.positive? && csv.first.headers.all? { |header| !header.nil? }
         errors = resource_model.import_csv(csv, current_organization.id)
-        flash[:notice] = if errors.nil?
-          "#{resource_model_humanized} were imported successfully!"
+        if errors.nil?
+          flash[:notice] = "#{resource_model_humanized} were imported successfully!"
         else
-          "The following #{resource_model_humanized} did not import successfully:\n#{errors.join("\n")}"
+          flash[:error] = "The following #{resource_model_humanized} did not import successfully:\n#{errors.join("\n")}"
         end
       else
         flash[:error] = "Check headers in file!"

From 851673c648e1e1f93f10cc91b252a0d38a10ae68 Mon Sep 17 00:00:00 2001
From: JP <85654561+jp524@users.noreply.github.com>
Date: Sun, 6 Oct 2024 16:23:24 -0500
Subject: [PATCH 5/7] Update test

---
 spec/requests/partners_requests_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/requests/partners_requests_spec.rb b/spec/requests/partners_requests_spec.rb
index 8064b5cc83..9084b6251d 100644
--- a/spec/requests/partners_requests_spec.rb
+++ b/spec/requests/partners_requests_spec.rb
@@ -238,8 +238,8 @@
 
       it "presents a flash notice message displaying the import errors" do
         subject
-        expect(response).to have_notice(/The following #{model_class.name.underscore.humanize.pluralize} did not import successfully:/)
-        expect(response).to have_notice(/Partner 2: Email is invalid/)
+        expect(response).to have_error(/The following #{model_class.name.underscore.humanize.pluralize} did not import successfully:/)
+        expect(response).to have_error(/Partner 2: Email is invalid/)
       end
     end
   end

From 83fa5819cfee8e70ba7b4f2b9feb4a1744292bdd Mon Sep 17 00:00:00 2001
From: JP <85654561+jp524@users.noreply.github.com>
Date: Mon, 14 Oct 2024 17:28:34 -0500
Subject: [PATCH 6/7] Update logic for 'import_csv' methods

---
 app/controllers/concerns/importable.rb | 2 +-
 app/models/concerns/provideable.rb     | 4 ++--
 app/models/donation_site.rb            | 4 ++--
 app/models/partner.rb                  | 2 +-
 app/models/storage_location.rb         | 4 ++--
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/app/controllers/concerns/importable.rb b/app/controllers/concerns/importable.rb
index ec2664c391..488614efac 100644
--- a/app/controllers/concerns/importable.rb
+++ b/app/controllers/concerns/importable.rb
@@ -28,7 +28,7 @@ def import_csv
       csv = CSV.parse(data, headers: true).reject { |row| row.to_hash.values.any?(&:nil?) }
       if csv.count.positive? && csv.first.headers.all? { |header| !header.nil? }
         errors = resource_model.import_csv(csv, current_organization.id)
-        if errors.nil?
+        if errors.empty?
           flash[:notice] = "#{resource_model_humanized} were imported successfully!"
         else
           flash[:error] = "The following #{resource_model_humanized} did not import successfully:\n#{errors.join("\n")}"
diff --git a/app/models/concerns/provideable.rb b/app/models/concerns/provideable.rb
index 5879684443..da2b1732e6 100644
--- a/app/models/concerns/provideable.rb
+++ b/app/models/concerns/provideable.rb
@@ -15,14 +15,14 @@ module Provideable
       where(organization: organization).order(:business_name)
     }
 
-    def self.import_csv(csv, organization)
+    def self.import_csv(csv, organization, errors = [])
       csv.each do |row|
         loc = new(row.to_hash)
         loc.organization_id = organization
 
         loc.save!
       end
-      nil
+      errors
     end
 
     def self.csv_export_headers
diff --git a/app/models/donation_site.rb b/app/models/donation_site.rb
index 10eedfe7cc..7a787c1b89 100644
--- a/app/models/donation_site.rb
+++ b/app/models/donation_site.rb
@@ -40,13 +40,13 @@ class DonationSite < ApplicationRecord
 
   scope :alphabetized, -> { order(:name) }
 
-  def self.import_csv(csv, organization)
+  def self.import_csv(csv, organization, errors = [])
     csv.each do |row|
       loc = DonationSite.new(row.to_hash)
       loc.organization_id = organization
       loc.save!
     end
-    nil
+    errors
   end
 
   def self.csv_export_headers
diff --git a/app/models/partner.rb b/app/models/partner.rb
index 3b67835bdd..32eaf598da 100644
--- a/app/models/partner.rb
+++ b/app/models/partner.rb
@@ -169,7 +169,7 @@ def self.import_csv(csv, organization_id, errors = [])
         errors << "#{svc.partner.name}: #{svc.partner.errors.full_messages.to_sentence}"
       end
     end
-    errors.empty? ? nil : errors
+    errors
   end
 
   def self.csv_export_headers
diff --git a/app/models/storage_location.rb b/app/models/storage_location.rb
index e8ab0f097f..153912ef7a 100644
--- a/app/models/storage_location.rb
+++ b/app/models/storage_location.rb
@@ -115,13 +115,13 @@ def to_csv
   end
 
   # NOTE: We should generalize this elsewhere -- Importable concern?
-  def self.import_csv(csv, organization)
+  def self.import_csv(csv, organization, errors = [])
     csv.each do |row|
       loc = StorageLocation.new(row.to_hash)
       loc.organization_id = organization
       loc.save!
     end
-    nil
+    errors
   end
 
   # NOTE: We should generalize this elsewhere -- Importable concern?

From 88cc1fafb174b5632b3fcce3697efe33cf63acdb Mon Sep 17 00:00:00 2001
From: JP <85654561+jp524@users.noreply.github.com>
Date: Wed, 16 Oct 2024 17:38:35 -0500
Subject: [PATCH 7/7] Refactor code

---
 app/models/concerns/provideable.rb | 4 ++--
 app/models/donation_site.rb        | 4 ++--
 app/models/partner.rb              | 3 ++-
 app/models/storage_location.rb     | 4 ++--
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/app/models/concerns/provideable.rb b/app/models/concerns/provideable.rb
index 484d975822..56a64620c1 100644
--- a/app/models/concerns/provideable.rb
+++ b/app/models/concerns/provideable.rb
@@ -12,14 +12,14 @@ module Provideable
       where(organization: organization).order(:business_name)
     }
 
-    def self.import_csv(csv, organization, errors = [])
+    def self.import_csv(csv, organization)
       csv.each do |row|
         loc = new(row.to_hash)
         loc.organization_id = organization
 
         loc.save!
       end
-      errors
+      []
     end
 
     def self.csv_export_headers
diff --git a/app/models/donation_site.rb b/app/models/donation_site.rb
index 7a787c1b89..99ddd71d97 100644
--- a/app/models/donation_site.rb
+++ b/app/models/donation_site.rb
@@ -40,13 +40,13 @@ class DonationSite < ApplicationRecord
 
   scope :alphabetized, -> { order(:name) }
 
-  def self.import_csv(csv, organization, errors = [])
+  def self.import_csv(csv, organization)
     csv.each do |row|
       loc = DonationSite.new(row.to_hash)
       loc.organization_id = organization
       loc.save!
     end
-    errors
+    []
   end
 
   def self.csv_export_headers
diff --git a/app/models/partner.rb b/app/models/partner.rb
index 32eaf598da..4d7175e1dd 100644
--- a/app/models/partner.rb
+++ b/app/models/partner.rb
@@ -157,7 +157,8 @@ def approvable?
   end
 
   # better to extract this outside of the model
-  def self.import_csv(csv, organization_id, errors = [])
+  def self.import_csv(csv, organization_id)
+    errors = []
     organization = Organization.find(organization_id)
 
     csv.each do |row|
diff --git a/app/models/storage_location.rb b/app/models/storage_location.rb
index 153912ef7a..3c7f7a2f5f 100644
--- a/app/models/storage_location.rb
+++ b/app/models/storage_location.rb
@@ -115,13 +115,13 @@ def to_csv
   end
 
   # NOTE: We should generalize this elsewhere -- Importable concern?
-  def self.import_csv(csv, organization, errors = [])
+  def self.import_csv(csv, organization)
     csv.each do |row|
       loc = StorageLocation.new(row.to_hash)
       loc.organization_id = organization
       loc.save!
     end
-    errors
+    []
   end
 
   # NOTE: We should generalize this elsewhere -- Importable concern?