From bf5d8442ce56502a0bdcf1656ef18c81788a6c31 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Fri, 19 May 2023 08:52:28 +0100 Subject: [PATCH] CLDC-1731 Allow data coordinators to view parent's schemes (#1637) * Update permissions and scheme/location views * Display parent organisation's schemes in the list * Hide toggle scheme button from child organisation * Update show location to use user_can_edit_scheme? * Refactor user_allowed_action? * remove redundant return --- app/controllers/locations_controller.rb | 11 +++-- app/controllers/organisations_controller.rb | 2 +- app/controllers/schemes_controller.rb | 6 ++- app/helpers/locations_helper.rb | 4 ++ app/views/locations/index.html.erb | 8 ++- app/views/locations/show.html.erb | 4 +- app/views/schemes/show.html.erb | 4 +- spec/requests/locations_controller_spec.rb | 54 +++++++++++++++++++-- spec/requests/schemes_controller_spec.rb | 54 +++++++++++++++++++-- 9 files changed, 129 insertions(+), 18 deletions(-) diff --git a/app/controllers/locations_controller.rb b/app/controllers/locations_controller.rb index ffb2a9c9a..f978a3b10 100644 --- a/app/controllers/locations_controller.rb +++ b/app/controllers/locations_controller.rb @@ -4,7 +4,7 @@ class LocationsController < ApplicationController before_action :authenticate_scope! before_action :find_location, except: %i[create index] before_action :find_scheme - before_action :authenticate_action! + before_action :authenticate_action!, only: %i[create update index new_deactivation deactivate_confirm deactivate postcode local_authority name units type_of_unit mobility_standards availability check_answers] before_action :scheme_and_location_present, except: %i[create index] include Modules::SearchFilter @@ -21,6 +21,7 @@ class LocationsController < ApplicationController end def postcode; end + def update; end def update_postcode @location.postcode = location_params[:postcode] @@ -225,11 +226,15 @@ private end def authenticate_action! - if %w[create update index new_deactivation deactivate_confirm deactivate postcode local_authority name units type_of_unit mobility_standards availability check_answers].include?(action_name) && !((current_user.organisation == @scheme&.owning_organisation) || current_user.support?) - render_not_found and return + unless user_allowed_action? + render_not_found end end + def user_allowed_action? + current_user.support? || current_user.organisation == @scheme&.owning_organisation || current_user.organisation.parent_organisations.exists?(@scheme&.owning_organisation_id) + end + def location_params required_params = params.require(:location).permit(:postcode, :location_admin_district, :location_code, :name, :units, :type_of_unit, :mobility_type, "startdate(1i)", "startdate(2i)", "startdate(3i)").merge(scheme_id: @scheme.id) required_params[:postcode] = PostcodeService.clean(required_params[:postcode]) if required_params[:postcode] diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index b430136ae..db31fb12a 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -19,7 +19,7 @@ class OrganisationsController < ApplicationController end def schemes - all_schemes = Scheme.where(owning_organisation: @organisation).order_by_completion.order_by_service_name + all_schemes = Scheme.where(owning_organisation: [@organisation] + @organisation.parent_organisations).order_by_completion.order_by_service_name @pagy, @schemes = pagy(filtered_collection(all_schemes, search_term)) @searched = search_term.presence diff --git a/app/controllers/schemes_controller.rb b/app/controllers/schemes_controller.rb index e60620bc5..ad7884819 100644 --- a/app/controllers/schemes_controller.rb +++ b/app/controllers/schemes_controller.rb @@ -265,11 +265,15 @@ private def authenticate_scope! head :unauthorized and return unless current_user.data_coordinator? || current_user.support? - if %w[show locations primary_client_group confirm_secondary_client_group secondary_client_group support details check_answers edit_name deactivate].include?(action_name) && !((current_user.organisation == @scheme&.owning_organisation) || current_user.support?) + if %w[show locations primary_client_group confirm_secondary_client_group secondary_client_group support details check_answers edit_name deactivate].include?(action_name) && !user_allowed_action? render_not_found and return end end + def user_allowed_action? + current_user.support? || current_user.organisation == @scheme&.owning_organisation || current_user.organisation.parent_organisations.exists?(@scheme&.owning_organisation_id) + end + def redirect_if_scheme_confirmed redirect_to @scheme if @scheme.confirmed? end diff --git a/app/helpers/locations_helper.rb b/app/helpers/locations_helper.rb index 5bf7bc8ff..7e38ed36c 100644 --- a/app/helpers/locations_helper.rb +++ b/app/helpers/locations_helper.rb @@ -84,6 +84,10 @@ module LocationsHelper end end + def user_can_edit_scheme?(user, scheme) + user.support? || user.organisation == scheme.owning_organisation + end + private ActivePeriod = Struct.new(:from, :to) diff --git a/app/views/locations/index.html.erb b/app/views/locations/index.html.erb index 7294b9729..fafe16a92 100644 --- a/app/views/locations/index.html.erb +++ b/app/views/locations/index.html.erb @@ -64,7 +64,9 @@ <% end %> <% end %> <% end %> - <%= govuk_button_to "Add a location", scheme_locations_path(@scheme), method: "post", secondary: true %> + <% if user_can_edit_scheme?(current_user, @scheme) %> + <%= govuk_button_to "Add a location", scheme_locations_path(@scheme), method: "post", secondary: true %> + <% end %> @@ -118,7 +120,9 @@ <% end %> <% end %> <% end %> - <%= govuk_button_to "Add a location", scheme_locations_path(@scheme), method: "post", secondary: true %> + <% if user_can_edit_scheme?(current_user, @scheme) %> + <%= govuk_button_to "Add a location", scheme_locations_path(@scheme), method: "post", secondary: true %> + <% end %> <% end %> diff --git a/app/views/locations/show.html.erb b/app/views/locations/show.html.erb index 299a19357..005d68422 100644 --- a/app/views/locations/show.html.erb +++ b/app/views/locations/show.html.erb @@ -16,12 +16,12 @@ <%= summary_list.row do |row| %> <% row.key { attr[:name] } %> <% row.value { attr[:attribute].eql?("status") ? status_tag(attr[:value]) : details_html(attr) } %> - <% row.action(text: "Change", href: scheme_location_name_path(@scheme, @location, referrer: "details")) if attr[:attribute] == "name" %> + <% row.action(text: "Change", href: scheme_location_name_path(@scheme, @location, referrer: "details")) if attr[:attribute] == "name" && user_can_edit_scheme?(current_user, @scheme) %> <% end %> <% end %> <% end %> -<% if FeatureToggle.location_toggle_enabled? %> +<% if FeatureToggle.location_toggle_enabled? && user_can_edit_scheme?(current_user, @scheme) %> <%= toggle_location_link(@location) %> <% end %> diff --git a/app/views/schemes/show.html.erb b/app/views/schemes/show.html.erb index beb4508e0..96137b2bb 100644 --- a/app/views/schemes/show.html.erb +++ b/app/views/schemes/show.html.erb @@ -22,7 +22,7 @@ <%= summary_list.row do |row| %> <% row.key { attr[:name] } %> <% row.value { details_html(attr) } %> - <% row.action(text: "Change", href: scheme_edit_name_path(scheme_id: @scheme.id)) if attr[:edit] %> + <% row.action(text: "Change", href: scheme_edit_name_path(scheme_id: @scheme.id)) if attr[:edit] && user_can_edit_scheme?(current_user, @scheme) %> <% end %> <% end %> <% end %> @@ -32,6 +32,6 @@ <% end %> -<% if FeatureToggle.scheme_toggle_enabled? %> +<% if FeatureToggle.scheme_toggle_enabled? && user_can_edit_scheme?(current_user, @scheme) %> <%= toggle_scheme_link(@scheme) %> <% end %> diff --git a/spec/requests/locations_controller_spec.rb b/spec/requests/locations_controller_spec.rb index ab9dcd268..ef39e15ab 100644 --- a/spec/requests/locations_controller_spec.rb +++ b/spec/requests/locations_controller_spec.rb @@ -145,7 +145,7 @@ RSpec.describe LocationsController, type: :request do end end - it "shows locations with correct data wben the new locations layout feature toggle is enabled" do + it "shows locations with correct data when the new locations layout feature toggle is enabled" do locations.each do |location| expect(page).to have_content(location.id) expect(page).to have_content(location.postcode) @@ -154,7 +154,7 @@ RSpec.describe LocationsController, type: :request do end end - it "shows locations with correct data wben the new locations layout feature toggle is disabled" do + it "shows locations with correct data when the new locations layout feature toggle is disabled" do allow(FeatureToggle).to receive(:location_toggle_enabled?).and_return(false) get "/schemes/#{scheme.id}/locations" locations.each do |location| @@ -248,6 +248,30 @@ RSpec.describe LocationsController, type: :request do expect(page).to have_title(expected_title) end end + + context "when coordinator attempts to see scheme belonging to a parent organisation" do + let(:parent_organisation) { FactoryBot.create(:organisation) } + let!(:scheme) { FactoryBot.create(:scheme, owning_organisation: parent_organisation) } + let!(:locations) { FactoryBot.create_list(:location, 3, scheme:, startdate: Time.zone.local(2022, 4, 1)) } + + before do + create(:organisation_relationship, parent_organisation:, child_organisation: user.organisation) + get "/schemes/#{scheme.id}/locations" + end + + it "shows all the locations" do + locations.each do |location| + expect(page).to have_content(location.id) + expect(page).to have_content(location.postcode) + expect(page).to have_content(location.name) + expect(page).to have_content(location.status) + end + end + + it "does not allow adding new locations" do + expect(page).not_to have_button("Add a location") + end + end end context "when signed in as a support user" do @@ -261,16 +285,17 @@ RSpec.describe LocationsController, type: :request do get "/schemes/#{scheme.id}/locations" end - it "shows locations with correct data wben the new locations layout feature toggle is enabled" do + it "shows locations with correct data when the new locations layout feature toggle is enabled" do locations.each do |location| expect(page).to have_content(location.id) expect(page).to have_content(location.postcode) expect(page).to have_content(location.name) expect(page).to have_content(location.status) end + expect(page).to have_button("Add a location") end - it "shows locations with correct data wben the new locations layout feature toggle is disabled" do + it "shows locations with correct data when the new locations layout feature toggle is disabled" do allow(FeatureToggle).to receive(:location_toggle_enabled?).and_return(false) get "/schemes/#{scheme.id}/locations" locations.each do |location| @@ -1677,6 +1702,27 @@ RSpec.describe LocationsController, type: :request do expect(page).not_to have_link("Deactivate this location") end end + + context "and are viewing their parent organisation's location" do + let(:parent_organisation) { FactoryBot.create(:organisation) } + let!(:scheme) { FactoryBot.create(:scheme, owning_organisation: parent_organisation) } + let!(:location) { FactoryBot.create(:location, scheme:) } + let(:add_deactivations) {} + + before do + create(:organisation_relationship, parent_organisation:, child_organisation: user.organisation) + end + + it "shows the location" do + expect(page).to have_content("Location name") + expect(page).to have_content(location.name) + end + + it "does not allow editing the location" do + expect(page).not_to have_link("Change") + expect(page).not_to have_link("Deactivate this location", href: "/schemes/#{scheme.id}/locations/#{location.id}/new-deactivation") + end + end end end diff --git a/spec/requests/schemes_controller_spec.rb b/spec/requests/schemes_controller_spec.rb index 1a450a922..9aeaac6ca 100644 --- a/spec/requests/schemes_controller_spec.rb +++ b/spec/requests/schemes_controller_spec.rb @@ -39,6 +39,9 @@ RSpec.describe SchemesController, type: :request do let(:user) { FactoryBot.create(:user, :data_coordinator) } before do + schemes.each do |scheme| + scheme.update!(owning_organisation: user.organisation) + end sign_in user get "/schemes" end @@ -47,6 +50,33 @@ RSpec.describe SchemesController, type: :request do follow_redirect! expect(path).to match("/organisations/#{user.organisation.id}/schemes") end + + it "shows a list of schemes for the organisation" do + follow_redirect! + schemes.each do |scheme| + expect(page).to have_content(scheme.id_to_display) + end + end + + context "when parent organisation has schemes" do + let(:parent_organisation) { FactoryBot.create(:organisation) } + let!(:parent_schemes) { FactoryBot.create_list(:scheme, 5, owning_organisation: parent_organisation) } + + before do + create(:organisation_relationship, parent_organisation:, child_organisation: user.organisation) + parent_schemes.each do |scheme| + FactoryBot.create(:location, scheme:) + end + get "/schemes" + end + + it "shows parent organisation schemes" do + follow_redirect! + parent_schemes.each do |scheme| + expect(page).to have_content(scheme.id_to_display) + end + end + end end context "when signed in as a support user" do @@ -233,9 +263,6 @@ RSpec.describe SchemesController, type: :request do expect(page).to have_content(specific_scheme.id_to_display) expect(page).to have_content(specific_scheme.service_name) expect(page).to have_content(specific_scheme.sensitive) - expect(page).to have_content(specific_scheme.id_to_display) - expect(page).to have_content(specific_scheme.service_name) - expect(page).to have_content(specific_scheme.sensitive) expect(page).to have_content(specific_scheme.scheme_type) expect(page).to have_content(specific_scheme.registered_under_care_act) expect(page).to have_content(specific_scheme.primary_client_group) @@ -306,6 +333,27 @@ RSpec.describe SchemesController, type: :request do end end end + + context "when coordinator attempts to see scheme belonging to a parent organisation" do + let(:parent_organisation) { FactoryBot.create(:organisation) } + let!(:specific_scheme) { FactoryBot.create(:scheme, owning_organisation: parent_organisation) } + + before do + FactoryBot.create(:location, scheme: specific_scheme) + create(:organisation_relationship, parent_organisation:, child_organisation: user.organisation) + get "/schemes/#{specific_scheme.id}" + end + + it "shows the scheme" do + expect(page).to have_content(specific_scheme.id_to_display) + end + + it "does not allow editing the scheme" do + expect(page).not_to have_link("Change") + expect(page).not_to have_content("Reactivate this scheme") + expect(page).not_to have_content("Deactivate this scheme") + end + end end context "when signed in as a support user" do