CLDC-2479 Allow deleting users (#2288)

* Add delete confirmation page

* Add status to the user

* Allow deleting user

* Refactor sign_in user in tests

* Add delete user button and update policy

* Update user policy

* Do not display deleted users as an answer option

* Update user table and details

* Do not show deleted users

* Disable delete user on production

* Update test

app/controllers/organisations_controller.rb

@@ -46,14 +46,14 @@ class OrganisationsController < ApplicationController
   end
 
   def users
-    organisation_users = @organisation.users.sorted_by_organisation_and_role
+    organisation_users = @organisation.users.visible.sorted_by_organisation_and_role
     unpaginated_filtered_users = filter_manager.filtered_users(organisation_users, search_term, session_filters)
 
     respond_to do |format|
       format.html do
         @pagy, @users = pagy(unpaginated_filtered_users)
         @searched = search_term.presence
-        @total_count = @organisation.users.size
+        @total_count = @organisation.users.visible.size
         @filter_type = "users"
 
         if current_user.support?

app/controllers/users_controller.rb

@@ -13,7 +13,7 @@ class UsersController < ApplicationController
   def index
     redirect_to users_organisation_path(current_user.organisation) unless current_user.support?
 
-    all_users = User.sorted_by_organisation_and_role
+    all_users = User.visible.sorted_by_organisation_and_role
     filtered_users = filter_manager.filtered_users(all_users, search_term, session_filters)
     @pagy, @users = pagy(filtered_users)
     @searched = search_term.presence
@@ -122,6 +122,16 @@ class UsersController < ApplicationController
     end
   end
 
+  def delete_confirmation
+    authorize @user
+  end
+
+  def delete
+    authorize @user
+    @user.discard!
+    redirect_to users_organisation_path(@user.organisation), notice: I18n.t("notification.user_deleted", name: @user.name)
+  end
+
 private
 
   def validate_attributes
@@ -197,6 +207,7 @@ private
     if action_name == "create"
       head :unauthorized and return unless current_user.data_coordinator? || current_user.support?
     else
+      render_not_found and return if @user.status == :deleted
       render_not_found and return unless (current_user.organisation == @user.organisation) || current_user.support?
       render_not_found and return if action_name == "edit_password" && current_user != @user
       render_not_found and return unless action_name == "show" ||

app/helpers/tag_helper.rb

@@ -30,7 +30,7 @@ module TagHelper
     deactivated: "grey",
     deleted: "red",
     merged: "orange",
-    unconfirmed: "red",
+    unconfirmed: "blue",
   }.freeze
 
   def status_tag(status, classes = [])

app/helpers/user_helper.rb

@@ -10,4 +10,8 @@ module UserHelper
   def can_edit_org?(current_user)
     current_user.data_coordinator? || current_user.support?
   end
+
+  def delete_user_link(user)
+    govuk_button_link_to "Delete this user", delete_confirmation_user_path(user), warning: true
+  end
 end

app/models/form/lettings/questions/created_by_id.rb

@@ -22,15 +22,15 @@ class Form::Lettings::Questions::CreatedById < ::Form::Question
                [
                  (
                    if log.owning_organisation
-                     log.owning_organisation.absorbing_organisation.present? ? log.owning_organisation&.absorbing_organisation&.users : log.owning_organisation&.users
+                     log.owning_organisation.absorbing_organisation.present? ? log.owning_organisation&.absorbing_organisation&.users&.visible : log.owning_organisation&.users&.visible
                    end),
                  (
                    if log.managing_organisation
-                     log.managing_organisation.absorbing_organisation.present? ? log.managing_organisation&.absorbing_organisation&.users : log.managing_organisation.users
+                     log.managing_organisation.absorbing_organisation.present? ? log.managing_organisation&.absorbing_organisation&.users&.visible : log.managing_organisation.users&.visible
                    end),
                ].flatten
              else
-               current_user.organisation.users
+               current_user.organisation.users.visible
              end.uniq.compact
 
     users.each_with_object(ANSWER_OPTS.dup) do |user, hsh|

app/models/form/sales/questions/created_by_id.rb

@@ -23,11 +23,11 @@ class Form::Sales::Questions::CreatedById < ::Form::Question
                [
                  (
                    if log.managing_organisation
-                     log.managing_organisation.absorbing_organisation.present? ? log.managing_organisation&.absorbing_organisation&.users : log.managing_organisation.users
+                     log.managing_organisation.absorbing_organisation.present? ? log.managing_organisation&.absorbing_organisation&.users&.visible : log.managing_organisation.users.visible
                    end),
                ].flatten
              else
-               log.managing_organisation.users
+               log.managing_organisation.users.visible
              end.uniq.compact
     users.each_with_object(ANSWER_OPTS.dup) do |user, hsh|
       hsh[user.id] = present_user(user)

app/models/user.rb

@@ -76,6 +76,7 @@ class User < ApplicationRecord
   scope :not_signed_in, -> { where(last_sign_in_at: nil, active: true) }
   scope :deactivated, -> { where(active: false) }
   scope :active_status, -> { where(active: true).where.not(last_sign_in_at: nil) }
