diff --git a/app/controllers/lettings_logs_controller.rb b/app/controllers/lettings_logs_controller.rb index ebaf9e64c..d940c8af9 100644 --- a/app/controllers/lettings_logs_controller.rb +++ b/app/controllers/lettings_logs_controller.rb @@ -1,5 +1,5 @@ class LettingsLogsController < LogsController - before_action :find_resource, except: %i[create index edit] + before_action :find_resource, except: %i[create index edit delete_confirmation] before_action :session_filters, if: :current_user, only: %i[index email_csv download_csv] before_action :set_session_filters, if: :current_user, only: %i[index email_csv download_csv] before_action :authenticate_scope!, only: %i[download_csv email_csv] @@ -68,17 +68,27 @@ class LettingsLogsController < LogsController end def destroy - if @log - if @log.delete - head :no_content - else - render json: { errors: @log.errors.messages }, status: :unprocessable_entity - end + render_not_found and return unless @log + + authorize @log, policy_class: LogPolicy + + if @log.delete + redirect_to lettings_logs_path, notice: "Log #{@log.id} has been deleted" else - render_not_found_json("Log", params[:id]) + render_not_found end end + def delete_confirmation + @log = LettingsLog.visible.find_by(id: params[:lettings_log_id]) + + render_not_found and return unless @log + + authorize @log, :destroy?, policy_class: LogPolicy + + render "logs/delete_confirmation" + end + def download_csv unpaginated_filtered_logs = filtered_logs(current_user.lettings_logs, search_term, @session_filters) diff --git a/app/views/logs/delete_confirmation.html.erb b/app/views/logs/delete_confirmation.html.erb new file mode 100644 index 000000000..e92d25ec2 --- /dev/null +++ b/app/views/logs/delete_confirmation.html.erb @@ -0,0 +1,31 @@ +<% content_for :before_content do %> + <% content_for :title, "Are you sure you want to delete this log?" %> + <%= govuk_back_link href: @log.lettings? ? lettings_logs_path : sales_logs_path %> +<% end %> + +
+
+ Delete log <%= @log.id %> +

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

+ + <%= govuk_warning_text(text: "You will not be able to undo this action.") %> + +
+ <%= govuk_button_to( + "Delete this log", + @log.lettings? ? lettings_log_path(@log) : sales_log_path(@log), + class: "govuk-!-margin-right-6", + method: :delete + ) %> + <%= govuk_button_to( + "Cancel", + @log.lettings? ? lettings_log_path(@log) : sales_log_path(@log), + button: true, + method: :get, + secondary: true, + ) %> +
+
+
diff --git a/app/views/logs/edit.html.erb b/app/views/logs/edit.html.erb index 81b87e861..c65af44ec 100644 --- a/app/views/logs/edit.html.erb +++ b/app/views/logs/edit.html.erb @@ -51,5 +51,15 @@ <%= govuk_button_link_to "Back to sales logs", sales_logs_path %> <% end %> <% end %> + + <% if LogPolicy.new(current_user, @log).destroy? %> + <%= govuk_button_to( + "Delete log", + lettings_log_delete_confirmation_path(@log), + button: true, + method: :get, + warning: true, + ) %> + <% end %> diff --git a/config/routes.rb b/config/routes.rb index 07090f073..39f5068b4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -150,6 +150,8 @@ Rails.application.routes.draw do end resources :lettings_logs, path: "/lettings-logs" do + get "delete-confirmation", to: "lettings_logs#delete_confirmation" + collection do post "bulk-upload", to: "bulk_upload#bulk_upload" get "bulk-upload", to: "bulk_upload#show" diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index 7f1dda118..619bbef41 100644 --- a/spec/requests/lettings_logs_controller_spec.rb +++ b/spec/requests/lettings_logs_controller_spec.rb @@ -1309,55 +1309,91 @@ RSpec.describe LettingsLogsController, type: :request do end describe "DELETE" do + let(:headers) { { "Accept" => "text/html" } } + let(:page) { Capybara::Node::Simple.new(response.body) } + let(:user) { create(:user, :support) } let!(:lettings_log) do - FactoryBot.create(:lettings_log, :in_progress) + create(:lettings_log, :completed) end let(:id) { lettings_log.id } + let(:delete_request) { delete "/lettings-logs/#{id}", headers: } - context "when deleting a lettings log" do - before do - delete "/lettings-logs/#{id}", headers: - end + before do + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + end - it "returns http success" do - expect(response).to have_http_status(:success) + context "when delete permitted" do + it "redirects to lettings logs and shows message" do + delete_request + expect(response).to redirect_to(lettings_logs_path) + follow_redirect! + expect(page).to have_content("Log #{id} has been deleted") end - it "deletes the lettings log" do - expect { LettingsLog.find(id) }.to raise_error(ActiveRecord::RecordNotFound) + it "deletes the log" do + expect { delete_request }.to change { LettingsLog.exists?(id) }.from(true).to(false) end + end - context "with an invalid lettings log id" do - let(:id) { (LettingsLog.order(:id).last&.id || 0) + 1 } + context "when log does not exist" do + let(:id) { -1 } - it "returns 404" do - expect(response).to have_http_status(:not_found) - end + it "returns 404" do + delete_request + expect(response).to have_http_status(:not_found) end + end - context "with a request containing invalid credentials" do - let(:basic_credentials) do - ActionController::HttpAuthentication::Basic.encode_credentials(api_username, "Oops") - end + context "when user not authorised" do + let(:user) { create(:user) } - it "returns 401" do - expect(response).to have_http_status(:unauthorized) - end + it "returns 404" do + delete_request + expect(response).to have_http_status(:unauthorized) end end + end - context "when a lettings log deletion fails" do - let(:mock_scope) { instance_double("LettingsLog::ActiveRecord_Relation", find_by: lettings_log) } + describe "GET delete-confirmation" do + let(:headers) { { "Accept" => "text/html" } } + let(:page) { Capybara::Node::Simple.new(response.body) } + let(:user) { create(:user, :support) } + let!(:lettings_log) do + create(:lettings_log, :completed) + end + let(:id) { lettings_log.id } + let(:request) { get "/lettings-logs/#{id}/delete-confirmation", headers: } - before do - allow(LettingsLog).to receive(:visible).and_return(mock_scope) - allow(lettings_log).to receive(:delete).and_return(false) + before do + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + end - delete "/lettings-logs/#{id}", headers: + context "when delete permitted" do + it "renders page" do + request + expect(response).to have_http_status(:ok) + + expect(page).to have_content("Are you sure you want to delete this log?") end + end - it "returns an unprocessable entity 422" do - expect(response).to have_http_status(:unprocessable_entity) + context "when log does not exist" do + let(:id) { -1 } + + it "returns 404" do + request + expect(response).to have_http_status(:not_found) + end + end + + context "when user not authorised" do + let(:user) { create(:user) } + + it "returns 404" do + request + expect(response).to have_http_status(:unauthorized) end end end