diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index 0156f1f36..e7f5c4bef 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -111,19 +111,14 @@ class OrganisationsController < ApplicationController if @organisation.update(org_params) case org_params[:active] when "false" - @organisation.users.filter_by_active - .update_all( - active: false, - confirmed_at: nil, - sign_in_count: 0, - initial_confirmation_sent: false, - reactivate_with_organisation: true, - ) + @organisation.users.filter_by_active.each do |user| + user.deactivate!(reactivate_with_organisation: true) + end flash[:notice] = I18n.t("organisation.deactivated", organisation: @organisation.name) when "true" users_to_reactivate = @organisation.users.where(reactivate_with_organisation: true) users_to_reactivate.each do |user| - user.update!(active: true, reactivate_with_organisation: false) + user.reactivate! user.send_confirmation_instructions end flash[:notice] = I18n.t("organisation.reactivated", organisation: @organisation.name) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 6a76cb047..3fe3b1813 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -62,9 +62,10 @@ class UsersController < ApplicationController else user_name = @user.name&.possessive || @user.email.possessive if user_params[:active] == "false" - @user.update!(confirmed_at: nil, sign_in_count: 0, initial_confirmation_sent: false) + @user.deactivate! flash[:notice] = I18n.t("devise.activation.deactivated", user_name:) elsif user_params[:active] == "true" + @user.reactivate! @user.send_confirmation_instructions flash[:notice] = I18n.t("devise.activation.reactivated", user_name:) elsif user_params.key?("email") diff --git a/app/models/location.rb b/app/models/location.rb index 250529492..75fbe9d36 100644 --- a/app/models/location.rb +++ b/app/models/location.rb @@ -54,7 +54,8 @@ class Location < ApplicationRecord scope :deactivated, lambda { merge(LocationDeactivationPeriod.deactivations_without_reactivation) - .where("location_deactivation_periods.deactivation_date <= ?", Time.zone.now) + .where("location_deactivation_periods.deactivation_date <= ?", Time.zone.now) + .or("location.organisation.active", false) } scope :deactivating_soon, lambda { diff --git a/app/models/scheme.rb b/app/models/scheme.rb index a912250a9..1cd7ce575 100644 --- a/app/models/scheme.rb +++ b/app/models/scheme.rb @@ -52,7 +52,8 @@ class Scheme < ApplicationRecord scope :deactivated, lambda { merge(SchemeDeactivationPeriod.deactivations_without_reactivation) - .where("scheme_deactivation_periods.deactivation_date <= ?", Time.zone.now) + .where("scheme_deactivation_periods.deactivation_date <= ?", Time.zone.now) + .or("scheme.organisation.active", false) } scope :deactivating_soon, lambda { diff --git a/app/models/user.rb b/app/models/user.rb index 6d13e8cd1..27187529a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -134,6 +134,23 @@ class User < ApplicationRecord update!(is_dpo: true) end + def deactivate!(reactivate_with_organisation: false) + update!( + active: false, + confirmed_at: nil, + sign_in_count: 0, + initial_confirmation_sent: false, + reactivate_with_organisation:, + ) + end + + def reactivate! + update!( + active: true, + reactivate_with_organisation: false, + ) + end + MFA_TEMPLATE_ID = "6bdf5ee1-8e01-4be1-b1f9-747061d8a24c".freeze RESET_PASSWORD_TEMPLATE_ID = "2c410c19-80a7-481c-a531-2bcb3264f8e6".freeze CONFIRMABLE_TEMPLATE_ID = "3fc2e3a7-0835-4b84-ab7a-ce51629eb614".freeze diff --git a/app/policies/organisation_policy.rb b/app/policies/organisation_policy.rb index 4e0e2c1b0..3e0c9a946 100644 --- a/app/policies/organisation_policy.rb +++ b/app/policies/organisation_policy.rb @@ -6,12 +6,11 @@ class OrganisationPolicy @organisation = organisation end - %w[ - deactivate? - reactivate? - ].each do |method_name| - define_method method_name do - user.support? - end + def deactivate? + user.support? && organisation.status == :active + end + + def reactivate? + user.support? && organisation.status == :deactivated end end diff --git a/app/views/organisations/show.html.erb b/app/views/organisations/show.html.erb index 824922639..2dced52bd 100644 --- a/app/views/organisations/show.html.erb +++ b/app/views/organisations/show.html.erb @@ -41,14 +41,11 @@ -<% if @organisation.active? %> - <% if OrganisationPolicy.new(current_user, @organisation).deactivate? %> - <%= govuk_button_link_to "Deactivate this organisation", deactivate_organisation_path(@organisation), warning: true %> - <% end %> -<% else %> - <% if OrganisationPolicy.new(current_user, @organisation).reactivate? %> - +<% if OrganisationPolicy.new(current_user, @organisation).deactivate? %> + <%= govuk_button_link_to "Deactivate this organisation", deactivate_organisation_path(@organisation), warning: true %> +<% end %> +<% if OrganisationPolicy.new(current_user, @organisation).reactivate? %> + <%= govuk_button_link_to "Reactivate this organisation", reactivate_organisation_path(@organisation) %> - <% end %> <% end %> diff --git a/spec/policies/organisation_policy_spec.rb b/spec/policies/organisation_policy_spec.rb index 855d259ea..4fb4c6761 100644 --- a/spec/policies/organisation_policy_spec.rb +++ b/spec/policies/organisation_policy_spec.rb @@ -1,5 +1,4 @@ require "rails_helper" -# rubocop:disable RSpec/RepeatedExample RSpec.describe OrganisationPolicy do subject(:policy) { described_class } @@ -11,30 +10,57 @@ RSpec.describe OrganisationPolicy do permissions :deactivate? do it "does not permit data providers to deactivate an organisation" do + organisation.active = true expect(policy).not_to permit(data_provider, organisation) end it "does not permit data coordinators to deactivate an organisation" do - expect(policy).not_to permit(data_coordinator, data_provider) + organisation.active = true + expect(policy).not_to permit(data_coordinator, organisation) end - it "permits support users to deactivate an organisation" do - expect(policy).to permit(support, data_provider) + it "permits support users to deactivate an active organisation" do + organisation.active = true + expect(policy).to permit(support, organisation) + end + + it "does not permit support users to deactivate an inactive organisation" do + organisation.active = false + expect(policy).not_to permit(support, organisation) + end + + it "does not permit support users to deactivate a merged organisation" do + organisation.active = true + organisation.merge_date = Time.zone.local(2023, 8, 2) + expect(policy).not_to permit(support, organisation) end end permissions :reactivate? do it "does not permit data providers to reactivate an organisation" do + organisation.active = false expect(policy).not_to permit(data_provider, organisation) end it "does not permit data coordinators to reactivate an organisation" do - expect(policy).not_to permit(data_coordinator, data_provider) + organisation.active = false + expect(policy).not_to permit(data_coordinator, organisation) + end + + it "permits support users to reactivate an inactive organisation" do + organisation.active = false + expect(policy).to permit(support, organisation) + end + + it "does not permit support users to reactivate an active organisation" do + organisation.active = true + expect(policy).not_to permit(support, organisation) end - it "permits support users to reactivate an organisation" do - expect(policy).to permit(support, data_provider) + it "does not permit support users to reactivate a merged organisation" do + organisation.active = false + organisation.merge_date = Time.zone.local(2023, 8, 2) + expect(policy).not_to permit(support, organisation) end end end -# rubocop:enable RSpec/RepeatedExample diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index 0bb1c3bb6..fa5163003 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -1512,16 +1512,18 @@ RSpec.describe OrganisationsController, type: :request do end describe "#reactivate" do + let(:inactive_organisation) { create(:organisation, name: "Inactive org", active: false) } + before do - get "/organisations/#{organisation.id}/reactivate", headers:, params: {} + get "/organisations/#{inactive_organisation.id}/reactivate", headers:, params: {} end it "shows reactivation page with reactivate and cancel buttons for the organisation" do - expect(path).to include("/organisations/#{organisation.id}/reactivate") - expect(page).to have_content(organisation.name) + expect(path).to include("/organisations/#{inactive_organisation.id}/reactivate") + expect(page).to have_content(inactive_organisation.name) expect(page).to have_content("Are you sure you want to reactivate this organisation?") expect(page).to have_button("Reactivate this organisation") - expect(page).to have_link("Cancel", href: "/organisations/#{organisation.id}") + expect(page).to have_link("Cancel", href: "/organisations/#{inactive_organisation.id}") end end