Merge pull request #6008 from nhsuk/alistair/fix-performed-at-time

Allow `performed_at_time` to be optional for national reporting users

Author: alistair-white-horne-tng

app/controllers/draft_vaccination_records_controller.rb

@@ -70,6 +70,11 @@ def update
 
   def validate_params
     if current_step == :date_and_time
+      TimeParamsNormalizer.call!(
+        params: params[:draft_vaccination_record],
+        field_name: :performed_at_time
+      )
+
       date_validator =
         DateParamsValidator.new(
           field_name: :performed_at_date,

app/lib/time_params_normalizer.rb

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+class TimeParamsNormalizer
+  def initialize(params:, field_name:)
+    @params = params
+    @field_name = field_name
+  end
+
+  HOUR_SUFFIX = "(4i)"
+  MINUTE_SUFFIX = "(5i)"
+  SECOND_SUFFIX = "(6i)"
+
+  def call!
+    if hour_blank? && minute_blank? && seconds_present?
+      @params["#{@field_name}#{SECOND_SUFFIX}"] = ""
+    end
+    @params
+  end
+
+  def self.call!(...) = new(...).call!
+
+  private_class_method :new
+
+  private
+
+  def hour_blank?
+    @params["#{@field_name}#{HOUR_SUFFIX}"].blank?
+  end
+
+  def minute_blank?
+    @params["#{@field_name}#{MINUTE_SUFFIX}"].blank?
+  end
+
+  def seconds_present?
+    @params.key?("#{@field_name}#{SECOND_SUFFIX}")
+  end
+end

app/lib/time_params_validator.rb

@@ -22,6 +22,8 @@ def time_params_valid?
     time = time_params_as_struct
 
     return true if [time.hour, time.minute, time.second].all?(&:blank?)
+    # If `omit_second` is set on the field, then the seconds field defaults to "0"
+    return true if time.hour.blank? && time.minute.blank? && time.second == "0"
 
     if time.second.blank?
       object.errors.add(field_name, :missing_second)

app/models/draft_vaccination_record.rb

@@ -77,7 +77,10 @@ def wizard_steps
   end
 
   on_wizard_step :date_and_time, exact: true do
-    validates :performed_at_date, :performed_at_time, presence: true
+    validates :performed_at_date, presence: true
+    validates :performed_at_time,
+              presence: true,
+              unless: :national_reporting_user_and_record?
     validate :performed_at_date_within_range
   end
 
@@ -159,9 +162,11 @@ def wizard_steps
     validates :delivery_method,
               :delivery_site,
               :performed_at_date,
-              :performed_at_time,
               :protocol,
               presence: true
+    validates :performed_at_time,
+              presence: true,
+              unless: :national_reporting_user_and_record?
     validates :batch_number,
               :batch_expiry,
               presence: true,

config/locales/en.yml

@@ -157,9 +157,9 @@ en:
             performed_at_time:
               blank: Enter a date time
               invalid: Enter a valid time
-              missing_hour: Enter a day
-              missing_minute: Enter a month
-              missing_second: Enter a year
+              missing_hour: Enter an hour
+              missing_minute: Enter a minute
+              missing_second: Enter a second
             performed_by_given_name:
               blank: First name can't be blank
             performed_by_family_name:

spec/features/edit_vaccination_record_spec.rb

@@ -398,6 +398,38 @@
       then_i_should_see_the_vaccination_record
     end
 
+    scenario "Edits the date and time with or without time" do
+      given_i_am_signed_in
+      and_a_national_reporting_vaccination_record_exists
+
+      when_i_navigate_to_the_edit_vaccination_record_page
+
+      when_i_click_on_change_date
+      then_i_should_see_the_date_time_form
+
+      when_i_fill_in_a_valid_date_with_time
+      then_i_see_the_edit_vaccination_record_page
+      and_i_should_see_the_updated_date_with_time
+
+      when_i_click_on_save_changes
+      then_i_should_see_the_vaccination_record
+      and_the_vaccination_record_should_have_the_time
+
+      when_i_click_on_edit_vaccination_record
+      then_i_see_the_edit_vaccination_record_page
+
+      when_i_click_on_change_date
+      then_i_should_see_the_date_time_form
+
+      when_i_fill_in_a_valid_date_without_time
+      then_i_see_the_edit_vaccination_record_page
+      and_i_should_see_the_updated_date_without_time
+
+      when_i_click_on_save_changes
+      then_i_should_see_the_vaccination_record
+      and_the_vaccination_record_should_have_no_time
+    end
+
     scenario "Parent details are not visible when viewing vaccination records" do
       given_i_am_signed_in
       and_a_national_reporting_vaccination_record_exists
@@ -1099,4 +1131,60 @@ def then_i_should_see_a_success_message
       text: "Vaccination outcome recorded for flu"
     )
   end
+
+  def when_i_fill_in_a_valid_date_with_time
+    @valid_date = Date.current - 1.day
+    @valid_time_hour = "14"
+    @valid_time_minute = "30"
+
+    fill_in "Year", with: @valid_date.year.to_s
+    fill_in "Month", with: @valid_date.month.to_s
+    fill_in "Day", with: @valid_date.day.to_s
+
+    fill_in "Hour", with: @valid_time_hour
+    fill_in "Minute", with: @valid_time_minute
+
+    click_on "Continue"
+  end
+
+  def and_i_should_see_the_updated_date_with_time
+    formatted_date = @valid_date.strftime("%-d %B %Y")
+
+    expect(page).to have_content("Date#{formatted_date}")
+    expect(page).to have_content("Time2:30pm")
+  end
+
+  def and_the_vaccination_record_should_have_the_time
+    @vaccination_record.reload
+    expect(@vaccination_record.performed_at_time).not_to be_nil
+    expect(@vaccination_record.performed_at_time.hour).to eq(14)
+    expect(@vaccination_record.performed_at_time.min).to eq(30)
+    expect(@vaccination_record.performed_at_date).to eq(@valid_date)
+  end
+
+  def when_i_fill_in_a_valid_date_without_time
+    @valid_date = Date.current - 1.day
+
+    fill_in "Year", with: @valid_date.year.to_s
+    fill_in "Month", with: @valid_date.month.to_s
+    fill_in "Day", with: @valid_date.day.to_s
+
+    fill_in "Hour", with: ""
+    fill_in "Minute", with: ""
+
+    click_on "Continue"
+  end
+
+  def and_i_should_see_the_updated_date_without_time
+    formatted_date = @valid_date.strftime("%-d %B %Y")
+
+    expect(page).to have_content("Date#{formatted_date}")
+    expect(page).not_to have_content(/Time\d/)
+  end
+
+  def and_the_vaccination_record_should_have_no_time
+    @vaccination_record.reload
+    expect(@vaccination_record.performed_at_time).to be_nil
+    expect(@vaccination_record.performed_at_date).to eq(@valid_date)
+  end
 end

spec/lib/time_params_normalizer_spec.rb

@@ -0,0 +1,154 @@
+# frozen_string_literal: true
+
+describe TimeParamsNormalizer do
+  describe ".call" do
+    subject(:call_normalizer) { described_class.call!(params:, field_name:) }
+
+    let(:field_name) { :performed_at_time }
+
+    context "when hour and minute are blank but seconds is present" do
+      let(:params) do
+        {
+          "performed_at_time(4i)" => "",
+          "performed_at_time(5i)" => "",
+          "performed_at_time(6i)" => "0"
+        }
+      end
+
+      it "blanks the seconds field" do
+        call_normalizer
+        expect(params["performed_at_time(6i)"]).to eq("")
+      end
+
+      it "returns the params hash" do
+        expect(call_normalizer).to eq(params)
+      end
+    end
+
+    context "when hour and minute are nil but seconds is present" do
+      let(:params) do
+        {
+          "performed_at_time(4i)" => nil,
+          "performed_at_time(5i)" => nil,
+          "performed_at_time(6i)" => "0"
+        }
+      end
+
+      it "blanks the seconds field" do
+        call_normalizer
+        expect(params["performed_at_time(6i)"]).to eq("")
+      end
+    end
+
+    context "when all time fields are blank" do
+      let(:params) do
+        {
+          "performed_at_time(4i)" => "",
+          "performed_at_time(5i)" => "",
+          "performed_at_time(6i)" => ""
+        }
+      end
+
+      it "leaves seconds blank" do
+        call_normalizer
+        expect(params["performed_at_time(6i)"]).to eq("")
+      end
+    end
+
+    context "when hour and minute are blank and seconds is not present" do
+      let(:params) do
+        { "performed_at_time(4i)" => "", "performed_at_time(5i)" => "" }
+      end
+
+      it "does not add a seconds field" do
+        call_normalizer
+        expect(params).not_to have_key("performed_at_time(6i)")
+      end
+    end
+
+    context "when hour is present but minute is blank" do
+      let(:params) do
+        {
+          "performed_at_time(4i)" => "12",
+          "performed_at_time(5i)" => "",
+          "performed_at_time(6i)" => "0"
+        }
+      end
+
+      it "does not modify seconds" do
+        call_normalizer
+        expect(params["performed_at_time(6i)"]).to eq("0")
+      end
+    end
+
+    context "when minute is present but hour is blank" do
+      let(:params) do
+        {
+          "performed_at_time(4i)" => "",
+          "performed_at_time(5i)" => "30",
+          "performed_at_time(6i)" => "0"
+        }
+      end
+
+      it "does not modify seconds" do
+        call_normalizer
+        expect(params["performed_at_time(6i)"]).to eq("0")
+      end
+    end
+
+    context "when hour and minute are both present" do
+      let(:params) do
+        {
+          "performed_at_time(4i)" => "12",
+          "performed_at_time(5i)" => "30",
+          "performed_at_time(6i)" => "0"
+        }
+      end
+
+      it "does not modify seconds" do
+        call_normalizer
+        expect(params["performed_at_time(6i)"]).to eq("0")
+      end
+    end
+
+    context "with a different field name" do
+      let(:field_name) { :another_time }
+
+      let(:params) do
+        {
+          "another_time(4i)" => "",
+          "another_time(5i)" => "",
+          "another_time(6i)" => "0"
+        }
+      end
+
+      it "normalizes the correct field" do
+        call_normalizer
+        expect(params["another_time(6i)"]).to eq("")
+      end
+    end
+
+    context "when params contain other fields" do
+      let(:params) do
+        {
+          "performed_at_date(1i)" => "2025",
+          "performed_at_date(2i)" => "7",
+          "performed_at_date(3i)" => "31",
+          "performed_at_time(4i)" => "",
+          "performed_at_time(5i)" => "",
+          "performed_at_time(6i)" => "0",
+          "notes" => "Some notes"
+        }
+      end
+
+      it "only modifies the time seconds field" do
+        call_normalizer
+        expect(params["performed_at_date(1i)"]).to eq("2025")
+        expect(params["performed_at_date(2i)"]).to eq("7")
+        expect(params["performed_at_date(3i)"]).to eq("31")
+        expect(params["performed_at_time(6i)"]).to eq("")
+        expect(params["notes"]).to eq("Some notes")
+      end
+    end
+  end
+end