Refactor

app/controllers/lettings_logs_filters_controller.rb

@@ -1,42 +1,35 @@
 class LettingsLogsFiltersController < ApplicationController
-  before_action :session_filters, if: :current_user, only: %i[update]
+  before_action :session_filters, if: :current_user
-  before_action -> { filter_manager.serialize_filters_to_session }, if: :current_user, only: %i[update]
+  before_action -> { filter_manager.serialize_filters_to_session }, if: :current_user
 
   %w[years status needstype assigned_to owned_by managed_by].each do |filter|
     define_method(filter) do
-      @filter_form = Forms::FilterForm.new
       @filter_type = "lettings_logs"
-      @search_term = params["search"]
-      @codes_only = params["codes_only"]
       render "filters/lettings_log_filters/#{filter}"
     end
   end
 
-  def update
+  %w[status needstype assigned_to owned_by managed_by].each do |filter|
-    @filter_form = Forms::FilterForm.new(filter_form_params)
+    define_method("update_#{filter}") do
-    @filter_type = "lettings_logs"
+      @filter_type = "lettings_logs"
-    @search_term = params["search"]
+
-    @codes_only = params["codes_only"]
+      redirect_to csv_download_lettings_logs_path(search: params["search"], codes_only: params["codes_only"])
+    end
+  end
 
-    if @filter_form.valid?
+  def update_years
-      session_filters
+    @filter_type = "lettings_logs"
-      redirect_to csv_download_lettings_logs_path(search: @search_term, codes_only: @codes_only)
+    if params["years"].nil?
+      redirect_to filters_years_lettings_logs_path(search: params["search"], codes_only: params["codes_only"], error: "Please select a year")
     else
-      render "filters/lettings_log_filters/years", status: :unprocessable_entity
+      redirect_to csv_download_lettings_logs_path(search: params["search"], codes_only: params["codes_only"])
     end
   end
 end
 
 private
 
-def filter_form_params
-  filter_params = params.permit(:years, :status, :needstypes, :assigned_to, :user, :owning_organisation_select, :owning_organisation, :managing_organisation_select, :managing_organisation)
-  filter_params[:years] = session_filters["years"] if filter_params[:years].blank?
-  filter_params
-end
-
 def session_filters
-  params["forms_filter_form"].each { |key, value| params[key] = value } if params["forms_filter_form"].present?
   params["years"] = [params["years"]] if params["years"].present?
   filter_manager.session_filters
 end

app/controllers/sales_logs_filters_controller.rb

@@ -1,42 +1,35 @@
 class SalesLogsFiltersController < ApplicationController
-  before_action :session_filters, if: :current_user, only: %i[update]
+  before_action :session_filters, if: :current_user
-  before_action -> { filter_manager.serialize_filters_to_session }, if: :current_user, only: %i[update]
+  before_action -> { filter_manager.serialize_filters_to_session }, if: :current_user
 
   %w[years status assigned_to owned_by managed_by].each do |filter|
     define_method(filter) do
-      @filter_form = Forms::FilterForm.new
       @filter_type = "sales_logs"
-      @search_term = params["search"]
-      @codes_only = params["codes_only"]
       render "filters/sales_log_filters/#{filter}"
     end
   end
 
-  def update
+  %w[status assigned_to owned_by managed_by].each do |filter|
-    @filter_form = Forms::FilterForm.new(filter_form_params)
+    define_method("update_#{filter}") do
-    @filter_type = "sales_logs"
+      @filter_type = "sales_logs"
-    @search_term = params["search"]
+
-    @codes_only = params["codes_only"]
+      redirect_to csv_download_sales_logs_path(search: params["search"], codes_only: params["codes_only"])
+    end
+  end
 
-    if @filter_form.valid?
+  def update_years
-      session_filters
+    @filter_type = "sales_logs"
-      redirect_to csv_download_sales_logs_path(search: @search_term, codes_only: @codes_only)
+    if params["years"].nil?
+      redirect_to filters_years_sales_logs_path(search: params["search"], codes_only: params["codes_only"], error: "Please select a year")
     else
