Bug/handle authorizenet delayed reporting #1201
Conversation
WalkthroughIntroduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as transaction_history Handler
participant CompactClient as CompactClient
participant TransactionClient as TransactionClient
participant PurchaseClient as PurchaseClient
participant DynamoDB as DynamoDB
Handler->>CompactClient: Check compact live status
alt Compact not live
Handler-->>Handler: Return COMPLETE (skip)
else Compact is live
Handler->>TransactionClient: get_most_recent_transaction_for_compact()
TransactionClient->>DynamoDB: Query current + 3 prior months
DynamoDB-->>TransactionClient: Most recent transaction (or error)
TransactionClient-->>Handler: TransactionData
rect rgb(200, 220, 255)
note right of Handler: Derive time window from most recent transaction
end
Handler->>PurchaseClient: get_settled_transactions(compact, start_time, end_time, limit, ...)
PurchaseClient-->>Handler: SettledTransactions (up to limit)
alt Transactions found
Handler->>TransactionClient: add_privilege_information_to_transactions()
TransactionClient-->>Handler: EnrichedTransactions
Handler->>TransactionClient: store_transactions()
TransactionClient->>DynamoDB: Save transactions
rect rgb(200, 255, 220)
note right of Handler: Reconcile settled vs unsettled
end
Handler->>TransactionClient: reconcile_unsettled_transactions()
TransactionClient-->>Handler: Unmatched settled IDs
alt All transactions processed
Handler-->>Handler: status = COMPLETE
else More transactions exist
Handler-->>Handler: status = IN_PROGRESS + pagination fields
end
end
rect rgb(255, 220, 200)
note right of Handler: Handle settlement errors & old unsettled
end
alt Settlement errors or old unsettled (48+ hrs)
Handler-->>Handler: status = BATCH_FAILURE + batchFailureErrorMessage
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py (1)
219-220: Consider using consistent timezone terminology.The comment mentions "4:00pm Pacific Time" but then refers to "6pm PST" and "2am UTC". For clarity, consider using "Pacific Time" consistently throughout, since the offset between Pacific Time and UTC varies between PST (UTC-8) and PDT (UTC-7) depending on daylight saving time. The cron schedule correctly uses 2am UTC, which corresponds to 6pm Pacific Time during winter and 7pm Pacific Time during summer.
Apply this diff for consistency:
- # In practice, we've seen settlements running up to an hour late, so this daily collector runs two hours later - # (6pm PST, which is 2am UTC) to collect all settled transactions since the previous run. + # In practice, we've seen settlements running up to an hour late, so this daily collector runs two hours later + # (2am UTC) to collect all settled transactions since the previous run.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (1)
8-17: Update docstring to reflect the lineItems setter.The docstring states "this one does not include setters," but a setter is defined for
lineItemsat lines 46-48. Please update the docstring to accurately reflect this exception.Apply this diff:
Class representing a Transaction with read-only properties. - Unlike several other CCDataClass subclasses, this one does not include setters. This is because + Unlike several other CCDataClass subclasses, this one includes only a single setter (lineItems). This is because transaction records are only created during transaction processing, so we can pass the entire record from the processing into the constructor.backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py (1)
207-243: Previous-transaction seeding mirrors real history behavior
_add_previous_transaction_to_historycorrectly:
- defaults to a settlement time relative to
MOCK_SCHEDULED_TIME,- calculates a plausible local time, and
- stores the transaction through
TransactionClient.store_transactions.This gives the new “query from last settlement forward” path realistic state without test duplication. The hard‑coded EST offset is fine for test expectations since only the UTC timestamp actually drives the DynamoDB sort key.
If you later add tests for non‑EST compacts, consider parameterizing the local offset instead of inlining
-5here.backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
162-167: Privilege mapping on TransactionData looks consistent; consider guarding empty provider_idsUpdating
_set_privilege_id_in_line_itemto useline_item.get('itemId')avoids KeyErrors on malformed items, and the rest ofadd_privilege_information_to_transactionscorrectly:
- derives jurisdictions from privilege line items,
- queries the GSI with the compact/transactionId key,
- sets
licenseeIdfrom the privilege providerId, and- writes privilege IDs back into
transaction.lineItems.One minor defensive improvement would be to handle the case where
records_for_transaction_idis non-empty but contains notype in {'privilege','privilegeUpdate'}, which would currently causeprovider_ids.pop()to raise. Logging and defaultinglicenseeIdto'UNKNOWN'(similar to how privilegeId is handled) would make this path safer if the provider table ever contains unexpected record types.Also applies to: 168-276
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/purchase/api.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py(12 hunks)backend/compact-connect/lambdas/python/common/common_test/test_constants.py(2 hunks)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py(2 hunks)backend/compact-connect/lambdas/python/common/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py(7 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_transaction.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_transaction_client.py(8 hunks)backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py(1 hunks)backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py(1 hunks)backend/compact-connect/lambdas/python/purchases/purchase_client.py(9 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py(21 hunks)backend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py(4 hunks)backend/compact-connect/stacks/disaster_recovery_stack/__init__.py(1 hunks)backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is a property (not a method), and can be properly patched in tests by directly supplying a datetime value rather than wrapping it in a lambda: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.pybackend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.pybackend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is a property rather than a method, and should be patched in tests by directly providing a datetime value: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.pybackend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.pybackend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py
📚 Learning: 2025-05-23T14:15:40.107Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 796
File: backend/compact-connect/lambdas/python/purchases/tests/function/__init__.py:27-31
Timestamp: 2025-05-23T14:15:40.107Z
Learning: The TestDataGenerator class in CompactConnect contains only static methods and should be used directly without instantiation (e.g., TestDataGenerator.method_name() rather than creating an instance first).
Applied to files:
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.pybackend/compact-connect/lambdas/python/common/tests/__init__.py
📚 Learning: 2025-09-03T22:29:24.535Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py:0-0
Timestamp: 2025-09-03T22:29:24.535Z
Learning: The EncumbranceEventDetailSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py is used across multiple instances/contexts where adverseActionCategory and adverseActionId fields may be required in some cases and not present in others. This is an intentional design pattern for handling different event variants with a single schema.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py
📚 Learning: 2025-07-08T18:40:24.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 882
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py:287-359
Timestamp: 2025-07-08T18:40:24.408Z
Learning: In the CompactConnect codebase, landonshumway-ia prefers to avoid extraneous unit tests when existing test coverage is already sufficient to catch bugs. For the get_privilege_purchase_options method's live-jurisdiction filtering logic, the existing tests in the purchases test suite provide adequate coverage without needing additional edge case tests.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.pybackend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is implemented as a property that returns `datetime.now(tz=UTC).replace(microsecond=0)`. The correct way to patch it in tests is by directly providing the datetime value, not wrapping it in a lambda: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.pybackend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py
📚 Learning: 2025-08-21T02:51:28.199Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/common/requirements.in:4-4
Timestamp: 2025-08-21T02:51:28.199Z
Learning: In CompactConnect, the purchases lambda contains requests as a transitive dependency from the Authorize.net SDK, which is automatically resolved by pip-compile. This should not be manually removed even when requests is also available in the common layer, as it's managed automatically by the dependency resolver.
Applied to files:
backend/compact-connect/lambdas/python/purchases/purchase_client.py
🧬 Code graph analysis (12)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py (1)
get_privilege_purchase_options(325-400)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (4)
get_most_recent_transaction_for_compact(81-120)add_privilege_information_to_transactions(168-276)store_transactions(18-32)reconcile_unsettled_transactions(315-383)backend/compact-connect/lambdas/python/purchases/purchase_client.py (3)
get_settled_transactions(223-244)get_settled_transactions(734-913)get_settled_transactions(1081-1114)
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_transaction_client.py (4)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_default_transaction(653-682)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
store_transactions(18-32)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
update(182-203)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (3)
lineItems(39-44)lineItems(47-48)licenseeId(55-56)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (1)
TransactionData(7-76)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
update(182-203)create_new(96-121)
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_transaction.py (5)
backend/compact-connect/lambdas/python/common/tests/__init__.py (1)
TstLambdas(9-111)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (2)
TestDataGenerator(23-719)generate_default_transaction(653-682)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/record.py (1)
TransactionRecordSchema(34-73)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
serialize_to_database_record(205-208)from_database_record(124-139)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (13)
TransactionData(7-76)transactionProcessor(26-27)transactionId(30-31)batch(34-36)lineItems(39-44)lineItems(47-48)compact(51-52)licenseeId(55-56)responseCode(59-60)settleAmount(63-64)submitTimeUTC(67-68)transactionStatus(71-72)transactionType(75-76)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
CCDataClass(46-208)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/record.py (1)
TransactionRecordSchema(34-73)
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (8)
TransactionData(7-76)batch(34-36)transactionProcessor(26-27)compact(51-52)lineItems(39-44)lineItems(47-48)transactionId(30-31)licenseeId(55-56)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/record.py (1)
UnsettledTransactionRecordSchema(77-102)backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
transaction_history_table(274-275)current_standard_datetime(253-257)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
serialize_to_database_record(205-208)from_database_record(124-139)update(182-203)
backend/compact-connect/stacks/disaster_recovery_stack/__init__.py (1)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
ssn_table(92-93)
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py (2)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (8)
TestDataGenerator(23-719)generate_default_transaction(653-682)put_default_jurisdiction_configuration_in_configuration_table(634-650)put_default_compact_configuration_in_configuration_table(588-604)put_default_privilege_record_in_provider_table(342-352)generate_default_privilege(311-333)generate_default_privilege_update(361-388)store_record_in_provider_table(336-339)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (2)
TransactionClient(12-383)store_transactions(18-32)
backend/compact-connect/lambdas/python/common/tests/__init__.py (2)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
TestDataGenerator(23-719)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
_Config(21-321)
backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py (3)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_default_transaction(653-682)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
serialize_to_database_record(205-208)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (2)
store_transactions(18-32)get_most_recent_transaction_for_compact(81-120)
backend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (7)
transactionId(30-31)compact(51-52)licenseeId(55-56)batch(34-36)lineItems(39-44)lineItems(47-48)transactionStatus(71-72)backend/compact-connect/lambdas/python/purchases/purchase_client.py (3)
get_settled_transactions(223-244)get_settled_transactions(734-913)get_settled_transactions(1081-1114)
backend/compact-connect/lambdas/python/purchases/purchase_client.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (8)
TransactionData(7-76)compact(51-52)transactionStatus(71-72)submitTimeUTC(67-68)transactionType(75-76)responseCode(59-60)settleAmount(63-64)batch(34-36)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
create_new(96-121)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/compact/common.py (1)
PaymentProcessorType(13-14)
⏰ 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 (18)
backend/compact-connect/stacks/disaster_recovery_stack/__init__.py (1)
76-78: Formatting consistency improvement, but disconnected from PR objectives.The multi-line reformatting aligns with the existing code style used elsewhere in the file (e.g., lines 71–73, 87–93) and improves readability. However, this formatting change appears unrelated to the stated PR objectives around handling Authorize.net delayed batch reporting. Confirm this is intentional as part of a broader code-quality pass and not an accidental inclusion.
backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py (1)
213-221: LGTM! Syntactic correction to test data structure.The closing bracket properly terminates the
configuredStateslist for the octp compact configuration, fixing the dictionary structure. This aligns with the formatting of similar test data throughout the file.backend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py (1)
1299-1376: LGTM! Good test coverage for declined transaction handling.The new test validates that declined transactions are correctly filtered out from the results, ensuring only successfully settled transactions are returned. The test setup is clear and comprehensive.
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (3)
44-67: Compact live/jurisdiction gate looks solidUsing
get_privilege_purchase_optionsplus the compact/jurisdiction type filtering to short‑circuit when either the compact config or live jurisdictions are missing is a clean way to avoid unnecessary Authorize.net calls and keeps the step function behavior explicit (status: COMPLETEand preservedscheduledTime).
114-199: Settlement error aggregation and unsettled reconciliation are well structuredThe flow that:
- enriches transactions with privilege IDs, stores them via
TransactionClient.store_transactions,- accumulates
settlementErrorTransactionIdsacross iterations intobatchFailureErrorMessage,- flips
statustoBATCH_FAILUREonly after all transactions are processed, and- reconciles unsettled transactions every iteration, flagging >48‑hour ones when done,
is coherent and matches the intended alerting behavior. The pagination/status handling around
_all_transactions_processedis also consistent with the newPurchaseClient.get_settled_transactionscontract.
69-94: The review comment is incorrect; no refactoring is neededThe original concern about timezone handling is unfounded. In Python 3.13,
datetime.fromisoformat()accepts trailing "Z" timestamps and returns timezone-aware UTC datetimes. Critically, the.replace()method on a timezone-aware datetime preserves tzinfo rather than removing it—all datetime objects in this code remain timezone-aware throughout the operations. Themax()comparison betweenmost_recent_settlementandoldest_allowed_startoperates on compatible tz-aware datetimes without error.The code is robust as written; no normalization or explicit UTC coercion is necessary.
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py (3)
34-74: Test transaction generation via TestDataGenerator is a good fitSwitching
_generate_mock_transactionto build line items in a loop and delegate the actual transaction construction toTestDataGenerator.generate_default_transactionkeeps the tests aligned with the production schema and avoids hand‑rolled dicts. It will make future schema changes less brittle for these tests.
81-107: Compact/jurisdiction config setup helper is clear and reusable
_add_compact_configuration_datanow populates jurisdiction and compact configuration rows via the shared generator, usingconfiguredStatesderived from the provided jurisdictions. This mirrors how the handler consumesget_privilege_purchase_optionsand gives a single, readable setup path for all tests that need a “live compact”.
876-932: New start_time tests precisely exercise the updated window logicThe two tests:
test_process_settled_transactions_uses_previous_transaction_settlement_time_for_start_time, andtest_process_settled_transactions_uses_30_day_fallback_when_no_previous_transactionsnicely pin down the intended behavior of
start_timeboth when a prior settlement exists and whenget_most_recent_transaction_for_compactraises. Patching_Config.current_standard_datetimedirectly matches the existing pattern in this repo and keeps the queries deterministic relative to the seeded data.backend/compact-connect/lambdas/python/purchases/purchase_client.py (2)
734-755: TransactionData-based get_settled_transactions wiring looks correctThe refactor to:
- build
TransactionDataviaTransactionData.create_new(...)withcompactandtransactionProcessorset, and- have
PurchaseClient.get_settled_transactionsdelegate directly to the processor client while threadingcompact,lines up with the new
TransactionClient.store_transactions/add_privilege_information_to_transactionsexpectations. This should make the storage and enrichment layers more robust than the previous dict-based flow, while preserving the same response shape (transactions list plus pagination and settlement error IDs).Also applies to: 858-881, 1081-1114
43-45: Status string is correct; consider whether other non-settlement statuses should be ignoredVerified: Authorize.net's API returns "declined" (lowercase) for declined transactions, so the enum value is correct.
However, other non-settlement / non-final statuses exist (e.g.,
authorizedPendingCapture,capturedPendingSettlement,refundPendingSettlement,approvedReview,FDSPendingReview,FDSAuthorizedPendingReview,communicationError,couldNotVoid,expired,voided). Confirm whether ignoring onlydeclinedis intentional, or if other non-final statuses should also be added toAuthorizeNetTransactionIgnoreStatesto prevent them from surfacing as settlement errors.backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (3)
18-32: store_transactions now matches the TransactionData model and schemaSwitching
store_transactionsto acceptlist[TransactionData]and delegating serialization toserialize_to_database_record()is a clean separation of concerns and ensures pk/sk and other derived fields stay in sync with the schema. The explicit processor-type check and ValueError on unsupported types are appropriate.
81-120: get_most_recent_transaction_for_compact implements the desired month-scoped lookupThe new
get_most_recent_transaction_for_compact:
- uses
current_standard_datetimeto pick the current month partition,- walks back up to three months, and
- queries each month with
ScanIndexForward=FalseandLimit=1to fetch only the most recent record,which is exactly what the handler needs to anchor its start_time. The behavior when no records are found (raising
ValueError) is clear, and the tests intest_transaction_client.pycover current-month, previous-month, and no‑data cases.
278-314: Unsettled transaction storage and reconciliation integrate cleanly with the handler
store_unsettled_transactionandreconcile_unsettled_transactionsnow:
- use
UnsettledTransactionRecordSchemato generate pk/sk for unsettled records,- swallow validation/write errors for this monitoring-only path, and
- reconcile by matching settled
TransactionData.transactionIds, deleting matches, and flagging >48‑hour unmatched transactions.This matches the new handler behavior around unsettled transaction alerts and is well covered by the tests in both the purchases and common test suites.
Also applies to: 315-383
backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py (4)
11-24: Local TransactionData helper is consistent with the shared generatorThe
_generate_mock_transactionhelper wrapstest_data_generator.generate_default_transactionand then overridesbatchfields as needed, which keeps these tests in sync with the data model while allowing explicit control over transactionId, compact, settlementTimeUTC, and batchId. This is a good way to avoid duplicating schema details in the tests.
41-52: Edge-time test now validates both the stored shape and range behaviorRewriting
test_transaction_history_edge_timesto:
- create edge transactions via
generate_default_transactionwith overridden batch data,- reuse
serialize_to_database_record()for the expected record, and- strip
dateOfUpdatebefore comparisonensures the test is checking the exact DynamoDB record shape produced by
store_transactionsas well as the range filtering behavior. This is a solid regression guard for subtle changes in transaction serialization.Also applies to: 79-93
173-185: Reconciliation tests now exercise the TransactionData-based pathUpdating the reconciliation tests to pass
TransactionDatainstances (from the generator) intoreconcile_unsettled_transactionsmatches the new method signature and how the handler calls it. The remaining “no unsettled” test still passes dicts but never dereferences them (it returns early), so it remains safe.Also applies to: 243-246
263-379: get_most_recent_transaction_for_compact tests thoroughly cover new behaviorThe three new tests:
- confirm current-month selection when records exist,
- confirm fallback to previous months when the current month is empty, and
- verify that a
ValueErroris raised (with correct message) and that exactly three monthly queries are performed when no records exist,provide strong coverage for the new lookup logic. Patching
_Config.current_standard_datetimeand mocking the DynamoDB Table in the no-data case are both done cleanly and in line with existing practices for this codebase.
...mpact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py
Show resolved
Hide resolved
...nd/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py
Outdated
Show resolved
Hide resolved
landonshumway-ia
left a comment
landonshumway-ia
left a comment
There was a problem hiding this comment.
Barring the dependency check failure, the python code looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
170-278: Harden privilege mapping around emptyprovider_idsand fix logging fieldIn
add_privilege_information_to_transactions:
- You build
provider_idsfrom privilege records and then unconditionally callprovider_ids.pop()to setlicenseeId. If the GSI returns items but none withtype in {'privilege','privilegeUpdate'},provider_idswill be empty andpop()will raise, aborting the whole batch.- In the
len(provider_ids) > 1case, the log argumentprovider_ids=transaction.licenseeIdappears to be a copy/paste bug; it should log the ambiguousprovider_idsset, not the (possibly masked) licenseeId.Consider something along these lines:
- provider_ids = { - item['providerId'] - for item in records_for_transaction_id - if item['type'] == 'privilege' or item['type'] == 'privilegeUpdate' - } + provider_ids = { + item['providerId'] + for item in records_for_transaction_id + if item['type'] in ('privilege', 'privilegeUpdate') + } # there should only be one provider id in the set if len(provider_ids) > 1: logger.error( 'More than one matching provider id found for a transaction id.', compact=compact, - transaction_id=transaction.transactionId, - # attempt to grab the licensee id from the authorize.net data, which may be invalid if it was masked - provider_ids=transaction.licenseeId, + transaction_id=transaction.transactionId, + provider_ids=list(provider_ids), ) - # The licensee id recorded in Authorize.net cannot be trusted... - transaction.update({'licenseeId': provider_ids.pop()}) + if provider_ids: + # The licensee id recorded in Authorize.net cannot be trusted... + transaction.update({'licenseeId': provider_ids.pop()}) + else: + logger.error( + 'No providerId found in privilege records for this transaction id.', + compact=compact, + transaction_id=transaction.transactionId, + ) + # Optionally fall back to UNKNOWN or leave licenseeId unchanged, depending on reporting needs.This avoids an unhandled exception and makes the error log more informative.
🧹 Nitpick comments (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
81-122:get_most_recent_transaction_for_compactbehavior is well‑defined but has a tight 3‑month horizonThe month‑by‑month descending query pattern with
Limit=1andScanIndexForward=Falsecorrectly returns the latest transaction within the last up‑to‑three months and surfaces “no history” viaValueError.The 3‑month cap is a policy choice: for compacts that have gone quiet for longer than that, you will intentionally behave as “no transactions found” and let callers fall back to their oldest‑allowed window. That’s reasonable, but it’s worth being sure this horizon matches your operational expectations for very low volume compacts.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (1)
1-72: TransactionData wrapper is consistent and type‑safe; consider clarifying mutability semanticsThe TransactionData CCDataClass is wired correctly to
TransactionRecordSchemaand exposes the expected read‑only properties, which makes downstream code much clearer than raw dicts.Note that while the properties themselves are read‑only,
batchandlineItemsreturn the underlying mutable structures. That’s useful (tests and privilege enrichment mutate them), but it means callers can bypass theupdate()validation path if they mutate in place. If you want to enforce stricter invariants later, it may be worth documenting this explicitly in the class docstring.backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py (2)
11-24: Helper_generate_mock_transactionmatches the new TransactionData modelUsing
generate_default_transactionwithvalue_overridesto controltransactionId/compactand then mutatingtransaction.batchfor optionalsettlement_time_utc/batch_idis a pragmatic way to build tailored TransactionData instances for tests.If you want to stay closer to the schema‑driven creation path, you could instead pass a
batchoverride intovalue_overrides(as you do in other tests), but that’s a stylistic choice rather than a correctness issue.
143-157: Minor type inconsistency intest_reconcile_unsettled_transactions_no_unsettled_passesHere you still pass
settled_transactionsas a list of dicts, while the production method now expectslist[TransactionData]. The test passes only because the function returns early when there are no unsettled records and never touchessettled_transactions.For consistency with the other reconciliation tests (and to guard against future refactors that might use this argument earlier), consider switching this test to use
generate_default_transactionas well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py(13 hunks)backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py(7 hunks)backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1201
File: backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py:224-224
Timestamp: 2025-11-13T22:07:29.172Z
Learning: In the CompactConnect transaction history collection workflow (backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py and backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py), the cron schedule runs at 2 AM UTC but the handler queries for transactions up to 1 AM UTC. This one-hour gap is intentional to accommodate a delay between when Authorize.net reports a settlement time and when the batch actually becomes visible in their API.
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
📚 Learning: 2025-11-13T22:07:29.172Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1201
File: backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py:224-224
Timestamp: 2025-11-13T22:07:29.172Z
Learning: In the CompactConnect transaction history collection workflow (backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py and backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py), the cron schedule runs at 2 AM UTC but the handler queries for transactions up to 1 AM UTC. This one-hour gap is intentional to accommodate a delay between when Authorize.net reports a settlement time and when the batch actually becomes visible in their API.
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is implemented as a property that returns `datetime.now(tz=UTC).replace(microsecond=0)`. The correct way to patch it in tests is by directly providing the datetime value, not wrapping it in a lambda: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is a property (not a method), and can be properly patched in tests by directly supplying a datetime value rather than wrapping it in a lambda: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is a property rather than a method, and should be patched in tests by directly providing a datetime value: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py
📚 Learning: 2025-09-03T22:29:24.535Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py:0-0
Timestamp: 2025-09-03T22:29:24.535Z
Learning: The EncumbranceEventDetailSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py is used across multiple instances/contexts where adverseActionCategory and adverseActionId fields may be required in some cases and not present in others. This is an intentional design pattern for handling different event variants with a single schema.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py
🧬 Code graph analysis (4)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (2)
compact(47-48)batch(34-36)backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py (1)
get_privilege_purchase_options(325-400)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (4)
get_most_recent_transaction_for_compact(81-122)add_privilege_information_to_transactions(170-278)store_transactions(18-32)reconcile_unsettled_transactions(317-395)backend/compact-connect/lambdas/python/purchases/purchase_client.py (3)
get_settled_transactions(223-244)get_settled_transactions(734-913)get_settled_transactions(1081-1114)
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (7)
TransactionData(7-72)batch(34-36)transactionProcessor(26-27)compact(47-48)lineItems(39-44)transactionId(30-31)licenseeId(51-52)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/record.py (1)
UnsettledTransactionRecordSchema(77-102)backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
transaction_history_table(274-275)current_standard_datetime(253-257)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
serialize_to_database_record(205-208)from_database_record(124-139)update(182-203)
backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (3)
compact(47-48)batch(34-36)transactionId(30-31)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_default_transaction(653-682)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
serialize_to_database_record(205-208)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (5)
TransactionClient(12-395)store_unsettled_transaction(280-315)reconcile_unsettled_transactions(317-395)store_transactions(18-32)get_most_recent_transaction_for_compact(81-122)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
CCDataClass(46-208)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/record.py (1)
TransactionRecordSchema(34-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (7)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (2)
44-67: Contextual logging and live-config gating look solidWrapping the handler in
logger.append_context_keysand short‑circuiting when there is no compact/jurisdiction configuration is a good addition; it keeps logs rich and avoids unnecessary Authorize.Net calls for compacts that are not yet live.
143-199: Settlement error and 48‑hour unsettled detection flow looks consistentThe way you:
- accumulate
settlementErrorTransactionIdsacross iterations viabatchFailureErrorMessage,- only flip to
BATCH_FAILUREonce_all_transactions_processed(transaction_response)is true, and- then augment the same error payload when unreconciled unsettled transactions older than 48 hours are detected
gives a clear, single failure surface at the end of the run while still preserving all failure IDs. This matches the operational intent for compact ops alerts.
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (2)
18-32: Typedstore_transactionsimplementation looks correctSwitching
store_transactionsto acceptlist[TransactionData]and usingserialize_to_database_record()under a processor guard is clean and keeps DynamoDB writes consistent with the schema. Raising on unsupported processors is also appropriate here.
317-381: Unsettled reconciliation enhancements align with the new data modelUsing
settled_transaction_ids = {tx.transactionId for tx in settled_transactions}and loggingunreconciled_transactionswhen there are settled IDs with no matching unsettled records is a good improvement. The rest of the flow (batch delete matched unsettled, 48‑hour cutoff for old unsettled, and warning log) remains intact and reads correctly with the TransactionData type.backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py (3)
44-93: Edge-time test refactor correctly tracks the serialized representationSwitching
test_transaction_history_edge_timesto:
- create transactions via
generate_default_transaction(...),- rely on
store_transactionsto serialize via the schema, and- build
expected_transactionusingTransactionData.serialize_to_database_record()and strippingdateOfUpdatekeeps the test aligned with the actual storage format and removes hand‑rolled dicts. This is a solid improvement.
263-317: New log-verification test for unreconciled settled transactions is effective
test_reconcile_unsettled_transactions_logs_error_when_settled_transactions_not_matchedcleanly validates that:
- an error is logged when settled transactions can’t be matched to unsettled ones, and
- the
unreconciled_transactionspayload contains exactly the unmatched transaction IDs,while still ensuring functional behavior (no old unsettled IDs returned and matched unsettled records deleted). This gives good coverage of the new logging branch in
reconcile_unsettled_transactions.
318-434: get_most_recent_transaction_for_compact tests cover key behaviors; note boto3 internal couplingThe three new tests:
- verify selection of the newest transaction within the current month,
- verify fallback to the previous month when the current month is empty, and
- verify that a
ValueErroris raised after three months of empty queries, including checking the partition keys used,which together exercise the main paths of
get_most_recent_transaction_for_compact. Patching_Config.current_standard_datetimeto a fixed datetime matches the established pattern for this property. Based on learnings.The only caveat is that asserting against
KeyConditionExpression._valuescouples the test to boto3 internals; acceptable for now, but something to keep in mind if boto3 changes its implementation.
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py
Outdated
Show resolved
Hide resolved
landonshumway-ia
left a comment
landonshumway-ia
left a comment
There was a problem hiding this comment.
See CodeRabbit finding
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (1)
121-178: Settlement error propagation and unsettled reconciliation logic is coherent
- Accumulating
settlementErrorTransactionIdsacross iterations, persistingbatchFailureErrorMessageas JSON, and only flipping toBATCH_FAILUREonce_all_transactions_processedis true yields the right alerting semantics without prematurely failing in the middle of pagination.- Calling
reconcile_unsettled_transactionsevery iteration and then augmentingbatchFailureErrorMessagewhen there are >48-hour unsettled transactions (and all processing is complete) neatly captures the “old unsettled” error case without double-counting or masking prior failures.- The overall behavior—status
COMPLETEvsBATCH_FAILURE, plus inclusion ofbatchFailureErrorMessage—matches the issue description and the test coverage in the updated suite.You might consider wrapping the
batchFailureErrorMessageJSON parse/merge logic into a tiny helper to avoid repeating thejson.loads/json.dumpspattern and to centralize any future schema changes. Based on learningsbackend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py (2)
81-107: Compact/jurisdiction configuration seeding is realistic and reusable
_add_compact_configuration_datanow seeds jurisdiction and compact configuration via the shared TestDataGenerator helpers, and constructsconfiguredStatesfrom the provided jurisdiction list. This mirrors the real DynamoDB layout and keeps tests robust against future schema tweaks.If you find yourself adding more tests with the same default jurisdiction set, consider lifting the “Ohio-only” case into a constant list (e.g.,
DEFAULT_JURISDICTIONS) to reduce repetition. Based on learnings
207-243: Previous-transaction seeding helper matches production storage pattern
_add_previous_transaction_to_historybuilds aTransactionDatavia_generate_mock_transaction, adjusts the batch metadata, and persists viaTransactionClient.store_transactions, which accurately mimics how prior settled transactions exist in DynamoDB.Two minor notes:
- The default “2 days before scheduled time” behavior is a nice, simple baseline for tests validating start-time selection.
- You hard-code settlementTimeLocal as UTC−5 (“assuming EST”). That’s fine for deterministic tests, but if tests ever start asserting on local times across DST boundaries, you may want to centralize a small helper that mirrors whatever timezone logic production uses for settlementTimeLocal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py(1 hunks)backend/compact-connect/lambdas/python/purchases/purchase_client.py(9 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py(24 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1201
File: backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py:224-224
Timestamp: 2025-11-13T22:07:29.172Z
Learning: In the CompactConnect transaction history collection workflow (backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py and backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py), the cron schedule runs at 2 AM UTC but the handler queries for transactions up to 1 AM UTC. This one-hour gap is intentional to accommodate a delay between when Authorize.net reports a settlement time and when the batch actually becomes visible in their API.
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1201
File: backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py:69-112
Timestamp: 2025-11-14T18:44:41.614Z
Learning: In the CompactConnect transaction history processing workflow (backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py), when pagination occurs across multiple lambda invocations, the start_time and end_time for the query window must remain stable. The time window should only be computed on the first invocation (when lastProcessedTransactionId and currentBatchId are absent) and then passed through the event dictionary (startTime and endTime fields) for subsequent invocations. Recomputing the window on each iteration would cause transactions to be skipped when the transaction_limit is reached mid-batch.
<!--
📚 Learning: 2025-11-14T18:44:41.614Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1201
File: backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py:69-112
Timestamp: 2025-11-14T18:44:41.614Z
Learning: In the CompactConnect transaction history processing workflow (backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py), when pagination occurs across multiple lambda invocations, the start_time and end_time for the query window must remain stable. The time window should only be computed on the first invocation (when lastProcessedTransactionId and currentBatchId are absent) and then passed through the event dictionary (startTime and endTime fields) for subsequent invocations. Recomputing the window on each iteration would cause transactions to be skipped when the transaction_limit is reached mid-batch.
<!--
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.pybackend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.pybackend/compact-connect/lambdas/python/purchases/purchase_client.py
📚 Learning: 2025-11-13T22:07:29.172Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1201
File: backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py:224-224
Timestamp: 2025-11-13T22:07:29.172Z
Learning: In the CompactConnect transaction history collection workflow (backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py and backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py), the cron schedule runs at 2 AM UTC but the handler queries for transactions up to 1 AM UTC. This one-hour gap is intentional to accommodate a delay between when Authorize.net reports a settlement time and when the batch actually becomes visible in their API.
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.pybackend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.pybackend/compact-connect/lambdas/python/purchases/purchase_client.py
📚 Learning: 2025-06-17T19:05:36.255Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py
📚 Learning: 2025-08-26T15:30:29.956Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1012
File: backend/compact-connect/docs/design/README.md:503-512
Timestamp: 2025-08-26T15:30:29.956Z
Learning: In CompactConnect, the use of UTC-4:00 for privilege timeline events (encumbrances and expirations) was an agreed-upon business decision, not a technical choice made by individual developers. This fixed offset approach should be respected as an intentional organizational requirement.
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py
📚 Learning: 2025-06-17T19:02:12.761Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:02:12.761Z
Learning: In the CompactConnect codebase, the datetime comparison between config.current_standard_datetime and Cognito's UserLastModifiedDate works without timezone conversion in sandbox environment, suggesting both datetime objects are already compatible in their timezone awareness.
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py
📚 Learning: 2025-08-25T22:31:44.837Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1005
File: backend/compact-connect/lambdas/python/common/cc_common/dsa_auth.py:212-228
Timestamp: 2025-08-25T22:31:44.837Z
Learning: The datetime.fromisoformat() method in the CompactConnect DSA authentication module already supports both 'Z' and '+00:00' ISO 8601 timestamp formats, as verified by existing tests. The timezone-naive timestamp handling should be added as an additional safety measure.
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py
📚 Learning: 2025-07-08T18:40:24.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 882
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py:287-359
Timestamp: 2025-07-08T18:40:24.408Z
Learning: In the CompactConnect codebase, landonshumway-ia prefers to avoid extraneous unit tests when existing test coverage is already sufficient to catch bugs. For the get_privilege_purchase_options method's live-jurisdiction filtering logic, the existing tests in the purchases test suite provide adequate coverage without needing additional edge case tests.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is implemented as a property that returns `datetime.now(tz=UTC).replace(microsecond=0)`. The correct way to patch it in tests is by directly providing the datetime value, not wrapping it in a lambda: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is a property (not a method), and can be properly patched in tests by directly supplying a datetime value rather than wrapping it in a lambda: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is a property rather than a method, and should be patched in tests by directly providing a datetime value: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py
📚 Learning: 2025-08-21T02:51:28.199Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/common/requirements.in:4-4
Timestamp: 2025-08-21T02:51:28.199Z
Learning: In CompactConnect, the purchases lambda contains requests as a transitive dependency from the Authorize.net SDK, which is automatically resolved by pip-compile. This should not be manually removed even when requests is also available in the common layer, as it's managed automatically by the dependency resolver.
Applied to files:
backend/compact-connect/lambdas/python/purchases/purchase_client.py
🧬 Code graph analysis (3)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (4)
backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
compact_configuration_client(48-51)transaction_client(260-263)backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py (1)
get_privilege_purchase_options(325-400)backend/compact-connect/lambdas/python/purchases/purchase_client.py (3)
get_settled_transactions(224-245)get_settled_transactions(735-914)get_settled_transactions(1082-1115)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (3)
add_privilege_information_to_transactions(170-278)store_transactions(18-32)reconcile_unsettled_transactions(317-395)
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py (2)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (8)
TestDataGenerator(23-719)generate_default_transaction(653-682)put_default_jurisdiction_configuration_in_configuration_table(634-650)put_default_compact_configuration_in_configuration_table(588-604)put_default_privilege_record_in_provider_table(342-352)generate_default_privilege(311-333)generate_default_privilege_update(361-388)store_record_in_provider_table(336-339)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (2)
TransactionClient(12-395)store_transactions(18-32)
backend/compact-connect/lambdas/python/purchases/purchase_client.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (8)
TransactionData(7-72)compact(47-48)transactionStatus(67-68)submitTimeUTC(63-64)transactionType(71-72)responseCode(55-56)settleAmount(59-60)batch(34-36)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
create_new(96-121)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/compact/common.py (1)
PaymentProcessorType(13-14)
⏰ 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 (10)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (2)
44-88: End-to-end flow, compact live check, and pagination wiring look correct
- The logger context enrichment, live-compact gating via
get_privilege_purchase_options, and earlyCOMPLETEreturn when no live jurisdictions are present all look sound and match the intended “skip non-live compacts” behavior.- Using
_get_time_window_strings(event, compact)before invokingPurchaseClient.get_settled_transactionskeeps the time-window logic centralized and correctly threadslastProcessedTransactionId,currentBatchId, andprocessedBatchIdsthrough to the client.- The new response shape (including
startTime/endTime, pagination fields only when needed, and always includingcompact/scheduledTime) matches the workflow requirements and supports stable re-invocation.Also applies to: 90-120
181-213: Time-window helper correctly enforces 30‑day cap and stable pagination window
- The helper now:
- Reuses
startTime/endTimefrom the event when present, which keeps the time window stable across paginated lambda invocations.- On first invocation, computes
end_timefromscheduledTimeat 01:00 UTC and clampsstart_timebetween “most recent settled transaction + 1s” and “end_time - 30 days”, honoring the Authorize.net 31‑day limit and the intentional 1‑hour delay window.- Handles the “no previous transactions” case by logging a warning and falling back cleanly to the 30‑day window.
- The returned
'%Y-%m-%dT%H:%M:%SZ'strings align with what the purchase client expects, and tests verify both the previous-transaction and 30‑day fallback behavior.backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py (6)
31-74: Test transaction generation now aligned with shared TestDataGeneratorSwitching
_generate_mock_transactionto callTestDataGenerator.generate_default_transactionwith overrides (including the hand-builtlineItems) is a good move: it keeps tests in sync with the canonicalTransactionDataschema and reduces hand-crafted dict drift. The parametrization of transaction status, batch settlement state, and jurisdictions is clear.
109-157: Privilege and privilege‑update helpers cover both mapping paths cleanlyThe
_add_mock_privilege_to_databaseand_add_mock_privilege_update_to_databasehelpers now lean onTestDataGeneratorto seed provider-table records, including thepreviouspayload for updates. This gives good coverage for both “privilege” and “privilegeUpdate” mapping paths inTransactionClient.add_privilege_information_to_transactionswhile keeping fixture code concise.
244-279: Existing happy‑path and pagination tests now validate computed window as wellAdding the current-standard datetime patch and seeding a previous transaction in the “complete” and “IN_PROGRESS with pagination” tests, then asserting
startTimeandendTimeagainst the expected “most recent settlement + 1s” → “scheduled 01:00 UTC” window, gives strong regression coverage for the new time-window logic without overcomplicating the fixtures.Also applies to: 417-453
455-566: Batch‑failure test accurately exercises multi‑iteration error aggregationThe updated
test_process_settled_transactions_returns_batch_failure_status_after_processing_all_transactionnow:
- Seeds a previous transaction so the same time-window logic is used.
- Simulates settlement errors across two iterations with
side_effect.- Verifies
startTime/endTimestability,IN_PROGRESS→BATCH_FAILUREtransition, and concatenatedfailedTransactionIdsinbatchFailureErrorMessage.This is a solid, realistic scenario for the new workflow behavior.
811-910: Old/unsettled reconciliation tests nicely validate error and cleanup pathsThe tests for “detects_old_unsettled_transactions” and “reconciles_unsettled_transactions” exercise:
- Detection of >48‑hour unsettled transactions with a
BATCH_FAILUREresponse andunsettledTransactionIdspopulated.- Deletion of matched unsettled entries when a settled transaction arrives, with
COMPLETEstatus and no failure message.They accurately mirror the intended semantics of
TransactionClient.reconcile_unsettled_transactions.
911-1058: New window‑selection and persistence tests thoroughly cover edge casesThe four new tests at the bottom collectively validate the core time-window behavior:
uses_previous_transaction_settlement_time_for_start_timeasserts thatstart_timeis “most recent settlement + 1s”.uses_30_day_fallback_when_no_previous_transactionsasserts the 30‑day fallback when history is empty.uses_provided_start_and_end_timesensures event-suppliedstartTime/endTimeoverride recomputation and are echoed back.persists_start_and_end_times_across_pagination_iterationsconfirms that once computed, the same window is reused across paginated iterations and passed through both the response and subsequentget_settled_transactionscalls.This aligns perfectly with the pagination stability requirement captured in the previous learning.
backend/compact-connect/lambdas/python/purchases/purchase_client.py (2)
43-45: Compact threading and TransactionData usage look good
- Adding
compact: strto bothPaymentProcessorClient.get_settled_transactionsandAuthorizeNetPaymentProcessorClient.get_settled_transactions, then threading it throughPurchaseClient.get_settled_transactions, cleanly scopes queries per compact and matches the handler’s call-site.- Constructing
TransactionDataviaTransactionData.create_new(...)with a fully shaped dict (includingbatch,lineItems,transactionProcessor, andcompact) is the right way to align with the shared data model; tests asserting stored DB records confirm the shape.- The new
AuthorizeNetTransactionIgnoreStatesenum is a reasonable hook for centralizing ignorable Authorize.net statuses.If you haven’t already, it’d be worth running the updated tests that hit
TransactionClient.store_transactionsto confirm thattransaction.transactionProcessorstill compares equal toAUTHORIZE_DOT_NET_CLIENT_TYPEafter going through the schema (i.e., the enum value continues to serialize to the expected string).Also applies to: 735-756, 1082-1115
818-835: The review comment is incorrect for this codebase; the current code is correct as-is.The review assumes that in Python,
x in EnumClassonly matches enum members, not raw strings. However, this understanding does not apply to the CompactConnect codebase, which runs on Python 3.12.In Python 3.12+, StrEnum membership checking with strings works correctly, meaning the current code at lines 819 and 827—
str(tx.transactionStatus) in AuthorizeNetTransactionErrorStates—functions as intended and will match the enum values like'settlementError','generalError', and'declined'.No changes are required.
Likely an incorrect or invalid review comment.
jusdino
force-pushed
the
bug/handle-authorizenet-delayed-reporting
branch
from
1a9e9e4 to
c6d2ac4
Compare
|
@jlkravitz , this one is ready for you. The node failure looks like a new dependency vulnerability, not related to this PR, since I didn't touch anything NodeJS. We'll make a point to mop that up in one of our next PRs. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
225-240: Avoid KeyError when provider_ids is empty; fix logged fieldIf no 'privilege'/'privilegeUpdate' items exist, provider_ids is empty and pop() raises. Also the error log prints licenseeId under provider_ids.
- if len(provider_ids) > 1: - logger.error( - 'More than one matching provider id found for a transaction id.', - compact=compact, - transaction_id=transaction.transactionId, - # attempt to grab the licensee id from the authorize.net data, which may be invalid if it was masked - provider_ids=transaction.licenseeId, - ) - - # The licensee id recorded in Authorize.net cannot be trusted, as Authorize.net masks any values that look + if len(provider_ids) > 1: + logger.error( + 'More than one matching provider id found for a transaction id.', + compact=compact, + transaction_id=transaction.transactionId, + provider_ids=list(provider_ids), + ) + + # The licensee id recorded in Authorize.net cannot be trusted, as Authorize.net masks any values that look # like a credit card number (consecutive digits separated by dashes). We need to grab the provider id from # the privileges associated with this transaction and set the licensee id on the transaction to that value # to ensure it is valid. - transaction.update({'licenseeId': provider_ids.pop()}) + provider_id = next(iter(provider_ids), None) + if not provider_id: + logger.error( + 'No provider id found while adding privilege info for transaction', + compact=compact, + transaction_id=transaction.transactionId, + ) + continue + transaction.update({'licenseeId': provider_id})
🧹 Nitpick comments (4)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
652-682: Avoid mutatingvalue_overridesingenerate_default_transaction
value_overrides.pop('batch')mutates the caller-provided dict, which can surprise tests that reuse the same overrides or inspect them afterward. You can avoid side effects by copying first and working on the copy:- def generate_default_transaction(value_overrides: dict | None = None): + def generate_default_transaction(value_overrides: dict | None = None): @@ - default_batch = deepcopy(DEFAULT_COMPACT_TRANSACTION_BATCH) - if value_overrides and 'batch' in value_overrides.keys(): - default_batch.update(value_overrides.pop('batch')) + default_batch = deepcopy(DEFAULT_COMPACT_TRANSACTION_BATCH) + overrides = dict(value_overrides) if value_overrides else {} + if 'batch' in overrides: + default_batch.update(overrides.pop('batch')) @@ - if value_overrides: - default_transaction.update(value_overrides) + if overrides: + default_transaction.update(overrides)This keeps the external API safer without changing behavior.
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py (1)
959-964: Align fallback window comment with the asserted start timeIn
test_process_settled_transactions_uses_30_day_fallback_when_no_previous_transactions, the comment says:# oldest_allowed_start = end_time - timedelta(days=30) + timedelta(seconds=1)but
expected_start_timeis computed asend_time - timedelta(days=30)(no extra second). Either the comment or the expectation is stale; updating one of them to match the intended behavior will avoid confusion for future readers.backend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py (1)
70-85: Optional: tighten MagicMock specUsing spec=json_obj on a dict doesn’t constrain accidental attribute typos. Consider spec_set with explicit keys to catch test drift.
- if isinstance(json_obj, dict): - # set to the spec of the object so that extra attributes do not return True for hasattr - mock = MagicMock(spec=json_obj) + if isinstance(json_obj, dict): + # restrict attributes to the JSON keys to catch typos in tests + mock = MagicMock(spec_set=list(json_obj.keys()))backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (1)
121-146: Batch failure aggregation works; consider de-duplicationMerging failedTransactionIds across iterations is good. Optionally de-duplicate to avoid repeats on retries.
- batch_failure_error_message['failedTransactionIds'].extend(failed_transactions_ids) + existing = set(batch_failure_error_message.get('failedTransactionIds', [])) + existing.update(failed_transactions_ids) + batch_failure_error_message['failedTransactionIds'] = sorted(existing)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/purchase/api.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py(12 hunks)backend/compact-connect/lambdas/python/common/common_test/test_constants.py(2 hunks)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py(2 hunks)backend/compact-connect/lambdas/python/common/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py(7 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_transaction.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_transaction_client.py(8 hunks)backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py(1 hunks)backend/compact-connect/lambdas/python/purchases/purchase_client.py(9 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py(24 hunks)backend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py(4 hunks)backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/init.py
- backend/compact-connect/lambdas/python/common/tests/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/purchase/api.py
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1201
File: backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py:224-224
Timestamp: 2025-11-13T22:07:29.172Z
Learning: In the CompactConnect transaction history collection workflow (backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py and backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py), the cron schedule runs at 2 AM UTC but the handler queries for transactions up to 1 AM UTC. This one-hour gap is intentional to accommodate a delay between when Authorize.net reports a settlement time and when the batch actually becomes visible in their API.
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
📚 Learning: 2025-11-14T18:44:41.614Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1201
File: backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py:69-112
Timestamp: 2025-11-14T18:44:41.614Z
Learning: In the CompactConnect transaction history processing workflow (backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py), when pagination occurs across multiple lambda invocations, the start_time and end_time for the query window must remain stable. The time window should only be computed on the first invocation (when lastProcessedTransactionId and currentBatchId are absent) and then passed through the event dictionary (startTime and endTime fields) for subsequent invocations. Recomputing the window on each iteration would cause transactions to be skipped when the transaction_limit is reached mid-batch.
<!--
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.pybackend/compact-connect/lambdas/python/purchases/handlers/transaction_history.pybackend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.pybackend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.pybackend/compact-connect/lambdas/python/purchases/purchase_client.py
📚 Learning: 2025-11-13T22:07:29.172Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1201
File: backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py:224-224
Timestamp: 2025-11-13T22:07:29.172Z
Learning: In the CompactConnect transaction history collection workflow (backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py and backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py), the cron schedule runs at 2 AM UTC but the handler queries for transactions up to 1 AM UTC. This one-hour gap is intentional to accommodate a delay between when Authorize.net reports a settlement time and when the batch actually becomes visible in their API.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.pybackend/compact-connect/lambdas/python/purchases/handlers/transaction_history.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.pybackend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.pybackend/compact-connect/lambdas/python/purchases/purchase_client.py
📚 Learning: 2025-07-08T18:40:24.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 882
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py:287-359
Timestamp: 2025-07-08T18:40:24.408Z
Learning: In the CompactConnect codebase, landonshumway-ia prefers to avoid extraneous unit tests when existing test coverage is already sufficient to catch bugs. For the get_privilege_purchase_options method's live-jurisdiction filtering logic, the existing tests in the purchases test suite provide adequate coverage without needing additional edge case tests.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.pybackend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is implemented as a property that returns `datetime.now(tz=UTC).replace(microsecond=0)`. The correct way to patch it in tests is by directly providing the datetime value, not wrapping it in a lambda: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.pybackend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is a property (not a method), and can be properly patched in tests by directly supplying a datetime value rather than wrapping it in a lambda: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.pybackend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, `cc_common.config._Config.current_standard_datetime` is a property rather than a method, and should be patched in tests by directly providing a datetime value: `patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP))`.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.pybackend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py
📚 Learning: 2025-06-17T19:05:36.255Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.pybackend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py
📚 Learning: 2025-08-26T15:30:29.956Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1012
File: backend/compact-connect/docs/design/README.md:503-512
Timestamp: 2025-08-26T15:30:29.956Z
Learning: In CompactConnect, the use of UTC-4:00 for privilege timeline events (encumbrances and expirations) was an agreed-upon business decision, not a technical choice made by individual developers. This fixed offset approach should be respected as an intentional organizational requirement.
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.pybackend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py
📚 Learning: 2025-06-17T19:02:12.761Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:02:12.761Z
Learning: In the CompactConnect codebase, the datetime comparison between config.current_standard_datetime and Cognito's UserLastModifiedDate works without timezone conversion in sandbox environment, suggesting both datetime objects are already compatible in their timezone awareness.
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.pybackend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py
📚 Learning: 2025-08-25T22:31:44.837Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1005
File: backend/compact-connect/lambdas/python/common/cc_common/dsa_auth.py:212-228
Timestamp: 2025-08-25T22:31:44.837Z
Learning: The datetime.fromisoformat() method in the CompactConnect DSA authentication module already supports both 'Z' and '+00:00' ISO 8601 timestamp formats, as verified by existing tests. The timezone-naive timestamp handling should be added as an additional safety measure.
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py
📚 Learning: 2025-08-21T02:51:28.199Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/common/requirements.in:4-4
Timestamp: 2025-08-21T02:51:28.199Z
Learning: In CompactConnect, the purchases lambda contains requests as a transitive dependency from the Authorize.net SDK, which is automatically resolved by pip-compile. This should not be manually removed even when requests is also available in the common layer, as it's managed automatically by the dependency resolver.
Applied to files:
backend/compact-connect/lambdas/python/purchases/purchase_client.py
🧬 Code graph analysis (9)
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py (2)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (8)
TestDataGenerator(23-719)generate_default_transaction(653-682)put_default_jurisdiction_configuration_in_configuration_table(634-650)put_default_compact_configuration_in_configuration_table(588-604)put_default_privilege_record_in_provider_table(342-352)generate_default_privilege(311-333)generate_default_privilege_update(361-388)store_record_in_provider_table(336-339)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (2)
TransactionClient(12-395)store_transactions(18-32)
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_transaction_client.py (4)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_default_transaction(653-682)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (1)
store_transactions(18-32)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
update(182-203)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (2)
lineItems(39-44)licenseeId(51-52)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py (1)
get_privilege_purchase_options(325-400)backend/compact-connect/lambdas/python/purchases/purchase_client.py (3)
get_settled_transactions(224-245)get_settled_transactions(735-914)get_settled_transactions(1082-1115)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (4)
add_privilege_information_to_transactions(170-278)store_transactions(18-32)reconcile_unsettled_transactions(317-395)get_most_recent_transaction_for_compact(81-122)
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_transaction.py (5)
backend/compact-connect/lambdas/python/common/tests/__init__.py (1)
TstLambdas(9-112)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (2)
TestDataGenerator(23-719)generate_default_transaction(653-682)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/record.py (1)
TransactionRecordSchema(34-73)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
serialize_to_database_record(205-208)from_database_record(124-139)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (12)
TransactionData(7-72)transactionProcessor(26-27)transactionId(30-31)batch(34-36)lineItems(39-44)compact(47-48)licenseeId(51-52)responseCode(55-56)settleAmount(59-60)submitTimeUTC(63-64)transactionStatus(67-68)transactionType(71-72)
backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (7)
TransactionData(7-72)batch(34-36)transactionProcessor(26-27)compact(47-48)lineItems(39-44)transactionId(30-31)licenseeId(51-52)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/record.py (1)
UnsettledTransactionRecordSchema(77-102)backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
transaction_history_table(274-275)current_standard_datetime(253-257)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
serialize_to_database_record(205-208)from_database_record(124-139)update(182-203)
backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (3)
compact(47-48)batch(34-36)transactionId(30-31)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_default_transaction(653-682)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
serialize_to_database_record(205-208)backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py (5)
TransactionClient(12-395)store_unsettled_transaction(280-315)reconcile_unsettled_transactions(317-395)store_transactions(18-32)get_most_recent_transaction_for_compact(81-122)
backend/compact-connect/lambdas/python/purchases/purchase_client.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (8)
TransactionData(7-72)compact(47-48)transactionStatus(67-68)submitTimeUTC(63-64)transactionType(71-72)responseCode(55-56)settleAmount(59-60)batch(34-36)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
create_new(96-121)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/compact/common.py (1)
PaymentProcessorType(13-14)
backend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (6)
transactionId(30-31)compact(47-48)licenseeId(51-52)batch(34-36)lineItems(39-44)transactionStatus(67-68)backend/compact-connect/lambdas/python/purchases/purchase_client.py (3)
get_settled_transactions(224-245)get_settled_transactions(735-914)get_settled_transactions(1082-1115)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (1)
TransactionData(7-72)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
update(182-203)create_new(96-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (19)
backend/compact-connect/stacks/transaction_monitoring_stack/transaction_history_processing_workflow.py (1)
219-224: LGTM! Schedule change correctly implements the timing adjustment.The schedule change from 1 AM to 2 AM UTC and the updated comment accurately reflect the PR objective to accommodate Authorize.net's settlement reporting delays. The timing calculation is correct (6pm PST = 2am UTC), and the 2-hour buffer from the default 4pm Pacific settlement time provides adequate coverage for late settlements.
Based on learnings.
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_transaction.py (1)
12-47: Good coverage of transaction schema and data class behaviorThese tests nicely cover happy-path serde, validation failures (missing
transactionId, invalidtransactionProcessor), the data-class getters, and the exact DB serialization shape (minus the dynamicdateOfUpdate). This should catch regressions in both the Marshmallow schema andTransactionDatabehavior.Also applies to: 55-127
backend/compact-connect/lambdas/python/common/common_test/test_constants.py (1)
32-62: Transaction constants align with schema and test expectationsThe new batch and line-item defaults (including quantities, prices, and optional
privilegeId) andTRANSACTION_RECORD_TYPEmatch the structures exercised in the transaction schema and client tests. They provide a solid, centralized fixture for transaction-related tests.Also applies to: 91-91
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_transaction_client.py (1)
1-9: Tests correctly exercise TransactionClient with the new TransactionData modelUsing
TestDataGenerator.generate_default_transactionand the shared line-item constants keeps these tests consistent with the schema and other suites. The assertions on stored items (including pk/sk and dynamicdateOfUpdate), unsupported processor handling, and privilege/ provider-id mapping all look accurate for the current TransactionClient implementation.Also applies to: 29-60, 64-77, 107-136, 155-167, 179-182
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_history.py (2)
34-74: Time-window & pagination tests look consistent with the new transaction-history behaviorThe refactor to:
- Generate transactions via
TestDataGenerator.generate_default_transaction,- Seed compact/jurisdiction/privilege data through shared helpers, and
- Use
_add_previous_transaction_to_history+TransactionClientto simulate prior settlements,produces clear, high-signal tests around:
- Computing
startTimefrom the most recent prior settlement (plus one second).- Falling back to a bounded lookback window when no prior transactions exist.
- Respecting provided
startTime/endTimein the event.- Persisting
startTimeandendTimeacross paginated iterations so the window stays stable.Combined with the existing assertions on DynamoDB contents and status values (
COMPLETE,IN_PROGRESS,BATCH_FAILURE), this gives strong coverage of the updated handler semantics.Also applies to: 81-107, 109-157, 207-243, 911-1058
223-232: Python runtime confirmed; no compatibility issueVerification shows Python 3.11.2 is in use, which fully supports
datetime.fromisoformatwith the'Z'suffix. The code across all flagged lines (223–232, 261–267, 433–439, 515–520, 920–923, 961–964) is compatible with the runtime. Additionally, the learnings confirm this pattern is already tested and proven in the codebase.backend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py (4)
1115-1121: Attribute-style assertions look correctSwitch to TransactionData properties reads cleanly and matches the model API.
1200-1203: Pagination assertions align with new data modelVerifies IDs via TransactionData; good coverage for batched paging.
Also applies to: 1221-1223
1293-1298: Settlement error assertions updated correctlyUsing transaction.transactionId/transactionStatus is consistent with TransactionData.
1299-1376: Nice addition: declined transactions are skippedTest precisely captures the ignore-state behavior; this protects reports from noise.
backend/compact-connect/lambdas/python/purchases/handlers/transaction_history.py (3)
69-76: Stable window computation is correctreuses start/end across invocations; prevents mid-batch skipping. Matches the 2:00→1:00 UTC gap design.
101-120: Response threading is completestartTime/endTime returned with pagination fields only when needed; good for Step Functions replayability.
181-213: No action needed—code is compatible with Python 3.12Python 3.11 and 3.12 both support parsing timestamps with a trailing "Z" via datetime.fromisoformat(). Since the repository targets Python 3.12, the code correctly relies on this support without requiring normalization to
+00:00.backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py (3)
318-356: Good coverage: current-month selectionValidates the “most recent in current month” path with deterministic timestamps.
356-386: Good coverage: previous-month fallbackExercises month-walkback logic and ensures correct PK selection.
387-435: Negative path is well-testedMocks 3 queries and asserts correct PKs; failure message verified.
backend/compact-connect/lambdas/python/purchases/purchase_client.py (3)
859-881: TransactionData construction is correctFields are string-cast for JSON safety; compact is included; good alignment with schema.
790-899: Reject offset calculation; original code is correct for page-based paginationThe web search confirms Authorize.Net's
Paging.offsetis a 1-based page number (not a record offset). The API docs explicitly state that "offset — The number of the page to return results from. For example, if you use alimitof 100, settingoffsetto 1 will return the first 100 transactions, settingoffsetto 2 will return the second 100 transactions".The proposed diff calculating
(page_offset - 1) * MAXIMUM_TRANSACTION_API_LIMIT + 1is incorrect and would break pagination. The original code correctly treatspage_offsetas a page number and should not be modified with that formula.However, the loop logic concern regarding
transactions_in_pagebeing set fromtotalNumInResultSet(whether that represents the total overall or count in the current page) and the while condition termination still requires verification against the API response schema to confirm if the pagination loop has a genuine issue.
827-835: Runtime verified as Python 3.12; code is correctThe Lambda runtime is Python 3.12, which satisfies the requirement and supports StrEnum membership checks with strings. The pattern
str(tx.transactionStatus) in AuthorizeNetTransactionIgnoreStateswill work correctly at runtime.
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
LGTM! @isabeleliassen Good to merge.
4 participants
Description List
Testing List
Closes #1192
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores