Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ def alt_rev_validate_form_526_submission_values
return if form_attributes.empty?

alt_rev_validate_claim_process_type_bdd if bdd_claim?
# ensure 'claimDate', if provided, is not in the future
Copy link
Copy Markdown
Contributor Author

@siddharthalamsal siddharthalamsal Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noticed claimDate business logic was not being checked before we used it for validations. This probably could have waited until the claimDate was actually added to the schema, so open to revert but I don't think it hurts anything.

alt_rev_validate_form_526_claim_date
# ensure mailing address country is valid
alt_rev_validate_form_526_identification
# ensure disabilities are valid
Expand All @@ -42,6 +44,15 @@ def alt_rev_validate_form_526_submission_values

private

def alt_rev_validate_form_526_claim_date
return if claim_date <= Date.current

collect_error_messages(
source: '/claimDate',
detail: 'claimDate must not be in the future.'
)
end

def alt_rev_validate_form_526_change_of_address
return if form_attributes['changeOfAddress'].blank?

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@

require 'claims_api/v2/disability_compensation_shared_service_module'
require 'claims_api/lighthouse_military_address_validator'
require 'claims_api/disability_compensation_validations_helper'

module ClaimsApi
module V2
module DisabilityCompensationValidation # rubocop:disable Metrics/ModuleLength
include DisabilityCompensationSharedServiceModule
include ClaimsApi::DisabilityCompensationValidationsHelper
Comment thread
jeremyhunton-va marked this conversation as resolved.
include LighthouseMilitaryAddressValidator

DATE_FORMATS = {
Expand All @@ -18,13 +20,12 @@ module DisabilityCompensationValidation # rubocop:disable Metrics/ModuleLength
BDD_LOWER_LIMIT = 90
BDD_UPPER_LIMIT = 180

YYYY_YYYYMM_REGEX = '^(?:19|20)[0-9][0-9]$|^(?:19|20)[0-9][0-9]-(0[1-9]|1[0-2])$'.freeze
YYYY_MM_DD_REGEX = '^(?:[0-9]{4})-(?:0[1-9]|1[0-2])-(?:0[1-9]|[1-2][0-9]|3[0-1])$'.freeze

def validate_form_526_submission_values(target_veteran)
return if form_attributes.empty?

validate_claim_process_type_bdd if bdd_claim?
# ensure 'claimDate', if provided, is not in the future
validate_form_526_claim_date
# ensure 'claimantCertification' is true
validate_form_526_claimant_certification
# ensure mailing address country is valid
Expand All @@ -51,12 +52,13 @@ def validate_form_526_submission_values(target_veteran)

private

def claim_date
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a repeat of the helper file claim_date method

@claim_date = if date_is_valid?(form_attributes['claimDate'], 'claimDate', true)
Date.parse(form_attributes['claimDate'])
else
Date.current
end
def validate_form_526_claim_date
return if claim_date <= Date.current

collect_error_messages(
source: '/claimDate',
detail: 'claimDate must not be in the future.'
)
end

def validate_form_526_change_of_address
Expand Down Expand Up @@ -1275,44 +1277,6 @@ def date_range_overlap?(range_one, range_two)
(range_one&.last&.> range_two&.first) || (range_two&.last&.< range_one&.first)
end

# Will check for a real date including leap year
def date_is_valid?(date, property, is_full_date = false) # rubocop:disable Style/OptionalBooleanParameter
return false if date.blank?

# check for something like 'July 2017'
collect_date_error(date, property) unless /^[\d-]+$/ =~ date || property == 'claimDate'

return false if is_full_date && !date.match(YYYY_MM_DD_REGEX)

return true if date.match(YYYY_YYYYMM_REGEX) # valid YYYY or YYYY-MM date

date_y, date_m, date_d = date.split('-').map(&:to_i)

return true if Date.valid_date?(date_y, date_m, date_d)

# due to claimDate being an optional field with a fallback, this is the only date
# we allow to be invalid/nil without an error
collect_date_error(date, property) unless property == 'claimDate'

false
end

def collect_date_error(date, property = '/')
collect_error_messages(
detail: "#{date} is not a valid date.",
source: "data/attributes/#{property}"
)
end

def errors_array
@errors ||= []
end

def collect_error_messages(detail: 'Missing or invalid attribute', source: '/',
title: 'Unprocessable Entity', status: '422')
errors_array.push({ detail:, source:, title:, status: })
end

def error_collection
errors_array.uniq! { |e| e[:detail] }
errors_array # set up the object to match other error returns
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1487,5 +1487,20 @@ def validate_field(field_path, expected_detail, expected_source)
expect(errors).to be_empty
end
end

context 'when claimDate is in the future' do
it 'collects an error with the correct message' do
future_date = (Date.current + 1.day).iso8601
test_526_validation_instance.form_attributes['claimDate'] = future_date
test_526_validation_instance.instance_variable_set(:@claim_date, nil)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test manually resets @claim_date with instance_variable_set(:@claim_date, nil). This is fine for testing, but note that claim_date in the shared helper uses memoization (@claim_date = ...). If claim_date was previously called in the test setup with a non-future date, and @claim_date isn't reset, the validation would pass incorrectly. The test does reset it, so it works — but this is worth noting as a pattern that couples tests to internal implementation details.


test_526_validation_instance.send(:alt_rev_validate_form_526_claim_date)
errors = current_error_array

expect(errors).not_to be_empty
expect(errors[0][:detail]).to eq('claimDate must not be in the future.')
expect(errors[0][:source]).to eq('/claimDate')
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,7 @@ def validate_field(field_path, expected_detail, expected_source)
# Ensure claimDate is not set
expect(subject.form_attributes).not_to have_key('claimDate')

expect(subject.send(:claim_date)).to eq(Time.zone.today)
expect(subject.send(:claim_date)).to eq(Date.current)

errors = test_526_validation_instance.send(:error_collection)

Expand All @@ -725,11 +725,26 @@ def validate_field(field_path, expected_detail, expected_source)
subject.form_attributes['claimDate'] = 'invalid-date'
subject.instance_variable_set(:@claim_date, nil)

expect(subject.send(:claim_date)).to eq(Time.zone.today)
expect(subject.send(:claim_date)).to eq(Date.current)
errors = test_526_validation_instance.send(:error_collection)

expect(errors).to be_empty
end
end

context 'when claimDate is in the future' do
it 'collects an error with the correct message' do
future_date = (Date.current + 1.day).iso8601
subject.form_attributes['claimDate'] = future_date
subject.instance_variable_set(:@claim_date, nil)

test_526_validation_instance.send(:validate_form_526_claim_date)
errors = test_526_validation_instance.send(:error_collection)

expect(errors).not_to be_empty
expect(errors[0][:detail]).to eq('claimDate must not be in the future.')
expect(errors[0][:source]).to eq('/claimDate')
end
end
end
end
Loading