diff --git a/app/controllers/delete_logs_controller.rb b/app/controllers/delete_logs_controller.rb new file mode 100644 index 000000000..e2776825b --- /dev/null +++ b/app/controllers/delete_logs_controller.rb @@ -0,0 +1,56 @@ +class DeleteLogsController < ApplicationController + include Modules::LogsFilter + + rescue_from ActiveRecord::RecordNotFound, with: :render_not_found + + before_action :session_filters, if: :current_user, except: [:delete_logs] + + def delete_lettings_logs + @delete_logs_form = delete_logs_form + render "logs/delete_lettings_logs" + end + + def delete_lettings_logs_with_selected_ids + selected_ids = params.require(:selected_ids).split.map(&:to_i) + @delete_logs_form = delete_logs_form(selected_ids:) + render "logs/delete_lettings_logs" + end + + def delete_lettings_logs_confirmation + default_attributes = { + current_user:, + log_filters: @session_filters, + log_type: :lettings, + } + form_attributes = params.require(:forms_delete_logs_form).permit(:search_term, selected_ids: []) + attributes = form_attributes.merge(default_attributes) + attributes[:selected_ids] = [] unless attributes.key? :selected_ids + @delete_path = delete_logs_lettings_logs_path + @delete_logs_form = Forms::DeleteLogsForm.new(attributes) + if @delete_logs_form.valid? + render "logs/delete_logs_confirmation" + else + render "logs/delete_lettings_logs" + end + end + + def discard_lettings_logs + logs = LettingsLog.find(params.require(:ids)) + logs.each do |log| + authorize log, :destroy? + log.discard! + end + + redirect_to lettings_logs_path, notice: I18n.t("notification.logs_deleted", count: logs.count) + end + +private + + def delete_logs_form(selected_ids: nil) + Forms::DeleteLogsForm.new(current_user:, search_term:, log_filters: @session_filters, log_type: :lettings, selected_ids:) + end + + def search_term + params["search"] + end +end diff --git a/app/controllers/lettings_logs_controller.rb b/app/controllers/lettings_logs_controller.rb index bffac9efd..f67e94acc 100644 --- a/app/controllers/lettings_logs_controller.rb +++ b/app/controllers/lettings_logs_controller.rb @@ -112,41 +112,6 @@ class LettingsLogsController < LogsController end end - def delete_lettings_logs - @delete_logs_form = delete_logs_form - end - - def delete_lettings_logs_with_selected_ids - selected_ids = params.require(:selected_ids).split.map(&:to_i) - @delete_logs_form = delete_logs_form(selected_ids:) - render :delete_lettings_logs - end - - def delete_logs_confirmation - default_attributes = { - current_user:, - log_filters: @session_filters, - log_type: :lettings, - } - form_attributes = params.require(:forms_delete_logs_form).permit(:search_term, selected_ids: []) - attributes = form_attributes.merge(default_attributes) - attributes[:selected_ids] = [] unless attributes.key? :selected_ids - @delete_logs_form = Forms::DeleteLogsForm.new(attributes) - unless @delete_logs_form.valid? - render :delete_lettings_logs - end - end - - def delete_logs - logs = LettingsLog.find(params.require(:ids)) - logs.each do |log| - authorize log, :destroy? - log.discard! - end - - redirect_to lettings_logs_path, notice: I18n.t("notification.logs_deleted", count: logs.count) - end - private def session_filters @@ -163,10 +128,6 @@ private ) end - def delete_logs_form(selected_ids: nil) - Forms::DeleteLogsForm.new(current_user:, search_term:, log_filters: @session_filters, log_type: :lettings, selected_ids:) - end - def authenticate_scope! head :unauthorized and return if codes_only_export? && !current_user.support? end diff --git a/app/models/forms/delete_logs_form.rb b/app/models/forms/delete_logs_form.rb index fae4299bf..11b1c158e 100644 --- a/app/models/forms/delete_logs_form.rb +++ b/app/models/forms/delete_logs_form.rb @@ -1,6 +1,5 @@ module Forms class DeleteLogsForm - include Modules::LogsFilter include ActiveModel::Model include ActiveModel::Validations diff --git a/app/views/logs/delete_logs_confirmation.html.erb b/app/views/logs/delete_logs_confirmation.html.erb index 1953de9a9..bbd7c5cde 100644 --- a/app/views/logs/delete_logs_confirmation.html.erb +++ b/app/views/logs/delete_logs_confirmation.html.erb @@ -16,8 +16,8 @@ <% end %>
- <%= govuk_button_to "Delete logs", delete_logs_lettings_logs_path, method: "delete", params: { ids: @delete_logs_form.selected_ids } %> - <%= form_with url: delete_logs_lettings_logs_path do |f| %> + <%= govuk_button_to "Delete logs", @delete_path, method: "delete", params: { ids: @delete_logs_form.selected_ids } %> + <%= form_with url: @delete_path do |f| %> <%= f.hidden_field :selected_ids, value: @delete_logs_form.selected_ids %> <%= f.hidden_field :search, value: @delete_logs_form.search_term %> <%= f.govuk_submit "Cancel", secondary: true %> diff --git a/config/routes.rb b/config/routes.rb index 5d23bb7ed..7b32c86c7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -170,10 +170,10 @@ Rails.application.routes.draw do post "email-csv", to: "lettings_logs#email_csv" get "csv-confirmation", to: "lettings_logs#csv_confirmation" - get "delete-logs", to: "lettings_logs#delete_lettings_logs" - post "delete-logs", to: "lettings_logs#delete_lettings_logs_with_selected_ids" - post "delete-logs-confirmation", to: "lettings_logs#delete_logs_confirmation" - delete "delete-logs", to: "lettings_logs#delete_logs" + get "delete-logs", to: "delete_logs#delete_lettings_logs" + post "delete-logs", to: "delete_logs#delete_lettings_logs_with_selected_ids" + post "delete-logs-confirmation", to: "delete_logs#delete_lettings_logs_confirmation" + delete "delete-logs", to: "delete_logs#discard_lettings_logs" resources :bulk_upload_lettings_logs, path: "bulk-upload-logs", only: %i[show update] do collection do diff --git a/spec/requests/delete_logs_controller_spec.rb b/spec/requests/delete_logs_controller_spec.rb new file mode 100644 index 000000000..68c9aa4eb --- /dev/null +++ b/spec/requests/delete_logs_controller_spec.rb @@ -0,0 +1,262 @@ +require "rails_helper" + +RSpec.describe "DeleteLogs", type: :request do + let(:page) { Capybara::Node::Simple.new(response.body) } + + describe "GET delete-logs" do + let(:user) { create(:user, name: "Richard MacDuff") } + let!(:log_1) { create(:lettings_log, :in_progress, created_by: user) } + let!(:log_2) { create(:lettings_log, :completed, created_by: user) } + + before do + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + allow(FilterService).to receive(:filter_logs).and_return LettingsLog.all + end + + it "calls the filter service with the filters in the session and the search term from the query params" do + search = "Schrödinger's cat" + logs_filters = { + "years" => [""], + "status" => ["", "in_progress"], + "user" => "all", + } + get lettings_logs_path(logs_filters) # adds the filters to the session + + expect(FilterService).to receive(:filter_logs) { |arg1, arg2, arg3, _arg4, _arg5| + expect(arg1).to contain_exactly(log_1, log_2) + expect(arg2).to eq search + expect(arg3).to eq logs_filters + }.and_return LettingsLog.all + + get delete_logs_lettings_logs_path(search:) + end + + it "displays the logs returned by the filter service" do + get delete_logs_lettings_logs_path + + table_body_rows = page.find_all("tbody tr") + expect(table_body_rows.count).to be 2 + ids_in_table = table_body_rows.map { |row| row.first("td").text } + expect(ids_in_table).to match_array [log_1.id.to_s, log_2.id.to_s] + end + + it "checks all checkboxes by default" do + get delete_logs_lettings_logs_path + + checkboxes = page.find_all("tbody tr").map { |row| row.find("input") } + expect(checkboxes.count).to be 2 + expect(checkboxes).to all be_checked + end + end + + describe "POST delete-logs" do + let(:user) { create(:user, name: "Richard MacDuff") } + let!(:log_1) { create(:lettings_log, :in_progress, created_by: user) } + let!(:log_2) { create(:lettings_log, :completed, created_by: user) } + let(:selected_ids) { log_1.id } + + before do + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + allow(FilterService).to receive(:filter_logs).and_return LettingsLog.all + end + + it "throws an error if selected ids are not provided" do + expect { post delete_logs_lettings_logs_path }.to raise_error ActionController::ParameterMissing + end + + it "calls the filter service with the filters in the session and the search term from the query params" do + search = "Schrödinger's cat" + logs_filters = { + "years" => [""], + "status" => ["", "in_progress"], + "user" => "all", + } + get lettings_logs_path(logs_filters) # adds the filters to the session + + expect(FilterService).to receive(:filter_logs) { |arg1, arg2, arg3, _arg4, _arg5| + expect(arg1).to contain_exactly(log_1, log_2) + expect(arg2).to eq search + expect(arg3).to eq logs_filters + }.and_return LettingsLog.all + + post delete_logs_lettings_logs_path(search:, selected_ids:) + end + + it "displays the logs returned by the filter service" do + post delete_logs_lettings_logs_path(selected_ids:) + + table_body_rows = page.find_all("tbody tr") + expect(table_body_rows.count).to be 2 + ids_in_table = table_body_rows.map { |row| row.first("td").text } + expect(ids_in_table).to match_array [log_1.id.to_s, log_2.id.to_s] + end + + it "only checks the selected checkboxes when selected_ids provided" do + post delete_logs_lettings_logs_path(selected_ids:) + + checkboxes = page.find_all("tbody tr").map { |row| row.find("input") } + checkbox_expected_checked = checkboxes.find { |cb| cb.value == log_1.id.to_s } + checkbox_expected_unchecked = checkboxes.find { |cb| cb.value == log_2.id.to_s } + expect(checkbox_expected_checked).to be_checked + expect(checkbox_expected_unchecked).not_to be_checked + end + end + + describe "POST delete-logs-confirmation" do + let(:user) { create(:user, name: "Urban Chronotis") } + let(:log_1) { create(:lettings_log, :in_progress) } + let(:log_2) { create(:lettings_log, :completed) } + let(:log_3) { create(:lettings_log, :in_progress) } + let(:params) do + { + forms_delete_logs_form: { + search_term: "milk", + selected_ids: [log_1, log_2].map(&:id), + }, + } + end + + before do + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + post delete_logs_confirmation_lettings_logs_path, params: params + end + + it "requires delete logs form data to be provided" do + expect { post delete_logs_confirmation_lettings_logs_path }.to raise_error(ActionController::ParameterMissing) + end + + it "shows the correct title" do + expect(page.find("h1").text).to include "Are you sure you want to delete these logs?" + end + + it "shows the correct information text to the user" do + expect(page).to have_selector("p", text: "You've selected 2 logs to delete") + end + + context "when only one log is selected" do + let(:params) do + { + forms_delete_logs_form: { + search_term: "milk", + selected_ids: [log_1].map(&:id), + }, + } + end + + it "shows the correct information text to the user in the singular" do + post delete_logs_confirmation_lettings_logs_path, params: params + + expect(page).to have_selector("p", text: "You've selected 1 log to delete") + end + 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 logs" do + expect(page).to have_selector("form.button_to button", text: "Delete logs") + end + + it "the delete logs 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_logs_lettings_logs_path + expect(form_containing_button).to have_field "_method", type: :hidden, with: "delete" + expect(form_containing_button).to have_field "ids[]", type: :hidden, with: log_1.id + expect(form_containing_button).to have_field "ids[]", type: :hidden, with: log_2.id + end + + it "shows a cancel button with the correct style" do + expect(page).to have_selector("button.govuk-button--secondary", text: "Cancel") + end + + it "the cancel button submits the correct data to the correct path" do + form_containing_cancel = page.find_all("form").find { |form| form.has_selector?("button.govuk-button--secondary") } + expect(form_containing_cancel).to have_field("selected_ids", type: :hidden, with: [log_1, log_2].map(&:id).join(" ")) + expect(form_containing_cancel).to have_field("search", type: :hidden, with: "milk") + expect(form_containing_cancel[:method]).to eq "post" + expect(form_containing_cancel[:action]).to eq delete_logs_lettings_logs_path + end + + context "when no logs are selected" do + let(:params) do + { + forms_delete_logs_form: { + log_type: :lettings, + log_ids: [log_1, log_2, log_3].map(&:id).join(" "), + }, + } + end + + before do + post delete_logs_confirmation_lettings_logs_path, params: params + end + + it "renders the list of logs table again" do + expect(page.find("h1").text).to include "Review the logs you want to delete" + end + + it "displays an error message" do + expect(page).to have_selector(".govuk-error-summary", text: "Select at least one log to delete or press cancel to return") + end + + it "renders the table with all checkboxes unchecked" do + checkboxes = page.find_all("tbody tr").map { |row| row.find("input") } + checkboxes.each do |checkbox| + expect(checkbox).not_to be_checked + end + end + end + end + + describe "DELETE delete-logs" do + let(:urban_chronotis) { create(:user, :data_provider, name: "Urban Chronotis") } + let(:log_1) { create(:lettings_log, :in_progress, created_by: urban_chronotis) } + let(:params) { { ids: [log_1.id, log_2.id] } } + + before do + allow(urban_chronotis).to receive(:need_two_factor_authentication?).and_return(false) + sign_in urban_chronotis + end + + context "when the user is authorized to delete the logs provided" do + let(:log_2) { create(:lettings_log, :completed, created_by: urban_chronotis) } + + it "deletes the logs provided" do + delete delete_logs_lettings_logs_path, params: params + log_1.reload + expect(log_1.status).to eq "deleted" + expect(log_1.discarded_at).not_to be nil + log_2.reload + expect(log_2.status).to eq "deleted" + expect(log_2.discarded_at).not_to be nil + end + + it "redirects to the lettings log index and displays a notice that the logs have been deleted" do + delete delete_logs_lettings_logs_path, params: params + expect(response).to redirect_to lettings_logs_path + follow_redirect! + expect(page).to have_selector(".govuk-notification-banner--success") + expect(page).to have_selector(".govuk-notification-banner--success", text: "2 logs have been deleted") + end + end + + context "when the user is not authorized to delete all the logs provided" do + let(:log_2) { create(:lettings_log, :completed) } + + it "returns unauthorised and only deletes logs for which the user is authorised" do + delete delete_logs_lettings_logs_path, params: params + expect(response).to have_http_status(:unauthorized) + log_1.reload + expect(log_1.status).to eq "deleted" + expect(log_1.discarded_at).not_to be nil + log_2.reload + expect(log_2.discarded_at).to be nil + end + end + end +end diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index 515b80be0..8db09e9a6 100644 --- a/spec/requests/lettings_logs_controller_spec.rb +++ b/spec/requests/lettings_logs_controller_spec.rb @@ -1841,265 +1841,4 @@ RSpec.describe LettingsLogsController, type: :request do end end end - - describe "GET delete-logs" do - let(:page) { Capybara::Node::Simple.new(response.body) } - let(:user) { create(:user, name: "Richard MacDuff") } - let!(:log_1) { create(:lettings_log, :in_progress, created_by: user) } - let!(:log_2) { create(:lettings_log, :completed, created_by: user) } - - before do - allow(user).to receive(:need_two_factor_authentication?).and_return(false) - sign_in user - allow(FilterService).to receive(:filter_logs).and_return LettingsLog.all - end - - it "calls the filter service with the filters in the session and the search term from the query params" do - search = "Schrödinger's cat" - logs_filters = { - "years" => [""], - "status" => ["", "in_progress"], - "user" => "all", - } - get lettings_logs_path(logs_filters) # adds the filters to the session - - expect(FilterService).to receive(:filter_logs) { |arg1, arg2, arg3, _arg4, _arg5| - expect(arg1).to contain_exactly(log_1, log_2) - expect(arg2).to eq search - expect(arg3).to eq logs_filters - }.and_return LettingsLog.all - - get delete_logs_lettings_logs_path(search:) - end - - it "displays the logs returned by the filter service" do - get delete_logs_lettings_logs_path - - table_body_rows = page.find_all("tbody tr") - expect(table_body_rows.count).to be 2 - ids_in_table = table_body_rows.map { |row| row.first("td").text } - expect(ids_in_table).to match_array [log_1.id.to_s, log_2.id.to_s] - end - - it "checks all checkboxes by default" do - get delete_logs_lettings_logs_path - - checkboxes = page.find_all("tbody tr").map { |row| row.find("input") } - expect(checkboxes.count).to be 2 - expect(checkboxes).to all be_checked - end - end - - describe "POST delete-logs" do - let(:page) { Capybara::Node::Simple.new(response.body) } - let(:user) { create(:user, name: "Richard MacDuff") } - let!(:log_1) { create(:lettings_log, :in_progress, created_by: user) } - let!(:log_2) { create(:lettings_log, :completed, created_by: user) } - let(:selected_ids) { log_1.id } - - before do - allow(user).to receive(:need_two_factor_authentication?).and_return(false) - sign_in user - allow(FilterService).to receive(:filter_logs).and_return LettingsLog.all - end - - it "throws an error if selected ids are not provided" do - expect { post delete_logs_lettings_logs_path }.to raise_error ActionController::ParameterMissing - end - - it "calls the filter service with the filters in the session and the search term from the query params" do - search = "Schrödinger's cat" - logs_filters = { - "years" => [""], - "status" => ["", "in_progress"], - "user" => "all", - } - get lettings_logs_path(logs_filters) # adds the filters to the session - - expect(FilterService).to receive(:filter_logs) { |arg1, arg2, arg3, _arg4, _arg5| - expect(arg1).to contain_exactly(log_1, log_2) - expect(arg2).to eq search - expect(arg3).to eq logs_filters - }.and_return LettingsLog.all - - post delete_logs_lettings_logs_path(search:, selected_ids:) - end - - it "displays the logs returned by the filter service" do - post delete_logs_lettings_logs_path(selected_ids:) - - table_body_rows = page.find_all("tbody tr") - expect(table_body_rows.count).to be 2 - ids_in_table = table_body_rows.map { |row| row.first("td").text } - expect(ids_in_table).to match_array [log_1.id.to_s, log_2.id.to_s] - end - - it "only checks the selected checkboxes when selected_ids provided" do - post delete_logs_lettings_logs_path(selected_ids:) - - checkboxes = page.find_all("tbody tr").map { |row| row.find("input") } - checkbox_expected_checked = checkboxes.find { |cb| cb.value == log_1.id.to_s } - checkbox_expected_unchecked = checkboxes.find { |cb| cb.value == log_2.id.to_s } - expect(checkbox_expected_checked).to be_checked - expect(checkbox_expected_unchecked).not_to be_checked - end - end - - describe "POST delete-logs-confirmation" do - let(:page) { Capybara::Node::Simple.new(response.body) } - let(:user) { create(:user, name: "Urban Chronotis") } - let(:log_1) { create(:lettings_log, :in_progress) } - let(:log_2) { create(:lettings_log, :completed) } - let(:log_3) { create(:lettings_log, :in_progress) } - let(:params) do - { - forms_delete_logs_form: { - search_term: "milk", - selected_ids: [log_1, log_2].map(&:id), - }, - } - end - - before do - allow(user).to receive(:need_two_factor_authentication?).and_return(false) - sign_in user - post delete_logs_confirmation_lettings_logs_path, params: params - end - - it "requires delete logs form data to be provided" do - expect { post delete_logs_confirmation_lettings_logs_path }.to raise_error(ActionController::ParameterMissing) - end - - it "shows the correct title" do - expect(page.find("h1").text).to include "Are you sure you want to delete these logs?" - end - - it "shows the correct information text to the user" do - expect(page).to have_selector("p", text: "You've selected 2 logs to delete") - end - - context "when only one log is selected" do - let(:params) do - { - forms_delete_logs_form: { - search_term: "milk", - selected_ids: [log_1].map(&:id), - }, - } - end - - it "shows the correct information text to the user in the singular" do - post delete_logs_confirmation_lettings_logs_path, params: params - - expect(page).to have_selector("p", text: "You've selected 1 log to delete") - end - 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 logs" do - expect(page).to have_selector("form.button_to button", text: "Delete logs") - end - - it "the delete logs 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_logs_lettings_logs_path - expect(form_containing_button).to have_field "_method", type: :hidden, with: "delete" - expect(form_containing_button).to have_field "ids[]", type: :hidden, with: log_1.id - expect(form_containing_button).to have_field "ids[]", type: :hidden, with: log_2.id - end - - it "shows a cancel button with the correct style" do - expect(page).to have_selector("button.govuk-button--secondary", text: "Cancel") - end - - it "the cancel button submits the correct data to the correct path" do - form_containing_cancel = page.find_all("form").find { |form| form.has_selector?("button.govuk-button--secondary") } - expect(form_containing_cancel).to have_field("selected_ids", type: :hidden, with: [log_1, log_2].map(&:id).join(" ")) - expect(form_containing_cancel).to have_field("search", type: :hidden, with: "milk") - expect(form_containing_cancel[:method]).to eq "post" - expect(form_containing_cancel[:action]).to eq delete_logs_lettings_logs_path - end - - context "when no logs are selected" do - let(:params) do - { - forms_delete_logs_form: { - log_type: :lettings, - log_ids: [log_1, log_2, log_3].map(&:id).join(" "), - }, - } - end - - before do - post delete_logs_confirmation_lettings_logs_path, params: params - end - - it "renders the list of logs table again" do - expect(page.find("h1").text).to include "Review the logs you want to delete" - end - - it "displays an error message" do - expect(page).to have_selector(".govuk-error-summary", text: "Select at least one log to delete or press cancel to return") - end - - it "renders the table with all checkboxes unchecked" do - checkboxes = page.find_all("tbody tr").map { |row| row.find("input") } - checkboxes.each do |checkbox| - expect(checkbox).not_to be_checked - end - end - end - end - - describe "DELETE delete-logs" do - let(:page) { Capybara::Node::Simple.new(response.body) } - let(:urban_chronotis) { create(:user, :data_provider, name: "Urban Chronotis") } - let(:log_1) { create(:lettings_log, :in_progress, created_by: urban_chronotis) } - let(:params) { { ids: [log_1.id, log_2.id] } } - - before do - allow(urban_chronotis).to receive(:need_two_factor_authentication?).and_return(false) - sign_in urban_chronotis - end - - context "when the user is authorized to delete the logs provided" do - let(:log_2) { create(:lettings_log, :completed, created_by: urban_chronotis) } - - it "deletes the logs provided" do - delete delete_logs_lettings_logs_path, params: params - log_1.reload - expect(log_1.status).to eq "deleted" - expect(log_1.discarded_at).not_to be nil - log_2.reload - expect(log_2.status).to eq "deleted" - expect(log_2.discarded_at).not_to be nil - end - - it "redirects to the lettings log index and displays a notice that the logs have been deleted" do - delete delete_logs_lettings_logs_path, params: params - expect(response).to redirect_to lettings_logs_path - follow_redirect! - expect(page).to have_selector(".govuk-notification-banner--success") - expect(page).to have_selector(".govuk-notification-banner--success", text: "2 logs have been deleted") - end - end - - context "when the user is not authorized to delete all the logs provided" do - let(:log_2) { create(:lettings_log, :completed) } - - it "returns unauthorised and only deletes logs for which the user is authorised" do - delete delete_logs_lettings_logs_path, params: params - expect(response).to have_http_status(:unauthorized) - log_1.reload - expect(log_1.status).to eq "deleted" - expect(log_1.discarded_at).not_to be nil - log_2.reload - expect(log_2.discarded_at).to be nil - end - end - end end