Skip to content

Commit

Permalink
add first pass at error handling for filter dates
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Harriethw committed Nov 29, 2024
1 parent df77163 commit fa60e10
Show file tree
Hide file tree
Showing 11 changed files with 105 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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]",
Expand Down Expand Up @@ -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]",
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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

Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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 %>

<div class="govuk-grid-row content-block-manager-header">
<div class="govuk-grid-column-one-half">
<h1 class="gem-c-title__text govuk-heading-xl">
Expand Down Expand Up @@ -29,6 +36,7 @@
<%= render(
ContentBlockManager::ContentBlock::Document::Index::FilterOptionsComponent.new(
filters: @filters,
errors: @errors,
),
) %>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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:,
),
)

Expand Down

0 comments on commit fa60e10

Please sign in to comment.