From e44aa5a8d2a094adc07981c9e514e0b771556d9d Mon Sep 17 00:00:00 2001 From: Arthur Campbell Date: Thu, 1 Jun 2023 13:22:28 +0100 Subject: [PATCH] comprehensive reworking after code review ensure that we are not passing lists of ids through params in the query string, risking overflowing the maximum URL length, adjust tests accordingly, do not attempt to reuse the same table for sales and lettings --- app/controllers/lettings_logs_controller.rb | 41 +++++- app/frontend/styles/application.scss | 1 - app/helpers/log_list_helper.rb | 6 +- app/models/forms/delete_logs_form.rb | 38 ++++++ app/views/logs/_log_list.html.erb | 4 +- app/views/logs/delete_lettings_logs.html.erb | 48 +++++++ .../logs/delete_logs_confirmation.html.erb | 25 ++++ app/views/logs/index.html.erb | 5 +- app/views/organisations/logs.html.erb | 1 + config/locales/en.yml | 5 + config/routes.rb | 9 ++ spec/features/lettings_log_spec.rb | 8 +- .../requests/lettings_logs_controller_spec.rb | 122 +++++++++++++----- spec/views/logs/delete_lettings_logs_spec.rb | 72 +++++++++++ spec/views/logs/delete_logs_spec.rb | 102 --------------- 15 files changed, 342 insertions(+), 145 deletions(-) create mode 100644 app/models/forms/delete_logs_form.rb create mode 100644 app/views/logs/delete_lettings_logs.html.erb create mode 100644 app/views/logs/delete_logs_confirmation.html.erb create mode 100644 spec/views/logs/delete_lettings_logs_spec.rb delete mode 100644 spec/views/logs/delete_logs_spec.rb diff --git a/app/controllers/lettings_logs_controller.rb b/app/controllers/lettings_logs_controller.rb index eb1caa47c..bffac9efd 100644 --- a/app/controllers/lettings_logs_controller.rb +++ b/app/controllers/lettings_logs_controller.rb @@ -16,7 +16,7 @@ class LettingsLogsController < LogsController all_logs = current_user.lettings_logs.visible unpaginated_filtered_logs = filter_manager.filtered_logs(all_logs, search_term, session_filters) - @search_term = search_term + @delete_logs_path = delete_logs_lettings_logs_path(search: search_term) @pagy, @logs = pagy(unpaginated_filtered_logs) @searched = search_term.presence @total_count = all_logs.size @@ -112,6 +112,41 @@ 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 @@ -128,6 +163,10 @@ 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/frontend/styles/application.scss b/app/frontend/styles/application.scss index 2bd4fc487..c95c466b9 100644 --- a/app/frontend/styles/application.scss +++ b/app/frontend/styles/application.scss @@ -44,7 +44,6 @@ $govuk-breakpoints: ( @import "search"; @import "sub-navigation"; @import "errors"; -@import "log-list"; // App utilities .app-\!-colour-muted { diff --git a/app/helpers/log_list_helper.rb b/app/helpers/log_list_helper.rb index e5d705066..881ebdfef 100644 --- a/app/helpers/log_list_helper.rb +++ b/app/helpers/log_list_helper.rb @@ -1,9 +1,9 @@ module LogListHelper - def display_delete_logs? - if @current_user.data_provider? + def display_delete_logs?(current_user, search_term) + if current_user.data_provider? filter_selected?("user", "yours") else - any_filter_selected? || @searched.present? + any_filter_selected? || search_term.present? end end end diff --git a/app/models/forms/delete_logs_form.rb b/app/models/forms/delete_logs_form.rb new file mode 100644 index 000000000..fae4299bf --- /dev/null +++ b/app/models/forms/delete_logs_form.rb @@ -0,0 +1,38 @@ +module Forms + class DeleteLogsForm + include Modules::LogsFilter + include ActiveModel::Model + include ActiveModel::Validations + + attr_reader :logs, :log_type, :selected_ids, :search_term + + validate :at_least_one_log_selected + + def initialize(attributes) + @log_type = attributes[:log_type] + @search_term = attributes[:search_term] + @current_user = attributes[:current_user] + @logs = FilterService.filter_logs(all_logs, @search_term, attributes[:log_filters], nil, @current_user) + @selected_ids = attributes[:selected_ids] || @logs.map(&:id) + end + + def log_count + @logs.count + end + + private + + def at_least_one_log_selected + if selected_ids.blank? || selected_ids.reject(&:blank?).blank? + errors.add(:log_ids, "Select at least one log to delete or press cancel to return") + end + end + + def all_logs + case @log_type + when :lettings then @current_user.lettings_logs.visible + when :sales then @current_user.sales_logs.visible + end + end + end +end diff --git a/app/views/logs/_log_list.html.erb b/app/views/logs/_log_list.html.erb index a6b97c3f4..d3a6482e8 100644 --- a/app/views/logs/_log_list.html.erb +++ b/app/views/logs/_log_list.html.erb @@ -10,8 +10,8 @@ <% end %>
- <% if display_delete_logs? %> - <%= govuk_link_to "Delete logs", "#", class: "app-!-colour-red" %> + <% if logs&.any? && display_delete_logs?(@current_user, @searched) %> + <%= govuk_link_to "Delete logs", delete_logs_path, class: "app-!-colour-red" %> <% end %>
diff --git a/app/views/logs/delete_lettings_logs.html.erb b/app/views/logs/delete_lettings_logs.html.erb new file mode 100644 index 000000000..8ccab7578 --- /dev/null +++ b/app/views/logs/delete_lettings_logs.html.erb @@ -0,0 +1,48 @@ +<% title = "Delete logs" %> +<% content_for :title, title %> +<% content_for :before_content do %> + <%= govuk_back_link(href: :back) %> +<% end %> + +

+ <%= title %> + Review the logs you want to delete +

+

You've selected <%= @delete_logs_form.log_count %> <%= "log".pluralize(@delete_logs_form.log_count) %> to delete

+ +<%= form_with model: @delete_logs_form, url: delete_logs_confirmation_lettings_logs_path do |f| %> + <%= f.hidden_field :search_term, value: @delete_logs_form.search_term %> + <%= f.govuk_error_summary %> + <%= govuk_table do |table| %> + <% table.head do |head| %> + <% head.row do |row| %> + <% row.cell(header: true, text: "Log ID") %> + <% row.cell(header: true, text: "Tenancy code") %> + <% row.cell(header: true, text: "Property reference") %> + <% row.cell(header: true, text: "Status") %> + <% row.cell(header: true, text: "Delete?") %> + <% end %> + <% end %> + <% table.body do |body| %> + <% f.govuk_check_boxes_fieldset :selected_ids, small: true do %> + <% @delete_logs_form.logs.each do |log| %> + <% body.row do |row| %> + <% row.cell(text: log.id) %> + <% row.cell(text: log.tenancycode) %> + <% row.cell(text: log.propcode) %> + <% row.cell(text: status_tag(log.status)) %> + <% row.cell do %> + <% f.govuk_check_box :selected_ids, log.id, + label: { text: log.id, hidden: true }, + size: "s", + checked: @delete_logs_form.selected_ids.include?(log.id) %> + <% end %> + <% end %> + <% end %> + <% end %> + <% end %> + <% end %> + <%= f.govuk_submit "Continue" do %> + <%= govuk_button_link_to "Cancel", lettings_logs_path(search: @delete_logs_form.search_term), secondary: true %> + <% end %> +<% end %> diff --git a/app/views/logs/delete_logs_confirmation.html.erb b/app/views/logs/delete_logs_confirmation.html.erb new file mode 100644 index 000000000..1953de9a9 --- /dev/null +++ b/app/views/logs/delete_logs_confirmation.html.erb @@ -0,0 +1,25 @@ +<% title = "Delete logs" %> +<% content_for :title, title %> +<% content_for :before_content do %> + <%= govuk_back_link(href: :back) %> +<% end %> + +

+ <%= title %> + Are you sure you want to delete these logs? +

+<% log_count = @delete_logs_form.selected_ids.count %> +

You've selected <%= log_count %> <%= "log".pluralize(log_count) %> to delete

+ +<%= govuk_warning_text(icon_fallback_text: "Danger") do %> + You will not be able to undo this action +<% 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| %> + <%= 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 %> + <% end %> +
diff --git a/app/views/logs/index.html.erb b/app/views/logs/index.html.erb index e2cc2e0d1..7a9b34962 100644 --- a/app/views/logs/index.html.erb +++ b/app/views/logs/index.html.erb @@ -68,8 +68,9 @@ searched: @searched, item_label:, total_count: @total_count, - csv_download_url: csv_download_url_for_controller(controller:, search: @search_term, codes_only: false), - csv_codes_only_download_url: csv_download_url_for_controller(controller:, search: @search_term, codes_only: true), + csv_download_url: csv_download_url_for_controller(controller:, search: @searched, codes_only: false), + csv_codes_only_download_url: csv_download_url_for_controller(controller:, search: @searched, codes_only: true), + delete_logs_path: @delete_logs_path, } %> <%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "logs" } %> diff --git a/app/views/organisations/logs.html.erb b/app/views/organisations/logs.html.erb index 6b269590c..5820398e3 100644 --- a/app/views/organisations/logs.html.erb +++ b/app/views/organisations/logs.html.erb @@ -34,6 +34,7 @@ total_count: @total_count, csv_download_url: csv_download_url_by_log_type(@log_type, @organisation, search: @search_term, codes_only: false), csv_codes_only_download_url: csv_download_url_by_log_type(@log_type, @organisation, search: @search_term, codes_only: true), + delete_logs_path: "#", } %> <%= render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "logs" } %> diff --git a/config/locales/en.yml b/config/locales/en.yml index f5d8a56b7..e1635e748 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -175,6 +175,11 @@ en: new_organisation_telephone_number: blank: "Enter a valid telephone number" + notification: + logs_deleted: + one: "%{count} log has been deleted" + other: "%{count} logs have been deleted" + validations: organisation: data_sharing_agreement_not_signed: Your organisation must accept the Data Sharing Agreement before you can create any logs. diff --git a/config/routes.rb b/config/routes.rb index 9dd9b7325..5d23bb7ed 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -170,6 +170,11 @@ 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" + resources :bulk_upload_lettings_logs, path: "bulk-upload-logs", only: %i[show update] do collection do get :start @@ -227,6 +232,10 @@ Rails.application.routes.draw do post "email-csv", to: "sales_logs#email_csv" get "csv-confirmation", to: "sales_logs#csv_confirmation" + get "delete-logs", to: "sales_logs#delete_logs_list" + get "delete-logs-confirmation", to: "sales_logs#delete_logs_confirmation" + delete "delete-logs", to: "sales_logs#delete_logs" + resources :bulk_upload_sales_logs, path: "bulk-upload-logs" do collection do get :start diff --git a/spec/features/lettings_log_spec.rb b/spec/features/lettings_log_spec.rb index 9b848f3b6..f17ad1093 100644 --- a/spec/features/lettings_log_spec.rb +++ b/spec/features/lettings_log_spec.rb @@ -245,13 +245,13 @@ RSpec.describe "Lettings Log Features" do expect(rows.count).to be 2 id_to_delete, id_to_keep = rows.map { |row| row.first("td").text.to_i } expect([id_to_delete, id_to_keep]).to match_array [lettings_log_1.id, lettings_log_2.id] - check "forms-delete-logs-form-logs-to-delete-#{id_to_delete}-field" - uncheck "forms-delete-logs-form-logs-to-delete-#{id_to_keep}-field" + check "forms-delete-logs-form-selected-ids-#{id_to_delete}-field" + uncheck "forms-delete-logs-form-selected-ids-#{id_to_keep}-field" click_button "Continue" - expect(page).to have_current_path delete_logs_confirmation_lettings_logs_path, ignore_query: true + expect(page).to have_current_path delete_logs_confirmation_lettings_logs_path expect(page.text).to include "You've selected 1 log to delete" - expect(page.find("input#ids", visible: false).value.to_i).to be id_to_delete + expect(page.find("form.button_to")[:action]).to eq delete_logs_lettings_logs_path click_button "Delete logs" expect(page).to have_current_path lettings_logs_path diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index 79e92b299..515b80be0 100644 --- a/spec/requests/lettings_logs_controller_spec.rb +++ b/spec/requests/lettings_logs_controller_spec.rb @@ -1845,21 +1845,35 @@ RSpec.describe LettingsLogsController, type: :request do 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) } - let(:log_2) { create(:lettings_log, :completed) } + 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 - create(:lettings_log, :in_progress) + allow(FilterService).to receive(:filter_logs).and_return LettingsLog.all end - it "requires log_ids to be provided" do - expect { get delete_logs_lettings_logs_path }.to raise_error(ActionController::ParameterMissing) + 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 for the ids provided" do - get delete_logs_lettings_logs_path(log_ids: [log_1.id, log_2.id]) + 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 @@ -1868,14 +1882,60 @@ RSpec.describe LettingsLogsController, type: :request do end it "checks all checkboxes by default" do - get delete_logs_lettings_logs_path(log_ids: [log_1.id, log_2.id]) + 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 - get delete_logs_lettings_logs_path(log_ids: [log_1.id, log_2.id], selected_ids: [log_1.id]) + 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 } @@ -1885,7 +1945,7 @@ RSpec.describe LettingsLogsController, type: :request do end end - describe "GET delete-logs-confirmation" do + 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) } @@ -1894,9 +1954,8 @@ RSpec.describe LettingsLogsController, type: :request do let(:params) do { forms_delete_logs_form: { - log_type: :lettings, - log_ids: [log_1, log_2, log_3].map(&:id).join(" "), - logs_to_delete: [log_1, log_2].map(&:id), + search_term: "milk", + selected_ids: [log_1, log_2].map(&:id), }, } end @@ -1904,11 +1963,11 @@ RSpec.describe LettingsLogsController, type: :request do before do allow(user).to receive(:need_two_factor_authentication?).and_return(false) sign_in user - get delete_logs_confirmation_lettings_logs_path, params: params + post delete_logs_confirmation_lettings_logs_path, params: params end it "requires delete logs form data to be provided" do - expect { get delete_logs_confirmation_lettings_logs_path }.to raise_error(ActionController::ParameterMissing) + expect { post delete_logs_confirmation_lettings_logs_path }.to raise_error(ActionController::ParameterMissing) end it "shows the correct title" do @@ -1923,15 +1982,14 @@ RSpec.describe LettingsLogsController, type: :request do let(:params) do { forms_delete_logs_form: { - log_type: :lettings, - log_ids: [log_1, log_2, log_3].map(&:id).join(" "), - logs_to_delete: [log_1.id], + search_term: "milk", + selected_ids: [log_1].map(&:id), }, } end it "shows the correct information text to the user in the singular" do - get delete_logs_confirmation_lettings_logs_path, params: params + post delete_logs_confirmation_lettings_logs_path, params: params expect(page).to have_selector("p", text: "You've selected 1 log to delete") end @@ -1948,19 +2006,22 @@ RSpec.describe LettingsLogsController, type: :request do 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(ids: params[:forms_delete_logs_form][:logs_to_delete]) - expect(form_containing_button[:method]).to eq "post" + 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("a.govuk-button.govuk-button--secondary", text: "Cancel") + 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 - cancel_button = page.find("a.govuk-button.govuk-button--secondary", text: "Cancel") - expected_path = delete_logs_lettings_logs_path(log_ids: params[:forms_delete_logs_form][:log_ids].split, selected_ids: params[:forms_delete_logs_form][:logs_to_delete]) - - expect(cancel_button[:href]).to eq expected_path + 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 @@ -1974,7 +2035,7 @@ RSpec.describe LettingsLogsController, type: :request do end before do - get delete_logs_confirmation_lettings_logs_path, params: params + post delete_logs_confirmation_lettings_logs_path, params: params end it "renders the list of logs table again" do @@ -2027,14 +2088,15 @@ RSpec.describe LettingsLogsController, type: :request do end end - context "when the user is not authorized to delete the logs provided" do + 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 does not delete logs" do + 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.discarded_at).to be nil + 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 diff --git a/spec/views/logs/delete_lettings_logs_spec.rb b/spec/views/logs/delete_lettings_logs_spec.rb new file mode 100644 index 000000000..8abc489aa --- /dev/null +++ b/spec/views/logs/delete_lettings_logs_spec.rb @@ -0,0 +1,72 @@ +require "rails_helper" + +RSpec.describe "logs/delete_lettings_logs.html.erb" do + let(:user) { create(:user, :support, name: "Dirk Gently") } + let(:lettings_log_1) { create(:lettings_log, tenancycode: "Holistic", propcode: "Detective Agency", created_by: user) } + let(:lettings_logs) { [lettings_log_1] } + let(:delete_logs_form) { Forms::DeleteLogsForm.new(log_type: :lettings, current_user: user) } + + before do + sign_in user + allow(FilterService).to receive(:filter_logs).and_return lettings_logs + assign(:delete_logs_form, delete_logs_form) + end + + it "has the correct h1 content" do + render + fragment = Capybara::Node::Simple.new(rendered) + h1 = fragment.find("h1") + expect(h1.text).to include "Review the logs you want to delete" + end + + context "when there is one log to delete" do + it "shows the informative text in the singular" do + render + fragment = Capybara::Node::Simple.new(rendered) + info_text = fragment.first("p").text + expect(info_text).to eq "You've selected 1 log to delete" + end + end + + context "when there is more than one log to delete" do + let(:lettings_log_2) { create(:lettings_log, tenancycode: "01-354", propcode: "9112") } + + before do + lettings_logs << lettings_log_2 + allow(FilterService).to receive(:filter_logs).and_return lettings_logs + delete_logs_form = Forms::DeleteLogsForm.new(log_type: :lettings, current_user: user) + assign(:delete_logs_form, delete_logs_form) + end + + it "shows the informative text in the plural" do + render + fragment = Capybara::Node::Simple.new(rendered) + info_text = fragment.first("p").text + expect(info_text).to eq "You've selected #{lettings_logs.count} logs to delete" + end + end + + it "shows the correct headers in the table" do + render + fragment = Capybara::Node::Simple.new(rendered) + headers = fragment.find_all("table thead tr th").map(&:text) + expect(headers).to eq ["Log ID", "Tenancy code", "Property reference", "Status", "Delete?"] + end + + it "shows the correct information in each row" do + render + fragment = Capybara::Node::Simple.new(rendered) + row_data = fragment.find_all("table tbody tr td").map(&:text)[0...-1] + expect(row_data).to eq [lettings_log_1.id.to_s, lettings_log_1.tenancycode, lettings_log_1.propcode, lettings_log_1.status.humanize.capitalize] + end + + it "shows a checkbox with the correct hidden label in the final cell of each row" do + render + fragment = Capybara::Node::Simple.new(rendered) + final_cell = fragment.find_all("table tbody tr td")[-1] + expect(final_cell.find("input")[:type]).to eq "checkbox" + checkbox_label = final_cell.find("label span") + expect(checkbox_label.text).to eq lettings_log_1.id.to_s + expect(checkbox_label[:class]).to include "govuk-visually-hidden" + end +end diff --git a/spec/views/logs/delete_logs_spec.rb b/spec/views/logs/delete_logs_spec.rb deleted file mode 100644 index 52a3e7064..000000000 --- a/spec/views/logs/delete_logs_spec.rb +++ /dev/null @@ -1,102 +0,0 @@ -require "rails_helper" - -RSpec.describe "logs/delete_logs.html.erb" do - let(:lettings_log_1) { create(:lettings_log, tenancycode: "Holistic", propcode: "Detective Agency") } - let(:lettings_logs) { [lettings_log_1] } - let(:delete_logs_form) { Forms::DeleteLogsForm.new(log_ids: lettings_logs.map(&:id), log_type: :lettings) } - - before do - sign_in create(:user, :support, name: "Dirk Gently") - assign(:delete_logs_form, delete_logs_form) - end - - it "has the correct h1 content" do - render - fragment = Capybara::Node::Simple.new(rendered) - h1 = fragment.find("h1") - expect(h1.text).to include "Review the logs you want to delete" - end - - context "when there is one log to delete" do - it "shows the informative text in the singular" do - render - fragment = Capybara::Node::Simple.new(rendered) - info_text = fragment.first("p").text - expect(info_text).to eq "You've selected 1 log to delete" - end - end - - context "when there is more than one log to delete" do - let(:lettings_log_2) { create(:lettings_log, tenancycode: "01-354", propcode: "9112") } - - before do - lettings_logs << lettings_log_2 - delete_logs_form = Forms::DeleteLogsForm.new(log_ids: lettings_logs.map(&:id), log_type: :lettings) - assign(:delete_logs_form, delete_logs_form) - end - - it "shows the informative text in the plural" do - render - fragment = Capybara::Node::Simple.new(rendered) - info_text = fragment.first("p").text - expect(info_text).to eq "You've selected #{lettings_logs.count} logs to delete" - end - end - - context "when the user is deletings lettings logs" do - it "shows the correct headers in the table" do - render - fragment = Capybara::Node::Simple.new(rendered) - headers = fragment.find_all("table thead tr th").map(&:text) - expect(headers).to eq ["Log ID", "Tenancy code", "Property reference", "Status", "Delete?"] - end - - it "shows the correct information in each row" do - render - fragment = Capybara::Node::Simple.new(rendered) - row_data = fragment.find_all("table tbody tr td").map(&:text)[0...-1] - expect(row_data).to eq [lettings_log_1.id.to_s, lettings_log_1.tenancycode, lettings_log_1.propcode, lettings_log_1.status.humanize.capitalize] - end - - it "shows a checkbox with the correct hidden label in the final cell of each row" do - render - fragment = Capybara::Node::Simple.new(rendered) - final_cell = fragment.find_all("table tbody tr td")[-1] - expect(final_cell.find("input")[:type]).to eq "checkbox" - checkbox_label = final_cell.find("label span") - expect(checkbox_label.text).to eq lettings_log_1.id.to_s - expect(checkbox_label[:class]).to include "govuk-visually-hidden" - end - end - - context "when the user is deletings sales logs" do - let(:organisation) { create(:organisation, name: "awgan-eye-zay-shun") } - let(:sales_log_1) { create(:sales_log, owning_organisation: organisation) } - let(:sales_logs) { [sales_log_1] } - let(:delete_logs_form) { Forms::DeleteLogsForm.new(log_ids: sales_logs.map(&:id), log_type: :sales) } - - it "shows the correct headers in the table" do - render - fragment = Capybara::Node::Simple.new(rendered) - headers = fragment.find_all("table thead tr th").map(&:text) - expect(headers).to eq ["Log ID", "Owning organisation", "Created", "Status", "Delete?"] - end - - it "shows the correct information in each row" do - render - fragment = Capybara::Node::Simple.new(rendered) - row_data = fragment.find_all("table tbody tr td").map(&:text)[0...-1] - expect(row_data).to eq [sales_log_1.id.to_s, sales_log_1.owning_organisation.name, sales_log_1.created_at.to_formatted_s(:govuk_date), sales_log_1.status.humanize.capitalize] - end - - it "shows a checkbox with the correct hidden label in the final cell of each row" do - render - fragment = Capybara::Node::Simple.new(rendered) - final_cell = fragment.find_all("table tbody tr td")[-1] - expect(final_cell.find("input")[:type]).to eq "checkbox" - checkbox_label = final_cell.find("label span") - expect(checkbox_label.text).to eq sales_log_1.id.to_s - expect(checkbox_label[:class]).to include "govuk-visually-hidden" - end - end -end