-      render "filters/sales_log_filters/years", status: :unprocessable_entity
+      redirect_to csv_download_sales_logs_path(search: params["search"], codes_only: params["codes_only"])
     end
   end
 end
 
 private
 
-def filter_form_params
-  filter_params = params.permit(:years, :status, :assigned_to, :user, :owning_organisation_select, :owning_organisation, :managing_organisation_select, :managing_organisation)
-  filter_params[:years] = session_filters["years"] if filter_params[:years].blank?
-  filter_params
-end
-
 def session_filters
-  params["forms_filter_form"].each { |key, value| params[key] = value } if params["forms_filter_form"].present?
   params["years"] = [params["years"]] if params["years"].present?
   filter_manager.session_filters
 end

app/models/forms/filter_form.rb

@@ -1,10 +0,0 @@
-module Forms
-  class FilterForm
-    include ActiveModel::Model
-    include ActiveModel::Validations
-
-    attr_accessor :years, :status, :needstypes, :assigned_to, :user, :owning_organisation_select, :owning_organisation, :managing_organisation_select, :managing_organisation
-
-    validates :years, presence: true
-  end
-end

app/views/filters/lettings_log_filters/assigned_to.html.erb

@@ -1,4 +1,4 @@
-<%= form_with model: @filter_form, url: filters_lettings_logs_path(search: @search_term, codes_only: @codes_only) do |f| %>
+<%= form_with html: { method: :get }, url: filters_update_assigned_to_lettings_logs_path do |f| %>
   <%= render partial: "filters/radio_filter",
              locals: {
                 f:,
@@ -17,10 +17,15 @@
                 },
                 label: "Assigned to",
                 category: "assigned_to",
+                size: "l",
               } %>
+  <% if request.params["search"].present? %>
+    <%= f.hidden_field :search, value: request.params["search"] %>
+  <% end %>
+  <%= f.hidden_field :codes_only, value: request.params["codes_only"] %>
 
   <div class="govuk-button-group">
     <%= f.govuk_submit "Save changes" %>
-    <%= govuk_button_link_to "Cancel", csv_download_lettings_logs_path(search: @search_term, codes_only: @codes_only), secondary: true %>
+    <%= govuk_button_link_to "Cancel", csv_download_lettings_logs_path(search: request.params["search"], codes_only: request.params["codes_only"]), secondary: true %>
   </div>
 <% end %>

app/views/filters/lettings_log_filters/managed_by.html.erb

@@ -1,4 +1,4 @@
-<%= form_with model: @filter_form, url: filters_lettings_logs_path(search: @search_term, codes_only: @codes_only) do |f| %>
+<%= form_with html: { method: :get }, url: filters_update_managed_by_lettings_logs_path do |f| %>
   <%= render partial: "filters/radio_filter", locals: {
     f:,
     options: {
@@ -15,9 +15,16 @@
     },
     label: "Managed by",
     category: "managing_organisation_select",
+    size: "l",
   } %>
+
+  <% if request.params["search"].present? %>
+    <%= f.hidden_field :search, value: request.params["search"] %>
+  <% end %>
+  <%= f.hidden_field :codes_only, value: request.params["codes_only"] %>
+
   <div class="govuk-button-group">
     <%= f.govuk_submit "Save changes" %>
-    <%= govuk_button_link_to "Cancel", csv_download_lettings_logs_path(search: @search_term, codes_only: @codes_only), secondary: true %>
+    <%= govuk_button_link_to "Cancel", csv_download_lettings_logs_path(search: request.params["search"], codes_only: request.params["codes_only"]), secondary: true %>
   </div>
 <% end %>

app/views/filters/lettings_log_filters/needstype.html.erb

@@ -1,14 +1,20 @@
-<%= form_with model: @filter_form, url: filters_lettings_logs_path(search: @search_term, codes_only: @codes_only) do |f| %>
+<%= form_with html: { method: :get }, url: filters_update_needstype_lettings_logs_path do |f| %>
     <%= render partial: "filters/checkbox_filter",
                locals: {
                          f:,
                          options: needstype_filters,
                          label: "Needs type",
                          category: "needstypes",
+                         size: "l",
                        } %>
 
