Skip to content

FFS-4713 Add an environment variable to disable NSC in a given environment, forcing the Education flow to self-attestation only. - #2047

Open
krista-skylight wants to merge 8 commits into
mainfrom
kc/ffs-4713-disable-nsc-env-variable
Open

krista-skylight wants to merge 8 commits into
mainfrom
kc/ffs-4713-disable-nsc-env-variable

Conversation

@krista-skylight

@krista-skylight krista-skylight commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

FFS-4713

Changes

•Added helper method nsc_disabled? in ApplicationController and ApplicationHelper to safely parse and check the NSC_DISABLED environment variable across controllers and views.
•Updated ActivitiesHelper#education_add_path and app/views/activities/activities/index.html.erb to route the Activity Hub's education "Add" action directly to new_activities_flow_education_path when NSC_DISABLED is true.
•Updated Activities::EducationController actions (verify, create, show, sync, and error) and Activities::Education::TermCreditHoursController to guard against data-backed NSC paths and redirect to the self-attestation flow when NSC_DISABLED is enabled.
•Updated app/views/activities/education/error.html.erb to hide the data-backed retry button when nsc_disabled? is true.
•Documented NSC_DISABLED in app/.env, app/.env.local.example, and docs/local-replication.md.
•Added unit and controller tests across ActivitiesHelper, Activities::ActivitiesController, Activities::EducationController, and Activities::Education::TermCreditHoursController.

Acceptance testing

Tag product and design in Slack for acceptance: @emmy-acceptance-testers

  • No acceptance testing needed
    • This change will not affect the user experience (bugfix, dependency updates, etc.)
  • Acceptance testing prior to merge
    • This change can be verified visually via screenshots attached below or by sending a link to a local development environment to the acceptance tester
    • Acceptance testing should be done by design for visual changes, product for behavior/logic changes, or both for changes that impact both.
  • Acceptance testing in PR Environment
    • This change can be verified in a PR environment. Run this Github Action with the existing PR and most recent git sha.
  • Acceptance testing after merge
    • This change is hard to test locally, so we'll test it in the demo environment (deployed automatically after merge.)
    • Make sure to notify the team once this PR is merged so we don't inadvertently deploy the unaccepted change to production. (e.g. :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

  • USED_AI: I attest that I have read, understood, and take ownership of all AI-generated code in this PR.
  • NO_AI: I did not use AI.

Optional — for learning, not audited:

  • AI tools used: <e.g. GitHub Copilot, Claude Code — shows what's in use>
  • Prompt artifacts: <link a prompt/chat if worth keeping; otherwise skip>

Infrastructure Changes

  • Plan reviewed
  • Applied in dev before merge
  • Applied in demo after merge
  • Applied in prod after merge (note any exceptions or special coordination below)

Risk / Downtime:

Preview environment

@krista-skylight
krista-skylight marked this pull request as ready for review September 8, 2026 23:45
@krista-skylight
krista-skylight requested a review from a team as a code owner September 8, 2026 23:45
@github-actions
github-actions Bot requested a review from millerti September 8, 2026 23:45
Comment thread app/app/controllers/activities/education/term_credit_hours_controller.rb Outdated
Comment thread app/app/controllers/application_controller.rb
Comment thread app/.env
MISSION_CONTROL_PASSWORD=password

STRUCTURED_LOGGING_ENABLED=false
NSC_DISABLED=false

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.

This is good for a local default and testing, but we're going to need to set this in AWS Parameter Store and load the setting in each environment that way. Take a look at infra/app/app-config/env-config/environment-variables.tf to see how we do that with other variables like SITE_ALERT_ENABLED. Take a gander at the PR that added that for a guide on what's needed.

(And, in addition, we will need to go through and create this variable in Parameter Store in each environment, and set it to false.)

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 think I got it done. How do I go about getting credentials to create the variable in parameter store?

@emmybot-cms

emmybot-cms commented Sep 10, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

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.

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 the precomputed AI_REVIEW_DIFF_RANGE (origin/main HEAD), which correctly captured PR #2047's NSC_DISABLED feature-flag change (23 files); no base re-resolution was needed. Reproduced CI's rspec job (Ruby 3.4.10, Postgres 16 container, npm ci + rake assets:precompile, RAILS_ENV=test) and ran bundle exec rspec spec/ --exclude-pattern '**/e2e/**': 2584 examples, 0 failures. An initial run showed ~70 red controller specs all failing on Sprockets AssetNotFound for the USWDS sprite; that was a missing assets:precompile step in my own bootstrap (which CI's setup-project action performs), and every one of those specs passed once assets were compiled, so nothing is classified. The new copy-drift assertions in employment_controller_spec.rb and add_your_work_controller_spec.rb were deleted alongside the intended en.yml/view changes, so no stale change-detector remains. spec/e2e/** was excluded, matching the rspec.yml CI job.

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.

@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

📝 Terraform Plan: Database

→ Resource Changes: 0 to create, 4 to update, 0 to re-create, 0 to delete, 0 ephemeral, 0 to import.

♻️ Update