+  scope :visible, -> { where(discarded_at: nil) }
 
   def lettings_logs
     if support?
@@ -238,13 +239,15 @@ class User < ApplicationRecord
   end
 
   def status
-    if active == false
+    return :deleted if discarded_at.present?
-      :deactivated
+    return :deactivated unless active
-    elsif confirmed? == false
+    return :unconfirmed unless confirmed?
-      :unconfirmed
+
-    else
+    :active
-      :active
+  end
-    end
+
+  def discard!
+    update!(discarded_at: Time.zone.now)
   end
 
 protected

app/policies/user_policy.rb

@@ -33,4 +33,29 @@ class UserPolicy
       (@current_user == @user || @current_user.data_coordinator? || @current_user.support?) && @user.active?
     end
   end
+
+  def delete_confirmation?
+    delete?
+  end
+
+  def delete?
+    return false unless current_user.support?
+    return false unless user.status == :deactivated
+
+    !has_any_logs_in_editable_collection_period && !has_signed_data_protection_agreement?
+  end
+
+private
+
+  def has_any_logs_in_editable_collection_period
+    editable_from_date = FormHandler.instance.earliest_open_for_editing_collection_start_date
+
+    LettingsLog.where(created_by_id: user.id).after_date(editable_from_date).or(LettingsLog.where(startdate: nil, created_by_id: user.id)).any?
+  end
+
+  def has_signed_data_protection_agreement?
+    return false unless user.is_dpo? && user.organisation.data_protection_confirmed?
+
+    user.organisation.data_protection_confirmation.data_protection_officer == user
+  end
 end

app/services/feature_toggle.rb

@@ -34,4 +34,8 @@ class FeatureToggle
   def self.delete_location_enabled?
     !Rails.env.production?
   end
+
+  def self.delete_user_enabled?
+    !Rails.env.production?
+  end
 end

app/views/users/_user_list.html.erb

@@ -18,6 +18,9 @@
         <% row.with_cell(header: true, text: "Last logged in", html_attributes: {
           scope: "col",
         }) %>
+        <% row.with_cell(header: true, text: "Status", html_attributes: {
+          scope: "col",
+        }) %>
       <% end %>
     <% end %>
     <% users.each do |user| %>
@@ -50,7 +53,8 @@
             <% end %>
           <% end %>
           <% row.with_cell(text: simple_format(org_cell(user), {}, wrapper_tag: "div")) %>
-          <% row.with_cell(text: user.active? ? user.last_sign_in_at&.to_formatted_s(:govuk_date) : status_tag(user.status)) %>
+          <% row.with_cell(text: user.last_sign_in_at&.to_formatted_s(:govuk_date)) %>
+          <% row.with_cell(text: status_tag(user.status)) %>
           <%= govuk_link_to users_path(user) do %>
             <span class="govuk-visually-hidden">User </span><%= user.id %>
           <% end %>

app/views/users/delete_confirmation.html.erb

@@ -0,0 +1,24 @@
+<% content_for :before_content do %>
+  <% content_for :title, "Are you sure you want to delete this user?" %>
+  <%= govuk_back_link(href: :back) %>
+<% end %>
+
+<div class="govuk-grid-row">
+  <div class="govuk-grid-column-two-thirds-from-desktop">
+    <span class="govuk-caption-xl">Delete <%= @user.name %></span>
+    <h1 class="govuk-heading-xl">
+      <%= content_for(:title) %>
+    </h1>
+
+    <%= govuk_warning_text(text: "You will not be able to undo this action.") %>
+
+    <div class="govuk-button-group">
+      <%= govuk_button_to(
+        "Delete this user",
+        delete_user_path(@user),
+        method: :delete,
+      ) %>
+      <%= govuk_button_link_to "Cancel", user_path(@user), html: { method: :get }, secondary: true %>
+    </div>
+  </div>
+</div>

app/views/users/show.html.erb

@@ -112,11 +112,18 @@
               row.with_action
             end
           end %>
-
+      <%= summary_list.with_row do |row| %>
+          <% row.with_key { "Status" } %>
+          <% row.with_value do %>
+           <%= details_html({ name: "Status", value: status_tag(@user.status), id: "status" }) %>
+            <% if @user.status == :deactivated && current_user.support? && !UserPolicy.new(current_user, @user).delete? %>
+              <span class="app-!-colour-muted">This user was active in an open or editable collection year, and cannot be deleted.</span>
+          <% end %>
+          <% end %>
+        <% end %>
       <%= summary_list.with_row do |row|
-            row.with_key { "Status" }
+            row.with_key { "Last logged in" }
-            row.with_value { status_tag(@user.status) }
+            row.with_value { @user.last_sign_in_at&.to_formatted_s(:govuk_date) }
-            row.with_action
           end %>
     <% end %>
 
@@ -133,6 +140,9 @@
           </span>
         <% end %>
       <% end %>
+      <% if UserPolicy.new(current_user, @user).delete? && FeatureToggle.delete_user_enabled? %>
+        <%= delete_user_link(@user) %>
+      <% end %>
     </div>
   </div>
 </div>

config/locales/en.yml