+  <% if request.params["search"].present? %>
+    <%= f.hidden_field :search, value: request.params["search"] %>
+  <% end %>
+  <%= f.hidden_field :codes_only, value: request.params["codes_only"] %>
+
   <div class="govuk-button-group">
     <%= f.govuk_submit "Save changes" %>
-    <%= govuk_button_link_to "Cancel", csv_download_lettings_logs_path(search: @search_term, codes_only: @codes_only), secondary: true %>
+    <%= govuk_button_link_to "Cancel", csv_download_lettings_logs_path(search: request.params["search"], codes_only: request.params["codes_only"]), secondary: true %>
   </div>
 <% end %>

app/views/filters/lettings_log_filters/owned_by.html.erb

@@ -1,4 +1,4 @@
-<%= form_with model: @filter_form, url: filters_lettings_logs_path(search: @search_term, codes_only: @codes_only) do |f| %>
+<%= form_with html: { method: :get }, url: filters_update_owned_by_lettings_logs_path do |f| %>
   <%= render partial: "filters/radio_filter", locals: {
     f:,
     options: {
@@ -15,9 +15,16 @@
     },
     label: "Owned by",
     category: "owning_organisation_select",
+    size: "l",
   } %>
+
+  <% if request.params["search"].present? %>
+    <%= f.hidden_field :search, value: request.params["search"] %>
+  <% end %>
+  <%= f.hidden_field :codes_only, value: request.params["codes_only"] %>
+
   <div class="govuk-button-group">
     <%= f.govuk_submit "Save changes" %>
-    <%= govuk_button_link_to "Cancel", csv_download_lettings_logs_path(search: @search_term, codes_only: @codes_only), secondary: true %>
+    <%= govuk_button_link_to "Cancel", csv_download_lettings_logs_path(search: request.params["search"], codes_only: request.params["codes_only"]), secondary: true %>
   </div>
 <% end %>

app/views/filters/lettings_log_filters/status.html.erb

@@ -1,13 +1,20 @@
-<%= form_with model: @filter_form, url: filters_lettings_logs_path(search: @search_term, codes_only: @codes_only) do |f| %>
+<%= form_with html: { method: :get }, url: filters_update_status_lettings_logs_path do |f| %>
   <%= render partial: "filters/checkbox_filter",
              locals: {
                 f:,
                 options: status_filters,
                 label: "Status",
                 category: "status",
+                size: "l",
               } %>
+
+  <% if request.params["search"].present? %>
+    <%= f.hidden_field :search, value: request.params["search"] %>
+  <% end %>
+  <%= f.hidden_field :codes_only, value: request.params["codes_only"] %>
+
   <div class="govuk-button-group">
     <%= f.govuk_submit "Save changes" %>
-    <%= govuk_button_link_to "Cancel", csv_download_lettings_logs_path(search: @search_term, codes_only: @codes_only), secondary: true %>
+    <%= govuk_button_link_to "Cancel", csv_download_lettings_logs_path(search: request.params["search"], codes_only: request.params["codes_only"]), secondary: true %>
   </div>
 <% end %>

app/views/filters/lettings_log_filters/years.html.erb

@@ -1,5 +1,20 @@
-<%= form_with model: @filter_form, url: filters_lettings_logs_path(search: @search_term, codes_only: @codes_only) do |f| %>
+<%= form_with html: { method: :get }, url: filters_update_years_lettings_logs_path do |f| %>
-  <%= f.govuk_error_summary %>
+  <% if params["error"].present? %>
+  <div class="govuk-error-summary" data-module="govuk-error-summary">
+    <div role="alert">
+      <h2 class="govuk-error-summary__title">
+        There is a problem
+      </h2>
+      <div class="govuk-error-summary__body">
+        <ul class="govuk-list govuk-error-summary__list">
+          <li>
+            <a href="#"><%= params["error"] %></a>
+          </li>
+        </ul>
+      </div>
+    </div>
+  </div>
+  <% end %>
 
   <%= render partial: "filters/radio_filter",
              locals: {
@@ -7,8 +22,14 @@
         options: collection_year_radio_options,
         label: "Which financial year do you want to download data for?",
         category: "years",
+        size: "l",
       } %>
 
