Allow Organization and Partner Profile edit when associated records h…

…ave validation errors (#4737)

* Validate Partner email during update too (not just creation)

* Update wording for partner profile's pick-up email validations

Starts with lowercase, no . at end

* Display errors in Partner when updating Partner Profile instead of failing silently

* Display errors in Partner Profile when updating Organization instead of showing 500

app/models/partner.rb

@@ -48,8 +48,7 @@ class Partner < ApplicationRecord
   validates :organization, presence: true
   validates :name, presence: true, uniqueness: { scope: :organization }
 
-  validates :email, presence: true, uniqueness: { case_sensitive: false },
-    format: { with: URI::MailTo::EMAIL_REGEXP, on: :create }
+  validates :email, presence: true, uniqueness: { case_sensitive: false }, format: { with: URI::MailTo::EMAIL_REGEXP }
 
   validates :quota, numericality: { greater_than_or_equal_to: 0 }, allow_blank: true
 

app/models/partners/profile.rb

@@ -158,14 +158,14 @@ def pick_up_email_addresses
 
       emails = split_pick_up_emails
       if emails.size > 3
-        errors.add(:pick_up_email, "There can't be more than three pick up email addresses.")
+        errors.add(:pick_up_email, "can't have more than three email addresses")
         nil
       end
       if emails.uniq.size != emails.size
-        errors.add(:pick_up_email, "There should not be repeated email addresses.")
+        errors.add(:pick_up_email, "should not have repeated email addresses")
       end
       emails.each do |e|
-        errors.add(:pick_up_email, "Invalid email address for '#{e}'.") unless e.match? URI::MailTo::EMAIL_REGEXP
+        errors.add(:pick_up_email, "is invalid") unless e.match? URI::MailTo::EMAIL_REGEXP
       end
     end
   end

app/services/organization_update_service.rb

@@ -30,7 +30,7 @@ def update(organization, params)
       result = organization.update(org_params)
 
       return false unless result
-      update_partner_flags(organization)
+      return false unless update_partner_flags(organization)
       true
     end
 
@@ -46,6 +46,9 @@ def update_partner_flags(organization)
         next if organization.send(field)
         organization.partners.each do |partner|
           partner.profile.update!(field => organization.send(field))
+        rescue ActiveRecord::RecordInvalid => e
+          organization.errors.add(:base, "Profile for partner '#{e.record.partner.name}' had error(s) preventing the organization from being saved. #{e.message}")
+          return false
         end
       end
     end

app/services/partner_profile_update_service.rb

@@ -18,6 +18,8 @@ def call
         @profile.served_areas.destroy_all
         @profile.attributes = @profile_params
         @profile.save!(context: :edit)
+      else
+        @error = "Partner '#{@partner.name}' had error(s) preventing the profile from being updated: #{@partner.errors.full_messages.join(", ")}"
       end
     end
   end

spec/services/organization_update_service_spec.rb

@@ -88,6 +88,52 @@
           end
         end
       end
+
+      context "when partner is invalid" do
+        let(:params) { {name: "New organization"} }
+
+        before do
+          organization.update!(enable_individual_requests: false)
+          # Want to have an invalid email on purpose
+          # rubocop:disable Rails::SkipsModelValidations
+          partner_one.update_columns(email: "not/an_email")
+          # rubocop:enable Rails::SkipsModelValidations
+        end
+
+        it "updates the organization and returns true" do
+          expect(described_class.update(organization, params)).to eq(true)
+          expect(organization.name).to eq("New organization")
+        end
+      end
+
+      context "when partner profile is invalid" do
+        let(:profile) { partner_one.profile }
+        let(:params) { {name: "New organization"} }
+
+        before do
+          organization.update!(enable_individual_requests: false)
+          # Want to have an invalid email on purpose
+          # rubocop:disable Rails::SkipsModelValidations
+          profile.update_columns(pick_up_email: "not/an/email")
+          # rubocop:enable Rails::SkipsModelValidations
+        end
+
+        it "returns false" do
+          expect(described_class.update(organization, params)).to eq(false)
+        end
+
+        it "adds an error message to the organization" do
+          described_class.update(organization, params)
+          expect(organization.errors.full_messages).to include(
+            "Profile for partner '#{partner_one.name}' had error(s) preventing the organization from being saved. Validation failed: Pick up email is invalid"
+          )
+        end
+
+        it "updates the organization regardless" do # because updating the organization takes place before updating partner profiles
+          described_class.update(organization, params)
+          expect(organization.name).to eq("New organization")
+        end
+      end
     end
   end
 

spec/services/partner_profile_update_service_spec.rb

@@ -43,6 +43,7 @@
         end
       end
     end
+
     context "when served area client shares pre-exist" do
       let!(:original_served_area_1) { create(:partners_served_area, partner_profile: profile, county: county_1, client_share: 51) }
       let!(:original_served_area_2) { create(:partners_served_area, partner_profile: profile, county: county_2, client_share: 49) }
@@ -93,5 +94,29 @@
         end
       end
     end
+
+    context "when the partner has invalid attributes" do
+      let(:partner) { profile.partner }
+
+      before do
+        # Want to have an invalid email on purpose
+        # rubocop:disable Rails::SkipsModelValidations
+        partner.update_columns(email: "not/an_email")
+        # rubocop:enable Rails::SkipsModelValidations
+      end
+
+      it "returns failure" do
+        result = PartnerProfileUpdateService.new(partner, partner_params, basic_correct_attributes).call
+        expect(result.success?).to eq(false)
+        expect(result.error.to_s).to include("Partner '#{partner.name}' had error(s) preventing the profile from being updated: Email is invalid")
+      end
+
+      it "doesn't update the partner profile" do
+        old_pick_up_email = profile.pick_up_email
+        valid_profile_params = {pick_up_email: "[email protected] "}
+        PartnerProfileUpdateService.new(profile.partner, partner_params, valid_profile_params).call
+        expect(profile.pick_up_email).to eq(old_pick_up_email)
+      end
+    end
   end
 end