diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index dc5a36c9b..4982c4287 100644 --- a/app/controllers/organisations_controller.rb +++ b/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? diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index a9344f0f7..6a76cb047 100644 --- a/app/controllers/users_controller.rb +++ b/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" || diff --git a/app/helpers/tag_helper.rb b/app/helpers/tag_helper.rb index 3cb02b769..c389942dc 100644 --- a/app/helpers/tag_helper.rb +++ b/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 = []) diff --git a/app/helpers/user_helper.rb b/app/helpers/user_helper.rb index 76fb78f57..6fcf6ca4c 100644 --- a/app/helpers/user_helper.rb +++ b/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 diff --git a/app/models/form/lettings/questions/created_by_id.rb b/app/models/form/lettings/questions/created_by_id.rb index 02176fb4d..b05f97e9f 100644 --- a/app/models/form/lettings/questions/created_by_id.rb +++ b/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| diff --git a/app/models/form/sales/questions/created_by_id.rb b/app/models/form/sales/questions/created_by_id.rb index 725361ee4..6bcaed595 100644 --- a/app/models/form/sales/questions/created_by_id.rb +++ b/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) diff --git a/app/models/user.rb b/app/models/user.rb index 25599da56..6d13e8cd1 100644 --- a/app/models/user.rb +++ b/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 - :deactivated - elsif confirmed? == false - :unconfirmed - else - :active - end + return :deleted if discarded_at.present? + return :deactivated unless active + return :unconfirmed unless confirmed? + + :active + end + + def discard! + update!(discarded_at: Time.zone.now) end protected diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index a4b1a3d5c..eb747266d 100644 --- a/app/policies/user_policy.rb +++ b/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 diff --git a/app/services/feature_toggle.rb b/app/services/feature_toggle.rb index f145b7132..1aaba1571 100644 --- a/app/services/feature_toggle.rb +++ b/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 diff --git a/app/views/users/_user_list.html.erb b/app/views/users/_user_list.html.erb index 19e206b03..436c0def2 100644 --- a/app/views/users/_user_list.html.erb +++ b/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 %> User <%= user.id %> <% end %> diff --git a/app/views/users/delete_confirmation.html.erb b/app/views/users/delete_confirmation.html.erb new file mode 100644 index 000000000..4c8653a78 --- /dev/null +++ b/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 %> + +
+
+ Delete <%= @user.name %> +

+ <%= content_for(:title) %> +

+ + <%= govuk_warning_text(text: "You will not be able to undo this action.") %> + +
+ <%= 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 %> +
+
+
diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index bf2877fbc..a93532e7b 100644 --- a/app/views/users/show.html.erb +++ b/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? %> + This user was active in an open or editable collection year, and cannot be deleted. + <% end %> + <% end %> + <% end %> <%= summary_list.with_row do |row| - row.with_key { "Status" } - row.with_value { status_tag(@user.status) } - row.with_action + row.with_key { "Last logged in" } + row.with_value { @user.last_sign_in_at&.to_formatted_s(:govuk_date) } end %> <% end %> @@ -133,6 +140,9 @@ <% end %> <% end %> + <% if UserPolicy.new(current_user, @user).delete? && FeatureToggle.delete_user_enabled? %> + <%= delete_user_link(@user) %> + <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index d95187a49..079967095 100644 --- a/config/locales/en.yml +++ b/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: diff --git a/config/routes.rb b/config/routes.rb index f0f497291..79dcf0a04 100644 --- a/config/routes.rb +++ b/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 diff --git a/db/migrate/20240305091927_add_discarded_at_column_to_users.rb b/db/migrate/20240305091927_add_discarded_at_column_to_users.rb new file mode 100644 index 000000000..469c4e145 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index da9c4b0aa..cca8b7f5c 100644 --- a/db/schema.rb +++ b/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 diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb index 180a92aec..e3335c762 100644 --- a/spec/features/user_spec.rb +++ b/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 diff --git a/spec/models/form/lettings/questions/created_by_id_spec.rb b/spec/models/form/lettings/questions/created_by_id_spec.rb index c175997f6..0ff09c597 100644 --- a/spec/models/form/lettings/questions/created_by_id_spec.rb +++ b/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 diff --git a/spec/models/form/sales/questions/created_by_id_spec.rb b/spec/models/form/sales/questions/created_by_id_spec.rb index 7645f45f0..742694f1d 100644 --- a/spec/models/form/sales/questions/created_by_id_spec.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 774a7893b..f00aa1c7c 100644 --- a/spec/models/user_spec.rb +++ b/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 diff --git a/spec/policies/user_policy_spec.rb b/spec/policies/user_policy_spec.rb index ec84fbceb..be0de12cb 100644 --- a/spec/policies/user_policy_spec.rb +++ b/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 diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index ddc457f6c..f73136948 100644 --- a/spec/requests/users_controller_spec.rb +++ b/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 - sign_in user - end + before do + sign_in user + 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