Support license upload reverts #1188
Merged
Conversation
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
Contributor
WalkthroughAdds a license-upload rollback disaster-recovery workflow (Lambda + Step Functions + S3), new update-tier SK model with migration, license-upload GSIs, event schemas/publishers for revert events, multiple data-model/schema and data-access changes (tiered retrieval), CDK constructs, tests, and smoke-test orchestration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StepFunctions
participant RollbackLambda as Rollback Lambda
participant DynamoDB
participant S3
participant EventBus
Client->>StepFunctions: Start rollback execution (compact,jurisdiction,start,end)
activate StepFunctions
StepFunctions->>RollbackLambda: Invoke RollbackLicenses
activate RollbackLambda
RollbackLambda->>S3: Try load existing RollbackResults (optional)
alt continuation present
S3-->>RollbackLambda: Return previous RollbackResults
end
RollbackLambda->>DynamoDB: Query licenseUploadDateGSI for time window
DynamoDB-->>RollbackLambda: Return provider/update items
loop Per provider
RollbackLambda->>DynamoDB: Read related license/privilege/provider records
DynamoDB-->>RollbackLambda: Records
alt Eligible for revert
RollbackLambda->>DynamoDB: TransactWriteItems (delete/update)
DynamoDB-->>RollbackLambda: Success
RollbackLambda->>EventBus: publish_license_revert_event / publish_privilege_revert_event
EventBus-->>RollbackLambda: Ack
RollbackLambda->>S3: Append ProviderRevertedSummary
else Ineligible / Orphan
RollbackLambda->>S3: Append ProviderSkippedDetails
else Error
RollbackLambda->>S3: Append ProviderFailedDetails
end
end
RollbackLambda->>S3: Write updated RollbackResults
S3-->>RollbackLambda: Return results key
alt All providers processed
RollbackLambda-->>StepFunctions: {status: COMPLETE, results_key}
else Time budget exceeded
RollbackLambda-->>StepFunctions: {status: IN_PROGRESS, results_key}
end
deactivate RollbackLambda
StepFunctions->>Client: Return status
deactivate StepFunctions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
landonshumway-ia
force-pushed
the
feat/license-upload-reverts
branch
3 times, most recently
from
3a2b1bb to
00ab88b
Compare
landonshumway-ia
force-pushed
the
feat/license-upload-reverts
branch
2 times, most recently
from
b9acbb9 to
6a6d833
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/compact-connect/common_constructs/user_pool.py (1)
189-227: Restore spacing in concatenated suppression reasons.The refactor split these
reasonstrings across literals without trailing whitespace, so Python merges them into words likepermissionsto describeanddeployment.A DLQ. Please reintroduce the spaces to keep the messages readable.- 'reason': 'This is an AWS-managed custom resource Lambda that requires wildcard permissions' - 'to describe CloudFront distributions.', + 'reason': ( + 'This is an AWS-managed custom resource Lambda that requires wildcard permissions ' + 'to describe CloudFront distributions.' + ), @@ - 'reason': 'This is an AWS-managed custom resource Lambda used only during deployment.' - 'A DLQ is not necessary.', + 'reason': ( + 'This is an AWS-managed custom resource Lambda used only during deployment. ' + 'A DLQ is not necessary.' + ), @@ - 'reason': 'This is an AWS-managed custom resource Lambda that needs internet access to' - 'describe CloudFront distributions.', + 'reason': ( + 'This is an AWS-managed custom resource Lambda that needs internet access to ' + 'describe CloudFront distributions.' + ),backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py (1)
218-224: Update comment to reflect the new schedule introduced in commit 7e412ca.The comment describes the old schedule (1am UTC / 5pm PST), but the code was intentionally changed to run at 2am UTC (6pm PST / 7pm PDT) on Nov 10 to account for authorize.net settlement delays. Update the comment to match the current implementation.
-# By default, the authorize.net accounts batch settlements at 4:00pm Pacific Time. -# This daily collector runs an hour later (5pm PST, which is 1am UTC) to collect -# all settled transaction for the last 24 hours. +# By default, the authorize.net accounts batch settlements at 4:00pm Pacific Time. +# This daily collector runs two hours later (6pm PST / 7pm PDT, which is 2am UTC) to collect +# all settled transactions for the last 24 hours, accounting for auth.net processing delays.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py (1)
132-170: IncludeproviderIdin the test GSI projectionThe moto table now defines
licenseUploadDateGSIas KEYS_ONLY, so queries in our rollback tests won’t returnproviderId. The rollback Lambda depends on that attribute (see the production table’s INCLUDE projection inbackend/compact-connect/stacks/persistent_stack/provider_table.py), and today’s integration tests will fail with a missing key once they hit the new path. Please mirror the production schema by projectingproviderId.- { - 'IndexName': 'licenseUploadDateGSI', - 'KeySchema': [ - {'AttributeName': 'licenseUploadDateGSIPK', 'KeyType': 'HASH'}, - {'AttributeName': 'licenseUploadDateGSISK', 'KeyType': 'RANGE'}, - ], - 'Projection': {'ProjectionType': 'KEYS_ONLY'}, - }, + { + 'IndexName': 'licenseUploadDateGSI', + 'KeySchema': [ + {'AttributeName': 'licenseUploadDateGSIPK', 'KeyType': 'HASH'}, + {'AttributeName': 'licenseUploadDateGSISK', 'KeyType': 'RANGE'}, + ], + 'Projection': { + 'ProjectionType': 'INCLUDE', + 'NonKeyAttributes': ['providerId'], + }, + },
🧹 Nitpick comments (10)
backend/compact-connect/lambdas/python/migration/tests/__init__.py (1)
30-86: Consider extracting JURISDICTIONS list to a constant.The 55-line inline jurisdiction list is functional but could be more maintainable as a shared constant or loaded from a data file. However, for test setup, having the data inline provides clear visibility into test configuration, so this refactor is optional.
backend/compact-connect/pipeline/backend_stage.py (1)
208-222: Update the dependency comment to reflect the actual stacks.The comment calls out an ApiLambdaStack dependency, but the code declares dependencies on ApiStack and EventListenerStack. Please either adjust the comment or add the intended ApiLambdaStack dependency so everything stays in sync.
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py (3)
46-57: Avoid accumulating entire table in memory; stream process per scan page.Scanning the whole table then batching can spike memory. Process each scan page directly into transactions to cap memory.
Sketch:
- update_records = [] - scan_pagination = {} - while True: - response = config.provider_table.scan(..., **scan_pagination) - items = response.get('Items', []) - update_records.extend(items) - ... - for i in range(0, len(update_records), batch_size): - batch = update_records[i : i + batch_size] - _process_batch(batch) + scan_pagination = {} + batch = [] + while True: + response = config.provider_table.scan(..., **scan_pagination) + items = response.get('Items', []) + for it in items: + batch.append(it) + if len(batch) == batch_size: + _process_batch(batch); batch = [] + lek = response.get('LastEvaluatedKey') + if not lek: + break + scan_pagination = {'ExclusiveStartKey': lek} + if batch: + _process_batch(batch)Also applies to: 71-81
56-56: Misleading log message (“privilege update records”).You’re scanning provider, license, and privilege update records. Update the log text for clarity.
19-27: Docstring mismatch on on_update.The on_update() docstring says “No-op on delete.” Fix to “No-op on update.”
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (4)
971-1008: Direct privilege get: clean error path and normalize key casing.
- Return path already checks 'Item' in response; the KeyError except is redundant.
- Consider lowercasing inputs to match canonical key format (jurisdiction, abbr) to avoid case-sensitive misses.
- try: - response = self.config.provider_table.get_item( - Key={'pk': pk, 'sk': sk}, - ConsistentRead=consistent_read, - ) - if 'Item' not in response: - raise CCNotFoundException('Privilege not found') - return PrivilegeData.from_database_record(response['Item']) - except KeyError as e: - raise CCNotFoundException('Privilege not found') from e + response = self.config.provider_table.get_item( + Key={'pk': pk, 'sk': sk}, + ConsistentRead=consistent_read, + ) + item = response.get('Item') + if not item: + raise CCNotFoundException('Privilege not found') + return PrivilegeData.from_database_record(item)
1009-1067: Dual-pattern query can return duplicate logical updates.Querying both old and new SK patterns is correct during migration, but it may surface two copies of the same logical update. Deduplicate (e.g., by a stable fingerprint such as sk, or a change-hash if available) before returning.
- response_items = [] + response_items: list[dict] = [] @@ - return [PrivilegeUpdateData.from_database_record(item) for item in response_items] + # Deduplicate by ('type','pk','sk') to avoid duplicates during migration + seen = set() + unique_items = [] + for it in response_items: + key = (it.get('type'), it.get('pk'), it.get('sk')) + if key not in seen: + seen.add(key) + unique_items.append(it) + return [PrivilegeUpdateData.from_database_record(item) for item in unique_items]
2721-2769: Minor: updatedValues shouldn’t include dateOfUpdate; set dateOfUpdate only on the main record.In jurisdiction-change deactivation, we set updatedValues.homeJurisdictionChangeStatus and also include dateOfUpdate inside updatedValues. Typically dateOfUpdate is a top-level field of the target record, not part of updatedValues in the history record. Consider removing it from updatedValues to keep update records semantically clean.
927-939: Consistent timestamp source: prefer self.config.current_standard_datetime.Several places use module-level config.current_standard_datetime; others use self.config. For consistency and testability, prefer self.config.current_standard_datetime everywhere.
Also applies to: 2636-2649, 2798-2808, 3494-3507
backend/compact-connect/lambdas/python/disaster-recovery/tests/__init__.py (1)
12-31: Test env bootstrap—LGTM.Good coverage of env vars; config monkey‑patch is clear. Consider a brief comment that LICENSE_TYPES structure mirrors prod format to prevent drift.
Also applies to: 88-104
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/__init__.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
Show resolved
Hide resolved
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
1120-1123: Logger decorator arg name mismatch with function param.Decorator requests 'license_type' but function arg is 'license_type_abbr'; context key stays empty.
- @logger_inject_kwargs(logger, 'compact', 'provider_id', 'jurisdiction', 'license_type') + @logger_inject_kwargs(logger, 'compact', 'provider_id', 'jurisdiction', 'license_type_abbr') def deactivate_privilege( - self, *, compact: str, provider_id: str, jurisdiction: str, license_type_abbr: str, deactivation_details: dict + self, *, compact: str, provider_id: str, jurisdiction: str, license_type_abbr: str, deactivation_details: dict
1869-1884: License investigation close: missing closeDate filter.Comment says “open investigations,” but filter does not exclude closed ones (unlike privilege path).
- open_investigations = provider_records.get_investigation_records_for_license( - jurisdiction, - license_type_abbreviation, - filter_condition=lambda inv: inv.investigationId != investigation_id, - ) + open_investigations = provider_records.get_investigation_records_for_license( + jurisdiction, + license_type_abbreviation, + filter_condition=lambda inv: inv.closeDate is None and inv.investigationId != investigation_id, + )
♻️ Duplicate comments (2)
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py (1)
116-159: Backfill createDate/effectiveDate from legacy dateOfUpdate to match migration intent.Docstring claims createDate is added, but code never sets it; license/privilege updates also need effectiveDate.
@@ - update_data = data_class.from_database_record(original_update_record) - migrated_provider_update_record = update_data.serialize_to_database_record() + update_data = data_class.from_database_record(original_update_record) + # Backfill timestamps from legacy dateOfUpdate if missing + legacy_dou = original_update_record.get('dateOfUpdate') + dou_dt = None + if legacy_dou: + try: + from datetime import datetime + dou_dt = datetime.fromisoformat(legacy_dou) + except Exception: # keep None if parse fails + dou_dt = None + if dou_dt: + current = update_data.to_dict() + if record_type == ProviderRecordType.PROVIDER_UPDATE and not current.get('createDate'): + update_data.update({'createDate': dou_dt}) + if record_type in (ProviderRecordType.LICENSE_UPDATE, ProviderRecordType.PRIVILEGE_UPDATE): + patch = {} + if not current.get('createDate'): + patch['createDate'] = dou_dt + if not current.get('effectiveDate'): + patch['effectiveDate'] = dou_dt + if patch: + update_data.update(patch) + migrated_provider_update_record = update_data.serialize_to_database_record()If schemas already auto-backfill, update the docstring instead. [Based on learnings]
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (1)
178-201: Preserve stored revisionId types (UUID) when loading from S3.Currently passing strings into dataclasses typed as UUID; parse them. Prevents type drift in subsequent publishes.
- licenses_reverted=[ + licenses_reverted=[ RevertedLicense( jurisdiction=reverted_license['jurisdiction'], license_type=reverted_license['licenseType'], - revision_id=reverted_license['revisionId'], + revision_id=UUID(reverted_license['revisionId']), action=reverted_license['action'], ) for reverted_license in summary.get('licensesReverted', []) ], privileges_reverted=[ RevertedPrivilege( jurisdiction=reverted_privilege['jurisdiction'], license_type=reverted_privilege['licenseType'], - revision_id=reverted_privilege['revisionId'], + revision_id=UUID(reverted_privilege['revisionId']), action=reverted_privilege['action'], ) for reverted_privilege in summary.get('privilegesReverted', []) ],
🧹 Nitpick comments (6)
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py (2)
96-110: Clarify the time calculation comment.The comment on line 106 states "the license was originally uploaded a day before the bad upload," but the actual calculation on line 107 sets it to
self.default_start_datetime - timedelta(days=1), which is two days before the bad upload (sincedefault_start_datetimeis already one day beforedefault_upload_datetime).Consider updating the comment to accurately reflect the calculation:
- # by default, the license was originally uploaded a day before the bad upload + # by default, the license was originally uploaded two days before the bad upload (one day before the rollback window start) license_upload_datetime = self.default_start_datetime - timedelta(days=1)
620-883: Consider more maintainable assertions for SHA values.The tests correctly verify the S3 results structure, but the hardcoded SHA values (lines 647, 724-725, 1134, 1150) are brittle and will break if test data changes, as noted in the comments. While this provides precise verification, it creates maintenance overhead.
Consider using pattern matching or format validation instead:
# Instead of exact SHA match: 'updatesDeleted': [ 'aslp#UPDATE#3#license/oh/slp/2025-10-23T07:15:00+00:00/d92450a96739428f1a77c051dce9d4a6' ] # Consider validating the format: self.assertEqual(len(results_data['revertedProviderSummaries'][0]['updatesDeleted']), 1) update_key = results_data['revertedProviderSummaries'][0]['updatesDeleted'][0] self.assertRegex(update_key, r'^aslp#UPDATE#\d+#license/oh/slp/[\d\-T:+]+/[a-f0-9]{32}$')This would make the tests more resilient to test data changes while still verifying the correct format.
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py (1)
57-58: Misleading log message.You’re scanning all update types, not just privilege updates.
-logger.info(f'Found {len(items)} privilege update records in current scan batch') +logger.info(f'Found {len(items)} update records in current scan batch')backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (1)
516-523: SK extraction should handle both typed and untyped transaction items.After TypeSerializer, Items/Keys are nested. Make the logger robust.
- if 'Put' in transaction_item: - return transaction_item['Put']['Item'].get('sk') + if 'Put' in transaction_item: + item = transaction_item['Put']['Item'] + return (item.get('sk') or (item.get('M') or {}).get('sk', {}).get('S')) if 'Delete' in transaction_item: - return transaction_item['Delete']['Key'].get('sk') + key = transaction_item['Delete']['Key'] + return (key.get('sk') or key.get('sk', {}).get('S')) if 'Update' in transaction_item: - return transaction_item['Update']['Key'].get('sk') + key = transaction_item['Update']['Key'] + return (key.get('sk') or key.get('sk', {}).get('S')) return Nonebackend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py (2)
360-360: Consider moving import to module level.Importing
dateinside the function works but is unconventional. Consider adding it to the module-level imports at line 5 for consistency.Apply this diff:
-from datetime import UTC, datetime, timedelta +from datetime import UTC, date, datetime, timedeltaThen remove the import from line 360.
407-410: Add ConsistentRead for test reliability.The
get_itemcall should useConsistentRead=Trueto ensure it reads the latest license data, especially since this follows recent uploads. Without it, the test might read stale data from a read replica, potentially causing flakiness.Apply this diff:
response = config.provider_user_dynamodb_table.get_item( - Key={'pk': f'{compact}#PROVIDER#{provider_id}', 'sk': license_sk} + Key={'pk': f'{compact}#PROVIDER#{provider_id}', 'sk': license_sk}, + ConsistentRead=True )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(10 hunks)backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py(1 hunks)backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py(1 hunks)backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py(1 hunks)backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
📚 Learning: 2025-08-18T19:24:36.640Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1012
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py:187-187
Timestamp: 2025-08-18T19:24:36.640Z
Learning: In the CompactConnect codebase, when migrating effectiveDate fields from date to datetime, a migration script is used to update existing records rather than adding backward compatibility validation at the schema load level.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
📚 Learning: 2025-09-03T22:33:32.687Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.ts:51-53
Timestamp: 2025-09-03T22:33:32.687Z
Learning: For encumbrance and lifting_encumbrance events in the privilege system, createDate should always be present. If createDate is absent, it indicates a bug in the data pipeline that should be allowed to surface rather than being defensively handled.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-09-04T17:55:02.692Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1058
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:785-786
Timestamp: 2025-09-04T17:55:02.692Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py, the sanitize_provider_account_profile_fields_from_response method is called within generate_api_response_object, which serves as the central method that all endpoints use to construct provider response objects. This ensures that sensitive profile fields are sanitized at the source, eliminating the need for additional sanitization calls in downstream methods like those in utils.py.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-06-03T16:41:07.757Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 796
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:58-64
Timestamp: 2025-06-03T16:41:07.757Z
Learning: The static `get_provider_record` method in `ProviderRecordUtility` is legacy code being phased out in favor of the `ProviderUserRecords` class. During this migration, they are keeping the static method backwards compatible (returning `None`) while the new `ProviderUserRecords` class implements the better pattern of raising exceptions.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-05-14T16:26:16.829Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 798
File: backend/compact-connect/lambdas/python/common/cc_common/license_util.py:36-46
Timestamp: 2025-05-14T16:26:16.829Z
Learning: In the `get_valid_license_type_abbreviations` method in `license_util.py`, KeyErrors should not be caught as they indicate an invalid compact value, which is intended to fail explicitly rather than silently returning an empty set.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-06-18T05:57:18.225Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 864
File: backend/compact-connect/lambdas/python/common/cc_common/license_util.py:18-37
Timestamp: 2025-06-18T05:57:18.225Z
Learning: In the `get_license_type_by_abbreviation` method in `license_util.py`, KeyError exceptions from invalid compact codes are intentionally caught and re-raised as CCInvalidRequestException with `from e` to provide a consistent interface between invalid compact and invalid abbreviation scenarios, while preserving the original exception information in the chain.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-09-03T22:27:56.621Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py:71-71
Timestamp: 2025-09-03T22:27:56.621Z
Learning: The privilegeJurisdictions field in provider schemas consistently uses load_default=set() pattern across multiple schema files in the CompactConnect codebase (provider/record.py and provider/api.py), and developers prioritize maintaining this consistency.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-08-12T19:49:24.999Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1001
File: backend/compact-connect/lambdas/python/disaster-recovery/requirements.in:1-1
Timestamp: 2025-08-12T19:49:24.999Z
Learning: In CompactConnect disaster-recovery Lambda functions, runtime dependencies like boto3, aws-lambda-powertools, and botocore are provided by lambda layers at deploy time rather than being specified in requirements.in files. The requirements.in file intentionally contains only a comment explaining this approach.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-06-04T17:38:22.392Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-09-12T14:14:53.986Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1083
File: backend/compact-connect/bin/generate_mock_license_csv_upload_file.py:39-41
Timestamp: 2025-09-12T14:14:53.986Z
Learning: In backend/compact-connect/bin/generate_mock_license_csv_upload_file.py, the functions are intentionally designed for CLI-only usage and should not be called programmatically. The code is meant to fail fast if developers try to import and use these functions outside of the CLI context, rather than having defensive programming guards.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
🧬 Code graph analysis (5)
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py (6)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (1)
LicenseUpdateData(148-201)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
PrivilegeUpdateData(95-166)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
ProviderRecordType(29-42)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
ProviderUpdateData(151-192)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
provider_table(88-89)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
from_database_record(124-139)serialize_to_database_record(205-208)
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py (7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-32)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCNotFoundException(32-33)backend/compact-connect/lambdas/python/disaster-recovery/tests/function/__init__.py (2)
TstFunction(15-129)setUp(18-37)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (6)
put_default_provider_record_in_provider_table(476-496)put_default_license_record_in_provider_table(291-301)put_default_license_update_record_in_provider_table(332-343)put_default_privilege_record_in_provider_table(377-387)put_default_privilege_update_record_in_provider_table(426-440)put_default_provider_update_record_in_provider_table(530-548)backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (2)
to_dict(95-147)rollback_license_upload(205-391)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(177-220)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (7)
get_provider_record(81-86)get_provider_record(694-703)get_license_records(524-531)get_all_license_update_records(782-793)get_privilege_records(493-501)get_all_privilege_update_records(795-806)get_specific_license_record(459-474)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-32)backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
logger_inject_kwargs(215-235)get(86-87)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (3)
ProviderUserRecords(412-931)get_provider_record(81-86)get_provider_record(694-703)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
current_standard_datetime(261-265)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (4)
create_new(96-121)to_dict(169-180)from_database_record(124-139)update(182-203)
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (12)
ProviderUserRecords(412-931)get_all_license_update_records(782-793)get_license_records(524-531)get_all_provider_update_records(808-819)get_privileges_associated_with_license(503-522)get_update_records_for_privilege(842-861)get_specific_privilege_record(476-491)get_update_records_for_license(821-840)get_provider_record(81-86)get_provider_record(694-703)find_best_license_in_current_known_licenses(705-765)populate_provider_record(192-228)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (5)
UpdateCategory(296-312)to_dict(169-180)update(182-203)licenseTypeAbbreviation(156-167)create_new(96-121)backend/compact-connect/lambdas/python/common/cc_common/event_batch_writer.py (1)
EventBatchWriter(4-49)backend/compact-connect/lambdas/python/common/cc_common/config.py (7)
disaster_recovery_results_bucket_name(191-192)provider_table(88-89)data_client(42-45)provider_table_name(155-156)events_client(68-69)event_bus_client(328-331)s3_client(34-35)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(177-220)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (2)
LicenseData(9-145)firstUploadDate(144-145)backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (2)
publish_license_revert_event(441-490)publish_privilege_revert_event(492-541)
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (2)
LicenseData(9-145)LicenseUpdateData(148-201)backend/compact-connect/tests/smoke/smoke_common.py (9)
SmokeTestFailureException(13-17)create_test_app_client(491-532)create_test_staff_user(107-134)delete_test_app_client(535-543)delete_test_staff_user(86-104)get_api_base_url(196-197)get_client_auth_headers(590-601)get_provider_user_records(254-282)load_smoke_test_env(224-227)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
get_all_license_update_records(782-793)backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (1)
to_dict(95-147)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: TestApp
- GitHub Check: TestApp
🔇 Additional comments (8)
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py (6)
1-78: Excellent test structure and comprehensive setup.The test class setup is well-organized with clear helper methods for creating test data. The time window calculations (lines 46-48) correctly establish a realistic test scenario. The use of
@mock_awsand datetime patching ensures isolated, reproducible tests.
343-507: LGTM! Core rollback functionality is thoroughly tested.These tests effectively verify the primary rollback scenarios including provider record reversion, license deletion, license state restoration, privilege reactivation, and update record cleanup. The assertions are specific and verify both the rollback status and the actual data state.
508-605: LGTM! Skip scenarios and input validation are well covered.The tests appropriately verify that providers with intervening updates are skipped (preserving data integrity) and that invalid inputs are properly rejected with clear error messages.
914-1046: LGTM! Complex but necessary test for S3 results persistence.While this test is lengthy (132 lines), the complexity is justified by the importance of verifying that rollback operations can correctly load, preserve, and append to existing results across multiple invocations. The test comprehensively validates this critical resumability feature.
1047-1158: LGTM! Pagination logic is thoroughly tested.This test effectively simulates the Lambda timeout scenario by mocking
time.time()to control execution timing. It correctly verifies that:
- The handler returns
IN_PROGRESSwith a continuation token when timing out- The handler resumes from the correct provider on the next invocation
- Results from both invocations are properly persisted in S3
The test demonstrates good understanding of Step Function continuation patterns.
1159-1329: LGTM! Edge cases and error handling are comprehensively tested.These tests cover critical edge cases:
- Transaction failures are properly caught and logged
- Orphaned update records (data inconsistency) trigger appropriate skip behavior with
typeOfUpdate='Orphaned'correctly set in the handler- Non-upload updates within the time window are correctly distinguished from upload-related updates
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (1)
636-666: transact_write_items payload must be AttributeValues; add TypeSerializer and typed Keys.Current Put/Key shapes are untyped dicts and will fail with the low-level client.
@@ - def add_put(item: dict, update_record: bool): + from boto3.dynamodb.types import TypeSerializer + _ts = TypeSerializer() + def add_put(item: dict, update_record: bool): @@ - transaction_item = { + transaction_item = { 'Put': { 'TableName': table_name, - 'Item': item, + 'Item': _ts.serialize(item)['M'], } } @@ - def add_delete(pk: str, sk: str, update_record: bool): + def add_delete(pk: str, sk: str, update_record: bool): @@ - transaction_item = { + transaction_item = { 'Delete': { 'TableName': table_name, - 'Key': {'pk': pk, 'sk': sk}, + 'Key': {'pk': {'S': pk}, 'sk': {'S': sk}}, } } @@ - primary_record_transaction_items.append( + primary_record_transaction_items.append( { 'Update': { 'TableName': table_name, - 'Key': {'pk': serialized_privilege['pk'], 'sk': serialized_privilege['sk']}, + 'Key': { + 'pk': {'S': serialized_privilege['pk']}, + 'sk': {'S': serialized_privilege['sk']}, + }, 'UpdateExpression': 'REMOVE licenseDeactivatedStatus', } } ) @@ - _perform_transaction(transaction_items, provider_id) + _perform_transaction(transaction_items, provider_id)Also applies to: 768-776, 883-896
⛔ Skipped due to learnings
Learnt from: landonshumway-ia Repo: csg-org/CompactConnect PR: 1001 File: backend/compact-connect/lambdas/python/disaster-recovery/handlers/copy_records.py:1-9 Timestamp: 2025-08-12T22:03:08.846Z Learning: In the CompactConnect system, DynamoDB table primary keys (pk) and sort keys (sk) do not use Decimal types, so LastEvaluatedKey from scan operations can be safely serialized using json.dumps() without needing DynamoDB TypeSerializer/TypeDeserializer for Decimal handling.backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py (1)
170-235: Excellent fix for the duplicate provider ID issue!The implementation now correctly uses a set to track unique provider IDs across polling iterations, preventing premature success when duplicate IDs are accumulated. The deterministic sorted return value is also a nice touch.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
Show resolved
Hide resolved
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
Show resolved
Hide resolved
landonshumway-ia
force-pushed
the
feat/license-upload-reverts
branch
from
5c5005c to
4aa6724
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (3)
858-859: Reverted license is a primary record; should use update_record=False.Line 858 calls
add_put(serialized_reverted_license, update_record=True), but the reverted license is a primary (top-level) license record with a primary SK pattern, not an update record. This adds it toupdate_record_transactions_itemsinstead ofprimary_record_transaction_items. While this might not cause functional issues since both lists are combined (line 889), it's semantically incorrect and could affect transaction ordering expectations.Apply this diff:
- add_put(serialized_reverted_license, update_record=True) + add_put(serialized_reverted_license, update_record=False)
180-197: Convert revisionId strings to UUID when deserializing from S3.The
RevertedLicenseandRevertedPrivilegedataclasses definerevision_id: UUID(lines 63, 73), butfrom_dictpasses the JSON string directly without conversion (lines 184, 193). Python dataclasses don't auto-convert types, so this stores a string in a UUID-typed field, which can cause type errors downstream or when the value is used in operations expecting a UUID.Apply this diff:
licenses_reverted=[ RevertedLicense( jurisdiction=reverted_license['jurisdiction'], license_type=reverted_license['licenseType'], - revision_id=reverted_license['revisionId'], + revision_id=UUID(reverted_license['revisionId']), action=reverted_license['action'], ) for reverted_license in summary.get('licensesReverted', []) ], privileges_reverted=[ RevertedPrivilege( jurisdiction=reverted_privilege['jurisdiction'], license_type=reverted_privilege['licenseType'], - revision_id=reverted_privilege['revisionId'], + revision_id=UUID(reverted_privilege['revisionId']), action=reverted_privilege['action'], ) for reverted_privilege in summary.get('privilegesReverted', []) ],
675-686: ReplacedateOfUpdatewithcreateDateon lines 675 and 680.ProviderUpdateData does not expose a
dateOfUpdateproperty. The ProviderUpdateRecordSchema contains adateOfUpdatefield, but its pre-load hook converts it tocreateDateduring deserialization. The only exposed property on ProviderUpdateData iscreateDate.Accessing
update.dateOfUpdateon lines 675 and 680 will raiseAttributeError. Change both instances toupdate.createDate.backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py (1)
323-329: Fix docstring parameter mismatch.The docstring references a
bucket_nameparameter that doesn't exist in the function signature. The bucket name is extracted from theresults_s3_keyS3 URI.Apply this diff to fix the docstring:
def get_rollback_results_from_s3(results_s3_key: str): """ Retrieve rollback results from S3. :param results_s3_key: S3 URI or key to the results file - :param bucket_name: S3 bucket name :return: Parsed results data """
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
1036-1043: Track migration completion and remove old SK pattern.The dual-pattern query (new + old SK patterns) is appropriate during migration, but the old pattern should be removed once all records are migrated.
Consider creating a tracking issue or setting a timeline to verify migration completion and clean up the old pattern query at lines 1042-1043.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(10 hunks)backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py(1 hunks)backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py(1 hunks)backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py(2 hunks)backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/init.py
🧰 Additional context used
🧠 Learnings (30)
📓 Common learnings
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
📚 Learning: 2025-06-04T17:38:22.392Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-09-12T14:14:53.986Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1083
File: backend/compact-connect/bin/generate_mock_license_csv_upload_file.py:39-41
Timestamp: 2025-09-12T14:14:53.986Z
Learning: In backend/compact-connect/bin/generate_mock_license_csv_upload_file.py, the functions are intentionally designed for CLI-only usage and should not be called programmatically. The code is meant to fail fast if developers try to import and use these functions outside of the CLI context, rather than having defensive programming guards.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-08-18T19:24:36.640Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1012
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py:187-187
Timestamp: 2025-08-18T19:24:36.640Z
Learning: In the CompactConnect codebase, when migrating effectiveDate fields from date to datetime, a migration script is used to update existing records rather than adding backward compatibility validation at the schema load level.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.pybackend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-09-03T22:33:32.687Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.ts:51-53
Timestamp: 2025-09-03T22:33:32.687Z
Learning: For encumbrance and lifting_encumbrance events in the privilege system, createDate should always be present. If createDate is absent, it indicates a bug in the data pipeline that should be allowed to surface rather than being defensively handled.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.pybackend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-08-12T22:03:08.846Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1001
File: backend/compact-connect/lambdas/python/disaster-recovery/handlers/copy_records.py:1-9
Timestamp: 2025-08-12T22:03:08.846Z
Learning: In the CompactConnect system, DynamoDB table primary keys (pk) and sort keys (sk) do not use Decimal types, so LastEvaluatedKey from scan operations can be safely serialized using json.dumps() without needing DynamoDB TypeSerializer/TypeDeserializer for Decimal handling.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
📚 Learning: 2025-10-28T15:05:49.315Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1167
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:1698-1701
Timestamp: 2025-10-28T15:05:49.315Z
Learning: In the CompactConnect Python codebase, enums such as InvestigationStatusEnum, InvestigationAgainstEnum, ActiveInactiveStatus, LicenseEncumberedStatusEnum, and PrivilegeEncumberedStatusEnum derive from StrEnum. When writing DynamoDB AttributeValues (e.g., {'S': ...}) in files like backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py, pass the enum member directly (without .value). Future reviews should not recommend using .value for these enums.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-06-17T19:05:36.255Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-07-29T02:35:59.401Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 952
File: backend/compact-connect/lambdas/python/migration/license_update_effective_date_931/main.py:0-0
Timestamp: 2025-07-29T02:35:59.401Z
Learning: For temporary migration scripts in CompactConnect that process pre-vetted database records and have been verified in development environments, extensive validation for data format may be unnecessary overhead. The user landonshumway-ia indicated that migration scripts can rely on data integrity when the records have been vetted and the script has been tested.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
📚 Learning: 2025-07-21T20:39:47.625Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 907
File: backend/compact-connect/lambdas/python/cognito-backup/handlers/cognito_backup.py:148-149
Timestamp: 2025-07-21T20:39:47.625Z
Learning: In the CompactConnect Cognito backup system, UserCreateDate and UserLastModifiedDate fields from the AWS Cognito ListUsers API are accessed using direct dictionary access (user_data['UserCreateDate']) rather than .get() method. This is intentional per landonshumway-ia's decision to fail fast if AWS breaks their documented API contract that guarantees these fields are always present. The KeyError that would result from missing fields should trigger alerts to detect API contract violations rather than being silently handled.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.pybackend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-21T15:23:26.019Z
Learnt from: rmolinares
Repo: csg-org/CompactConnect PR: 1025
File: webroot/src/models/Licensee/Licensee.model.ts:242-254
Timestamp: 2025-08-21T15:23:26.019Z
Learning: In the CompactConnect codebase, the LicenseSerializer.fromServer() method maps the server response field `dateOfIssuance` to the License model's `issueDate` property. After serialization, License instances only contain `issueDate` and never have `dateOfIssuance`. When reviewing date comparisons in reducers that work with License objects, check if they're working with serialized License instances (which use `issueDate`) rather than raw server data (which uses `dateOfIssuance`).
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.pybackend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-10-15T19:56:58.050Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py:60-0
Timestamp: 2025-10-15T19:56:58.050Z
Learning: In the CompactConnect codebase, when migrating from a single field to a list field (e.g., clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories), both fields are intentionally kept as optional during the migration period to support backwards compatibility. Mutual exclusivity validation is not enforced during this phase, as the deprecated field will be removed in a follow-up PR with migration scripts.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
📚 Learning: 2025-08-21T19:57:13.276Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:0-0
Timestamp: 2025-08-21T19:57:13.276Z
Learning: In the CompactConnect codebase, ProviderData.recoveryExpiry property returns `datetime | None` after the type hint fix. The marshmallow DateTime field in the record schema handles conversion between ISO strings (database storage) and datetime objects (application use), so test assertions can directly compare datetime objects.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
📚 Learning: 2025-08-12T19:49:24.999Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1001
File: backend/compact-connect/lambdas/python/disaster-recovery/requirements.in:1-1
Timestamp: 2025-08-12T19:49:24.999Z
Learning: In CompactConnect disaster-recovery Lambda functions, runtime dependencies like boto3, aws-lambda-powertools, and botocore are provided by lambda layers at deploy time rather than being specified in requirements.in files. The requirements.in file intentionally contains only a comment explaining this approach.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-08-29T21:45:05.792Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1016
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2702-2710
Timestamp: 2025-08-29T21:45:05.792Z
Learning: In the lift_home_jurisdiction_license_privilege_encumbrances method, when latest_effective_lift_date is None, the method always returns an empty list for affected_privileges, so the existing if affected_privileges guard in the calling code already prevents event publishing without needing explicit None checks for the date.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-04-29T02:10:51.996Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.py:28-30
Timestamp: 2025-04-29T02:10:51.996Z
Learning: JSON decoding errors are handled in the api_handler wrapper function in this codebase, so individual handlers don't need to explicitly catch json.JSONDecodeError.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-09-04T17:55:02.692Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1058
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:785-786
Timestamp: 2025-09-04T17:55:02.692Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py, the sanitize_provider_account_profile_fields_from_response method is called within generate_api_response_object, which serves as the central method that all endpoints use to construct provider response objects. This ensures that sensitive profile fields are sanitized at the source, eliminating the need for additional sanitization calls in downstream methods like those in utils.py.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-06-10T03:16:16.896Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/migration/provider_user_pool_migration_551/main.py:35-39
Timestamp: 2025-06-10T03:16:16.896Z
Learning: In the provider user pool migration (provider_user_pool_migration_551), the FilterExpression intentionally only checks for the existence of compactConnectRegisteredEmailAddress. The migration should only remove currentHomeJurisdiction if compactConnectRegisteredEmailAddress is also present, targeting records that went through the old registration process. Records with only currentHomeJurisdiction but no compactConnectRegisteredEmailAddress should be left untouched.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-05-14T16:26:16.829Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 798
File: backend/compact-connect/lambdas/python/common/cc_common/license_util.py:36-46
Timestamp: 2025-05-14T16:26:16.829Z
Learning: In the `get_valid_license_type_abbreviations` method in `license_util.py`, KeyErrors should not be caught as they indicate an invalid compact value, which is intended to fail explicitly rather than silently returning an empty set.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-06-18T05:57:18.225Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 864
File: backend/compact-connect/lambdas/python/common/cc_common/license_util.py:18-37
Timestamp: 2025-06-18T05:57:18.225Z
Learning: In the `get_license_type_by_abbreviation` method in `license_util.py`, KeyError exceptions from invalid compact codes are intentionally caught and re-raised as CCInvalidRequestException with `from e` to provide a consistent interface between invalid compact and invalid abbreviation scenarios, while preserving the original exception information in the chain.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-11-13T21:02:07.057Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1188
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:187-198
Timestamp: 2025-11-13T21:02:07.057Z
Learning: In Python, StrEnum members inherit from str and can be directly used in operations that accept strings. For example, int(UpdateTierEnum.TIER_ONE) where TIER_ONE = '1' works without needing .value because the enum member itself is a string. This applies to type conversions and string operations on StrEnum members.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-09-10T13:50:30.047Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1069
File: backend/compact-connect/lambdas/python/purchases/handlers/transaction_reporting.py:18-22
Timestamp: 2025-09-10T13:50:30.047Z
Learning: In Python 3.11, using `'string' in StrEnum` raises TypeError. StrEnum membership checking with strings only works starting in Python 3.12. For Python 3.11, convert StrEnum values to a set for membership testing or use explicit equality comparisons.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-09-10T13:50:30.047Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1069
File: backend/compact-connect/lambdas/python/purchases/handlers/transaction_reporting.py:18-22
Timestamp: 2025-09-10T13:50:30.047Z
Learning: When using Python StrEnum for filtering, the `in` operator correctly checks string values against enum members. `'settledSuccessfully' in ReportableTransactionStatuses` works properly and is more maintainable than using raw string sets. StrEnum members are both enum instances and strings, making them ideal for this use case.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-04-29T01:59:51.222Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/stacks/api_stack/v1_api/api_model.py:397-401
Timestamp: 2025-04-29T01:59:51.222Z
Learning: In the CompactConnect project, validation constraints like enum values should be defined only in runtime code (Lambda) rather than duplicating them in CDK API schema definitions. This applies to fields like clinicalPrivilegeActionCategory and other similar enumerations.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-06-03T16:41:07.757Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 796
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:58-64
Timestamp: 2025-06-03T16:41:07.757Z
Learning: The static `get_provider_record` method in `ProviderRecordUtility` is legacy code being phased out in favor of the `ProviderUserRecords` class. During this migration, they are keeping the static method backwards compatible (returning `None`) while the new `ProviderUserRecords` class implements the better pattern of raising exceptions.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-09-03T22:27:56.621Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py:71-71
Timestamp: 2025-09-03T22:27:56.621Z
Learning: The privilegeJurisdictions field in provider schemas consistently uses load_default=set() pattern across multiple schema files in the CompactConnect codebase (provider/record.py and provider/api.py), and developers prioritize maintaining this consistency.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
🧬 Code graph analysis (5)
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (2)
LicenseData(9-145)LicenseUpdateData(148-201)backend/compact-connect/tests/smoke/smoke_common.py (10)
SmokeTestFailureException(13-17)create_test_app_client(491-532)create_test_staff_user(107-134)delete_test_app_client(535-543)delete_test_staff_user(86-104)get_api_base_url(196-197)get_client_auth_headers(590-601)get_provider_user_records(254-282)get_staff_user_auth_headers(179-183)load_smoke_test_env(224-227)backend/compact-connect/tests/smoke/config.py (4)
load_smoke_test_env(100-103)state_api_base_url(20-21)provider_user_dynamodb_table(44-45)license_upload_rollback_step_function_arn(40-41)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
get_all_license_update_records(782-793)backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (1)
to_dict(95-147)
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py (6)
backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-32)backend/compact-connect/lambdas/python/disaster-recovery/tests/function/__init__.py (2)
TstFunction(15-129)setUp(18-37)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (6)
put_default_provider_record_in_provider_table(476-496)put_default_license_record_in_provider_table(291-301)put_default_license_update_record_in_provider_table(332-343)put_default_privilege_record_in_provider_table(377-387)put_default_privilege_update_record_in_provider_table(426-440)put_default_provider_update_record_in_provider_table(530-548)backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (2)
to_dict(95-147)rollback_license_upload(205-391)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(177-220)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (7)
get_provider_record(81-86)get_provider_record(694-703)get_license_records(524-531)get_all_license_update_records(782-793)get_privilege_records(493-501)get_all_privilege_update_records(795-806)get_specific_license_record(459-474)
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py (7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (1)
LicenseUpdateData(148-201)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
PrivilegeUpdateData(95-166)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
ProviderRecordType(29-42)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
ProviderUpdateData(151-192)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCInternalException(44-45)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
provider_table(88-89)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
from_database_record(124-139)serialize_to_database_record(205-208)
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (12)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (14)
ProviderRecordUtility(55-409)ProviderUserRecords(412-931)get_all_license_update_records(782-793)get_license_records(524-531)get_all_provider_update_records(808-819)get_privileges_associated_with_license(503-522)get_update_records_for_privilege(842-861)get_specific_privilege_record(476-491)get_update_records_for_license(821-840)get_provider_record(81-86)get_provider_record(694-703)get_privilege_records(493-501)find_best_license_in_current_known_licenses(705-765)populate_provider_record(192-228)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
UpdateCategory(296-312)update(182-203)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
PrivilegeData(13-92)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
ProviderData(13-148)backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-32)backend/compact-connect/lambdas/python/common/cc_common/event_batch_writer.py (1)
EventBatchWriter(4-49)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCNotFoundException(32-33)backend/compact-connect/lambdas/python/common/cc_common/config.py (7)
disaster_recovery_results_bucket_name(191-192)provider_table(88-89)data_client(42-45)provider_table_name(155-156)events_client(68-69)event_bus_client(328-331)s3_client(34-35)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(177-220)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (2)
LicenseData(9-145)firstUploadDate(144-145)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (1)
LicenseRecordSchema(38-163)backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (2)
publish_license_revert_event(441-490)publish_privilege_revert_event(492-541)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-32)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (4)
ProviderUserRecords(412-931)ProviderRecordType(29-42)get_provider_record(81-86)get_provider_record(694-703)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (5)
compact(29-30)compact(175-176)ProviderUpdateData(151-192)providerId(25-26)providerId(171-172)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (8)
compact(28-29)compact(112-113)providerId(24-25)providerId(108-109)jurisdiction(32-33)jurisdiction(116-117)PrivilegeData(13-92)PrivilegeUpdateData(95-166)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
current_standard_datetime(261-265)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: TestApp
- GitHub Check: TestApp
🔇 Additional comments (6)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (4)
183-198: LGTM! Tier-based query logic is well-designed.The conditional SK query logic correctly handles both backward-compatible queries (main records only) and tiered update record retrieval. The use of
int(include_update_tier)on the StrEnum is valid since StrEnum members inherit from str and support direct conversion.Based on learnings
928-936: LGTM! Consistent createDate timestamp pattern.The pattern of capturing
now = config.current_standard_datetimeand using it forcreateDateis implemented consistently across all provider update record creation points. This ensures accurate timestamp tracking for the rollback feature.Also applies to: 2637-2645, 2799-2807, 3494-3502
1120-1203: LGTM! Enhanced deactivation tracking.The addition of
deactivation_detailsparameter and use ofUpdateCategory.DEACTIVATIONprovides better audit trail for privilege deactivations, supporting the rollback feature requirements.
1009-1067: No action required - method handles migration period correctly.The method is designed appropriately for the migration period. DynamoDB transactions ensure each privilege update record is atomically migrated (old record deleted and new record created together). A specific privilege update cannot exist in both patterns simultaneously; the method queries both patterns because during migration some updates remain in the old pattern while others have been moved to the new pattern. This is expected behavior, not a duplicate issue.
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py (2)
504-512: Verify logger keyword arguments are supported by log configuration.Lines 506 and 512 pass
skipped=detailandfailed=detailas keyword arguments tologger.error(). While Python's logging accepts arbitrary kwargs, these won't appear in the log output unless your log formatter is specifically configured to include extra fields. In most configurations, these details will be silently ignored.Consider using explicit formatting instead:
if num_skipped != expected_skipped_count: logger.error(f'Found {num_skipped} skipped providers, expected {expected_skipped_count}:') for detail in skipped[:5]: # Show first 5 - logger.error(f'Details for skipped provider: {detail["providerId"]}', skipped=detail) + logger.error(f'Details for skipped provider {detail["providerId"]}: {detail}') raise SmokeTestFailureException(f'Expected {expected_skipped_count} skipped providers but found {num_skipped}') if num_failed > 0: logger.error(f'Found {num_failed} failed providers:') for detail in failed[:5]: # Show first 5 - logger.error(f'Details for failed provider: {detail["providerId"]}', failed=detail) + logger.error(f'Details for failed provider {detail["providerId"]}: {detail}') raise SmokeTestFailureException(f'Expected 0 failed providers but found {num_failed}')Or verify that your logging configuration includes these extra fields in the output.
181-181: No changes needed - line 181 is consistent with project's type hint convention.The project explicitly targets Python 3.13 (per
requirements.txt), and all smoke test files consistently use modern type hints likelist[dict],tuple[str, str], andset[str]. Line 181'sset[str]syntax is appropriate and aligns with the established codebase pattern.Likely an incorrect or invalid review comment.
...pact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
Show resolved
Hide resolved
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py (1)
196-217: Clarify comment about update timing.The comment on line 204 states "last update was 5 days before upload", but the code sets
createDate: first_upload_datetime - timedelta(days=5). Sincefirst_upload_datetimeis 30 minutes afterdefault_start_datetime(the window start), this places the update ~5 days and 30 minutes before the window start, not 5 days before the first upload within the window. The logic is correct (the update is intentionally placed before the window and should be ignored), but the comment could be more precise.Consider revising the comment to:
- # old update record before upload window (e.g., RENEWAL) + # Existing update record from well before the upload window (should be ignored during rollback)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py (6)
backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-32)backend/compact-connect/lambdas/python/disaster-recovery/tests/function/__init__.py (2)
TstFunction(15-129)setUp(18-37)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (6)
put_default_provider_record_in_provider_table(476-496)put_default_license_record_in_provider_table(291-301)put_default_license_update_record_in_provider_table(332-343)put_default_privilege_record_in_provider_table(377-387)put_default_privilege_update_record_in_provider_table(426-440)put_default_provider_update_record_in_provider_table(530-548)backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (2)
to_dict(95-147)rollback_license_upload(205-391)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(177-220)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (7)
get_provider_record(81-86)get_provider_record(694-703)get_license_records(524-531)get_all_license_update_records(782-793)get_privilege_records(493-501)get_all_privilege_update_records(795-806)get_specific_license_record(459-474)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (6)
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py (6)
1-54: LGTM - Well-structured test setup.The test class setup is comprehensive with appropriate mocking (AWS services, datetime), clear test fixtures, and sensible default time windows for rollback scenarios.
472-673: LGTM - Thorough test coverage for primary rollback scenarios.These tests comprehensively verify the core rollback functionality:
- Provider and license record reversion to prior values
- Full deletion when no prior state exists
- Handling multiple updates by reverting to earliest prior state
- Privilege reactivation when licenses are reverted
- Verification through both record state and serialization comparison
The assertions are precise and verify both the outcomes and the intermediate state.
734-770: LGTM - Input validation properly tested.The validation tests cover the critical input validation scenarios:
- Malformed datetime strings
- Invalid time window ordering
- Maximum time window enforcement
All assertions correctly verify the error responses.
786-1078: LGTM - Comprehensive S3 result verification.These tests thoroughly verify the S3 results structure for all outcome types (reverted, skipped, failed) across diverse scenarios. The tests properly verify:
- Correct categorization of providers
- Complete details for each outcome type
- Proper reason messages for skipped providers
- Error handling and failure logging
Note: The hardcoded SHAs in
updatesDeletedassertions (e.g., line 813) are fragile and will break if test data generation changes. This is documented in comments but consider if there's a more maintainable approach.
1080-1323: LGTM - Pagination and continuation logic thoroughly tested.These tests properly verify the multi-invocation workflow:
Result merging test (lines 1080-1211): Confirms existing S3 results are loaded and preserved when new results are appended, which is critical for the step function continuation pattern.
Pagination test (lines 1213-1323): Properly mocks the timeout scenario with the three-element
side_effectarray fortime.time(), simulating the lambda approaching its time limit and returningIN_PROGRESSstatus with a continuation token. The second invocation correctly resumes from the saved provider ID.Both tests verify the complete data flow including S3 reads/writes and status transitions.
1325-1495: LGTM - Excellent error handling and edge case coverage.These final tests cover critical error and edge case scenarios:
Transaction failure test (lines 1325-1359): Properly mocks DynamoDB transaction failures and verifies the provider is marked as failed with appropriate error details. Correctly patches at the handler module level.
Orphaned updates test (lines 1361-1442): Tests an important data consistency edge case where update records exist without corresponding license records. Properly verifies the provider is skipped with a clear reason indicating manual review is needed.
Non-upload update within window test (lines 1444-1495): Validates that the rollback logic correctly distinguishes between upload-related updates (which can be rolled back) and other update types (which should cause skipping), even when both occur within the time window. This demonstrates the importance of the
updateTypecategorization.The test suite is comprehensive and well-structured.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py (1)
84-113: Consider documenting the table.meta.client dict handling.While you've confirmed this works via an undocumented boto3 feature, adding a brief comment would help future maintainers understand why plain Python dicts are used instead of the documented AttributeValue format.
Example comment to add near line 102:
def _generate_put_transaction_item(item: dict) -> dict: """ Generate a put transaction item for a provider record. Note: When using table.meta.client, boto3 accepts plain Python dicts without explicit AttributeValue serialization (undocumented behavior). :param item: The provider record to put. :return: Put transaction item """This would prevent similar review questions and clarify the design decision.
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (2)
149-202: Hydraterevision_idasUUIDto match dataclass typing
RevertedLicense.revision_idandRevertedPrivilege.revision_idare typed asUUID, butRollbackResults.from_dictcurrently passes raw strings from S3. This works at runtime but diverges from the declared type and from how new revision IDs are created elsewhere, which can confuse type-checkers and future refactors.You can preserve the stored value while restoring the correct type by wrapping with
UUID(...):reverted_provider_summaries=[ ProviderRevertedSummary( provider_id=summary['providerId'], licenses_reverted=[ RevertedLicense( jurisdiction=reverted_license['jurisdiction'], license_type=reverted_license['licenseType'], - revision_id=reverted_license['revisionId'], + revision_id=UUID(reverted_license['revisionId']), action=reverted_license['action'], ) for reverted_license in summary.get('licensesReverted', []) ], privileges_reverted=[ RevertedPrivilege( jurisdiction=reverted_privilege['jurisdiction'], license_type=reverted_privilege['licenseType'], - revision_id=reverted_privilege['revisionId'], + revision_id=UUID(reverted_privilege['revisionId']), action=reverted_privilege['action'], ) for reverted_privilege in summary.get('privilegesReverted', []) ],This keeps persisted IDs intact while aligning the in‑memory model with its annotations.
213-233: Align docstring with actual return contract (include'FAILED'status and extra fields)The docstring’s “Returns” section documents only
'IN_PROGRESS' | 'COMPLETE', but the function also returns'FAILED'along with an'error'field in validation paths, and includes additional fields (e.g.,compact,jurisdiction,rollbackReason,executionName) in theIN_PROGRESSresponse.To avoid confusion for Step Function consumers and future maintainers, update the return-shape docs to cover:
'rollbackStatus': 'IN_PROGRESS' | 'COMPLETE' | 'FAILED'- Extra fields present on
IN_PROGRESSresponses- The
'error'field onFAILEDresponsesNo behavior change needed—just documentation alignment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py(1 hunks)backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py(1 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
📚 Learning: 2025-08-18T19:24:36.640Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1012
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py:187-187
Timestamp: 2025-08-18T19:24:36.640Z
Learning: In the CompactConnect codebase, when migrating effectiveDate fields from date to datetime, a migration script is used to update existing records rather than adding backward compatibility validation at the schema load level.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.pybackend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-09-03T22:33:32.687Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.ts:51-53
Timestamp: 2025-09-03T22:33:32.687Z
Learning: For encumbrance and lifting_encumbrance events in the privilege system, createDate should always be present. If createDate is absent, it indicates a bug in the data pipeline that should be allowed to surface rather than being defensively handled.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.pybackend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
📚 Learning: 2025-08-12T22:03:08.846Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1001
File: backend/compact-connect/lambdas/python/disaster-recovery/handlers/copy_records.py:1-9
Timestamp: 2025-08-12T22:03:08.846Z
Learning: In the CompactConnect system, DynamoDB table primary keys (pk) and sort keys (sk) do not use Decimal types, so LastEvaluatedKey from scan operations can be safely serialized using json.dumps() without needing DynamoDB TypeSerializer/TypeDeserializer for Decimal handling.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
📚 Learning: 2025-10-28T15:05:49.315Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1167
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:1698-1701
Timestamp: 2025-10-28T15:05:49.315Z
Learning: In the CompactConnect Python codebase, enums such as InvestigationStatusEnum, InvestigationAgainstEnum, ActiveInactiveStatus, LicenseEncumberedStatusEnum, and PrivilegeEncumberedStatusEnum derive from StrEnum. When writing DynamoDB AttributeValues (e.g., {'S': ...}) in files like backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py, pass the enum member directly (without .value). Future reviews should not recommend using .value for these enums.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
📚 Learning: 2025-06-17T19:05:36.255Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
📚 Learning: 2025-07-29T02:35:59.401Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 952
File: backend/compact-connect/lambdas/python/migration/license_update_effective_date_931/main.py:0-0
Timestamp: 2025-07-29T02:35:59.401Z
Learning: For temporary migration scripts in CompactConnect that process pre-vetted database records and have been verified in development environments, extensive validation for data format may be unnecessary overhead. The user landonshumway-ia indicated that migration scripts can rely on data integrity when the records have been vetted and the script has been tested.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
📚 Learning: 2025-07-21T20:39:47.625Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 907
File: backend/compact-connect/lambdas/python/cognito-backup/handlers/cognito_backup.py:148-149
Timestamp: 2025-07-21T20:39:47.625Z
Learning: In the CompactConnect Cognito backup system, UserCreateDate and UserLastModifiedDate fields from the AWS Cognito ListUsers API are accessed using direct dictionary access (user_data['UserCreateDate']) rather than .get() method. This is intentional per landonshumway-ia's decision to fail fast if AWS breaks their documented API contract that guarantees these fields are always present. The KeyError that would result from missing fields should trigger alerts to detect API contract violations rather than being silently handled.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.pybackend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-21T15:23:26.019Z
Learnt from: rmolinares
Repo: csg-org/CompactConnect PR: 1025
File: webroot/src/models/Licensee/Licensee.model.ts:242-254
Timestamp: 2025-08-21T15:23:26.019Z
Learning: In the CompactConnect codebase, the LicenseSerializer.fromServer() method maps the server response field `dateOfIssuance` to the License model's `issueDate` property. After serialization, License instances only contain `issueDate` and never have `dateOfIssuance`. When reviewing date comparisons in reducers that work with License objects, check if they're working with serialized License instances (which use `issueDate`) rather than raw server data (which uses `dateOfIssuance`).
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.pybackend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-10-15T19:56:58.050Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py:60-0
Timestamp: 2025-10-15T19:56:58.050Z
Learning: In the CompactConnect codebase, when migrating from a single field to a list field (e.g., clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories), both fields are intentionally kept as optional during the migration period to support backwards compatibility. Mutual exclusivity validation is not enforced during this phase, as the deprecated field will be removed in a follow-up PR with migration scripts.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
📚 Learning: 2025-08-21T19:57:13.276Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:0-0
Timestamp: 2025-08-21T19:57:13.276Z
Learning: In the CompactConnect codebase, ProviderData.recoveryExpiry property returns `datetime | None` after the type hint fix. The marshmallow DateTime field in the record schema handles conversion between ISO strings (database storage) and datetime objects (application use), so test assertions can directly compare datetime objects.
Applied to files:
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-11-13T22:13:48.407Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1188
File: backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py:354-372
Timestamp: 2025-11-13T22:13:48.407Z
Learning: In the CompactConnect codebase, privilege records use datetime fields (not date fields) for dateOfIssuance, dateOfRenewal, and dateOfExpiration, which is intentionally different from license records that use date fields for these same attributes.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-29T21:45:05.792Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1016
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2702-2710
Timestamp: 2025-08-29T21:45:05.792Z
Learning: In the lift_home_jurisdiction_license_privilege_encumbrances method, when latest_effective_lift_date is None, the method always returns an empty list for affected_privileges, so the existing if affected_privileges guard in the calling code already prevents event publishing without needing explicit None checks for the date.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-25T22:31:44.837Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1005
File: backend/compact-connect/lambdas/python/common/cc_common/dsa_auth.py:212-228
Timestamp: 2025-08-25T22:31:44.837Z
Learning: In cc_common.dsa_auth._validate_dsa_signature, ensure naive datetimes are treated as UTC (set tzinfo=UTC) before comparing to now(UTC) to avoid aware-vs-naive TypeError.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-04-29T02:10:51.996Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.py:28-30
Timestamp: 2025-04-29T02:10:51.996Z
Learning: JSON decoding errors are handled in the api_handler wrapper function in this codebase, so individual handlers don't need to explicitly catch json.JSONDecodeError.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
🧬 Code graph analysis (2)
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py (7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (1)
LicenseUpdateData(148-201)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
PrivilegeUpdateData(95-166)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
ProviderRecordType(29-42)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
ProviderUpdateData(151-192)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCInternalException(44-45)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
provider_table(88-89)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
from_database_record(124-139)serialize_to_database_record(205-208)
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (12)
ProviderUserRecords(412-931)get_all_license_update_records(782-793)get_license_records(524-531)get_all_provider_update_records(808-819)get_privileges_associated_with_license(503-522)get_update_records_for_privilege(842-861)get_specific_privilege_record(476-491)get_update_records_for_license(821-840)get_provider_record(81-86)get_provider_record(694-703)find_best_license_in_current_known_licenses(705-765)populate_provider_record(192-228)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (4)
UpdateCategory(296-312)to_dict(169-180)update(182-203)licenseTypeAbbreviation(156-167)backend/compact-connect/lambdas/python/common/cc_common/event_batch_writer.py (1)
EventBatchWriter(4-49)backend/compact-connect/lambdas/python/common/cc_common/config.py (7)
disaster_recovery_results_bucket_name(191-192)provider_table(88-89)data_client(42-45)provider_table_name(155-156)events_client(68-69)event_bus_client(328-331)s3_client(34-35)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(177-220)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (1)
LicenseRecordSchema(38-163)backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (2)
publish_license_revert_event(441-490)publish_privilege_revert_event(492-541)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: TestApp
- GitHub Check: TestApp
🔇 Additional comments (5)
backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py (4)
13-30: LGTM! Clean custom resource handler structure.The class correctly implements a one-time migration pattern with on_create triggering the migration and no-ops for update/delete events.
33-82: LGTM! Migration logic is well-structured.The scan pagination, batch size calculation (50 records × 2 operations = 100 items, under DynamoDB's limit), and early exit are all correct. Loading all records into memory before batch processing is acceptable for reasonable dataset sizes in a one-time migration.
116-159: LGTM! Clean migration approach with proper data preservation.The deserialize/re-serialize pattern correctly generates new sort keys via schema logic. Preserving the original
dateOfUpdatefield (line 145) is critical for maintaining historical accuracy and is correctly implemented.
162-187: LGTM! Appropriate error handling for a migration script.The broad exception catch (line 173) with detailed logging allows the migration to continue past individual record failures while preserving atomic batch semantics. This is appropriate for a migration processing pre-vetted data. Based on learnings
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (1)
525-548: Current DynamoDB TransactWriteItems limit is 100, not 25—review comment is based on outdated informationThe current documented maximum for AWS DynamoDB TransactWriteItems is 100 items in a single request, and this limit was increased to 100 on September 6, 2022. The code's batch size of 100 is already compliant with the current documented limit and does not risk the runtime failures described in the review comment. The recommendation to reduce the batch size to 25 is based on the historical limit and should not be implemented.
Likely an incorrect or invalid review comment.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (1)
247-257: Consider adding security warning about SK sensitivity for consistency.The SK generation correctly implements the tier-2 update pattern and uses the UpdateTierEnum constant (addressing the past review comment). However, the license update schema includes a detailed security note about treating the SK as sensitive since it contains a hash of record values. Provider updates also contain sensitive PII (SSN, DOB, names), so consider adding a similar warning here for consistency.
Apply this diff to add the security warning:
@post_dump # Must be _post_ dump so we have values that are more easily hashed def generate_pk_sk(self, in_data, **kwargs): # noqa: ARG001 unused-argument + """ + NOTE: Because the 'sk' field in this record type contains a hash that is generated based on the values of the + record itself and because, in some cases, the values could be guessed and verified by the hash with relative + ease, regardless of the strength of the hash, we need to treat the 'sk' field as if it were just as sensitive as + the most sensitive field in the record. More to the point, we need to be sure that this internal field is never + served out via API. + """ in_data['pk'] = f'{in_data["compact"]}#PROVIDER#{in_data["providerId"]}'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py(6 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-18T19:24:36.640Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1012
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py:187-187
Timestamp: 2025-08-18T19:24:36.640Z
Learning: In the CompactConnect codebase, when migrating effectiveDate fields from date to datetime, a migration script is used to update existing records rather than adding backward compatibility validation at the schema load level.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-08-21T19:57:13.276Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:0-0
Timestamp: 2025-08-21T19:57:13.276Z
Learning: In the CompactConnect codebase, ProviderData.recoveryExpiry property returns `datetime | None` after the type hint fix. The marshmallow DateTime field in the record schema handles conversion between ISO strings (database storage) and datetime objects (application use), so test assertions can directly compare datetime objects.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-09-03T22:27:56.621Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py:71-71
Timestamp: 2025-09-03T22:27:56.621Z
Learning: The privilegeJurisdictions field in provider schemas consistently uses load_default=set() pattern across multiple schema files in the CompactConnect codebase (provider/record.py and provider/api.py), and developers prioritize maintaining this consistency.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py
📚 Learning: 2025-04-29T21:09:04.842Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py:87-90
Timestamp: 2025-04-29T21:09:04.842Z
Learning: In the CCDataClass implementation, validation is performed during both the dump() and load() processes in Marshmallow, so calling dump() first still results in ValidationError if the provided data is invalid.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py
🧬 Code graph analysis (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-32)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (3)
createDate(179-180)updatedValues(187-188)removedValues(191-192)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (2)
generate_pk_sk(123-127)generate_pk_sk(237-254)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py (2)
generate_pk_sk(121-125)generate_pk_sk(235-245)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
hash_changes(423-444)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-32)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (1)
firstUploadDate(144-145)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
license_type_abbreviations(131-146)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
hash_changes(423-444)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (11)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (3)
29-29: LGTM! Import aligns with tiered update architecture.The UpdateTierEnum import is correctly used in the SK generation to implement the tier-2 classification for provider updates.
229-230: LGTM! Field addition aligns with the tiered update migration.The createDate field correctly tracks update record creation time and is consistent with the same field in license and privilege update schemas.
236-245: Backwards compatibility hook is sound — dateOfUpdate availability confirmed.The hook's assumption is safe.
dateOfUpdateis a required field inBaseRecordSchema(line 42:DateTime(required=True, allow_none=False)), so all records stored in the database must have it by schema contract. When loading old records during migration,dateOfUpdatewill always be present. The hook cleanly handles the transition: ifcreateDateis missing, it's populated from the requireddateOfUpdatefield. The TODO comment correctly marks this for removal after migration completes.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (8)
11-11: LGTM: Imports are correctly added for new functionality.The new imports support the tier-based update records and GSI field generation for upload date tracking.
Also applies to: 31-34
51-57: LGTM: New fields properly defined for backwards compatibility.The optional GSI fields and firstUploadDate are correctly structured to support existing records without these fields.
161-162: LGTM: GSI field cleanup correctly updated.The post_load hook properly removes the new license upload GSI fields, following the established pattern.
232-234: LGTM: GSI fields consistently defined.The optional GSI fields match the pattern used in LicenseRecordSchema and are correctly structured.
279-285: LGTM: Post-load cleanup correctly implemented.The post_load hook properly removes the license upload GSI fields after loading, maintaining consistency with the data model's approach of keeping GSI fields internal.
256-277: GSI field generation logic is correct.Verification confirms
LICENSE_UPLOAD_UPDATE_CATEGORIESis properly defined as a set containing three update categories:DEACTIVATION,RENEWAL, andLICENSE_UPLOAD_UPDATE_OTHER. The conditional generation of GSI fields on line 261 correctly uses this constant to identify upload-related updates.
135-153: Validation is confirmed to be in place—no changes needed.LicenseRecordSchema inherits from LicenseCommonSchema, which includes ValidatesLicenseTypeMixin. The
validate_license_typemethod validates thatlicenseTypeis one of the valid types for the compact during the load phase, before any serialization occurs. When the@pre_dumphook at line 135 runs, thelicenseTypehas already been validated and is guaranteed to exist inconfig.license_type_abbreviations. Additionally, bothconfig.license_types_for_compact()andconfig.license_type_abbreviationsare built from the same source (theLICENSE_TYPESenvironment variable), ensuring they remain synchronized.
246-254: LGTM: Tier-based SK generation correctly implemented.The updated SK generation properly incorporates
UpdateTierEnum.TIER_THREEandcreateDate, enabling the tiered query approach. Since this is a post_dump hook, createDate is already serialized to an ISO string by the DateTime field. The codebase usesconfig.current_standard_datetime(which returnsdatetime.now(tz=UTC)) to create all DateTime values, ensuring they are UTC-aware. When the DateTime field serializes these objects, they become ISO 8601 strings with the UTC timezone indicator (e.g.,2024-11-08T23:59:59+00:00), which sort correctly lexicographically in the SK.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md (1)
16-16: Improve phrasing for clarity.The sentence structure is awkward: "To do this, determine which Cognito app clien..." Consider rephrasing:
- To do this, determine which Cognito app client(s) the jurisdiction is using for the compact(s) and delete the appropriate app client(s) from the State Auth Cognito user pool. + To do this: determine which Cognito app client(s) the jurisdiction uses for the compact(s), then delete the appropriate app client(s) from the State Auth Cognito user pool.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T21:20:35.260Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1029
File: backend/compact-connect/docs/api-specification/latest-oas30.json:468-471
Timestamp: 2025-08-22T21:20:35.260Z
Learning: The file backend/compact-connect/docs/api-specification/latest-oas30.json is auto-generated by API Gateway and should not be modified inline. Any schema changes would need to be addressed at the source in the CDK/CloudFormation definitions.
Applied to files:
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
🪛 LanguageTool
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
[style] ~16-~16: Consider a more expressive alternative.
Context: ...ges until system has been recovered. To do this, determine which Cognito app clien...
(DO_ACHIEVE)
🪛 markdownlint-cli2 (0.18.1)
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
45-45: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (2)
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md (2)
39-39: Verify the Step Function name against the actual CDK construct.The Step Function name
LicenseUploadRollbackLicenseUploadRollbackStateMachineappears to have the prefix repeated. Confirm this matches the actual resource name generated by the CDK stack inbackend/compact-connect/stacks/disaster_recovery_stack/license_upload_rollback_step_function.py, as it may be a typo or unintended duplication in the construct ID.
20-34: Request verification of technical constraints and implementation details.Verify the following details against the actual Lambda/Step Function implementation:
- Is the 7-day time window constraint (line 34) actually enforced in code?
- Do the example JSON structures in the Results section (lines 87–157) match the actual
RollbackResultsmodel output?- Is the
licenseUploadDateGSIpartition/sort key format (lines 175–176) accurate?- Does the S3 bucket naming pattern
*disasterrecoveryrollbackresults*(line 74) match the actual CDK-provisioned bucket?
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
Outdated
Show resolved
Hide resolved
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md(1 hunks)backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py(1 hunks)backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py(1 hunks)
🧰 Additional context used
🧠 Learnings (35)
📓 Common learnings
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1001
File: backend/compact-connect/stacks/disaster_recovery_stack/restore_dynamo_db_table_step_function.py:186-190
Timestamp: 2025-08-12T20:52:05.861Z
Learning: AWS Step Functions DynamoDB service integration for RestoreTableToPointInTime accepts mixed casing for SSE parameters (e.g., SseSpecificationOverride, KmsMasterKeyId, SseType) in addition to the standard AWS API casing. This has been verified through AWS Step Functions editing tool generation and successful sandbox testing.
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.pybackend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-09-03T22:33:32.687Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.ts:51-53
Timestamp: 2025-09-03T22:33:32.687Z
Learning: For encumbrance and lifting_encumbrance events in the privilege system, createDate should always be present. If createDate is absent, it indicates a bug in the data pipeline that should be allowed to surface rather than being defensively handled.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-11-13T22:13:48.407Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1188
File: backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py:354-372
Timestamp: 2025-11-13T22:13:48.407Z
Learning: In the CompactConnect codebase, privilege records use datetime fields (not date fields) for dateOfIssuance, dateOfRenewal, and dateOfExpiration, which is intentionally different from license records that use date fields for these same attributes.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.pybackend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-07-21T20:39:47.625Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 907
File: backend/compact-connect/lambdas/python/cognito-backup/handlers/cognito_backup.py:148-149
Timestamp: 2025-07-21T20:39:47.625Z
Learning: In the CompactConnect Cognito backup system, UserCreateDate and UserLastModifiedDate fields from the AWS Cognito ListUsers API are accessed using direct dictionary access (user_data['UserCreateDate']) rather than .get() method. This is intentional per landonshumway-ia's decision to fail fast if AWS breaks their documented API contract that guarantees these fields are always present. The KeyError that would result from missing fields should trigger alerts to detect API contract violations rather than being silently handled.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-29T21:45:05.792Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1016
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2702-2710
Timestamp: 2025-08-29T21:45:05.792Z
Learning: In the lift_home_jurisdiction_license_privilege_encumbrances method, when latest_effective_lift_date is None, the method always returns an empty list for affected_privileges, so the existing if affected_privileges guard in the calling code already prevents event publishing without needing explicit None checks for the date.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-21T15:23:26.019Z
Learnt from: rmolinares
Repo: csg-org/CompactConnect PR: 1025
File: webroot/src/models/Licensee/Licensee.model.ts:242-254
Timestamp: 2025-08-21T15:23:26.019Z
Learning: In the CompactConnect codebase, the LicenseSerializer.fromServer() method maps the server response field `dateOfIssuance` to the License model's `issueDate` property. After serialization, License instances only contain `issueDate` and never have `dateOfIssuance`. When reviewing date comparisons in reducers that work with License objects, check if they're working with serialized License instances (which use `issueDate`) rather than raw server data (which uses `dateOfIssuance`).
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.pybackend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-08-25T22:31:44.837Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1005
File: backend/compact-connect/lambdas/python/common/cc_common/dsa_auth.py:212-228
Timestamp: 2025-08-25T22:31:44.837Z
Learning: In cc_common.dsa_auth._validate_dsa_signature, ensure naive datetimes are treated as UTC (set tzinfo=UTC) before comparing to now(UTC) to avoid aware-vs-naive TypeError.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.pybackend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-08-18T19:24:36.640Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1012
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py:187-187
Timestamp: 2025-08-18T19:24:36.640Z
Learning: In the CompactConnect codebase, when migrating effectiveDate fields from date to datetime, a migration script is used to update existing records rather than adding backward compatibility validation at the schema load level.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.pybackend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-04-29T02:10:51.996Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.py:28-30
Timestamp: 2025-04-29T02:10:51.996Z
Learning: JSON decoding errors are handled in the api_handler wrapper function in this codebase, so individual handlers don't need to explicitly catch json.JSONDecodeError.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-25T22:31:44.837Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1005
File: backend/compact-connect/lambdas/python/common/cc_common/dsa_auth.py:212-228
Timestamp: 2025-08-25T22:31:44.837Z
Learning: The repository targets Python 3.12; datetime.fromisoformat accepts ISO 8601 timestamps with a trailing 'Z', so no normalization is required for 'Z'.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-25T22:31:44.837Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1005
File: backend/compact-connect/lambdas/python/common/cc_common/dsa_auth.py:212-228
Timestamp: 2025-08-25T22:31:44.837Z
Learning: The datetime.fromisoformat() method in the CompactConnect DSA authentication module already supports both 'Z' and '+00:00' ISO 8601 timestamp formats, as verified by existing tests. The timezone-naive timestamp handling should be added as an additional safety measure.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.pybackend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-06-23T21:31:15.787Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 870
File: backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py:128-132
Timestamp: 2025-06-23T21:31:15.787Z
Learning: For smoke tests in the CompactConnect project, broad exception handling (like `except Exception as e:`) is acceptable since these are not runtime code and prioritize simplicity over specific exception handling.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-19T02:07:26.373Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:3037-3041
Timestamp: 2025-08-19T02:07:26.373Z
Learning: In the CompactConnect codebase, when provider records are expected to exist based on successful license record matching (such as in account recovery flows), missing provider records should raise CCInternalException rather than CCNotFoundException, as this indicates a data consistency issue that requires system alerts rather than treating it as a user error.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-19T02:09:33.033Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:3068-3071
Timestamp: 2025-08-19T02:09:33.033Z
Learning: In the CompactConnect account recovery flow, when clearing provider account recovery data after successful verification, if the provider record is unexpectedly missing (ConditionalCheckFailedException), it should raise CCInternalException rather than CCNotFoundException because the provider was confirmed to exist during the recovery token verification step, indicating a data consistency issue requiring system alerts.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.pybackend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-08-21T19:57:13.276Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:0-0
Timestamp: 2025-08-21T19:57:13.276Z
Learning: In the CompactConnect codebase, ProviderData.recoveryExpiry property returns `datetime | None` after the type hint fix. The marshmallow DateTime field in the record schema handles conversion between ISO strings (database storage) and datetime objects (application use), so test assertions can directly compare datetime objects.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.pybackend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is a property rather than a method, and should be patched in tests by directly providing a datetime value: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.pybackend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-04-29T01:59:51.222Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/stacks/api_stack/v1_api/api_model.py:397-401
Timestamp: 2025-04-29T01:59:51.222Z
Learning: In the CompactConnect project, validation constraints like enum values should be defined only in runtime code (Lambda) rather than duplicating them in CDK API schema definitions. This applies to fields like clinicalPrivilegeActionCategory and other similar enumerations.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-06-04T17:38:22.392Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-09-12T14:14:53.986Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1083
File: backend/compact-connect/bin/generate_mock_license_csv_upload_file.py:39-41
Timestamp: 2025-09-12T14:14:53.986Z
Learning: In backend/compact-connect/bin/generate_mock_license_csv_upload_file.py, the functions are intentionally designed for CLI-only usage and should not be called programmatically. The code is meant to fail fast if developers try to import and use these functions outside of the CLI context, rather than having defensive programming guards.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-08-22T15:49:43.755Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1016
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:194-224
Timestamp: 2025-08-22T15:49:43.755Z
Learning: The API Gateway handler in this CompactConnect codebase returns JSON-formatted responses even for non-200 status codes, so response.json() calls don't need try/catch protection in API client code.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-06-04T20:05:18.245Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/app/test_event_listener.py:18-27
Timestamp: 2025-06-04T20:05:18.245Z
Learning: In the CompactConnect test suite, the established pattern is to let file operations in test setup methods raise natural exceptions (FileNotFoundError, JSONDecodeError) rather than adding explicit error handling. This approach maintains consistency across tests and provides sufficient error information for debugging.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-08-13T20:28:33.191Z
Learnt from: rmolinares
Repo: csg-org/CompactConnect PR: 1011
File: webroot/src/models/Licensee/Licensee.model.ts:276-279
Timestamp: 2025-08-13T20:28:33.191Z
Learning: The two-year encumbrance wait period feature only applies to privileges, not licenses. The logic should only check if privilege encumbrances that are no longer active have lift dates within the past two years.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-08-12T22:46:20.850Z
Learnt from: rmolinares
Repo: csg-org/CompactConnect PR: 1009
File: webroot/src/pages/LicenseeProof/LicenseeProof.ts:89-93
Timestamp: 2025-08-12T22:46:20.850Z
Learning: In the CompactConnect codebase, active privileges are guaranteed to always have an activeFromDate value populated. This is a business rule/data invariant, so null-safe handling for activeFromDate is not necessary when working with active privileges.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-08-26T15:30:29.956Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1012
File: backend/compact-connect/docs/design/README.md:503-512
Timestamp: 2025-08-26T15:30:29.956Z
Learning: In CompactConnect, the use of UTC-4:00 for privilege timeline events (encumbrances and expirations) was an agreed-upon business decision, not a technical choice made by individual developers. This fixed offset approach should be respected as an intentional organizational requirement.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-09-03T23:06:45.015Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2645-2651
Timestamp: 2025-09-03T23:06:45.015Z
Learning: PrivilegeData extends CCDataClass which provides the licenseTypeAbbreviation property, so PrivilegeData instances have access to licenseTypeAbbreviation through inheritance.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-06-17T19:05:36.255Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-08-25T19:04:48.800Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:288-293
Timestamp: 2025-08-25T19:04:48.800Z
Learning: In the CompactConnect project, landonshumway-ia has already provided feedback on patch cleanup patterns in test setup methods for the account recovery tests, so similar suggestions about stopping patches in setUp/tearDown should not be repeated.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-10-15T19:56:58.050Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py:60-0
Timestamp: 2025-10-15T19:56:58.050Z
Learning: In the CompactConnect codebase, when migrating from a single field to a list field (e.g., clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories), both fields are intentionally kept as optional during the migration period to support backwards compatibility. Mutual exclusivity validation is not enforced during this phase, as the deprecated field will be removed in a follow-up PR with migration scripts.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-07-08T18:40:24.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 882
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py:287-359
Timestamp: 2025-07-08T18:40:24.408Z
Learning: In the CompactConnect codebase, landonshumway-ia prefers to avoid extraneous unit tests when existing test coverage is already sufficient to catch bugs. For the get_privilege_purchase_options method's live-jurisdiction filtering logic, the existing tests in the purchases test suite provide adequate coverage without needing additional edge case tests.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-08-21T15:23:30.649Z
Learnt from: rmolinares
Repo: csg-org/CompactConnect PR: 1025
File: webroot/src/models/Licensee/Licensee.model.ts:242-254
Timestamp: 2025-08-21T15:23:30.649Z
Learning: In the CompactConnect frontend codebase, the LicenseSerializer.fromServer() method maps server response field `dateOfIssuance` to the License model field `issueDate`. After serialization, License objects only contain `issueDate` and never `dateOfIssuance`, so reducer logic only needs to check for null `issueDate` values.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py
📚 Learning: 2025-08-12T19:15:19.275Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1001
File: backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_copy_records.py:103-140
Timestamp: 2025-08-12T19:15:19.275Z
Learning: In the disaster recovery module tests, exact assertions on pagination keys like copyLastEvaluatedKey are intentionally brittle to serve as change detection mechanisms. If these tests fail, it indicates that pagination logic, item ordering, or key serialization has changed and requires developer verification.
Applied to files:
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.pybackend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
📚 Learning: 2025-08-22T21:20:35.260Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1029
File: backend/compact-connect/docs/api-specification/latest-oas30.json:468-471
Timestamp: 2025-08-22T21:20:35.260Z
Learning: The file backend/compact-connect/docs/api-specification/latest-oas30.json is auto-generated by API Gateway and should not be modified inline. Any schema changes would need to be addressed at the source in the CDK/CloudFormation definitions.
Applied to files:
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
🧬 Code graph analysis (2)
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (9)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (11)
ProviderUserRecords(412-931)get_all_license_update_records(782-793)get_license_records(524-531)get_all_provider_update_records(808-819)get_privileges_associated_with_license(503-522)get_update_records_for_privilege(842-861)get_specific_privilege_record(476-491)get_update_records_for_license(821-840)get_provider_record(81-86)get_provider_record(694-703)populate_provider_record(192-228)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
UpdateCategory(296-312)to_dict(169-180)update(182-203)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (1)
LicenseRecordSchema(38-163)backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-32)backend/compact-connect/lambdas/python/common/cc_common/event_batch_writer.py (1)
EventBatchWriter(4-49)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (2)
CCInternalException(44-45)CCNotFoundException(32-33)backend/compact-connect/lambdas/python/common/cc_common/config.py (8)
disaster_recovery_results_bucket_name(191-192)license_upload_date_index_name(221-222)provider_table(88-89)provider_table_name(155-156)data_client(42-45)events_client(68-69)event_bus_client(328-331)s3_client(34-35)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(177-220)backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (2)
publish_license_revert_event(444-494)publish_privilege_revert_event(496-546)
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (2)
LicenseData(9-145)LicenseUpdateData(148-201)backend/compact-connect/tests/smoke/smoke_common.py (10)
SmokeTestFailureException(13-17)create_test_app_client(490-531)create_test_staff_user(107-134)delete_test_app_client(534-542)delete_test_staff_user(86-104)get_api_base_url(196-197)get_client_auth_headers(589-600)get_provider_user_records(254-281)get_staff_user_auth_headers(179-183)load_smoke_test_env(224-227)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(177-220)backend/compact-connect/tests/smoke/config.py (4)
load_smoke_test_env(100-103)state_api_base_url(20-21)provider_user_dynamodb_table(44-45)license_upload_rollback_step_function_arn(40-41)backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (1)
to_dict(104-155)
🪛 LanguageTool
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
[style] ~16-~16: Consider a more expressive alternative.
Context: ...ges until system has been recovered. To do this, determine which Cognito app clien...
(DO_ACHIEVE)
🪛 markdownlint-cli2 (0.18.1)
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
45-45: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (12)
backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py (1)
1-793: LGTM! Comprehensive end-to-end smoke test.This smoke test provides thorough coverage of the license upload rollback workflow:
- Batch license upload with proper delays and retry logic
- Provider and update record creation verification with polling
- Skip-condition testing (privilege and encumbrance updates)
- Step Functions execution orchestration and monitoring
- S3 results retrieval and validation
- Database state verification post-rollback
- Proper cleanup in all scenarios (success/failure paths)
The linear narrative structure aligns with project patterns for smoke tests, making it easy to follow the complete workflow. The resolved pagination and deduplication issues have been properly addressed.
Based on learnings
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (11)
1-36: LGTM!Imports are well-organized, the 7-day rollback window safety limit is reasonable, and the custom exception class is appropriate for distinguishing provider-specific rollback failures from system-level errors.
38-209: LGTM!Data classes are well-designed with clear responsibilities. The serialization/deserialization logic in
RollbackResults.to_dict()andfrom_dict()correctly handles nested structures for S3 persistence. Past issues with revision IDs have been resolved.
212-406: LGTM!The main handler is well-structured with thorough input validation, appropriate time window constraints, and robust continuation logic for long-running executions. The 12-minute execution limit with graceful IN_PROGRESS returns ensures the Lambda can handle large provider sets without timing out.
408-467: LGTM!The GSI query logic correctly handles multi-month time windows with proper year/month incrementation and pagination. The SK range query with the trailing
#~ensures complete coverage of the timestamp range, and returning a set appropriately deduplicates provider IDs found across multiple months.
469-513: LGTM!Clean orchestration layer that appropriately handles the custom
ProviderRollbackFailedExceptionto convert provider-specific failures into structuredProviderFailedDetailsresults, allowing the workflow to continue processing other providers.
515-555: LGTM!Transaction batching correctly respects DynamoDB's 100-item limit, and the error handling provides excellent debugging information by logging all failed SKs from the batch while converting ClientErrors to
ProviderRollbackFailedExceptionfor proper workflow continuation.
557-600: LGTM!The orphaned update check is a critical data consistency validation that prevents rollback when the data is in an inconsistent state. The clear error message guides operators toward manual review when needed.
602-976: LGTM!This is an impressive and comprehensive implementation of the rollback logic. All past review issues have been addressed:
- Provider and privilege update records now correctly use
createDate- Empty
license_updates_in_windowis properly guarded- Missing transaction items correctly raises
CCInternalExceptionThe two-phase transaction approach (licenses/privileges first, then provider record update) is appropriate given the dependency on
find_best_license_in_current_known_licenses()requiring the rolled-back state.
978-1054: LGTM!Event publishing is appropriately treated as non-critical with comprehensive error logging. The
EventBatchWritercontext manager ensures efficient batch delivery while allowing the rollback workflow to complete even if event publishing encounters issues.
1056-1087: LGTM!S3 helper functions are well-implemented. The past issue about catching
JSONDecodeErrorinstead ofTypeErrorhas been correctly addressed, and the error handling appropriately distinguishes between JSON serialization errors and other S3 operation failures.
1-1087: Excellent implementation of a complex disaster recovery feature.This rollback handler demonstrates careful attention to:
- Data consistency validation (orphaned update checks)
- Eligibility verification (distinguishing upload-related vs. other updates)
- Transaction safety (batching, two-phase provider updates)
- Operational resilience (continuation logic, non-fatal event publishing)
- Debugging support (extensive structured logging, S3-persisted results)
All previously raised issues have been properly addressed. The complexity is warranted given the critical nature of disaster recovery operations.
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (1)
445-547: Unresolved: Change provider_id type to UUID and adopt load→dump validation pattern.The typing inconsistency and validation pattern remain unaddressed from the previous review:
Type mismatch: Both
provider_idparameters are typed asstr, butLicenseRevertDetailSchemaandPrivilegeRevertDetailSchemarequireproviderId = UUID(required=True). Other similar methods in this file (e.g.,publish_license_encumbrance_eventat line 180) useprovider_id: UUID.Validation pattern: The current
dump()→validate()approach (lines 485-488, 537-540) is redundant, asdump()already validates. The idiomatic pattern used bypublish_privilege_purchase_event(lines 112-113),publish_privilege_issued_event(lines 139-140), andpublish_privilege_renewed_event(lines 166-167) isload()→dump():def publish_license_revert_event( self, source: str, compact: str, - provider_id: str, + provider_id: UUID, jurisdiction: str, license_type: str, rollback_reason: str, start_time: datetime, end_time: datetime, execution_name: str, event_batch_writer: EventBatchWriter | None = None, ): ... license_revert_detail_schema = LicenseRevertDetailSchema() - deserialized_detail = license_revert_detail_schema.dump(event_detail) - validation_errors = license_revert_detail_schema.validate(deserialized_detail) - if validation_errors: - raise ValidationError(message=validation_errors) + loaded_detail = license_revert_detail_schema.load(event_detail) + deserialized_detail = license_revert_detail_schema.dump(loaded_detail) self._publish_event(...)Apply the same changes to
publish_privilege_revert_event(lines 497-547).
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (1)
236-245: Consider adding defensive check fordateOfUpdateexistence.The backwards compatibility hook is a sound migration approach. However, for defensive coding, consider checking if
dateOfUpdateexists before accessing it:@pre_load def populate_create_date_for_backwards_compatibility(self, in_data, **kwargs): """ For backwards compatibility, populate createDate from dateOfUpdate if createDate is missing. This allows us to load old records that were created before the createDate field was added. """ if 'createDate' not in in_data: - in_data['createDate'] = in_data['dateOfUpdate'] + in_data['createDate'] = in_data.get('dateOfUpdate', in_data.get('createDate')) return in_dataAlternatively, if
dateOfUpdateis guaranteed to exist (required base field), the current implementation is acceptable.Also, the TODO comment would benefit from a tracking issue reference or timeline for removal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py(6 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py(2 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1001
File: backend/compact-connect/stacks/disaster_recovery_stack/restore_dynamo_db_table_step_function.py:186-190
Timestamp: 2025-08-12T20:52:05.861Z
Learning: AWS Step Functions DynamoDB service integration for RestoreTableToPointInTime accepts mixed casing for SSE parameters (e.g., SseSpecificationOverride, KmsMasterKeyId, SseType) in addition to the standard AWS API casing. This has been verified through AWS Step Functions editing tool generation and successful sandbox testing.
📚 Learning: 2025-08-18T19:24:36.640Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1012
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py:187-187
Timestamp: 2025-08-18T19:24:36.640Z
Learning: In the CompactConnect codebase, when migrating effectiveDate fields from date to datetime, a migration script is used to update existing records rather than adding backward compatibility validation at the schema load level.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-08-21T19:57:13.276Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:0-0
Timestamp: 2025-08-21T19:57:13.276Z
Learning: In the CompactConnect codebase, ProviderData.recoveryExpiry property returns `datetime | None` after the type hint fix. The marshmallow DateTime field in the record schema handles conversion between ISO strings (database storage) and datetime objects (application use), so test assertions can directly compare datetime objects.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-09-03T22:27:56.621Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py:71-71
Timestamp: 2025-09-03T22:27:56.621Z
Learning: The privilegeJurisdictions field in provider schemas consistently uses load_default=set() pattern across multiple schema files in the CompactConnect codebase (provider/record.py and provider/api.py), and developers prioritize maintaining this consistency.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-09-03T22:29:24.535Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py:0-0
Timestamp: 2025-09-03T22:29:24.535Z
Learning: The EncumbranceEventDetailSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py is used across multiple instances/contexts where adverseActionCategory and adverseActionId fields may be required in some cases and not present in others. This is an intentional design pattern for handling different event variants with a single schema.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
📚 Learning: 2025-08-21T20:24:39.042Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py:0-0
Timestamp: 2025-08-21T20:24:39.042Z
Learning: When validating identifier fields like providerId in Marshmallow schemas, prefer using the UUID field type over String with Length validation, as UUID provides automatic format validation and is more semantically appropriate.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
📚 Learning: 2025-04-29T21:09:04.842Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py:87-90
Timestamp: 2025-04-29T21:09:04.842Z
Learning: In the CCDataClass implementation, validation is performed during both the dump() and load() processes in Marshmallow, so calling dump() first still results in ValidationError if the provided data is invalid.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
📚 Learning: 2025-11-13T22:13:48.407Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1188
File: backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py:354-372
Timestamp: 2025-11-13T22:13:48.407Z
Learning: In the CompactConnect codebase, privilege records use datetime fields (not date fields) for dateOfIssuance, dateOfRenewal, and dateOfExpiration, which is intentionally different from license records that use date fields for these same attributes.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py
🧬 Code graph analysis (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-32)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (3)
createDate(179-180)updatedValues(187-188)removedValues(191-192)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (2)
generate_pk_sk(124-128)generate_pk_sk(238-255)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py (2)
generate_pk_sk(122-126)generate_pk_sk(236-246)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
hash_changes(423-444)
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (2)
LicenseRevertDetailSchema(71-77)PrivilegeRevertDetailSchema(80-86)backend/compact-connect/lambdas/python/common/cc_common/event_batch_writer.py (1)
EventBatchWriter(4-49)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
current_standard_datetime(261-265)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-32)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (1)
firstUploadDate(144-145)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
license_type_abbreviations(131-146)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
hash_changes(423-444)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-32)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
hash_changes(423-444)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
license_type_abbreviations(131-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (3)
29-29: LGTM! UpdateTierEnum import addresses previous feedback.Good addition of the UpdateTierEnum import. This directly addresses the past review comment about using the update tiers enum in the post_dump hooks.
229-230: LGTM! Clear field addition for tracking update record creation time.The
createDatefield is well-defined and clearly documented. The distinction betweencreateDate(when the update record was created) anddateOfUpdate(when the base record was modified) is appropriate for tracking update history.
247-259: LGTM! Sort key generation properly implements tiered pattern.Excellent implementation of the tiered sort key pattern:
- Properly uses
UpdateTierEnum.TIER_TWOconstant- Consistent with license (TIER_THREE) and privilege (TIER_ONE) update patterns
- Well-documented rationale for tier 2 placement (balancing access frequency and query performance)
- ISO datetime format from
createDateis lexicographically sortable, ideal for chronological ordering in sort keys- Simpler format than license/privilege updates (no jurisdiction/licenseType) is appropriate since provider updates represent provider entity changes rather than license-specific modifications
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py (2)
30-30: LGTM: Import aligns with tier-based update pattern.The
UpdateTierEnumimport is properly used in the sort key construction on line 244, andTIER_ONEis the correct tier for privilege updates.
236-246: Code is correct and backward-compatible.The implementation properly handles both concerns raised:
DateTime serialization: Marshmallow's DateTime field automatically serializes to ISO 8601 format, which occurs before the
@post_dumphook executes. ThecreateDatewill be an ISO 8601 string by the time it's used in the sort key.Backward compatibility: The codebase already handles the sort key format transition:
- Query logic in
data_client.py(lines 1040-1042) explicitly supports both old ({compact}#PROVIDER#privilege/...#UPDATE) and new ({compact}#UPDATE#{tier}#privilege/...) patterns.- A migration tool exists to convert existing records to the new format.
- Tests confirm the new format works correctly with proper ISO 8601 datetime formatting and hash computation.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (2)
52-58: LGTM: License upload date GSI fields for LicenseRecordSchema.The addition of
licenseUploadDateGSIPK,licenseUploadDateGSISK, andfirstUploadDatefields with conditional GSI generation is correctly implemented:
- Optional fields maintain backward compatibility (as noted in the comment at lines 55-57)
- GSI generation only occurs when
firstUploadDateis present- Format matches requirements: PK uses
C#{compact}#J#{jurisdiction}#D#{YYYY-MM}, SK usesTIME#{epoch}#LT#{abbr}#PID#{providerId}- Hook timing and field lifecycle are correct (
pre_dumpfor generation,post_loadfor cleanup)Also applies to: 136-154, 162-163
234-236: LGTM: License upload date GSI and tiered SK for LicenseUpdateRecordSchema.The changes correctly implement:
- Optional GSI fields (
licenseUploadDateGSIPK,licenseUploadDateGSISK) generated conditionally for upload-related update types- Tiered sort key using
UpdateTierEnum.TIER_THREE(line 253) to enable range queries that can exclude license updates when needed- Embedding
createDatein the SK (line 253) as part of the migration to the new key pattern- Proper hook timing:
pre_dumpat line 257 ensurescreateDateis still a datetime object when calling.timestamp()The SK format change at line 253 is breaking for existing records, but per the PR objectives, a migration script handles the conversion.
Also applies to: 247-278, 280-286
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (1)
712-712: Remove unused variablereverted_licenses_dict.The list
reverted_licenses_dictis initialized at line 712 and populated at line 883, but it's never referenced or used anywhere in the function after that point. This dead code should be removed to improve code clarity.Apply this diff to remove the unused code:
- reverted_licenses_dict = [] - for license_record in license_records:add_put(serialized_reverted_license, update_record=False) logger.info('Reverting license record to pre-upload state') - # Track for provider record regeneration - license_schema = LicenseRecordSchema() - reverted_licenses_dict.append(license_schema.load(serialized_reverted_license)) - reverted_licenses.append( RevertedLicense(Also applies to: 883-883
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py(1 hunks)
🧰 Additional context used
🧠 Learnings (18)
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-09-03T22:33:32.687Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: webroot/src/components/PrivilegeEventNode/PrivilegeEventNode.ts:51-53
Timestamp: 2025-09-03T22:33:32.687Z
Learning: For encumbrance and lifting_encumbrance events in the privilege system, createDate should always be present. If createDate is absent, it indicates a bug in the data pipeline that should be allowed to surface rather than being defensively handled.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-11-13T22:13:48.407Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1188
File: backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py:354-372
Timestamp: 2025-11-13T22:13:48.407Z
Learning: In the CompactConnect codebase, privilege records use datetime fields (not date fields) for dateOfIssuance, dateOfRenewal, and dateOfExpiration, which is intentionally different from license records that use date fields for these same attributes.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-07-21T20:39:47.625Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 907
File: backend/compact-connect/lambdas/python/cognito-backup/handlers/cognito_backup.py:148-149
Timestamp: 2025-07-21T20:39:47.625Z
Learning: In the CompactConnect Cognito backup system, UserCreateDate and UserLastModifiedDate fields from the AWS Cognito ListUsers API are accessed using direct dictionary access (user_data['UserCreateDate']) rather than .get() method. This is intentional per landonshumway-ia's decision to fail fast if AWS breaks their documented API contract that guarantees these fields are always present. The KeyError that would result from missing fields should trigger alerts to detect API contract violations rather than being silently handled.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-29T21:45:05.792Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1016
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2702-2710
Timestamp: 2025-08-29T21:45:05.792Z
Learning: In the lift_home_jurisdiction_license_privilege_encumbrances method, when latest_effective_lift_date is None, the method always returns an empty list for affected_privileges, so the existing if affected_privileges guard in the calling code already prevents event publishing without needing explicit None checks for the date.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-21T15:23:26.019Z
Learnt from: rmolinares
Repo: csg-org/CompactConnect PR: 1025
File: webroot/src/models/Licensee/Licensee.model.ts:242-254
Timestamp: 2025-08-21T15:23:26.019Z
Learning: In the CompactConnect codebase, the LicenseSerializer.fromServer() method maps the server response field `dateOfIssuance` to the License model's `issueDate` property. After serialization, License instances only contain `issueDate` and never have `dateOfIssuance`. When reviewing date comparisons in reducers that work with License objects, check if they're working with serialized License instances (which use `issueDate`) rather than raw server data (which uses `dateOfIssuance`).
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-25T22:31:44.837Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1005
File: backend/compact-connect/lambdas/python/common/cc_common/dsa_auth.py:212-228
Timestamp: 2025-08-25T22:31:44.837Z
Learning: In cc_common.dsa_auth._validate_dsa_signature, ensure naive datetimes are treated as UTC (set tzinfo=UTC) before comparing to now(UTC) to avoid aware-vs-naive TypeError.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-18T19:24:36.640Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1012
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py:187-187
Timestamp: 2025-08-18T19:24:36.640Z
Learning: In the CompactConnect codebase, when migrating effectiveDate fields from date to datetime, a migration script is used to update existing records rather than adding backward compatibility validation at the schema load level.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-04-29T02:10:51.996Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.py:28-30
Timestamp: 2025-04-29T02:10:51.996Z
Learning: JSON decoding errors are handled in the api_handler wrapper function in this codebase, so individual handlers don't need to explicitly catch json.JSONDecodeError.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-25T22:31:44.837Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1005
File: backend/compact-connect/lambdas/python/common/cc_common/dsa_auth.py:212-228
Timestamp: 2025-08-25T22:31:44.837Z
Learning: The repository targets Python 3.12; datetime.fromisoformat accepts ISO 8601 timestamps with a trailing 'Z', so no normalization is required for 'Z'.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-25T22:31:44.837Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1005
File: backend/compact-connect/lambdas/python/common/cc_common/dsa_auth.py:212-228
Timestamp: 2025-08-25T22:31:44.837Z
Learning: The datetime.fromisoformat() method in the CompactConnect DSA authentication module already supports both 'Z' and '+00:00' ISO 8601 timestamp formats, as verified by existing tests. The timezone-naive timestamp handling should be added as an additional safety measure.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-06-23T21:31:15.787Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 870
File: backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py:128-132
Timestamp: 2025-06-23T21:31:15.787Z
Learning: For smoke tests in the CompactConnect project, broad exception handling (like `except Exception as e:`) is acceptable since these are not runtime code and prioritize simplicity over specific exception handling.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-19T02:07:26.373Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:3037-3041
Timestamp: 2025-08-19T02:07:26.373Z
Learning: In the CompactConnect codebase, when provider records are expected to exist based on successful license record matching (such as in account recovery flows), missing provider records should raise CCInternalException rather than CCNotFoundException, as this indicates a data consistency issue that requires system alerts rather than treating it as a user error.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-19T02:09:33.033Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:3068-3071
Timestamp: 2025-08-19T02:09:33.033Z
Learning: In the CompactConnect account recovery flow, when clearing provider account recovery data after successful verification, if the provider record is unexpectedly missing (ConditionalCheckFailedException), it should raise CCInternalException rather than CCNotFoundException because the provider was confirmed to exist during the recovery token verification step, indicating a data consistency issue requiring system alerts.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-08-21T19:57:13.276Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:0-0
Timestamp: 2025-08-21T19:57:13.276Z
Learning: In the CompactConnect codebase, ProviderData.recoveryExpiry property returns `datetime | None` after the type hint fix. The marshmallow DateTime field in the record schema handles conversion between ISO strings (database storage) and datetime objects (application use), so test assertions can directly compare datetime objects.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is a property rather than a method, and should be patched in tests by directly providing a datetime value: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
📚 Learning: 2025-04-29T01:59:51.222Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/stacks/api_stack/v1_api/api_model.py:397-401
Timestamp: 2025-04-29T01:59:51.222Z
Learning: In the CompactConnect project, validation constraints like enum values should be defined only in runtime code (Lambda) rather than duplicating them in CDK API schema definitions. This applies to fields like clinicalPrivilegeActionCategory and other similar enumerations.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (10)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (10)
ProviderUserRecords(412-931)get_all_license_update_records(782-793)get_license_records(524-531)get_all_provider_update_records(808-819)get_privileges_associated_with_license(503-522)get_update_records_for_privilege(842-861)get_specific_privilege_record(476-491)get_update_records_for_license(821-840)get_provider_record(81-86)get_provider_record(694-703)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (6)
UpdateCategory(296-312)to_dict(169-180)update(182-203)licenseTypeAbbreviation(156-167)serialize_to_database_record(205-208)create_new(96-121)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (2)
LicenseData(9-145)firstUploadDate(144-145)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (1)
LicenseRecordSchema(39-164)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
PrivilegeData(13-92)backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-32)backend/compact-connect/lambdas/python/common/cc_common/event_batch_writer.py (1)
EventBatchWriter(4-49)backend/compact-connect/lambdas/python/common/cc_common/config.py (8)
disaster_recovery_results_bucket_name(191-192)license_upload_date_index_name(221-222)provider_table(88-89)provider_table_name(155-156)data_client(42-45)events_client(68-69)event_bus_client(328-331)s3_client(34-35)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(177-220)backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (2)
publish_license_revert_event(445-495)publish_privilege_revert_event(497-547)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
This feature has been deployed for several weeks without issue. This removes the associated feature flag
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py (1)
66-77: LGTM! SSN duplication check is now unconditional.The logic correctly detects duplicate SSNs by comparing the set size to the list size. The error message is clear and the exception type is appropriate for API responses.
Optional enhancement: Consider providing more detail about which specific records contain duplicate SSNs (similar to the bulk_upload.py implementation on lines 156-163) to help users identify and fix the issue more quickly.
Example enhancement:
# verify that none of the SSNs are repeats within the same batch license_ssns = [license_record['ssn'] for license_record in licenses] if len(set(license_ssns)) < len(license_ssns): + # Find duplicate SSN details for better error message + seen = {} + duplicate_indices = [] + for idx, ssn in enumerate(license_ssns): + if ssn in seen: + duplicate_indices.append((idx, seen[ssn])) + else: + seen[ssn] = idx raise CCInvalidRequestCustomResponseException( response_body={ 'message': 'Invalid license records in request. See errors for more detail.', 'errors': { - 'SSN': 'Same SSN detected on multiple rows. ' - 'Every record must have a unique SSN within the same request.' + 'SSN': f'Duplicate SSNs detected at record indices: {duplicate_indices}. ' + 'Every record must have a unique SSN within the same request.' }, } )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_enum.py(0 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_bulk_upload.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_licenses.py(1 hunks)backend/compact-connect/stacks/feature_flag_stack/__init__.py(0 hunks)
💤 Files with no reviewable changes (2)
- backend/compact-connect/stacks/feature_flag_stack/init.py
- backend/compact-connect/lambdas/python/common/cc_common/feature_flag_enum.py
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-12T14:16:08.280Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1083
File: backend/compact-connect/bin/generate_mock_license_csv_upload_file.py:71-73
Timestamp: 2025-09-12T14:16:08.280Z
Learning: In backend/compact-connect/bin/generate_mock_license_csv_upload_file.py, the function parameters ssn_prefix and all_active intentionally do not have default values because the script ensures values are always passed from the top-level CLI argument parsing. Since this is a CLI-only script, defaults are not needed for the internal functions.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is implemented as a property that returns `datetime.now(tz=UTC).replace(microsecond=0)`. The correct way to patch it in tests is by directly providing the datetime value, not wrapping it in a lambda: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_licenses.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is a property (not a method), and can be properly patched in tests by directly supplying a datetime value rather than wrapping it in a lambda: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_licenses.py
📚 Learning: 2025-09-12T14:14:53.986Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1083
File: backend/compact-connect/bin/generate_mock_license_csv_upload_file.py:39-41
Timestamp: 2025-09-12T14:14:53.986Z
Learning: In backend/compact-connect/bin/generate_mock_license_csv_upload_file.py, the functions are intentionally designed for CLI-only usage and should not be called programmatically. The code is meant to fail fast if developers try to import and use these functions outside of the CLI context, rather than having defensive programming guards.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_licenses.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is a property rather than a method, and should be patched in tests by directly providing a datetime value: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_licenses.py
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py (2)
backend/compact-connect/lambdas/python/provider-data-v1/license_csv_reader.py (1)
licenses(12-20)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCInvalidRequestCustomResponseException(11-17)
🔇 Additional comments (5)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_bulk_upload.py (1)
3-3: LGTM! Import cleanup aligns with feature flag removal.The removal of
MagicMockfrom imports is consistent with removing the feature flag mocking infrastructure. The tests remain comprehensive and continue to verify SSN duplication detection (see test_bulk_upload_prevents_repeated_ssns_within_the_same_file_upload on lines 270-350).backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py (1)
156-163: LGTM! Duplicate SSN detection is correctly implemented.The incremental duplicate detection is well-designed:
- Uses 1-based indexing (
i + 1) which ensuresmatched_ssn_indexis never 0, making the truthy check on line 157 safe- Provides helpful error messages that identify which record number was duplicated
- Memory-efficient for large files (checks incrementally vs. collecting all SSNs first)
- Duplicate records are correctly excluded from the tracking dictionary after raising ValidationError
This approach is superior to the pattern in licenses.py (lines 66-77) which doesn't specify which records are duplicates in the error message.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_licenses.py (3)
13-13: LGTM!The
current_standard_datetimepatch is correctly implemented by directly providing the datetime value as the second argument, which aligns with the established pattern for patching this property.Based on learnings.
14-493: LGTM!The test suite is comprehensive and well-structured, covering multiple scenarios including:
- Successful license posting with queue message validation
- Request body validation and error handling
- Signature authentication (both with and without keys configured)
- Whitespace stripping from string fields
- Path parameter override protection
The tests properly verify both success and failure cases with appropriate assertions.
3-3: No issues found. The feature flag mock removal is safe and appropriate.The handler code contains no feature flag usage or
is_feature_enabledcalls. The removed feature flag mock was never needed, and its removal doesn't affect test coverage or handler behavior. The current test correctly patches onlycurrent_standard_datetime, which is actually used by the handler. This aligns with the principle of keeping test mocks lean and purposeful.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
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/common_test/test_data_generator.py (1)
118-131: Return type should belist[ProviderUpdateData]for consistency.This method returns
list[dict], while the similar methodsquery_privilege_update_records_for_given_record_from_databaseandquery_license_update_records_for_given_record_from_databasereturn typed data class lists (list[PrivilegeUpdateData]andlist[LicenseUpdateData]respectively). This inconsistency could lead to type confusion in tests.Apply this diff to align with the other query methods:
@staticmethod def query_provider_update_records_for_given_record_from_database(provider_record: ProviderData) -> list[dict]: """ Helper method to query update records from the database using the provider data class instance. All of our update records use the same pk as the actual record that is being updated. The sk of the actual record is the prefix for all the update records. Using this pattern, we can query for all of the update records that have been written for the given record. """ serialized_record = provider_record.serialize_to_database_record() sk_prefix = f'{provider_record.compact}#UPDATE#2#provider' - return TestDataGenerator._query_records_by_pk_and_sk_prefix(serialized_record['pk'], sk_prefix) + provider_update_records = TestDataGenerator._query_records_by_pk_and_sk_prefix( + serialized_record['pk'], sk_prefix + ) + + return [ProviderUpdateData.from_database_record(update_record) for update_record in provider_update_records]And update the return type annotation:
- def query_provider_update_records_for_given_record_from_database(provider_record: ProviderData) -> list[dict]: + def query_provider_update_records_for_given_record_from_database( + provider_record: ProviderData, + ) -> list[ProviderUpdateData]:
🧹 Nitpick comments (2)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py (1)
156-163: LGTM! Duplicate SSN validation is now correctly enforced.The logic is sound:
- The dictionary lookup with
.get()safely returnsNonefor new SSNs, and stored indices are 1-based (never 0), so the truthy check on line 157 works correctly.- The error message provides the 1-based record number of the first occurrence, consistent with validation error reporting elsewhere.
- The check executes after schema validation, ensuring only valid SSNs are tracked.
Removing the feature flag gating ensures this security/audit control is always active, aligning with the PR's disaster recovery objectives.
Optional minor refinement: Line 163 could use direct assignment for single key-value pairs:
- ssns_in_file_upload.update({license_ssn: i + 1}) + ssns_in_file_upload[license_ssn] = i + 1backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
332-344: LGTM! License update helper correctly implemented.The method follows the established pattern for storing update records in the provider table.
Optional enhancement: Consider adding
date_of_update_overrideparameter.For consistency with main record helpers (
put_default_license_record_in_provider_table,put_default_provider_record_in_provider_table) and the provider update equivalent, you might consider adding adate_of_update_overrideparameter to enable more flexible test scenarios. However, this can be added later if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py(1 hunks)backend/compact-connect/lambdas/python/common/common_test/test_constants.py(2 hunks)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py(5 hunks)backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py
- backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Applied to files:
backend/compact-connect/lambdas/python/common/common_test/test_constants.pybackend/compact-connect/lambdas/python/common/common_test/test_data_generator.py
📚 Learning: 2025-06-03T16:41:07.757Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 796
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:58-64
Timestamp: 2025-06-03T16:41:07.757Z
Learning: The static `get_provider_record` method in `ProviderRecordUtility` is legacy code being phased out in favor of the `ProviderUserRecords` class. During this migration, they are keeping the static method backwards compatible (returning `None`) while the new `ProviderUserRecords` class implements the better pattern of raising exceptions.
Applied to files:
backend/compact-connect/lambdas/python/common/common_test/test_constants.py
📚 Learning: 2025-09-03T22:27:56.621Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py:71-71
Timestamp: 2025-09-03T22:27:56.621Z
Learning: The privilegeJurisdictions field in provider schemas consistently uses load_default=set() pattern across multiple schema files in the CompactConnect codebase (provider/record.py and provider/api.py), and developers prioritize maintaining this consistency.
Applied to files:
backend/compact-connect/lambdas/python/common/common_test/test_constants.py
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py
📚 Learning: 2025-09-12T14:16:08.280Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1083
File: backend/compact-connect/bin/generate_mock_license_csv_upload_file.py:71-73
Timestamp: 2025-09-12T14:16:08.280Z
Learning: In backend/compact-connect/bin/generate_mock_license_csv_upload_file.py, the function parameters ssn_prefix and all_active intentionally do not have default values because the script ensures values are always passed from the top-level CLI argument parsing. Since this is a CLI-only script, defaults are not needed for the internal functions.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (4)
ProviderData(13-148)ProviderUpdateData(151-192)compact(29-30)compact(175-176)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
license_type_abbreviations(131-146)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (8)
compact(32-33)compact(172-173)licenseType(40-41)licenseType(180-181)jurisdiction(36-37)jurisdiction(176-177)LicenseData(9-145)LicenseUpdateData(148-201)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
serialize_to_database_record(205-208)from_database_record(124-139)update(182-203)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (6)
backend/compact-connect/lambdas/python/common/common_test/test_constants.py (1)
91-91: LGTM! Constants follow established patterns.The new
PROVIDER_UPDATE_RECORD_TYPEandDEFAULT_PROVIDER_UPDATE_TYPEconstants are consistent with existing update record types and align with the provider update functionality introduced in this PR.Also applies to: 111-112
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (5)
17-17: LGTM! Necessary import for provider update functionality.The
ProviderUpdateDataimport is required for the new provider update test helpers and follows the existing import pattern.
106-115: LGTM! Tier-based sort key pattern correctly implemented.The migration to tier-1 privilege update keys (
{compact}#UPDATE#1#privilege/{jurisdiction}/{license_type_abbr}/) aligns with the PR's sort-key migration objectives. The license type abbreviation lookup is correct, and the method properly returns typedPrivilegeUpdateDatainstances.
132-152: LGTM! License update query method properly implemented.The new method correctly implements tier-3 license update queries with appropriate type conversion to
LicenseUpdateData. The sort key pattern aligns with the PR's migration strategy.
499-528: LGTM! Provider update generator is well-implemented.The method properly constructs provider update data with appropriate defaults. The defensive handling of
dateOfUpdatein the previous provider dict (lines 508-510) ensures the update record has valid data even when the previous provider lacks this field.
531-549: LGTM! Provider update storage method correctly handles overrides.The method properly handles
date_of_update_overrideand recreates the data object from the database record (line 549) to ensure consistency between the stored and returned data. This is the correct approach when modifying serialized fields.
Collaborator
Author
|
@jlkravitz This is ready for your review. Thanks |
This reverts commit 4feef8a
Some licensees can have more than one license, so we need to check for duplicate SSNs in multiple records which have the same license type.
jlkravitz
reviewed
Nov 21, 2025
Collaborator
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
Mostly questions arising from my lack of knowledge, plus a few suggestions!
...pact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py
Show resolved
Hide resolved
...pact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py
Outdated
Show resolved
Hide resolved
...pact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py
Show resolved
Hide resolved
...pact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
backend/compact-connect/stacks/disaster_recovery_stack/license_upload_rollback_step_function.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py
Show resolved
Hide resolved
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md (2)
29-30: Align date examples across the document.The parameter table (lines 29-30) and JSON input example (lines 56-57) use
2020-01-15, but the Step 3 execution name example (line 47) and Step 5 folder path (line 76) reference2025-01-15. Use consistent dates throughout to avoid confusing operators.-| `startDateTime` | UTC timestamp when problematic uploads began (inclusive) | `"2020-01-15T08:00:00Z"` | -| `endDateTime` | UTC timestamp when problematic uploads ended (inclusive) | `"2020-01-15T17:59:59Z"` | +| `startDateTime` | UTC timestamp when problematic uploads began (inclusive) | `"2025-01-15T08:00:00Z"` | +| `endDateTime` | UTC timestamp when problematic uploads ended (inclusive) | `"2025-01-15T17:59:59Z"` | And in the JSON input example: ```diff "compact": "aslp", "jurisdiction": "oh", - "startDateTime": "2020-01-15T08:00:00Z", - "endDateTime": "2020-01-15T17:59:59Z", + "startDateTime": "2025-01-15T08:00:00Z", + "endDateTime": "2025-01-15T17:59:59Z",Also applies to: 56-57
108-109: Replace placeholder with concrete example.Line 108-109 contains placeholder text instead of a concrete example. Replace it with representative sort key strings following the documented update record format to help operators understand what they're looking at.
"updatesDeleted": [ - <sort keys of update records that were deleted> + "UT#2025-01-15T08:30:00Z#ASLP#01234567-89ab-cdef-0123-456789abcdef", + "UT#2025-01-15T14:15:00Z#ASLP#fedcba98-7654-3210-fedc-ba9876543210" ]
🧹 Nitpick comments (4)
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md (4)
29-30: Clarify whether time boundaries are inclusive or exclusive.Lines 29-30 state "inclusive" for both
startDateTimeandendDateTime, which is good. However, the implications should be made explicit: does the system include records timestamped exactly at these boundaries, and what precision (seconds vs. milliseconds) is used for matching? A brief note clarifying this operational detail would help operators avoid off-by-one confusion.-| `startDateTime` | UTC timestamp when problematic uploads began (inclusive) | `"2025-01-15T08:00:00Z"` | -| `endDateTime` | UTC timestamp when problematic uploads ended (inclusive) | `"2025-01-15T17:59:59Z"` | +| `startDateTime` | UTC timestamp when problematic uploads began (inclusive; includes records at this exact timestamp) | `"2025-01-15T08:00:00Z"` | +| `endDateTime` | UTC timestamp when problematic uploads ended (inclusive; includes records at this exact timestamp) | `"2025-01-15T17:59:59Z"` |
70-70: Provide explicit guidance for retrieving the S3 output key.The current text directs operators to "check the Step Function execution output/logs." To reduce ambiguity, explicitly specify where to find the S3 key: either in the final Step Function execution output JSON (accessible via AWS Console > Executions > select execution > Output tab) or extract it programmatically via AWS CLI (e.g.,
aws stepfunctions describe-execution --execution-arn <arn>).-Once the execution completes, comprehensive results are stored in S3. The S3 key is returned as output from the lambda step of the step function. Check the Step Function execution output/logs to get the S3 key. +Once the execution completes, comprehensive results are stored in S3. The S3 key is returned in the Step Function execution output: +- **Via AWS Console**: Step Functions → select execution → Output tab → look for the S3 key in the JSON output +- **Via AWS CLI**: `aws stepfunctions describe-execution --execution-arn <arn> --query 'output' --output text` and parse the returned JSON
183-186: Clarify the purpose and consumption of published events.The events section states these are "purely for auditing purposes" and "not currently referenced by any downstream processes," which raises the question: why publish them? Consider adding a line explaining the intended use case (e.g., audit trail, future integrations, compliance logging) so operators understand the operational relevance.
-These events purely for auditing purposes. They are not currently referenced by any downstream processes. +These events are emitted purely for auditing and compliance purposes to maintain a full record of all license reverts. They are not currently consumed by downstream processes but are available for integration with external audit systems or future compliance workflows.
16-16: Minor style suggestion: use more expressive language.Line 16 uses "until system has been recovered," which is somewhat passive. Consider a more direct phrasing.
-2. ✅ **Disable automated access for Jurisdiction**: If jurisdiction has API credentials for automated uploads, disable those credentials to prevent further data changes until system has been recovered. To do this, determine which Cognito app client(s) the jurisdiction is using for the compact(s) and delete the appropriate app client(s) from the State Auth Cognito user pool. +2. ✅ **Disable automated access for Jurisdiction**: If jurisdiction has API credentials for automated uploads, disable those credentials immediately to prevent further data ingestion while recovery is underway. To do this, determine which Cognito app client(s) the jurisdiction is using for the compact(s) and delete the appropriate app client(s) from the State Auth Cognito user pool.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py(1 hunks)backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
📚 Learning: 2025-08-22T22:02:41.865Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1029
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_licenses.py:40-73
Timestamp: 2025-08-22T22:02:41.865Z
Learning: The test framework in backend/compact-connect/lambdas/python uses self.addCleanup(self.delete_resources) in the base TstFunction/TstLambdas classes to automatically clean up all test resources (DynamoDB tables, SQS queues, S3 buckets, Cognito user pools) after each test. The delete_resources() method provides comprehensive cleanup that prevents cross-test interference, eliminating the need for additional tearDown logic.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py
📚 Learning: 2025-08-22T22:02:41.865Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1029
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_licenses.py:40-73
Timestamp: 2025-08-22T22:02:41.865Z
Learning: The TstLambdas base class in backend/compact-connect/lambdas/python/*/tests/function/__init__.py uses self.addCleanup(self.delete_resources) to automatically clean up test resources (including SQS queues) after each test, preventing cross-test interference. No additional tearDown logic is needed for queue cleanup.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py
📚 Learning: 2025-09-12T14:14:53.986Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1083
File: backend/compact-connect/bin/generate_mock_license_csv_upload_file.py:39-41
Timestamp: 2025-09-12T14:14:53.986Z
Learning: In backend/compact-connect/bin/generate_mock_license_csv_upload_file.py, the functions are intentionally designed for CLI-only usage and should not be called programmatically. The code is meant to fail fast if developers try to import and use these functions outside of the CLI context, rather than having defensive programming guards.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py
📚 Learning: 2025-06-17T19:05:36.255Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py
📚 Learning: 2025-08-25T19:04:48.800Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:288-293
Timestamp: 2025-08-25T19:04:48.800Z
Learning: In the CompactConnect project, landonshumway-ia has already provided feedback on patch cleanup patterns in test setup methods for the account recovery tests, so similar suggestions about stopping patches in setUp/tearDown should not be repeated.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py
📚 Learning: 2025-08-22T21:20:35.260Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1029
File: backend/compact-connect/docs/api-specification/latest-oas30.json:468-471
Timestamp: 2025-08-22T21:20:35.260Z
Learning: The file backend/compact-connect/docs/api-specification/latest-oas30.json is auto-generated by API Gateway and should not be modified inline. Any schema changes would need to be addressed at the source in the CDK/CloudFormation definitions.
Applied to files:
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
📚 Learning: 2025-08-12T19:15:19.275Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1001
File: backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_copy_records.py:103-140
Timestamp: 2025-08-12T19:15:19.275Z
Learning: In the disaster recovery module tests, exact assertions on pagination keys like copyLastEvaluatedKey are intentionally brittle to serve as change detection mechanisms. If these tests fail, it indicates that pagination logic, item ordering, or key serialization has changed and requires developer verification.
Applied to files:
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
📚 Learning: 2025-08-21T15:23:26.019Z
Learnt from: rmolinares
Repo: csg-org/CompactConnect PR: 1025
File: webroot/src/models/Licensee/Licensee.model.ts:242-254
Timestamp: 2025-08-21T15:23:26.019Z
Learning: In the CompactConnect codebase, the LicenseSerializer.fromServer() method maps the server response field `dateOfIssuance` to the License model's `issueDate` property. After serialization, License instances only contain `issueDate` and never have `dateOfIssuance`. When reviewing date comparisons in reducers that work with License objects, check if they're working with serialized License instances (which use `issueDate`) rather than raw server data (which uses `dateOfIssuance`).
Applied to files:
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Applied to files:
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py (8)
backend/compact-connect/lambdas/python/common/cc_common/data_model/update_tier_enum.py (1)
UpdateTierEnum(4-40)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCNotFoundException(32-33)backend/compact-connect/lambdas/python/disaster-recovery/tests/function/__init__.py (2)
TstFunction(15-129)setUp(18-37)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (6)
put_default_provider_record_in_provider_table(477-497)put_default_license_record_in_provider_table(292-302)put_default_license_update_record_in_provider_table(333-344)put_default_privilege_record_in_provider_table(378-388)put_default_privilege_update_record_in_provider_table(427-441)put_default_provider_update_record_in_provider_table(531-549)backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (2)
to_dict(104-155)rollback_license_upload(212-416)backend/compact-connect/lambdas/python/common/cc_common/config.py (5)
data_client(42-45)current_standard_datetime(261-265)s3_client(34-35)disaster_recovery_results_bucket_name(191-192)provider_table(88-89)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(177-220)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (7)
get_provider_record(81-86)get_provider_record(694-703)get_license_records(524-531)get_all_license_update_records(782-793)get_privilege_records(493-501)get_all_privilege_update_records(795-806)get_specific_license_record(459-474)
🪛 LanguageTool
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
[style] ~16-~16: Consider a more expressive alternative.
Context: ...ges until system has been recovered. To do this, determine which Cognito app clien...
(DO_ACHIEVE)
🪛 markdownlint-cli2 (0.18.1)
backend/compact-connect/disaster_recovery/LICENSE_UPLOAD_ROLLBACK.md
46-46: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (1)
backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py (1)
1-1531: Excellent comprehensive test coverage for the rollback functionality.This test module demonstrates thorough testing practices with well-structured scenarios covering:
- Core rollback logic: License/privilege reversions, deletions, and reactivations
- Edge cases: Orphaned updates, multiple updates, encumbrances within upload window
- Validation: Datetime format, time window ordering, maximum window enforcement
- Integration: S3 result persistence, EventBus publishing, pagination/continuation
- Error handling: Transaction failures, schema validation failures
The helper methods (
_when_provider_had_license_updated_from_upload,_when_license_was_updated_twice, etc.) effectively set up complex test scenarios while maintaining clarity. Test names are descriptive and follow a consistent pattern, making the test suite easy to navigate.The intentional use of hardcoded SHA values in assertions (lines 814, 887, 1288, 1302) serves as change detection for update record serialization, consistent with the project's disaster recovery testing approach.
jlkravitz
approved these changes
Nov 21, 2025
Collaborator
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
@isabeleliassen Good to merge!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
In the event that a state inadvertently uploads invalid data into the system for a compact, we need a way to effectively 'undo' that upload so that license data within the system can be reverted to its state before the upload. This PR includes the following enhancements to the system:
Move license/privilege update records under separate sort key to decouple updates from other main records
To reduce risk that massive invalid updates will cause the system to crash when loading provider data, we have migrated the sort keys of our update records to follow a tier based pattern, which will allow us to query for update records only as needed. This requires some more complexity in the query patterns used to grab provider records, but most of this complexity is abstracted by a small number of methods.
Add licenseUploadDate GSI
We've added a GSI to the provider table to track when uploads occurred for every license update record, and we will begin tracking the first time a license record was uploaded into the system. The PK for this new GSI includes the compact, jurisdiction, and upload month. The SK includes the epoch timestamp of the time the record was uploaded/updated, the license type, and the provider id.
These OPTIONAL GSI fields will be added to all license and license update records moving forward for every ingest event.
Add Step Function for reverting license uploads
We've added a step function that takes in the compact, jurisdiction, and time window (start date time and end date time).
The step function looks up all licenses that were uploaded for that compact/jurisdiction between the timestamps using the new GSI, and reverts the license record to the latest state prior to the rollback time if there were not other updates made to that provider since the upload time period. When completed it generates a JSON report stored in S3 which includes details about which licenses were reverted and which providers will require manual review due to detected updates not related to the license upload.
These enhancements will protect the system from accidental uploads and provide a process for reverting such incidents.
Requirements List
Closes #1175
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.