@@ -198,6 +198,7 @@ en:
       other: "There are %{count} sets of duplicate logs"
     location_deleted: "%{postcode} has been deleted."
     scheme_deleted: "%{service_name} has been deleted."
+    user_deleted: "%{name} has been deleted."
 
   validations:
     organisation:

config/routes.rb

@@ -129,6 +129,8 @@ Rails.application.routes.draw do
       get "deactivate", to: "users#deactivate"
       get "reactivate", to: "users#reactivate"
       post "resend-invite", to: "users#resend_invite"
+      get "delete-confirmation", to: "users#delete_confirmation"
+      delete "delete", to: "users#delete"
     end
   end
 

db/migrate/20240305091927_add_discarded_at_column_to_users.rb

@@ -0,0 +1,5 @@
+class AddDiscardedAtColumnToUsers < ActiveRecord::Migration[7.0]
+  def change
+    add_column :users, :discarded_at, :datetime
+  end
+end

db/schema.rb

@@ -763,6 +763,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_03_19_122706) do
     t.datetime "confirmation_sent_at"
     t.string "unconfirmed_email"
     t.boolean "initial_confirmation_sent"
+    t.datetime "discarded_at"
     t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true
     t.index ["email"], name: "index_users_on_email", unique: true
     t.index ["encrypted_otp_secret_key"], name: "index_users_on_encrypted_otp_secret_key", unique: true

spec/features/user_spec.rb

@@ -480,14 +480,14 @@ RSpec.describe "User Features" do
       it "allows to cancel user deactivation" do
         click_link("No – I’ve changed my mind")
         expect(page).to have_current_path("/users/#{other_user.id}")
-        expect(page).to have_no_content("This user has been deactivated.")
+        assert_selector ".govuk-tag", text: /Deactivated/, count: 0
         expect(page).to have_no_css(".govuk-notification-banner.govuk-notification-banner--success")
       end
 
       it "allows to deactivate the user" do
         click_button("I’m sure – deactivate this user")
         expect(page).to have_current_path("/users/#{other_user.id}")
-        expect(page).to have_content("Deactivated")
+        assert_selector ".govuk-tag", text: /Deactivated/, count: 1
         expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success")
       end
     end
@@ -517,7 +517,7 @@ RSpec.describe "User Features" do
       it "allows to cancel user reactivation" do
         click_link("No – I’ve changed my mind")
         expect(page).to have_current_path("/users/#{other_user.id}")
-        expect(page).to have_content("Deactivated")
+        assert_selector ".govuk-tag", text: /Deactivated/, count: 1
         expect(page).to have_no_css(".govuk-notification-banner.govuk-notification-banner--success")
       end
 
@@ -525,7 +525,7 @@ RSpec.describe "User Features" do
         expect(notify_client).to receive(:send_email).with(email_address: other_user.email, template_id: User::USER_REACTIVATED_TEMPLATE_ID, personalisation:).once
         click_button("I’m sure – reactivate this user")
         expect(page).to have_current_path("/users/#{other_user.id}")
-        expect(page).to have_no_content("This user has been deactivated.")
+        assert_selector ".govuk-tag", text: /Deactivated/, count: 0
         expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success")
       end
     end

spec/models/form/lettings/questions/created_by_id_spec.rb

@@ -60,6 +60,17 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do
       it "only displays users that belong to owning and managing organisations" do
         expect(question.displayed_answer_options(lettings_log, support_user)).to eq(expected_option_for_users(managing_org_user.organisation.users + owning_org_user.organisation.users))
       end
+
+      context "when organisation has deleted users" do
+        before do
+          create(:user, name: "Deleted user", discarded_at: Time.zone.yesterday, organisation: owning_org_user.organisation)
+          create(:user, name: "Deleted managing user", discarded_at: Time.zone.yesterday, organisation: managing_org_user.organisation)
+        end
+
+        it "does not display deleted users" do
+          expect(question.displayed_answer_options(lettings_log, support_user)).to eq(expected_option_for_users(managing_org_user.organisation.users.visible + owning_org_user.organisation.users.visible))
+        end
+      end
     end
   end
 
@@ -77,6 +88,16 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do
       it "only displays users that belong user's org" do
         expect(question.displayed_answer_options(lettings_log, data_coordinator)).to eq(expected_option_for_users(data_coordinator.organisation.users))
       end
+
+      context "when organisation has deleted users" do
+        before do
+          create(:user, name: "Deleted user", discarded_at: Time.zone.yesterday, organisation: data_coordinator.organisation)
+        end
+
+        it "does not display deleted users" do
+          expect(question.displayed_answer_options(lettings_log, data_coordinator)).to eq(expected_option_for_users(data_coordinator.organisation.users.visible))
+        end
+      end
     end
   end
 end

spec/models/form/sales/questions/created_by_id_spec.rb

@@ -57,6 +57,16 @@ RSpec.describe Form::Sales::Questions::CreatedById, type: :model do
       it "only displays users that belong to the managing organisation" do
         expect(question.displayed_answer_options(sales_log, support_user)).to eq(expected_option_for_users(owning_org_user.organisation.users))
       end
