From 64bfdae6a5ec46df675b34775540b17cc3cf4979 Mon Sep 17 00:00:00 2001 From: Louis Kirkham Date: Sat, 21 Sep 2024 16:07:27 +0100 Subject: [PATCH] chore: Refactor code to use hidden_field instead of text_field for form_answer_ids Signed-off-by: Louis Kirkham --- .../javascripts/admin/form_answers.js.coffee | 14 ----- .../admin/form_answers_controller.rb | 53 ++++++------------- ...enant_assignment_collections_controller.rb | 28 +++++----- app/controllers/concerns/form_answer_mixin.rb | 27 +++++++++- .../lieutenant_assignment_collection.rb | 4 +- .../_bulk_assignment_button_top.html.slim | 2 +- .../bulk_assign_assessors.html.slim | 2 +- .../bulk_assign_lieutenants.html.slim | 9 +++- config/initializers/nokogiri.rb | 2 +- forms/nominations_bulk_action_form.rb | 47 ++++++++-------- 10 files changed, 92 insertions(+), 96 deletions(-) diff --git a/app/assets/javascripts/admin/form_answers.js.coffee b/app/assets/javascripts/admin/form_answers.js.coffee index 4c9f20b43..952689730 100644 --- a/app/assets/javascripts/admin/form_answers.js.coffee +++ b/app/assets/javascripts/admin/form_answers.js.coffee @@ -130,20 +130,6 @@ ready = -> else area.addClass("if-js-hide") - $(document).on "submit", "#new_assessor_assignment_collection", (e) -> - form = $(this) - ids = "" - $(".form-answer-check:checked").each -> - ids += ($(@).val() + ",") - $("#assessor_assignment_collection_form_answer_ids").val(ids) - - $(document).on "submit", "#new_lieutenant_assignment_collection", (e) -> - form = $(this) - ids = "" - $(".form-answer-check:checked").each -> - ids += ($(@).val() + ",") - $("#lieutenant_assignment_collection_form_answer_ids").val(ids) - changeRagStatus = -> $(document).on "click", ".btn-rag .dropdown-menu a", (e) -> e.preventDefault() diff --git a/app/controllers/admin/form_answers_controller.rb b/app/controllers/admin/form_answers_controller.rb index bb9e2b253..be8c210a7 100644 --- a/app/controllers/admin/form_answers_controller.rb +++ b/app/controllers/admin/form_answers_controller.rb @@ -66,22 +66,8 @@ def index .with_comments_counters .group("form_answers.id") .page(params[:page]) - respond_to do |format| - format.html do - raise params[:bulk_action].to_yaml if params[:bulk_action].present? - - @processor = NominationsBulkActionForm.new(params) - - redirect_url = @processor.redirect_url - # works as a string but not working with helper method - # redirect_url = "/admin/form_answers/bulk_assign_lieutenants" - - if redirect_url - redirect_to redirect_url - return - end - end + format.html format.csv do timestamp = Time.zone.now.strftime("%d-%m-%Y") @@ -270,38 +256,28 @@ def awarded_trade_applications def bulk_assign_lieutenants authorize :lieutenant_assignment_collection, :create? - - @form = LieutenantAssignmentCollection.new(params) - @form.form_answer_ids = [] - end - - def bulk_assign_lieutenants_confirm - authorize :lieutenant_assignment_collection, :create? - - # ConfirmationForm.new(kind: "bulk_assign_lieutenants") + form_answer_ids = if params[:bulk_action] + params[:bulk_action][:ids] + elsif params[:lieutenant_assignment_collection] + # get form_answer_ids after validation error + params[:lieutenant_assignment_collection][:form_answer_ids] + end + @form = LieutenantAssignmentCollection.new(form_answer_ids: form_answer_ids, params: bulk_assign_params) + @form.form_answer_ids = form_answer_ids end def bulk_assign_assessors authorize :assessor_assignment_collection, :create? - @form = AssessorAssignmentCollection.new - @form.form_answer_ids = [] - end - - def bulk_assign_assessors_confirm - authorize :assessor_assignment_collection, :create? - - # ConfirmationForm.new(kind: "bulk_assign_assessors") + form_answer_ids = params[:bulk_action][:ids] + @form = AssessorAssignmentCollection.new(form_answer_ids: form_answer_ids) + @form.form_answer_ids = form_answer_ids end def bulk_assign_eligibility authorize :eligibility_assignment_collection, :create? end - def bulk_assign_eligibility_confirm - authorize :eligibility_assignment_collection, :create? - end - # / batch actions private @@ -330,6 +306,11 @@ def eligibility_params ) end + def bulk_assign_params + # params.require(:bulk_action).permit(ids: []).merge(params.permit(:bulk_assign_lieutenants)) + params.permit! + end + def resolve_layout case action_name when "edit", "update", "save" diff --git a/app/controllers/admin/lieutenant_assignment_collections_controller.rb b/app/controllers/admin/lieutenant_assignment_collections_controller.rb index 34a76653c..27e4f853b 100644 --- a/app/controllers/admin/lieutenant_assignment_collections_controller.rb +++ b/app/controllers/admin/lieutenant_assignment_collections_controller.rb @@ -1,27 +1,25 @@ class Admin::LieutenantAssignmentCollectionsController < Admin::BaseController def create - @lieutenant_assignment_collection = LieutenantAssignmentCollection.new(create_params) - authorize @lieutenant_assignment_collection, :create? + @form = LieutenantAssignmentCollection.new(create_params) + authorize @form, :create? - @lieutenant_assignment_collection.subject = current_subject + @form.subject = current_subject - @lieutenant_assignment_collection.save - - respond_to do |format| - format.html do - flash[:notice] = @lieutenant_assignment_collection.notice_message - flash[:error] = @lieutenant_assignment_collection.errors.full_messages.to_sentence - redirect_back(fallback_location: root_path) - end + if @form.save + # Change this - don't permit all params + params.permit! + redirect_to admin_form_answers_path(params: params), notice: @form.notice_message + else + # Ensure form_answer_ids is an array + @form.form_answer_ids = @form.form_answer_ids.split(",") if @form.form_answer_ids.is_a?(String) + render 'admin/form_answers/bulk_assign_lieutenants' end end private def create_params - params - .require(:lieutenant_assignment_collection) - .permit(:form_answer_ids, - :ceremonial_county_id) + params.require(:lieutenant_assignment_collection) + .permit(:form_answer_ids, :ceremonial_county_id, :year) end end diff --git a/app/controllers/concerns/form_answer_mixin.rb b/app/controllers/concerns/form_answer_mixin.rb index ab0af7f60..cd0a8d110 100644 --- a/app/controllers/concerns/form_answer_mixin.rb +++ b/app/controllers/concerns/form_answer_mixin.rb @@ -95,9 +95,34 @@ def financial_data_ops def save_or_load_search! search_params = params[:search] || default_filters + if params[:bulk_action].present? + bulk_params = params.permit( + # :year, + # :search_id, + :bulk_assign_lieutenants, + # :bulk_assign_assessors, + # :bulk_assign_eligibility, + bulk_action: { ids: [] } + ) + + @processor = NominationsBulkActionForm.new(bulk_params) + # raise params[:bulk_action].to_yaml if params[:bulk_action].present? + # Debugging: Inspect the parameters and validation errors + + unless @processor.valid? + raise "Invalid bulk action parameters: #{params.to_yaml}\nErrors: #{@processor.errors.full_messages.to_yaml}" + end + + redirect_url = @processor.redirect_url + + if redirect_url + redirect_to redirect_url and return + end + end + if params[:search] && params[:search][:search_filter] != FormAnswerSearch.default_search[:search_filter] search = NominationSearch.create(serialized_query: params[:search].to_json) - redirect_to [namespace_name, :form_answers, search_id: search.id, year: params[:year]] + redirect_to [namespace_name, :form_answers, search_id: search.id, year: params[:year]] and return end if params[:search_id] diff --git a/app/models/lieutenant_assignment_collection.rb b/app/models/lieutenant_assignment_collection.rb index 3f1a4ed70..77bb660e7 100644 --- a/app/models/lieutenant_assignment_collection.rb +++ b/app/models/lieutenant_assignment_collection.rb @@ -2,12 +2,10 @@ class LieutenantAssignmentCollection < AssignmentCollection NOT_ASSIGNED = "not assigned" attribute :ceremonial_county_id, String - - binding.pry + validates :ceremonial_county_id, presence: true def save return unless valid? - form_answers.update_all(ceremonial_county_id: ceremonial_county_id) end diff --git a/app/views/admin/form_answers/_bulk_assignment_button_top.html.slim b/app/views/admin/form_answers/_bulk_assignment_button_top.html.slim index 5c6030424..e84b19e32 100644 --- a/app/views/admin/form_answers/_bulk_assignment_button_top.html.slim +++ b/app/views/admin/form_answers/_bulk_assignment_button_top.html.slim @@ -3,7 +3,7 @@ p.govuk-body.nominations-checked-total aria-live='polite' strong ' Bulk actions: - ' select groups from the list below and click an approperiate bulk action button. + ' select groups from the list below and click an appropriate bulk action button. .bulk-assignment-buttons-container = f.submit "Bulk assign to Lord Lieutenancy office", class: "govuk-button bulk-assign-lieutenants-link", name: "bulk_assign_lieutenants" = f.submit "Bulk assign to national assessor sub-group", class: "govuk-button bulk-assign-assessors-link", name: "bulk_assign_assessors" diff --git a/app/views/admin/form_answers/bulk_assign_assessors.html.slim b/app/views/admin/form_answers/bulk_assign_assessors.html.slim index 020f828db..29e4920bc 100644 --- a/app/views/admin/form_answers/bulk_assign_assessors.html.slim +++ b/app/views/admin/form_answers/bulk_assign_assessors.html.slim @@ -10,7 +10,7 @@ h3.govuk-heading-m { include_blank: true }, class: "govuk-select custom-select" - = f.text_field :form_answer_ids, class: "if-js-hide visuallyhidden" + = f.hidden_field :form_answer_ids, value: @form.form_answer_ids.join(",") .govuk-button-group = f.submit "Bulk assign groups to national assessor sub-group", class: "govuk-button" diff --git a/app/views/admin/form_answers/bulk_assign_lieutenants.html.slim b/app/views/admin/form_answers/bulk_assign_lieutenants.html.slim index d0a0a64b9..cd3f54f50 100644 --- a/app/views/admin/form_answers/bulk_assign_lieutenants.html.slim +++ b/app/views/admin/form_answers/bulk_assign_lieutenants.html.slim @@ -1,16 +1,21 @@ h3.govuk-heading-m | Bulk assign groups to Lord Lieutenancy office +.govuk-body + | Please select the Lord Lieutenancy office that you would like to assign selected groups to. = form_for [namespace_name, @form] do |f| - = flash[:error] .govuk-form-group.primary-lieutenant-group = f.label :ceremonial_county_id, "Select Lord Lieutenancy office", class: "govuk-label" = f.select :ceremonial_county_id, CeremonialCounty.ordered.pluck(:name, :id), { include_blank: true }, class: "govuk-select custom-select" + - if @form.errors[:ceremonial_county_id].any? + .govuk-error-message + | Please select a Lord Lieutenancy office - = f.text_field :form_answer_ids, class: "if-js-hide visuallyhidden" + - if @form.form_answer_ids.present? + = f.hidden_field :form_answer_ids, value: @form.form_answer_ids.join(",") .govuk-button-group = f.submit "Bulk assign groups to Lord Lieutenancy office", class: "govuk-button" diff --git a/config/initializers/nokogiri.rb b/config/initializers/nokogiri.rb index cd6ed0b5c..c9542d2e9 100644 --- a/config/initializers/nokogiri.rb +++ b/config/initializers/nokogiri.rb @@ -1 +1 @@ -Nokogiri::Gumbo::DEFAULT_MAX_TREE_DEPTH = -1 \ No newline at end of file +Nokogiri::Gumbo::DEFAULT_MAX_TREE_DEPTH = -1 diff --git a/forms/nominations_bulk_action_form.rb b/forms/nominations_bulk_action_form.rb index a17d0281b..b267668c4 100644 --- a/forms/nominations_bulk_action_form.rb +++ b/forms/nominations_bulk_action_form.rb @@ -1,46 +1,49 @@ class NominationsBulkActionForm - attr_reader :params + attr_reader :params, :errors include Rails.application.routes.url_helpers def initialize(params) @params = params - end - - def kind - @kind ||= if params[:bulk_assign_lieutenants] - "lieutenants" - elsif params[:bulk_assign_assessors] - "assessors" - elsif params[:bulk_assign_eligibility] - "eligibility" - end + @errors = ActiveModel::Errors.new(self) + @kind = determine_kind end def valid? if !params[:bulk_action].present? - errors.add(:bulk_base, "You must select at least one group from the list below before clicking a bulk action button.") - + @errors.add(:bulk_base, "You must select at least one group from the list below before clicking a bulk action button.") return false end - case kind - when "lieutenants" - params[:bulk_action].present? - when "assessors" - params[:bulk_action].present? - when "eligibility" + case @kind + when "lieutenants", "assessors", "eligibility" params[:bulk_action].present? + else + @errors.add(:bulk_base, "Invalid bulk action type.") + false end end def redirect_url - case kind + case @kind when "lieutenants" bulk_assign_lieutenants_admin_form_answers_path(params: params) when "assessors" - bulk_assign_assessors_admin_form_answers_path(params: params) + bulk_assign_assessors_admin_form_answers_path(params: permitted_params) when "eligibility" - bulk_assign_eligibility_admin_form_answers_path(params: params) + bulk_assign_eligibility_admin_form_answers_path(params: permitted_params) + else + # Handle invalid kind + raise "Invalid bulk action type." + end + end + + def determine_kind + if params[:bulk_assign_lieutenants] + "lieutenants" + elsif params[:bulk_assign_assessors] + "assessors" + elsif params[:bulk_assign_eligibility] + "eligibility" end end end