[JCC] - Add 'previousSSN' field to license upload process for correcting incorrect SSN uploads#1726
Open
landonshumway-ia wants to merge 21 commits into
Open
[JCC] - Add 'previousSSN' field to license upload process for correcting incorrect SSN uploads#1726landonshumway-ia wants to merge 21 commits into
landonshumway-ia wants to merge 21 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional ChangesSSN correction migration
Rollback month boundary fix
Home-jurisdiction event behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant StateAPI
participant IngestPreprocessor
participant DataClient
participant S3
participant Cognito
participant EmailService
StateAPI->>IngestPreprocessor: submit corrected license with previousSSN
IngestPreprocessor->>DataClient: resolve provider IDs
IngestPreprocessor->>DataClient: migrate provider records
DataClient->>S3: move provider objects
DataClient->>Cognito: delete old provider user
Cognito->>EmailService: request re-registration email
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts`:
- Line 440: Avoid logging recipient email addresses in the ssn correction
reregistration notification flow. Update the logger call in the
email-notification service method that sends this notification so it keeps only
safe metadata about the compact/practitioner and removes the
recipients/specificEmails payload. If you need traceability, log a non-PII
indicator such as recipient count or delivery type instead, and apply the same
sanitizing pattern used by other notification methods in this file.
In
`@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py`:
- Around line 3154-3175: The new-provider record creation path in
_build_new_provider_record_if_absent is race-prone because the existence check
and the later Put are not atomic. Update the write path used for the provider
summary record so the Put includes a condition like attribute_not_exists(pk),
matching the other conditional writes in this flow, and keep the existing
get_provider_top_level_record / ProviderRecordUtility.populate_provider_record
logic for the pre-check and record construction.
- Around line 2931-2932: `full_teardown` is being decided from a stale provider
snapshot, so add a commit-time concurrency guard in the teardown path. Update
`update_provider_license_and_schema_data` to revalidate the old provider’s
license set against the current database state immediately before deleting the
provider record, not just via the existing `dateOfUpdate` fence. Use the same
teardown/delete flow that currently handles `old_provider_records`,
`old_provider_license_records`, and the provider deletion block to ensure a
concurrent ingest cannot add a license and leave it orphaned.
- Around line 2887-2891: The SSN-correction migration currently infers
teardown/retry state from whether `old_provider_data` still exists, which breaks
replay after a later DynamoDB batch failure past the 100-item limit. Update the
migration flow in `data_client.py` to persist an explicit cleanup/teardown
marker (or equivalent state) when the old provider record is deleted, and have
the replay path use that marker instead of `old_provider_data is None`. Adjust
the logic around the `old_provider_registered_email` / Cognito delete /
re-registration email branch so it still runs correctly on retry even when the
old provider record has already been removed.
In `@backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py`:
- Around line 373-478: The teardown work in _perform_ssn_correction_migration is
coupled too tightly to migrate_provider_for_ssn_correction, so once
migration_performed becomes false on a retry,
_move_provider_documents_to_new_keyspace and
_delete_old_cognito_user_and_send_reregistration_email never run. Add a separate
persisted teardown state (for example in the existing migration result or a new
tracking record) so the S3 move and Cognito/email steps can resume independently
after a crash. Update the retry path in _perform_ssn_correction_migration to
check that teardown state rather than relying only on migration_performed, and
keep _move_s3_object and _delete_old_cognito_user_and_send_reregistration_email
idempotent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e728dc78-699b-468b-8b55-c9d6593ab8fb
📒 Files selected for processing (27)
backend/compact-connect/docs/README.mdbackend/compact-connect/lambdas/nodejs/email-notification-service/lambda.tsbackend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.tsbackend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.tsbackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/ingest.pybackend/compact-connect/lambdas/python/common/cc_common/email_service_client.pybackend/compact-connect/lambdas/python/common/cc_common/feature_flag_enum.pybackend/compact-connect/lambdas/python/common/cc_common/utils.pybackend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.pybackend/compact-connect/lambdas/python/common/tests/unit/test_email_service_client.pybackend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.pybackend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_bulk_upload.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_licenses.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/unit/test_handlers/test_ingest.pybackend/compact-connect/pipeline/backend_stage.pybackend/compact-connect/stacks/feature_flag_stack/__init__.pybackend/compact-connect/stacks/ingest_stack.pybackend/compact-connect/stacks/state_api_stack/v1_api/api_model.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
2890-2894: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "first transaction batch" claim contradicts the actual final-phase fence placement.
This docstring says the old-provider fence write is "executed in the first transaction batch," but the detailed phase comments a few lines below (2947-2966) and the actual code path (
create_transaction_items→delete_transaction_items→final_transaction_items) show the fence is bundled into the last phase together with the target license delete — that's precisely what makes large migrations replay-safe (both critical records survive together until the very end). Update the docstring so it doesn't contradict the phase-order explanation just below it.📝 Proposed fix
- Concurrency: the write against the old top-level provider record is conditioned on the dateOfUpdate - read at the start of the migration and executed in the first transaction batch. A concurrent migration - for the same old provider will fail that condition before writing anything, and its SQS retry re-reads - current state. The targeted license's delete is executed in the last batch so a crash mid-migration - leaves the license in place for the replay's idempotency guard to find; all other writes are idempotent. + Concurrency: the write against the old top-level provider record is conditioned on the dateOfUpdate + read at the start of the migration and is bundled, together with the targeted license's delete, into + the final transaction batch. A concurrent migration for the same old provider will fail that condition + before writing anything, and its SQS retry re-reads current state. Keeping the fence and the license + delete in the same final batch means a crash mid-migration leaves both records in place for the + replay's idempotency guard to find; all other writes are idempotent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py` around lines 2890 - 2894, Update the migration docstring around create_transaction_items, delete_transaction_items, and final_transaction_items to remove the claim that the old-provider fence runs in the first transaction batch. Describe the fence as executing in the final phase together with the targeted license delete, while preserving the replay-safety and concurrency behavior described by the surrounding phase comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py`:
- Around line 2890-2894: Update the migration docstring around
create_transaction_items, delete_transaction_items, and final_transaction_items
to remove the claim that the old-provider fence runs in the first transaction
batch. Describe the fence as executing in the final phase together with the
targeted license delete, while preserving the replay-safety and concurrency
behavior described by the surrounding phase comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29cf2775-6597-406a-9642-a4d51e4135bb
📒 Files selected for processing (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.pybackend/compact-connect/stacks/ingest_stack.py
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/compact-connect/stacks/ingest_stack.py
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py
- backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py (1)
8-22: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRead
LICENSE_SSN_CORRECTION_MIGRATION_FLAGat invocation time Module-level caching freezes the value for warm Lambda containers, so a flag flip or fail-closed startup result can stay stuck until a cold start. Since this controls record-migration deletes, fetch it inside the handler or add a short TTL cache in bothlicenses.pyandbulk_upload.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py` around lines 8 - 22, Move evaluation of LICENSE_SSN_CORRECTION_MIGRATION_FLAG out of module scope and into the relevant handler flow so each invocation observes current flag state while retaining fail_default=False. Apply the same change in backend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py at lines 8-22 and backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py at lines 29-32; no module-level cached value should gate record migration deletes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py`:
- Around line 184-189: Move the ssn_correction_migration_flag_enabled check and
validated_license.pop('previousSSN', None) logic before the schema.load(...)
call in the bulk upload validation flow. Keep the existing disabled-feature log
message and ensure previousSSN is removed from the input before schema
validation, while preserving it for enabled migrations.
In `@backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py`:
- Around line 599-603: Update the restoration flow around
_query_provider_ids_by_name and _restore_test_provider_account so it never
selects current_ids[0] based on unspecified search ordering. Track and use the
authoritative migration provider ID, or validate each candidate against DynamoDB
and choose the one matching the expected provider record, while retaining
original_provider_id only when no valid candidate exists.
- Around line 128-134: The resource setup in _create_test_app_client_headers and
the affected tests must clean up partial acquisitions when later setup fails.
Track each created app client and staff user immediately, move staff-user
creation inside the cleanup scope, and use nested try/finally or ExitStack so
failed token acquisition or subsequent setup removes every resource already
created while preserving normal test cleanup.
- Around line 555-594: The migration smoke tests only verify record movement,
not persisted SSN metadata. Update the full migration and roundtrip assertions
near _verify_all_records_migrated, plus the partial migration checks around the
corresponding test section, to inspect each target license’s ssnLastFour and
assert 8877 after full migration, 0000 after roundtrip, and 8899/8888 for the
partial target/source licenses.
- Around line 366-373: Update the Cognito/provider-record recovery logic around
the existing ClientError handler to evaluate the two states independently,
covering all four combinations of Cognito user presence and provider-record
presence. When a Cognito user already exists, read and validate its immutable
custom:providerId against provider_id before treating it as valid; reject or
recover mismatches. Restore whichever missing record or user is needed, while
preserving the no-op only when both records exist and the binding matches.
---
Outside diff comments:
In
`@backend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py`:
- Around line 8-22: Move evaluation of LICENSE_SSN_CORRECTION_MIGRATION_FLAG out
of module scope and into the relevant handler flow so each invocation observes
current flag state while retaining fail_default=False. Apply the same change in
backend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py at
lines 8-22 and
backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py
at lines 29-32; no module-level cached value should gate record migration
deletes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99eef7f7-3ec0-41fb-acc2-092d8780894c
📒 Files selected for processing (9)
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.tsbackend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_bulk_upload.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_licenses.pybackend/compact-connect/tests/smoke/README.mdbackend/compact-connect/tests/smoke/config.pybackend/compact-connect/tests/smoke/smoke_tests_env_example.jsonbackend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_licenses.py
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_bulk_upload.py
- backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There have been issues where states have entered the wrong SSN for a license record, but the last 4 of the SSN were still accurate so practitioners have been able to register under a license record and purchase privileges. We need to provide a way for states to correct the SSNs for their particular license records. This ticket involves adding a new optional field to the license upload API, 'previousSSN'. When this value is included with a properly formatted SSN, then the system will look up any existing records for the provider id associated with that SSN. If a matching license record is found, it will move that license record along with any associated privileges and other records over to the new provider id, and delete the old records under the previous ssn to remove the duplicate records.
Requirements List
Testing List
yarn test:unit:allshould run without errors or warningsyarn serveshould run without errors or warningsyarn buildshould run without errors or warningsbackend/compact-connect/tests/unit/test_api.pyrun compact-connect/bin/download_oas30.pyCloses #1725
Summary by CodeRabbit
New Features
previousSSNsupport to enable SSN corrections that move an existing license and related records to the corrected provider.Bug Fixes
Documentation
previousSSN, and expanded smoke-test setup notes.Tests