rework the #rent_period_labels method to return All under the correct conditions, rework tests related to that. + fix assorted tests that were either flakey or breaking due to addition of rent periods logic to create and update

app/models/organisation.rb

@@ -108,8 +108,11 @@ class Organisation < ApplicationRecord
   end
 
   def rent_period_labels
-    labels = rent_periods.map { |period| RentPeriod.rent_period_mappings.dig(period.to_s, "value") }
+    rent_period_ids = rent_periods
-    labels.compact.presence || %w[All]
+    mappings = RentPeriod.rent_period_mappings
+    return %w[All] if (mappings.keys.map(&:to_i) - rent_period_ids).empty?
+
+    rent_period_ids.map { |id| mappings.dig(id.to_s, "value") }.compact
   end
 
   def data_protection_confirmed?

spec/models/organisation_spec.rb

@@ -108,32 +108,62 @@ RSpec.describe Organisation, type: :model do
       end
     end
 
-    context "when the organisation only uses specific rent periods" do
+    context "with associated rent periods" do
-      let(:rent_period_mappings) do
+      let(:period_1_label) { "Every minute" }
-        { "2" => { "value" => "Weekly for 52 weeks" }, "3" => { "value" => "Every 2 weeks" } }
+      let(:fake_rent_periods) do
+        {
+          "1" => { "value" => period_1_label },
+          "2" => { "value" => "Every decade" },
+        }
       end
 
       before do
-        create(:organisation_rent_period, organisation:, rent_period: 2)
+        create(:organisation_rent_period, organisation:, rent_period: 1)
+        allow(RentPeriod).to receive(:rent_period_mappings).and_return(fake_rent_periods)
+      end
+
+      context "when the org does not use all rent periods" do
+        it "#rent_periods returns the correct ids" do
+          expect(organisation.rent_periods).to eq [1]
+        end
+
+        it "#rent_period_labels returns the correct labels" do
+          expect(organisation.rent_period_labels).to eq [period_1_label]
+        end
+
+        context "and has organisation rent periods associated for rent periods that no longer appear in the form" do
+          before do
             create(:organisation_rent_period, organisation:, rent_period: 3)
+          end
+
+          it "#rent_period_labels returns the correct labels" do
+            expect(organisation.rent_period_labels).to eq [period_1_label]
+          end
+        end
+      end
 
-        # Unmapped and ignored by `rent_period_labels`
+      context "when the org uses all rent periods" do
-        create(:organisation_rent_period, organisation:, rent_period: 10)
+        before do
-        allow(RentPeriod).to receive(:rent_period_mappings).and_return(rent_period_mappings)
+          create(:organisation_rent_period, organisation:, rent_period: 2)
         end
 
-      it "has rent periods associated" do
+        it "#rent_periods returns the correct ids" do
-        expect(organisation.rent_periods).to eq([2, 3, 10])
+          expect(organisation.rent_periods).to eq [1, 2]
         end
 
-      it "maps the rent periods to display values" do
+        it "#rent_period_labels returns All" do
-        expect(organisation.rent_period_labels).to eq(["Weekly for 52 weeks", "Every 2 weeks"])
+          expect(organisation.rent_period_labels).to eq %w[All]
         end
+
+        context "and has organisation rent periods associated for rent periods that no longer appear in the form" do
+          before do
+            create(:organisation_rent_period, organisation:, rent_period: 3)
           end
 
-    context "when the organisation has not specified which rent periods it uses" do
+          it "#rent_period_labels returns All" do
-      it "displays `all`" do
+            expect(organisation.rent_period_labels).to eq %w[All]
-        expect(organisation.rent_period_labels).to eq(%w[All])
+          end
+        end
       end
     end
 

spec/requests/organisations_controller_spec.rb

@@ -7,7 +7,8 @@ RSpec.describe OrganisationsController, type: :request do
   let(:page) { Capybara::Node::Simple.new(response.body) }
   let(:user) { create(:user, :data_coordinator) }
   let(:new_value) { "Test Name 35" }
-  let(:params) { { id: organisation.id, organisation: { name: new_value } } }
+  let(:active) { nil }
+  let(:params) { { id: organisation.id, organisation: { name: new_value, active:, rent_periods: [] } } }
 
   before do
     Timecop.freeze(Time.zone.local(2024, 3, 1))
@@ -581,12 +582,7 @@ RSpec.describe OrganisationsController, type: :request do
           end
 
           context "with active parameter true" do
-            let(:params) do
+            let(:active) { true }
-              {
-                id: organisation.id,
-                organisation: { active: "true" },
-              }
-            end
 
             it "redirects" do
               expect(response).to have_http_status(:unauthorized)
@@ -594,12 +590,7 @@ RSpec.describe OrganisationsController, type: :request do
           end
 
           context "with active parameter false" do
-            let(:params) do
+            let(:active) { false }
-              {
-                id: organisation.id,
-                organisation: { active: "false" },
-              }
-            end
 
             it "redirects" do
               expect(response).to have_http_status(:unauthorized)
@@ -705,6 +696,7 @@ RSpec.describe OrganisationsController, type: :request do
               provider_type: "LA",
               holds_own_stock: "true",
               housing_registration_no: "7917937",
+              rent_periods: [],
             },
           }
         end
@@ -1329,7 +1321,7 @@ RSpec.describe OrganisationsController, type: :request do
           end
 
           it "allows to edit the organisation details" do
-            expect(page).to have_link("Change", count: 3)
+            expect(page).to have_link("Change")
           end
         end
 
@@ -1434,8 +1426,10 @@ RSpec.describe OrganisationsController, type: :request do
       end
 
       describe "#update" do
+        let(:params) { { id: organisation.id, organisation: { active:, rent_periods: [] } } }
+
         context "with active parameter false" do
-          let(:params) { { id: organisation.id, organisation: { active: "false" } } }
+          let(:active) { false }
 
           user_to_update = nil
 
@@ -1455,15 +1449,9 @@ RSpec.describe OrganisationsController, type: :request do
           user_to_reactivate = nil
           user_not_to_reactivate = nil
 
-          let(:params) do
-            {
-              id: organisation.id,
-              organisation: { active: "true" },
-            }
-          end
           let(:notify_client) { instance_double(Notifications::Client) }
           let(:devise_notify_mailer) { DeviseNotifyMailer.new }
-
+          let(:active) { true }
           let(:expected_personalisation) do
             {
               name: user_to_reactivate.name,
@@ -1547,6 +1535,7 @@ RSpec.describe OrganisationsController, type: :request do
               provider_type:,
               holds_own_stock:,
               housing_registration_no:,
+              rent_periods: [],
             },
           }
         end