+  <% if request.params["search"].present? %>
+    <%= f.hidden_field :search, value: request.params["search"] %>
+  <% end %>
+  <%= f.hidden_field :codes_only, value: request.params["codes_only"] %>
+
   <div class="govuk-button-group">
     <%= f.govuk_submit "Save changes" %>
     <%= govuk_button_link_to "Cancel", lettings_logs_path, secondary: true %>

app/views/filters/sales_log_filters/assigned_to.html.erb

@@ -1,4 +1,4 @@
-<%= form_with model: @filter_form, url: filters_sales_logs_path(search: @search_term, codes_only: @codes_only) do |f| %>
+<%= form_with html: { method: :get }, url: filters_update_assigned_to_sales_logs_path do |f| %>
   <%= render partial: "filters/radio_filter",
              locals: {
                 f:,
@@ -17,10 +17,16 @@
                 },
                 label: "Assigned to",
                 category: "assigned_to",
+                size: "l",
               } %>
 
+  <% if request.params["search"].present? %>
+    <%= f.hidden_field :search, value: request.params["search"] %>
+  <% end %>
+  <%= f.hidden_field :codes_only, value: request.params["codes_only"] %>
+
   <div class="govuk-button-group">
     <%= f.govuk_submit "Save changes" %>
-    <%= govuk_button_link_to "Cancel", csv_download_sales_logs_path(search: @search_term, codes_only: @codes_only), secondary: true %>
+    <%= govuk_button_link_to "Cancel", csv_download_sales_logs_path(search: request.params["search"], codes_only: request.params["codes_only"]), secondary: true %>
   </div>
 <% end %>

app/views/filters/sales_log_filters/managed_by.html.erb

@@ -1,4 +1,4 @@
-<%= form_with model: @filter_form, url: filters_sales_logs_path(search: @search_term, codes_only: @codes_only) do |f| %>
+<%= form_with html: { method: :get }, url: filters_update_manaed_by_sales_logs_path do |f| %>
   <%= render partial: "filters/radio_filter", locals: {
     f:,
     options: {
@@ -15,9 +15,16 @@
     },
     label: "Reported by",
     category: "managing_organisation_select",
+    size: "l",
   } %>
+
+  <% if request.params["search"].present? %>
+    <%= f.hidden_field :search, value: request.params["search"] %>
+  <% end %>
+  <%= f.hidden_field :codes_only, value: request.params["codes_only"] %>
+
   <div class="govuk-button-group">
     <%= f.govuk_submit "Save changes" %>
-    <%= govuk_button_link_to "Cancel", csv_download_sales_logs_path(search: @search_term, codes_only: @codes_only), secondary: true %>
+    <%= govuk_button_link_to "Cancel", csv_download_sales_logs_path(search: request.params["search"], codes_only: request.params["codes_only"]), secondary: true %>
   </div>
 <% end %>

app/views/filters/sales_log_filters/owned_by.html.erb

@@ -1,4 +1,4 @@
-<%= form_with model: @filter_form, url: filters_sales_logs_path(search: @search_term, codes_only: @codes_only) do |f| %>
+<%= form_with html: { method: :get }, url: filters_update_owned_by_sales_logs_path do |f| %>
   <%= render partial: "filters/radio_filter", locals: {
     f:,
     options: {
@@ -15,9 +15,16 @@
     },
     label: "Owned by",
     category: "owning_organisation_select",
+    size: "l",
   } %>
+
+  <% if request.params["search"].present? %>
+    <%= f.hidden_field :search, value: request.params["search"] %>
+  <% end %>
+  <%= f.hidden_field :codes_only, value: request.params["codes_only"] %>
+
   <div class="govuk-button-group">
     <%= f.govuk_submit "Save changes" %>
