Skip to content

LG-13896: State ID expiration validation#10995

Merged
amirbey merged 7 commits intomainfrom
amirbey/LG-13896-doc-auth-expired
Aug 5, 2024
Merged

LG-13896: State ID expiration validation#10995
amirbey merged 7 commits intomainfrom
amirbey/LG-13896-doc-auth-expired

Conversation

@amirbey
Copy link
Contributor

@amirbey amirbey commented Jul 26, 2024

🎫 Ticket

LG-13896

🛠 Summary of changes

Check to see if the the state ID expiration date has not past when validating user data extracted from the ID.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Submit an ID or yaml file with an expiration date that has past and verify the user proceeds to the SSN page
  • Submit an ID or yaml file with an expiration date that has not past and verify the user sees the Try Again page

@amirbey amirbey changed the title Amirbey/lg 13896 doc auth expired LG-13896: State ID expiration validation Jul 26, 2024
@amirbey amirbey self-assigned this Jul 26, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

I might rename this to make it a little clearer what's being checked?

Suggested change
validate :expired?
validate :state_id_expiration?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding on to this comment:

  • The method doesn't return a boolean, so I wouldn't expect a ? suffix
  • Most of our "validate" methods are prefixed with validate_
Suggested change
validate :expired?
validate :validate_state_id_expiration

Comment on lines 11 to 15
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized this is the same regex as DateParser::AMERICAN_REGEX, maybe we could reuse that code here?

Suggested change
['dob', 'state_id_expiration'].each do |dt|
if (m = data.dig('document', dt).to_s.
match(%r{(?<month>\d{1,2})/(?<day>\d{1,2})/(?<year>\d{4})}))
data['document'][dt] =
Date.new(m[:year].to_i, m[:month].to_i, m[:day].to_i)
end
end
%w[dob state_id_expiration].each do |date_key|
if (date_s = data.dig('document', date_key'))
data['document'][date_key] = DateParser.parse_legacy(date_s)
end
rescue Date::Error
# maybe don't rescue so we get the error?
end

@amirbey amirbey requested review from jmax-gsa, solipet and theabrad July 30, 2024 18:10
@amirbey amirbey marked this pull request as ready for review July 30, 2024 18:10
Copy link
Contributor

@theabrad theabrad left a comment

Choose a reason for hiding this comment

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

LGTM. Tested locally with a yml file that had an expired ID and was sent to the "Try again" page.

@amirbey amirbey force-pushed the amirbey/LG-13896-doc-auth-expired branch from 5bd27b5 to bb3fe0a Compare August 5, 2024 19:23
Copy link
Contributor

@jmax-gsa jmax-gsa left a comment

Choose a reason for hiding this comment

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

LGTM

['dob', 'state_id_expiration'].each do |date_key|
if (date_s = data.dig('document', date_key))
data['document'][date_key] = DateParser.parse_legacy(date_s)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Hah! I just got done writing a very similar code block here independently while working on LG-13517. I think our PRs are going to mesh nicely. Excellent.

@amirbey amirbey merged commit f65b43d into main Aug 5, 2024
@amirbey amirbey deleted the amirbey/LG-13896-doc-auth-expired branch August 5, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants