[JCC] - Add 'previousSSN' field to license upload process for correcting incorrect SSN uploads#1726
Open
landonshumway-ia wants to merge 13 commits into
Open
[JCC] - Add 'previousSSN' field to license upload process for correcting incorrect SSN uploads#1726landonshumway-ia wants to merge 13 commits into
landonshumway-ia wants to merge 13 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughChangesSSN Correction Migration
Email Logging Protection
Rollback Month Boundary Fix
Home-Jurisdiction Event Behavior
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant StateAPI
participant preprocess_license_ingest
participant DataClient
participant S3
participant Cognito
participant EmailService
StateAPI->>preprocess_license_ingest: submit previousSSN
preprocess_license_ingest->>DataClient: resolve previousProviderId
preprocess_license_ingest->>DataClient: migrate provider records
DataClient->>S3: move provider documents
DataClient->>Cognito: delete old user
DataClient->>EmailService: request re-registration notification
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
Comment on lines
+3154
to
+3175
| def _build_new_provider_record_if_absent( | ||
| self, | ||
| *, | ||
| compact: str, | ||
| new_provider_id: str, | ||
| rekeyed_target_license: LicenseData, | ||
| rekeyed_privileges: list[PrivilegeData], | ||
| ) -> ProviderData | None: | ||
| """ | ||
| Build a top-level provider record for the new provider from the migrated license/privileges, or return | ||
| None if the new provider already has one (a pre-existing record is never modified). | ||
| """ | ||
| try: | ||
| self.get_provider_top_level_record(compact=compact, provider_id=new_provider_id) | ||
| return None | ||
| except CCNotFoundException: | ||
| return ProviderRecordUtility.populate_provider_record( | ||
| current_provider_record=None, | ||
| license_record=rekeyed_target_license.to_dict(), | ||
| privilege_records=[privilege_data.to_dict() for privilege_data in rekeyed_privileges], | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Consider guarding the new-provider-record put with attribute_not_exists(pk).
The absence check (get_provider_top_level_record) and the later Put aren't atomic. Two concurrent migrations both targeting the same new provider id could both observe "absent" and race to write, with the second silently overwriting the first's provider summary. Adding ConditionExpression: 'attribute_not_exists(pk)' to the put (mirroring the conditioned writes used elsewhere in this method) would close this gap cheaply.
🤖 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 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.
Comment on lines
+373
to
+478
|
|
||
|
|
||
| def _perform_ssn_correction_migration( | ||
| *, | ||
| compact: str, | ||
| previous_provider_id: str, | ||
| new_provider_id: str, | ||
| jurisdiction: str, | ||
| license_type: str, | ||
| new_ssn_last_four: str, | ||
| ): | ||
| """ | ||
| Orchestrate the migration of a practitioner's records after a state corrected the SSN on a license upload. | ||
|
|
||
| The DynamoDB migration runs first; on a full teardown the S3 document move, Cognito user deletion, and | ||
| re-registration email follow, each idempotent so an SQS retry of a partially-completed migration converges. | ||
| A concurrency conflict inside the migration raises, letting SQS redeliver the message after the visibility | ||
| timeout. | ||
| """ | ||
| logger.info('Performing SSN correction migration', previous_provider_id=previous_provider_id) | ||
|
|
||
| result = config.data_client.migrate_provider_for_ssn_correction( | ||
| compact=compact, | ||
| previous_provider_id=previous_provider_id, | ||
| new_provider_id=new_provider_id, | ||
| jurisdiction=jurisdiction, | ||
| license_type=license_type, | ||
| new_ssn_last_four=new_ssn_last_four, | ||
| ) | ||
| if not result.migration_performed: | ||
| logger.info('No records to migrate for previous provider id; proceeding with normal ingest') | ||
| return | ||
|
|
||
| if result.full_teardown: | ||
| _move_provider_documents_to_new_keyspace( | ||
| compact=compact, | ||
| previous_provider_id=previous_provider_id, | ||
| new_provider_id=new_provider_id, | ||
| ) | ||
| if result.old_provider_registered_email is not None: | ||
| _delete_old_cognito_user_and_send_reregistration_email( | ||
| compact=compact, | ||
| old_registered_email=result.old_provider_registered_email, | ||
| ) | ||
|
|
||
|
|
||
| def _move_provider_documents_to_new_keyspace(*, compact: str, previous_provider_id: str, new_provider_id: str): | ||
| """Move every object under the old provider id's S3 keyspace to the new provider id's. | ||
|
|
||
| Rather than relying on any single record type's tracked document keys, this lists everything under the | ||
| old provider's keyspace prefix (`compact/{compact}/provider/{provider_id}/`) and moves it, changing only | ||
| the provider id segment of each key. This picks up every document type a provider might have uploaded, | ||
| including ones this migration logic has no other knowledge of. | ||
|
|
||
| Runs after the DynamoDB migration (whose migrated militaryAffiliation records already reference the new | ||
| keys). Best-effort per object: a copy/delete failure is logged and skipped rather than failing the | ||
| migration, and re-running against an already-moved key is a no-op (the source object is simply absent). | ||
| """ | ||
| old_prefix = f'compact/{compact}/provider/{previous_provider_id}/' | ||
| new_prefix = f'compact/{compact}/provider/{new_provider_id}/' | ||
|
|
||
| paginator = config.s3_client.get_paginator('list_objects_v2') | ||
| for page in paginator.paginate(Bucket=config.provider_user_bucket_name, Prefix=old_prefix): | ||
| for s3_object in page.get('Contents', []): | ||
| old_key = s3_object['Key'] | ||
| new_key = new_prefix + old_key[len(old_prefix) :] | ||
| _move_s3_object(old_key=old_key, new_key=new_key) | ||
|
|
||
|
|
||
| def _move_s3_object(*, old_key: str, new_key: str): | ||
| try: | ||
| config.s3_client.copy_object( | ||
| Bucket=config.provider_user_bucket_name, | ||
| CopySource={'Bucket': config.provider_user_bucket_name, 'Key': old_key}, | ||
| Key=new_key, | ||
| ) | ||
| config.s3_client.delete_object(Bucket=config.provider_user_bucket_name, Key=old_key) | ||
| except ClientError as e: | ||
| logger.error( | ||
| 'Failed to move provider document to the new keyspace', | ||
| old_key=old_key, | ||
| new_key=new_key, | ||
| error=str(e), | ||
| ) | ||
|
|
||
|
|
||
| def _delete_old_cognito_user_and_send_reregistration_email(*, compact: str, old_registered_email: str): | ||
| """Delete the old provider's Cognito user and email the practitioner to re-register. | ||
|
|
||
| The email is only sent when a user was actually deleted in this run, so an SQS retry of a | ||
| partially-completed migration does not send a duplicate notification. | ||
| """ | ||
| try: | ||
| config.cognito_client.admin_delete_user( | ||
| UserPoolId=config.provider_user_pool_id, | ||
| Username=old_registered_email, | ||
| ) | ||
| except config.cognito_client.exceptions.UserNotFoundException: | ||
| logger.info('Old Cognito user not found (already deleted); skipping re-registration email') | ||
| return | ||
|
|
||
| logger.info('Deleted old Cognito user after SSN correction; sending re-registration email') | ||
| config.email_service_client.send_provider_ssn_correction_reregistration_email( | ||
| compact=compact, | ||
| provider_email=old_registered_email, | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant functions and surrounding code.
git ls-files 'backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py'
ast-grep outline backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py --view expanded
# Show the relevant sections with line numbers.
sed -n '1,220p' backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py | cat -n
sed -n '220,520p' backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py | cat -nRepository: csg-org/CompactConnect
Length of output: 27642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the migration implementation and its caller.
rg -n "migrate_provider_for_ssn_correction|_perform_ssn_correction_migration|_move_provider_documents_to_new_keyspace|_delete_old_cognito_user_and_send_reregistration_email|migration_performed|full_teardown" backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py backend/compact-connect/lambdas/python/provider-data-v1 -S
# Inspect the data client implementation that decides whether migration runs.
rg -n "def migrate_provider_for_ssn_correction|migration_performed|full_teardown|old_provider_registered_email" backend/compact-connect/lambdas/python/provider-data-v1 -SRepository: csg-org/CompactConnect
Length of output: 5444
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the data client implementation and any related status markers.
fd -a '.*' backend/compact-connect/lambdas/python/provider-data-v1 | sed -n '1,200p'
rg -n "class .*DataClient|def migrate_provider_for_ssn_correction|migration_performed|full_teardown|old_provider_registered_email|teardown pending|ssn correction" backend/compact-connect/lambdas/python/provider-data-v1 -SRepository: csg-org/CompactConnect
Length of output: 8361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py')
text = p.read_text()
for needle in ['def _perform_ssn_correction_migration', 'def _move_provider_documents_to_new_keyspace', 'def _move_s3_object', 'def _delete_old_cognito_user_and_send_reregistration_email']:
idx = text.find(needle)
print(f'\n=== {needle} @ {idx} ===')
if idx != -1:
start = max(0, text.rfind('\n', 0, idx-300))
end = min(len(text), idx+2200)
print(text[start:end])
PYRepository: csg-org/CompactConnect
Length of output: 8647
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation of the migrate_provider_for_ssn_correction return contract.
rg -n "migrate_provider_for_ssn_correction" backend -SRepository: csg-org/CompactConnect
Length of output: 440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact implementation boundaries and any explicit retry/status handling.
python3 - <<'PY'
from pathlib import Path
p = Path('backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py')
lines = p.read_text().splitlines()
targets = [
'_perform_ssn_correction_migration',
'_move_provider_documents_to_new_keyspace',
'_move_s3_object',
'_delete_old_cognito_user_and_send_reregistration_email',
]
for i, line in enumerate(lines, 1):
if any(line.startswith(f'def {t}') for t in targets):
start = max(1, i-15)
end = min(len(lines), i+120)
print(f'\n### {line.strip()} (lines {start}-{end})')
for j in range(start, end + 1):
print(f'{j:4d}: {lines[j-1]}')
PYRepository: csg-org/CompactConnect
Length of output: 15034
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py')
text = p.read_text().splitlines()
for i, line in enumerate(text, 1):
if 'def migrate_provider_for_ssn_correction' in line:
start = max(1, i - 40)
end = min(len(text), i + 220)
for j in range(start, end + 1):
print(f'{j:4d}: {text[j-1]}')
break
PYRepository: csg-org/CompactConnect
Length of output: 15714
Keep teardown retryable after the DB migration.
migrate_provider_for_ssn_correction returns migration_performed=False once the corrected license is gone from the old provider, so a redelivered message never reaches the S3 move or Cognito/email branch in ingest.py. A crash after the DB transaction can therefore leave old documents or the old Cognito user behind permanently. Persist a separate teardown state so those side effects can resume independently.
🤖 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/ingest.py`
around lines 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.
Collaborator
Author
There was a problem hiding this comment.
We have broken the transactions out into three phases in the case of a large number of items that need to be migrated. Please review and confirm if this address the issue.
Contributor
There was a problem hiding this comment.
Oops, something went wrong! Please try again later. 🐰 💔
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