-    <%= govuk_button_link_to "Cancel", csv_download_sales_logs_path(search: @search_term, codes_only: @codes_only), secondary: true %>
+    <%= govuk_button_link_to "Cancel", csv_download_sales_logs_path(search: request.params["search"], codes_only: request.params["codes_only"]), secondary: true %>
   </div>
 <% end %>

app/views/filters/sales_log_filters/status.html.erb

@@ -1,13 +1,20 @@
-<%= form_with model: @filter_form, url: filters_sales_logs_path(search: @search_term, codes_only: @codes_only) do |f| %>
+<%= form_with html: { method: :get }, url: filters_update_status_sales_logs_path do |f| %>
   <%= render partial: "filters/checkbox_filter",
              locals: {
                 f:,
                 options: status_filters,
                 label: "Status",
                 category: "status",
+                size: "l",
               } %>
+
+  <% if request.params["search"].present? %>
+    <%= f.hidden_field :search, value: request.params["search"] %>
+  <% end %>
+  <%= f.hidden_field :codes_only, value: request.params["codes_only"] %>
+
   <div class="govuk-button-group">
     <%= f.govuk_submit "Save changes" %>
-    <%= govuk_button_link_to "Cancel", csv_download_sales_logs_path(search: @search_term, codes_only: @codes_only), secondary: true %>
+    <%= govuk_button_link_to "Cancel", csv_download_sales_logs_path(search: request.params["search"], codes_only: request.params["codes_only"]), secondary: true %>
   </div>
 <% end %>

app/views/filters/sales_log_filters/years.html.erb

@@ -1,5 +1,19 @@
-<%= form_with model: @filter_form, url: filters_sales_logs_path(search: @search_term, codes_only: @codes_only) do |f| %>
+<%= form_with html: { method: :get }, url: filters_update_years_sales_logs_path do |f| %>  <% if params["error"].present? %>
-  <%= f.govuk_error_summary %>
+  <div class="govuk-error-summary" data-module="govuk-error-summary">
+    <div role="alert">
+      <h2 class="govuk-error-summary__title">
+        There is a problem
+      </h2>
+      <div class="govuk-error-summary__body">
+        <ul class="govuk-list govuk-error-summary__list">
+          <li>
+            <a href="#"><%= params["error"] %></a>
+          </li>
+        </ul>
+      </div>
+    </div>
+  </div>
+  <% end %>
 
   <%= render partial: "filters/radio_filter",
              locals: {
@@ -7,8 +21,14 @@
         options: collection_year_radio_options,
         label: "Which financial year do you want to download data for?",
         category: "years",
+        size: "l",
       } %>
 
+  <% if request.params["search"].present? %>
+    <%= f.hidden_field :search, value: request.params["search"] %>
+  <% end %>
+  <%= f.hidden_field :codes_only, value: request.params["codes_only"] %>
+
   <div class="govuk-button-group">
     <%= f.govuk_submit "Save changes" %>
     <%= govuk_button_link_to "Cancel", sales_logs_path, secondary: true %>

config/routes.rb

@@ -216,8 +216,8 @@ Rails.application.routes.draw do
 
       %w[years status needstype assigned-to owned-by managed-by].each do |filter|
         get "filters/#{filter}", to: "lettings_logs_filters##{filter.underscore}"
+        get "filters/update_#{filter}", to: "lettings_logs_filters#update_#{filter.underscore}"
       end
-      post "filters", to: "lettings_logs_filters#update"
 
       resources :bulk_upload_lettings_logs, path: "bulk-upload-logs", only: %i[show update] do
         collection do
@@ -286,8 +286,8 @@ Rails.application.routes.draw do
 
       %w[years status assigned-to owned-by managed-by].each do |filter|
         get "filters/#{filter}", to: "sales_logs_filters##{filter.underscore}"
+        get "filters/update_#{filter}", to: "sales_logs_filters#update_#{filter.underscore}"
       end
-      post "filters", to: "sales_logs_filters#update"
 
       resources :bulk_upload_sales_logs, path: "bulk-upload-logs" do
         collection do

spec/features/lettings_log_spec.rb

