FFS-4592: Fix NSC test scenarios in launcher - #2012
bencalegari wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Updates NSC launcher/test personas and fixtures to reflect current NSC sandbox responses (missing nameOnSchoolRecord, changed statuses), while hardening enrollment parsing and refining forward-dating so scenarios remain stable and debuggable.
Changes:
- Re-recorded/updated NSC sandbox fixtures and added a
FIXTURE_PERSONAStable + bulk fixture regeneration helper. - Updated launcher scenarios (Dominique now full-time; added Scott Tobin for “not currently enrolled”) and adjusted specs/views accordingly.
- Guarded
NscDataFetcherServiceagainst missingnameOnSchoolRecord, refined forward-dating anchor term selection, and narrowed job retry behavior for programming/validation errors.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/spec/support/nsc_api_helper.rb | Adds FIXTURE_PERSONAS and bulk fixture regen helper for NSC sandbox fixtures. |
| app/spec/support/fixtures/nsc/scott_tobin/insight.json | Updates fixture response structure/fields for Scott (CN scenario). |
| app/spec/support/fixtures/nsc/rick_banas/insight.json | Updates Rick fixture, reflecting sandbox schema changes (e.g., schoolType). |
| app/spec/support/fixtures/nsc/lynette/insight.json | Updates Lynette fixture and term ordering/data for forward-dating behavior. |
| app/spec/support/fixtures/nsc/linda/insight.json | Updates Linda fixture formatting and transaction metadata. |
| app/spec/support/fixtures/nsc/lakhshmi_saxena/insight.json | Removes unused fixture. |
| app/spec/support/fixtures/nsc/dominique_ricardo/insight.json | Updates Dominique fixture to match new CC status and schema. |
| app/spec/services/nsc_data_fetcher_service_spec.rb | Adds coverage for missing/present nameOnSchoolRecord and swaps CN scenario to Scott. |
| app/spec/services/launcher/nsc_forward_dating_service_spec.rb | Adds coverage for new anchoring behavior and new Scott scenario behavior. |
| app/spec/jobs/nsc_synchronization_job_spec.rb | Ensures programming errors fail fast and don’t enqueue retries. |
| app/spec/jobs/application_job_spec.rb | Adds explicit non-retryable error behavior tests. |
| app/spec/factories/identity.rb | Adds :nsc_scott identity trait for the new persona. |
| app/spec/controllers/launcher_controller_spec.rb | Updates simple launcher default scenario and verifies preselection behavior. |
| app/spec/controllers/launcher_controller_advanced_spec.rb | Updates advanced launcher persona list and applicant creation checks (adds Scott). |
| app/app/views/launcher/launcher.html.erb | Updates simple launcher default selection and adds “intensity not reported” option. |
| app/app/views/launcher/advanced.html.erb | Updates advanced launcher persona tiles (Dominique full-time; adds Scott). |
| app/app/services/nsc_data_fetcher_service.rb | Falls back to identity name when nameOnSchoolRecord is missing. |
| app/app/services/launcher/nsc_forward_dating_service.rb | Anchors forward-dating on latest term end <= as-of date; adds stable as-of for Dominique. |
| app/app/jobs/application_job.rb | Introduces NON_RETRYABLE_ERRORS and limits retries for programming/validation errors. |
| app/app/controllers/launcher_controller.rb | Adds Scott to launcher persona map. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
c1e1a79 to
962cd6a
Compare
There was a problem hiding this comment.
test-classifier: AI triage of failing tests
AI Test Classifier — no action required
Observed — these verdicts are grounded in the actual test run output.
Ran the repo's own CI test path (rspec.yml + setup-project action): started Postgres 16, installed gems with bundler, loaded the test schema, ran npm install and rake assets:precompile, then executed rspec. The precomputed AI_REVIEW_DIFF_RANGE (origin/main HEAD) was verified to genuinely reflect PR #2012's change, so no base re-resolution was needed. Targeted run of the 10 changed spec files: 368 examples, 0 failures. Full CI-scope run (spec/ excluding e2e, as rspec.yml does): 2564 examples, 0 failures. An earlier run showed 101 view-rendering controller failures, but all were 'The asset @uswds/uswds/dist/img/sprite.svg is not present in the asset pipeline' caused by my not yet having run the CI bootstrap's npm install + assets:precompile; after completing that bootstrap every one of them passed, so they are a bootstrap artifact and not classified as failures. Nothing to triage.
React 👍 if this is right (nothing needed triage) / 👎 if a real failure was missed, and on a 👎 please reply with a one-line reason. Advisory, non-blocking.
|
LGTM! 🚢-it |
| around_perform :with_error_reporting | ||
|
|
||
| retry_on Exception, wait: :polynomially_longer, attempts: 5 | ||
| retry_on(*NON_RETRYABLE_ERRORS, attempts: 1) |
There was a problem hiding this comment.
Perhaps we should just retry_on StandardError instead of Exception. All of the NON_RETRYABLE_ERRORS you list are not StandardErrors, and there are other ones that are not StandardErrors too that we probably also don't want to retry on, like SyntaxError, NotImplementedError, and LoadError
| last_name: enrollment_detail["nameOnSchoolRecord"]["lastName"], | ||
| first_name: name_on_school_record["firstName"] || identity&.first_name, | ||
| middle_name: name_on_school_record["middleName"], | ||
| last_name: name_on_school_record["lastName"] || identity&.last_name, |
There was a problem hiding this comment.
Hmm. I'm not sure we should fall back to saving the identity data again in the nsc_enrollment_terms - since nsc_enrollment_terms is just capturing the NSC result, supplementing it with other data might confuse us as to whether we got the data from NSC or somewhere else.
Can we just leave these fields blank if NSC returns nothing, and then handle it in whatever is reading the NSC enrollment term?
Also, did you say NSC no longer returns this field, or only sometimes? Or do we think it will be returned in production but not in sandbox?
FFS-4592
Rick and other NSC results were failing because
nameOnSchoolRecordhad gone missing from the api response. We were gettingNoMethodErrors, but the retry logic was so broad we didn't see the failure show up locally for many many minutes later. I updated the fixtures and guarded against it, defaulting to the identity's first and last name. Personas also changed status in the sandbox: Dominique now returnsCC(currently enrolled) instead ofCN, so she took over the full-time scenario and Scott Tobin was added to cover the not-currently-enrolled case.Changes
nsc_save_fixture_for_userhelper with aFIXTURE_PERSONAStable plusnsc_save_all_fixturesso fixtures can be regenerated in one command.Launcher::NscForwardDatingServiceanchors forward-dating on the latest term ending on or before the persona's launcher as-of date, rather than the latest term overall, and adds a stable as-of date for Dominique.ApplicationJobno longer retries programming errors (NameError,TypeError,ArgumentError) orActiveRecord::RecordInvalid, they fail on the first attempt.lakhshmi_saxenafixture.Context for reviewers
nameOnSchoolRecordandstudentAddressand now returnsschoolType, so the re-recorded fixtures dropped those fields.schoolTypeis a string of a number referring to whether its a 4 year, 2 year, or 1 (less than 2 year) institution which isn't relevant for our purposes.enrollment_less_than_half_time?only returns true when the status is literallyless_than_half_timewhere we actually want it to be true when itsless_than_half_time,enrolled, orunknown, which is the logic we used everywhere except there.Acceptance testing
Tag product and design in Slack for acceptance: @emmy-acceptance-testers
AI Usage
CMS Cloud review environment