Turn off txn reporting for pre-live compacts #996
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughThe code introduces early exit logic to transaction processing and reporting functions, ensuring they only operate for compacts that are live with valid configuration and jurisdictions. Tests are updated and expanded to cover these early exit scenarios. Infrastructure is adjusted to grant necessary permissions and update alarm notification behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant StepFunction
participant LambdaHandler
participant ConfigClient
participant Logger
StepFunction->>LambdaHandler: Invoke with event
LambdaHandler->>ConfigClient: Get compact & jurisdiction configs
ConfigClient-->>LambdaHandler: Return configs
alt Compact or jurisdictions missing
LambdaHandler->>Logger: Log warning
LambdaHandler-->>StepFunction: Return early (status: COMPLETE)
else Configs present
LambdaHandler->>LambdaHandler: Continue transaction/report processing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15-20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
landonshumway-ia
requested changes
Aug 7, 2025
Collaborator
landonshumway-ia
left a comment
landonshumway-ia
left a comment
There was a problem hiding this comment.
The code changes look good. Since we are currently in this space, I found from our testing with the OT authorize.net account that there is another transactionStatus value, generalError, that did not show up in the sandbox environment which we will want to account for. I've marked the place in the reporting where these will need to be filtered, and we will also need to add a check for this status type in the conditional here:
backend/compact-connect/lambdas/python/purchases/handlers/transaction_reporting.py
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/purchases/purchase_client.py (1)
799-799: Verify the intentional broadening of error detection logic.The change from
str(tx.transactionStatus) == 'settlementError'tostr(tx.transactionStatus) in AuthorizeNetTransactionErrorStatesexpands error detection to include both 'settlementError' and 'generalError' states. This broadens the scope of transactions flagged as having errors.Consider updating the variable name
settlement_error_transaction_idsto reflect that it now includes general errors too, perhapserror_transaction_idsorfailed_transaction_ids.- settlement_error_transaction_ids = [] + error_transaction_ids = []And update the corresponding references in the method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/purchases/handlers/transaction_reporting.py(3 hunks)backend/compact-connect/lambdas/python/purchases/purchase_client.py(4 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_reporting.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/compact-connect/lambdas/python/purchases/handlers/transaction_reporting.py
- backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_reporting.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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
PR: csg-org/CompactConnect#882
File: backend/compact-connect/lambdas/python/migration/compact_configured_states_871/main.py:127-130
Timestamp: 2025-06-26T16:42:00.781Z
Learning: In the compact_configured_states_871 migration, existing jurisdiction configurations that have licenseeRegistrationEnabled: true are migrated with isLive: true for backwards compatibility. This prevents breaking existing live functionality since those states are already operational. The migration preserves the current live state of jurisdictions to maintain service continuity, while new states added after migration can start with isLive: false and be controlled by compact admins.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#852
File: backend/compact-connect/stacks/persistent_stack/__init__.py:545-549
Timestamp: 2025-06-09T22:03:03.232Z
Learning: In the CompactConnect codebase, production active_compact_member_jurisdictions configurations are very stable and unlikely to be removed. The vast majority of configuration errors in get_list_of_active_jurisdictions_for_compact_environment() will occur when developers are deploying sandbox environments, so error messages that primarily reference sandbox configuration are appropriate for the typical use case.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:138-147
Timestamp: 2025-04-29T02:52:40.532Z
Learning: In CompactConnect tests, hardcoded values (like license type abbreviations 'slp') in test queries are sometimes used intentionally rather than using dynamic lookups. This is a deliberate design decision to make tests fail if default test data changes, requiring developers to consciously update related tests.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/migration/provider_user_pool_migration_551/main.py:35-39
Timestamp: 2025-06-10T03:16:16.896Z
Learning: In the provider user pool migration (provider_user_pool_migration_551), the FilterExpression intentionally only checks for the existence of compactConnectRegisteredEmailAddress. The migration should only remove currentHomeJurisdiction if compactConnectRegisteredEmailAddress is also present, targeting records that went through the old registration process. Records with only currentHomeJurisdiction but no compactConnectRegisteredEmailAddress should be left untouched.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#952
File: backend/compact-connect/lambdas/python/migration/license_update_effective_date_931/main.py:0-0
Timestamp: 2025-07-29T02:35:59.401Z
Learning: For temporary migration scripts in CompactConnect that process pre-vetted database records and have been verified in development environments, extensive validation for data format may be unnecessary overhead. The user landonshumway-ia indicated that migration scripts can rely on data integrity when the records have been vetted and the script has been tested.
🔇 Additional comments (2)
backend/compact-connect/lambdas/python/purchases/purchase_client.py (2)
1-7: LGTM! Good enum design for standardizing error states.The addition of
AuthorizeNetTransactionErrorStatesenum follows best practices by usingStrEnumfor string-based error states and centralizing previously hardcoded values. This improves type safety and maintainability.Also applies to: 42-45
621-621: LGTM! Clean enum usage for settlement error checking.The replacement of the hardcoded string
'settlementError'withAuthorizeNetTransactionErrorStates.SettlementErrormaintains identical behavior while improving type safety and maintainability.
landonshumway-ia
approved these changes
Aug 11, 2025
Collaborator
landonshumway-ia
left a comment
landonshumway-ia
left a comment
There was a problem hiding this comment.
Thanks for adding that 👍
Collaborator
|
@jlkravitz This is ready for your review. Thanks |
jlkravitz
reviewed
Aug 11, 2025
Collaborator
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
one small question but otherwise looks good!
jlkravitz
approved these changes
Aug 13, 2025
Collaborator
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
@isabeleliassen good to merge!
Collaborator
|
@jlkravitz let me know when that conversation is resolved! |
Collaborator
|
@isabeleliassen sorry-done! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description List
Testing List
Closes #766
Summary by CodeRabbit
Bug Fixes
Tests
Chores