LG-16017 Allow approving pending IPP enrollments in int by monfresh · Pull Request #12014 · 18F/identity-idp

monfresh · 2025-03-21T20:52:12Z

🎫 Ticket

Link to the relevant ticket:
LG-16017

Background

Currently, when testing the IPP flow in the sandbox (int), one must wait 1 to 2
hours before the verification is complete. The sandbox is the environment
that all our partners use to test their apps, and that the Partnerships team
uses to test, and for live demos for partners. Having to wait 1 to 2 hours is a huge blocker.

A few months ago, a feature was built that allows approving pending IPP
enrollments immediately via the /test/ipp route. This feature was originally
built for local development only. This PR seeks to make this feature available
in our int host as well.

🛠 Summary of changes

This PR adds a new config setting (IdentityConfig) that gives access to the /test/ipp route. It also includes some improvements to the existing feature, such as:

Add controller and feature tests
Move the logic that determines whether or not this feature is available to FeatureManagement
Make sure the user is fully authenticated
In deployed hosts, only display pending enrollments that belong to the current user
Make queries more robust and performant with find_by and avoiding N+1 queries
In the view, use size instead of count, since count can lead to some issues in some cases (although unlikely in this situation).

📜 Testing Plan

On a host where the feature is enabled, sign in with an unverified account via a sample application that requires IdV (such as https://int-identity-oidc-sinatra.app.cloud.gov/)
Go through IPP until you get the barcode
Visit the /test/ipp path

You should see your pending IPP enrollment, and an "Approve" button next to it.

Click the button.

You should receive an email letting you know you've been verified

Sign out and sign back in with a different unverified account and go through IPP
Once you get the barcode, sign out and sign back in with the first account you used in step 1, not the one you just used in step 5
Visit the /test/ipp path

You should not see the other user's pending enrollment

To test locally, add in_person_enrollments_immediate_approval_enabled: true as an entry under the development section of application.yml

When testing in local development, you should be able to see pending enrollments for all users on the same page.

Currently, when testing the IPP flow in the sandbox (int), one must wait 1 to 2 hours before the verification is complete. The sandbox is the environment that all our partners use to test their apps, and that the Partnerships team uses to test, and for live demos for partners. Having to wait 1 to 2 hours is a huge blocker. A few months ago, a feature was built that allows approving pending IPP enrollments immediately via the /test/ipp route. This feature was originally built for local development only. This PR seeks to make this feature available in our `int` host as well. In the process, I made some improvements to the existing feature, such as: - Add controller and feature tests - Move the logic that determines whether or not this feature is available to `FeatureManagement` - Make sure the user is fully authenticated - In `int`, only display pending enrollments that belong to the current user - Make queries more robust and performant with `find_by` and avoiding N+1 queries - In the view, use `size` instead of `count`, since `count` can lead to some issues in some cases (although unlikely in this situation). See this true story: https://www.moncefbelyamani.com/the-6-characters-that-could-bring-down-your-rails-app/

mitchellhenke · 2025-03-21T20:55:36Z

lib/feature_management.rb

+  # as opposed to having to wait more than 1 hour, which is not acceptable for
+  # testing purposes. See test/ipp_controller.rb
+  def self.allow_ipp_enrollment_approval?
+    Identity::Hostdata.env == 'int' || Rails.env.development?


I think this should be a discrete config element so that it could be enabled in dev, personal environments, etc.

Sure. That makes sense. Are you referring to adding a new item to IdentityConfig and application.yml?

I moved this logic to point to a new IdentityConfig option. PTAL.

This makes it more flexible to enable it on different hosts, without having to make code changes in FeatureManagement.

mitchellhenke · 2025-03-27T21:25:13Z

app/controllers/test/ipp_controller.rb

    def update
      enrollment_id = params['enrollment'].to_i
-      enrollment = InPersonEnrollment.find(enrollment_id)
+      enrollment = InPersonEnrollment.find_by(id: enrollment_id)


Should this include the user_id filter so that I can't approve someone else's enrollment?

Yes! Great catch. I will fix that.

I made that change. PTAL when you get a chance. Thanks!

eileen-nava

I left some comments. Thanks for beefing up the test coverage.

eileen-nava · 2025-03-28T15:00:26Z

lib/feature_management.rb

      outage_status.phone_finder_outage?
  end
+
+  # int is our sandbox environment that all partners use to test their apps,


I recommend editing this comment for brevity.

Good call. I shortened it. How does it look now?

eileen-nava · 2025-03-28T15:07:45Z

spec/controllers/test/ipp_controller_spec.rb

@@ -0,0 +1,124 @@
+require 'rails_helper'


I recommend moving setup, such as stubbing, allow statements, and factory bot usage, out of it blocks.

eileen-nava · 2025-03-28T15:13:57Z

spec/features/test/approving_pending_ipp_enrollments_spec.rb

+require 'axe-rspec'
+
+RSpec.describe 'Approving Pending IPP Enrollments' do
+  context 'when Rails env is not development and allow_ipp_enrollment_approval? is true' do


Nit: What are your thoughts about splitting this up into two context blocks, rather than one?

I'm not sure I understand. There are already 2 separate context blocks.

I was curious what you thought of using nesting.

context 'when Rails env is not development' do context ‘allow_ipp_enrollment_approval? is true’ do

That's an interesting idea. I don't think I've ever done that before. What would be the benefit?

In the order you suggested, I don't think it would make sense, but I can see how there might be a top level context ‘allow_ipp_enrollment_approval? is true’ block since that is common to both scenarios, and then the sub contexts would be when Rails env is development, and when it's not. But even that seems strange and harder to understand to me.

To me, a context represents a single scenario. In this case, the context is the scenario where 2 conditions must be true at the same time. Maybe my brain is different, but to me, it is easier to read and understand when all the conditions are in the same sentence rather than having to see 2 contexts that are nested and read each one separately.

Since the feature is enabled in both scenarios, I think one way to improve this is to put that condition in the description. That way, there are only 2 contexts: in local development, and not. So, something like this:

RSpec.describe 'Approving Pending IPP Enrollments when the feature is enabled' do context 'when Rails env is not development' do it 'only shows pending enrollments for the current user', allow_browser_log: true do ... end end context 'when Rails env is development' do it 'shows all pending enrollments', allow_browser_log: true do ... end end end

Alternatively, and maybe even better, since we are not testing when the feature is disabled in a feature spec since it's already tested in the controller spec, we can simply get rid of that condition in the descriptions. When someone sees allow(FeatureManagement).to receive(:allow_ipp_enrollment_approval?).and_return(true) it's clear that the feature is enabled in the test. I think that's what I'll do. Thanks for letting me think out loud.

eileen-nava · 2025-03-28T15:14:11Z

spec/features/test/approving_pending_ipp_enrollments_spec.rb

+    end
+  end
+
+  context 'when Rails env is development and allow_ipp_enrollment_approval? is true' do


Nit: What are your thoughts about splitting this up into two context blocks, rather than one?

eileen-nava · 2025-03-28T15:15:03Z

spec/features/test/approving_pending_ipp_enrollments_spec.rb

@@ -0,0 +1,42 @@
+require 'rails_helper'


I recommend moving setup, such as allow statements and factory bot usage, out of it blocks.

If you mean putting them in a before block, I disagree with that. I follow the school of thought that everything related to a test should be all within the test. This makes it easier to see exactly what is going on in each test. This is known as the Four-Phase test: setup, exercise, verify, teardown. Although teardown is handled automatically with RSpec.

Here are relevant resources that I agree with:

https://thoughtbot.com/blog/a-journey-towards-better-testing-practices
https://thoughtbot.com/blog/four-phase-test

I realize that the testing practices I advocate for represent one side of the story, so I'd love to learn more about your recommendation to move the setup outside of the it block. Is that a standard practice for this repo that's documented somewhere, or is it more of a personal preference, or perhaps the way you've always done it or seen it done?

Either way, I'd be curious to understand the reasoning behind moving stuff outside the it block, and how that makes the test easier to read and understand, and to maintain.

And I'd be curious what you think of the articles above.

eileen-nava · 2025-03-28T15:16:09Z

spec/lib/feature_management_spec.rb

@@ -537,4 +537,26 @@
      end


I recommend moving setup out of it blocks.

I had originally submitted PR #12014 to allow immediate approval of IPP enrollments in `int`, to make it easier for partners to test IPP in the sandbox environment. At the time, I thought this was the only way to do it, after seeing PR #11653 that introduced the ability to manually approve IPP enrollments in local development only. However, it turns out the route that displays the IPP enrollments is behind another config setting that requires enabling test routes. We didn't think it was a good idea to enable test routes in `int`, and we then realized that solving our problem was as easy as changing these two settings that were causing the 1.5 hour delay between completing IPP and receiving the confirmation email: ``` get_usps_proofing_results_job_cron: '0/1 * * * *' in_person_results_delay_in_hours: 0 ``` whereas the default values in our lower environments were: ``` get_usps_proofing_results_job_cron: '0/30 * * * *' in_person_results_delay_in_hours: 1 ``` which resulted in having to wait 1.5 hours to complete IPP testing.

* Remove unused config setting for approving IPP enrollments I had originally submitted PR #12014 to allow immediate approval of IPP enrollments in `int`, to make it easier for partners to test IPP in the sandbox environment. At the time, I thought this was the only way to do it, after seeing PR #11653 that introduced the ability to manually approve IPP enrollments in local development only. However, it turns out the route that displays the IPP enrollments is behind another config setting that requires enabling test routes. We didn't think it was a good idea to enable test routes in `int`, and we then realized that solving our problem was as easy as changing these two settings that were causing the 1.5 hour delay between completing IPP and receiving the confirmation email: ``` get_usps_proofing_results_job_cron: '0/1 * * * *' in_person_results_delay_in_hours: 0 ``` whereas the default values in our lower environments were: ``` get_usps_proofing_results_job_cron: '0/30 * * * *' in_person_results_delay_in_hours: 1 ``` which resulted in having to wait 1.5 hours to complete IPP testing. changelog: Internal, IPP, Remove unused config setting

* Remove unused config setting for approving IPP enrollments (#12477) * Remove unused config setting for approving IPP enrollments I had originally submitted PR #12014 to allow immediate approval of IPP enrollments in `int`, to make it easier for partners to test IPP in the sandbox environment. At the time, I thought this was the only way to do it, after seeing PR #11653 that introduced the ability to manually approve IPP enrollments in local development only. However, it turns out the route that displays the IPP enrollments is behind another config setting that requires enabling test routes. We didn't think it was a good idea to enable test routes in `int`, and we then realized that solving our problem was as easy as changing these two settings that were causing the 1.5 hour delay between completing IPP and receiving the confirmation email: ``` get_usps_proofing_results_job_cron: '0/1 * * * *' in_person_results_delay_in_hours: 0 ``` whereas the default values in our lower environments were: ``` get_usps_proofing_results_job_cron: '0/30 * * * *' in_person_results_delay_in_hours: 1 ``` which resulted in having to wait 1.5 hours to complete IPP testing. changelog: Internal, IPP, Remove unused config setting * Team Data 1105(feature) Add Fraud Ops Tracking for data warehouse and FCMS (#12503) (feature) Add Fraud Ops Tracking for data warehouse and FCMS * changelog: Internal, Reporting, Tracking for Fraud Ops (Team Data-1105) * Handle SAML request errors from saml_idp gem (#12520) Before, the saml_idp gem would return a 403 response if it found specific errors with the SAML request, such as a missing issuer, or a missing AuthnRequest. The problem is that this did not provide any helpful information to partners while testing their integration. To improve this, we updated the `saml_idp` gem to populate the `saml_request` object with errors and not do anything else. This allows the IdP to consume those errors and handle them appropriately, by rendering them in the existing `/saml/auth/error` template. This allows partners to quickly understand what part of their SAML Request is invalid, and makes it easier for them to fix the problem. If they don't know how to fix it themselves, they can let us know the error message they see, which makes it easier and faster for us to help our partners. changelog: User-Facing Improvements, SAML Request Handling, Display SAML request errors so partners can understand what they did wrong * Update cron schedules for reports to improve timing and avoid overlaps (#12531) * changelog: internal, reporting, issue-#1113 stagger jobs Update cron schedules for reports to improve timing and avoid overlaps * Prevent users from skipping choose_id_type (#12502) * Prevent users from skipping ID type selection when accessing document capture directly changelog: Bug Fixes, Identity Verification, Prevent users from skipping ID type selection when accessing document capture directly * Fix failing specs after choose_id_type enforcement * Use session state for choose_id_type enforcement * Prevent hybrid mobile users from skipping choose_id_type step * Update hybrid mobile flow test * Refactor ID type enforcement based on PR feedback * move logic from step_info preconditions to a dedicated before_action * fix failing test * patch failing test * fix issue in mobile flow * prevent users from skipping choose ID type step * Simplify document_capture preconditions and update test helper * address pr feedback * Rewrite the IRS Monthly Credential Metrics Report (#12527) * changelog: Bug Fixes, Reporting, Simplifies execution and logic of the monthly credential report by using a single-partner/single-issuer invocation of the CombinedInvoiceSupplementReportV2 Report Source Changes * Add partner strings config * Refactor data fetching and generation * Add MAU and other fields Email Report Changes * Update corresponding mailer preview invocation * Remove reference to key metrics report in email text * Transpose Data table Spec Updates * Add csv file as a fixture for testing row and column logic * Remove bucket testing since we are relying on default one from BaseReport See https://gitlab.login.gov/lg-teams/Team-Data/reporting/-/issues/222 * GL_Data_reporting: remove cred tenure from Fraud metric (#12534) * changelog: Internal, Reporting, remove credential tenure metric from fraud * changelog: Internal, Reporting, fix lint issue * Lg-16684 HowToverify mobile is always required (#12536) * changelog: Internal, Doc Auth, removing mobile required and updating logic to always have mobile required on how to verify presenter * Updating context title and resolving PR comments * Update pinpoint supported countries (#12539) * Update to reflect drift in footnote numbering changelog: Internal, Pinpoint, Update script to reflect drift in footnote numbering Looks like https://docs.aws.amazon.com/sms-voice/latest/userguide/phone-numbers-sms-by-country.html#sms-support-note-9 became https://docs.aws.amazon.com/sms-voice/latest/userguide/phone-numbers-sms-by-country.html#sms-support-note-8. * Remove country that ceases to exist See https://en.wikipedia.org/wiki/Netherlands_Antilles * Fix flaky feature test (#12526) changelog: Internal, Testing, Fix flaky feature test * LG-16724 Add methods to pii passport and state_id structs (#12533) Added methods to pii passport and state_id structs for determining if residential address is needed and constructing pii address from internal object data. changelog: Internal, Remote IdV, Add methods to pii passport and state_id structs * Enable freezing string literals when running tests in CI (#12541) changelog: Internal, Continuous Integration, Enable freezing string literals when running tests in CI * Bump libphonenumber-js from 1.12.22 to 1.12.23 (#12538) Bumps [libphonenumber-js](https://gitlab.com/catamphetamine/libphonenumber-js) from 1.12.22 to 1.12.23. - [Changelog](https://gitlab.com/catamphetamine/libphonenumber-js/blob/master/CHANGELOG.md) - [Commits](https://gitlab.com/catamphetamine/libphonenumber-js/compare/v1.12.22...v1.12.23) --- updated-dependencies: - dependency-name: libphonenumber-js dependency-version: 1.12.23 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * One Account Feature Tests (#12514) * feature tests * sign in spec * changelog: Internal, One Account, Basic Feature tests for flow * spec * update sign in spec * make sure setting is at 100 * make sure to reload ab test --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Moncef Belyamani <moncef.belyamani@gsa.gov> Co-authored-by: Aaron <aaron.nagucki@gmail.com> Co-authored-by: Sree Appachi <guruparan18@users.noreply.github.com> Co-authored-by: Malik Warren <33402370+Mawar2@users.noreply.github.com> Co-authored-by: Gerardo E. Cruz-Ortiz <59618057+astrogeco@users.noreply.github.com> Co-authored-by: shilen <shilen.patel@gsa.gov> Co-authored-by: A Shukla <abir.shukla@gsa.gov> Co-authored-by: Vraj Mohan <vraj.mohan@gsa.gov> Co-authored-by: Shane Chesnutt <shane.chesnutt@gsa.gov> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Malick Diarra <malick.diarra@gsa.gov>

monfresh requested review from gina-yamada, jmhooper and shanechesnutt-ft March 21, 2025 20:52

mitchellhenke reviewed Mar 21, 2025

View reviewed changes

Allow enabling the feature via IdentityConfig

fdbe78c

This makes it more flexible to enable it on different hosts, without having to make code changes in FeatureManagement.

monfresh requested a review from mitchellhenke March 26, 2025 14:37

Fix spec and Rubocop offenses

365765e

mitchellhenke approved these changes Mar 27, 2025

View reviewed changes

mitchellhenke suggested changes Mar 27, 2025

View reviewed changes

monfresh added 2 commits March 27, 2025 19:46

fix feature management specs

71d52b3

Only allow current user to update own enrollment

d1d4f89

eileen-nava self-requested a review March 28, 2025 13:51

eileen-nava reviewed Mar 28, 2025

View reviewed changes

monfresh added 2 commits March 28, 2025 17:08

Simplify feature spec

824b16f

shorten comment

ec83ddd

monfresh requested review from eileen-nava and mitchellhenke March 28, 2025 21:32

mitchellhenke approved these changes Mar 31, 2025

View reviewed changes

monfresh changed the title ~~Allow approving pending IPP enrollments in int~~ LG-16017 Allow approving pending IPP enrollments in int Mar 31, 2025

monfresh merged commit 1641dca into main Mar 31, 2025
1 check passed

monfresh deleted the mb-ipp-verify-button branch March 31, 2025 20:38

amirbey mentioned this pull request Apr 3, 2025

Deploy RC 466 to Production #12049

Merged

monfresh mentioned this pull request Sep 4, 2025

Remove unused config setting for approving IPP enrollments #12477

Merged

Conversation

monfresh commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

Background

🛠 Summary of changes

📜 Testing Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eileen-nava left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

monfresh commented Mar 21, 2025 •

edited

Loading