Fix/enhance alerting for transaction settlement errors #1164
Conversation
WalkthroughAdds unsettled-transaction schema and storage, records unsettled transactions at purchase time, reconciles unsettled vs settled transactions (including 48‑hour detection), surfaces unsettled IDs into batch-failure messages and emails, extends email API to accept batchFailureErrorMessage, adds submitTimeUTC to purchase responses, wires TransactionHistory table and monitoring, and moves app-client environment input into interactive flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Buyer
participant PostPrivileges as post_purchase_privileges
participant PurchaseClient as purchase_client
participant TransactionClient as transaction_client
participant DB as DynamoDB
participant Processor as transaction_processor
participant Email as EmailService
Buyer->>PostPrivileges: Submit purchase
PostPrivileges->>PurchaseClient: process_charge_on_credit_card(...)
PurchaseClient-->>PostPrivileges: success + submitTimeUTC
PostPrivileges->>TransactionClient: store_unsettled_transaction(compact, txId, submitTimeUTC)
TransactionClient->>DB: Put unsettled_transaction record
Note over Processor,DB: Later — settlement processing
Processor->>TransactionClient: reconcile_unsettled_transactions(compact, settled_transactions)
TransactionClient->>DB: Query unsettled records
alt Matches found
TransactionClient->>DB: Batch delete matched records
TransactionClient-->>Processor: reconciled (no old IDs)
else Old unsettled (>48h) present
TransactionClient-->>Processor: return old_unsettled_ids
Processor->>Email: sendTransactionBatchSettlementFailureEmail(..., batchFailureErrorMessage)
Email-->>Recipient: Render email with detailed error info (parsed JSON or raw)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
landonshumway-ia
force-pushed
the
fix/enhance-alerting-for-transaction-settlement-errors
branch
2 times, most recently
from
abfebd1 to
34a8c30
Compare
landonshumway-ia
force-pushed
the
fix/enhance-alerting-for-transaction-settlement-errors
branch
from
5d66920 to
c9f29c9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (1)
129-147: Guard against None when extending failedTransactionIds.If event already has batchFailureErrorMessage and this page has no new settlement errors, failed_transactions_ids is None and .extend(None) will throw. Default to [] and extend only when non-empty.
- failed_transactions_ids = transaction_response.get('settlementErrorTransactionIds') - if failed_transactions_ids or event.get('batchFailureErrorMessage'): + failed_transactions_ids = transaction_response.get('settlementErrorTransactionIds') or [] + if failed_transactions_ids or event.get('batchFailureErrorMessage'): @@ - if event.get('batchFailureErrorMessage'): + if event.get('batchFailureErrorMessage'): batch_failure_error_message = json.loads(event.get('batchFailureErrorMessage')) - batch_failure_error_message['failedTransactionIds'].extend(failed_transactions_ids) + if failed_transactions_ids: + batch_failure_error_message['failedTransactionIds'].extend(failed_transactions_ids) response['batchFailureErrorMessage'] = json.dumps(batch_failure_error_message)
🧹 Nitpick comments (3)
backend/compact-connect/app_clients/bin/create_app_client.py (1)
128-137: Interactive environment flow: solid; two small polish items.
- Deterministic scopes: consider sorting when deduping for stable output and reproducible diffs.
- Optionally validate that environment keys exist in auth/api URL maps (defensive check), even though input is validated.
- deduped_scopes = list(set(scopes)) + deduped_scopes = sorted(set(scopes))Also applies to: 197-203, 320-324, 346-347
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py (1)
724-819: Good coverage for unsettled paths; add time-stability to avoid flakes.
- Tests are clear and validate both failure surfacing and deletion. To avoid edge cases around the 48h cutoff, freeze time or inject now() via dependency to keep deterministic boundaries.
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
235-271: Unsettled storage: OK; normalize/validate input timestamp.store_unsettled_transaction assumes ISO strings that datetime.fromisoformat can parse later. If callers pass Z-suffixed UTC (e.g., 2024-01-01T12:00:00.000Z), later parsing will fail. Normalize or parse Z explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/compact-connect/app_clients/bin/create_app_client.py(4 hunks)backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts(1 hunks)backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts(2 hunks)backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/record.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py(2 hunks)backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py(2 hunks)backend/compact-connect/lambdas/python/purchases/handlers/privileges.py(1 hunks)backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py(2 hunks)backend/compact-connect/lambdas/python/purchases/purchase_client.py(1 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py(4 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py(2 hunks)backend/compact-connect/stacks/api_lambda_stack/purchases.py(5 hunks)backend/compact-connect/stacks/event_listener_stack/__init__.py(3 hunks)backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py(4 hunks)backend/compact-connect/tests/app/test_transaction_monitoring.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (1)
backend/compact-connect/lambdas/nodejs/lib/models/email-notification-service-event.ts (1)
EmailNotificationEvent(10-19)
backend/compact-connect/stacks/api_lambda_stack/purchases.py (2)
backend/compact-connect/stacks/persistent_stack/transaction_history_table.py (1)
TransactionHistoryTable(19-70)backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
transaction_history_table(274-275)compact_configuration_table(60-61)
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py (3)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (1)
process_settled_transactions(16-193)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_reporting.py (2)
_add_compact_configuration_data(228-245)_generate_mock_transaction(52-151)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
store_unsettled_transaction(235-270)
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
transaction_history_table(274-275)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/record.py (2)
TransactionRecordSchema(34-73)UnsettledTransactionRecordSchema(77-102)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
transaction_client(260-263)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
reconcile_unsettled_transactions(272-339)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/record.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py (2)
BaseRecordSchema(28-82)register_schema(68-75)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
Compact(48-50)
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py (3)
backend/compact-connect/lambdas/python/purchases/handlers/privileges.py (1)
post_purchase_privileges(176-464)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_privilege_options.py (1)
_when_testing_provider_user_event_with_custom_claims(17-36)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_credentials.py (1)
_generate_test_request_body(17-24)
backend/compact-connect/stacks/event_listener_stack/__init__.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
event_bus_name(84-85)provider_table(88-89)backend/common-cdk/common_constructs/stack.py (1)
common_env_vars(81-89)
backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
transaction_client(260-263)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (3)
TransactionClient(11-339)store_unsettled_transaction(235-270)reconcile_unsettled_transactions(272-339)
backend/compact-connect/lambdas/python/purchases/purchase_client.py (1)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
current_standard_datetime(253-257)
backend/compact-connect/lambdas/python/purchases/handlers/privileges.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
transaction_client(260-263)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
store_unsettled_transaction(235-270)
backend/compact-connect/app_clients/bin/create_app_client.py (1)
webroot/src/plugins/EnvConfig/envConfig.plugin.ts (1)
config(60-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (8)
backend/compact-connect/stacks/event_listener_stack/__init__.py (4)
63-63: Clarify alignment between PR objectives and changes.The PR objectives focus on transaction settlement error alerting (issue #1151), but the changes in this file configure license encumbrance event listeners. Could you clarify how these event bus configuration changes relate to the transaction settlement monitoring objectives?
Also applies to: 72-72, 113-113, 121-121
63-63: LGTM: Event bus name propagation.The
EVENT_BUS_NAMEenvironment variable is correctly propagated to both Lambda handlers, enabling them to publish events back to the data event bus.Also applies to: 113-113
72-72: LGTM: EventBridge permissions granted.The
grant_put_events_tocalls correctly authorize both Lambda handlers to publish events to the data event bus. The existingNagSuppressionsalready cover the IAM wildcard requirements for these permissions.Also applies to: 121-121
148-192: ****The exclusion of
EVENT_BUS_NAMEandgrant_put_events_tofor the license deactivation listener is intentional and correct. Unlike the encumbrance handlers which publish privilege events to the event bus, the deactivation handler only modifies data by callingdeactivate_license_privileges()without emitting events. The handler imports do not includeEventBusClientorEventBatchWriter, confirming it has no event publishing responsibility. Adding unnecessary event bus permissions would violate the principle of least privilege.Likely an incorrect or invalid review comment.
backend/compact-connect/stacks/api_lambda_stack/purchases.py (1)
34-42: Wiring and permissions look correct.
- Env var TRANSACTION_HISTORY_TABLE_NAME, handler param, and grant_read_write_data are all consistent with cc_common.config.transaction_history_table usage. LGTM.
Also applies to: 50-55, 73-74, 107-109
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (1)
95-99: Right place for reconciliation.Reconciling before enrichment/storage is sensible. LGTM.
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (1)
74-103: Nice job surfacing actionable error context.Parsing the optional payload and gracefully falling back to the raw message makes the alert far more informative without risking runtime failures, and threading the insertBody alignment preserves readability for the ops folks.
backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py (1)
111-267: Great coverage around unsettled reconciliation edge cases.These scenarios (bad input, empty queues, matched cleanup, and >48h stragglers) map directly to the client’s branching and should keep regressions from sneaking back in.
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/purchases/purchase_client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (1)
124-141: Fix None handling and ensure list before extend when merging failedTransactionIds.If there’s a prior batchFailureErrorMessage but no new settlement errors this run, failed_transactions_ids is None and .extend(None) will raise. Also assumes existing dict has failedTransactionIds. Coalesce to [] and merge safely.
- failed_transactions_ids = transaction_response.get('settlementErrorTransactionIds') + failed_transactions_ids = transaction_response.get('settlementErrorTransactionIds') or [] if failed_transactions_ids or event.get('batchFailureErrorMessage'): # error message should be a json object we can load if event.get('batchFailureErrorMessage'): batch_failure_error_message = json.loads(event.get('batchFailureErrorMessage')) - batch_failure_error_message['failedTransactionIds'].extend(failed_transactions_ids) + existing = batch_failure_error_message.get('failedTransactionIds', []) or [] + # dedupe while preserving type + merged_failed = list({*existing, *failed_transactions_ids}) + batch_failure_error_message['failedTransactionIds'] = merged_failed response['batchFailureErrorMessage'] = json.dumps(batch_failure_error_message) else: # if there was no previous error message, we'll create a new one response['batchFailureErrorMessage'] = json.dumps( { 'message': 'Settlement errors detected in one or more transactions.', - 'failedTransactionIds': failed_transactions_ids, + 'failedTransactionIds': failed_transactions_ids, } )
♻️ Duplicate comments (2)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (2)
157-174: Merge and dedupe unsettledTransactionIds; avoid leading/trailing spaces and duplicate message suffix.Currently overwrites any existing unsettledTransactionIds and can prepend a leading space if no existing message. Merge, dedupe, and avoid duplicate suffix.
- # Parse existing error message if it exists - existing_error = {} - if response.get('batchFailureErrorMessage'): - existing_error = json.loads(response.get('batchFailureErrorMessage')) - - existing_error.update( - { - 'message': existing_error.get('message', '') - + ' One or more transactions have not settled in over 48 hours.', - 'unsettledTransactionIds': old_unsettled_transaction_ids, - } - ) + # Parse existing error message if it exists + existing_error = json.loads(response.get('batchFailureErrorMessage')) if response.get('batchFailureErrorMessage') else {} + + suffix = 'One or more transactions have not settled in over 48 hours.' + prev_msg = (existing_error.get('message') or '').strip() + if not prev_msg: + new_msg = suffix + elif suffix not in prev_msg: + new_msg = f'{prev_msg} {suffix}' + else: + new_msg = prev_msg + existing_error['message'] = new_msg + + prev_unsettled = set(existing_error.get('unsettledTransactionIds', [])) + merged_unsettled = list(prev_unsettled.union(old_unsettled_transaction_ids)) + existing_error['unsettledTransactionIds'] = merged_unsettled
111-112: Use safe access for processedBatchIds to avoid KeyError.Mirror earlier defensive get on event; fall back to previous processed_batch_ids if the client omits it.
- 'processedBatchIds': transaction_response['processedBatchIds'], + 'processedBatchIds': transaction_response.get('processedBatchIds', processed_batch_ids),
🧹 Nitpick comments (5)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (5)
145-147: Log structured fields (and compact) instead of a large JSON string.Helps CloudWatch metric filters and reduces log volume; include compact for correlation.
- logger.error( - 'Batch settlement error detected', batchFailureErrorMessage=response['batchFailureErrorMessage'] - ) + _err = json.loads(response['batchFailureErrorMessage']) + logger.error( + 'Batch settlement error detected', + compact=compact, + message=_err.get('message'), + failedTransactionIds=_err.get('failedTransactionIds', []), + unsettledTransactionIds=_err.get('unsettledTransactionIds', []), + )
150-155: Consider isolating reconcile failures to avoid masking primary processing.If DynamoDB or reconcile throws, decide whether to continue or explicitly fail this iteration. Optional guard shown; still emits ERROR and BATCH_FAILURE as per alerting objectives.
- old_unsettled_transaction_ids = config.transaction_client.reconcile_unsettled_transactions( - compact=compact, settled_transactions=transaction_response['transactions'] - ) + try: + old_unsettled_transaction_ids = config.transaction_client.reconcile_unsettled_transactions( + compact=compact, settled_transactions=transaction_response.get('transactions', []) + ) + except Exception as e: + logger.error('Unsettled reconciliation failed', compact=compact, error=str(e)) + response['status'] = 'BATCH_FAILURE' + return response
175-179: Include compact and counts in ERROR log; avoid large arrays in logs.Emit counts to keep logs lean; arrays remain in batchFailureErrorMessage if needed.
- logger.error( - 'Unsettled transactions older than 48 hours detected', - unsettledTransactionIds=old_unsettled_transaction_ids, - ) + logger.error( + 'Unsettled transactions older than 48 hours detected', + compact=compact, + unsettledTransactionIdsCount=len(old_unsettled_transaction_ids), + )
68-69: Make scheduledTime parsing robust to ISO8601 'Z' and ensure UTC normalization.datetime.fromisoformat doesn’t parse 'Z' on some runtimes. Normalize to UTC and then set the hour.
-fromiso = datetime.fromisoformat -end_time = datetime.fromisoformat(scheduled_time).replace(hour=1, minute=0, second=0, microsecond=0) +from datetime import timezone +_ts = scheduled_time.replace('Z', '+00:00') +end_time = datetime.fromisoformat(_ts).astimezone(timezone.utc).replace(hour=1, minute=0, second=0, microsecond=0)Please confirm the actual format of scheduledTime from EventBridge (e.g., "2025-10-16T22:12:50Z"). If it’s always 'Z', the above change prevents runtime ValueError.
96-105: Minor: guard access to transactions key.Use .get to avoid KeyError if the client ever omits transactions on error. No behavior change when present.
- if transaction_response['transactions']: + if transaction_response.get('transactions'):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
transaction_client(260-263)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
reconcile_unsettled_transactions(272-339)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (1)
165-171: Consider trimming whitespace in message concatenation.If
existing_error.get('message', '')returns an empty string, line 168 produces a message with a leading space. Consider adding.strip()or checking for non-empty message before concatenation.Apply this diff to handle spacing more gracefully:
- existing_error.update( - { - 'message': existing_error.get('message', '') - + ' One or more transactions have not settled in over 48 hours.', - 'unsettledTransactionIds': old_unsettled_transaction_ids, - } - ) + message_parts = [existing_error.get('message', ''), 'One or more transactions have not settled in over 48 hours.'] + existing_error.update( + { + 'message': ' '.join(filter(None, message_parts)), + 'unsettledTransactionIds': old_unsettled_transaction_ids, + } + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
transaction_client(260-263)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
reconcile_unsettled_transactions(272-339)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (3)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (3)
126-148: LGTM! Settlement error handling is well-implemented.The defensive use of
.get('settlementErrorTransactionIds', [])on line 126, proper aggregation of failed transaction IDs across iterations, and the elevation of the log level toerrorwhen all transactions are processed (line 145) align perfectly with the PR objectives of enhancing alerting for transaction settlement errors.
150-155: LGTM! Reconciliation approach is sound.Running
reconcile_unsettled_transactionson every iteration ensures timely cleanup of matched transactions and reliable detection of stale unsettled records. The implementation correctly delegates to the transaction client and captures the 48-hour threshold violations for reporting.
157-179: Solid implementation of 48-hour unsettled transaction detection.The logic correctly defers reporting until all transactions are processed, properly merges with existing error messages, and sets the appropriate
BATCH_FAILUREstatus with error-level logging. This aligns well with the PR's monitoring objectives.
|
@jlkravitz This is ready for your review. Thanks |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
288-291: Add pagination to prevent silently missing unsettled transactions.The query lacks pagination and may exceed DynamoDB's 1 MB response limit if there are many unsettled transactions. This was flagged in the previous review and remains unresolved. Without pagination, some unsettled transactions will be silently missed, preventing proper reconciliation and 48-hour detection.
Apply this diff to add pagination following the existing pattern in
_query_transactions_for_month:- response = self.config.transaction_history_table.query( - KeyConditionExpression=Key('pk').eq(pk), - ) - - unsettled_transactions = response.get('Items', []) + unsettled_transactions = [] + last_evaluated_key = None + while True: + query_kwargs = {'KeyConditionExpression': Key('pk').eq(pk)} + if last_evaluated_key: + query_kwargs['ExclusiveStartKey'] = last_evaluated_key + + response = self.config.transaction_history_table.query(**query_kwargs) + unsettled_transactions.extend(response.get('Items', [])) + + last_evaluated_key = response.get('LastEvaluatedKey') + if not last_evaluated_key: + break
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
299-340: LGTM! Reconciliation and 48-hour detection logic is correct.The logic properly matches unsettled transactions with settled ones, batch deletes matched records, and identifies transactions older than 48 hours. The
datetime.fromisoformatusage at line 329 is safe sincetransactionDateis stored in normalized '+00:00' format (as per your earlier clarification).Optional: Line 318 logs the entire
settled_transaction_idsset, which could be verbose if there are many transactions. Consider logging just the count for cleaner logs:- settled_transaction_ids=settled_transaction_ids + settled_count=len(settled_transaction_ids)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T22:31:44.837Z
Learnt from: jusdino
PR: csg-org/CompactConnect#1005
File: backend/compact-connect/lambdas/python/common/cc_common/dsa_auth.py:212-228
Timestamp: 2025-08-25T22:31:44.837Z
Learning: The datetime.fromisoformat() method in the CompactConnect DSA authentication module already supports both 'Z' and '+00:00' ISO 8601 timestamp formats, as verified by existing tests. The timezone-naive timestamp handling should be added as an additional safety measure.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
transaction_history_table(274-275)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/record.py (2)
TransactionRecordSchema(34-73)UnsettledTransactionRecordSchema(77-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (2)
1-1: LGTM! Imports support the new unsettled transaction functionality.The added imports (
UTC,timedelta, andUnsettledTransactionRecordSchema) are all used appropriately in the new methods.Also applies to: 6-6
235-271: LGTM! Appropriate error handling for non-critical monitoring feature.The broad exception handling is justified since this is a monitoring feature that shouldn't disrupt the main transaction flow. The error logging ensures visibility while preventing cascading failures.
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
Looks good to me! @isabeleliassen good to merge.
4 participants
We need to be notified immediately of any errors that may potentially occur in the history collection process, so we can address it as soon as possible. This ticket involves adding a custom metric on the transaction_processor_handler in the transaction monitoring stack with an alarm that will alert us if any log is emitted with a ERROR level. This is in conjunction with the hot fix #1150 which address the core issue we identified. This must be a follow up PR to avoid massive merge conflicts with changes we currently have in our CDK configuration. Once those are merged in, we can apply this necessary monitoring.
Closes #1151
Summary by CodeRabbit
New Features
Tests