Skip to content

LG-15988: Add second DOS health check after post office selection#12112

Closed
eileen-nava wants to merge 3 commits intomainfrom
em/15988-dos-second-health-check
Closed

LG-15988: Add second DOS health check after post office selection#12112
eileen-nava wants to merge 3 commits intomainfrom
em/15988-dos-second-health-check

Conversation

@eileen-nava
Copy link
Copy Markdown
Contributor

🎫 Ticket

LG-15988: Add DoS health check (2nd check) after post office selection

🛠 Summary of changes

  • After the user selects a post office location.....
  • there is a second Department of State health check
  • if the health check passes, the user gets directed to the "choose your ID type" page
  • if the health check fails, then the user gets directed to the "state ID" page

📜 Testing Plan

(Shout out to @shanechesnutt-ft, I based a lot of this on your very thorough test plan for PR #12084.)

  • Please test both hybrid and standard flow.

Scenario: Global passports and In-Person Passports are enabled
The second DOS health check succeeds.
Setup below:

  in_person_proofing_enabled: true
  in_person_proofing_opt_in_enabled: true
  in_person_passports_enabled: true
  doc_auth_passports_enabled: true
  doc_auth_passports_percent: 100
  • Login through the oidc sinatra application and select the Identity Verified level of service.
  • Create a new account
  • Begin the ID-IPP flow and navigate to the post office selection page
  • Select a post office
  • Ensure user is navigated to the choose ID type page

Scenario: Global passports are enabled and in-person passports is disabled
There is no second DOS health check.
Setup below:

  in_person_proofing_enabled: true
  in_person_proofing_opt_in_enabled: true
  in_person_passports_enabled: false
  doc_auth_passports_enabled: true
  doc_auth_passports_percent: 100
  • Login through the oidc sinatra application and select the Identity Verified level of service.
  • Create a new account
  • Begin the ID-IPP flow and navigate to the post office selection page
  • Select a post office
  • Ensure user is navigated to the state ID page

@@ -0,0 +1,23 @@
{
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.

I'd appreciate feedback on whether this mock failure file looks reasonable.

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.

I'd appreciate feedback on whether this mock failure file looks reasonable.

overall LGTM 👍🏿 ... I have noticed that the case varies in the downstream systems .. it seems insignificant but it'd be best to mimic the response body as close as possible... here is a good example of that 👀 https://github.com/18F/identity-idp/blob/a602aff859922f350bfb9949b40f714cc50b7d52/spec/fixtures/dos/healthcheck/composite_health_success.json

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.

Thanks @amirbey, I'll change the case for the downstream systems' status. Any thoughts on what the comments field should be for the downstream systems?

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.

comments fields should be fine as is 👍🏿

@eileen-nava eileen-nava requested review from a team and gina-yamada April 24, 2025 20:23
@eileen-nava eileen-nava changed the title LG-15988: Add second DOS health check after post office selection (not ready for review) LG-15988: Add second DOS health check after post office selection Apr 24, 2025
@eileen-nava eileen-nava changed the title (not ready for review) LG-15988: Add second DOS health check after post office selection LG-15988: Add second DOS health check after post office selection Apr 25, 2025

def index
if idv_session.in_person_passports_allowed?
if idv_session.in_person_passports_allowed? && dos_passport_api_healthy?(analytics:)
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.

Suggested change
if idv_session.in_person_passports_allowed? && dos_passport_api_healthy?(analytics:)
if IdentityConfig.store.in_person_passports_enabled && document_capture_session.passport_allowed?

we expect the the passport api downtime to be an edge case ... the choose your ID type screen performs an api healthcheck and disables the "passport" radio button if the api is down

if helpful, 👀 #12073

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.

Thanks for taking a look at this. I am fine with changing the code. I am unsure about this suggestion, because the suggested change doesn't meet the AC for LG-15988. I am not sure how to proceed. Do we need to rewrite LG-15988?
Let me know if this would be easier to discuss in a meeting or via slack.

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.

Amir, Gina, Shane, and I talked about this as a group. I'll make a helper method, in_person_passports_allowed?, which I will place in idv_step_concern. It also sounds like we'll be scrapping this health check, but Amir will double-check with product just in case.

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.

confirmed with @tahineemay ...

no need to add an add'l healthcheck here. if IdentityConfig.store.in_person_passports_enabled && document_capture_session.passport_allowed?, then we should show the choose your id screen which already performs health check and disables the passport radio button if API is down 👍🏿

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.

Got it, thank you @amirbey and @tahineemay!

@eileen-nava
Copy link
Copy Markdown
Contributor Author

Since requirements have changed for the ticket, I'm going to close this PR. I will open a new PR when it's ready for code review.

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.

2 participants