+
+      context "when organisation has deleted users" do
+        before do
+          create(:user, name: "Deleted user", discarded_at: Time.zone.yesterday, organisation: owning_org_user.organisation)
+        end
+
+        it "does not display deleted users" do
+          expect(question.displayed_answer_options(sales_log, support_user)).to eq(expected_option_for_users(owning_org_user.organisation.users.visible))
+        end
+      end
     end
   end
 
@@ -78,6 +88,16 @@ RSpec.describe Form::Sales::Questions::CreatedById, type: :model do
       it "only displays users that belong to managing organisation" do
         expect(question.displayed_answer_options(sales_log, data_coordinator)).to eq(expected_option_for_users(owning_org_user.organisation.users))
       end
+
+      context "when organisation has deleted users" do
+        before do
+          create(:user, name: "Deleted user", discarded_at: Time.zone.yesterday, organisation: owning_org_user.organisation)
+        end
+
+        it "does not display deleted users" do
+          expect(question.displayed_answer_options(sales_log, data_coordinator)).to eq(expected_option_for_users(owning_org_user.organisation.users.visible))
+        end
+      end
     end
   end
 end

spec/models/user_spec.rb

@@ -517,5 +517,14 @@ RSpec.describe User, type: :model do
 
       expect(user.status).to eq(:active)
     end
+
+    context "when the user is deleted" do
+      let(:user) { create(:user, discarded_at: Time.zone.yesterday) }
+
+      it "returns the status of the user" do
+        user.destroy!
+        expect(user.status).to eq(:deleted)
+      end
+    end
   end
 end

spec/policies/user_policy_spec.rb

@@ -99,5 +99,129 @@ RSpec.describe UserPolicy do
       expect(policy).to permit(support, data_provider)
     end
   end
+
+  permissions :delete? do
+    context "with active user" do
+      let(:user) { create(:user, last_sign_in_at: Time.zone.yesterday) }
+
+      it "does not allow deleting a user as a provider" do
+        expect(user.status).to eq(:active)
+        expect(policy).not_to permit(data_provider, user)
+      end
+
+      it "does not allow allows deleting a user as a coordinator" do
+        expect(policy).not_to permit(data_coordinator, user)
+      end
+
+      it "does not allow deleting a user as a support user" do
+        expect(policy).not_to permit(support, user)
+      end
+    end
+
+    context "with unconfirmed user" do
+      let(:user) { create(:user) }
+
+      before do
+        user.confirmed_at = nil
+        user.save!(validate: false)
+      end
+
+      it "does not allow deleting a user as a provider" do
+        expect(user.status).to eq(:unconfirmed)
+        expect(policy).not_to permit(data_provider, user)
+      end
+
+      it "does not allow allows deleting a user as a coordinator" do
+        expect(policy).not_to permit(data_coordinator, user)
+      end
+
+      it "does not allow deleting a user as a support user" do
+        expect(policy).not_to permit(support, user)
+      end
+    end
+
+    context "with deactivated user" do
+      let(:user) { create(:user, active: false) }
+
+      before do
+        Timecop.freeze(Time.utc(2024, 4, 10))
+        log = create(:lettings_log, owning_organisation: user.organisation, created_by: user)
+        log.startdate = Time.zone.local(2022, 10, 10)
+        log.save!(validate: false)
+      end
+
+      after do
+        Timecop.unfreeze
+      end
+
+      context "and associated logs in editable collection period" do
+        before do
+          create(:lettings_log, :sh, owning_organisation: user.organisation, created_by: user, startdate: Time.zone.local(2024, 4, 9))
+        end
+
+        it "does not allow deleting a user as a provider" do
+          expect(policy).not_to permit(data_provider, user)
+        end
+
+        it "does not allow allows deleting a user as a coordinator" do
+          expect(policy).not_to permit(data_coordinator, user)
+        end
+
+        it "does not allow deleting a user as a support user" do
+          expect(policy).not_to permit(support, user)
+        end
+      end
+
+      context "and no associated logs in editable collection period" do
+        it "does not allow deleting a user as a provider" do
+          expect(policy).not_to permit(data_provider, user)
+        end
+
+        it "does not allow allows deleting a user as a coordinator" do
+          expect(policy).not_to permit(data_coordinator, user)
+        end
+
+        it "allows deleting a user as a support user" do
+          expect(policy).to permit(support, user)
+        end
+      end
+
+      context "and user is the DPO that has signed the agreement" do
+        let(:user) { create(:user, active: false, is_dpo: true) }
+
+        before do
+          user.organisation.data_protection_confirmation.update!(data_protection_officer: user)
+        end
+
+        it "does not allow deleting a user as a provider" do
+          expect(policy).not_to permit(data_provider, user)
+        end
+
+        it "does not allow allows deleting a user as a coordinator" do
+          expect(policy).not_to permit(data_coordinator, user)
+        end
+
+        it "does not allow deleting a user as a support user" do
+          expect(policy).not_to permit(support, user)
+        end
+      end
+
+      context "and user is the DPO that hasn't signed the agreement" do
+        let(:user) { create(:user, active: false, is_dpo: true) }
+
+        it "does not allow deleting a user as a provider" do
+          expect(policy).not_to permit(data_provider, user)
+        end
+
+        it "does not allow allows deleting a user as a coordinator" do
+          expect(policy).not_to permit(data_coordinator, user)
+        end
+
+        it "allows deleting a user as a support user" do
+          expect(policy).to permit(support, user)
+        end
+      end
+    end
+  end
 end
 # rubocop:enable RSpec/RepeatedExample

