Repository navigation
FFS-4903 Add education_type field to 3 places - #2158
krista-skylight wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Reviewed 29 files against origin/main. Found 0 findings (0 critical, 0 high, 0 medium, 0 low). Only the security perspective was applied because no IaC files changed. The new education_type attribute is only set from a server-side session value that is checked against an allowlist and tied to the current flow, and it is excluded from strong params. The model also validates it, and it is rendered through escaped I18n lookups. No secrets, PII or PHI were introduced.
Reviewed by AI, was this helpful? Please react with 👍 or 👎.
daphnegold
left a comment
There was a problem hiding this comment.
Your solution satisfies the ticket using session, but as a suggestion, for consistency, we could pass education_type through a URL param like Employment with compensation_type?
There's also an opportunity to use i18n in a lot of places where there's hardcoding in the tests.
…isplays with i18n lookups
There was a problem hiding this comment.
test-classifier: AI triage of failing tests
AI Test Classifier — triage of failing tests
Observed — these verdicts are grounded in the actual test run output.
Base origin/main..HEAD was correct (no re-resolve). Ran the full non-e2e rspec suite like CI, using ruby:3.4.9 and postgres:16 in Docker after an npm asset build: 2849 examples, 1 failure. All 291 examples in the changed non-e2e spec files passed. The one failure is a stale href assertion in other_controller_spec after the intended addition of the education_type=other param. E2E (js) specs were not run because they need a browser.
| Verdict | Test | Confidence | Scope |
|---|---|---|---|
| TEST_BUG | Activities::Education::OtherController#show renders the accordions with content and routing links |
high | change |
Per-test rationale
- TEST_BUG · behavioral-drift —
Activities::Education::OtherController#show renders the accordions with content and routing links(app/spec/controllers/activities/education/other_controller_spec.rb:33)
The PR intentionally changed the 'Report education manually' link in other/show.html.erb to new_activities_flow_education_path(education_type: "other"), but the spec still expects the bare /activities/education/new href. Update the expected href in the test; the app behavior is correct.
React 👍 if right / 👎 if wrong, and on a 👎 please reply to this comment with a one-line reason — that reply is the most useful tuning signal we get. Advisory, non-blocking.
daphnegold
left a comment
There was a problem hiding this comment.
Looking good, thanks for addressing my suggestions. Looks like you do need to make an rspec adjustment to get it green again though. It's failing because of an outdated URL expectation.
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.
Used AI_REVIEW_DIFF_RANGE origin/main..HEAD (it matches PR #2158; no re-resolution needed). Reproduced CI rspec setup (Postgres 16 + Ruby 3.4.9 Docker, assets built with Node 24). Changed specs + components: 548 examples, 0 failures; full non-E2E suite: 2849 examples, 0 failures. E2E specs (incl. changed activity_hub_education_self_attestation_spec.rb and activity_hub_spec.rb) were not run locally because they need browser tooling; confirm them in the e2e-tests workflow.
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.
FFS-4903
Changes
Created the field
education_typebased on the education type selection in the "Add your education" page.The new field were added to:
The JSON representation of education activity entries, for both the NSC-backed and self-attested field sets.
The OpenAPI documentation/schema for the CE JSON API, including the new field's enum values, description, and updated examples.
The PDF rendering of the education activity section.
CE "Review and Submit" page display of Education activities
Context for reviewers
Acceptance testing
Tag product and design in Slack for acceptance: @emmy-acceptance-testers
:alert: Deploy block! @ffs-eng I just merged PR [#123] and will be doing acceptance testing in demo - please don't deploy until I'm finished!)AI Usage
If you used AI:
Purpose for AI usage:
Which AI Tool(s) did you use:
Infrastructure Changes
Risk / Downtime:
CMS Cloud review environment