CLDC-2481: Review markups

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
+          @organisation.users.filter_by_active.each do |user|
-                       .update_all(
+            user.deactivate!(reactivate_with_organisation: true)
-                         active: false,
+          end
-                         confirmed_at: nil,
-                         sign_in_count: 0,
-                         initial_confirmation_sent: false,
-                         reactivate_with_organisation: true,
-                       )
           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)

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")

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 {

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 {

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

app/policies/organisation_policy.rb

@@ -6,12 +6,11 @@ class OrganisationPolicy
     @organisation = organisation
   end
 
-  %w[
+  def deactivate?
-    deactivate?
+    user.support? && organisation.status == :active
-    reactivate?
+  end
-  ].each do |method_name|
+
-    define_method method_name do
+  def reactivate?
-      user.support?
+    user.support? && organisation.status == :deactivated
-    end
   end
 end

app/views/organisations/show.html.erb

@@ -41,14 +41,11 @@
   </div>
 </div>
 
-<% if @organisation.active? %>
+<% if OrganisationPolicy.new(current_user, @organisation).deactivate? %>
-  <% if OrganisationPolicy.new(current_user, @organisation).deactivate? %>
+  <%= govuk_button_link_to "Deactivate this organisation", deactivate_organisation_path(@organisation), warning: true %>
-    <%= govuk_button_link_to "Deactivate this organisation", deactivate_organisation_path(@organisation), warning: true %>
+<% end %>
-  <% end %>
+<% if OrganisationPolicy.new(current_user, @organisation).reactivate? %>
-<% else %>
+  <span class="app-!-colour-muted govuk-!-margin-right-2">
-  <% if OrganisationPolicy.new(current_user, @organisation).reactivate? %>
-    <span class="app-!-colour-muted govuk-!-margin-right-2">
       <%= govuk_button_link_to "Reactivate this organisation", reactivate_organisation_path(@organisation) %>
     </span>
-  <% end %>
 <% end %>

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
+    it "permits support users to deactivate an active organisation" do
-      expect(policy).to permit(support, data_provider)
+      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
+    it "does not permit support users to reactivate a merged organisation" do
-      expect(policy).to permit(support, data_provider)
+      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

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(path).to include("/organisations/#{inactive_organisation.id}/reactivate")
-          expect(page).to have_content(organisation.name)
+          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