spec/requests/users_controller_spec.rb

@@ -103,13 +103,30 @@ RSpec.describe UsersController, type: :request do
         expect(response).to redirect_to(new_user_session_path)
       end
     end
+
+    describe "#delete-confirmation" do
+      it "redirects to the sign in page" do
+        get "/users/#{user.id}/delete-confirmation"
+        expect(response).to redirect_to("/account/sign-in")
+      end
+    end
+
+    describe "#delete" do
+      it "redirects to the sign in page" do
+        delete "/users/#{user.id}/delete"
+        expect(response).to redirect_to("/account/sign-in")
+      end
+    end
   end
 
   context "when user is signed in as a data provider" do
+    before do
+      sign_in user
+    end
+
     describe "#show" do
       context "when the current user matches the user ID" do
         before do
-          sign_in user
           get "/users/#{user.id}", headers:, params: {}
         end
 
@@ -155,7 +172,6 @@ RSpec.describe UsersController, type: :request do
         let(:user) { create(:user, role: nil) }
 
         before do
-          sign_in user
           get "/users/#{user.id}", headers:, params: {}
         end
 
@@ -166,7 +182,6 @@ RSpec.describe UsersController, type: :request do
 
       context "when the current user does not match the user ID" do
         before do
-          sign_in user
           get "/users/#{other_user.id}", headers:, params: {}
         end
 
@@ -223,7 +238,6 @@ RSpec.describe UsersController, type: :request do
     describe "#edit" do
       context "when the current user matches the user ID" do
         before do
-          sign_in user
           get "/users/#{user.id}/edit", headers:, params: {}
         end
 
@@ -242,7 +256,6 @@ RSpec.describe UsersController, type: :request do
 
       context "when the current user does not match the user ID" do
         before do
-          sign_in user
           get "/users/#{other_user.id}/edit", headers:, params: {}
         end
 
@@ -255,7 +268,6 @@ RSpec.describe UsersController, type: :request do
     describe "#edit_password" do
       context "when the current user matches the user ID" do
         before do
-          sign_in user
           get "/account/edit/password", headers:, params: {}
         end
 
@@ -270,7 +282,6 @@ RSpec.describe UsersController, type: :request do
 
       context "when the current user does not match the user ID" do
         before do
-          sign_in user
           get "/users/#{other_user.id}/edit", headers:, params: {}
         end
 
@@ -283,7 +294,6 @@ RSpec.describe UsersController, type: :request do
     describe "#update" do
       context "when the current user matches the user ID" do
         before do
-          sign_in user
           patch "/users/#{user.id}", headers:, params:
         end
 
@@ -313,7 +323,6 @@ RSpec.describe UsersController, type: :request do
 
       context "when the update fails to persist" do
         before do
-          sign_in user
           allow(User).to receive(:find_by).and_return(user)
           allow(user).to receive(:update).and_return(false)
           patch "/users/#{user.id}", headers:, params:
@@ -328,7 +337,6 @@ RSpec.describe UsersController, type: :request do
         let(:params) { { id: other_user.id, user: { name: new_name } } }
 
         before do
-          sign_in user
           patch "/users/#{other_user.id}", headers:, params:
         end
 
@@ -345,7 +353,6 @@ RSpec.describe UsersController, type: :request do
         end
 
         before do
-          sign_in user
           patch "/users/#{user.id}", headers:, params:
         end
 
@@ -368,10 +375,6 @@ RSpec.describe UsersController, type: :request do
       end
       let(:request) { post "/users/", headers:, params: }
 
-      before do
-        sign_in user
-      end
-
       it "does not invite a new user" do
         expect { request }.not_to change(User, :count)
       end
@@ -381,17 +384,37 @@ RSpec.describe UsersController, type: :request do
         expect(response).to have_http_status(:unauthorized)
       end
     end
+
+    describe "#delete-confirmation" do
+      before do
+        get "/users/#{user.id}/delete-confirmation"
+      end
+
+      it "returns 401 unauthorized" do
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
+
+    describe "#delete" do
+      before do
+        delete "/users/#{user.id}/delete"
+      end
+
+      it "returns 401 unauthorized" do
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
   end
 
   context "when user is signed in as a data coordinator" do
     let(:user) { create(:user, :data_coordinator, email: "coordinator@example.com", organisation: create(:organisation, :without_dpc)) }
     let!(:other_user) { create(:user, organisation: user.organisation, name: "filter name", email: "filter@example.com") }
 
-    describe "#index" do
+    before do
-      before do
+      sign_in user
-        sign_in user
+    end
-      end
 
+    describe "#index" do
       context "when there are no url params" do
         before do
           get "/users", headers:, params: {}
