Conversation
…ated process transaction function to use this
…ntinued processing
|
Caution Review failedThe pull request is closed. WalkthroughAdds a full monthly account reporting workflow: new Lambda handlers (cron trigger + SQS continuation), processing helpers in a monthly_reports layer (pagination, SFN retry, SQS continuation/DLQ), PDF generation and notification (Jinja2/xhtml2pdf + SES), SQS helper refactor, DynamoDB pagination helper, API endpoint for account transactions, infra updates, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant EB as EventBridge
participant Trigger as TriggerLambda
participant DB as DynamoDB
participant SFN as StepFunctions
participant SQS as ContinuationQueue
participant Process as ProcessLambda
participant DLQ as DeadLetterQueue
EB->>Trigger: scheduled event (monthly)
Trigger->>DB: Scan accounts (paginated)
loop per page
Trigger->>SFN: start_sfn_execution_with_retry (account batches)
alt nearing timeout
Trigger->>SQS: send_continuation_message (scan state / remaining accounts)
end
end
SQS->>Process: SQS continuation event
alt continuation_type == accounts_scan
Process->>DB: continue paginated scan
else continuation_type == batch_continuation
Process->>SFN: process remaining account batches
end
alt malformed / unknown
Process->>DLQ: send_message_to_sqs (bad message)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
tests/functions/monthly_reports/accounts/trigger_tests/test_app.py (1)
242-264: Verify environment variable restoration after test.The test removes the
CONTINUATION_QUEUE_URLenvironment variable and reloads the app module, but doesn't restore the original state afterwards, which could affect subsequent tests.Use a context manager or ensure proper cleanup:
def test_missing_queue_url( self, monthly_accounts_reports_app_with_mocks, monkeypatch ): # Store original value if it exists original_value = os.environ.get("CONTINUATION_QUEUE_URL") monkeypatch.delenv("CONTINUATION_QUEUE_URL", raising=False) reload(app) try: # ... test logic ... finally: # Restore original state if original_value: monkeypatch.setenv("CONTINUATION_QUEUE_URL", original_value) reload(app)
🧹 Nitpick comments (15)
tests/layers/monthly_reports/test_helpers.py (1)
34-37: Correct the test case comment.The comment on line 36 states "February 28th leap year" but 2023 is not a leap year.
( datetime.datetime(2023, 2, 28, 12, 0, 0, 0), "2023-01", - "February 28th leap year", + "February 28th non-leap year", ),tests/functions/transactions/process_transactions/test_app.py (1)
330-332: Fix typo in docstring.Line 330 contains "monthly_reports" which appears to be a copy-paste error and should be "reports".
- Test that when a business logic error occurs and updating transaction status fails, the record is sent to the DLQ and the handler monthly_reports a business logic failure. + Test that when a business logic error occurs and updating transaction status fails, the record is sent to the DLQ and the handler reports a business logic failure.tests/functions/monthly_reports/accounts/trigger_tests/test_app.py (1)
43-47: Consider simplifying the response body handling logic.The conditional check for
isinstance(response["body"], dict)appears in multiple test methods and suggests inconsistent response formatting from the Lambda handler.Extract this logic into a helper method to reduce duplication:
def _extract_response_body(response): return ( response["body"] if isinstance(response["body"], dict) else json.loads(response["body"]) )layers/python/helpers/dynamodb.py (1)
35-36: Consider enhancing parameter validation.The function accepts
Noneforscan_paramsbut doesn't validate thetableparameter, which could lead to unclear errors ifNoneis passed.Add validation for the required
tableparameter:if scan_params is None: scan_params = {} if not table: raise ValueError("table parameter is required")tests/layers/monthly_reports/test_responses.py (1)
18-25: Consider testing response body content as well.While the test verifies status codes, it doesn't validate the response body structure or message content, which could be valuable for ensuring complete response correctness.
Add assertions for response body content:
def test_create_response_status_codes(self, mock_logger, status, expected_code): metrics = { "processed_count": 1, "failed_starts_count": 0, "skipped_count": 0, "already_exists_count": 0, } result = create_response(metrics, status, mock_logger) assert result assert result.get("statusCode") == expected_code assert "body" in result assert "message" in result["body"] assert result["body"]["processed_count"] == 1layers/python/monthly_reports/monthly_reports/sfn.py (1)
22-24: Remove redundant error code checkThe error code is already extracted on line 20 and can be reused in the condition.
- if e.response["Error"]["Code"] == "ExecutionAlreadyExistsException": + if error_code == "ExecutionAlreadyExistsException": logger.info(f"SF execution {execution_name} already exists. Skipping.") return "already_exists"layers/python/monthly_reports/monthly_reports/responses.py (1)
18-26: Improve status code mapping for better maintainabilityConsider using a dictionary for status-to-code mapping and simplify the single-value check.
- if status in ["ERROR_NO_CONTINUATION_QUEUE", "CRITICAL_ERROR"]: - status_code = 500 - elif status in ["TIMEOUT_CONTINUATION"]: - status_code = 202 - elif status == "COMPLETED": - status_code = 200 - else: - status_code = 500 + status_code_map = { + "ERROR_NO_CONTINUATION_QUEUE": 500, + "CRITICAL_ERROR": 500, + "TIMEOUT_CONTINUATION": 202, + "COMPLETED": 200, + } + status_code = status_code_map.get(status, 500)layers/python/monthly_reports/monthly_reports/sqs.py (2)
8-23: Improve docstring and consider returning the send resultThe function should have a more descriptive docstring and could benefit from returning the result of the send operation for better error handling by callers.
def send_continuation_message( scan_params: Dict[str, Any], statement_period: str, remaining_accounts: Optional[List[Dict[str, Any]]], last_evaluated_key: Optional[Dict[str, Any]], continuation_type: str, sqs_endpoint: str, continuation_queue_url: str, aws_region: str, logger: Logger, -): - """Send continuation message to SQS""" +) -> bool: + """Send continuation message to SQS for resuming monthly report processing. + + Args: + scan_params: DynamoDB scan parameters for pagination + statement_period: The period for which reports are being generated + remaining_accounts: Optional list of accounts yet to be processed + last_evaluated_key: Optional DynamoDB pagination key + continuation_type: Type of continuation (e.g., 'accounts_scan') + sqs_endpoint: SQS endpoint URL + continuation_queue_url: URL of the continuation queue + aws_region: AWS region + logger: Logger instance + + Returns: + bool: True if message was sent successfully, False otherwise + """ if not continuation_queue_url: logger.error("Cannot send continuation message: CONTINUATION_QUEUE_URL not set") - return + return False
41-48: Return the result from send_message_to_sqsThe function should return the result to allow callers to handle failures.
- send_message_to_sqs( + return send_message_to_sqs( message=message_body, message_attributes=message_attributes, sqs_endpoint=sqs_endpoint, sqs_url=continuation_queue_url, aws_region=aws_region, logger=logger, )layers/python/helpers/sqs.py (1)
39-41: Update error message to reflect generic SQS usage.The error message still references "DLQ" but the function is now generic for any SQS queue.
- logger.error("DLQ URL not configured, cannot send message to DLQ") + logger.error("SQS URL not configured, cannot send message to SQS")functions/transactions/process_transactions/process_transactions/sqs.py (1)
25-63: Consider using enum for error types to improve maintainability.The hardcoded string comparisons for error types could be improved with an enum for better type safety and maintainability.
Create an enum for error types:
from enum import Enum class ErrorType(Enum): BUSINESS_LOGIC = "BusinessLogicError" TRANSACTION_SYSTEM = "TransactionSystemError" UNKNOWN = "UnknownError"Then update the function to use enum values for comparison.
functions/monthly_reports/accounts/process_pending_reports/process_pending_reports/app.py (1)
37-42: Consider raising an exception for critical missing environment variables.While logging a critical error is good, allowing the Lambda to continue with
accounts_table = Nonecould lead to runtime errors later. Consider raising an exception to fail fast during initialization.if ACCOUNTS_TABLE_NAME: accounts_table = dynamodb.Table(ACCOUNTS_TABLE_NAME) logger.debug(f"Initialized DynamoDB table: {ACCOUNTS_TABLE_NAME}") else: logger.critical("FATAL: ACCOUNTS_TABLE_NAME environment variable not set!") - accounts_table = None + raise ValueError("ACCOUNTS_TABLE_NAME environment variable is required")template.yml (1)
619-619: Add missing newline at end of file.The static analysis tool correctly identified that the file is missing a newline character at the end, which is a YAML formatting best practice.
Type: STANDARD +layers/python/monthly_reports/monthly_reports/processing.py (2)
66-80: Consider using a configuration object to reduce parameter countThe function has 11 parameters which makes it difficult to maintain and test. Consider grouping related parameters into a configuration object.
Example structure:
@dataclass class ProcessingConfig: sfn_client: Any state_machine_arn: str sqs_endpoint: str continuation_queue_url: str aws_region: str batch_size: int = 10 safety_buffer: int = 30
130-133: Simplify the flattening of remaining batchesThe nested loop can be replaced with a more Pythonic approach using itertools.
- remaining_accounts = [] - for remaining_batch in remaining_batches: - remaining_accounts.extend(remaining_batch) + from itertools import chain + remaining_accounts = list(chain.from_iterable(remaining_batches))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
functions/monthly_reports/accounts/process_pending_reports/process_pending_reports/app.py(1 hunks)functions/monthly_reports/accounts/trigger/trigger/app.py(1 hunks)functions/transactions/process_transactions/process_transactions/app.py(4 hunks)functions/transactions/process_transactions/process_transactions/sqs.py(1 hunks)layers/python/helpers/dynamodb.py(2 hunks)layers/python/helpers/sfn.py(1 hunks)layers/python/helpers/sqs.py(1 hunks)layers/python/monthly_reports/monthly_reports/helpers.py(1 hunks)layers/python/monthly_reports/monthly_reports/metrics.py(1 hunks)layers/python/monthly_reports/monthly_reports/processing.py(1 hunks)layers/python/monthly_reports/monthly_reports/responses.py(1 hunks)layers/python/monthly_reports/monthly_reports/sfn.py(1 hunks)layers/python/monthly_reports/monthly_reports/sqs.py(1 hunks)layers/python/monthly_reports/requirements.txt(1 hunks)pytest.ini(1 hunks)template.yml(5 hunks)tests/conftest.py(1 hunks)tests/functions/monthly_reports/accounts/process_pending_reports/conftest.py(1 hunks)tests/functions/monthly_reports/accounts/process_pending_reports/test_app.py(1 hunks)tests/functions/monthly_reports/accounts/trigger_tests/conftest.py(1 hunks)tests/functions/monthly_reports/accounts/trigger_tests/test_app.py(1 hunks)tests/functions/transactions/process_transactions/test_app.py(10 hunks)tests/layers/helpers/test_dynamodb.py(2 hunks)tests/layers/helpers/test_sfn.py(1 hunks)tests/layers/helpers/test_sqs.py(5 hunks)tests/layers/monthly_reports/test_helpers.py(1 hunks)tests/layers/monthly_reports/test_metrics.py(1 hunks)tests/layers/monthly_reports/test_processing.py(1 hunks)tests/layers/monthly_reports/test_responses.py(1 hunks)tests/layers/monthly_reports/test_sfn.py(1 hunks)tests/layers/monthly_reports/test_sqs.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (12)
layers/python/helpers/sfn.py (2)
layers/python/helpers/ses.py (1)
get_ses_client(8-20)tests/layers/helpers/test_ses.py (3)
test_get_ses_client_success(11-28)test_get_ses_client_exception(30-46)TestGetSesClient(9-46)
functions/transactions/process_transactions/process_transactions/app.py (1)
layers/python/helpers/sqs.py (1)
send_message_to_sqs(31-63)
tests/functions/monthly_reports/accounts/process_pending_reports/conftest.py (3)
tests/conftest.py (2)
dynamo_resource(60-69)mock_accounts_dynamo_table(113-135)tests/functions/accounts/get_accounts/conftest.py (1)
get_accounts_app_with_mocked_tables(70-92)tests/functions/cognito/post_sign_up/conftest.py (1)
app_with_mocked_accounts_table(10-28)
tests/functions/monthly_reports/accounts/trigger_tests/conftest.py (3)
tests/conftest.py (2)
dynamo_resource(60-69)mock_accounts_dynamo_table(113-135)tests/functions/accounts/get_accounts/conftest.py (1)
get_accounts_app_with_mocked_tables(70-92)tests/functions/cognito/post_sign_up/conftest.py (1)
app_with_mocked_accounts_table(10-28)
tests/layers/helpers/test_sfn.py (2)
layers/python/helpers/sfn.py (1)
get_sfn_client(5-12)tests/conftest.py (1)
mock_logger(284-288)
tests/layers/monthly_reports/test_responses.py (1)
tests/conftest.py (1)
mock_logger(284-288)
tests/layers/helpers/test_dynamodb.py (1)
tests/conftest.py (1)
mock_logger(284-288)
tests/layers/monthly_reports/test_sfn.py (1)
tests/conftest.py (2)
mock_logger(284-288)magic_mock_sfn_client(266-267)
functions/monthly_reports/accounts/process_pending_reports/process_pending_reports/app.py (1)
layers/python/helpers/sfn.py (1)
get_sfn_client(5-12)
functions/transactions/process_transactions/process_transactions/sqs.py (3)
tests/functions/transactions/process_transactions/conftest.py (1)
sample_event_with_records(40-64)functions/transactions/process_transactions/process_transactions/transaction_helpers.py (2)
process_single_transaction(67-148)update_transaction_status(21-64)functions/transactions/process_transactions/process_transactions/validation.py (1)
validate_transaction_data(13-61)
layers/python/monthly_reports/monthly_reports/sqs.py (1)
layers/python/helpers/sqs.py (1)
send_message_to_sqs(31-63)
layers/python/monthly_reports/monthly_reports/processing.py (1)
layers/python/monthly_reports/monthly_reports/sqs.py (1)
send_continuation_message(8-48)
🪛 YAMLlint (1.37.1)
template.yml
[error] 619-619: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (60)
layers/python/monthly_reports/monthly_reports/helpers.py (1)
4-11: LGTM - Correct implementation for calculating previous month period.The function correctly handles month boundaries and leap years by calculating the first day of the current month and subtracting one day. The use of UTC ensures consistent behaviour across time zones.
pytest.ini (1)
5-5: LGTM - Necessary addition for monthly reports testing.The addition of
layers/python/monthly_reportsto the pythonpath follows the established pattern and enables proper import resolution for the new monthly reports layer during testing.tests/layers/monthly_reports/test_helpers.py (1)
9-76: Excellent test coverage with comprehensive edge cases.The parametrized test covers a wide range of scenarios including leap years, month boundaries, and exact timing edge cases. The test structure is well-organised and provides clear descriptions for each test case.
tests/conftest.py (2)
257-263: LGTM! Consistent with existing AWS service fixtures.The
mock_sfn_clientfixture follows the established pattern of using moto'smock_aws()context manager, maintaining consistency with other AWS service fixtures in this file.
265-267: LGTM! Provides flexible mocking option.The
magic_mock_sfn_clientfixture offers a lightweight alternative to the full moto mock, which is useful for unit tests that don't require complex AWS service simulation.layers/python/monthly_reports/monthly_reports/metrics.py (2)
1-11: LGTM! Clean and well-structured metrics initialisation.The function provides a clear contract with predefined metric keys, making it easy to understand what metrics are tracked in the monthly reports system.
13-18: LGTM! Safe metric aggregation with key validation.The function correctly aggregates metrics and includes a safety check to only update keys that exist in the target dictionary, preventing accidental introduction of unexpected metrics.
tests/functions/transactions/process_transactions/test_app.py (9)
214-214: LGTM! Consistent refactoring from DLQ-specific to generic SQS messaging.The patch decorator correctly reflects the refactored function name from
send_dynamodb_record_to_dlqtosend_message_to_sqs.
271-271: LGTM! Function name consistently updated.The patch decorator properly reflects the renamed function.
318-318: LGTM! Consistent with the refactoring pattern.The function name change is applied consistently.
366-366: LGTM! Function name consistently updated.The patch decorator correctly uses the new function name.
408-408: LGTM! Consistent refactoring.The function name change is properly applied.
449-449: LGTM! Function name consistently updated.The patch decorator reflects the refactored function name.
489-489: LGTM! Consistent with the refactoring pattern.The function name change is applied correctly.
530-530: LGTM! Function name consistently updated.The patch decorator properly uses the new function name.
571-571: LGTM! Final function name update is consistent.The patch decorator correctly reflects the refactored function name.
tests/layers/monthly_reports/test_sfn.py (7)
1-8: LGTM! Clean imports and proper test setup.The imports are minimal and focused, importing only what's needed for the test module.
10-26: LGTM! Straightforward success test case.The test correctly verifies the happy path where Step Functions execution starts successfully on the first attempt.
27-44: LGTM! Proper handling of execution already exists scenario.The test correctly verifies that
ExecutionAlreadyExistsExceptionis handled gracefully and returns the appropriate status.
45-73: LGTM! Comprehensive retry logic testing.The parametrised test effectively covers all retryable error codes and verifies that retry logic works correctly with proper sleep mocking.
74-102: LGTM! Max retries exhaustion handling.The test properly verifies that the function raises an exception when maximum retries are exceeded, and correctly checks the number of attempts and sleep calls.
103-123: LGTM! Non-retryable error handling.The test correctly verifies that non-retryable errors are not retried and are immediately raised, with proper error logging.
124-146: LGTM! Custom retry limit verification.The test properly validates that custom
max_retriesvalues are respected and that the function stops retrying at the specified limit.tests/functions/monthly_reports/accounts/trigger_tests/conftest.py (2)
1-6: LGTM! Proper imports for fixture setup.The imports include all necessary modules for setting up the test fixture with app reloading and mocking.
8-29: LGTM! Well-structured fixture following established patterns.The fixture properly sets up environment variables, mocks AWS resources, and follows the same pattern used in other test fixtures throughout the codebase. The environment variable setup covers all necessary configuration for the monthly reports trigger app.
tests/functions/monthly_reports/accounts/trigger_tests/test_app.py (1)
150-159: Excellent test coverage for multi-page processing.The test properly simulates multiple DynamoDB scan results with
side_effectto verify pagination logic handles continuation tokens correctly.layers/python/helpers/dynamodb.py (1)
32-56: LGTM! Well-implemented paginated DynamoDB scan function.The function properly handles:
- Parameter validation and copying to avoid mutations
- Conditional index name inclusion
- Comprehensive error handling with logging
- Clear return structure for pagination continuation
The implementation follows DynamoDB best practices for paginated scanning.
tests/functions/monthly_reports/accounts/process_pending_reports/conftest.py (1)
10-31: LGTM! Fixture follows established patterns correctly.The fixture properly:
- Sets all required environment variables for the app
- Uses the established pattern of patching
boto3.resource- Reloads the app module to apply changes
- Configures the mocked DynamoDB table
- Yields the configured app instance
This is consistent with similar fixtures in the codebase such as
app_with_mocked_accounts_tableandget_accounts_app_with_mocked_tables.tests/layers/monthly_reports/test_responses.py (1)
8-28: LGTM! Comprehensive status code testing.The parameterized test effectively covers all expected status codes returned by the
create_responsefunction. The test structure is clean and follows good practices.functions/transactions/process_transactions/process_transactions/app.py (4)
8-10: LGTM! Clean refactoring of SQS messaging imports.The import changes properly reflect the transition from the specific
send_dynamodb_record_to_dlqfunction to the more generalsend_message_to_sqsfunction with supporting helper functions.
125-140: Excellent structured error handling with new SQS helpers.The refactored code properly uses
format_sqs_messageandget_message_attributesto create structured messages with appropriate metadata including error type, environment, and idempotency key.
163-174: Consistent error handling patterns across all error types.The refactoring maintains consistent error handling patterns across
TransactionSystemErrorand unknown errors, with proper message formatting and attribute inclusion.Also applies to: 185-198
143-157: No changes needed for idempotency key handling.
Theget_message_attributesfunction safely casts aNoneidempotency key toFalseand setsHasIdempotencyKeyto"False". PassingNonehere is expected and will not cause errors.layers/python/helpers/sqs.py (2)
31-38: LGTM! Function signature is well-designed for generic SQS usage.The refactored function signature provides better flexibility and reusability across different SQS messaging scenarios.
43-46: Good addition of message validation.The empty message check adds robustness to the function, ensuring meaningful messages are sent to SQS.
tests/layers/helpers/test_dynamodb.py (1)
73-140: Excellent test coverage for the new paginated function.The test class provides comprehensive coverage including:
- Basic success scenario
- Success with scan parameters
- Success with index name
- Error handling with ClientError
The use of proper mocking and assertions demonstrates good testing practices.
functions/transactions/process_transactions/process_transactions/sqs.py (1)
13-23: LGTM! Good use of base attributes pattern.The function correctly establishes common SQS message attributes with proper data types and current timestamp using UTC.
tests/functions/monthly_reports/accounts/process_pending_reports/test_app.py (1)
15-466: Excellent comprehensive test suite!This test module demonstrates outstanding testing practices:
- Comprehensive coverage of different continuation types (
accounts_scan,batch_continuation)- Edge cases handled (empty records, missing keys, invalid JSON)
- Error scenarios properly tested with exception propagation
- Environment variable testing with module reloading
- Proper mocking and assertion patterns throughout
The tests provide confidence in the Lambda handler's robustness across various scenarios.
tests/layers/monthly_reports/test_sqs.py (1)
6-57: Well-structured tests for continuation messaging.The test class effectively covers:
- Error handling when continuation queue URL is missing
- Successful message sending with proper message structure validation
- Correct message attributes verification
The use of mocking to isolate the SQS sending logic is appropriate and the assertions verify both the call patterns and data structures.
functions/monthly_reports/accounts/process_pending_reports/process_pending_reports/app.py (2)
61-102: LGTM! Well-structured continuation message processing.The conditional logic for handling different continuation types is clear and appropriate. The function correctly calls the relevant processing functions and merges metrics properly. The parameter passing to processing functions includes all necessary context and configuration.
104-108: LGTM! Proper error handling implementation.The error handling correctly logs the exception with stack trace and re-raises it, which is appropriate for Lambda functions to ensure proper error propagation and monitoring.
tests/layers/monthly_reports/test_metrics.py (2)
6-16: LGTM! Comprehensive test for metrics initialisation.The test properly verifies that
initialize_metrics()returns all expected metric keys initialized to zero. Good coverage of the function's behaviour.
19-128: LGTM! Excellent parameterised test coverage.The parameterised tests provide comprehensive coverage of the
merge_metricsfunction with well-designed test cases covering:
- Basic merging scenarios
- Edge cases (zeros, negatives, large numbers)
- Partial key overlaps
- Unknown key handling
- Identity preservation and in-place modification
The test implementation properly validates all aspects of the function behaviour including return value, object identity, and data integrity.
tests/layers/helpers/test_sqs.py (3)
5-5: LGTM! Updated import reflects refactoring to generic SQS function.The import change from a DLQ-specific function to the more generic
send_message_to_sqsimproves code reusability.
61-85: LGTM! Good edge case testing for invalid inputs.The new tests properly verify that the function handles edge cases (empty SQS URL and empty message) by returning
False, which is appropriate error handling behaviour.
87-172: LGTM! Tests properly updated for new function interface.The test implementations have been correctly updated to use the new
send_message_to_sqsfunction signature withmessageandmessage_attributesparameters. The tests maintain comprehensive coverage while properly reflecting the refactored function behaviour.tests/layers/monthly_reports/test_processing.py (6)
14-105: LGTM! Comprehensive testing of account batch processing.The test class provides excellent coverage of
process_account_batchincluding:
- Valid and invalid account handling
- Different execution results (success, already exists, failure)
- Exception handling scenarios
The mocking strategy appropriately isolates the function under test.
107-137: LGTM! Thorough testing of chunking utility.The tests provide comprehensive coverage of the
chunk_accountsfunction with appropriate edge cases including empty lists, various chunk sizes, and boundary conditions.
140-173: LGTM! Good integration testing for page processing.The test properly verifies that
process_accounts_pagecorrectly integrates with batch processing and metrics merging functionality.
176-298: LGTM! Excellent coverage of batch processing with timeout handling.The test class provides comprehensive coverage of
process_account_batchesincluding:
- Normal processing completion
- Timeout detection and continuation message sending
- Exception handling during batch processing
The timeout logic testing is particularly important for Lambda function reliability.
301-434: LGTM! Comprehensive testing of scan continuation logic.The test class thoroughly covers
process_accounts_scan_continuationwith scenarios including:
- Multi-page processing with pagination
- Timeout handling with continuation messages
- Empty result handling
- Proper metrics aggregation
This is crucial functionality for handling large account datasets.
437-566: LGTM! Thorough testing of continuation decision logic.The test class provides excellent coverage of
process_batch_continuationdecision logic, testing all combinations of:
- Remaining accounts present/absent
- Last evaluated key present/absent
- Proper function delegation based on these conditions
The tests verify the correct branching behaviour for continuation processing.
template.yml (6)
190-192: LGTM! Environment variables support SQS refactoring.The addition of
TRANSACTION_PROCESSING_DLQ_URLandSQS_ENDPOINTenvironment variables appropriately supports the refactoring to use the genericsend_message_to_sqsfunction.
342-381: LGTM! Well-configured monthly trigger function.The function configuration is appropriate for a monthly scheduled job:
- Proper timeout (300s) for potentially long-running operations
- Reserved concurrency of 1 prevents overlapping executions
- Correct cron schedule for monthly execution
- Appropriate permissions for required AWS services
383-422: LGTM! Properly configured SQS processing function.The function configuration is well-suited for SQS continuation message processing:
- Appropriate timeout (150s) for processing operations
- BatchSize of 1 ensures focused processing per invocation
- Correct SQS permissions for message handling
- Proper integration with Step Functions and DynamoDB
458-467: LGTM! Standard Lambda layer configuration.The Lambda layer configuration follows the established pattern and provides shared monthly reports functionality for the new Lambda functions.
579-585: LGTM! Well-configured continuation queue.The SQS queue configuration is appropriate for continuation processing:
- Visibility timeout (200s) exceeds Lambda timeout to prevent duplicate processing
- Long message retention (14 days) supports retry scenarios
- Long polling (20s) reduces costs and improves efficiency
611-618: LGTM! Minimal but functional state machine.The Step Functions state machine provides a basic success state that can be extended as needed for monthly report processing workflows.
layers/python/monthly_reports/monthly_reports/processing.py (2)
8-10: Clean implementation of chunking logicThe generator pattern is memory-efficient and the implementation is concise.
247-307: Well-structured continuation handlingThe function properly handles both batch continuation and scan continuation scenarios, with clear separation of concerns.
functions/monthly_reports/accounts/process_pending_reports/process_pending_reports/app.py
Show resolved
Hide resolved
functions/transactions/process_transactions/process_transactions/sqs.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
functions/monthly_reports/accounts/trigger/trigger/app.py (1)
37-43: Critical configuration issue remains unresolvedThe previous review comment about handling missing ACCOUNTS_TABLE_NAME is still valid. The code will fail at runtime when attempting to use
accounts_tableon line 88.
🧹 Nitpick comments (4)
tests/functions/transactions/process_transactions/test_sqs.py (1)
8-16: Consider adding positive test cases for comprehensive coverage.The current test only covers the error case when invalid input types are provided. Consider adding test cases that verify the function's behaviour with valid inputs to ensure comprehensive coverage of the
format_sqs_messagefunction.For example, you could add tests like:
def test_format_sqs_message_valid_input(self): record = { "dynamodb": { "ApproximateCreationDateTime": "2023-01-01T00:00:00Z", "SequenceNumber": "12345" } } result = format_sqs_message(record, "Test error") assert result["originalRecord"] == record assert result["errorMessage"] == "Test error" assert result["timestamp"] == "2023-01-01T00:00:00Z" assert result["sequenceNumber"] == "12345" def test_format_sqs_message_missing_dynamodb_fields(self): record = {} result = format_sqs_message(record) assert result["originalRecord"] == record assert result["errorMessage"] == "" assert result["timestamp"] is None assert result["sequenceNumber"] is Nonefunctions/monthly_reports/accounts/trigger/trigger/app.py (1)
124-125: Simplify DLQ condition checkSince
AWS_REGIONhas a default value of "eu-west-2" on line 25, checking it here is redundant.-if DLQ_URL and AWS_REGION: +if DLQ_URL:tests/functions/monthly_reports/accounts/process_pending_reports/test_app.py (1)
7-12: Consolidate imports from the same moduleCombine the imports from the same module for better readability.
-from functions.monthly_reports.accounts.process_pending_reports.process_pending_reports import ( - app, -) -from functions.monthly_reports.accounts.process_pending_reports.process_pending_reports.app import ( - lambda_handler, -) +from functions.monthly_reports.accounts.process_pending_reports.process_pending_reports import app +from functions.monthly_reports.accounts.process_pending_reports.process_pending_reports.app import lambda_handlertemplate.yml (1)
635-643: Add newline at end of file and consider state machine purpose
- Add a newline character at the end of the file to follow YAML best practices
- The state machine only has a Success state - verify this is intentional for the monthly reports workflow
Type: STANDARD +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
dev-requirements.txt(1 hunks)functions/accounts/get_accounts/requirements.txt(1 hunks)functions/auth/requirements.txt(1 hunks)functions/cognito/post_sign_up/requirements.txt(1 hunks)functions/monthly_reports/accounts/process_pending_reports/process_pending_reports/app.py(1 hunks)functions/monthly_reports/accounts/trigger/trigger/app.py(1 hunks)functions/transactions/get_transactions/requirements.txt(1 hunks)functions/transactions/process_transactions/process_transactions/sqs.py(1 hunks)functions/transactions/request_transaction/requirements.txt(1 hunks)layers/python/authentication/requirements.txt(1 hunks)layers/python/helpers/requirements.txt(1 hunks)layers/python/helpers/sfn.py(1 hunks)layers/python/helpers/sqs.py(1 hunks)layers/python/monthly_reports/monthly_reports/processing.py(1 hunks)layers/python/monthly_reports/monthly_reports/responses.py(1 hunks)layers/python/monthly_reports/monthly_reports/sfn.py(1 hunks)layers/python/monthly_reports/monthly_reports/sqs.py(1 hunks)layers/python/monthly_reports/requirements.txt(1 hunks)template.yml(5 hunks)tests/functions/monthly_reports/accounts/process_pending_reports/conftest.py(1 hunks)tests/functions/monthly_reports/accounts/process_pending_reports/test_app.py(1 hunks)tests/functions/monthly_reports/accounts/trigger_tests/conftest.py(1 hunks)tests/functions/monthly_reports/accounts/trigger_tests/test_app.py(1 hunks)tests/functions/transactions/process_transactions/test_sqs.py(1 hunks)tests/layers/helpers/test_sfn.py(1 hunks)tests/layers/monthly_reports/test_processing.py(1 hunks)tests/layers/monthly_reports/test_sqs.py(1 hunks)tests/requirements.txt(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- dev-requirements.txt
- layers/python/helpers/requirements.txt
- functions/transactions/request_transaction/requirements.txt
- functions/transactions/get_transactions/requirements.txt
- functions/auth/requirements.txt
- tests/requirements.txt
- functions/cognito/post_sign_up/requirements.txt
- functions/accounts/get_accounts/requirements.txt
- layers/python/authentication/requirements.txt
🚧 Files skipped from review as they are similar to previous changes (13)
- layers/python/monthly_reports/requirements.txt
- tests/layers/helpers/test_sfn.py
- tests/functions/monthly_reports/accounts/process_pending_reports/conftest.py
- tests/functions/monthly_reports/accounts/trigger_tests/test_app.py
- layers/python/helpers/sfn.py
- functions/transactions/process_transactions/process_transactions/sqs.py
- layers/python/monthly_reports/monthly_reports/responses.py
- layers/python/helpers/sqs.py
- functions/monthly_reports/accounts/process_pending_reports/process_pending_reports/app.py
- tests/functions/monthly_reports/accounts/trigger_tests/conftest.py
- tests/layers/monthly_reports/test_sqs.py
- layers/python/monthly_reports/monthly_reports/sfn.py
- layers/python/monthly_reports/monthly_reports/processing.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/functions/transactions/process_transactions/test_sqs.py (1)
functions/transactions/process_transactions/process_transactions/sqs.py (1)
format_sqs_message(4-12)
🪛 YAMLlint (1.37.1)
template.yml
[error] 643-643: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (6)
tests/functions/transactions/process_transactions/test_sqs.py (2)
1-5: LGTM!The import statements are clean and follow Python conventions. The function under test is correctly imported from the appropriate module.
8-9: LGTM!The test class follows standard naming conventions and is appropriately structured.
tests/functions/monthly_reports/accounts/process_pending_reports/test_app.py (1)
16-703: Well-structured and comprehensive test coverageThe test suite thoroughly covers various scenarios including success paths, error handling, timeout conditions, and edge cases. Good use of mocking and proper assertions.
layers/python/monthly_reports/monthly_reports/sqs.py (1)
83-96: Appropriate error handling for DLQ operationsGood practice to wrap DLQ sending in try-except to prevent cascading failures. The logging provides adequate visibility for both success and failure cases.
template.yml (1)
342-439: Well-configured Lambda functions for monthly reportsThe Lambda functions are properly configured with:
- Appropriate timeout values for batch processing
- Least-privilege IAM policies
- Correct monthly schedule trigger
- Proper SQS event source mapping
tests/layers/monthly_reports/test_processing.py (1)
1-655: Excellent test coverage for processing moduleThe test suite provides comprehensive coverage with:
- Well-organised test classes for each processing function
- Thorough edge case testing including timeouts, exceptions, and empty data
- Proper use of mocks to isolate dependencies
- Clear assertions validating expected behaviour
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
layers/python/monthly_reports/monthly_reports/sqs.py(1 hunks)tests/layers/monthly_reports/test_processing.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- layers/python/monthly_reports/monthly_reports/sqs.py
…count Signed-off-by: edinstance <edinstance@gmail.com>
…e monthly reports state machine Signed-off-by: edinstance <edinstance@gmail.com>
…th a dlq destination
Signed-off-by: edinstance <edinstance@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/functions/monthly_reports/accounts/create_report/test_generate_pdf.py (1)
348-386: Test name misleads; generator also leaves temp files behind.
- The test doesn’t assert cleanup; rename to reflect intent, or assert deletion.
- The generator writes to a NamedTemporaryFile(delete=False) and never removes it—this can leak disk across warm invocations.
Minimal test rename:
-def test_tempfile_cleanup(self, sample_event, mock_logger, mock_template_content): - """Test that temporary file is properly cleaned up.""" +def test_tempfile_creation_and_write(self, sample_event, mock_logger): + """Test that a temporary PDF is created and written to."""Consider updating the generator to unlink after logging:
# functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py # after logging PDF path: logger.info(f"PDF saved to: {pdf_file_path}") try: os.unlink(pdf_file_path) logger.debug(f"Temp PDF deleted: {pdf_file_path}") except OSError: logger.warning(f"Failed to delete temp PDF: {pdf_file_path}", exc_info=True)layers/python/helpers/ses.py (1)
23-54: Wrap and handle exceptions around send_user_email at each call site
- In send_report.py (around line 66): using
if responseis no longer effective—send_user_emailnow raises on failure. Wrap the call intry/except, catch the exception and map it to your"status": "failed"branch (or rethrow) instead of relying on a False return.- In post_sign_up/app.py (around lines 86–90): the
if not email_sent_successfullybranch will never execute because failures now raise. Either catch exceptions fromsend_user_emailto trigger your error logging/raising, or remove the redundant Boolean check and let the exception bubble to the existing outerexcept.
🧹 Nitpick comments (72)
functions/accounts/get_account_transactions/requirements.txt (1)
1-2: Consider avoiding bundling boto3 in each Lambda packageIncluding boto3 increases deployment artefact size and cold starts. Unless you require features newer than the runtime’s AWS SDK, prefer:
- relying on the managed boto3 in the Lambda runtime, or
- moving boto3 into a shared layer used by all functions that need it.
This also helps keep versions consistent across the repo.
functions/monthly_reports/accounts/notify_client/notify_client/date_helpers.py (2)
4-4: Name/behaviour mismatch: function returns True for current month as wellThe helper treats the current month as “in future”, which is fine if you only allow completed months, but the name is misleading. Either rename (e.g., period_is_current_or_future) or add a docstring to clarify intent.
Apply this minimal docstring:
def period_is_in_future(statement_period: str) -> bool: + """ + Returns True when the period is the current month or a future month (UTC). + Raises ValueError if statement_period is not 'YYYY-MM'. + """
5-8: Preserve original exception context for bad inputsChain the ValueError so callers get the root cause.
- try: - requested_date = datetime.datetime.strptime(statement_period, "%Y-%m") - except ValueError: - raise ValueError("Invalid statement_period format. Use 'YYYY-MM'.") + try: + requested_date = datetime.datetime.strptime(statement_period, "%Y-%m") + except ValueError as exc: + raise ValueError("Invalid statement_period format. Use 'YYYY-MM'.") from exclayers/python/authentication/authentication/user_details.py (3)
15-20: Avoid KeyError and keep traceback intact
- admin_get_user should include UserAttributes, but be defensive to prevent KeyError.
- Use plain
raiseto preserve the original traceback (instead ofraise e).- attrs = {attr["Name"]: attr["Value"] for attr in response["UserAttributes"]} + attrs = {attr["Name"]: attr["Value"] for attr in response.get("UserAttributes", [])} logger.info(f"Fetched attributes for user: {username}.") return attrs except Exception as e: logger.exception(f"Failed to fetch user {username} from Cognito") - raise e + raise
5-7: Tighten typing for returned attributesAnnotate the return type for clarity.
-def get_user_attributes( - aws_region: str, logger: Logger, username: str, user_pool_id: str -) -> dict: +def get_user_attributes( + aws_region: str, logger: Logger, username: str, user_pool_id: str +) -> dict[str, str]:
8-13: Reuse the Cognito client across invocationsCreating the client on every call adds latency and TLS connection churn. Prefer a module-level client (using AWS_REGION env var) or a small LRU cache keyed by region; alternatively inject the client for easier testing.
tests/functions/monthly_reports/accounts/notify_client/test_date_helpers.py (2)
12-16: Stabilise time to avoid month-boundary flakesThese tests can intermittently fail if they run across midnight UTC between computing the input string and the helper’s internal now(). Freeze time or monkeypatch the helper’s datetime.now to a fixed instant.
Example with pytest monkeypatch:
# at top of file import types def _freeze_now(monkeypatch, fixed): import functions.monthly_reports.accounts.notify_client.notify_client.date_helpers as dh class FakeDT(datetime.datetime): @classmethod def now(cls, tz=None): return fixed monkeypatch.setattr(dh, "datetime", types.SimpleNamespace( datetime=FakeDT, timezone=datetime.timezone ))Then in tests:
def test_period_is_current_month(self, monkeypatch): fixed = datetime.datetime(2025, 8, 15, tzinfo=datetime.timezone.utc) _freeze_now(monkeypatch, fixed) result = period_is_in_future(fixed.strftime("%Y-%m")) assert result is TrueAlso applies to: 17-22, 23-27
29-46: Minor: consolidate timezone usageYou mix datetime.UTC and datetime.timezone.utc. Pick one (prefer datetime.UTC in 3.11+) for consistency.
tests/conftest.py (5)
257-263: Clarify Step Functions fixture and ensure moto supportGood to have a real client, but moto’s Step Functions coverage can be patchy depending on version. If you only need basic start/describe calls this is fine; otherwise consider falling back to the MagicMock one or using mock_stepfunctions explicitly. Also add a short docstring and make scope explicit for consistency.
If you want, I can raise a follow-up to pin/verify the moto version and the SFN APIs you exercise.
Apply:
-@pytest.fixture -def mock_sfn_client(): - with mock_aws(): - client = boto3.client("stepfunctions", region_name=AWS_REGION) - - yield client +@pytest.fixture(scope="function") +def mock_sfn_client(): + """Mock AWS Step Functions client using moto.""" + with mock_aws(): + client = boto3.client("stepfunctions", region_name=AWS_REGION) + yield client
266-269: Add a docstring to match fixture styleMost fixtures here have short docstrings; add one for parity.
@pytest.fixture def magic_mock_ses_client(): - mock_client = MagicMock() + """Plain MagicMock SES client.""" + mock_client = MagicMock() return mock_client
279-283: Name clarity: this is a MagicMock, not a boto3 clientTo mirror magic_mock_ses_client and magic_mock_sfn_client, consider renaming to magic_mock_cognito_client (or add a docstring) to avoid confusion with cognito_client above that yields a real mocked boto3 client.
-@pytest.fixture -def mock_cognito_client(): - """Mock Cognito client for testing.""" - mock_client = MagicMock() - return mock_client +@pytest.fixture +def magic_mock_cognito_client(): + """Plain MagicMock Cognito client (not boto3).""" + mock_client = MagicMock() + return mock_client
285-287: Be explicit about scope and add a docstringAlign with other fixtures.
-@pytest.fixture(scope="function") -def magic_mock_sfn_client(): - return MagicMock() +@pytest.fixture(scope="function") +def magic_mock_sfn_client(): + """Plain MagicMock Step Functions client.""" + return MagicMock()
271-276: Rename duplicatemock_s3_clientfixture in create_report conftest
tests/functions/monthly_reports/accounts/create_report/conftest.py:32
Rename this fixture to avoid shadowing the root-levelmock_s3_client(e.g.mock_s3_client_for_create_report).functions/accounts/get_account_transactions/get_account_transactions/exceptions.py (1)
1-2: Add intent via docstring; optionally subclass ValueErrorA brief docstring helps, and since this signals invalid user input, subclassing ValueError can ease interoperability while remaining a distinct type.
-class ValidationError(Exception): - pass +class ValidationError(ValueError): + """Raised when request date parameters fail validation."""layers/python/helpers/s3.py (1)
6-13: Minor: add docstring; consider debug-level log and endpoint parity with SQS helperAdd a one-liner docstring for consistency. If you want parity with get_sqs_client, optionally switch to debug and accept an optional endpoint_url (useful for LocalStack). That would require updating tests.
Docstring-only (safe):
def get_s3_client(aws_region: str, logger: Logger): - try: + """Initialises and returns a boto3 S3 client for the specified AWS region.""" + try: client = boto3.client("s3", region_name=aws_region) logger.info("Initialized S3 client with default endpoint") return client except Exception: logger.error("Failed to initialize S3 client", exc_info=True) raiseOptional parity change (requires test updates):
-from logging import Logger +from logging import Logger +from typing import Optional @@ -def get_s3_client(aws_region: str, logger: Logger): +def get_s3_client(aws_region: str, logger: Logger, s3_endpoint: Optional[str] = None): try: - client = boto3.client("s3", region_name=aws_region) - logger.info("Initialized S3 client with default endpoint") + if s3_endpoint: + logger.debug(f"Initialized S3 client with endpoint {s3_endpoint}") + client = boto3.client("s3", region_name=aws_region, endpoint_url=s3_endpoint) + else: + logger.debug("Initialized S3 client with default endpoint") + client = boto3.client("s3", region_name=aws_region) return client except Exception: logger.error("Failed to initialize S3 client", exc_info=True) raisetests/layers/helpers/test_s3.py (2)
14-24: Patch the module under test, not the global boto3 symbolPatching s3.boto3.client avoids unintended cross-test coupling.
- with patch("boto3.client") as mock_boto3_client: + with patch("s3.boto3.client") as mock_boto3_client:
30-39: Same here; also assert no info log on failureKeeps the patch local and tightens the assertion.
- with patch("boto3.client") as mock_boto3_client: + with patch("s3.boto3.client") as mock_boto3_client: mock_boto3_client.side_effect = Exception("Connection error") @@ assert "Connection error" in str(exc_info.value) mock_logger.error.assert_called_once_with( "Failed to initialize S3 client", exc_info=True ) + mock_logger.info.assert_not_called()tests/functions/monthly_reports/accounts/create_report/test_exceptions.py (1)
1-171: Deduplicate via parametrisationThe assertions are repeated for each exception type. Parametrising will keep tests concise and easier to extend.
@@ -class TestReportExceptions: +class TestReportExceptions: @@ - def test_report_generation_error(self): - """Test ReportGenerationError exception.""" - error_message = "PDF generation failed" - error = ReportGenerationError(error_message) - - assert str(error) == error_message - assert isinstance(error, Exception) - assert isinstance(error, ReportGenerationError) + @pytest.mark.parametrize( + "exc_cls,msg", + [ + (ReportGenerationError, "PDF generation failed"), + (ReportTemplateError, "Template not found"), + (ReportUploadError, "S3 upload failed"), + ], + ) + def test_exception_str_and_type(self, exc_cls, msg): + err = exc_cls(msg) + assert str(err) == msg + assert isinstance(err, Exception) + assert isinstance(err, exc_cls) @@ - def test_exception_raising_and_catching(self): + @pytest.mark.parametrize( + "exc_cls,msg", + [ + (ReportGenerationError, "Test error message"), + (ReportTemplateError, "Test error message"), + (ReportUploadError, "Test error message"), + ], + ) + def test_exception_raising_and_catching(self, exc_cls, msg): """Test that exceptions can be raised and caught properly.""" - error_message = "Test error message" - - # Test ReportGenerationError - with pytest.raises(ReportGenerationError) as exc_info: - raise ReportGenerationError(error_message) - assert str(exc_info.value) == error_message - - # Test ReportTemplateError - with pytest.raises(ReportTemplateError) as exc_info: - raise ReportTemplateError(error_message) - assert str(exc_info.value) == error_message - - # Test ReportUploadError - with pytest.raises(ReportUploadError) as exc_info: - raise ReportUploadError(error_message) - assert str(exc_info.value) == error_message + with pytest.raises(exc_cls) as exc_info: + raise exc_cls(msg) + assert str(exc_info.value) == msgfunctions/monthly_reports/accounts/create_report/create_report/exceptions.py (1)
1-10: Introduce a common base to simplify catching/reportingDefine a shared base (e.g., ReportError) so callers can catch all report-related failures uniformly while still handling specifics when needed. Also export the public API via all.
+class ReportError(Exception): + """Base class for monthly report errors.""" + -class ReportGenerationError(Exception): +class ReportGenerationError(ReportError): """Raised when PDF generation fails.""" -class ReportTemplateError(Exception): +class ReportTemplateError(ReportError): """Raised when the Jinja2 template is missing or invalid.""" -class ReportUploadError(Exception): +class ReportUploadError(ReportError): """Raised when uploading to S3 fails.""" + +__all__ = [ + "ReportError", + "ReportGenerationError", + "ReportTemplateError", + "ReportUploadError", +]tests/layers/authentication/test_user_details.py (2)
57-81: Prefer asserting type/message over equality to exception objectComparing exceptions by equality can be brittle; assert the type and message instead.
- assert exception_info.value == expected_exception + assert isinstance(exception_info.value, Exception) + assert str(exception_info.value) == "Cognito service error"
103-127: Minor DRY: parametrize error-path tests and reuse a boto3 patch fixtureReduce repetition by:
- parametrising over (Exception, ClientError) paths
- centralising the boto3.client patch in a fixture
Example sketch:
@pytest.fixture def patched_cognito(mock_cognito_client): with patch("authentication.user_details.boto3.client") as m: m.return_value = mock_cognito_client yield m @pytest.mark.parametrize("exc_factory", [ lambda: Exception("Cognito service error"), lambda: ClientError({"Error": {"Code": "UserNotFoundException", "Message": "User does not exist"}}, "AdminGetUser"), ]) def test_errors(mock_cognito_client, mock_logger, patched_cognito, exc_factory): username = "test_user" mock_cognito_client.admin_get_user.side_effect = exc_factory() with pytest.raises(Exception): get_user_attributes(aws_region=TEST_AWS_REGION, logger=mock_logger, username=username, user_pool_id=TEST_USER_POOL_ID)functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py (2)
23-29: Validate required event fields early and fail fast with a clear errorProtect against KeyError and provide actionable logs if upstream payloads drift.
- html_out = template.render( - accountId=event["accountId"], - statementPeriod=event["statementPeriod"], - transactions=event["transactions"], - accountBalance=event["accountBalance"], + required = {"accountId", "statementPeriod", "transactions", "accountBalance"} + missing = sorted(k for k in required if k not in event) + if missing: + logger.error("Missing required event fields: %s", ", ".join(missing)) + raise ReportGenerationError(f"Missing event fields: {', '.join(missing)}") + + html_out = template.render( + accountId=event["accountId"], + statementPeriod=event["statementPeriod"], + transactions=event["transactions"], + accountBalance=event["accountBalance"], generationDate=datetime.now(timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC"), )
31-37: Surface underlying pisa errors to aid debuggingInclude pisa error count or message in logs/exceptions for quicker diagnosis.
- if pisa_status.err: - logger.error("xhtml2pdf failed to generate PDF") - raise ReportGenerationError("Error generating PDF") + if pisa_status.err: + logger.error("xhtml2pdf failed to generate PDF (errors=%s)", pisa_status.err) + raise ReportGenerationError(f"Error generating PDF (errors={pisa_status.err})")functions/accounts/get_account_transactions/get_account_transactions/transaction_helpers.py (2)
20-23: Reduce PII in logs (mask accountId or downgrade to debug).Consider masking accountId (e.g., last 4 chars) or logging at debug to avoid leaking identifiers in CloudWatch.
Example:
- logger.info( - f"Querying transactions for account {account_id} " - f"from {start_iso} to {end_iso} (period {statement_period})" - ) + masked = f"***{account_id[-4:]}" if account_id else "***" + logger.info( + f"Querying transactions for account {masked} " + f"from {start_iso} to {end_iso} (period {statement_period})" + )
7-15: Add a brief docstring and optional type hints for table for IDE/mypy help.Annotate table as a DynamoDB Table (if you use type stubs) and document params/returns.
tests/functions/accounts/get_account_transactions/test_transaction_helpers.py (1)
36-83: Add pagination and default/custom range tests to lock behaviour.
- Verify LastEvaluatedKey is surfaced and Limit/ExclusiveStartKey are passed through.
- Add tests for default last-month range (no period/start/end) and for custom start/end.
Apply:
@@ class TestQueryTransactions: @@ def test_query_transactions_no_items(self, mock_get_date_range): @@ assert result["transactions"] == [] + + @patch( + "functions.accounts.get_account_transactions.get_account_transactions.date_helpers.get_date_range" + ) + def test_query_transactions_pagination(self, mock_get_date_range): + mock_get_date_range.return_value = ( + "2024-09", + "2024-09-01T00:00:00Z", + "2024-09-30T23:59:59Z", + ) + mock_logger = MagicMock() + mock_table = MagicMock() + mock_table.query.return_value = { + "Items": [{"id": "txn1"}], + "LastEvaluatedKey": {"accountId": "acc999", "createdAt": "2024-09-15T12:00:00Z"}, + } + res = query_transactions( + table=mock_table, + account_id="acc999", + logger=mock_logger, + period="2024-09", + limit=50, + ) + kwargs = mock_table.query.call_args[1] + assert kwargs["Limit"] == 50 + assert "ExclusiveStartKey" not in kwargs + assert res["lastEvaluatedKey"] == {"accountId": "acc999", "createdAt": "2024-09-15T12:00:00Z"}tests/functions/monthly_reports/accounts/create_report/test_generate_pdf.py (3)
88-90: Patch the symbol where it’s used to avoid aliasing issues.Target the imported module path in generate_pdf to make the patch robust:
-with patch("xhtml2pdf.pisa.CreatePDF") as mock_pisa: +with patch("functions.monthly_reports.accounts.create_report.create_report.generate_pdf.pisa.CreatePDF") as mock_pisa:Also applies to: 180-182, 219-221, 279-281, 322-324, 364-366
43-69: Remove unused fixture and its parameters.The mock_template_content fixture isn’t used. Drop it and clean up test signatures.
- @pytest.fixture - def mock_template_content(self): - """Mock HTML template content.""" - return """ - <html> - <head><title>Account Statement</title></head> - <body> - <h1>Account Statement</h1> - <p>Account ID: {{ accountId }}</p> - <p>Statement Period: {{ statementPeriod }}</p> - <p>Account Balance: {{ accountBalance }}</p> - <p>Generated: {{ generationDate }}</p> - <table> - <tr><th>ID</th><th>Amount</th><th>Description</th><th>Date</th></tr> - {% for transaction in transactions %} - <tr> - <td>{{ transaction.id }}</td> - <td>{{ transaction.amount }}</td> - <td>{{ transaction.description }}</td> - <td>{{ transaction.date }}</td> - </tr> - {% endfor %} - </table> - </body> - </html> - """ + # Removed unused mock_template_content fixture-def test_successful_pdf_generation( - self, sample_event, mock_logger, mock_template_content -): +def test_successful_pdf_generation(self, sample_event, mock_logger):-def test_pdf_generation_error( - self, sample_event, mock_logger, mock_template_content -): +def test_pdf_generation_error(self, sample_event, mock_logger):-def test_empty_transactions(self, mock_logger, mock_template_content): +def test_empty_transactions(self, mock_logger):-def test_large_transactions_list(self, mock_logger, mock_template_content): +def test_large_transactions_list(self, mock_logger):-def test_generation_date_format( - self, sample_event, mock_logger, mock_template_content -): +def test_generation_date_format(self, sample_event, mock_logger):Also applies to: 70-73, 162-165, 194-196, 244-246, 305-306
74-76: Avoid patching os.path.dirname.It’s unnecessary for these tests and can mask genuine path issues. Drop the dirname patches; the Jinja environment is already fully mocked.
Also applies to: 136-138, 203-205, 263-265, 308-310, 350-352
functions/monthly_reports/accounts/create_report/create_report/app.py (4)
21-22: Don’t log the entire event (PII/size risk).Log identifiers; move the full payload (if needed) to debug.
-logger.info(f"Received event: {event}") +logger.append_keys( + accountId=event.get("accountId"), + userId=event.get("userId"), + statementPeriod=event.get("statementPeriod"), +) +logger.info("Received create-report request") +# logger.debug(f"Event payload: {event}") # optional
30-37: Harden S3 put with server-side encryption (configurable).If your bucket policy doesn’t enforce encryption, set SSE here.
- s3.put_object( + put_params = dict( Bucket=REPORTS_BUCKET, Key=s3_key, Body=pdf_bytes, ContentType="application/pdf", - ) + ) + kms_key_id = os.environ.get("REPORTS_KMS_KEY_ID") + if kms_key_id: + put_params.update( + {"ServerSideEncryption": "aws:kms", "SSEKMSKeyId": kms_key_id} + ) + s3.put_object(**put_params)
42-43: Fix log message grammar.-logger.info("Generating PDF uploaded to S3") +logger.info("Generated PDF uploaded to S3")
45-49: Make presigned URL TTL configurable.Expose via env var to tune without code changes.
- presigned_url = s3.generate_presigned_url( + expires = int(os.environ.get("REPORT_URL_TTL_SECONDS", "3600")) + presigned_url = s3.generate_presigned_url( "get_object", Params={"Bucket": REPORTS_BUCKET, "Key": s3_key}, - ExpiresIn=3600, + ExpiresIn=expires, )functions/accounts/get_account_transactions/get_account_transactions/date_helpers.py (3)
7-8: Add type hints and a brief docstring.Improves clarity and tooling.
-def get_date_range(period: str = None, start: str = None, end: str = None): +from typing import Optional, Tuple + +def get_date_range(period: Optional[str] = None, start: Optional[str] = None, end: Optional[str] = None) -> Tuple[str, str, str]: + """Return (statement_period, start_iso, end_iso) in UTC for a given monthly period or custom date range."""
41-43: Avoid broad exception catch for period parsing.Narrow to expected errors.
- except Exception: + except (ValueError, IndexError): raise ValidationError("Invalid period format, must be YYYY-MM")
47-64: Consider injecting ‘today’ for testability.Accept a now: datetime parameter (default utcnow) to remove the need for patching datetime in tests. If you prefer current approach, ignore.
tests/layers/helpers/test_ses.py (2)
74-81: Update docstring to match new return type.The function now returns the SES response dict, not True.
- Test that send_user_email sends an email with all parameters and logs success. - - Verifies that the SES client's send_email method is called with the correct arguments, the logger records a success message, and the function returns True. + Test that send_user_email sends an email with all parameters, logs success, and returns the SES response dict.
200-217: Remove duplicated assertions.The subject/body/filename checks are repeated.
- assert isinstance(kwargs["RawMessage"]["Data"], str) - raw_data = kwargs["RawMessage"]["Data"] - assert self.subject_data in raw_data - assert self.body_text in raw_data - assert f'filename="{self.attachment_filename}"' in raw_data - - assert self.subject_data in raw_data - assert self.body_text in raw_data - assert f'filename="{self.attachment_filename}"' in raw_data + assert isinstance(kwargs["RawMessage"]["Data"], str) + raw_data = kwargs["RawMessage"]["Data"] + assert self.subject_data in raw_data + assert self.body_text in raw_data + assert f'filename="{self.attachment_filename}"' in raw_datafunctions/monthly_reports/accounts/notify_client/notify_client/processing.py (2)
34-38: Validate recipient email address, not just presenceAvoid sending to malformed emails; cheaply validate via parseaddr and normalise name.
Apply this diff, and add the import in a separate comment below:
- recipient = user_attributes.get("email") - user_name = user_attributes.get("name", "Customer") - - if not recipient: - raise ValueError(f"User {user_id} has no email attribute in Cognito") + recipient = (user_attributes.get("email") or "").strip() + user_name = (user_attributes.get("name") or "Customer").strip() or "Customer" + + if not recipient or not parseaddr(recipient)[1]: + raise ValueError(f"User {user_id} has invalid or missing email in Cognito")
1-3: Add lightweight email parser import for validationRequired for the parseaddr check.
Apply this diff:
from aws_lambda_powertools import Logger from botocore.exceptions import ClientError +from email.utils import parseaddrtests/functions/accounts/get_account_transactions/conftest.py (1)
9-9: Make VALID_UUID deterministic for test stabilityRandomising at import time can make snapshots/logs flaky and complicate reproductions.
Apply this diff:
-VALID_UUID = str(uuid.uuid4()) +VALID_UUID = "11111111-1111-4111-8111-111111111111"tests/functions/monthly_reports/accounts/notify_client/conftest.py (3)
56-67: Use canonical Python Lambda context APIThe Python runtime exposes get_remaining_time_in_millis(); rename for fidelity.
Apply this diff:
- context.remaining_time_in_millis = lambda: 30000 + context.get_remaining_time_in_millis = lambda: 30000
2-2: Import patch for targeted AWS client/function stubsYou use MagicMock but also need patch to route AWS calls to your mocks.
Apply this diff:
-from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch
39-44: Ensure Cognito/SES paths are mocked to prevent live callsPatch the Cognito client construction and email send helpers within the notify flow to guarantee isolation.
Apply this diff:
- reload(app) - - app.s3 = mock_s3_client - - yield app + with patch("authentication.user_details.boto3.client", return_value=mock_cognito_client), \ + patch("functions.monthly_reports.accounts.notify_client.notify_client.send_report.send_user_email", return_value={"MessageId": "test-message-id-123"}), \ + patch("functions.monthly_reports.accounts.notify_client.notify_client.send_report.send_user_email_with_attachment", return_value={"MessageId": "test-message-id-456"}): + reload(app) + app.s3 = mock_s3_client + yield apptests/functions/monthly_reports/accounts/create_report/conftest.py (1)
31-36: Avoid duplicating mock_s3_client fixture across suitesThere’s already a mock_s3_client in tests/conftest.py; reuse it to reduce drift, unless this variant needs different behaviour.
functions/monthly_reports/accounts/create_report/create_report/template.html (5)
105-118: Add styles for additional/unknown statusesCurrent styles cover processed/pending/failed only. Transactions elsewhere use "COMPLETED"; without a class, the badge won’t be styled.
Add:
.status.failed { background: #fee2e2; color: #991b1b; } + .status.completed { + background: #d1fae5; + color: #065f46; + } + .status.unknown { + background: #e5e7eb; + color: #374151; + }
223-225: Missing .type-badge stylesThe markup uses a type-badge but no CSS exists, so it renders unstyled.
Add:
+ .type-badge { + display: inline-block; + padding: 2px 8px; + border-radius: 12px; + background: #eef2ff; + color: #1e3a8a; + font-size: 12px; + font-weight: 600; + text-transform: uppercase; + letter-spacing: .03em; + }
226-227: Avoid hard-coding currency symbolMake currency configurable (e.g., GBP/EUR/USD) and pass it from the renderer.
Apply:
- <td class="amount">£{{ "%.2f"|format(txn.amount) }}</td> + <td class="amount">{{ currency_symbol | default("£") }}{{ "%.2f"|format(txn.amount) }}</td>- <td class="amount"><strong>£{{ "%.2f"|format(accountBalance) }}</strong></td> + <td class="amount"><strong>{{ currency_symbol | default("£") }}{{ "%.2f"|format(accountBalance) }}</strong></td>Also applies to: 231-231
216-217: Safer ID truncationIf ids can be shorter than 8 chars or None, guard before appending “…”.
Apply:
- <span class="transaction-id">{{ txn.id[:8] }}...</span> + <span class="transaction-id">{{ (txn.id[:8] ~ '...') if txn.id and (txn.id | length) > 8 else (txn.id or '') }}</span>
170-175: Remove unused CSS rule or apply the class.account-info is never used; keep CSS lean.
Either remove this block or wrap the “Period” section in a div.account-info.
tests/functions/monthly_reports/accounts/notify_client/test_app.py (4)
39-43: Assert S3 get_object call for attachment pathVerifies we actually fetch the PDF when sending attachments.
Apply:
app.s3.head_object.assert_called_once_with( Bucket="test-reports-bucket", Key=f"{sample_event['accountId']}/{sample_event['statementPeriod']}.pdf", ) + app.s3.get_object.assert_called_once_with( + Bucket="test-reports-bucket", + Key=f"{sample_event['accountId']}/{sample_event['statementPeriod']}.pdf", + )
112-120: Prefer dynamic attachment filenameUsing the period in the filename helps recipients.
Apply:
- attachment_filename="statement.pdf", + attachment_filename=f"statement-{sample_event['statementPeriod']}.pdf",If you align code accordingly, I can update all affected assertions.
517-522: Return 400 for “period in future” (client error), not 500A future period is a validation issue; 400 makes the API contract clearer.
Apply:
- assert result["statusCode"] == 500 + assert result["statusCode"] == 400 - assert "Internal server error" in response_body.get("message", "") + assert "Statement period is in the future" in response_body.get("message", "")I can provide the corresponding app change to return BadRequestError.
363-406: Reduce duplication with parametrised API-GW testsAuth/ownership happy-path and 401 cases share setup; parametrize to shrink test code and speed CI.
I can push a parametric version if you’d like.
Also applies to: 408-457, 459-490
tests/functions/accounts/get_account_transactions/test_app.py (3)
117-126: Raise ValidationError from query_transactions, not table.queryKeep HTTP-layer tests focused; stub the helper that performs validation rather than the DynamoDB client.
Apply:
- with patch( - "functions.accounts.get_account_transactions.get_account_transactions.app.table" - ) as mock_table: - mock_table.query.side_effect = ValidationError("Invalid date range") + with patch( + "functions.accounts.get_account_transactions.get_account_transactions.transaction_helpers.query_transactions" + ) as mock_query: + mock_query.side_effect = ValidationError("Invalid date range")
204-215: Consider masking Step Functions error messagesCurrently returns the raw DB error to the SFN caller. If SFN output can be surfaced downstream, mask it similarly to API responses.
Happy to add a masked-path test and update the app accordingly.
51-80: Parametrise period/date-range casesBoth cases are identical apart from query params; parametrising will DRY the test and improve coverage if you add more variants.
I can submit a parametric version using pytest.mark.parametrize.
Also applies to: 81-113
functions/monthly_reports/accounts/notify_client/notify_client/send_report.py (3)
21-33: Use the S3 object’s basename instead of a hard-coded filenameImproves UX and avoids duplicate filenames.
Apply:
+import os @@ - attachment_filename="statement.pdf", + attachment_filename=os.path.basename(s3_key) or "statement.pdf",
23-38: Normalise error handling and return contractHelpers raise on failure; your current “failed” status path is unreachable. Either bubble exceptions consistently or catch and return a failure status.
Option (catch and return):
- response = send_user_email_with_attachment( + try: + response = send_user_email_with_attachment( aws_region=aws_region, logger=logger, sender_email=ses_no_reply_email, to_addresses=[recipient], subject_data=subject, body_text=body_text, attachment_bytes=pdf_bytes, - attachment_filename="statement.pdf", - ) + attachment_filename=os.path.basename(s3_key) or "statement.pdf", + ) + except Exception as e: + logger.error(f"Attachment email failed: {e}", exc_info=True) + return {"status": "failed", "messageId": None, "mode": "attachment"} @@ - response = send_user_email( + try: + response = send_user_email( aws_region=aws_region, logger=logger, sender_email=ses_no_reply_email, to_addresses=[recipient], subject_data=subject, text_body_data=body_text, - ) + ) + except Exception as e: + logger.error(f"Link email failed: {e}", exc_info=True) + return {"status": "failed", "messageId": None, "mode": "link"}If you prefer to bubble, drop the status dict entirely and let exceptions propagate.
Also applies to: 66-79
52-56: Make presigned URL expiry configurableConsider an env var or function argument for ExpiresIn to support different SLAs.
layers/python/helpers/ses.py (1)
120-123: Set the attachment MIME subtype to PDFImproves client rendering and spam scoring.
Apply:
- part = MIMEApplication(attachment_bytes) + part = MIMEApplication(attachment_bytes, _subtype="pdf") part.add_header("Content-Disposition", "attachment", filename=attachment_filename)functions/accounts/get_account_transactions/get_account_transactions/app.py (2)
31-37: Avoid ambiguous variable namingRename dynamodb to dynamodb_resource for clarity.
Apply:
-dynamodb = get_dynamodb_resource(DYNAMODB_ENDPOINT, AWS_REGION, logger) -if TRANSACTIONS_TABLE_NAME: - table = dynamodb.Table(TRANSACTIONS_TABLE_NAME) +dynamodb_resource = get_dynamodb_resource(DYNAMODB_ENDPOINT, AWS_REGION, logger) +if TRANSACTIONS_TABLE_NAME: + table = dynamodb_resource.Table(TRANSACTIONS_TABLE_NAME)
40-56: Optional: expose pagination cursorIf result sets can exceed 1MB, consider returning/accepting a cursor (LastEvaluatedKey) via query params to page results.
Happy to sketch a minimal cursor pattern for APIGatewayRestResolver if you want it in this PR.
functions/monthly_reports/accounts/notify_client/notify_client/app.py (4)
24-24: Inconsistent environment variable namingThe environment variable name uses
REPORTS_BUCKET_NAMEwhile other S3 bucket references in the codebase use the pattern without the_NAMEsuffix (e.g., justREPORTS_BUCKET).-REPORTS_BUCKET = os.environ.get("REPORTS_BUCKET_NAME") +REPORTS_BUCKET = os.environ.get("REPORTS_BUCKET")Also update the corresponding environment variable in
template.yml:- REPORTS_BUCKET_NAME: !Ref MonthlyReportsBucket + REPORTS_BUCKET: !Ref MonthlyReportsBucket
64-64: Unnecessary lowercase conversion for AWS_REGIONAWS region names are already lowercase by convention, making this conversion redundant.
- AWS_REGION.lower(), + AWS_REGION,
68-69: Redundant check after authenticate_requestThe
authenticate_requestfunction already raises an exception if authentication fails or no user ID is returned, making this check redundant.- if not user_id: - raise UnauthorizedError("Unauthorized") -
113-119: Consider using HTTP status codes constantsFor better maintainability and consistency, consider using HTTP status code constants.
Add at the top of the file:
from http import HTTPStatusThen update the response:
- "statusCode": 400, + "statusCode": HTTPStatus.BAD_REQUEST.value,template.yml (5)
378-380: Consider increasing timeout for batch processingThe trigger function has a 300-second timeout, but the continuation function (Line 429) only has 150 seconds. Consider aligning these timeouts or documenting why they differ.
- Timeout: 300 + Timeout: 300 # Extended timeout for processing large account batches
480-480: Environment variable naming inconsistency in CreateAccountsReportFunctionFor consistency with the notification function, consider using the same environment variable name.
- REPORTS_BUCKET_NAME: !Ref MonthlyReportsBucket + REPORTS_BUCKET: !Ref MonthlyReportsBucket
838-841: Consider adjusting retry strategy for GetAccountTransactionsThe exponential backoff with base 2.0 might result in long delays (5s, 10s, 20s). Consider if this aligns with your SLA requirements for report generation.
- ErrorEquals: [ "States.ALL" ] IntervalSeconds: 5 MaxAttempts: 3 - BackoffRate: 2.0 + BackoffRate: 1.5 # Results in 5s, 7.5s, 11.25s delays
890-890: Add newline at end of fileYAML files should end with a newline character for proper POSIX compliance.
- SQSSendMessagePolicy: QueueName: !GetAtt MonthlyAccountReportsCreationDLQ.QueueName +
394-394: Specify explicit timezone or confirm UTC schedule for monthly reports (template.yml:394)cron(0 0 1 * ? *) fires at 00:00 UTC on the 1st of every month. If you need a local‐wall‐clock time (DST-aware), refactor to use EventBridge Scheduler’s
ScheduleExpressionTimezonewith an IANA zone; otherwise confirm UTC meets requirements and account for DST/end-of-month edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (37)
README.md(1 hunks)dev-requirements.txt(1 hunks)functions/accounts/get_account_transactions/get_account_transactions/app.py(1 hunks)functions/accounts/get_account_transactions/get_account_transactions/date_helpers.py(1 hunks)functions/accounts/get_account_transactions/get_account_transactions/exceptions.py(1 hunks)functions/accounts/get_account_transactions/get_account_transactions/transaction_helpers.py(1 hunks)functions/accounts/get_account_transactions/requirements.txt(1 hunks)functions/monthly_reports/accounts/create_report/create_report/app.py(1 hunks)functions/monthly_reports/accounts/create_report/create_report/exceptions.py(1 hunks)functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py(1 hunks)functions/monthly_reports/accounts/create_report/create_report/template.html(1 hunks)functions/monthly_reports/accounts/create_report/requirements.txt(1 hunks)functions/monthly_reports/accounts/notify_client/notify_client/app.py(1 hunks)functions/monthly_reports/accounts/notify_client/notify_client/date_helpers.py(1 hunks)functions/monthly_reports/accounts/notify_client/notify_client/processing.py(1 hunks)functions/monthly_reports/accounts/notify_client/notify_client/send_report.py(1 hunks)functions/monthly_reports/accounts/trigger/trigger/app.py(1 hunks)layers/python/authentication/authentication/user_details.py(1 hunks)layers/python/helpers/s3.py(1 hunks)layers/python/helpers/ses.py(2 hunks)layers/python/monthly_reports/monthly_reports/processing.py(1 hunks)template.yml(11 hunks)tests/conftest.py(1 hunks)tests/functions/accounts/get_account_transactions/conftest.py(1 hunks)tests/functions/accounts/get_account_transactions/test_app.py(1 hunks)tests/functions/accounts/get_account_transactions/test_date_helpers.py(1 hunks)tests/functions/accounts/get_account_transactions/test_transaction_helpers.py(1 hunks)tests/functions/monthly_reports/accounts/create_report/conftest.py(1 hunks)tests/functions/monthly_reports/accounts/create_report/test_app.py(1 hunks)tests/functions/monthly_reports/accounts/create_report/test_exceptions.py(1 hunks)tests/functions/monthly_reports/accounts/create_report/test_generate_pdf.py(1 hunks)tests/functions/monthly_reports/accounts/notify_client/conftest.py(1 hunks)tests/functions/monthly_reports/accounts/notify_client/test_app.py(1 hunks)tests/functions/monthly_reports/accounts/notify_client/test_date_helpers.py(1 hunks)tests/layers/authentication/test_user_details.py(1 hunks)tests/layers/helpers/test_s3.py(1 hunks)tests/layers/helpers/test_ses.py(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- README.md
- functions/monthly_reports/accounts/create_report/requirements.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- dev-requirements.txt
- functions/monthly_reports/accounts/trigger/trigger/app.py
- layers/python/monthly_reports/monthly_reports/processing.py
🧰 Additional context used
🧬 Code graph analysis (27)
tests/functions/monthly_reports/accounts/notify_client/conftest.py (2)
tests/conftest.py (4)
mock_s3_client(272-275)magic_mock_ses_client(266-268)mock_cognito_client(279-282)mock_context(291-300)tests/functions/monthly_reports/accounts/create_report/conftest.py (4)
mock_s3_client(32-35)sample_event(39-60)mock_pdf_bytes(64-66)mock_presigned_url(70-72)
tests/functions/monthly_reports/accounts/notify_client/test_date_helpers.py (1)
functions/monthly_reports/accounts/notify_client/notify_client/date_helpers.py (1)
period_is_in_future(4-19)
tests/functions/monthly_reports/accounts/create_report/test_generate_pdf.py (2)
functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py (1)
generate_transactions_pdf(13-48)functions/monthly_reports/accounts/create_report/create_report/exceptions.py (2)
ReportGenerationError(1-2)ReportTemplateError(5-6)
tests/functions/monthly_reports/accounts/create_report/test_exceptions.py (1)
functions/monthly_reports/accounts/create_report/create_report/exceptions.py (3)
ReportGenerationError(1-2)ReportTemplateError(5-6)ReportUploadError(9-10)
functions/accounts/get_account_transactions/requirements.txt (2)
tests/functions/transactions/process_transactions/test_account_balance_helpers.py (1)
test_update_account_balance_success(83-96)functions/transactions/process_transactions/process_transactions/account_balance_helpers.py (1)
update_account_balance(38-61)
tests/functions/monthly_reports/accounts/create_report/conftest.py (2)
tests/conftest.py (1)
mock_s3_client(272-275)tests/functions/monthly_reports/accounts/notify_client/conftest.py (3)
sample_event(47-53)mock_pdf_bytes(78-80)mock_presigned_url(84-86)
tests/functions/accounts/get_account_transactions/test_transaction_helpers.py (2)
functions/accounts/get_account_transactions/get_account_transactions/app.py (1)
get_account_transactions(41-62)functions/accounts/get_account_transactions/get_account_transactions/transaction_helpers.py (1)
query_transactions(7-35)
functions/monthly_reports/accounts/notify_client/notify_client/date_helpers.py (1)
tests/functions/accounts/get_account_transactions/test_date_helpers.py (1)
now(71-72)
layers/python/helpers/s3.py (2)
layers/python/helpers/sqs.py (1)
get_sqs_client(7-28)tests/layers/helpers/test_sqs.py (4)
test_get_sqs_client_without_endpoint(28-42)TestGetSqsClient(8-57)test_get_sqs_client_error_handling(44-57)test_get_sqs_client_with_endpoint(9-26)
tests/layers/authentication/test_user_details.py (2)
layers/python/authentication/authentication/user_details.py (1)
get_user_attributes(5-20)tests/conftest.py (2)
mock_cognito_client(279-282)mock_logger(304-308)
tests/functions/accounts/get_account_transactions/test_date_helpers.py (2)
functions/accounts/get_account_transactions/get_account_transactions/date_helpers.py (1)
get_date_range(7-68)functions/accounts/get_account_transactions/get_account_transactions/exceptions.py (1)
ValidationError(1-2)
functions/accounts/get_account_transactions/get_account_transactions/transaction_helpers.py (1)
functions/accounts/get_account_transactions/get_account_transactions/date_helpers.py (1)
get_date_range(7-68)
tests/functions/accounts/get_account_transactions/test_app.py (4)
functions/accounts/get_account_transactions/get_account_transactions/app.py (2)
get_account_transactions(41-62)lambda_handler(66-99)functions/accounts/get_account_transactions/get_account_transactions/exceptions.py (1)
ValidationError(1-2)tests/functions/accounts/get_account_transactions/conftest.py (3)
valid_get_transactions_event(13-30)step_functions_event(34-43)get_account_transactions_app_with_mocked_tables(47-70)tests/conftest.py (1)
mock_context(291-300)
tests/functions/monthly_reports/accounts/notify_client/test_app.py (1)
tests/functions/monthly_reports/accounts/notify_client/conftest.py (7)
notify_client_app_with_mocks(8-43)sample_event(47-53)mock_context(57-68)mock_user_attributes(72-74)mock_pdf_bytes(78-80)mock_presigned_url(84-86)api_gateway_event(104-124)
tests/layers/helpers/test_s3.py (1)
layers/python/helpers/s3.py (1)
get_s3_client(6-13)
functions/monthly_reports/accounts/notify_client/notify_client/send_report.py (1)
layers/python/helpers/ses.py (2)
send_user_email(23-88)send_user_email_with_attachment(91-146)
functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py (1)
functions/monthly_reports/accounts/create_report/create_report/exceptions.py (2)
ReportGenerationError(1-2)ReportTemplateError(5-6)
functions/accounts/get_account_transactions/get_account_transactions/date_helpers.py (2)
functions/accounts/get_account_transactions/get_account_transactions/exceptions.py (1)
ValidationError(1-2)tests/functions/accounts/get_account_transactions/test_date_helpers.py (1)
now(71-72)
functions/monthly_reports/accounts/notify_client/notify_client/processing.py (2)
layers/python/authentication/authentication/user_details.py (1)
get_user_attributes(5-20)functions/monthly_reports/accounts/notify_client/notify_client/send_report.py (2)
send_report_as_attachment(6-38)send_report_as_link(41-79)
tests/layers/helpers/test_ses.py (3)
layers/python/helpers/ses.py (3)
get_ses_client(11-20)send_user_email(23-88)send_user_email_with_attachment(91-146)tests/conftest.py (3)
mock_ses_client(215-225)mock_logger(304-308)mock_get_ses_client(229-240)tests/functions/cognito/post_sign_up/test_post_sign_up.py (3)
test_missing_email_with_ses_enabled(168-203)test_successful_email_sending(62-82)test_failed_email_sending(84-107)
tests/functions/accounts/get_account_transactions/conftest.py (4)
functions/accounts/get_account_transactions/get_account_transactions/app.py (1)
get_account_transactions(41-62)tests/conftest.py (2)
dynamo_resource(60-69)mock_transactions_dynamo_table(73-109)tests/functions/accounts/get_accounts/conftest.py (1)
get_accounts_app_with_mocked_tables(70-92)tests/functions/transactions/get_transactions/conftest.py (1)
get_transactions_app_with_mocked_tables(70-92)
functions/monthly_reports/accounts/create_report/create_report/app.py (3)
functions/monthly_reports/accounts/create_report/create_report/exceptions.py (3)
ReportGenerationError(1-2)ReportTemplateError(5-6)ReportUploadError(9-10)layers/python/helpers/s3.py (1)
get_s3_client(6-13)functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py (1)
generate_transactions_pdf(13-48)
functions/monthly_reports/accounts/notify_client/notify_client/app.py (4)
layers/python/authentication/authentication/authenticate_request.py (1)
authenticate_request(6-36)layers/python/accounts/checks.py (1)
check_user_owns_account(20-36)layers/python/helpers/dynamodb.py (1)
get_dynamodb_resource(6-29)layers/python/helpers/s3.py (1)
get_s3_client(6-13)
functions/accounts/get_account_transactions/get_account_transactions/app.py (5)
layers/python/helpers/dynamodb.py (1)
get_dynamodb_resource(6-29)functions/accounts/get_account_transactions/get_account_transactions/exceptions.py (1)
ValidationError(1-2)functions/accounts/get_account_transactions/get_account_transactions/transaction_helpers.py (1)
query_transactions(7-35)functions/accounts/get_accounts/get_accounts/app.py (2)
lambda_handler(85-95)get_accounts(43-59)functions/transactions/get_transactions/get_transactions/app.py (2)
lambda_handler(108-123)get_transactions(43-67)
layers/python/helpers/ses.py (1)
tests/functions/cognito/post_sign_up/test_post_sign_up.py (1)
test_missing_email_with_ses_enabled(168-203)
tests/functions/monthly_reports/accounts/create_report/test_app.py (3)
functions/monthly_reports/accounts/create_report/create_report/exceptions.py (3)
ReportGenerationError(1-2)ReportTemplateError(5-6)ReportUploadError(9-10)tests/functions/monthly_reports/accounts/create_report/conftest.py (4)
create_report_app_with_mocks(8-28)sample_event(39-60)mock_pdf_bytes(64-66)mock_presigned_url(70-72)tests/conftest.py (1)
mock_context(291-300)
tests/conftest.py (1)
tests/functions/monthly_reports/accounts/create_report/conftest.py (1)
mock_s3_client(32-35)
🪛 YAMLlint (1.37.1)
template.yml
[error] 890-890: no new line character at the end of file
(new-line-at-end-of-file)
functions/accounts/get_account_transactions/get_account_transactions/app.py
Show resolved
Hide resolved
functions/accounts/get_account_transactions/get_account_transactions/transaction_helpers.py
Show resolved
Hide resolved
functions/monthly_reports/accounts/create_report/create_report/app.py
Outdated
Show resolved
Hide resolved
functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py
Outdated
Show resolved
Hide resolved
functions/monthly_reports/accounts/notify_client/notify_client/processing.py
Show resolved
Hide resolved
functions/monthly_reports/accounts/notify_client/notify_client/send_report.py
Show resolved
Hide resolved
functions/monthly_reports/accounts/notify_client/notify_client/send_report.py
Show resolved
Hide resolved
Signed-off-by: edinstance <edinstance@gmail.com>
Signed-off-by: edinstance <edinstance@gmail.com>
Signed-off-by: edinstance <edinstance@gmail.com>
Signed-off-by: edinstance <edinstance@gmail.com>
Signed-off-by: edinstance <edinstance@gmail.com>
edinstance
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py (1)
15-18: Autoescape enabled – good hardening.Jinja2 autoescape for HTML/XML is correctly configured. Thanks for addressing the prior concern.
functions/monthly_reports/accounts/notify_client/notify_client/app.py (1)
36-43: Fatal when ACCOUNTS_TABLE_NAME is unset to avoid None table usage.Continuing with table=None will crash at first DB access. Fail fast on misconfiguration.
dynamodb = get_dynamodb_resource(DYNAMODB_ENDPOINT, AWS_REGION, logger) if ACCOUNTS_TABLE_NAME: table = dynamodb.Table(ACCOUNTS_TABLE_NAME) logger.debug(f"Initialized DynamoDB table: {ACCOUNTS_TABLE_NAME}") else: - logger.critical("FATAL: ACCOUNTS_TABLE_NAME environment variable not set!") - table = None + error_msg = "FATAL: ACCOUNTS_TABLE_NAME environment variable not set!" + logger.critical(error_msg) + raise RuntimeError(error_msg)template.yml (1)
492-507: REPORTS_BUCKET env var matches code.The bucket var name is now consistent with the Lambda code; previous mismatch addressed.
🧹 Nitpick comments (5)
functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py (1)
26-32: Defensive input validation before rendering.Accessing event[...] will raise KeyError and bubble up as a 500. Validate required keys and fail fast with a clear error.
- html_out = template.render( - accountId=event["accountId"], - statementPeriod=event["statementPeriod"], - transactions=event["transactions"], - accountBalance=event["accountBalance"], - generationDate=datetime.now(timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC"), - ) + required_keys = ("accountId", "statementPeriod", "transactions", "accountBalance") + missing = [k for k in required_keys if k not in event] + if missing: + logger.error("Missing required event keys: %s", ",".join(missing)) + raise ReportGenerationError(f"Invalid input: missing {', '.join(missing)}") + + html_out = template.render( + accountId=event["accountId"], + statementPeriod=event["statementPeriod"], + transactions=event["transactions"], + accountBalance=event["accountBalance"], + generationDate=datetime.now(timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC"), + )tests/functions/monthly_reports/accounts/notify_client/conftest.py (1)
27-30: SES mock is prepared but never injected.Unless the SES client is patched in the code under test, the email path may still use a real boto3 client. Consider monkeypatching the SES factory to return your MagicMock.
+ # Ensure SES calls use the MagicMock client + import helpers.ses as ses_helpers + monkeypatch.setattr(ses_helpers, "get_ses_client", lambda aws_region, logger: magic_mock_ses_client)If helpers.ses does not expose get_ses_client, patch boto3.client in that module instead. Please confirm the actual SES wiring used in notify_client processing.
functions/monthly_reports/accounts/notify_client/notify_client/app.py (1)
44-46: Consider making MAX_ATTACHMENT_SIZE configurable.Hard-coding 7 MiB limits flexibility and environment parity. Expose an env var (with sane default) to tune this without code changes.
-# SES limit: 10 MB total, ~7 MB usable for attachments -MAX_ATTACHMENT_SIZE = 7 * 1024 * 1024 # 7 MB +# SES limit: 10 MB total, ~7 MB usable for attachments +MAX_ATTACHMENT_SIZE = int(os.getenv("MAX_ATTACHMENT_SIZE_BYTES", str(7 * 1024 * 1024)))template.yml (2)
824-890: Step Functions Lambda integration style – optional modernisation.You’re using the legacy “Resource: ” style. Consider migrating to arn:aws:states:::lambda:invoke with Payload to gain better control and consistency across states.
Example (for one state):
GetAccountTransactions: Type: Task Resource: arn:aws:states:::lambda:invoke Parameters: FunctionName: !GetAtt GetAccountTransactionsFunction.Arn Payload.$: $ ResultSelector: Payload.$: $.Payload Next: CreateAccountsReport
890-890: Add a trailing newline.Yamllint flags missing final newline.
- - SQSSendMessagePolicy: - QueueName: !GetAtt MonthlyAccountReportsCreationDLQ.QueueName + - SQSSendMessagePolicy: + QueueName: !GetAtt MonthlyAccountReportsCreationDLQ.QueueName +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
functions/monthly_reports/accounts/create_report/create_report/app.py(1 hunks)functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py(1 hunks)functions/monthly_reports/accounts/notify_client/notify_client/app.py(1 hunks)template.yml(11 hunks)tests/functions/monthly_reports/accounts/create_report/conftest.py(1 hunks)tests/functions/monthly_reports/accounts/create_report/test_app.py(1 hunks)tests/functions/monthly_reports/accounts/notify_client/conftest.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/functions/monthly_reports/accounts/create_report/test_app.py
- functions/monthly_reports/accounts/create_report/create_report/app.py
- tests/functions/monthly_reports/accounts/create_report/conftest.py
🧰 Additional context used
🧬 Code graph analysis (4)
tests/functions/monthly_reports/accounts/notify_client/conftest.py (2)
tests/functions/monthly_reports/accounts/create_report/conftest.py (3)
mock_s3_client(32-35)mock_pdf_bytes(64-66)mock_presigned_url(70-72)tests/conftest.py (4)
mock_s3_client(272-275)magic_mock_ses_client(266-268)mock_cognito_client(279-282)mock_context(291-300)
functions/monthly_reports/accounts/notify_client/notify_client/app.py (5)
layers/python/authentication/authentication/authenticate_request.py (1)
authenticate_request(6-36)layers/python/accounts/checks.py (1)
check_user_owns_account(20-36)layers/python/helpers/dynamodb.py (1)
get_dynamodb_resource(6-29)layers/python/helpers/s3.py (1)
get_s3_client(6-13)functions/monthly_reports/accounts/notify_client/notify_client/processing.py (1)
process_report(11-76)
functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py (1)
functions/monthly_reports/accounts/create_report/create_report/exceptions.py (2)
ReportGenerationError(1-2)ReportTemplateError(5-6)
template.yml (2)
functions/transactions/process_transactions/process_transactions/app.py (1)
lambda_handler(44-200)tests/functions/transactions/process_transactions/conftest.py (1)
sample_event_with_records(40-64)
🪛 GitHub Actions: Lint and Format Check Code
functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py
[error] 4-4: Step 'make lint' failed: Ruff error F401 ('tempfile' imported but unused) in generate_pdf.py:4. This is fixable with 'ruff --fix'.
🪛 YAMLlint (1.37.1)
template.yml
[error] 890-890: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
functions/monthly_reports/accounts/notify_client/notify_client/app.py (1)
55-58: APIGateway event headers access is incorrect.app.current_event is an object; calling get("headers") is invalid. Use the headers attribute.
- event = app.current_event - raw_headers = event.get("headers") or {} + event = app.current_event + raw_headers = getattr(event, "headers", {}) or {}Likely an incorrect or invalid review comment.
template.yml (1)
41-41: SES_NO_REPLY_EMAIL wired globally – nice.This aligns with the code’s expectation for SES_NO_REPLY_EMAIL.
functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py
Outdated
Show resolved
Hide resolved
Docstrings generation was requested by @edinstance. * #36 (comment) The following files were modified: * `functions/accounts/get_account_transactions/get_account_transactions/app.py` * `functions/accounts/get_account_transactions/get_account_transactions/date_helpers.py` * `functions/accounts/get_account_transactions/get_account_transactions/transaction_helpers.py` * `functions/monthly_reports/accounts/create_report/create_report/app.py` * `functions/monthly_reports/accounts/create_report/create_report/generate_pdf.py` * `functions/monthly_reports/accounts/notify_client/notify_client/app.py` * `functions/monthly_reports/accounts/notify_client/notify_client/date_helpers.py` * `functions/monthly_reports/accounts/notify_client/notify_client/processing.py` * `functions/monthly_reports/accounts/notify_client/notify_client/send_report.py` * `functions/monthly_reports/accounts/process_pending_reports/process_pending_reports/app.py` * `functions/monthly_reports/accounts/trigger/trigger/app.py` * `functions/transactions/process_transactions/process_transactions/app.py` * `functions/transactions/process_transactions/process_transactions/sqs.py` * `layers/python/authentication/authentication/user_details.py` * `layers/python/helpers/dynamodb.py` * `layers/python/helpers/s3.py` * `layers/python/helpers/ses.py` * `layers/python/helpers/sfn.py` * `layers/python/helpers/sqs.py` * `layers/python/monthly_reports/monthly_reports/helpers.py` * `layers/python/monthly_reports/monthly_reports/metrics.py` * `layers/python/monthly_reports/monthly_reports/processing.py` * `layers/python/monthly_reports/monthly_reports/responses.py` * `layers/python/monthly_reports/monthly_reports/sfn.py` * `layers/python/monthly_reports/monthly_reports/sqs.py` * `tests/conftest.py` * `tests/functions/accounts/get_account_transactions/conftest.py` * `tests/functions/accounts/get_account_transactions/test_app.py` * `tests/functions/accounts/get_account_transactions/test_date_helpers.py` * `tests/functions/monthly_reports/accounts/create_report/conftest.py` * `tests/functions/monthly_reports/accounts/create_report/test_app.py` * `tests/functions/monthly_reports/accounts/create_report/test_exceptions.py` * `tests/functions/monthly_reports/accounts/create_report/test_generate_pdf.py` * `tests/functions/monthly_reports/accounts/notify_client/conftest.py` * `tests/functions/monthly_reports/accounts/notify_client/test_app.py` * `tests/functions/monthly_reports/accounts/trigger_tests/conftest.py`
|
Note Generated docstrings for this pull request at #37 |
1 participant
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores