From 6d9a6020f49f61738d49c4405d278803fcc944c6 Mon Sep 17 00:00:00 2001
From: Arthur Campbell <arfa.camble@gmail.com>
Date: Thu, 2 May 2024 17:11:18 +0100
Subject: [PATCH] update validation to reflect the fact that an org having no
 associated rent periods no longer means they accept all rent periods update
 tests adding both cases and removing unnecessary additional db additions

---
 .../validations/financial_validations.rb      |  5 ++--
 .../validations/financial_validations_spec.rb | 28 +++++++++++++------
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/app/models/validations/financial_validations.rb b/app/models/validations/financial_validations.rb
index e0e3f081e..5e7d92d2c 100644
--- a/app/models/validations/financial_validations.rb
+++ b/app/models/validations/financial_validations.rb
@@ -145,8 +145,9 @@ module Validations::FinancialValidations
   end
 
   def validate_rent_period(record)
-    if record.managing_organisation.present? && record.managing_organisation.rent_periods.present? &&
-        record.period && !record.managing_organisation.rent_periods.include?(record.period)
+    return unless record.managing_organisation && record.period
+
+    unless record.managing_organisation.rent_periods.include? record.period
       record.errors.add :period, :wrong_rent_period, message: I18n.t(
         "validations.financial.rent_period.invalid_for_org",
         org_name: record.managing_organisation.name,
diff --git a/spec/models/validations/financial_validations_spec.rb b/spec/models/validations/financial_validations_spec.rb
index aa2e2e96a..acd8ae388 100644
--- a/spec/models/validations/financial_validations_spec.rb
+++ b/spec/models/validations/financial_validations_spec.rb
@@ -139,23 +139,33 @@ RSpec.describe Validations::FinancialValidations do
   end
 
   describe "rent period validations" do
-    let(:organisation) { FactoryBot.create(:organisation) }
-    let(:user) { FactoryBot.create(:user) }
-    let(:record) { FactoryBot.create(:lettings_log, owning_organisation: user.organisation, managing_organisation: organisation, assigned_to: user) }
+    let(:user) { create(:user) }
+    let(:record) { create(:lettings_log, owning_organisation: user.organisation, managing_organisation: user.organisation, assigned_to: user) }
+    let(:used_period) { 2 }
 
     before do
-      FactoryBot.create(:organisation_relationship, parent_organisation: user.organisation, child_organisation: organisation)
-      FactoryBot.create(:organisation_rent_period, organisation:, rent_period: 2)
+      create(:organisation_rent_period, organisation: user.organisation, rent_period: used_period)
+      record.period = period
     end
 
-    context "when the organisation only uses specific rent periods" do
-      it "validates that the selected rent period is used by the managing organisation" do
-        record.period = 3
+    context "when the log uses a period that the org allows" do
+      let(:period) { used_period }
+
+      it "does not apply a validation" do
+        financial_validator.validate_rent_period(record)
+        expect(record.errors["period"]).to be_empty
+      end
+    end
+
+    context "when the log uses a period that the org does not allow" do
+      let(:period) { used_period + 1 }
+
+      it "does apply a validation" do
         financial_validator.validate_rent_period(record)
         expect(record.errors["period"])
           .to include(match I18n.t(
             "validations.financial.rent_period.invalid_for_org",
-            org_name: organisation.name,
+            org_name: user.organisation.name,
             rent_period: "every 4 weeks",
           ))
       end