@@ -532,7 +555,6 @@ RSpec.describe UsersController, type: :request do
       let(:user) { create(:user) }
 
       before do
-        sign_in user
         get "/users", headers:, params: {}
       end
 
@@ -544,7 +566,6 @@ RSpec.describe UsersController, type: :request do
     describe "#show" do
       context "when the current user matches the user ID" do
         before do
-          sign_in user
           get "/users/#{user.id}", headers:, params: {}
         end
 
@@ -579,12 +600,15 @@ RSpec.describe UsersController, type: :request do
           it "does not allow resending invitation emails" do
             expect(page).not_to have_button("Resend invite link")
           end
+
+          it "does not allow deleting the the user" do
+            expect(page).not_to have_link("Delete this user", href: "/users/#{user.id}/delete-confirmation")
+          end
         end
       end
 
       context "when the current user does not match the user ID" do
         before do
-          sign_in user
           get "/users/#{other_user.id}", headers:, params: {}
         end
 
@@ -622,7 +646,7 @@ RSpec.describe UsersController, type: :request do
             end
 
             it "shows if user is not active" do
-              expect(page).to have_content("Deactivated")
+              assert_select ".govuk-tag", text: /Deactivated/, count: 1
             end
 
             it "allows reactivating the user" do
@@ -652,7 +676,6 @@ RSpec.describe UsersController, type: :request do
     describe "#edit" do
       context "when the current user matches the user ID" do
         before do
-          sign_in user
           get "/users/#{user.id}/edit", headers:, params: {}
         end
 
@@ -673,7 +696,6 @@ RSpec.describe UsersController, type: :request do
 
       context "when the current user does not match the user ID" do
         before do
-          sign_in user
           get "/users/#{other_user.id}/edit", headers:, params: {}
         end
 
@@ -706,7 +728,6 @@ RSpec.describe UsersController, type: :request do
     describe "#edit_password" do
       context "when the current user matches the user ID" do
         before do
-          sign_in user
           get "/account/edit/password", headers:, params: {}
         end
 
@@ -720,10 +741,6 @@ RSpec.describe UsersController, type: :request do
       end
 
       context "when the current user does not match the user ID" do
