From e6cb76d85b9c809761ed892b94c12d21b82a1c37 Mon Sep 17 00:00:00 2001 From: Arthur Campbell Date: Wed, 24 Apr 2024 14:26:35 +0100 Subject: [PATCH] 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 | 7 +- spec/models/organisation_spec.rb | 66 ++++++++++++++----- .../requests/organisations_controller_spec.rb | 33 ++++------ 3 files changed, 64 insertions(+), 42 deletions(-) diff --git a/app/models/organisation.rb b/app/models/organisation.rb index 133407be8..0bd69f847 100644 --- a/app/models/organisation.rb +++ b/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") } - labels.compact.presence || %w[All] + rent_period_ids = rent_periods + 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? diff --git a/spec/models/organisation_spec.rb b/spec/models/organisation_spec.rb index 5714e80b3..71359e251 100644 --- a/spec/models/organisation_spec.rb +++ b/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 - let(:rent_period_mappings) do - { "2" => { "value" => "Weekly for 52 weeks" }, "3" => { "value" => "Every 2 weeks" } } + context "with associated rent periods" do + let(:period_1_label) { "Every minute" } + 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: 3) - - # Unmapped and ignored by `rent_period_labels` - create(:organisation_rent_period, organisation:, rent_period: 10) - allow(RentPeriod).to receive(:rent_period_mappings).and_return(rent_period_mappings) + create(:organisation_rent_period, organisation:, rent_period: 1) + allow(RentPeriod).to receive(:rent_period_mappings).and_return(fake_rent_periods) end - it "has rent periods associated" do - expect(organisation.rent_periods).to eq([2, 3, 10]) - 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 "maps the rent periods to display values" do - expect(organisation.rent_period_labels).to eq(["Weekly for 52 weeks", "Every 2 weeks"]) + it "#rent_period_labels returns the correct labels" do + expect(organisation.rent_period_labels).to eq [period_1_label] + end + end end - end - context "when the organisation has not specified which rent periods it uses" do - it "displays `all`" do - expect(organisation.rent_period_labels).to eq(%w[All]) + context "when the org uses all rent periods" do + before do + create(:organisation_rent_period, organisation:, rent_period: 2) + end + + it "#rent_periods returns the correct ids" do + expect(organisation.rent_periods).to eq [1, 2] + end + + it "#rent_period_labels returns All" do + 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 + + it "#rent_period_labels returns All" do + expect(organisation.rent_period_labels).to eq %w[All] + end + end end end diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index 00d8cb125..9b83df529 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/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 - { - id: organisation.id, - organisation: { active: "true" }, - } - end + let(:active) { true } 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 - { - id: organisation.id, - organisation: { active: "false" }, - } - end + let(:active) { false } 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