From fa60e103f2281fb207816d3a7a52c4924f662afb Mon Sep 17 00:00:00 2001 From: Harriet H-W Date: Fri, 29 Nov 2024 14:09:15 +0000 Subject: [PATCH] add first pass at error handling for filter dates this does not include case where one or more of the values is blank, or where the 'to' date is before the 'from' date. if there are errors, we do the rest of the filters without the erroring ones, so as to return something helpful. --- .../index/date_filter_component.html.erb | 2 + .../document/index/date_filter_component.rb | 3 +- .../index/filter_options_component.html.erb | 2 +- .../index/filter_options_component.rb | 3 +- .../content_block/documents_controller.rb | 7 ++- .../content_block/document/document_filter.rb | 50 ++++++++++++++----- .../content_block/documents/index.html.erb | 8 +++ .../features/search_for_object.feature | 6 +++ .../content_block_manager_steps.rb | 15 ++++++ .../index/date_filter_component_test.rb | 15 ++++++ .../index/filter_options_component_test.rb | 13 +++-- 11 files changed, 105 insertions(+), 19 deletions(-) diff --git a/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/date_filter_component.html.erb b/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/date_filter_component.html.erb index 1c26a59f5ef..478ac1fb0d5 100644 --- a/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/date_filter_component.html.erb +++ b/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/date_filter_component.html.erb @@ -4,6 +4,7 @@ prefix: "last_updated_from", date_heading: "From", date_only: true, + error_items: helpers.errors_for(@errors, "last_updated_from"), year: { id: "last_updated_from_1i", name: "last_updated_from[1i]", @@ -32,6 +33,7 @@ prefix: "last_updated_to", date_heading: "To", date_only: true, + error_items: helpers.errors_for(@errors, "last_updated_to"), year: { id: "last_updated_to_1i", name: "last_updated_to[1i]", diff --git a/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/date_filter_component.rb b/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/date_filter_component.rb index e242c50e53d..7a0bf3ff07b 100644 --- a/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/date_filter_component.rb +++ b/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/date_filter_component.rb @@ -1,5 +1,6 @@ class ContentBlockManager::ContentBlock::Document::Index::DateFilterComponent < ViewComponent::Base - def initialize(filters: nil) + def initialize(filters: nil, errors: nil) @filters = filters + @errors = errors end end diff --git a/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/filter_options_component.html.erb b/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/filter_options_component.html.erb index cad79ee1d14..3f48dc5de97 100644 --- a/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/filter_options_component.html.erb +++ b/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/filter_options_component.html.erb @@ -62,7 +62,7 @@ }, content: { html: ( - render(ContentBlockManager::ContentBlock::Document::Index::DateFilterComponent.new(filters: @filters)) + render(ContentBlockManager::ContentBlock::Document::Index::DateFilterComponent.new(filters: @filters, errors: @errors)) ), }, expanded: true, diff --git a/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/filter_options_component.rb b/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/filter_options_component.rb index 4fd6218a900..898d748161b 100644 --- a/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/filter_options_component.rb +++ b/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/index/filter_options_component.rb @@ -1,7 +1,8 @@ class ContentBlockManager::ContentBlock::Document::Index::FilterOptionsComponent < ViewComponent::Base include ActionView::Helpers::RecordTagHelper - def initialize(filters:) + def initialize(filters:, errors: nil) @filters = filters + @errors = errors end private diff --git a/lib/engines/content_block_manager/app/controllers/content_block_manager/content_block/documents_controller.rb b/lib/engines/content_block_manager/app/controllers/content_block_manager/content_block/documents_controller.rb index 017aff44708..22d71457ab8 100644 --- a/lib/engines/content_block_manager/app/controllers/content_block_manager/content_block/documents_controller.rb +++ b/lib/engines/content_block_manager/app/controllers/content_block_manager/content_block/documents_controller.rb @@ -3,7 +3,12 @@ def index if params_filters.any? session[:content_block_filters] = params_filters @filters = params_filters - @content_block_documents = ContentBlockManager::ContentBlock::Document::DocumentFilter.new(@filters).paginated_documents + filter_result = ContentBlockManager::ContentBlock::Document::DocumentFilter.new(@filters) + @content_block_documents = filter_result.paginated_documents + unless filter_result.valid? + @errors = filter_result.errors + @error_summary_errors = @errors.map { |error| { text: error.full_message, href: "##{error.attribute}_3i" } } + end render :index elsif params[:reset_fields].blank? && session_filters.any? redirect_to content_block_manager.content_block_manager_root_path(session_filters) diff --git a/lib/engines/content_block_manager/app/models/content_block_manager/content_block/document/document_filter.rb b/lib/engines/content_block_manager/app/models/content_block_manager/content_block/document/document_filter.rb index af4f9d2dc3d..b5abadb6c30 100644 --- a/lib/engines/content_block_manager/app/models/content_block_manager/content_block/document/document_filter.rb +++ b/lib/engines/content_block_manager/app/models/content_block_manager/content_block/document/document_filter.rb @@ -1,15 +1,39 @@ module ContentBlockManager class ContentBlock::Document::DocumentFilter + FILTER_ERROR = Data.define(:attribute, :full_message) + attr_reader :errors + def initialize(filters = {}) @filters = filters + @errors = [] end def paginated_documents unpaginated_documents.page(page).per(default_page_size) end + def valid? + return @valid if defined?(@valid) + + @valid ||= begin + validate_date(:last_updated_from) + validate_date(:last_updated_to) + + errors.empty? + end + end + private + def validate_date(key) + return unless is_date_present?(key) + + date = date_from_filters(key) + Time.zone.local(date[:year], date[:month], date[:day]) + rescue ArgumentError, TypeError, NoMethodError + @errors << FILTER_ERROR.new(attribute: key.to_s, full_message: "#{key.to_s.humanize} is not a valid date") + end + def page @filters[:page].presence || 1 end @@ -22,23 +46,25 @@ def is_date_present?(date_key) @filters[date_key].present? && @filters[date_key].all? { |_, value| value.present? } end + def date_from_filters(date_key) + filter = @filters[date_key] + year = filter["1i"].to_i + month = filter["2i"].to_i + day = filter["3i"].to_i + { year:, month:, day: } + end + def from_date @from_date ||= if is_date_present?(:last_updated_from) - filter = @filters[:last_updated_from] - year = filter["1i"].to_i - month = filter["2i"].to_i - day = filter["3i"].to_i - Time.zone.local(year, month, day) + date = date_from_filters(:last_updated_from) + Time.zone.local(date[:year], date[:month], date[:day]) end end def to_date @to_date ||= if is_date_present?(:last_updated_to) - filter = @filters[:last_updated_to] - year = filter["1i"].to_i - month = filter["2i"].to_i - day = filter["3i"].to_i - Time.zone.local(year, month, day, 23, 59, 59) + date = date_from_filters(:last_updated_to) + Time.zone.local(date[:year], date[:month], date[:day], 23, 59, 59) end end @@ -49,8 +75,8 @@ def unpaginated_documents documents = documents.with_keyword(@filters[:keyword]) if @filters[:keyword].present? documents = documents.where(block_type: @filters[:block_type]) if @filters[:block_type].present? documents = documents.with_lead_organisation(@filters[:lead_organisation]) if @filters[:lead_organisation].present? - documents = documents.from_date(from_date) if from_date - documents = documents.to_date(to_date) if to_date + documents = documents.from_date(from_date) if valid? && from_date + documents = documents.to_date(to_date) if valid? && to_date documents.order("content_block_editions.updated_at DESC") end end diff --git a/lib/engines/content_block_manager/app/views/content_block_manager/content_block/documents/index.html.erb b/lib/engines/content_block_manager/app/views/content_block_manager/content_block/documents/index.html.erb index c36f9c28a31..3d74a20fc84 100644 --- a/lib/engines/content_block_manager/app/views/content_block_manager/content_block/documents/index.html.erb +++ b/lib/engines/content_block_manager/app/views/content_block_manager/content_block/documents/index.html.erb @@ -1,6 +1,13 @@ <% content_for :title_margin_bottom, 6 %> +<% if @error_summary_errors %> + <%= render "govuk_publishing_components/components/error_summary", { + title: "There is a problem", + items: @error_summary_errors, + } %> +<% end %> +

@@ -29,6 +36,7 @@ <%= render( ContentBlockManager::ContentBlock::Document::Index::FilterOptionsComponent.new( filters: @filters, + errors: @errors, ), ) %>

diff --git a/lib/engines/content_block_manager/features/search_for_object.feature b/lib/engines/content_block_manager/features/search_for_object.feature index e3654c763ca..4e8bc8a18ad 100644 --- a/lib/engines/content_block_manager/features/search_for_object.feature +++ b/lib/engines/content_block_manager/features/search_for_object.feature @@ -71,6 +71,12 @@ Feature: Search for a content object And I click to view results And "1" content blocks are returned in total + Scenario: GDS Editor sees errors when searching by invalid dates + When I visit the Content Block Manager home page + And I input invalid dates to filter by + And I click to view results + Then I should see a message that the filter dates are invalid + @javascript Scenario: GDS Editor can copy embed code When I visit the Content Block Manager home page diff --git a/lib/engines/content_block_manager/features/step_definitions/content_block_manager_steps.rb b/lib/engines/content_block_manager/features/step_definitions/content_block_manager_steps.rb index 729d941fd7e..85768eb6879 100644 --- a/lib/engines/content_block_manager/features/step_definitions/content_block_manager_steps.rb +++ b/lib/engines/content_block_manager/features/step_definitions/content_block_manager_steps.rb @@ -401,6 +401,11 @@ def should_show_edit_form_for_email_address_content_block(document_title, email_ assert_text "#{ContentBlockManager::ContentBlock::Edition.human_attribute_name("details_#{field_name}")} is an invalid #{format.titleize}" end +Then("I should see a message that the filter dates are invalid") do + expect(page).to have_selector("a[href='#last_updated_from_3i']"), text: "Last updated from is not a valid date" + expect(page).to have_selector("a[href='#last_updated_to_3i']"), text: "Last updated to is not a valid date" +end + Then("I should see a permissions error") do assert_text "Permissions error" end @@ -595,6 +600,16 @@ def should_show_edit_form_for_email_address_content_block(document_title, email_ fill_in "last_updated_to_3i", with: date.day end +When("I input invalid dates to filter by") do + fill_in "last_updated_from_1i", with: "1" + fill_in "last_updated_from_2i", with: "34" + fill_in "last_updated_from_3i", with: "56" + + fill_in "last_updated_to_1i", with: "1" + fill_in "last_updated_to_2i", with: "67" + fill_in "last_updated_to_3i", with: "56" +end + When("I enter an invalid date") do fill_in "Year", with: "01" end diff --git a/lib/engines/content_block_manager/test/components/content_block/document/index/date_filter_component_test.rb b/lib/engines/content_block_manager/test/components/content_block/document/index/date_filter_component_test.rb index b94cd1c95a5..7c0eff6a9fa 100644 --- a/lib/engines/content_block_manager/test/components/content_block/document/index/date_filter_component_test.rb +++ b/lib/engines/content_block_manager/test/components/content_block/document/index/date_filter_component_test.rb @@ -37,4 +37,19 @@ class ContentBlockManager::ContentBlock::Document::Index::DateFilterComponentTes assert_selector "input[name='last_updated_to[2i]'][value='4']" assert_selector "input[name='last_updated_to[1i]'][value='2026']" end + + it "renders errors if there are errors on the date filter" do + errors = [ + ContentBlockManager::ContentBlock::Document::DocumentFilter::FILTER_ERROR.new( + attribute: "last_updated_from", full_message: "From date is not in the correct format", + ), + ContentBlockManager::ContentBlock::Document::DocumentFilter::FILTER_ERROR.new( + attribute: "last_updated_to", full_message: "To date is not in the correct format", + ), + ] + render_inline(ContentBlockManager::ContentBlock::Document::Index::DateFilterComponent.new(errors:)) + + assert_selector ".govuk-error-message", text: "From date is not in the correct format" + assert_selector ".govuk-error-message", text: "To date is not in the correct format" + end end diff --git a/lib/engines/content_block_manager/test/components/content_block/document/index/filter_options_component_test.rb b/lib/engines/content_block_manager/test/components/content_block/document/index/filter_options_component_test.rb index 40c874b69d6..f2c35631304 100644 --- a/lib/engines/content_block_manager/test/components/content_block/document/index/filter_options_component_test.rb +++ b/lib/engines/content_block_manager/test/components/content_block/document/index/filter_options_component_test.rb @@ -82,14 +82,21 @@ class ContentBlockManager::ContentBlock::Document::Index::FilterOptionsComponent assert_selector "option[selected='selected'][value=2]" end - it "passes filters to Date component" do + it "passes filters and errors to Date component" do filters = { lead_organisation: "2" } - date_component = ContentBlockManager::ContentBlock::Document::Index::DateFilterComponent.new(filters:) - ContentBlockManager::ContentBlock::Document::Index::DateFilterComponent.expects(:new).with(filters:).returns(date_component) + errors = [ + ContentBlockManager::ContentBlock::Document::DocumentFilter::FILTER_ERROR.new( + attribute: "last_updated_from", full_message: "From date is not in the correct format", + ), + ] + date_component = ContentBlockManager::ContentBlock::Document::Index::DateFilterComponent.new(filters:, errors:) + ContentBlockManager::ContentBlock::Document::Index::DateFilterComponent.expects(:new).with(filters:, errors:) + .returns(date_component) render_inline( ContentBlockManager::ContentBlock::Document::Index::FilterOptionsComponent.new( filters:, + errors:, ), )