@@ -133,10 +133,8 @@ RSpec.describe "Lettings Log Features" do
           click_button("Save changes")
 
           expect(page).to have_selector(".govuk-error-summary__title")
-          expect(page).to have_selector("#forms-filter-form-years-error")
           expect(page).to have_content("There is a problem")
-
+          choose("years-#{lettings_log.form.start_date.year}-field", allow_label_click: true)
-          choose("forms-filter-form-years-#{lettings_log.form.start_date.year}-field", allow_label_click: true)
           click_button("Save changes")
 
           expect(page).to have_current_path("/lettings-logs/csv-download?codes_only=false&search=1")
@@ -148,10 +146,9 @@ RSpec.describe "Lettings Log Features" do
           click_button("Save changes")
 
           expect(page).to have_selector(".govuk-error-summary__title")
-          expect(page).to have_selector("#forms-filter-form-years-error")
           expect(page).to have_content("There is a problem")
 
-          choose("forms-filter-form-years-2023-field", allow_label_click: true)
+          choose("years-#{lettings_log.form.start_date.year}-field", allow_label_click: true)
           click_button("Save changes")
 
           expect(page).to have_current_path("/lettings-logs/csv-download?codes_only=true&search=1")
@@ -183,14 +180,14 @@ RSpec.describe "Lettings Log Features" do
           click_link("Change", href: "/lettings-logs/filters/needstype?search=1&codes_only=true")
           expect(page).to have_current_path("/lettings-logs/filters/needstype?search=1&codes_only=true")
 
-          check("forms-filter-form-needstypes-1-field", allow_label_click: true)
+          check("needstypes-1-field", allow_label_click: true)
           click_button("Save changes")
 
           expect(page).to have_current_path("/lettings-logs/csv-download?codes_only=true&search=1")
           expect(page).to have_content("You've selected 1 logs")
 
           click_link("Change", href: "/lettings-logs/filters/status?search=1&codes_only=true")
-          check("forms-filter-form-status-not-started-field", allow_label_click: true)
+          check("status-not-started-field", allow_label_click: true)
           click_button("Save changes")
 
           expect(page).to have_content("You haven't selected any logs. Please check your filters")

spec/features/sales_log_spec.rb

@@ -159,10 +159,9 @@ RSpec.describe "Sales Log Features" do
           click_button("Save changes")
 
           expect(page).to have_selector(".govuk-error-summary__title")
-          expect(page).to have_selector("#forms-filter-form-years-error")
           expect(page).to have_content("There is a problem")
 
-          choose("forms-filter-form-years-2023-field", allow_label_click: true)
+          choose("years-2023-field", allow_label_click: true)
           click_button("Save changes")
 
           expect(page).to have_current_path("/sales-logs/csv-download?codes_only=false&search=1")
@@ -174,10 +173,9 @@ RSpec.describe "Sales Log Features" do
           click_button("Save changes")
 
           expect(page).to have_selector(".govuk-error-summary__title")
-          expect(page).to have_selector("#forms-filter-form-years-error")
           expect(page).to have_content("There is a problem")
 
-          choose("forms-filter-form-years-2023-field", allow_label_click: true)
+          choose("years-2023-field", allow_label_click: true)
           click_button("Save changes")
 
           expect(page).to have_current_path("/sales-logs/csv-download?codes_only=true&search=1")
@@ -209,13 +207,13 @@ RSpec.describe "Sales Log Features" do
           expect(page).to have_content("You've selected 2 logs")
           click_link("Change", href: "/sales-logs/filters/assigned-to?search=1&codes_only=true")
 
-          choose("forms-filter-form-assigned-to-you-field", allow_label_click: true)
+          choose("assigned-to-you-field", allow_label_click: true)
           click_button("Save changes")
 
           expect(page).to have_content("You've selected 1 logs")
 
           click_link("Change", href: "/sales-logs/filters/status?search=1&codes_only=true")
-          check("forms-filter-form-status-not-started-field", allow_label_click: true)
+          check("status-not-started-field", allow_label_click: true)
           click_button("Save changes")
 
           expect(page).to have_content("You haven't selected any logs. Please check your filters")