Skip to content

LG-16371 Use constants for idv document types#12398

Merged
shanechesnutt-ft merged 1 commit intomainfrom
sc/LG-16371
Aug 12, 2025
Merged

LG-16371 Use constants for idv document types#12398
shanechesnutt-ft merged 1 commit intomainfrom
sc/LG-16371

Conversation

@shanechesnutt-ft
Copy link
Contributor

🎫 Ticket

Link to the relevant ticket:
LG-16371

🛠 Summary of changes

Create constants for idv document types

📜 Testing Plan

  • Passing specs

Copy link
Contributor

Choose a reason for hiding this comment

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

should we have Idp::Constants:DocumentTypes::STATE_ID_CARD here?

Copy link
Contributor Author

@shanechesnutt-ft shanechesnutt-ft Aug 7, 2025

Choose a reason for hiding this comment

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

I wasn't sure if this should be a constant or not. Maybe it should be the value that we send when you select state_id on the choose_id_type page.

Copy link
Contributor

Choose a reason for hiding this comment

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

After a quick trace through where this method is called and ends up, we can take two paths:
The quick path would be to make a constant for state_id no card.
The other path would be to investigate all the uses of idType in the react portion of document capture. Starting at app/javascript/packages/document-capture/context/upload.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into the latter today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am actually wondering if it make more sense to use drivers_license here instead... That is the type that can be selected on the choose_id_type page. Thoughts?

Copy link
Contributor

@AShukla-GSA AShukla-GSA left a comment

Choose a reason for hiding this comment

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

Agreed with earlier thread by alex, but otherwise LGTM

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

Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

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

This is a HUGE improvement! I left a few optional suggestions - your choice.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
`choose_id_type` #{@chosen_id_type} is invalid,
`chosen_id_type` #{@chosen_id_type} is invalid,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use the constants here?

Suggested change
['passport', 'state_id_card', 'drivers_license'].each do |id_type|
[
Idp::Constants::DocumentTypes::PASSPORT,
Idp::Constants::DocumentTypes::STATE_ID_CARD,
Idp::Constants::DocumentTypes::DRIVERS_LICENSE,
].each do |id_type|

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@AShukla-GSA AShukla-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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I talked to @solipet about this change and at the end of our conversation we felt that this value should be state_id_card. So I am reverting this commit

@shanechesnutt-ft shanechesnutt-ft merged commit dbfa1a0 into main Aug 12, 2025
1 check passed
@shanechesnutt-ft shanechesnutt-ft deleted the sc/LG-16371 branch August 12, 2025 15:35
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