From 72c45ec23f9e3802ba5f2e915e5d652a450bc064 Mon Sep 17 00:00:00 2001 From: Ryan Brown Date: Thu, 24 Aug 2023 16:59:25 +0100 Subject: [PATCH 1/3] Use govuk date component for schedule publication date field This commit switches the schedule publication field to use a date time field component that leverages the GOVUK shared date field component, in order to ensure consistency with other GOVUK interfaces. It maintains the existing request shape and data types. Once all uses of the datetime fields component have been replaced with our new component, we will delete the old component and remove the awkward `with_govuk_date_component` suffix from the new one. --- .../_scheduled_publication_fields.html.erb | 13 +- ..._fields_with_govuk_date_component.html.erb | 118 ++++++++++++++++++ ...oller_scheduled_publishing_test_helpers.rb | 15 +-- 3 files changed, 137 insertions(+), 9 deletions(-) create mode 100644 app/views/components/_datetime_fields_with_govuk_date_component.html.erb diff --git a/app/views/admin/editions/_scheduled_publication_fields.html.erb b/app/views/admin/editions/_scheduled_publication_fields.html.erb index 345a2ed418c..34bdc56febc 100644 --- a/app/views/admin/editions/_scheduled_publication_fields.html.erb +++ b/app/views/admin/editions/_scheduled_publication_fields.html.erb @@ -13,24 +13,33 @@ label: "Schedule for publication", value: 1, checked: edition.scheduled_publication.present?, - conditional: render("components/datetime_fields", { + conditional: render("components/datetime_fields_with_govuk_date_component", { field_name: "scheduled_publication", prefix: "edition", error_items: errors_for(edition.errors, :scheduled_publication), date_heading: "Date", - date_hint: "For example, 01 August 2022", + date_hint: "For example, 01 08 2022", time_hint: "For example, 09:30 or 19:30", year: { value: edition.scheduled_publication&.year || Time.zone.today.year, id: "edition_scheduled_publication_1i", + name: "edition[scheduled_publication(1i)]", + label: "Year", + width: 4, }, month: { value: edition.scheduled_publication&.month || Time.zone.today.month, id: "edition_scheduled_publication_2i", + name: "edition[scheduled_publication(2i)]", + label: "Month", + width: 2, }, day: { value: edition.scheduled_publication&.day || Time.zone.today.day, id: "edition_scheduled_publication_3i", + name: "edition[scheduled_publication(3i)]", + label: "Day", + width: 2, }, hour: { value: edition.scheduled_publication&.hour || 9, diff --git a/app/views/components/_datetime_fields_with_govuk_date_component.html.erb b/app/views/components/_datetime_fields_with_govuk_date_component.html.erb new file mode 100644 index 00000000000..70a2a6e67f6 --- /dev/null +++ b/app/views/components/_datetime_fields_with_govuk_date_component.html.erb @@ -0,0 +1,118 @@ +<% + prefix = prefix + field_name = field_name + id = id + date_id = "#{id}_date" + date_only ||= false + date_heading ||= nil + + error_items = error_items ||= nil + + heading_level = heading_level ||= nil + heading_size = heading_size ||= nil + + date_hint = date_hint ||= nil + + time_hint = time_hint ||= nil + time_hint_id = "time-hint-#{SecureRandom.hex(4)}" + + year ||= {} + + month ||= {} + + day ||= {} + + hour ||= {} + hour_value = hour[:value] + hour_select_id = hour[:id] || "select-hour-#{SecureRandom.hex(4)}" + hour_label_id = "hour-#{SecureRandom.hex(4)}" + + minute ||= {} + minute_value = minute[:value] + minute_select_id = minute[:id] || "select-minute-#{SecureRandom.hex(4)}" + minute_label_id = "minute-#{SecureRandom.hex(4)}" + + root_classes = %w[app-c-datetime-fields govuk-form-group] + root_classes << "govuk-form-group--error" if error_items.present? + data_attributes ||= {} +%> + +<%= tag.div class: root_classes, data: data_attributes, id: id do %> + <% unless date_only && !date_heading %> + <%= render "govuk_publishing_components/components/heading", { + text: date_heading || "Date (required)", + heading_level: heading_level || 3, + font_size: heading_size || "m", + padding: true, + } %> + <% end %> + + <%= render "govuk_publishing_components/components/date_input", { + id: date_id, + hint: date_hint, + error_items: error_items, + items: [day, month, year] + } %> + + <% unless date_only %> +
+ <%= render "govuk_publishing_components/components/heading", { + text: "Time", + heading_level: heading_level || 3, + font_size: heading_size || "m", + padding: true, + } %> +
+ + <% if time_hint %> + <%= render "govuk_publishing_components/components/hint", { + text: time_hint, + id: time_hint_id + } %> + <% end %> + +
+
+ <%= render "govuk_publishing_components/components/label", { + text: "Hour", + html_for: hour_select_id, + id: hour_label_id, + } %> + + <%= select_hour hour_value, + { + include_blank: true, + prefix: prefix, + field_name: "#{field_name}(4i)" + }, + { + id: hour_select_id, + class: "govuk-select app-c-datetime-fields__date-time-input", + "aria-describedby": "#{hour_label_id} #{time_hint_id if time_hint.present?}".strip + } %> +
+ +

:

+ +
+ <%= render "govuk_publishing_components/components/label", { + text: "Minute", + html_for: minute_select_id, + id: minute_label_id, + } %> + + <%= select_minute minute_value, + { + include_blank: true, + prefix: prefix, + field_name: "#{field_name}(5i)" + }, + { + id: minute_select_id, + class: "govuk-select app-c-datetime-fields__date-time-input", + "aria-describedby": "#{minute_label_id} #{time_hint_id if time_hint.present?}".strip + } %> +
+
+ <% end %> +<% end %> \ No newline at end of file diff --git a/test/support/admin_edition_controller_scheduled_publishing_test_helpers.rb b/test/support/admin_edition_controller_scheduled_publishing_test_helpers.rb index e4768159b0a..d94253944e7 100644 --- a/test/support/admin_edition_controller_scheduled_publishing_test_helpers.rb +++ b/test/support/admin_edition_controller_scheduled_publishing_test_helpers.rb @@ -21,7 +21,8 @@ def should_allow_scheduled_publication_of(edition_type) assert_select "form#new_edition" do assert_select "input[type=checkbox][name='scheduled_publication_active']" - assert_select "select[name*='edition[scheduled_publication']", count: 5 + assert_select "input[type=text][name*='edition[scheduled_publication']", count: 3 + assert_select "select[name*='edition[scheduled_publication']", count: 2 end end @@ -121,9 +122,9 @@ def should_allow_scheduled_publication_of(edition_type) assert_select "form#edit_edition" do assert_select "input[type=checkbox][name='scheduled_publication_active'][checked='checked']" - assert_select "select[name='edition[scheduled_publication(1i)]'] option[value='#{Time.zone.today.year + 1}'][selected='selected']" - assert_select "select[name='edition[scheduled_publication(2i)]'] option[value='6'][selected='selected']" - assert_select "select[name='edition[scheduled_publication(3i)]'] option[value='3'][selected='selected']" + assert_select "input[type=text][name='edition[scheduled_publication(1i)]'][value='#{Time.zone.today.year + 1}']" + assert_select "input[type=text][name='edition[scheduled_publication(2i)]'][value='6']" + assert_select "input[type=text][name='edition[scheduled_publication(3i)]'][value='3']" assert_select "select[name='edition[scheduled_publication(4i)]'] option[value='10'][selected='selected']" assert_select "select[name='edition[scheduled_publication(5i)]'] option[value='30'][selected='selected']" end @@ -138,9 +139,9 @@ def should_allow_scheduled_publication_of(edition_type) assert_select "form#edit_edition" do assert_select "input[type=checkbox][name='scheduled_publication_active']" assert_select "input[type=checkbox][name='scheduled_publication_active'][checked='checked']", count: 0 - assert_select "select[name='edition[scheduled_publication(1i)]'] option[value='#{date.year}'][selected='selected']" - assert_select "select[name='edition[scheduled_publication(2i)]'] option[value='#{date.month}'][selected='selected']" - assert_select "select[name='edition[scheduled_publication(3i)]'] option[value='#{date.day}'][selected='selected']" + assert_select "input[name='edition[scheduled_publication(1i)]'][value='#{date.year}']" + assert_select "input[name='edition[scheduled_publication(2i)]'][value='#{date.month}']" + assert_select "input[name='edition[scheduled_publication(3i)]'][value='#{date.day}']" assert_select "select[name='edition[scheduled_publication(4i)]'] option[value='09'][selected='selected']" assert_select "select[name='edition[scheduled_publication(5i)]'] option[value='30'][selected='selected']" end From f87fbc971fe9a217975c58aec9b6ed65930af709 Mon Sep 17 00:00:00 2001 From: Ryan Brown Date: Wed, 6 Sep 2023 12:00:42 +0100 Subject: [PATCH 2/3] Validate scheduled publication input Without this change, it is possible to crash Whitehall by entering an invalid date such as 2023/10/40 in the schedule publication input. The custom setter verifies that the input date is valid and numeric and sets an error on the scheduled publication value if that is not the case. --- app/models/edition.rb | 26 +++++++++++++++++++ .../app/models/edition/validation_test.rb | 20 ++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/app/models/edition.rb b/app/models/edition.rb index 109b0d02568..ed3c17e1e4a 100644 --- a/app/models/edition.rb +++ b/app/models/edition.rb @@ -54,6 +54,7 @@ class Edition < ApplicationRecord validates_each :first_published_at do |record, attr, value| record.errors.add(attr, "can't be set to a future date") if value && Time.zone.now < value end + validate :scheduled_publication, :valid_date UNMODIFIABLE_STATES = %w[scheduled published superseded deleted].freeze FROZEN_STATES = %w[superseded deleted].freeze @@ -714,6 +715,31 @@ def publishing_api_presenter PublishingApi::GenericEditionPresenter end + def scheduled_publication=(date) + if date.is_a?(Hash) + @date_field_validity = {} if @date_field_validity.nil? + + begin + raise ArgumentError if date.values.any?(&:nil?) + raise ArgumentError if date.values.any? { |date_part| date_part.to_i.zero? } + + Date.new(date[1], date[2], date[3]) + @date_field_validity[:scheduled_publication] = true + rescue ArgumentError + @date_field_validity[:scheduled_publication] = false + date = nil + end + end + + super(date) + end + + def valid_date + if @date_field_validity.present? && @date_field_validity[:scheduled_publication] == false + errors.add(:scheduled_publication, "must be a valid date in the format XX XX XXXX") + end + end + private def date_for_government diff --git a/test/unit/app/models/edition/validation_test.rb b/test/unit/app/models/edition/validation_test.rb index a47cf9a463c..d060d3bab1b 100644 --- a/test/unit/app/models/edition/validation_test.rb +++ b/test/unit/app/models/edition/validation_test.rb @@ -194,4 +194,24 @@ class Edition::ValidationTest < ActiveSupport::TestCase edition.supporting_organisations = [organisation1] assert edition.valid? end + + test "should be valid when scheduled publication date is a valid date" do + edition = build(:draft_edition, scheduled_publication: { 1 => 2023, 2 => 9, 3 => 10 }) + assert edition.valid? + end + + test "should be invalid when scheduled publication date is an invalid date" do + edition = build(:draft_edition, scheduled_publication: { 1 => 2023, 2 => 9, 3 => 40 }) + assert_not edition.valid? + end + + test "should be invalid when scheduled publication date is partially completed" do + edition = build(:draft_edition, scheduled_publication: { 1 => 2023, 2 => nil, 3 => 9 }) + assert_not edition.valid? + end + + test "should be invalid when not all scheduled publication date parts are numeric" do + edition = build(:draft_edition, scheduled_publication: { 1 => 2023, 2 => "January", 3 => 20 }) + assert_not edition.valid? + end end From 195007831b7da90d467ada4fcce751c9fed622ad Mon Sep 17 00:00:00 2001 From: davidgisbey Date: Wed, 6 Sep 2023 12:32:00 +0100 Subject: [PATCH 3/3] Retain scheduled publication values when invalid date is input At the moment, when you input an invalid date for a scheduled publication, an error is added to the edition. The persisted value is retained on the edition and used to populate the fields. This means that the users input is lost. This digs through the params to look for the users input and uses it if found. If not the existing behaviour remains. It looks for a value on the edition and if not uses the default values. --- .../_scheduled_publication_fields.html.erb | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/app/views/admin/editions/_scheduled_publication_fields.html.erb b/app/views/admin/editions/_scheduled_publication_fields.html.erb index 34bdc56febc..361f76377bc 100644 --- a/app/views/admin/editions/_scheduled_publication_fields.html.erb +++ b/app/views/admin/editions/_scheduled_publication_fields.html.erb @@ -1,6 +1,13 @@
<%= hidden_field_tag :scheduled_publication_active, 0, id: "" %> + <% ## The date input needs a custom edition_scheduled_publication id passed into the day input so we can anchor error to the first input. %> + + <% + hour_param = params.dig("edition", "scheduled_publication(4i)") + minute_param = params.dig("edition", "scheduled_publication(5i)") + %> + <%= render "govuk_publishing_components/components/checkboxes", { name: "scheduled_publication_active", id: "scheduled_publication_active", @@ -12,7 +19,7 @@ { label: "Schedule for publication", value: 1, - checked: edition.scheduled_publication.present?, + checked: params[:scheduled_publication_active] || edition.scheduled_publication.present?, conditional: render("components/datetime_fields_with_govuk_date_component", { field_name: "scheduled_publication", prefix: "edition", @@ -21,32 +28,32 @@ date_hint: "For example, 01 08 2022", time_hint: "For example, 09:30 or 19:30", year: { - value: edition.scheduled_publication&.year || Time.zone.today.year, + value: params.dig("edition", "scheduled_publication(1i)") || edition.scheduled_publication&.year || Time.zone.today.year, id: "edition_scheduled_publication_1i", name: "edition[scheduled_publication(1i)]", label: "Year", width: 4, }, month: { - value: edition.scheduled_publication&.month || Time.zone.today.month, + value: params.dig("edition", "scheduled_publication(2i)") || edition.scheduled_publication&.month || Time.zone.today.month, id: "edition_scheduled_publication_2i", name: "edition[scheduled_publication(2i)]", label: "Month", width: 2, }, day: { - value: edition.scheduled_publication&.day || Time.zone.today.day, - id: "edition_scheduled_publication_3i", + value: params.dig("edition", "scheduled_publication(3i)") || edition.scheduled_publication&.day || Time.zone.today.day, + id: "edition_scheduled_publication", name: "edition[scheduled_publication(3i)]", label: "Day", width: 2, }, hour: { - value: edition.scheduled_publication&.hour || 9, + value: hour_param ? hour_param.to_i : (edition.scheduled_publication&.hour || 9), id: "edition_scheduled_publication_4i", }, minute: { - value: edition.scheduled_publication&.min || 30, + value: minute_param ? minute_param.to_i : (edition.scheduled_publication&.min || 30), id: "edition_scheduled_publication_5i", }, })