From c051bf69f38cc4485533893ea05e271443376841 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Tue, 10 Oct 2023 12:07:09 +0100 Subject: [PATCH 1/4] Extract options_for_organisation_select helper We want to improve these options by sorting alphabetically and excluding closed organisations. First let's extract a helper method and add some tests for the existing behaviour. --- app/helpers/role_organisations_helper.rb | 9 ++++++ .../account/role_organisations/show.html.erb | 2 +- .../helpers/role_organisations_helper_test.rb | 30 +++++++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 app/helpers/role_organisations_helper.rb create mode 100644 test/helpers/role_organisations_helper_test.rb diff --git a/app/helpers/role_organisations_helper.rb b/app/helpers/role_organisations_helper.rb new file mode 100644 index 000000000..a12543df0 --- /dev/null +++ b/app/helpers/role_organisations_helper.rb @@ -0,0 +1,9 @@ +module RoleOrganisationsHelper + def options_for_your_organisation_select(current_user) + Pundit.policy_scope(current_user, Organisation).map do |organisation| + { text: organisation.name_with_abbreviation, + value: organisation.id, + selected: current_user.organisation == organisation } + end + end +end diff --git a/app/views/account/role_organisations/show.html.erb b/app/views/account/role_organisations/show.html.erb index 1edede628..e2d3db4c0 100644 --- a/app/views/account/role_organisations/show.html.erb +++ b/app/views/account/role_organisations/show.html.erb @@ -60,7 +60,7 @@ id: "user_organisation_id", name: "user[organisation_id]", label: "Organisation", - options: policy_scope(Organisation).map { |organisation| { text: organisation.name_with_abbreviation, value: organisation.id, selected: current_user.organisation == organisation } } + options: options_for_your_organisation_select(current_user) } %> <%= render "govuk_publishing_components/components/button", { text: "Change organisation" diff --git a/test/helpers/role_organisations_helper_test.rb b/test/helpers/role_organisations_helper_test.rb new file mode 100644 index 000000000..e2f558414 --- /dev/null +++ b/test/helpers/role_organisations_helper_test.rb @@ -0,0 +1,30 @@ +require "test_helper" + +class RoleOrganisationsHelperTest < ActionView::TestCase + context "#options_for_your_organisation_select" do + setup do + @user_organisation = create(:organisation, name: "User Organisation", abbreviation: "UO") + @other_organisation = create(:organisation, name: "Other Organisation") + @user = create(:admin_user, organisation: @user_organisation) + end + + should "return options suitable for select component with users organisation selected" do + options = options_for_your_organisation_select(@user) + + expected_options = [ + { + text: "Other Organisation", + value: @other_organisation.id, + selected: false, + }, + { + text: "User Organisation – UO", + value: @user_organisation.id, + selected: true, + }, + ] + + assert_equal expected_options, options + end + end +end From e37e5d97bead00fc18d3df86d96689765362c375 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Tue, 10 Oct 2023 12:14:24 +0100 Subject: [PATCH 2/4] Sort organisation options alphabetically by name This hopefully makes it easier for users to find the organisation they are looking for in the (long) list of options. --- app/helpers/role_organisations_helper.rb | 4 +++- test/helpers/role_organisations_helper_test.rb | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/helpers/role_organisations_helper.rb b/app/helpers/role_organisations_helper.rb index a12543df0..cd1ce94fe 100644 --- a/app/helpers/role_organisations_helper.rb +++ b/app/helpers/role_organisations_helper.rb @@ -1,6 +1,8 @@ module RoleOrganisationsHelper def options_for_your_organisation_select(current_user) - Pundit.policy_scope(current_user, Organisation).map do |organisation| + organisations = Pundit.policy_scope(current_user, Organisation).order(:name) + + organisations.map do |organisation| { text: organisation.name_with_abbreviation, value: organisation.id, selected: current_user.organisation == organisation } diff --git a/test/helpers/role_organisations_helper_test.rb b/test/helpers/role_organisations_helper_test.rb index e2f558414..31edf4f6b 100644 --- a/test/helpers/role_organisations_helper_test.rb +++ b/test/helpers/role_organisations_helper_test.rb @@ -26,5 +26,11 @@ class RoleOrganisationsHelperTest < ActionView::TestCase assert_equal expected_options, options end + + should "sort by organisation name alphabetically" do + options = options_for_your_organisation_select(@user) + + assert_equal ["Other Organisation", "User Organisation – UO"], (options.map { |o| o[:text] }) + end end end From e7dd6c50e18ff37e964c88c7574099c7d2c5a4e0 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Tue, 10 Oct 2023 12:18:51 +0100 Subject: [PATCH 3/4] Exclude closed organisations from the organisation select We don't want to assign users to closed organisations so it doesn't make sense to include them in the select list. --- app/helpers/role_organisations_helper.rb | 2 +- test/helpers/role_organisations_helper_test.rb | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/helpers/role_organisations_helper.rb b/app/helpers/role_organisations_helper.rb index cd1ce94fe..a14314405 100644 --- a/app/helpers/role_organisations_helper.rb +++ b/app/helpers/role_organisations_helper.rb @@ -1,6 +1,6 @@ module RoleOrganisationsHelper def options_for_your_organisation_select(current_user) - organisations = Pundit.policy_scope(current_user, Organisation).order(:name) + organisations = Pundit.policy_scope(current_user, Organisation).where(closed: false).order(:name) organisations.map do |organisation| { text: organisation.name_with_abbreviation, diff --git a/test/helpers/role_organisations_helper_test.rb b/test/helpers/role_organisations_helper_test.rb index 31edf4f6b..3a95bfe9a 100644 --- a/test/helpers/role_organisations_helper_test.rb +++ b/test/helpers/role_organisations_helper_test.rb @@ -5,6 +5,7 @@ class RoleOrganisationsHelperTest < ActionView::TestCase setup do @user_organisation = create(:organisation, name: "User Organisation", abbreviation: "UO") @other_organisation = create(:organisation, name: "Other Organisation") + @closed_organisation = create(:organisation, name: "Closed Organisation", closed: true) @user = create(:admin_user, organisation: @user_organisation) end @@ -32,5 +33,11 @@ class RoleOrganisationsHelperTest < ActionView::TestCase assert_equal ["Other Organisation", "User Organisation – UO"], (options.map { |o| o[:text] }) end + + should "not include closed organisations" do + closed_organisation_option = options_for_your_organisation_select(@user).detect { |o| o[:value] == @closed_organisation.id } + + assert_not closed_organisation_option + end end end From 924e00c89fe97a175a1bb19733589a4806f30d95 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Tue, 10 Oct 2023 15:43:10 +0100 Subject: [PATCH 4/4] Extract Organisation#not_closed scope --- app/helpers/role_organisations_helper.rb | 2 +- app/models/organisation.rb | 2 ++ test/models/organisation_test.rb | 6 ++++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/helpers/role_organisations_helper.rb b/app/helpers/role_organisations_helper.rb index a14314405..9e875914d 100644 --- a/app/helpers/role_organisations_helper.rb +++ b/app/helpers/role_organisations_helper.rb @@ -1,6 +1,6 @@ module RoleOrganisationsHelper def options_for_your_organisation_select(current_user) - organisations = Pundit.policy_scope(current_user, Organisation).where(closed: false).order(:name) + organisations = Pundit.policy_scope(current_user, Organisation).not_closed.order(:name) organisations.map do |organisation| { text: organisation.name_with_abbreviation, diff --git a/app/models/organisation.rb b/app/models/organisation.rb index b5d3e155d..0d56b6177 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -12,6 +12,8 @@ class Organisation < ApplicationRecord before_save :strip_whitespace_from_name + scope :not_closed, -> { where(closed: false) } + def name_with_abbreviation return_value = if abbreviation.present? && abbreviation != name "#{name} – #{abbreviation}" diff --git a/test/models/organisation_test.rb b/test/models/organisation_test.rb index 25ec7ae2d..4d96fdb53 100644 --- a/test/models/organisation_test.rb +++ b/test/models/organisation_test.rb @@ -23,6 +23,12 @@ def setup assert_equal "An organisation", organisation.name end + test "#not_closed" do + create(:organisation, closed: true) + + assert_equal [@organisation], Organisation.not_closed.to_a + end + context "displaying name with abbreviation" do should "use abbreviation when it is not the same as name" do organisation = build(:organisation, name: "An Organisation", abbreviation: "ABBR")