Feat/purchase email #733
Feat/purchase email #733
Conversation
## Walkthrough
This update introduces a complete event-driven notification system for privilege purchases. It adds infrastructure for a notification stack, event bus clients, and handlers that process privilege purchase events, send detailed email confirmations to providers, and monitor failures. Enhancements span infrastructure, backend logic, and email formatting, with comprehensive tests validating the new flows.
## Changes
| File(s) | Change Summary |
|---------|---------------|
| `backend/compact-connect/stacks/notification_stack.py` | Introduced `NotificationStack` for event-driven privilege purchase notifications, including Lambda processing, SQS queueing, alarms, and EventBridge integration. |
| `backend/compact-connect/pipeline/backend_stage.py` | Instantiates `NotificationStack` alongside `ReportingStack` when a hosted zone is present. |
| `backend/compact-connect/stacks/api_stack/v1_api/api.py`, `provider_management.py`, `purchases.py` | Injects and wires up `data_event_bus` to API stacks and handlers, enabling event publishing. |
| `backend/compact-connect/common_constructs/queued_lambda_processor.py` | Adds configurable DLQ alarm threshold to processor. |
| `backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py` | New module: Implements `EventBusClient` for publishing privilege-related events. |
| `backend/compact-connect/lambdas/python/common/cc_common/config.py` | Adds cached property to provide `EventBusClient` instance. |
| `backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py` | Adds method to send privilege purchase notification emails. |
| `backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py` | `create_provider_privileges` now returns summary dicts of created privileges. |
| `backend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.py` | Adds SQS handler for privilege purchase messages, sends notification emails, handles errors. |
| `backend/compact-connect/lambdas/python/purchases/handlers/privileges.py` | Publishes privilege purchase, issued, and renewed events after successful transactions. |
| `backend/compact-connect/lambdas/python/purchases/purchase_client.py` | Serializes line items as strings in responses for downstream compatibility. |
| `backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts`, `lib/email/email-notification-service.ts`, `lib/email/base-email-service.ts` | Implements new email template and logic for privilege purchase notifications, with structured privilege and cost breakdown. |
| `backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts` | Adds tests for privilege purchase notification email logic and error handling. |
| `backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py` | Adds assertions for returned privilege data from `create_provider_privileges`. |
| `backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py`, `unit/test_purchase_client.py`, `tests/function/__init__.py`, `tests/__init__.py` | Updates and adds tests for line item details, event bus setup, and privilege purchase flows. |
| `backend/compact-connect/lambdas/python/common/tests/unit/test_event_batch_writer.py`, `provider-data-v1/handlers/bulk_upload.py`, `provider-data-v1/handlers/ingest.py` | Updates imports for `EventBatchWriter`. |
| `backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/...` | Minor whitespace/formatting changes only. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant PurchasesAPI
participant LambdaHandler
participant EventBus
participant NotificationStack
participant SQS
participant PrivilegePurchaseHandler
participant EmailService
User->>PurchasesAPI: POST /purchase-privileges
PurchasesAPI->>LambdaHandler: Handle purchase
LambdaHandler->>EventBus: Publish privilege.purchase event
EventBus->>SQS: Route event (via EventBridge rule)
SQS->>PrivilegePurchaseHandler: Deliver message
PrivilegePurchaseHandler->>EmailService: Send privilege purchase email
EmailService-->>PrivilegePurchaseHandler: Email sentAssessment against linked issues
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
187-219: LGTM - Well-implemented privilege purchase email notificationThe implementation follows the established pattern in this class for email sending and properly structures the template variables needed for privilege purchase notifications.
Consider adding unit tests to verify this new method works correctly with various inputs, including edge cases like:
- Empty privileges list
- Missing cost line items
- Very large cost values
backend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.py (1)
125-164: New SQS handler correctly processes privilege purchase notifications.The handler follows proper patterns for event processing and error handling, which will ensure reliable notification delivery with appropriate retry mechanisms.
However, there's a misleading error message that should be fixed:
- error_message = f'Failed to send jurisdiction privilege purchase notification: {str(e)}' + error_message = f'Failed to send provider privilege purchase notification: {str(e)}'Line 157 refers to a "jurisdiction" notification, but this handler is actually sending a notification to the provider.
backend/compact-connect/lambdas/python/purchases/handlers/privileges.py (2)
296-297: Remove unnecessary comment mark.This change to the docstring appears unintentional and should be reverted.
- #
315-319: Consider using sum() with a generator for more concise total calculation.The total cost calculation is functionally correct but could be more concise using Python's built-in functions.
- total_cost = 0 - for line_item in cost_line_items: - total_cost = total_cost + float(line_item['unitPrice']) * int(line_item['quantity']) + total_cost = sum(float(item['unitPrice']) * int(item['quantity']) for item in cost_line_items)backend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py (2)
234-255: Reduce duplication & improve maintainability of the new line-item testThe new test is valuable but it repeats a lot of set-up logic that already exists in surrounding tests (
mock_secrets_manager_client, privilege purchase invocation, etc.). Consider extracting the common arrange/act steps into a helper (or usingpytestfixtures if the project eventually migrates) and asserting the line-items via a data-driven loop:- # we check every line item of the object to ensure that the correct values are being set - self.assertEqual(2, len(response['lineItems'])) - # first line item is the jurisdiction fee - self.assertEqual('priv:aslp-oh-slp', response['lineItems'][0]['itemId']) - ... - self.assertEqual('Compact fee applied for each privilege purchased', - response['lineItems'][1]['description'], - ) + expected = [ + { + 'itemId': 'priv:aslp-oh-slp', + 'name': 'Ohio Compact Privilege', + 'unitPrice': '100', + 'quantity': '1', + 'description': 'Compact Privilege for Ohio', + }, + { + 'itemId': 'aslp-compact-fee', + 'name': 'ASLP Compact Fee', + 'unitPrice': '50.5', + 'quantity': '1', + 'description': 'Compact fee applied for each privilege purchased', + }, + ] + self.assertListEqual(expected, response['lineItems'])This keeps the signal-to-noise ratio high and makes future updates (e.g. additional attributes) less painful.
320-323: Avoid magic literals for duplicate-window seconds
'10'is now hard-coded in two places (implementation & test). A small constant (e.g.,DUPLICATE_WINDOW_SECONDS = '10') imported by both the app code and the test would guarantee they stay in sync and makes the reason for the value explicit.backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (2)
292-299:specificEmails[0]may beundefinedin loggerThe debug log is executed before the empty-recipient guard, so an empty array will still yield:
{ providerEmail: undefined }Move the log statement after the length check or log the entire array instead.
315-326: Price math silently drops decimals for quantity
parseInt(lineItem.quantity, 10)removes any fractional part, which is fine if quantity is always an integer but brittle otherwise. UsingNumber(quantity)conveys intent and works for'1','2', or'2.5'.In addition, consider
parseFloat/NumberonunitPricefor symmetry.backend/compact-connect/stacks/notification_stack.py (2)
34-41: Stale doc-string references “deactivation” instead of “purchase”The private helper’s doc-string mentions “privilege deactivation” even though the function wires the purchase flow. This could mislead future maintainers.
105-112: EventBridge rule lacks an explicit retry policyWhile the target queue provides durability, failed rule invocations are only alarmed on but not automatically retried. AWS allows setting
retry_attempts/max_event_ageon theSqsQueuetarget. Consider configuring them (e.g., 185 attempts over 24 h) to avoid message loss during transient failures.backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (1)
42-59: Enhance thepublish_privilege_purchase_eventfor improved traceability.Adding a correlation ID to the events would improve traceability across systems. Also, consider adding documentation for the parameters.
def publish_privilege_purchase_event( self, source: str, provider_email: str, transaction_date: datetime, privileges: list[dict], total_cost: str, cost_line_items: list[dict], + correlation_id: str = None, ): + """ + Publish a privilege purchase event to the event bus. + + Args: + source: The source of the event + provider_email: The email of the provider who purchased the privilege + transaction_date: The date of the transaction + privileges: List of privilege data dictionaries + total_cost: The total cost of the purchase + cost_line_items: List of cost line item dictionaries + correlation_id: Optional ID to correlate this event with other events in the system + """ event_detail = { 'providerEmail': provider_email, 'transactionDate': transaction_date.strftime('%Y-%m-%d'), 'privileges': privileges, 'totalCost': total_cost, 'costLineItems': cost_line_items, + 'correlationId': correlation_id or str(uuid.uuid4()), + 'timestamp': datetime.datetime.utcnow().isoformat(), } self._publish_event(source=source, detail_type='privilege.purchase', detail=event_detail)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (32)
backend/compact-connect/common_constructs/queued_lambda_processor.py(4 hunks)backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts(1 hunks)backend/compact-connect/lambdas/nodejs/lib/email/base-email-service.ts(2 hunks)backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/config.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/api.py(0 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py(0 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py(0 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/common.py(0 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/user/api.py(0 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/user/record.py(0 hunks)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py(2 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_event_batch_writer.py(6 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.py(2 hunks)backend/compact-connect/lambdas/python/purchases/handlers/privileges.py(3 hunks)backend/compact-connect/lambdas/python/purchases/purchase_client.py(2 hunks)backend/compact-connect/lambdas/python/purchases/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/__init__.py(1 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py(7 hunks)backend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py(2 hunks)backend/compact-connect/pipeline/backend_stage.py(3 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(4 hunks)backend/compact-connect/stacks/api_stack/v1_api/provider_management.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/purchases.py(7 hunks)backend/compact-connect/stacks/notification_stack.py(1 hunks)
💤 Files with no reviewable changes (6)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/api.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/user/api.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/common.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/user/record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py
🧰 Additional context used
🧬 Code Graph Analysis (9)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py (1)
backend/compact-connect/lambdas/python/common/cc_common/event_batch_writer.py (1)
EventBatchWriter(4-49)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py (1)
backend/compact-connect/lambdas/python/common/cc_common/event_batch_writer.py (1)
EventBatchWriter(4-49)
backend/compact-connect/lambdas/python/common/tests/unit/test_event_batch_writer.py (1)
backend/compact-connect/lambdas/python/common/cc_common/event_batch_writer.py (2)
EventBatchWriter(4-49)put_event(42-49)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (1)
EventBusClient(8-78)
backend/compact-connect/stacks/api_stack/v1_api/api.py (3)
backend/compact-connect/common_constructs/ssm_parameter_utility.py (2)
SSMParameterUtility(8-46)load_data_event_bus_from_ssm_parameter(26-46)backend/compact-connect/common_constructs/stack.py (1)
Stack(18-86)backend/compact-connect/stacks/persistent_stack/__init__.py (1)
get_list_of_compact_abbreviations(515-519)
backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
create_provider_privileges(310-483)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.py (4)
backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (2)
CCInternalException(31-32)CCInvalidRequestException(7-8)backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
sqs_handler(410-445)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
email_service_client(292-299)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
send_privilege_purchase_email(187-218)
backend/compact-connect/stacks/api_stack/v1_api/purchases.py (2)
backend/compact-connect/stacks/persistent_stack/event_bus.py (1)
EventBus(9-25)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
event_bus_name(82-83)
backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (1)
backend/compact-connect/stacks/persistent_stack/event_bus.py (1)
EventBus(9-25)
🔇 Additional comments (43)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py (1)
2-2: Skip formatting-only change.
The added blank line after the initial comment is purely stylistic and does not affect functionality.backend/compact-connect/lambdas/python/purchases/tests/function/__init__.py (1)
40-40: Event Bus mock correctly added to test setupThis addition properly creates a mock AWS EventBridge event bus for testing, which aligns with the PR's objective of introducing event-driven notifications for privilege purchases.
backend/compact-connect/lambdas/python/purchases/tests/__init__.py (1)
26-26: Environment variable correctly added for event bus testingAdding the EVENT_BUS_NAME environment variable ensures the test environment has all necessary configuration for the new event-driven functionality. This complements the mock event bus creation in the TstFunction class.
backend/compact-connect/lambdas/python/common/tests/unit/test_event_batch_writer.py (1)
13-13: Import path standardization improves code organizationThe import statements have been properly updated to use the fully qualified
cc_common.event_batch_writermodule path instead of a local import. This standardization improves code maintainability by centralizing the EventBatchWriter utility in the cc_common package.Also applies to: 41-41, 69-69, 96-96, 138-138, 173-173
backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py (1)
10-10: Import path standardization improves code organizationThe import statement has been properly updated to use the fully qualified
cc_common.event_batch_writermodule path instead of a local import. This change is consistent with the import standardization across the codebase and supports the event-driven notification system introduced in this PR.backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py (1)
11-11: Good refactoring to use absolute importChanging from a relative import to an absolute import from the
cc_commonpackage improves maintainability and aligns with the broader refactoring of event handling across the codebase.backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
301-305: Approve adding EventBusClient to configThis change properly adds a new cached property for the EventBusClient, which is a key component in the new event-driven notification architecture. The implementation correctly follows the same pattern as other cached properties in this class and imports the dependency inside the method to avoid circular imports.
backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py (2)
147-147: Good capturing of return value for validationThe test now properly captures the return value from
create_provider_privileges, which is necessary for subsequent assertions.
194-199: Approve assertions for returned privilege dataGreat addition of assertions to verify that the returned privilege data contains all expected fields (compact, providerId, jurisdiction, licenseTypeAbbrev, privilegeId). This validates the new functionality where the Data client returns privilege data upon creation, which is essential for the event-driven notification flow.
backend/compact-connect/stacks/api_stack/v1_api/api.py (3)
4-5: LGTM - Import new dependenciesThe addition of these imports supports the refactored event bus handling pattern.
32-33: Good refactoring to centralize event bus loadingCentralizing the event bus loading at the V1Api level prevents duplicate SSM parameter calls and provides better dependency management.
116-116: LGTM - Inject event bus to child componentsThe event bus is now explicitly passed to child components, which improves dependency management and testability.
Also applies to: 150-150
backend/compact-connect/lambdas/python/purchases/purchase_client.py (2)
462-476: LGTM - Properly sanitize line item data for serializationAppropriate conversion of line item data to string format ensures that the data can be safely serialized for event publishing and notification workflows.
479-479: LGTM - Include line items in transaction responseAdding line items to the transaction response supports the notification functionality for privilege purchases.
backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (1)
46-46: LGTM - Consistent with dependency injection patternAccepting the event bus as a parameter rather than loading it internally aligns with the refactoring in api.py and follows good dependency injection practices.
backend/compact-connect/common_constructs/queued_lambda_processor.py (4)
31-31: Good enhancement: Added configurable DLQ alarm thresholdAdding a configurable threshold for the dead letter queue alarm allows for fine-tuning the sensitivity based on different use cases while maintaining backward compatibility with the default value.
95-101: Code correctly passes the new parameter to the internal methodThe new parameter is properly forwarded to the
_add_queue_alarmsmethod, maintaining the expected flow of configuration values.
115-122: Good update to method signature with consistent defaultThe method signature is properly updated with the same default value as the constructor, ensuring consistent behavior when the method is called directly.
144-144: Parameter correctly applied to the alarm thresholdThe configurable threshold is properly used in the actual DLQ alarm creation, replacing the previous hardcoded value.
backend/compact-connect/pipeline/backend_stage.py (3)
7-7: Properly imported the new NotificationStackThe import statement is correctly added to include the new stack.
70-71: Clear documentation of dependenciesThe updated comment clearly explains that both notifications and reporting depend on having a valid domain for email functionality, which is helpful for understanding the conditional initialization.
73-82: Properly integrated the NotificationStackThe NotificationStack is correctly initialized with the same parameters as the ReportingStack, under the same condition (having a hosted zone), maintaining consistency in the infrastructure setup.
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (4)
497-497: Good addition of test suite for the new email notification featureAdding a dedicated test suite for privilege purchase notifications ensures proper test coverage for this new feature.
498-522: Complete and well-structured test fixtureThe sample event includes all necessary data: recipient email, transaction details, privileges, and cost breakdown, providing a comprehensive test case that matches real-world usage.
524-550: Thorough success case testingThe test verifies that the email is sent with the correct recipient, subject, and content structure, ensuring the notification works as expected.
552-559: Good error handling testTesting the empty recipients scenario ensures that the system properly handles error cases with appropriate error messages, preventing silent failures.
backend/compact-connect/stacks/api_stack/v1_api/purchases.py (5)
7-7: Added import for required EventBus classThe EventBus import is correctly added to support the new event-driven notification system.
28-28: Properly added EventBus parameter to constructorAdding the EventBus as a parameter follows good dependency injection practices, making dependencies explicit rather than implicitly created.
42-42: EventBus name correctly exposed to Lambda environmentThe event bus name is properly added to the Lambda environment variables, allowing the Lambda function to access it at runtime.
69-70: Consistently propagated EventBus to handler methodsThe EventBus parameter is properly propagated through all relevant methods, maintaining consistency in the parameter chain.
Also applies to: 101-102
122-122: Critical security permission granted for event publishingThe Lambda function is properly granted the specific permission needed to publish events to the event bus, following the principle of least privilege.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
347-348: Well-designed modification to return privilege data after creation.The enhancement to return a list of created privileges after the transaction completes is a clean and efficient approach. Rather than requiring downstream code to re-query the database, this provides immediate access to the necessary privilege details for event publishing.
Also applies to: 379-385, 483-484
backend/compact-connect/lambdas/python/purchases/handlers/privileges.py (2)
309-353: Well-implemented event publication system for privilege activities.The event publishing implementation is thorough and correctly handles different scenarios:
- Publishing a general purchase event with detailed line items and cost
- Distinguishing between new privileges (issued) and existing privileges (renewed)
- Including all necessary context data for downstream processing
This event-driven approach will enable proper decoupling between the purchase process and notification systems.
329-352:❓ Verification inconclusive
Verify event bus availability for reliable notification delivery.
The code carefully separates privileges into "renewed" and "issued" categories for accurate event publishing, which is excellent. However, there's no error handling around the event publishing calls.
Consider adding try/except blocks around the event publishing calls to ensure that a failure in event publication doesn't prevent the completion of the purchase transaction:
🏁 Script executed:
#!/bin/bash # Check if the event bus exists and has the right permissions aws events describe-event-bus --name $(grep -o "config.event_bus_name" backend/compact-connect/lambdas/python/purchases/handlers/privileges.py | head -1 | cut -d '"' -f 2) || echo "Event bus not found or not accessible"Length of output: 352
Ensure resilient event publishing
The separation of “issued” vs. “renewed” privileges is spot-on, but any failure calling the event bus today will bubble up and could interrupt the purchase flow. We also couldn’t verify the bus existence here (no AWS CLI in this sandbox), so please:
• Manually confirm that the configured event bus exists in your AWS account and that the Lambda role has permission to put events.
• Wrap eachpublish_privilege_*_eventcall in a try/except (or implement retries) so that a transient failure in the bus API won’t block the transaction.Example:
- config.event_bus_client.publish_privilege_issued_event( + try: + config.event_bus_client.publish_privilege_issued_event( source='post_purchase_privileges', provider_email=provider_email, date=transaction_date, privilege=privilege_jurisdiction_issued, - ) + ) + except Exception as e: + logger.error(f"Failed to publish issued-event for {privilege_jurisdiction_issued}: {e}")Repeat similarly for the renewed-event loop. This will ensure your purchase transaction completes even if the event bus is temporarily unavailable.
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py (3)
33-33: Well-defined mock data for testing line items.The mock line items data structure provides a good representation of the expected data format from the purchase client.
140-142: Thorough test updates to verify line item handling.The test updates properly verify that line items are included in the purchase response and correctly renamed the test method to reflect the expanded functionality being tested.
Also applies to: 249-262
611-614: Comprehensive edge case testing for transaction voiding.Good job ensuring that the transaction voiding functionality correctly handles the line items data structure, which is an important edge case to cover.
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (1)
327-333: Table takes a header string but never uses it
insertTwoColumnTable(emailContent, 'Cost breakdown', rows);passes a title that is ignored byinsertTwoColumnTable(per the helper’s implementation elsewhere). Either:
- Remove the unused first argument, or
- Make the helper render the caption/heading.
Leaving unused parameters invites confusion.
backend/compact-connect/stacks/notification_stack.py (1)
76-88: Alarm treats missing data as “NOT_BREACHING” — verify intentFor a critical notification flow, suppressing alarms when metrics are missing can hide failures (e.g., Lambda never triggered). Confirm that
TreatMissingData.NOT_BREACHINGis deliberate; most teams preferMISSINGorBREACHINGto surface silent failures.backend/compact-connect/lambdas/nodejs/lib/email/base-email-service.ts (4)
313-344: Good enhancement to support text alignment.The modification to
insertBodyto accept a text alignment parameter improves the flexibility of the method. The implementation is clean and includes a null check before applying the alignment.
346-411: Well-structured key-value tuple implementation.The
insertTuplemethod is a good addition for displaying key-value pairs with appropriate styling. The implementation follows the established patterns of the codebase.
413-440: Clear and reusable two-column table implementation.The
insertTwoColumnTablemethod is well-structured and leverages theinsertTwoColumnRowmethod for each row, promoting code reuse.
529-537: Good defensive programming in style application.Your null checks before applying the bold styling are an excellent practice to prevent runtime errors. This type of defensive programming is especially important in TypeScript when dealing with potentially undefined properties.
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (1)
277-291: Parameter order hampers DX & may cause misusePlacing an optional parameter (
specificEmailswith a default value) after required ones forces unnecessary code at call sites.🧰 Tools
🪛 GitHub Check: CheckLambdas
[failure] 290-290:
Unexpected trailing comma🪛 GitHub Actions: Check-Lambda-JS
[error] 290-290: ESLint: Unexpected trailing comma (comma-dangle)
🧹 Nitpick comments (6)
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (6)
269-276: Fix JSDoc typo for better code documentation.There's a typo in the JSDoc comment on line 273: "teh" should be "the".
- * @param privileges - The relevant privilege data necessary to generate teh email + * @param privileges - The relevant privilege data necessary to generate the email
319-319: Remove unnecessary empty line.There's an empty line at 319 that can be removed for cleaner code.
const unitPriceNum = Number(lineItem.unitPrice); - const quantityText = quantityNum > 1 ? `x ${quantityNum}` : '';
296-298: Improve error message specificity.The current error message when no recipients are found is generic. Consider making it more specific to help with debugging.
if (recipients.length === 0) { - throw new Error(`No recipients found`); + throw new Error(`No recipient emails specified for privilege purchase notification`); }
294-294: Consider parameter validation.The method directly assigns
specificEmailstorecipientswithout any validation on the input parameters. Consider adding validation for the other parameters as well.const recipients = specificEmails; + + // Validate required parameters + if (!transactionDate) { + throw new Error('Transaction date is required'); + } + if (!privileges || privileges.length === 0) { + throw new Error('At least one privilege is required'); + } + if (totalCost === undefined || totalCost < 0) { + throw new Error('Valid total cost is required'); + } + if (!costLineItems || costLineItems.length === 0) { + throw new Error('At least one cost line item is required'); + }
327-327: Use consistent formatting for currency values.Consider using a helper function for consistent currency formatting throughout the application.
- const totalCostDisplay = `$${totalCost.toFixed(2)}`; + const totalCostDisplay = this.formatCurrency(totalCost);Then add this helper method to your service class:
private formatCurrency(amount: number): string { return `$${amount.toFixed(2)}`; }This would ensure consistent currency formatting across all emails.
308-313: Consider extracting privilege formatting to a helper method.The privilege formatting logic could be extracted to improve readability and maintainability.
- privileges.forEach((privilege) => { - const titleText = `${privilege.licenseTypeAbbrev.toUpperCase()} - ${privilege.jurisdiction.toUpperCase()}`; - const privilegeIdText = `Privilege Id: ${privilege.privilegeId}`; - - this.insertTuple(emailContent, titleText, privilegeIdText); - }); + privileges.forEach((privilege) => { + this.insertPrivilegeDetail(emailContent, privilege); + });With a new helper method:
private insertPrivilegeDetail(template: any, privilege: { jurisdiction: string, licenseTypeAbbrev: string, privilegeId: string }) { const titleText = `${privilege.licenseTypeAbbrev.toUpperCase()} - ${privilege.jurisdiction.toUpperCase()}`; const privilegeIdText = `Privilege Id: ${privilege.privilegeId}`; this.insertTuple(template, titleText, privilegeIdText); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CheckLambdas
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts
[failure] 290-290:
Unexpected trailing comma
🪛 GitHub Actions: Check-Lambda-JS
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts
[error] 290-290: ESLint: Unexpected trailing comma (comma-dangle)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts
Outdated
Show resolved
Hide resolved
Requirements List
Description List
Testing List
backend/compact-connect/tests/unit/test_api.pyCloses #533
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores