Feat/enhance encumbrance notifications#1179
Feat/enhance encumbrance notifications#1179isabeleliassen merged 18 commits intocsg-org:developmentfrom
Conversation
WalkthroughAdds persistent event-state tracking and idempotent notification delivery: new EventStateClient, NotificationTracker, sqs_handler_with_notification_tracking, config wiring and CDK EventState stack/table; encumbrance handlers updated to use trackers and propagate event metadata; tests, table setup, and an alarm for email failures added. Changes
Sequence Diagram(s)sequenceDiagram
participant SQS as SQS Batch
participant Handler as Encumbrance Handler
participant Tracker as NotificationTracker
participant ESC as EventStateClient
participant DDB as DynamoDB
participant Email as Email Service
SQS->>Handler: invoke(message, tracker) %% tracker may be None
alt tracker present
Handler->>Tracker: init(compact, message_id)
activate Tracker
Tracker->>ESC: _get_notification_attempts(compact, message_id)
ESC->>DDB: Query attempts
DDB-->>ESC: attempts
ESC-->>Tracker: attempts loaded
deactivate Tracker
end
Handler->>Tracker: should_send_provider_notification()?
alt already sent
Tracker-->>Handler: skip provider send
else send
Handler->>Email: send provider notification (provider_id,event_type,event_time)
Email-->>Handler: success/failure
Handler->>Tracker: record_success/record_failure(...)
Tracker->>ESC: record_notification_attempt(...)
ESC->>DDB: PutItem (ttl,status,pk,sk,...)
DDB-->>ESC: ack
end
note right of Handler: Additional state notifications follow same pattern per jurisdiction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus review on:
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
285-292: Cache the DynamoDB Table resource for consistency/perf.Align with other tables by using
@cached_propertyto avoid re-creating the Table client repeatedly.- @property - def event_state_table(self): - return boto3.resource('dynamodb').Table(self.event_state_table_name) + @cached_property + def event_state_table(self): + return boto3.resource('dynamodb').Table(self.event_state_table_name)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
35-41: Protocol addition LGTM; consider harmonizing return typing.Most send_* methods return
dict[str, Any]from_invoke_lambda. Align the protocol (and methods that saydict[str, str]) todict[str, Any]or a sharedInvokeResultalias for consistency.backend/compact-connect/stacks/event_state_stack/__init__.py (2)
17-27: Constructor signature clarityWorks as-is (environment_context flows via kwargs), but adding environment_context: dict to the signature improves readability and parity with other stacks. Optional.
28-36: Removal policy duplicationPolicy derived from environment_name matches PersistentStack logic. Consider accepting a removal_policy parameter (defaulting to the computed value) to ease future changes. Optional.
backend/compact-connect/lambdas/python/common/tests/function/__init__.py (1)
87-108: Optional: mirror production TTL attributeProduction table defines time_to_live_attribute='ttl'. Tests don’t need TTL enabled, but adding the attribute keeps schema parity and avoids surprises when asserting fields. Optional.
backend/compact-connect/lambdas/python/data-events/tests/function/__init__.py (1)
31-35: Setup/teardown completenessTable creation and deletion are symmetric here. Consider unifying naming with common test base (create_rate_limiting_table vs create_rate_limit_table) to reduce cognitive load, but optional.
Also applies to: 48-58, 59-80, 123-127
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (1)
3036-3151: Tighten assertions; avoid relying on dict insertion order and keep types consistent
- expected_sks is compared as a list. DynamoDB query ordering can change; compare sets or sort for stability.
- provider_id is passed as a str to tracker.record_*; elsewhere tests use UUID(DEFAULT_PROVIDER_ID). Use UUID(...) for consistency. Optional.
Apply within‑test patch:
- expected_sks = [ + expected_sks = [ 'ENCUMBRANCE_NOTIFICATION#provider#', 'ENCUMBRANCE_NOTIFICATION#state#ky', 'ENCUMBRANCE_NOTIFICATION#state#ne', 'ENCUMBRANCE_NOTIFICATION#state#oh', ] - - self.assertEqual(expected_sks, list(notification_records.keys())) + self.assertEqual(set(expected_sks), set(notification_records.keys()))And prefer:
- tracker.record_success( + tracker.record_success( recipient_type=RecipientType.PROVIDER, - provider_id=DEFAULT_PROVIDER_ID, + provider_id=UUID(DEFAULT_PROVIDER_ID), ... )backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
453-475: Include message_id in error log for correlationOn failure, include record['messageId'] in the error log to simplify tracing.
Apply this patch:
@@ def process_messages(event, context: LambdaContext): # noqa: ARG001 unused-argument - except Exception as e: # noqa: BLE001 broad-exception-caught - logger.error('Failed to process message', exc_info=e) + except Exception as e: # noqa: BLE001 broad-exception-caught + logger.error('Failed to process message', message_id=record.get('messageId'), exc_info=e) batch_failures.append({'itemIdentifier': record['messageId']})backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py (1)
45-46: Prefer Key('pk').eq(pk) for DynamoDB queriesUse boto3.dynamodb.conditions.Key for clarity and consistency with other tests.
Example patch for one occurrence:
-from boto3.dynamodb.conditions import Key -... -response = self._event_state_table.query( - KeyConditionExpression='pk = :pk', ExpressionAttributeValues={':pk': pk} -) +from boto3.dynamodb.conditions import Key +... +response = self._event_state_table.query( + KeyConditionExpression=Key('pk').eq(pk) +)Repeat similarly for the other query sites in this file.
Also applies to: 88-90, 125-126, 352-353, 384-385
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/compact-connect/lambdas/python/common/cc_common/config.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/utils.py(3 hunks)backend/compact-connect/lambdas/python/common/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/function/__init__.py(3 hunks)backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py(1 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(26 hunks)backend/compact-connect/lambdas/python/data-events/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/__init__.py(3 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(8 hunks)backend/compact-connect/pipeline/backend_stage.py(3 hunks)backend/compact-connect/stacks/event_state_stack/__init__.py(1 hunks)backend/compact-connect/stacks/event_state_stack/event_state_table.py(1 hunks)backend/compact-connect/stacks/notification_stack.py(5 hunks)backend/compact-connect/stacks/persistent_stack/__init__.py(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-22T22:02:41.865Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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/data-events/tests/function/__init__.py
📚 Learning: 2025-10-09T21:11:36.590Z
Learnt from: jusdino
PR: csg-org/CompactConnect#1143
File: backend/compact-connect/tests/app/base.py:514-530
Timestamp: 2025-10-09T21:11:36.590Z
Learning: In the BackendStage class within the CompactConnect backend infrastructure, the following stacks are always required and their absence indicates a serious error: api_lambda_stack, api_stack, disaster_recovery_stack, event_listener_stack, feature_flag_stack, ingest_stack, managed_login_stack, persistent_stack, provider_users_stack, state_api_stack, state_auth_stack, and transaction_monitoring_stack. Only backup_infrastructure_stack can be None (when backups are disabled). The reporting_stack and notification_stack are conditionally present based on hosted_zone configuration.
Applied to files:
backend/compact-connect/pipeline/backend_stage.py
📚 Learning: 2025-06-30T17:30:31.252Z
Learnt from: jusdino
PR: csg-org/CompactConnect#864
File: backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py:866-866
Timestamp: 2025-06-30T17:30:31.252Z
Learning: In the CompactConnect project, the `sqs_handler` decorator transforms Lambda functions to accept `(event, context)` parameters and automatically handles iteration over `event['Records']`, parsing JSON from each record's body, and managing SQS batch item failures. Functions decorated with `sqs_handler` have their underlying implementation accept a single `message: dict` parameter, but the decorator makes them callable with standard Lambda `(event, context)` signature. Tests should call these decorated functions with `(event, context)` parameters.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/utils.py
🧬 Code graph analysis (9)
backend/compact-connect/stacks/notification_stack.py (2)
backend/compact-connect/stacks/event_state_stack/__init__.py (1)
EventStateStack(9-36)backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
environment_name(100-101)event_state_table(290-291)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
EventStateClient(32-112)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (4)
compact(32-33)compact(164-165)jurisdiction(36-37)jurisdiction(168-169)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
compact(29-30)compact(175-176)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py (2)
compact(58-59)compact(108-109)
backend/compact-connect/stacks/event_state_stack/__init__.py (3)
backend/common-cdk/common_constructs/stack.py (1)
AppStack(92-142)backend/compact-connect/stacks/event_state_stack/event_state_table.py (1)
EventStateTable(14-48)backend/compact-connect/stacks/persistent_stack/__init__.py (1)
PersistentStack(37-492)
backend/compact-connect/pipeline/backend_stage.py (1)
backend/compact-connect/stacks/event_state_stack/__init__.py (1)
EventStateStack(9-36)
backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py (2)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (11)
EventStateClient(32-112)EventType(23-29)NotificationStatus(16-20)NotificationTracker(115-237)RecipientType(9-13)record_notification_attempt(38-94)_get_notification_attempts(96-112)should_send_provider_notification(132-139)record_success(151-192)record_failure(194-237)should_send_state_notification(141-149)backend/compact-connect/lambdas/python/common/tests/function/__init__.py (1)
TstFunction(20-378)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
event_state_table(290-291)event_state_client(294-297)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (4)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
event_state_client(294-297)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (6)
EventType(23-29)NotificationTracker(115-237)RecipientType(9-13)record_success(151-192)record_failure(194-237)NotificationStatus(16-20)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (1)
license_encumbrance_notification_listener(637-723)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (3)
put_default_provider_record_in_provider_table(409-429)put_default_license_record_in_provider_table(238-248)put_default_privilege_record_in_provider_table(310-320)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (4)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (5)
EncumbranceNotificationTemplateVariables(14-24)JurisdictionNotificationMethod(35-40)ProviderNotificationMethod(27-32)send_privilege_encumbrance_lifting_provider_notification_email(459-487)send_license_encumbrance_lifting_provider_notification_email(337-365)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (7)
EventType(23-29)NotificationTracker(115-237)RecipientType(9-13)should_send_provider_notification(132-139)record_success(151-192)record_failure(194-237)should_send_state_notification(141-149)backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
sqs_handler(480-489)sqs_handler_with_message_id(492-504)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
compactConnectRegisteredEmailAddress(77-83)
⏰ 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 (23)
backend/compact-connect/stacks/persistent_stack/__init__.py (1)
2-3: LGTM!The CloudWatch imports are properly added and used in the email notification alarm setup below.
backend/compact-connect/lambdas/python/common/tests/__init__.py (1)
21-21: Env var added for event-state table — looks good.Ensure your test harness creates/mocks this DynamoDB table (e.g., moto/localstack) before tests that touch
config.event_state_table, otherwise boto3 will error at runtime.backend/compact-connect/lambdas/python/data-events/tests/__init__.py (1)
23-23: Good: event-state env wired into data-events tests.As with common tests, make sure the test scaffolding provisions/mocks the table before exercising code paths that hit it.
backend/compact-connect/stacks/event_state_stack/event_state_table.py (1)
42-48: EnsureeventTimevalues are ISO‑8601 strings for correct GSI ordering.The STRING sort key will only sort chronologically if values are ISO‑8601 (e.g.,
2025-10-25T12:34:56Z). IfeventTimeisstr(datetime), ordering may be incorrect.If needed, normalize in the writer code (EventStateClient):
eventTime=event_time.astimezone(timezone.utc).isoformat(timespec='seconds').replace('+00:00','Z').backend/compact-connect/pipeline/backend_stage.py (1)
55-63: Event state wiring looks correctEventStateStack is created and passed into NotificationStack; cross‑stack references will enforce deployment ordering. LGTM.
Also applies to: 155-164
backend/compact-connect/stacks/notification_stack.py (2)
31-39: New dependency injection is appropriateAccepting event_state_stack and storing it local keeps concerns separated. LGTM.
Also applies to: 44-49
176-181: Correct permissions and environmentPassing EVENT_STATE_TABLE_NAME and granting read/write aligns with NotificationTracker’s usage. Good change.
Also applies to: 185-188, 189-202
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (2)
60-60: Dynamic messageId in SQS eventGood change — avoids brittle tests tied to a magic ID.
1308-1309: Batch failure assertions updated to use real messageIdCorrect and future-proof. LGTM.
Also applies to: 1995-1996, 2231-2232, 2682-2683
backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py (1)
16-21: Overall: test coverage and behaviors look solidGood breadth across EventStateClient and NotificationTracker behaviors (success, failure, per-jurisdiction independence). LGTM.
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (5)
1-30: LGTM! Clean enum definitions.The enums are well-documented and use
StrEnumappropriately for DynamoDB compatibility. TheEventTypevalues align correctly with the event system's event types.
32-95: LGTM! Solid implementation of notification attempt recording.The partition key and sort key construction is consistent with the checking logic in
NotificationTracker. Type conversions (UUID to string) are handled correctly for DynamoDB compatibility, and the TTL-based cleanup is a good practice.
96-113: LGTM! Good use of consistent reads for correctness.Using
ConsistentRead=Trueis essential here to prevent race conditions in concurrent lambda invocations, ensuring idempotency is maintained correctly.
115-150: LGTM! Correct idempotency checks.The
should_send_*methods correctly implement idempotency by checking if status is not 'SUCCESS', which handles both missing records and failed attempts. The sort key construction is consistent with the recording logic.
151-237: LGTM! Defensive error handling is well-reasoned.The decision to swallow exceptions in
record_successandrecord_failureis appropriate given the explanations:
- For successes, the notification was already sent, so metadata failure shouldn't block completion.
- For failures, the lambda will retry the entire flow anyway.
The logging provides sufficient visibility for debugging any issues with state tracking.
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (8)
9-19: LGTM! Necessary imports for idempotent notifications.The imports correctly bring in the new notification protocols, event state tracking components, and the SQS handler decorator that provides message_id for idempotency.
54-118: LGTM! Proper idempotent notification flow.The function correctly:
- Checks if the notification was already sent before attempting to send
- Records success after a successful send
- Records failure and re-raises exceptions to trigger SQS retry
- Logs all outcomes appropriately for observability
120-188: LGTM! Correct state notification idempotency.The function properly:
- Uses
JurisdictionNotificationMethodfor state-level notifications- Includes
provider_idin template variables for state notifications (appropriate since states need to know which provider is affected)- Checks and records notification state per jurisdiction
- Handles errors consistently with provider notifications
190-284: LGTM! Efficient multi-jurisdiction notification handling.The function correctly implements per-jurisdiction idempotency checks within the loop. Creating the template variables once (line 233) and reusing them is a good optimization.
411-499: LGTM! Consistent idempotent notification implementation.The listener correctly:
- Uses
@sqs_handler_with_message_idto receive the message ID- Initializes the tracker before processing notifications
- Passes all required parameters (tracker, provider_id, event_type, event_time) to notification functions
- Uses the appropriate
EventType.PRIVILEGE_ENCUMBRANCEenum value
501-634: LGTM! Improved error handling and consistent idempotency.The listener correctly implements idempotent notifications and improves error handling:
- Lines 540-542, 568-573: Assigning error messages to variables before logging and raising makes the code cleaner and more maintainable
- Tracker initialization after validation ensures we don't create unnecessary state records for invalid events
- Uses the appropriate
EventType.PRIVILEGE_ENCUMBRANCE_LIFTEDenum value
636-724: LGTM! Consistent implementation across event types.The license encumbrance notification listener follows the same solid pattern as the privilege encumbrance listener, ensuring consistent idempotent behavior across all notification types.
726-838: LGTM! Complete and consistent idempotency implementation.The license encumbrance lifting listener completes the consistent implementation pattern across all notification listeners:
- Improved error handling (lines 765-767)
- Proper tracker initialization
- Correct event type (
EventType.LICENSE_ENCUMBRANCE_LIFTED)All four notification listeners now properly implement idempotent notification delivery.
backend/compact-connect/lambdas/python/common/cc_common/utils.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/tests/function/__init__.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
443-444: Fix SQS handler docstrings to match behavior; avoid “ingest queue” wordingDocstrings say messageId is injected into the message body and reference “ingest queue”. Implementation passes message_id as a second positional arg. Update docs for accuracy and consistency.
@@ def _process_sqs_batch(fn: Callable, include_message_id: bool = False): - :param include_message_id: If True, inject the SQS messageId into the message body as 'AWSSQSMessageId' + :param include_message_id: If True, pass the SQS messageId as a second positional argument to the handler @@ def sqs_handler_with_message_id(fn: Callable): - Process messages from the ingest queue with SQS message ID injected into the message body. + Process messages from an SQS queue and pass the SQS message ID to the handler. @@ def sqs_handler_with_message_id(fn: Callable): - The message ID is added as 'AWSSQSMessageId' field in the message dict, allowing handlers - to track individual message retries for idempotency purposes. + Handlers must accept (message: dict, message_id: str), enabling idempotent processing and tracing.Also applies to: 492-505
🧹 Nitpick comments (7)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (2)
66-95: Make SUCCESS sticky; avoid overwriting with later FAILED writesRight now put_item unconditionally overwrites the item for the same pk+sk. If a SUCCESS is ever written, a later failure write could accidentally regress status and cause retries. Make SUCCESS sticky with a simple conditional write so FAILED can be overwritten by SUCCESS, but not vice‑versa. Also guards a small race window across concurrent retries.
Apply this diff inside record_notification_attempt:
- # Write to table - self.config.event_state_table.put_item(Item=item) + # Write to table with a guard: allow create/update unless status is already SUCCESS + self.config.event_state_table.put_item( + Item=item, + ConditionExpression='(attribute_not_exists(pk) AND attribute_not_exists(sk)) OR #s <> :success', + ExpressionAttributeNames={'#s': 'status'}, + ExpressionAttributeValues={':success': NotificationStatus.SUCCESS}, + )
106-112: Use Key('pk').eq(pk) for Table.query to avoid expression type issuesResource Table.query expects a ConditionBase. Using a raw string risks ParamValidationError in production. Switch to Key(...) for safety.
+from boto3.dynamodb.conditions import Key @@ - response = self.config.event_state_table.query( - KeyConditionExpression='pk = :pk', - ExpressionAttributeValues={':pk': pk}, - ConsistentRead=True, - ) + response = self.config.event_state_table.query( + KeyConditionExpression=Key('pk').eq(pk), + ConsistentRead=True, + )backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
521-563: Don’t pass tracker=None; use a NullTracker or enforce presenceIf compact is missing you call fn(message, None). Handlers assume a NotificationTracker and will call methods on it. Use a Null object (no‑op should_send*/record_*) or raise early.
- if not compact: - logger.warning('No compact found in message, skipping notification tracking', message_id=message_id) - # Still process the message but without tracking - fn(message, None) - continue + if not compact: + logger.error('Missing detail.compact; notification tracking required', message_id=message_id) + raise ValueError('Missing compact in message detail')Alternatively, introduce a lightweight NullNotificationTracker implementing the same methods and pass that instead.
Confirm all events feeding these listeners include detail.compact (schema‑enforced). Based on learnings.
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (1)
3142-3151: Avoid relying on DynamoDB query ordering in testsYou assert exact key order from attempts. Query returns ascending by sort key today, but relying on ordering is brittle. Prefer set comparison or sort() both sides before comparing.
- self.assertEqual(expected_sks, list(notification_records.keys())) + self.assertEqual(sorted(expected_sks), sorted(notification_records.keys()))backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (3)
54-79: Broaden provider_id type annotation for helper signaturesUpstream schema may yield UUID, but other call sites/tests sometimes use strings. To avoid confusion, annotate as str | UUID (you already stringify before persistence).
- provider_id: UUID, + provider_id: UUID | str,Apply the same to _send_primary_state_notification and _send_additional_state_notifications.
120-146: Same provider_id annotation adjustment hereMirror the str | UUID suggestion for consistency.
- provider_id: UUID, + provider_id: UUID | str,
190-218: Same provider_id annotation adjustment hereMirror the str | UUID suggestion for consistency.
- provider_id: UUID, + provider_id: UUID | str,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/utils.py(3 hunks)backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py(1 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(23 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-30T17:30:31.252Z
Learnt from: jusdino
PR: csg-org/CompactConnect#864
File: backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py:866-866
Timestamp: 2025-06-30T17:30:31.252Z
Learning: In the CompactConnect project, the `sqs_handler` decorator transforms Lambda functions to accept `(event, context)` parameters and automatically handles iteration over `event['Records']`, parsing JSON from each record's body, and managing SQS batch item failures. Functions decorated with `sqs_handler` have their underlying implementation accept a single `message: dict` parameter, but the decorator makes them callable with standard Lambda `(event, context)` signature. Tests should call these decorated functions with `(event, context)` parameters.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/utils.py
📚 Learning: 2025-08-29T21:45:05.792Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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/data-events/handlers/encumbrance_events.py
🧬 Code graph analysis (5)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (3)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (6)
EventType(23-29)NotificationTracker(115-237)RecipientType(9-13)record_success(151-192)record_failure(194-237)NotificationStatus(16-20)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (1)
license_encumbrance_notification_listener(629-711)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (3)
put_default_provider_record_in_provider_table(409-429)put_default_license_record_in_provider_table(238-248)put_default_privilege_record_in_provider_table(310-320)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
event_state_table(290-291)event_state_client(294-297)
backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py (3)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
event_state_client(294-297)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (11)
EventStateClient(32-112)EventType(23-29)NotificationStatus(16-20)NotificationTracker(115-237)RecipientType(9-13)record_notification_attempt(38-94)_get_notification_attempts(96-112)should_send_provider_notification(132-139)record_success(151-192)record_failure(194-237)should_send_state_notification(141-149)backend/compact-connect/lambdas/python/common/tests/function/__init__.py (1)
TstFunction(20-378)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
event_state_client(294-297)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
NotificationTracker(115-237)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (4)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (3)
EncumbranceNotificationTemplateVariables(14-24)JurisdictionNotificationMethod(35-40)ProviderNotificationMethod(27-32)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (7)
EventType(23-29)NotificationTracker(115-237)RecipientType(9-13)should_send_provider_notification(132-139)record_success(151-192)record_failure(194-237)should_send_state_notification(141-149)backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
sqs_handler(480-489)sqs_handler_with_notification_tracking(507-562)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
compactConnectRegisteredEmailAddress(77-83)ProviderData(13-148)
⏰ 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/lambdas/python/common/tests/function/test_event_state_client.py (1)
22-65: Good coverage of write shape and field assertionsTests thoroughly validate item shape, types, and TTL presence. LGTM.
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
9-31: Runtime verification complete—no issues identifiedThe Lambdas are configured with Python 3.12 or 3.13 (default is 3.13 in
python_function.py). StrEnum is supported in Python 3.11+, so there are no runtime import errors.
landonshumway-ia
force-pushed
the
feat/enhance-encumbrance-notifications
branch
from
6f3a03b to
2f5a60e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (15)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
289-291: Cache the DynamoDB Table resource (align with other tables).Use cached_property to avoid recreating the Table proxy on each access.
- @property - def event_state_table(self): - return boto3.resource('dynamodb').Table(self.event_state_table_name) + @cached_property + def event_state_table(self): + return boto3.resource('dynamodb').Table(self.event_state_table_name)backend/compact-connect/lambdas/python/data-events/tests/function/__init__.py (1)
33-35: Add waiters for creation and deletion to reduce flakiness.Create/wait/delete symmetry avoids eventual-consistency races in tests (and enables real-account runs).
@@ def build_resources(self): - self.create_data_event_table() - self.create_rate_limit_table() - self.create_event_state_table() - self.create_provider_table() + self.create_data_event_table() + self.create_rate_limit_table() + self.create_event_state_table() + self.create_provider_table() + # Wait for tables to exist (helps when running against AWS and under moto) + waiter = boto3.client('dynamodb').get_waiter('table_exists') + waiter.wait(TableName=self._rate_limit_table.name) + waiter.wait(TableName=self._event_state_table.name) @@ def delete_resources(self): - self._data_event_table.delete() - self._rate_limit_table.delete() - self._event_state_table.delete() - self._provider_table.delete() + self._data_event_table.delete() + self._rate_limit_table.delete() + self._event_state_table.delete() + self._provider_table.delete() + # Wait for deletions to complete + waiter = boto3.client('dynamodb').get_waiter('table_not_exists') + waiter.wait(TableName=self._rate_limit_table.name) + waiter.wait(TableName=self._event_state_table.name)Also applies to: 48-57, 59-80, 123-127
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
471-480: Clarify handler contract and avoid per-record import.
- Tracker may be None when compact is missing; make this explicit in the docstring.
- Hoist NotificationTracker import outside the loop (still inside the wrapper) to avoid repeated imports.
@@ def sqs_handler_with_notification_tracking(fn: Callable): - """ - Process messages from SQS with notification tracking capabilities. - - This decorator provides a generic pattern for tracking notification delivery state - across SQS message retries. It creates a NotificationTracker and passes it as a - parameter to the handler function. - - The handler function should accept (message: dict, tracker: NotificationTracker) as parameters. - """ + """ + Process messages from SQS with notification tracking. + + - For each record, create a NotificationTracker and pass it to the handler. + - If compact is missing, the handler is invoked with tracker=None. + + Handler signature: (message: dict, tracker: "NotificationTracker | None") -> None. + """ @@ - def process_messages(event, context: LambdaContext): # noqa: ARG001 unused-argument + def process_messages(event, context: LambdaContext): # noqa: ARG001 unused-argument + # Local import to avoid module-level cycle; done once per batch, not per record + from cc_common.event_state_client import NotificationTracker @@ - # Create notification tracker and pass as parameter - from cc_common.event_state_client import NotificationTracker - tracker = NotificationTracker(compact=compact, message_id=message_id)Also applies to: 490-507
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (3)
1308-1309: DRY the repeated “single-record batch failure” assertion.Extract a helper to reduce duplication and improve readability.
@@ class TestEncumbranceEvents(TstFunction): + def _assert_single_record_batch_failure(self, event, result): + self.assertEqual({'batchItemFailures': [{'itemIdentifier': event['Records'][0]['messageId']}]}, result) @@ - expected_failure = {'batchItemFailures': [{'itemIdentifier': event['Records'][0]['messageId']}]} - self.assertEqual(expected_failure, result) + self._assert_single_record_batch_failure(event, result) @@ - expected_failure = {'batchItemFailures': [{'itemIdentifier': event['Records'][0]['messageId']}]} - self.assertEqual(expected_failure, result) + self._assert_single_record_batch_failure(event, result) @@ - expected_failure = {'batchItemFailures': [{'itemIdentifier': event['Records'][0]['messageId']}]} - self.assertEqual(expected_failure, result) + self._assert_single_record_batch_failure(event, result) @@ - expected_failure = {'batchItemFailures': [{'itemIdentifier': event['Records'][0]['messageId']}]} - self.assertEqual(expected_failure, result) + self._assert_single_record_batch_failure(event, result)Also applies to: 1995-1996, 2231-2232, 2682-2683
3116-3119: Fix test docstring to reflect purpose (success tracking).Docstring repeats prior test wording. Suggest updating to match the name.
- """ - Test that license encumbrance notification listener skips notifications that were already sent successfully - and only retries notifications that failed in a previous attempt. - """ + """Test that successful notification attempts are recorded to the EventState table."""
3142-3151: Avoid reliance on SK sort order in assertions.DynamoDB Query sorts by SK today, but tests are clearer if order-agnostic.
- self.assertEqual(expected_sks, list(notification_records.keys())) + self.assertCountEqual(expected_sks, list(notification_records.keys()))backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py (1)
17-19: Patched datetime is unused due to TTL implementation.You patch _Config.current_standard_datetime, but EventStateClient computes TTL via time.time(). Either remove this patch or update EventStateClient to base TTL on config.current_standard_datetime for deterministic tests. See suggested change in EventStateClient comment.
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (4)
71-75: Base TTL on config time for determinism and testability.Using time.time() makes TTL hard to freeze in tests. Prefer config.current_standard_datetime.timestamp().
Apply:
- # Calculate TTL - ttl = int(time.time()) + int(timedelta(weeks=ttl_weeks).total_seconds()) + # Calculate TTL using standardized, patchable clock + base_ts = int(self.config.current_standard_datetime.timestamp()) + ttl = base_ts + int(timedelta(weeks=ttl_weeks).total_seconds())
78-83: Store enums explicitly as strings to avoid serializer surprises.Boto3/DynamoDB accept str; StrEnum is str-like but being explicit improves clarity and avoids edge cases.
- 'status': status, + 'status': str(status), 'providerId': str(provider_id), # Convert UUID to string for DynamoDB - 'eventType': event_type, + 'eventType': str(event_type),
132-140: Avoid magic strings in comparisons.Use NotificationStatus.SUCCESS for consistency. Behavior stays the same.
- return self._attempts.get(sk, {}).get('status') != 'SUCCESS' + return self._attempts.get(sk, {}).get('status') != str(NotificationStatus.SUCCESS)- return self._attempts.get(sk, {}).get('status') != 'SUCCESS' + return self._attempts.get(sk, {}).get('status') != str(NotificationStatus.SUCCESS)Also applies to: 148-150
92-95: Idempotency durability: avoid downgrading SUCCESS with later FAILED writes.Optional hardening: use update_item with a condition to prevent overwriting a prior SUCCESS with FAILED (e.g., due to out-of-band retries). Keep allowing FAILED→SUCCESS.
Pseudocode sketch:
self.config.event_state_table.update_item( Key={'pk': pk, 'sk': sk}, UpdateExpression='SET #s=:status, providerId=:pid, eventType=:etype, eventTime=:etime, ttl=:ttl', ConditionExpression='attribute_not_exists(#s) OR #s <> :success OR :status = :success', ExpressionAttributeNames={'#s': 'status'}, ExpressionAttributeValues={ ':status': str(status), ':success': str(NotificationStatus.SUCCESS), ':pid': str(provider_id), ':etype': str(event_type), ':etime': str(event_time), ':ttl': ttl, }, )backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (4)
54-77: provider_id type mismatch vs event payload.Event detail providerId is typically a string; helper signature requires UUID. Either widen to UUID | str or normalize at extraction.
-def _send_provider_notification( +def _send_provider_notification( notification_method: ProviderNotificationMethod, notification_type: str, *, provider_record: ProviderData, compact: str, - provider_id: UUID, + provider_id: UUID | str, event_type: EventType, event_time: str, tracker: NotificationTracker, **notification_kwargs, ) -> None:Apply the same widening in _send_primary_state_notification and _send_additional_state_notifications. Alternatively, coerce once at listener entry: provider_id = UUID(provider_id).
121-131: Same provider_id type mismatch here.Adjust parameter type to UUID | str to match event data (or coerce once upstream).
-def _send_primary_state_notification( +def _send_primary_state_notification( notification_method: JurisdictionNotificationMethod, ... - provider_id: UUID, + provider_id: UUID | str,
190-203: Same provider_id type mismatch in additional states helper.Widen to UUID | str (or normalize once in the listener).
-def _send_additional_state_notifications( +def _send_additional_state_notifications( notification_method: JurisdictionNotificationMethod, ... - provider_id: UUID, + provider_id: UUID | str,
240-283: LGTM: Deduped per-jurisdiction notifications with idempotent gating.Set-based dedupe plus tracker checks looks good. Consider lowercasing jurisdictions to avoid case drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/compact-connect/lambdas/python/common/cc_common/config.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/utils.py(2 hunks)backend/compact-connect/lambdas/python/common/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/function/__init__.py(5 hunks)backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py(1 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(23 hunks)backend/compact-connect/lambdas/python/data-events/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/__init__.py(3 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(8 hunks)backend/compact-connect/pipeline/backend_stage.py(3 hunks)backend/compact-connect/stacks/event_state_stack/__init__.py(1 hunks)backend/compact-connect/stacks/event_state_stack/event_state_table.py(1 hunks)backend/compact-connect/stacks/notification_stack.py(5 hunks)backend/compact-connect/stacks/persistent_stack/__init__.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/compact-connect/stacks/event_state_stack/event_state_table.py
- backend/compact-connect/stacks/event_state_stack/init.py
- backend/compact-connect/lambdas/python/common/tests/init.py
- backend/compact-connect/lambdas/python/data-events/tests/init.py
- backend/compact-connect/stacks/notification_stack.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-09T21:11:36.590Z
Learnt from: jusdino
PR: csg-org/CompactConnect#1143
File: backend/compact-connect/tests/app/base.py:514-530
Timestamp: 2025-10-09T21:11:36.590Z
Learning: In the BackendStage class within the CompactConnect backend infrastructure, the following stacks are always required and their absence indicates a serious error: api_lambda_stack, api_stack, disaster_recovery_stack, event_listener_stack, feature_flag_stack, ingest_stack, managed_login_stack, persistent_stack, provider_users_stack, state_api_stack, state_auth_stack, and transaction_monitoring_stack. Only backup_infrastructure_stack can be None (when backups are disabled). The reporting_stack and notification_stack are conditionally present based on hosted_zone configuration.
Applied to files:
backend/compact-connect/pipeline/backend_stage.py
📚 Learning: 2025-06-30T17:30:31.252Z
Learnt from: jusdino
PR: csg-org/CompactConnect#864
File: backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py:866-866
Timestamp: 2025-06-30T17:30:31.252Z
Learning: In the CompactConnect project, the `sqs_handler` decorator transforms Lambda functions to accept `(event, context)` parameters and automatically handles iteration over `event['Records']`, parsing JSON from each record's body, and managing SQS batch item failures. Functions decorated with `sqs_handler` have their underlying implementation accept a single `message: dict` parameter, but the decorator makes them callable with standard Lambda `(event, context)` signature. Tests should call these decorated functions with `(event, context)` parameters.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/utils.py
📚 Learning: 2025-08-29T21:45:05.792Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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/data-events/handlers/encumbrance_events.py
📚 Learning: 2025-08-22T22:02:41.865Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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/data-events/tests/function/__init__.py
🧬 Code graph analysis (8)
backend/compact-connect/pipeline/backend_stage.py (1)
backend/compact-connect/stacks/event_state_stack/__init__.py (1)
EventStateStack(9-36)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
event_state_client(294-297)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
NotificationTracker(115-237)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (5)
backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
email_service_client(312-317)event_state_client(294-297)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (3)
EncumbranceNotificationTemplateVariables(14-24)JurisdictionNotificationMethod(35-40)ProviderNotificationMethod(27-32)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (7)
EventType(23-29)NotificationTracker(115-237)RecipientType(9-13)should_send_provider_notification(132-139)record_success(151-192)record_failure(194-237)should_send_state_notification(141-149)backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
sqs_handler(433-468)sqs_handler_with_notification_tracking(471-530)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
compactConnectRegisteredEmailAddress(77-83)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (4)
compact(32-33)compact(164-165)jurisdiction(36-37)jurisdiction(168-169)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
compact(29-30)compact(175-176)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py (2)
compact(58-59)compact(108-109)
backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py (2)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (11)
EventStateClient(32-112)EventType(23-29)NotificationStatus(16-20)NotificationTracker(115-237)RecipientType(9-13)record_notification_attempt(38-94)_get_notification_attempts(96-112)should_send_provider_notification(132-139)record_success(151-192)record_failure(194-237)should_send_state_notification(141-149)backend/compact-connect/lambdas/python/common/tests/function/__init__.py (1)
TstFunction(20-380)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
backend/compact-connect/lambdas/python/common/cc_common/config.py (3)
_Config(21-335)event_state_table(290-291)event_state_client(294-297)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
event_state_client(294-297)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (6)
EventType(23-29)NotificationTracker(115-237)RecipientType(9-13)record_success(151-192)record_failure(194-237)NotificationStatus(16-20)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
EventStateClient(32-112)
⏰ 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 (19)
backend/compact-connect/stacks/persistent_stack/__init__.py (2)
2-3: LGTM: Imports added correctly.The CloudWatch alarm and SNS action imports are appropriate for the email notification failure alerting functionality.
338-346: LGTM: Alarm configuration correctly implements failure alerting.The alarm implementation properly addresses the PR objective to surface email notification failures. The configuration is appropriate:
- Immediate alerting (threshold=1, evaluation_periods=1) ensures every failure is caught
- SNS action correctly routes alerts to the existing alarm topic
- Removing the explicit
alarm_nameparameter (as noted in past review) allows CDK to auto-generate environment-specific names, preventing deployment conflictsbackend/compact-connect/pipeline/backend_stage.py (2)
55-63: Good: EventStateStack is created early and independently.Instantiation precedes dependent stacks and uses the same env/tags/persistent_stack pattern as others. LGTM.
155-164: Gathering more context to confirm the lambda handler details:Let me verify if there are other notification handlers that need similar permissions:
Let me look for any other notification handlers that might need event_state_stack permissions:
Verification Complete: Event State Stack Integration Confirmed
Both requirements from the review comment have been verified as properly implemented:
EVENT_STATE_TABLE_NAME exposure: The environment variable is correctly passed to the email notification handler at
backend/compact-connect/stacks/notification_stack.py:178viaself.event_state_stack.event_state_table.table_name.IAM permissions: Read/write access to the event state table is properly granted to the notification handler at
backend/compact-connect/stacks/notification_stack.py:187using the AWS CDKgrant_read_write_datamethod.The implementation follows AWS CDK best practices and properly integrates the event_state_stack into the NotificationStack.
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
293-297: LGTM on client construction.cached_property for event_state_client keeps one client per cold start and avoids import cycles. Looks good.
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
35-41: Protocol addition looks good.Adds a clear callable contract for jurisdiction notifications; complements ProviderNotificationMethod.
backend/compact-connect/lambdas/python/common/tests/function/__init__.py (1)
46-56: Nice: deterministic setup/teardown for EventStateTable.GSI shape matches usage; existence/non-existence waiters reduce test flakes. LGTM.
Also applies to: 87-108, 214-214, 223-223
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
433-441: LGTM on generic SQS wording.Doc now correctly describes a generic SQS handler. No further action.
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (1)
60-61: Good: messageId uses real UUIDs.Avoids brittle hard-coding and matches SQS semantics. LGTM.
backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py (4)
51-64: LGTM: Item shape verification is comprehensive.Validates pk/sk composition, enum string values, and ttl presence. Good coverage.
95-98: LGTM: Provider SK format and jurisdiction omission verified.Correctly asserts SK “NOTIFICATION#provider#” and absence of jurisdiction.
128-133: LGTM: Failure path captures error_message.Ensures failures persist diagnostics.
178-196: LGTM: Retrieval returns per-recipient attempts with correct statuses.Confirms keys for provider and per-jurisdiction state records.
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (6)
147-173: LGTM: Idempotent gating for primary state.Properly checks tracker.should_send_state_notification and records outcome.
411-440: LGTM: Tracker threaded through and event_time propagated.Good: provider/state flows are gated and outcomes recorded.
530-569: Input validation on lifting path is clear and actionable.Explicit CCInternalException with logs reads well for ops.
747-786: LGTM: License lifting notification flow mirrors privilege path with tracker and effective lift date.Consistent gating and metadata propagation.
9-13: Imports are well-scoped; minor tidy-up optional.RecipientType is imported and used; no unused additions. Nothing to change.
Also applies to: 16-20
411-495: Verification complete: All email sends are properly guarded by tracker helpers.The codebase has been verified to implement idempotent notification delivery correctly:
- No direct invocations of
send_*_encumbrance*_notification_emailmethods exist outside the handler file- All 12 email method references in the handler are passed to three helper functions:
_send_provider_notification,_send_primary_state_notification, and_send_additional_state_notifications- Each helper function:
- Accepts
tracker: NotificationTrackerparameter- Uses tracker to guard sends:
tracker.should_send_provider_notification()ortracker.should_send_state_notification()- Invokes the email method only when guarded check passes
- Calls
tracker.record_success()after successful send to prevent resends on retriesThis implementation ensures idempotent delivery across the privilege encumbrance, privilege encumbrance lifting, license encumbrance, and license encumbrance lifting event handlers.
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
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/data-events/handlers/encumbrance_events.py (1)
768-786: Validate thatlatest_effective_lift_dateis not None before using it.The method
get_latest_effective_lift_date_for_license_adverse_actionscan returnNone. If the license is unencumbered but no lift date exists, passingNoneaseffective_dateto the notification could cause downstream issues in email template rendering. The privilege handler validates this at lines 562-568; add similar validation here.Apply this diff to add validation:
# license is unencumbered, get latest effective lift date for all adverse actions latest_effective_lift_date = provider_records.get_latest_effective_lift_date_for_license_adverse_actions( license_jurisdiction=target_license.jurisdiction, license_type_abbreviation=target_license.licenseTypeAbbreviation, ) + + if latest_effective_lift_date is None: + error_message = ( + 'No latest effective lift date found for this license record. Records with an unencumbered ' + 'status should have a latest effective lift date' + ) + logger.error(error_message) + raise CCInternalException(error_message) # Provider Notification _send_provider_notification(
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (1)
80-116: Do not log raw email addresses (PII). Mask or omit.Logger includes
provider_emailat lines 82 and 115. This is PII and can violate logging/privacy guidance. Mask before logging or drop it and rely onprovider_idfor correlation.Apply this diff to remove PII from logs:
if tracker.should_send_provider_notification(): - logger.info(f'Sending {notification_type} notification to provider', provider_email=provider_email) + logger.info(f'Sending {notification_type} notification to provider', provider_id=provider_id) try: notification_method( ... else: - logger.info('Skipping provider notification (already sent successfully)', provider_id=provider_id) + logger.info('Skipping provider notification (already sent successfully)', provider_id=provider_id)
🧹 Nitpick comments (2)
backend/compact-connect/stacks/event_state_stack/event_state_table.py (2)
42-42: Simplify the boolean expression.The ternary operator is redundant since the comparison already returns a boolean.
Apply this diff:
- deletion_protection=True if removal_policy == RemovalPolicy.RETAIN else False, + deletion_protection=(removal_policy == RemovalPolicy.RETAIN),
46-52: Consider making the GSI name a class constant.Storing the index name as an instance variable works, but defining it as a class constant would improve discoverability and make it clear this is a fixed configuration value rather than dynamic state.
Apply this diff:
class EventStateTable(Table): """ DynamoDB table for tracking event processing state across SQS message retries. This table is used to maintain idempotency and track the success/failure state of various operations performed during event processing, particularly for notification delivery tracking. """ + + PROVIDER_EVENT_TIME_INDEX_NAME = 'providerId-eventTime-index' def __init__( self, scope: Construct, construct_id: str, encryption_key: IKey, removal_policy: RemovalPolicy, ) -> None: super().__init__( scope, construct_id, billing_mode=BillingMode.PAY_PER_REQUEST, encryption=TableEncryption.CUSTOMER_MANAGED, encryption_key=encryption_key, partition_key={'name': 'pk', 'type': AttributeType.STRING}, sort_key={'name': 'sk', 'type': AttributeType.STRING}, point_in_time_recovery_specification=PointInTimeRecoverySpecification(point_in_time_recovery_enabled=True), removal_policy=removal_policy, deletion_protection=(removal_policy == RemovalPolicy.RETAIN), time_to_live_attribute='ttl', ) - self.provider_event_time_index_name = 'providerId-eventTime-index' + self.provider_event_time_index_name = self.PROVIDER_EVENT_TIME_INDEX_NAME self.add_global_secondary_index( index_name=self.provider_event_time_index_name, partition_key=Attribute(name='providerId', type=AttributeType.STRING), sort_key=Attribute(name='eventTime', type=AttributeType.STRING), projection_type=ProjectionType.ALL, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(23 hunks)backend/compact-connect/stacks/event_state_stack/event_state_table.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T21:45:05.792Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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/data-events/handlers/encumbrance_events.py
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (5)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (3)
EncumbranceNotificationTemplateVariables(14-24)JurisdictionNotificationMethod(35-40)ProviderNotificationMethod(27-32)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (7)
EventType(23-29)NotificationTracker(115-237)RecipientType(9-13)should_send_provider_notification(132-139)record_success(151-192)record_failure(194-237)should_send_state_notification(141-149)backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
sqs_handler(433-468)sqs_handler_with_notification_tracking(471-530)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
compactConnectRegisteredEmailAddress(77-83)ProviderData(13-148)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
ProviderUserRecords(403-791)
⏰ 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/stacks/event_state_stack/event_state_table.py (2)
1-14: LGTM!The imports are well-organized and all necessary constructs are included. The use of both
PointInTimeRecoverySpecificationandTableEncryptionaligns with the established patterns in the codebase.
16-23: LGTM!The class documentation clearly explains the purpose and use case for this table, which aligns perfectly with the PR objectives of tracking event state to prevent duplicate notifications.
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (5)
1-20: LGTM: Imports support new tracking functionality.All necessary types and decorators imported to enable idempotent notification delivery.
120-188: LGTM: Primary state notification logic is correct.Idempotency checks, success/failure tracking, and exception handling are all properly implemented.
190-284: LGTM: Additional state notifications correctly implement per-jurisdiction tracking.The loop properly checks idempotency for each jurisdiction and records success/failure individually.
411-495: LGTM: Privilege encumbrance notification handler correctly integrated with tracking.The decorator, tracker parameter, and all notification calls are properly configured for idempotent delivery.
497-626: LGTM: Privilege lifting handler correctly validates lift dates and integrates tracking.Proper validation ensures notifications are sent only when lift dates are available, and all tracking parameters are correctly passed.
backend/compact-connect/stacks/event_state_stack/event_state_table.py
Outdated
Show resolved
Hide resolved
jusdino
left a comment
jusdino
left a comment
There was a problem hiding this comment.
Looks great! Only a couple pretty minor nits.
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/stacks/notification_stack.py (1)
44-44: Drop unused stack attribute
self.event_state_stackis assigned but never referenced. To keep cross-stack dependencies traceable (and in line with the earlier preference to avoid storing external stacks onself), please drop the attribute and continue passingevent_state_stackexplicitly into the helpers. Based on learnings
📜 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/transaction_client.py(1 hunks)backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py(1 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(23 hunks)backend/compact-connect/stacks/disaster_recovery_stack/__init__.py(1 hunks)backend/compact-connect/stacks/event_state_stack/event_state_table.py(1 hunks)backend/compact-connect/stacks/notification_stack.py(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/stacks/disaster_recovery_stack/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/stacks/event_state_stack/event_state_table.py
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-06-09T22:03:03.232Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 852
File: backend/compact-connect/stacks/persistent_stack/__init__.py:545-549
Timestamp: 2025-06-09T22:03:03.232Z
Learning: In the CompactConnect codebase, production active_compact_member_jurisdictions configurations are very stable and unlikely to be removed. The vast majority of configuration errors in get_list_of_active_jurisdictions_for_compact_environment() will occur when developers are deploying sandbox environments, so error messages that primarily reference sandbox configuration are appropriate for the typical use case.
Applied to files:
backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py
📚 Learning: 2025-06-26T16:42:00.781Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 882
File: backend/compact-connect/lambdas/python/migration/compact_configured_states_871/main.py:127-130
Timestamp: 2025-06-26T16:42:00.781Z
Learning: In the compact_configured_states_871 migration, existing jurisdiction configurations that have licenseeRegistrationEnabled: true are migrated with isLive: true for backwards compatibility. This prevents breaking existing live functionality since those states are already operational. The migration preserves the current live state of jurisdictions to maintain service continuity, while new states added after migration can start with isLive: false and be controlled by compact admins.
Applied to files:
backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py
📚 Learning: 2025-10-09T21:11:36.590Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1143
File: backend/compact-connect/tests/app/base.py:514-530
Timestamp: 2025-10-09T21:11:36.590Z
Learning: In the BackendStage class within the CompactConnect backend infrastructure, the following stacks are always required and their absence indicates a serious error: api_lambda_stack, api_stack, disaster_recovery_stack, event_listener_stack, feature_flag_stack, ingest_stack, managed_login_stack, persistent_stack, provider_users_stack, state_api_stack, state_auth_stack, and transaction_monitoring_stack. Only backup_infrastructure_stack can be None (when backups are disabled). The reporting_stack and notification_stack are conditionally present based on hosted_zone configuration.
Applied to files:
backend/compact-connect/stacks/notification_stack.py
📚 Learning: 2025-06-18T19:52:23.159Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 864
File: backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py:220-526
Timestamp: 2025-06-18T19:52:23.159Z
Learning: The user prefers pragmatic solutions that directly address the specific problem rather than structural refactoring that doesn't provide real value. When dealing with parameter bloat warnings, they favor keyword-only arguments over dataclass wrappers to prevent positional argument misuse without over-engineering.
Applied to files:
backend/compact-connect/stacks/notification_stack.py
📚 Learning: 2025-08-18T17:04:16.686Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1001
File: backend/compact-connect/stacks/disaster_recovery_stack/__init__.py:42-56
Timestamp: 2025-08-18T17:04:16.686Z
Learning: In CDK stacks, KMS key policies with restrictive conditions like kms:EncryptionContext:aws:logs:arn are incompatible with auto-generated log group resources because the exact ARN isn't known at policy creation time. Regional service principals (e.g., logs.region.amazonaws.com) provide sufficient security for CloudWatch Logs encryption without creating deployment failures.
Applied to files:
backend/compact-connect/stacks/notification_stack.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/data-events/handlers/encumbrance_events.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/data-events/handlers/encumbrance_events.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/data-events/handlers/encumbrance_events.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/data-events/handlers/encumbrance_events.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/data-events/handlers/encumbrance_events.py
📚 Learning: 2025-05-30T13:57:36.286Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:1357-1490
Timestamp: 2025-05-30T13:57:36.286Z
Learning: CCInternalException is the appropriate exception type to use when adverse action records exist without corresponding license or privilege records, as this indicates a data consistency issue that should fire alerts.
Applied to files:
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
🧬 Code graph analysis (2)
backend/compact-connect/stacks/notification_stack.py (2)
backend/compact-connect/stacks/event_state_stack/__init__.py (1)
EventStateStack(9-36)backend/compact-connect/stacks/persistent_stack/__init__.py (1)
PersistentStack(37-491)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (5)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (3)
EncumbranceNotificationTemplateVariables(14-24)JurisdictionNotificationMethod(35-40)ProviderNotificationMethod(27-32)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (7)
EventType(23-29)NotificationTracker(115-237)RecipientType(9-13)should_send_provider_notification(132-139)record_success(151-192)record_failure(194-237)should_send_state_notification(141-149)backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
sqs_handler(433-468)sqs_handler_with_notification_tracking(471-530)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
compactConnectRegisteredEmailAddress(77-83)ProviderData(13-148)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
ProviderUserRecords(403-791)
🔇 Additional comments (2)
backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py (1)
219-219: LGTM! Syntax correction.This is a minor syntax fix ensuring proper bracket alignment in the test data structure. No functional changes.
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
318-318: LGTM!The trailing comma addition follows Python best practices for multi-line function calls and makes future edits cleaner.
landonshumway-ia
force-pushed
the
feat/enhance-encumbrance-notifications
branch
from
92f731e to
8518d62
Compare
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 (2)
backend/compact-connect/lambdas/python/common/tests/function/__init__.py (1)
112-118: Fix DynamoDB AttributeDefinitions: 'famGiv' AttributeType must be 'S' (not 'RANGE')RANGE is a KeyType, not an AttributeType. This will fail table creation against real DynamoDB.
Apply:- {'AttributeName': 'famGiv', 'AttributeType': 'RANGE'}, + {'AttributeName': 'famGiv', 'AttributeType': 'S'},backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (1)
768-785: Validate latest_effective_lift_date before use.Unlike
privilege_encumbrance_lifting_notification_listener(lines 562-568), this handler doesn't validate thatlatest_effective_lift_dateis notNone. If a license is unencumbered but has no lift date,Nonewould be passed to notification functions (lines 785, 802, 819), which likely expects a date value.Apply this diff to add validation:
latest_effective_lift_date = provider_records.get_latest_effective_lift_date_for_license_adverse_actions( license_jurisdiction=target_license.jurisdiction, license_type_abbreviation=target_license.licenseTypeAbbreviation, ) + + if latest_effective_lift_date is None: + error_message = ( + 'No latest effective lift date found for this license record. Records with an unencumbered ' + 'status should have a latest effective lift date' + ) + logger.error(error_message) + raise CCInternalException(error_message) # Provider Notification
🧹 Nitpick comments (3)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
35-41: Good addition: Protocol for jurisdiction notificationsConsider annotating send_state methods or injection points with JurisdictionNotificationMethod for stronger typing consistency.
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (1)
3036-3151: Strong idempotency coverage for notification trackingValidates skip of successes and retry of failures; also checks persisted SUCCESS statuses. Consider adding a case where the email send raises to assert tracker.record_failure is written by the listener.
Also applies to: 3110-3151
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
471-480: Clarify handler contract and avoid per-record import
- Handlers may receive tracker=None when compact is missing; document it.
- Move NotificationTracker import out of the per-record loop.
def sqs_handler_with_notification_tracking(fn: Callable): - """ - Process messages from SQS with notification tracking capabilities. - - This decorator provides a generic pattern for tracking notification delivery state - across SQS message retries. It creates a NotificationTracker and passes it as a - parameter to the handler function. - - The handler function should accept (message: dict, tracker: NotificationTracker) as parameters. - """ + """ + Process messages from SQS with notification tracking. + + - Creates a NotificationTracker per message and passes it to the handler. + - If the message is missing detail.compact, the handler is invoked with tracker=None. + + Handler signature: (message: dict, tracker: NotificationTracker | None) -> Any + """ @@ - for record in records: + # Import once to avoid per-record overhead + from cc_common.event_state_client import NotificationTracker + + for record in records: @@ - # Create notification tracker and pass as parameter - from cc_common.event_state_client import NotificationTracker - tracker = NotificationTracker(compact=compact, message_id=message_id)Add a unit test that sends a message with no detail.compact and asserts the handler received tracker=None.
Also applies to: 490-507
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backend/compact-connect/lambdas/python/common/cc_common/config.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/utils.py(2 hunks)backend/compact-connect/lambdas/python/common/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/function/__init__.py(5 hunks)backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py(1 hunks)backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py(1 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(23 hunks)backend/compact-connect/lambdas/python/data-events/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/__init__.py(3 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(8 hunks)backend/compact-connect/pipeline/backend_stage.py(3 hunks)backend/compact-connect/stacks/disaster_recovery_stack/__init__.py(1 hunks)backend/compact-connect/stacks/event_state_stack/__init__.py(1 hunks)backend/compact-connect/stacks/event_state_stack/event_state_table.py(1 hunks)backend/compact-connect/stacks/notification_stack.py(11 hunks)backend/compact-connect/stacks/persistent_stack/__init__.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/stacks/disaster_recovery_stack/init.py
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/compact-connect/stacks/event_state_stack/init.py
- backend/compact-connect/lambdas/python/common/cc_common/config.py
- backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py
- backend/compact-connect/stacks/notification_stack.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py
- backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py
🧰 Additional context used
🧠 Learnings (16)
📚 Learning: 2025-08-12T20:52:05.861Z
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.
Applied to files:
backend/compact-connect/stacks/event_state_stack/event_state_table.py
📚 Learning: 2025-07-21T20:37:29.741Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 907
File: backend/compact-connect/common_constructs/backup_plan.py:47-47
Timestamp: 2025-07-21T20:37:29.741Z
Learning: In CompactConnect, landonshumway-ia prefers to manage all backup configuration through Infrastructure as Code (CDK/CloudFormation) and does not want to support console-based modifications of backup rules. This aligns with best practices for infrastructure management and means AWS Console limitations (like 50-character rule name limits) are not a concern since console editing is not intended to be supported.
Applied to files:
backend/compact-connect/stacks/event_state_stack/event_state_table.pybackend/compact-connect/stacks/persistent_stack/__init__.py
📚 Learning: 2025-08-12T19:23:01.151Z
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:282-296
Timestamp: 2025-08-12T19:23:01.151Z
Learning: In the CompactConnect disaster recovery system, table name validation (tableNameRecoveryConfirmation) is performed at the downstream Lambda function level (in the cleanup_records and copy_records handlers) rather than at the Step Function level, ensuring destructive operations fail safely if the confirmation doesn't match the intended destination table.
Applied to files:
backend/compact-connect/stacks/event_state_stack/event_state_table.py
📚 Learning: 2025-10-09T21:11:36.590Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1143
File: backend/compact-connect/tests/app/base.py:514-530
Timestamp: 2025-10-09T21:11:36.590Z
Learning: In the BackendStage class within the CompactConnect backend infrastructure, the following stacks are always required and their absence indicates a serious error: api_lambda_stack, api_stack, disaster_recovery_stack, event_listener_stack, feature_flag_stack, ingest_stack, managed_login_stack, persistent_stack, provider_users_stack, state_api_stack, state_auth_stack, and transaction_monitoring_stack. Only backup_infrastructure_stack can be None (when backups are disabled). The reporting_stack and notification_stack are conditionally present based on hosted_zone configuration.
Applied to files:
backend/compact-connect/stacks/persistent_stack/__init__.pybackend/compact-connect/pipeline/backend_stage.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/stacks/persistent_stack/__init__.pybackend/compact-connect/lambdas/python/common/cc_common/utils.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 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/data-events/tests/function/__init__.pybackend/compact-connect/lambdas/python/common/tests/function/__init__.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/data-events/tests/function/__init__.pybackend/compact-connect/lambdas/python/common/tests/function/__init__.py
📚 Learning: 2025-06-30T17:30:31.252Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 864
File: backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py:866-866
Timestamp: 2025-06-30T17:30:31.252Z
Learning: In the CompactConnect project, the `sqs_handler` decorator transforms Lambda functions to accept `(event, context)` parameters and automatically handles iteration over `event['Records']`, parsing JSON from each record's body, and managing SQS batch item failures. Functions decorated with `sqs_handler` have their underlying implementation accept a single `message: dict` parameter, but the decorator makes them callable with standard Lambda `(event, context)` signature. Tests should call these decorated functions with `(event, context)` parameters.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/utils.pybackend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.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/common/cc_common/utils.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/data-events/tests/function/test_encumbrance_events.pybackend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.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/data-events/handlers/encumbrance_events.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/data-events/handlers/encumbrance_events.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/data-events/handlers/encumbrance_events.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/data-events/handlers/encumbrance_events.py
📚 Learning: 2025-05-30T13:57:36.286Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:1357-1490
Timestamp: 2025-05-30T13:57:36.286Z
Learning: CCInternalException is the appropriate exception type to use when adverse action records exist without corresponding license or privilege records, as this indicates a data consistency issue that should fire alerts.
Applied to files:
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
📚 Learning: 2025-08-18T21:59:25.029Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/stacks/api_stack/v1_api/api.py:31-36
Timestamp: 2025-08-18T21:59:25.029Z
Learning: The CompactConnect codebase has two separate V1Api implementations: one for the internal API stack (backend/compact-connect/stacks/api_stack/v1_api/api.py) and one for the state API stack (backend/compact-connect/stacks/state_api_stack/v1_api/api.py). These are distinct classes with different constructor signatures and purposes. Changes to one V1Api implementation do not necessarily affect the other.
Applied to files:
backend/compact-connect/pipeline/backend_stage.py
🧬 Code graph analysis (6)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (4)
compact(32-33)compact(164-165)jurisdiction(36-37)jurisdiction(168-169)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
compact(29-30)compact(175-176)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py (2)
compact(58-59)compact(108-109)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
event_state_client(294-297)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
NotificationTracker(115-237)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (3)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (6)
EventType(23-29)NotificationTracker(115-237)RecipientType(9-13)record_success(151-192)record_failure(194-237)NotificationStatus(16-20)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (1)
license_encumbrance_notification_listener(629-711)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (3)
put_default_provider_record_in_provider_table(409-429)put_default_license_record_in_provider_table(238-248)put_default_privilege_record_in_provider_table(310-320)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (4)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (3)
EncumbranceNotificationTemplateVariables(14-24)JurisdictionNotificationMethod(35-40)ProviderNotificationMethod(27-32)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (7)
EventType(23-29)NotificationTracker(115-237)RecipientType(9-13)should_send_provider_notification(132-139)record_success(151-192)record_failure(194-237)should_send_state_notification(141-149)backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
sqs_handler(433-468)sqs_handler_with_notification_tracking(471-530)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
compactConnectRegisteredEmailAddress(77-83)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
backend/compact-connect/lambdas/python/common/cc_common/config.py (3)
_Config(21-335)event_state_table(290-291)event_state_client(294-297)
backend/compact-connect/pipeline/backend_stage.py (1)
backend/compact-connect/stacks/event_state_stack/__init__.py (1)
EventStateStack(9-36)
⏰ 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 (14)
backend/compact-connect/lambdas/python/data-events/tests/__init__.py (1)
23-23: LGTM! Event state table environment variable added.The addition of
EVENT_STATE_TABLE_NAMEis consistent with the existing pattern and directly supports the PR's objective of tracking event-related state for notification management.backend/compact-connect/lambdas/python/data-events/tests/function/__init__.py (4)
33-34: LGTM! Test table setup extended appropriately.The additions integrate smoothly with the existing test infrastructure pattern.
48-57: Clarify the relationship to PR objectives.The table creation follows the correct pattern and structure. However, the rate limiting table doesn't appear to be directly mentioned in the PR objectives focused on encumbrance notification tracking and failure alerting. Is this supporting infrastructure for rate-limiting notification sends, or is it unrelated work included in this PR?
59-80: LGTM! Event state table creation is well-structured.The table schema directly supports the PR's objective of tracking event-related state to prevent duplicate notifications. The GSI on
providerId-eventTime-indexenables efficient queries by provider and temporal ordering, which aligns well with the notification tracking use case.
125-126: LGTM! Cleanup properly extended for new tables.The new table cleanup follows the established pattern and integrates correctly with the auto-cleanup framework.
Based on learnings.
backend/compact-connect/stacks/persistent_stack/__init__.py (2)
2-3: LGTM! Necessary imports for alarm functionality.The imports are properly added to support the new CloudWatch alarm and SNS action configuration for monitoring email notification failures.
340-348: LGTM! Alarm properly configured and previous naming concern resolved.The CloudWatch alarm implementation is solid:
- Previous concern addressed: The
alarm_nameparameter has been removed, allowing CDK to auto-generate unique names per environment. This prevents the CloudFormation conflicts flagged in the previous review.- Appropriate sensitivity: The threshold of 1 error with a 1-minute evaluation period is suitable for a critical notification service where every delivery failure should be investigated.
- Correct wiring: The SNS action properly routes alerts to the existing
alarm_topic(configured at lines 79-85), which handles email and Slack notifications based on environment configuration.The implementation aligns with the PR objective to surface email notification failures and follows CDK best practices.
backend/compact-connect/lambdas/python/common/tests/__init__.py (1)
21-22: Env var added for EventState table looks goodUnblocks tests that rely on config.event_state_table_name.
backend/compact-connect/lambdas/python/common/tests/function/__init__.py (1)
46-56: End-to-end EventState test resource lifecycle is solidCreate/wait/teardown sequence is correct; GSI definition matches runtime.
Also applies to: 87-108, 214-224
backend/compact-connect/stacks/event_state_stack/event_state_table.py (1)
32-44: Table definition matches project standardsCMK encryption, TTL, GSI, and removal policy are consistent with other stacks.
Also applies to: 46-63
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (2)
58-61: Use of dynamic SQS messageId improves test robustnessEliminates brittle coupling to fixed IDs; aligns with batch failure references.
1308-1310: Batch failure assertions updated correctly to use actual messageIdPrevents false negatives when messageId changes per run.
Also applies to: 1995-1997, 2231-2233, 2682-2684
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
96-113: Use KeyConditionExpression builder, not a string, in Table.queryboto3 Table.query expects a ConditionExpression (e.g., Key('pk').eq(...)), not a raw string. Current code risks ParamValidationError.
+from boto3.dynamodb.conditions import Key @@ - response = self.config.event_state_table.query( - KeyConditionExpression='pk = :pk', - ExpressionAttributeValues={':pk': pk}, - ConsistentRead=True, - ) + response = self.config.event_state_table.query( + KeyConditionExpression=Key('pk').eq(pk), + ConsistentRead=True, + )⛔ Skipped due to learnings
Learnt from: landonshumway-ia Repo: csg-org/CompactConnect PR: 1014 File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/account_recovery.py:34-45 Timestamp: 2025-08-20T20:38:42.101Z Learning: boto3 DynamoDB Table.query() supports both string-based KeyConditionExpression with ExpressionAttributeValues and Key() condition objects from boto3.dynamodb.conditions. Both syntaxes are valid - string expressions like 'pk = :pk AND sk BETWEEN :start_sk AND :end_sk' with corresponding ExpressionAttributeValues work correctly and are documented in the official AWS documentation.Learnt from: landonshumway-ia Repo: csg-org/CompactConnect PR: 1014 File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/account_recovery.py:34-45 Timestamp: 2025-08-20T20:38:42.101Z Learning: boto3 DynamoDB Table.query() supports both string-based KeyConditionExpression with ExpressionAttributeValues and Key() condition objects from boto3.dynamodb.conditions. Both syntaxes are valid - string expressions like 'pk = :pk AND sk BETWEEN :start_sk AND :end_sk' with corresponding ExpressionAttributeValues work correctly.backend/compact-connect/pipeline/backend_stage.py (1)
55-63: EventStateStack wiring to NotificationStack is correct and properly implementedThe verification confirms:
- EventStateStack is instantiated at lines 55–63 before NotificationStack
- NotificationStack accepts
event_state_stackparameter (line 39 of notification_stack.py)- event_state_stack is correctly passed only when
hosted_zoneis present (line 162)- All required stacks from BackendStage are instantiated; optional stacks (notification_stack, reporting_stack) are properly gated by
hosted_zoneconditionNo changes needed.
|
@jlkravitz This is ready for your review. Thanks |
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/data-events/tests/function/__init__.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
Show resolved
Hide resolved
This will allow us to use it for other notification types
landonshumway-ia
force-pushed
the
feat/enhance-encumbrance-notifications
branch
from
8518d62 to
4b08d4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
48-53: Consider renaming to avoid Protocol name collision.The Protocol name
JurisdictionNotificationMethodis also defined ininvestigation_events.py(lines 12-17) with a different signature (usingInvestigationNotificationTemplateVariablesinstead ofEncumbranceNotificationTemplateVariables). This name collision could cause confusion when both are imported in the same scope.Consider renaming to be more specific, e.g.:
EncumbranceJurisdictionNotificationMethod(here)InvestigationJurisdictionNotificationMethod(in investigation_events.py)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
66-70: Consider simplifying SK construction to avoid trailing separator.The SK construction creates
NOTIFICATION#provider#for provider notifications (with trailing#) versusNOTIFICATION#state#{jurisdiction}for state notifications. While consistent with theshould_send_*methods, you could simplify by omitting the trailing#for provider notifications:-sk = f'NOTIFICATION#{recipient_type}#{jurisdiction or ""}' +sk = f'NOTIFICATION#{recipient_type}#{f"#{jurisdiction}" if jurisdiction else ""}'And update the corresponding checks in
NotificationTracker:-sk = f'NOTIFICATION#{RecipientType.PROVIDER}#' +sk = f'NOTIFICATION#{RecipientType.PROVIDER}'backend/compact-connect/stacks/notification_stack.py (1)
44-44: Remove unused instance attribute assignment.
self.event_state_stackis assigned but never used, since all methods receiveevent_state_stackas an explicit parameter. Per the past review feedback preferring explicit parameter passing for better dependency tracing, you can remove this line.-self.event_state_stack = event_state_stackBased on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backend/compact-connect/lambdas/python/common/cc_common/config.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/utils.py(2 hunks)backend/compact-connect/lambdas/python/common/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/function/__init__.py(5 hunks)backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py(1 hunks)backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py(1 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(23 hunks)backend/compact-connect/lambdas/python/data-events/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/__init__.py(3 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(8 hunks)backend/compact-connect/pipeline/backend_stage.py(3 hunks)backend/compact-connect/stacks/disaster_recovery_stack/__init__.py(1 hunks)backend/compact-connect/stacks/event_state_stack/__init__.py(1 hunks)backend/compact-connect/stacks/event_state_stack/event_state_table.py(1 hunks)backend/compact-connect/stacks/notification_stack.py(15 hunks)backend/compact-connect/stacks/persistent_stack/__init__.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/compact-connect/lambdas/python/data-events/tests/init.py
- backend/compact-connect/lambdas/python/common/tests/function/test_event_state_client.py
- backend/compact-connect/lambdas/python/common/tests/function/init.py
- backend/compact-connect/pipeline/backend_stage.py
- backend/compact-connect/stacks/event_state_stack/event_state_table.py
- backend/compact-connect/stacks/event_state_stack/init.py
- backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py
🧰 Additional context used
🧠 Learnings (19)
📚 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/data-events/tests/function/__init__.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/data-events/tests/function/__init__.py
📚 Learning: 2025-07-21T20:40:56.491Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 907
File: backend/compact-connect/lambdas/python/common/requirements.txt:7-0
Timestamp: 2025-07-21T20:40:56.491Z
Learning: In CompactConnect, there is only one lambda layer in use for Python lambdas, and this single layer manages the versions of aws-lambda-powertools, boto3, and botocore dependencies. This eliminates concerns about version skew across multiple lambda layers since all Python lambdas share the same dependency management through this single layer.
Applied to files:
backend/compact-connect/lambdas/python/data-events/tests/function/__init__.py
📚 Learning: 2025-08-12T19:23:01.151Z
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:282-296
Timestamp: 2025-08-12T19:23:01.151Z
Learning: In the CompactConnect disaster recovery system, table name validation (tableNameRecoveryConfirmation) is performed at the downstream Lambda function level (in the cleanup_records and copy_records handlers) rather than at the Step Function level, ensuring destructive operations fail safely if the confirmation doesn't match the intended destination table.
Applied to files:
backend/compact-connect/stacks/disaster_recovery_stack/__init__.py
📚 Learning: 2025-08-12T19:51:53.245Z
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:52-71
Timestamp: 2025-08-12T19:51:53.245Z
Learning: The CompactConnect disaster recovery system uses input validation (tableNameRecoveryConfirmation) rather than resource-based policies for security. Admins must provide a confirmation field matching the destination table name in the Step Function input to prevent accidental execution of the DR workflow.
Applied to files:
backend/compact-connect/stacks/disaster_recovery_stack/__init__.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/data-events/tests/function/test_encumbrance_events.pybackend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
📚 Learning: 2025-06-30T17:30:31.252Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 864
File: backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py:866-866
Timestamp: 2025-06-30T17:30:31.252Z
Learning: In the CompactConnect project, the `sqs_handler` decorator transforms Lambda functions to accept `(event, context)` parameters and automatically handles iteration over `event['Records']`, parsing JSON from each record's body, and managing SQS batch item failures. Functions decorated with `sqs_handler` have their underlying implementation accept a single `message: dict` parameter, but the decorator makes them callable with standard Lambda `(event, context)` signature. Tests should call these decorated functions with `(event, context)` parameters.
Applied to files:
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.pybackend/compact-connect/lambdas/python/common/cc_common/utils.pybackend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.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/common/cc_common/utils.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/common/cc_common/utils.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/common/cc_common/config.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/data-events/handlers/encumbrance_events.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/data-events/handlers/encumbrance_events.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/data-events/handlers/encumbrance_events.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/data-events/handlers/encumbrance_events.py
📚 Learning: 2025-05-30T13:57:36.286Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:1357-1490
Timestamp: 2025-05-30T13:57:36.286Z
Learning: CCInternalException is the appropriate exception type to use when adverse action records exist without corresponding license or privilege records, as this indicates a data consistency issue that should fire alerts.
Applied to files:
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
📚 Learning: 2025-07-21T20:37:29.741Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 907
File: backend/compact-connect/common_constructs/backup_plan.py:47-47
Timestamp: 2025-07-21T20:37:29.741Z
Learning: In CompactConnect, landonshumway-ia prefers to manage all backup configuration through Infrastructure as Code (CDK/CloudFormation) and does not want to support console-based modifications of backup rules. This aligns with best practices for infrastructure management and means AWS Console limitations (like 50-character rule name limits) are not a concern since console editing is not intended to be supported.
Applied to files:
backend/compact-connect/stacks/persistent_stack/__init__.py
📚 Learning: 2025-10-09T21:11:36.590Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1143
File: backend/compact-connect/tests/app/base.py:514-530
Timestamp: 2025-10-09T21:11:36.590Z
Learning: In the BackendStage class within the CompactConnect backend infrastructure, the following stacks are always required and their absence indicates a serious error: api_lambda_stack, api_stack, disaster_recovery_stack, event_listener_stack, feature_flag_stack, ingest_stack, managed_login_stack, persistent_stack, provider_users_stack, state_api_stack, state_auth_stack, and transaction_monitoring_stack. Only backup_infrastructure_stack can be None (when backups are disabled). The reporting_stack and notification_stack are conditionally present based on hosted_zone configuration.
Applied to files:
backend/compact-connect/stacks/notification_stack.py
📚 Learning: 2025-06-18T19:52:23.159Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 864
File: backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py:220-526
Timestamp: 2025-06-18T19:52:23.159Z
Learning: The user prefers pragmatic solutions that directly address the specific problem rather than structural refactoring that doesn't provide real value. When dealing with parameter bloat warnings, they favor keyword-only arguments over dataclass wrappers to prevent positional argument misuse without over-engineering.
Applied to files:
backend/compact-connect/stacks/notification_stack.py
📚 Learning: 2025-08-18T17:04:16.686Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1001
File: backend/compact-connect/stacks/disaster_recovery_stack/__init__.py:42-56
Timestamp: 2025-08-18T17:04:16.686Z
Learning: In CDK stacks, KMS key policies with restrictive conditions like kms:EncryptionContext:aws:logs:arn are incompatible with auto-generated log group resources because the exact ARN isn't known at policy creation time. Regional service principals (e.g., logs.region.amazonaws.com) provide sufficient security for CloudWatch Logs encryption without creating deployment failures.
Applied to files:
backend/compact-connect/stacks/notification_stack.py
🧬 Code graph analysis (8)
backend/compact-connect/stacks/disaster_recovery_stack/__init__.py (1)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
ssn_table(92-93)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
backend/compact-connect/lambdas/python/data-events/handlers/investigation_events.py (1)
JurisdictionNotificationMethod(13-18)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (3)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (6)
EventType(23-29)NotificationTracker(115-237)RecipientType(9-13)record_success(151-192)record_failure(194-237)NotificationStatus(16-20)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (1)
license_encumbrance_notification_listener(629-711)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (3)
put_default_provider_record_in_provider_table(440-460)put_default_license_record_in_provider_table(269-279)put_default_privilege_record_in_provider_table(341-351)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/investigation/__init__.py (2)
compact(71-72)compact(75-76)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
event_state_client(294-297)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
NotificationTracker(115-237)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
EventStateClient(32-112)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (4)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (3)
EncumbranceNotificationTemplateVariables(14-24)JurisdictionNotificationMethod(48-53)ProviderNotificationMethod(40-45)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (7)
EventType(23-29)NotificationTracker(115-237)RecipientType(9-13)should_send_provider_notification(132-139)record_success(151-192)record_failure(194-237)should_send_state_notification(141-149)backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
sqs_handler(433-468)sqs_handler_with_notification_tracking(471-530)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
compactConnectRegisteredEmailAddress(77-83)ProviderData(13-148)
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
backend/compact-connect/lambdas/python/common/cc_common/config.py (3)
_Config(21-335)event_state_table(290-291)event_state_client(294-297)
backend/compact-connect/stacks/notification_stack.py (4)
backend/compact-connect/stacks/event_state_stack/__init__.py (1)
EventStateStack(9-36)backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
environment_name(100-101)event_state_table(290-291)backend/compact-connect/common_constructs/ssm_parameter_utility.py (2)
SSMParameterUtility(8-43)load_data_event_bus_from_ssm_parameter(26-43)backend/compact-connect/stacks/persistent_stack/__init__.py (1)
PersistentStack(40-499)
⏰ 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 (13)
backend/compact-connect/stacks/disaster_recovery_stack/__init__.py (1)
76-78: Formatting tweak looks goodSplit call preserves the same keyword argument and value; no behavioral change and improves readability.
backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (2)
9-30: LGTM!The enums are well-defined and using
StrEnumis appropriate for DynamoDB storage compatibility.
115-237: LGTM! Defensive exception handling is appropriate.The
NotificationTrackerimplementation correctly provides idempotency checks and gracefully handles tracking failures without blocking notification delivery. The broad exception catching inrecord_successandrecord_failure(lines 180, 226) is well-justified by the comments explaining this is a redundancy layer.backend/compact-connect/stacks/notification_stack.py (2)
172-233: LGTM!The environment variable and permissions for the event state table are correctly configured. The IAM suppression reason accurately reflects the resources accessed.
235-345: LGTM!All listener methods are consistently updated to accept and pass through the
event_state_stackparameter.backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (8)
9-19: LGTM!Import additions are appropriate for the notification tracking functionality.
54-118: LGTM! Idempotency and PII concerns properly addressed.The notification tracking implementation correctly prevents duplicate sends and records success/failure. The previous concern about logging PII (provider_email) has been resolved by using provider_id instead.
120-188: LGTM!State notification tracking is correctly implemented with per-jurisdiction idempotency checks.
190-284: LGTM!The per-jurisdiction iteration with idempotency checks is correctly implemented, and constructing template variables outside the loop is a good optimization.
411-495: LGTM!The privilege encumbrance notification listener correctly implements idempotent notification delivery using the tracker and properly propagates event metadata.
497-626: LGTM!The privilege encumbrance lifting listener correctly validates record state, determines the latest effective lift date, and implements idempotent notifications. Error handling for missing records appropriately raises
CCInternalExceptionper the established pattern.
628-712: LGTM!The license encumbrance notification listener correctly implements idempotent notification delivery with proper event metadata propagation.
714-822: LGTM!The license encumbrance lifting listener correctly validates license state and implements idempotent notifications. The error handling for missing records is appropriate.
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
@isabeleliassen Good to merge!
4 participants
Given the importance of encumbrance notifications in the system, we have identified the following enhancements that can be made:
Track which emails have been successfully sent and which have not for encumbrance notifications (do not resend successful notifications)
Alert of all email notification failures (custom alert for node.js email service lambda for any failures)
Description List
Closes #1142
Summary by CodeRabbit
New Features
Improvements
Infrastructure
Monitoring
Tests