From 63ed7c27b6c6e5a282722a258e147c79d977ae85 Mon Sep 17 00:00:00 2001 From: Harriet H-W Date: Fri, 29 Nov 2024 14:09:15 +0000 Subject: [PATCH] Handle errors when dates are invalid 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 | 58 +++++++++++++---- .../content_block/documents/index.html.erb | 8 +++ .../config/locales/en.yml | 7 ++ .../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 +++- .../document/document_filter_test.rb | 64 +++++++++++++++++++ 13 files changed, 183 insertions(+), 20 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 7ef7de89fcb..37e4522c219 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 67df1e16138..dc1dd44e0dd 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,6 +1,7 @@ class ContentBlockManager::ContentBlock::Document::Index::DateFilterComponent < ViewComponent::Base - def initialize(filters: nil) + def initialize(filters: nil, errors: nil) @filters = filters + @errors = errors end private 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 813fd1e335b..46ef0630090 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,5 +1,7 @@ module ContentBlockManager class ContentBlock::Document::DocumentFilter + FILTER_ERROR = Data.define(:attribute, :full_message) + def initialize(filters = {}) @filters = filters end @@ -8,8 +10,36 @@ def paginated_documents unpaginated_documents.page(page).per(default_page_size) end + def errors + @errors ||= begin + @errors = [] + from = validate_date(:last_updated_from) + to = validate_date(:last_updated_to) + + if @errors.empty? && to.present? && from.present? && from.after?(to) + @errors << FILTER_ERROR.new(attribute: "last_updated_from", full_message: I18n.t("content_block_document.index.errors.date.range.invalid")) + end + + @errors + end + end + + def valid? + errors.empty? + 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, RangeError + @errors << FILTER_ERROR.new(attribute: key.to_s, full_message: I18n.t("content_block_document.index.errors.date.invalid", attribute: key.to_s.humanize)) + nil + end + def page @filters[:page].presence || 1 end @@ -19,26 +49,28 @@ def default_page_size end def is_date_present?(date_key) - @filters[date_key].present? && @filters[date_key].all? { |_, value| value.present? } + @filters[date_key].present? && @filters[date_key].any? { |_, 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).end_of_day + date = date_from_filters(:last_updated_to) + Time.zone.local(date[:year], date[:month], date[:day]).end_of_day end end @@ -49,8 +81,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/config/locales/en.yml b/lib/engines/content_block_manager/config/locales/en.yml index ee379e2ad91..ec5ffbdd9cf 100644 --- a/lib/engines/content_block_manager/config/locales/en.yml +++ b/lib/engines/content_block_manager/config/locales/en.yml @@ -3,6 +3,13 @@ en: attributes: content_block_manager/content_block/edition/document: title: Title + content_block_document: + index: + errors: + date: + invalid: "%{attribute} is not a valid date" + range: + invalid: "From date must be before to date" content_block_edition: confirmation_page: scheduled: 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 30908b501a5..60901eca41a 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 @@ -441,6 +441,11 @@ def should_show_edit_form_for_email_address_content_block(document_title, email_ assert_text "Confirm details are correct", minimum: 2 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 @@ -675,6 +680,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:, ), ) diff --git a/lib/engines/content_block_manager/test/unit/app/models/content_block_edition/document/document_filter_test.rb b/lib/engines/content_block_manager/test/unit/app/models/content_block_edition/document/document_filter_test.rb index cbdafd89258..50ac25629c5 100644 --- a/lib/engines/content_block_manager/test/unit/app/models/content_block_edition/document/document_filter_test.rb +++ b/lib/engines/content_block_manager/test/unit/app/models/content_block_edition/document/document_filter_test.rb @@ -115,4 +115,68 @@ class ContentBlockManager::DocumentFilterTest < ActiveSupport::TestCase end end end + + describe "date validation" do + let(:document_filter) { ContentBlockManager::ContentBlock::Document::DocumentFilter.new(filters) } + + %i[last_updated_from last_updated_to].each do |attribute| + describe "when #{attribute} contains non-date values" do + let(:filters) do + { + "#{attribute}": { "3i" => "ddddd", "2i" => "ffsdfsd", "1i" => "ffff" }, + } + end + + it "returns invalid" do + assert_not document_filter.valid? + end + + it "returns errors" do + errors = document_filter.errors + assert_equal errors.count, 1 + assert_equal errors.first.attribute, attribute.to_s + assert_equal errors.first.full_message, I18n.t("content_block_document.index.errors.date.invalid", attribute: attribute.to_s.humanize) + end + end + + describe "when #{attribute} contains has missing values" do + let(:filters) do + { + "#{attribute}": { "3i" => "", "2i" => "3", "1i" => "2026" }, + } + end + + it "returns invalid" do + assert_not document_filter.valid? + end + + it "returns errors" do + errors = document_filter.errors + assert_equal errors.count, 1 + assert_equal errors.first.attribute, attribute.to_s + assert_equal errors.first.full_message, I18n.t("content_block_document.index.errors.date.invalid", attribute: attribute.to_s.humanize) + end + end + end + + describe "when last_updated_from is after last_updated_to" do + let(:filters) do + { + last_updated_from: { "3i" => "3", "2i" => "2", "1i" => "2026" }, + last_updated_to: { "3i" => "1", "2i" => "1", "1i" => "2025" }, + } + end + + it "returns invalid" do + assert_not document_filter.valid? + end + + it "returns errors" do + errors = document_filter.errors + assert_equal errors.count, 1 + assert_equal errors.first.attribute, "last_updated_from" + assert_equal errors.first.full_message, I18n.t("content_block_document.index.errors.date.range.invalid") + end + end + end end