module.database.aws_iam_role_policy.role_manager_access_to_db_password
  id     = "app-dev-manager:app-dev-role-manager-ssm-access"
  name   = "app-dev-role-manager-ssm-access"
! policy = jsonencode(
      {
        - Statement = [
            - {
                - Action   = [
                    - "kms:Decrypt",
                  ]
                - Effect   = "Allow"
                - Resource = [
                    - "arn:aws:kms:us-east-1:975050060969:key/bb327782-cdb7-4d12-97f4-f6d8edf6ae4a",
                  ]
              },
            - {
                - Action   = [
                    - "secretsmanager:GetSecretValue",
                  ]
                - Effect   = "Allow"
                - Resource = [
                    - "arn:aws:secretsmanager:us-east-1:975050060969:secret:rds!cluster-12d99380-1a72-4015-9f82-bd5601a96709-glnNNQ",
                  ]
              },
            - {
                - Action   = [
                    - "ssm:GetParameter",
                  ]
                - Effect   = "Allow"
                - Resource = [
                    - "arn:aws:ssm:us-east-1:975050060969:parameter/aws/reference/secretsmanager/rds!cluster-12d99380-1a72-4015-9f82-bd5601a96709",
                  ]
              },
          ]
        - Version   = "2012-10-17"
      }
  ) -> (known after apply)
  # (1 unchanged attribute hidden)
module.database.aws_lambda_function.role_manager
  id                             = "app-dev-role-manager"
  tags                           = {}
  # (28 unchanged attributes hidden)

! environment {
!     variables = {
        - "APP_USER"               = "app"
        - "DB_HOST"                = "app-dev.cluster-c966aaoui6ez.us-east-1.rds.amazonaws.com"
        - "DB_NAME"                = "app"
        - "DB_PASSWORD_PARAM_NAME" = "/aws/reference/secretsmanager/rds!cluster-12d99380-1a72-4015-9f82-bd5601a96709"
        - "DB_PASSWORD_SECRET_ARN" = "arn:aws:secretsmanager:us-east-1:975050060969:secret:rds!cluster-12d99380-1a72-4015-9f82-bd5601a96709-glnNNQ"
        - "DB_PORT"                = "5432"
        - "DB_SCHEMA"              = "app"
        - "DB_USER"                = "postgres"
        - "MIGRATOR_USER"          = "migrator"
        - "PYTHONPATH"             = "vendor"
      } -> (known after apply)
  }

  # (3 unchanged blocks hidden)
module.database.aws_rds_cluster.db
! engine_version                      = "14.17" -> "14.20"
  id                                  = "app-dev"
  tags                                = {}
  # (42 unchanged attributes hidden)

  # (1 unchanged block hidden)
module.database.aws_rds_cluster_instance.primary
! engine_version                        = "14.20" -> "14.17"
  id                                    = "app-dev-primary"
  tags                                  = {}
  # (29 unchanged attributes hidden)

Triggered by @krista-skylight, Commit: 8008e8456d0c18103a6ca167871d9f63b19afa64

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.

test-classifier: AI triage of failing tests

AI Test Classifier — no action required

Observed — these verdicts are grounded in the actual test run output.

Bootstrapped per the repo's own CI (postgres:16 service container, rake db:schema:load, npm install + rake assets:precompile) and ran the CI test command bundle exec rspec spec/ --exclude-pattern '{e2e/**/*_spec.rb}' on Ruby 3.4.10 (repo pins 3.4.9; pin temporarily relaxed for the run and reverted). Result: 2584 examples, 0 failures — nothing to classify. The AI_REVIEW_DIFF_RANGE base (origin/main) was valid and was used as-is. A first pass without asset precompilation produced ~80 Sprockets AssetNotFound errors; those were a local bootstrap gap (ENVIRONMENT_ISSUE), disappeared after running CI's assets:precompile step, and are therefore not reported as classifications. spec/e2e/ is excluded by the CI rspec job and was not run.

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.

Comment thread app/lib/nsc.rb Outdated

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.

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 the dispatcher-provided range origin/main..HEAD as-is; it correctly captured the PR #2047 change (NSC_DISABLED kill switch for education flows, 26 files), so no base re-resolution was needed. Reproduced CI's rspec.yml path: started Postgres, installed gems, npm install, rake assets:precompile, db:schema:load, then parallel_test spec/ -n 4 --exclude-pattern '^spec/e2e/' (same e2e exclusion CI uses; e2e runs in a separate workflow requiring browsers/external aggregators). Result: 2584 examples, 0 failures across all 4 processes; the 9 changed spec files alone give 339 examples, 0 failures. rubocop (664 files) and erb_lint (124 files) are also clean. Two bootstrap caveats: the Gemfile pins Ruby 3.4.9 but only 3.4.10 is available in the runner toolchain, so tests ran under 3.4.10 via a throwaway sidecar Gemfile.ci (deleted afterward, no tracked file modified); and an initial run performed before assets:precompile produced ~120 spurious Sprockets AssetNotFound errors in every view-rendering spec, which was an artifact of my own incomplete bootstrap rather than the change under test and which disappeared entirely once assets were compiled as CI does, so those are not classified. Nothing failed, so classifications is empty.

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.

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.

3 participants