From 91533d0308d20295c22c44ef905bce8bdcc2cb1b Mon Sep 17 00:00:00 2001 From: Arthur Campbell Date: Wed, 7 Jun 2023 14:16:46 +0100 Subject: [PATCH] refactor delete logs controller after tech review improve the private method that builds the form object so that it has the flexibility to do so for all controller methods ensure that the search term is passed to the delete logs controller when navigating through the organisations tab ensure that noly logs for that organisation are displayed when navigating to delete logs through the organisations tab --- app/controllers/delete_logs_controller.rb | 85 +++++++++----------- app/controllers/organisations_controller.rb | 4 +- app/models/forms/delete_logs_form.rb | 8 +- spec/requests/delete_logs_controller_spec.rb | 8 +- 4 files changed, 51 insertions(+), 54 deletions(-) diff --git a/app/controllers/delete_logs_controller.rb b/app/controllers/delete_logs_controller.rb index 6c60bf16d..c98ceab95 100644 --- a/app/controllers/delete_logs_controller.rb +++ b/app/controllers/delete_logs_controller.rb @@ -4,26 +4,20 @@ class DeleteLogsController < ApplicationController rescue_from ActiveRecord::RecordNotFound, with: :render_not_found before_action :session_filters, if: :current_user, except: %i[discard_lettings_logs discard_sales_logs discard_lettings_logs_for_organisation discard_sales_logs_for_organisation] + before_action :add_organisation_to_filters, only: %i[delete_lettings_logs_for_organisation delete_lettings_logs_for_organisation_with_selected_ids delete_lettings_logs_for_organisation_confirmation delete_sales_logs_for_organisation delete_sales_logs_for_organisation_with_selected_ids delete_sales_logs_for_organisation_confirmation] def delete_lettings_logs - @delete_logs_form = delete_logs_form(log_type: :lettings, paths: lettings_logs_paths) + @delete_logs_form = delete_logs_form(log_type: :lettings) render "logs/delete_logs" end def delete_lettings_logs_with_selected_ids - @delete_logs_form = delete_logs_form(selected_ids:, log_type: :lettings, paths: lettings_logs_paths) + @delete_logs_form = delete_logs_form(log_type: :lettings, selected_ids:) render "logs/delete_logs" end def delete_lettings_logs_confirmation - default_attributes = { - current_user:, - log_filters: @session_filters, - log_type: :lettings, - }.merge lettings_logs_paths - attributes = form_attributes.merge(default_attributes) - attributes[:selected_ids] = [] unless attributes.key? :selected_ids - @delete_logs_form = Forms::DeleteLogsForm.new(attributes) + @delete_logs_form = delete_logs_form(log_type: :lettings, form_params:) if @delete_logs_form.valid? render "logs/delete_logs_confirmation" else @@ -39,24 +33,17 @@ class DeleteLogsController < ApplicationController end def delete_sales_logs - @delete_logs_form = delete_logs_form(log_type: :sales, paths: sales_logs_paths) + @delete_logs_form = delete_logs_form(log_type: :sales) render "logs/delete_logs" end def delete_sales_logs_with_selected_ids - @delete_logs_form = delete_logs_form(selected_ids:, log_type: :sales, paths: sales_logs_paths) + @delete_logs_form = delete_logs_form(log_type: :sales, selected_ids:) render "logs/delete_logs" end def delete_sales_logs_confirmation - default_attributes = { - current_user:, - log_filters: @session_filters, - log_type: :sales, - }.merge sales_logs_paths - attributes = form_attributes.merge(default_attributes) - attributes[:selected_ids] = [] unless attributes.key? :selected_ids - @delete_logs_form = Forms::DeleteLogsForm.new(attributes) + @delete_logs_form = delete_logs_form(log_type: :sales, form_params:) if @delete_logs_form.valid? render "logs/delete_logs_confirmation" else @@ -72,24 +59,17 @@ class DeleteLogsController < ApplicationController end def delete_lettings_logs_for_organisation - @delete_logs_form = delete_logs_form(log_type: :lettings, paths: lettings_logs_for_organisation_paths) + @delete_logs_form = delete_logs_form(log_type: :lettings, organisation: true) render "logs/delete_logs" end def delete_lettings_logs_for_organisation_with_selected_ids - @delete_logs_form = delete_logs_form(selected_ids:, log_type: :lettings, paths: lettings_logs_for_organisation_paths) + @delete_logs_form = delete_logs_form(log_type: :lettings, organisation: true, selected_ids:) render "logs/delete_logs" end def delete_lettings_logs_for_organisation_confirmation - default_attributes = { - current_user:, - log_filters: @session_filters, - log_type: :lettings, - }.merge lettings_logs_for_organisation_paths - attributes = form_attributes.merge(default_attributes) - attributes[:selected_ids] = [] unless attributes.key? :selected_ids - @delete_logs_form = Forms::DeleteLogsForm.new(attributes) + @delete_logs_form = delete_logs_form(log_type: :lettings, organisation: true, form_params:) if @delete_logs_form.valid? render "logs/delete_logs_confirmation" else @@ -98,31 +78,24 @@ class DeleteLogsController < ApplicationController end def discard_lettings_logs_for_organisation - logs = LettingsLog.find(params.require(:ids)) + logs = LettingsLog.where(owning_organisation: params[:id]).find(params.require(:ids)) discard logs redirect_to lettings_logs_organisation_path, notice: I18n.t("notification.logs_deleted", count: logs.count) end def delete_sales_logs_for_organisation - @delete_logs_form = delete_logs_form(log_type: :sales, paths: sales_logs_for_organisation_paths) + @delete_logs_form = delete_logs_form(log_type: :sales, organisation: true) render "logs/delete_logs" end def delete_sales_logs_for_organisation_with_selected_ids - @delete_logs_form = delete_logs_form(selected_ids:, log_type: :sales, paths: sales_logs_for_organisation_paths) + @delete_logs_form = delete_logs_form(log_type: :sales, organisation: true, selected_ids:) render "logs/delete_logs" end def delete_sales_logs_for_organisation_confirmation - default_attributes = { - current_user:, - log_filters: @session_filters, - log_type: :sales, - }.merge sales_logs_for_organisation_paths - attributes = form_attributes.merge(default_attributes) - attributes[:selected_ids] = [] unless attributes.key? :selected_ids - @delete_logs_form = Forms::DeleteLogsForm.new(attributes) + @delete_logs_form = delete_logs_form(log_type: :sales, organisation: true, form_params:) if @delete_logs_form.valid? render "logs/delete_logs_confirmation" else @@ -131,7 +104,7 @@ class DeleteLogsController < ApplicationController end def discard_sales_logs_for_organisation - logs = SalesLog.find(params.require(:ids)) + logs = SalesLog.where(owning_organisation: params[:id]).find(params.require(:ids)) discard logs redirect_to sales_logs_organisation_path, notice: I18n.t("notification.logs_deleted", count: logs.count) @@ -139,12 +112,28 @@ class DeleteLogsController < ApplicationController private - def delete_logs_form(log_type:, paths:, selected_ids: nil) - Forms::DeleteLogsForm.new(current_user:, search_term:, log_filters: @session_filters, log_type:, selected_ids:, **paths) + def delete_logs_form(log_type:, organisation: false, selected_ids: nil, form_params: {}) + paths = case log_type + when :lettings + organisation ? lettings_logs_for_organisation_paths : lettings_logs_paths + when :sales + organisation ? sales_logs_for_organisation_paths : sales_logs_paths + end + attributes = { + log_type:, + current_user:, + log_filters: @session_filters, + search_term:, + selected_ids:, + **paths + }.merge(form_params).transform_keys(&:to_sym) + Forms::DeleteLogsForm.new(attributes) end - def form_attributes - params.require(:forms_delete_logs_form).permit(:search_term, selected_ids: []) + def form_params + form_attributes = params.require(:forms_delete_logs_form).permit(:search_term, selected_ids: []) + form_attributes[:selected_ids] = [] unless form_attributes.key? :selected_ids + form_attributes end def lettings_logs_paths @@ -179,6 +168,10 @@ private } end + def add_organisation_to_filters + @session_filters[:organisation] = params[:id] + end + def search_term params["search"] end diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index 4326a0869..98082bea9 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -96,7 +96,7 @@ class OrganisationsController < ApplicationController format.html do @search_term = search_term @pagy, @logs = pagy(unpaginated_filtered_logs) - @delete_logs_path = delete_lettings_logs_organisation_path + @delete_logs_path = delete_lettings_logs_organisation_path(search: @search_term) @searched = search_term.presence @total_count = organisation_logs.size @log_type = :lettings @@ -127,7 +127,7 @@ class OrganisationsController < ApplicationController format.html do @search_term = search_term @pagy, @logs = pagy(unpaginated_filtered_logs) - @delete_logs_path = delete_sales_logs_organisation_path + @delete_logs_path = delete_sales_logs_organisation_path(search: @search_term) @searched = search_term.presence @total_count = organisation_logs.size @log_type = :sales diff --git a/app/models/forms/delete_logs_form.rb b/app/models/forms/delete_logs_form.rb index 36fb33919..220fd15dd 100644 --- a/app/models/forms/delete_logs_form.rb +++ b/app/models/forms/delete_logs_form.rb @@ -11,7 +11,7 @@ module Forms @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) + @logs = FilterService.filter_logs(visible_logs, @search_term, attributes[:log_filters], nil, @current_user) @selected_ids = attributes[:selected_ids] || @logs.map(&:id) @delete_confirmation_path = attributes[:delete_confirmation_path] @back_to_logs_path = attributes[:back_to_logs_path] @@ -22,6 +22,10 @@ module Forms @logs.count end + def table_partial_name + "logs/delete_logs_table_#{@log_type}" + end + private def at_least_one_log_selected @@ -30,7 +34,7 @@ module Forms end end - def all_logs + def visible_logs case @log_type when :lettings then @current_user.lettings_logs.visible when :sales then @current_user.sales_logs.visible diff --git a/spec/requests/delete_logs_controller_spec.rb b/spec/requests/delete_logs_controller_spec.rb index 97bf24750..6de96df79 100644 --- a/spec/requests/delete_logs_controller_spec.rb +++ b/spec/requests/delete_logs_controller_spec.rb @@ -513,7 +513,7 @@ RSpec.describe "DeleteLogs", type: :request do 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 + expect(arg3).to eq logs_filters.merge(organisation: organisation.id.to_s) }.and_return LettingsLog.all get delete_lettings_logs_organisation_path(id: organisation, search:) @@ -562,7 +562,7 @@ RSpec.describe "DeleteLogs", type: :request do 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 + expect(arg3).to eq logs_filters.merge(organisation: organisation.id.to_s) }.and_return LettingsLog.all post delete_lettings_logs_organisation_path(id: organisation, search:, selected_ids:) @@ -738,7 +738,7 @@ RSpec.describe "DeleteLogs", type: :request do 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 + expect(arg3).to eq logs_filters.merge(organisation: organisation.id.to_s) }.and_return SalesLog.all get delete_sales_logs_organisation_path(id: organisation, search:) @@ -787,7 +787,7 @@ RSpec.describe "DeleteLogs", type: :request do 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 + expect(arg3).to eq logs_filters.merge(organisation: organisation.id.to_s) }.and_return SalesLog.all post delete_sales_logs_organisation_path(id: organisation, search:, selected_ids:)