From d743faa79d4d83bd4825bfc17e738267951bb02f Mon Sep 17 00:00:00 2001
From: Arthur Campbell <arfa.camble@gmail.com>
Date: Tue, 30 Apr 2024 13:59:32 +0100
Subject: [PATCH] changes following review

---
 app/controllers/organisations_controller.rb | 14 +++++++++-----
 app/helpers/organisations_helper.rb         |  8 ++------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb
index 5689d6ab6..5450cca8d 100644
--- a/app/controllers/organisations_controller.rb
+++ b/app/controllers/organisations_controller.rb
@@ -79,7 +79,7 @@ class OrganisationsController < ApplicationController
   end
 
   def create
-    selected_rent_periods = params.require(:organisation).permit(rent_periods: [])[:rent_periods].compact_blank
+    selected_rent_periods = rent_period_params
     @organisation = Organisation.new(org_params)
     if @organisation.save
       OrganisationRentPeriod.transaction do
@@ -95,8 +95,8 @@ class OrganisationsController < ApplicationController
 
   def edit
     if current_user.data_coordinator? || current_user.support?
-      checked_periods = @organisation.organisation_rent_periods.map(&:rent_period).map(&:to_s)
-      @rent_periods = helpers.rent_periods_with_checked_attr(checked_periods:)
+      current_allowed_rent_periods = @organisation.organisation_rent_periods.pluck(:rent_period).map(&:to_s)
+      @rent_periods = helpers.rent_periods_with_checked_attr(checked_periods: current_allowed_rent_periods)
       render "edit", layout: "application"
     else
       head :unauthorized
@@ -116,7 +116,7 @@ class OrganisationsController < ApplicationController
   end
 
   def update
-    selected_rent_periods = params.require(:organisation).permit(rent_periods: [])[:rent_periods].compact_blank
+    selected_rent_periods = rent_period_params
     if (current_user.data_coordinator? && org_params[:active].nil?) || current_user.support?
       if @organisation.update(org_params)
         case org_params[:active]
@@ -135,7 +135,7 @@ class OrganisationsController < ApplicationController
         else
           flash[:notice] = I18n.t("organisation.updated")
         end
-        existing_rent_periods = @organisation.organisation_rent_periods.map(&:rent_period)
+        existing_rent_periods = @organisation.organisation_rent_periods.pluck(:rent_period)
         rent_periods_to_create = selected_rent_periods.map(&:to_i) - existing_rent_periods
         rent_periods_to_delete = existing_rent_periods - selected_rent_periods.map(&:to_i)
         OrganisationRentPeriod.transaction do
@@ -293,6 +293,10 @@ private
     params.require(:organisation).permit(:name, :address_line1, :address_line2, :postcode, :phone, :holds_own_stock, :provider_type, :housing_registration_no, :active)
   end
 
+  def rent_period_params
+    params.require(:organisation).permit(rent_periods: [])[:rent_periods].compact_blank
+  end
+
   def codes_only_export?
     params.require(:codes_only) == "true"
   end
diff --git a/app/helpers/organisations_helper.rb b/app/helpers/organisations_helper.rb
index adbf5ac6e..1293d552f 100644
--- a/app/helpers/organisations_helper.rb
+++ b/app/helpers/organisations_helper.rb
@@ -40,12 +40,8 @@ module OrganisationsHelper
   end
 
   def rent_periods_with_checked_attr(checked_periods: nil)
-    all_rent_periods = RentPeriod.rent_period_mappings
-    rent_periods = all_rent_periods.map do |code, period|
-      period_copy = period.clone
-      period_copy[:checked] = true if checked_periods.nil? || checked_periods.include?(code)
-      [code, period_copy]
+    RentPeriod.rent_period_mappings.each_with_object({}) do |(period_code, period_value), result|
+      result[period_code] = period_value.merge(checked: checked_periods.nil? || checked_periods.include?(period_code))
     end
-    rent_periods.to_h
   end
 end