-        before do
-          sign_in user
-        end
-
         it "there is no route" do
           expect {
             get "/users/#{other_user.id}/password/edit", headers:, params: {}
@@ -735,7 +752,6 @@ RSpec.describe UsersController, type: :request do
     describe "#update" do
       context "when the current user matches the user ID" do
         before do
-          sign_in user
           patch "/users/#{user.id}", headers:, params:
         end
 
@@ -770,7 +786,6 @@ RSpec.describe UsersController, type: :request do
           end
 
           before do
-            sign_in user
             patch "/users/#{user.id}", headers:, params:
           end
 
@@ -782,10 +797,6 @@ RSpec.describe UsersController, type: :request do
       end
 
       context "when the current user does not match the user ID" do
-        before do
-          sign_in user
-        end
-
         context "when the user is part of the same organisation as the current user" do
           it "updates the user" do
             expect { patch "/users/#{other_user.id}", headers:, params: }
@@ -871,7 +882,6 @@ RSpec.describe UsersController, type: :request do
             let(:params) { { id: other_user.id, user: { name: new_name } } }
 
             before do
-              sign_in user
               patch "/users/#{other_user.id}", headers:, params:
             end
 
@@ -884,7 +894,6 @@ RSpec.describe UsersController, type: :request do
 
       context "when the update fails to persist" do
         before do
-          sign_in user
           allow(User).to receive(:find_by).and_return(user)
           allow(user).to receive(:update).and_return(false)
           patch "/users/#{user.id}", headers:, params:
@@ -905,7 +914,6 @@ RSpec.describe UsersController, type: :request do
         end
 
         before do
-          sign_in user
           patch "/users/#{user.id}", headers:, params:
         end
 
@@ -977,10 +985,6 @@ RSpec.describe UsersController, type: :request do
       end
       let(:request) { post "/users/", headers:, params: }
 
-      before do
-        sign_in user
-      end
-
       it "invites a new user" do
         expect { request }.to change(User, :count).by(1)
       end
@@ -1102,10 +1106,6 @@ RSpec.describe UsersController, type: :request do
     end
 
     describe "#new" do
-      before do
-        sign_in user
-      end
-
       it "cannot assign support role to the new user" do
         get "/users/new"
         expect(page).not_to have_field("user-role-support-field")
@@ -1113,10 +1113,6 @@ RSpec.describe UsersController, type: :request do
     end
 
     describe "#deactivate" do
-      before do
-        sign_in user
-      end
-
       context "when the current user matches the user ID" do
         before do
           get "/users/#{user.id}/deactivate", headers:, params: {}
@@ -1143,10 +1139,6 @@ RSpec.describe UsersController, type: :request do
     end
 
     describe "#reactivate" do
-      before do
-        sign_in user
-      end
-
       context "when the current user does not match the user ID" do
         before do
           other_user.update!(active: false)
@@ -1162,6 +1154,26 @@ RSpec.describe UsersController, type: :request do
         end
       end
     end
+
+    describe "#delete-confirmation" do
+      before do
+        get "/users/#{user.id}/delete-confirmation"
+      end
+
+      it "returns 401 unauthorized" do
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
+
+    describe "#delete" do
+      before do
+        delete "/users/#{user.id}/delete"
+      end
+
+      it "returns 401 unauthorized" do
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
   end
 
   context "when user is signed in as a support user" do
@@ -1170,15 +1182,15 @@ RSpec.describe UsersController, type: :request do
 
     before do
       allow(user).to receive(:need_two_factor_authentication?).and_return(false)
+      sign_in user
     end
 
     describe "#index" do
       let!(:other_user) { create(:user, organisation: user.organisation, name: "User 2", email: "other@example.com") }
-      let!(:inactive_user) { create(:user, organisation: user.organisation, active: false, name: "User 3", email: "inactive@example.com") }
+      let!(:inactive_user) { create(:user, organisation: user.organisation, active: false, name: "User 3", email: "inactive@example.com", last_sign_in_at: Time.zone.local(2022, 10, 10)) }
       let!(:other_org_user) { create(:user, name: "User 4", email: "otherorg@otherexample.com", organisation: create(:organisation, :without_dpc)) }
 
       before do
-        sign_in user
         get "/users", headers:, params: {}
       end
 
@@ -1189,7 +1201,11 @@ RSpec.describe UsersController, type: :request do
         expect(page).to have_content(other_org_user.name)
       end
 
-      it "shows last logged in as deactivated for inactive users" do
+      it "shows last logged in date for all users" do
+        expect(page).to have_content("10 October 2022")
+      end
+
+      it "shows status tag as deactivated for inactive users" do
         expect(page).to have_content("Deactivated")
       end
 
@@ -1326,7 +1342,6 @@ RSpec.describe UsersController, type: :request do
 
       before do
         create_list(:user, 25)
-        sign_in user
       end
 
       context "when there is no search param" do
@@ -1371,7 +1386,6 @@ RSpec.describe UsersController, type: :request do
     describe "#show" do
       context "when the current user matches the user ID" do
         before do
-          sign_in user
           get "/users/#{user.id}", headers:, params: {}
         end
 
@@ -1396,7 +1410,6 @@ RSpec.describe UsersController, type: :request do
 
       context "when the current user does not match the user ID" do
         before do
-          sign_in user
           get "/users/#{other_user.id}", headers:, params: {}
         end
 
@@ -1427,6 +1440,10 @@ RSpec.describe UsersController, type: :request do
             expect(page).to have_link("Deactivate user", href: "/users/#{other_user.id}/deactivate")
           end
 
+          it "does not alow deleting the the user" do
+            expect(page).not_to have_link("Delete this user", href: "/users/#{other_user.id}/delete-confirmation")
+          end
+
           context "when user never logged in" do
             before do
               other_user.update!(last_sign_in_at: nil)
@@ -1458,6 +1475,10 @@ RSpec.describe UsersController, type: :request do
             it "allows you to resend invitation emails" do
               expect(page).to have_button("Resend invite link")
             end
+
+            it "does not allow deleting the the user" do
+              expect(page).not_to have_link("Delete this user", href: "/users/#{other_user.id}/delete-confirmation")
+            end
           end
 
           context "when user is deactivated" do
@@ -1467,12 +1488,39 @@ RSpec.describe UsersController, type: :request do
             end
 
             it "shows if user is not active" do
-              expect(page).to have_content("Deactivated")
+              assert_select ".govuk-tag", text: /Deactivated/, count: 1
             end
 
             it "allows reactivating the user" do
               expect(page).to have_link("Reactivate user", href: "/users/#{other_user.id}/reactivate")
             end
+
+            it "allows deleting the the user" do
+              expect(page).to have_link("Delete this user", href: "/users/#{other_user.id}/delete-confirmation")
+            end
+
+            it "does not render informative text about deleting the user" do
+              expect(response).to have_http_status(:ok)
+              expect(page).not_to have_content("This user was active in an open or editable collection year, and cannot be deleted.")
+            end
+
+            context "and has associated logs in editable collection period" do
+              before do
+                create(:data_protection_confirmation, organisation: other_user.organisation, confirmed: true)
+                create(:lettings_log, owning_organisation: other_user.organisation, created_by: other_user)
+                get "/users/#{other_user.id}"
+              end
+
+              it "does not render delete this user" do
+                expect(response).to have_http_status(:ok)
+                expect(page).not_to have_link("Delete this user", href: "/users/#{user.id}/delete-confirmation")
+              end
+
+              it "adds informative text about deleting the user" do
+                expect(response).to have_http_status(:ok)
+                expect(page).to have_content("This user was active in an open or editable collection year, and cannot be deleted.")
+              end
+            end
           end
         end
 
@@ -1503,7 +1551,6 @@ RSpec.describe UsersController, type: :request do
     describe "#edit" do
       context "when the current user matches the user ID" do
         before do
-          sign_in user
           get "/users/#{user.id}/edit", headers:, params: {}
         end
 
@@ -1525,7 +1572,6 @@ RSpec.describe UsersController, type: :request do
 
       context "when the current user does not match the user ID" do
         before do
-          sign_in user
           get "/users/#{other_user.id}/edit", headers:, params: {}
         end
 
@@ -1581,7 +1627,6 @@ RSpec.describe UsersController, type: :request do
     describe "#edit_password" do
       context "when the current user matches the user ID" do
         before do
-          sign_in user
           get "/account/edit/password", headers:, params: {}
         end
 
@@ -1595,10 +1640,6 @@ RSpec.describe UsersController, type: :request do
       end
 
       context "when the current user does not match the user ID" do
-        before do
-          sign_in user
-        end
-
         it "there is no route" do
           expect {
             get "/users/#{other_user.id}/password/edit", headers:, params: {}
@@ -1611,10 +1652,6 @@ RSpec.describe UsersController, type: :request do
       context "when the current user matches the user ID" do
         let(:request) { patch "/users/#{user.id}", headers:, params: }
 
-        before do
-          sign_in user
-        end
-
         it "updates the user" do
           request
           user.reload
@@ -1726,7 +1763,6 @@ RSpec.describe UsersController, type: :request do
           end
 
           before do
-            sign_in user
             patch "/users/#{user.id}", headers:, params:
           end
 
@@ -1738,10 +1774,6 @@ RSpec.describe UsersController, type: :request do
       end
 
       context "when the current user does not match the user ID" do
-        before do
-          sign_in user
-        end
-
         context "when the user is part of the same organisation as the current user" do
           it "updates the user" do
             expect { patch "/users/#{other_user.id}", headers:, params: }
@@ -1796,10 +1828,6 @@ RSpec.describe UsersController, type: :request do
             let(:other_user) { create(:user) }
             let(:params) { { id: other_user.id, user: { name: new_name } } }
 
-            before do
-              sign_in user
-            end
-
             it "updates the user" do
               expect { patch "/users/#{other_user.id}", headers:, params: }
                 .to change { other_user.reload.name }.from(other_user.name).to(new_name)
@@ -1886,7 +1914,6 @@ RSpec.describe UsersController, type: :request do
 
       context "when the update fails to persist" do
         before do
-          sign_in user
           allow(User).to receive(:find_by).and_return(user)
           allow(user).to receive(:update).and_return(false)
           patch "/users/#{user.id}", headers:, params:
@@ -1914,10 +1941,6 @@ RSpec.describe UsersController, type: :request do
       end
       let(:request) { post "/users/", headers:, params: }
 
-      before do
-        sign_in user
-      end
-
       it "invites a new user" do
         expect { request }.to change(User, :count).by(1)
       end
@@ -1990,7 +2013,6 @@ RSpec.describe UsersController, type: :request do
 
     describe "#new" do
       before do
-        sign_in user
         create(:organisation, name: "other org")
       end
 
@@ -2018,6 +2040,68 @@ RSpec.describe UsersController, type: :request do
         end
       end
     end
+
+    describe "#delete-confirmation" do
+      let(:other_user) { create(:user, active: false) }
+
+      before do
+        get "/users/#{other_user.id}/delete-confirmation"
+      end
+
+      it "shows the correct title" do
+        expect(page.find("h1").text).to include "Are you sure you want to delete this user?"
+      end
+
+      it "shows a warning to the user" do
+        expect(page).to have_selector(".govuk-warning-text", text: "You will not be able to undo this action")
+      end
+
+      it "shows a button to delete the selected user" do
+        expect(page).to have_selector("form.button_to button", text: "Delete this user")
+      end
+
+      it "the delete user button submits the correct data to the correct path" do
+        form_containing_button = page.find("form.button_to")
+
+        expect(form_containing_button[:action]).to eq delete_user_path(other_user)
+        expect(form_containing_button).to have_field "_method", type: :hidden, with: "delete"
+      end
+
+      it "shows a cancel link with the correct style" do
+        expect(page).to have_selector("a.govuk-button--secondary", text: "Cancel")
+      end
+
+      it "shows cancel link that links back to the user page" do
+        expect(page).to have_link(text: "Cancel", href: user_path(other_user))
+      end
+    end
+
+    describe "#delete" do
+      let(:other_user) { create(:user, name: "User to be deleted", active: false) }
+
+      before do
+        delete "/users/#{other_user.id}/delete"
+      end
+
+      it "deletes the user" do
+        other_user.reload
+        expect(other_user.status).to eq(:deleted)
+        expect(other_user.discarded_at).not_to be nil
+      end
+
+      it "redirects to the users list and displays a notice that the user has been deleted" do
+        expect(response).to redirect_to users_organisation_path(other_user.organisation)
+        follow_redirect!
+        expect(page).to have_selector(".govuk-notification-banner--success")
+        expect(page).to have_selector(".govuk-notification-banner--success", text: "User to be deleted has been deleted.")
+      end
+
+      it "does not display the deleted user" do
+        expect(response).to redirect_to users_organisation_path(other_user.organisation)
+        follow_redirect!
+        expect(page).not_to have_link("User to be deleted")
+      end
+    end
   end
 
   describe "title link" do