Feat/deactivate privileges when home state license deactivated #870
Conversation
WalkthroughThis change introduces a comprehensive license deactivation workflow: when a home state license is deactivated, an event triggers automatic deactivation of all related privileges. The implementation spans event schemas, handlers, privilege status updates, CDK infrastructure, and extensive unit and smoke tests to ensure correct propagation and system behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant State as State System
participant API as Provider Data API
participant EventBus as Event Bus
participant Lambda as License Deactivation Listener Lambda
participant DB as Data Store
State->>API: Uploads license with deactivated status
API->>EventBus: Publishes license.deactivation event
EventBus->>Lambda: Triggers license_deactivation_listener
Lambda->>DB: Finds all privileges for provider/license
Lambda->>DB: Updates privileges to set licenseDeactivatedStatus
Lambda->>DB: Writes privilege update records
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py (1)
64-138: Consider function refactoring to reduce complexity.While the polling logic is correct and the function works as intended, static analysis correctly identifies high complexity. The function has 7 parameters and 17 local variables, which could impact maintainability.
Consider extracting configuration into a dataclass to reduce parameter count:
+from dataclasses import dataclass + +@dataclass +class PrivilegeValidationConfig: + provider_id: str + compact: str + license_jurisdiction: str + license_type: str + max_wait_time: int = 120 + check_interval: int = 15 + -def validate_privilege_deactivation( - staff_headers: dict, - provider_id: str, - compact: str, - license_jurisdiction: str, - license_type: str, - max_wait_time: int = 120, - check_interval: int = 15, -): +def validate_privilege_deactivation(staff_headers: dict, config: PrivilegeValidationConfig):
📜 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/compact_configuration_utils.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(4 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py(4 hunks)backend/compact-connect/lambdas/python/data-events/handlers/license_deactivation_events.py(1 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_license_deactivation_events.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py(2 hunks)backend/compact-connect/lambdas/python/purchases/handlers/privileges.py(2 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py(1 hunks)backend/compact-connect/stacks/event_listener_stack/__init__.py(4 hunks)backend/compact-connect/tests/app/test_event_listener.py(1 hunks)backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py(1 hunks)backend/compact-connect/tests/smoke/smoke_common.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py
[refactor] 87-87: Too few public methods (0/2)
(R0903)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py
[error] 387-387: Too many positional arguments for function call
(E1121)
backend/compact-connect/lambdas/python/data-events/tests/function/test_license_deactivation_events.py
[error] 74-74: Too many positional arguments for function call
(E1121)
[error] 118-118: Too many positional arguments for function call
(E1121)
[error] 159-159: Too many positional arguments for function call
(E1121)
[error] 203-203: Too many positional arguments for function call
(E1121)
[error] 254-254: Too many positional arguments for function call
(E1121)
[error] 298-298: Too many positional arguments for function call
(E1121)
backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py
[refactor] 64-64: Too many arguments (7/5)
(R0913)
[refactor] 64-64: Too many positional arguments (7/5)
(R0917)
[refactor] 64-64: Too many local variables (17/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (38)
backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_utils.py (1)
64-69: LGTM! Clean addition of U.S. Territories support.The jurisdiction mapping extensions for U.S. Territories are correctly formatted and follow the existing pattern. This ensures that the license deactivation feature will properly handle privileges and licenses from these jurisdictions.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
79-81: LGTM! Consistent property implementation.The
licenseDeactivatedStatusproperty follows the established pattern of other optional status properties in the class, using.get()with aNonedefault and proper type annotation.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (2)
10-10: LGTM! Proper import for the new enum.The import of
LicenseDeactivatedStatusEnumis correctly placed with other enum imports.
87-89: LGTM! Consistent field implementation.The
LicenseDeactivatedStatusFieldfollows the established pattern of other status validation fields in this file, properly inheriting fromStringand usingOneOfvalidation against the enum values. The pylint warning about too few public methods is a false positive for marshmallow validation fields.backend/compact-connect/lambdas/python/data-events/handlers/license_deactivation_events.py (1)
1-40: LGTM! Well-structured event handler implementation.The license deactivation event handler is well-implemented with:
- Proper SQS handler decoration
- Clear extraction of event details
- Good structured logging with context
- Appropriate delegation to the data client for privilege deactivation
The implementation follows best practices and integrates well with the existing event-driven architecture.
Verify that the license type abbreviation mapping is comprehensive to avoid potential KeyError:
#!/bin/bash # Description: Check if license type abbreviation mappings exist for all compacts and license types used in the system. # Expected: All license types should have corresponding abbreviations defined. # Search for license type abbreviation usage patterns rg -A 5 "license_type_abbreviations" --type py # Search for compact and license type configurations rg -A 10 "license_types_for_compact\|COMPACT_NAME_MAPPING" --type pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
266-267: LGTM! Clear enum addition with helpful documentation.The
LICENSE_DEACTIVATIONvalue is properly added to theUpdateCategoryenum with a clear comment explaining its specific use for privilege deactivation due to state license deactivation.
304-313: LGTM! Well-documented enum following established patterns.The
LicenseDeactivatedStatusEnumis properly implemented following the same pattern asHomeJurisdictionChangeStatusEnum. The documentation clearly explains its purpose and constraints, making it clear that this status will only appear for inactive privileges.backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py (1)
239-269: Well-implemented license deactivation event filtering logic.The implementation correctly distinguishes between expired licenses and jurisdiction-deactivated licenses, only emitting deactivation events for the latter. The logging provides good visibility and the event payload includes all necessary fields including the newly added
licenseType.backend/compact-connect/lambdas/python/purchases/handlers/privileges.py (2)
11-11: Appropriate import addition for license deactivation status handling.The import of
LicenseDeactivatedStatusEnumis correctly added to support the new validation logic.
297-299: Logical addition to privilege renewal validation.The condition correctly allows privilege renewal for previously license-deactivated privileges, following the same pattern as existing conditions for admin-deactivated and home-jurisdiction-changed privileges. This enables the intended workflow where privileges can be renewed after license reactivation.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py (2)
2-2: Appropriate import addition for date handling in tests.The
dateimport is correctly added to support date comparisons in the new test method.
340-392: ```shell
#!/bin/bashLocate the definition of ingest_license_message to verify its signature
rg -n "def ingest_license_message" .
Locate all calls to ingest_license_message to confirm how many arguments are passed
rg -n "ingest_license_message\s*(" .
</details> <details> <summary>backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py (1)</summary> `1003-1083`: **Comprehensive test coverage for license deactivation status removal.** This test effectively validates the license deactivation renewal workflow: 1. **Setup**: Creates a privilege with `licenseDeactivatedStatus` and an active license 2. **Action**: Renews the privilege through the purchase handler 3. **Verification**: Confirms the status field is removed and privilege becomes active 4. **Audit trail**: Ensures the removal is tracked in privilege update records The test follows established patterns and provides good coverage for this edge case scenario. </details> <details> <summary>backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py (3)</summary> `23-23`: **LGTM! Well-integrated field addition.** The `licenseDeactivatedStatus` field follows the established pattern of other status fields in the schema with clear documentation and proper typing. Also applies to: 92-94 --- `144-144`: **Correct integration with status calculation logic.** The field is properly included in the `_calculate_status` method to ensure privileges are marked as INACTIVE when this field is present, maintaining consistency with other deactivation status fields. --- `179-180`: **Proper inclusion in update schema.** The field is correctly added to `PrivilegeUpdatePreviousRecordSchema` to support tracking changes to this field in privilege update records. </details> <details> <summary>backend/compact-connect/tests/app/test_event_listener.py (1)</summary> `224-321`: **Comprehensive infrastructure test following established patterns.** This test properly validates all components of the license deactivation event listener: - Lambda function with correct handler `handlers.license_deactivation_events.license_deactivation_listener` - SQS queue with appropriate configuration (12-hour retention, DLQ, 5-minute visibility timeout) - EventBridge rule listening for `license.deactivation` events - Event source mapping with proper batch configuration The implementation follows the exact pattern used for existing encumbrance listeners, ensuring consistency. </details> <details> <summary>backend/compact-connect/stacks/event_listener_stack/__init__.py (3)</summary> `35-35`: **Proper integration of license deactivation listener.** The new listener is correctly added to the stack initialization, following the established pattern. --- `48-48`: **Reasonable timeout increase for processing complexity.** Increasing the timeout from 1 to 2 minutes for all event listeners is appropriate given the potential complexity of privilege deactivation operations and database updates. Also applies to: 96-96 --- `131-174`: **Well-structured license deactivation listener implementation.** The `_add_license_deactivation_listener` method follows the established pattern: - Proper Lambda configuration with correct handler and environment variables - Appropriate IAM permissions for provider table access - CDK NAG suppressions with clear justification - QueueEventListener setup with correct detail type `license.deactivation` The implementation is consistent with existing encumbrance listeners. </details> <details> <summary>backend/compact-connect/lambdas/python/data-events/tests/function/test_license_deactivation_events.py (7)</summary> `18-41`: **Well-structured test setup and helper methods.** The test class setup with proper mocking and helper methods for generating test data follows established patterns. The message generation and SQS event creation utilities are clean and reusable. --- `42-97`: **Comprehensive test for active privilege deactivation.** This test effectively validates the core functionality: - Sets up both active and already-deactivated privileges - Verifies only the active privilege gets deactivated - Confirms existing deactivated privileges remain unchanged - Uses proper assertions with enum values The test logic is sound and comprehensive. --- `98-129`: **Good edge case coverage for no matching privileges.** This test ensures the handler gracefully handles scenarios where no privileges match the deactivation criteria, preventing unintended side effects. --- `130-172`: **Thorough testing of multiple privilege deactivation.** This test validates that multiple matching privileges are all correctly deactivated, ensuring the handler processes all applicable records. --- `173-223`: **Excellent boundary testing for selective deactivation.** These tests verify that only privileges matching the specific license type and jurisdiction are affected, leaving others unchanged. This is crucial for preventing over-broad deactivation. Also applies to: 224-269 --- `270-312`: **Important audit trail verification.** This test ensures that privilege update records are properly created for all affected privileges, providing necessary audit trails for the deactivation actions. --- `74-74`: ```shell #!/bin/bash # Locate the definition of license_deactivation_listener and display its signature rg -n "def license_deactivation_listener" -A2backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py (3)
38-61: LGTM! Well-structured API interaction function.The function properly handles the API request for provider details with appropriate error handling and response validation.
141-273: Excellent comprehensive test workflow implementation.The test function follows best practices with proper setup, execution, validation, and cleanup phases. The use of try-finally ensures cleanup occurs even on test failure, and the test covers the complete end-to-end scenario.
275-296: LGTM! Clean test runner implementation.The runner function provides good error handling and logging for the complete test suite execution.
backend/compact-connect/tests/smoke/smoke_common.py (5)
243-300: LGTM! Well-implemented license upload utility function.The function provides a clean interface for uploading license records with sensible defaults and override capability. Error handling and logging are appropriate.
302-336: LGTM! Clean provider query implementation.The function handles API requests properly with appropriate error handling and response parsing logic.
338-386: LGTM! Robust polling implementation for provider creation.The function implements proper retry logic with configurable timeouts and intervals. The polling approach is appropriate for eventual consistency scenarios in distributed systems.
388-444: LGTM! Comprehensive privilege record creation.The function creates well-structured DynamoDB records with proper key formatting and required fields. The use of UTC timestamps and proper data types is appropriate.
446-471: LGTM! Proper cleanup implementation with error resilience.The cleanup function handles potential errors gracefully, which is important for test teardown scenarios. The broad exception handling is appropriate here since cleanup should be best-effort.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (3)
25-25: LGTM!The import for
LicenseDeactivatedStatusEnumis correctly added and properly grouped with other enum imports.
451-460: Correctly handles reactivation of previously deactivated privileges.The implementation properly removes both
homeJurisdictionChangeStatusandlicenseDeactivatedStatusfields when a privilege is renewed, ensuring that previously deactivated privileges are reactivated upon renewal.
1048-1073: Well-implemented helper method following established patterns.The
_generate_set_privilege_license_deactivated_status_itemmethod correctly generates DynamoDB transaction items and follows the same pattern as other status update methods in the class.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
2601-2684: Consider adding license type validation for consistency.The method takes a
license_typeparameter but doesn't validate it, which is inconsistent with similar methods likeencumber_home_jurisdiction_license_privilegesandlift_home_jurisdiction_license_privilege_encumbrancesthat validate license type abbreviations.For consistency and robustness, consider either:
- Taking
license_type_abbreviationparameter and validating it with_validate_license_type_abbreviation, or- Adding validation to ensure the
license_typeparameter is validThis would prevent potential issues if callers pass invalid license types or abbreviations instead of full names.
Example approach following the established pattern:
def deactivate_home_jurisdiction_license_privileges( self, compact: str, provider_id: str, jurisdiction: str, - license_type: str, + license_type_abbreviation: str, ) -> None: """ Deactivate all privileges associated with a home jurisdiction license due to license deactivation. ... - :param str license_type: The license type + :param str license_type_abbreviation: The license type abbreviation """ # Get all provider records provider_user_records: ProviderUserRecords = self.get_provider_user_records( compact=compact, provider_id=provider_id, consistent_read=True ) + # Validate the license type abbreviation + license_type = self._validate_license_type_abbreviation(compact, license_type_abbreviation) + # Find privileges associated with the license that was deactivated, which themselves are not currently # license-deactivated active_privileges_associated_with_license = provider_user_records.get_privilege_records( filter_condition=lambda p: ( p.licenseJurisdiction == jurisdiction and p.licenseType == license_type and p.licenseDeactivatedStatus is None ) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(4 hunks)backend/compact-connect/lambdas/python/data-events/handlers/license_deactivation_events.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/data-events/handlers/license_deactivation_events.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (3)
25-25: LGTM!The import addition for
LicenseDeactivatedStatusEnumis correctly placed and necessary for the new license deactivation functionality.
451-460: LGTM!The privilege renewal logic correctly handles removal of
licenseDeactivatedStatuswhen privileges are renewed, following the same pattern ashomeJurisdictionChangeStatus. The implementation properly tracks removed values in the update record.
1048-1073: LGTM!The helper method follows the established pattern for generating DynamoDB transaction items. The implementation correctly constructs the update expression, handles serialization, and includes proper timestamp updates.
backend/compact-connect/lambdas/python/data-events/handlers/license_deactivation_events.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py (5)
2-10: Convert docstrings to reST format as requested in previous review.The docstrings should use reST format to maintain consistency with the codebase standards.
Apply this diff to convert to reST format:
-""" -Smoke tests for license deactivation privilege functionality. - -This module contains end-to-end tests for the license deactivation workflow where privileges -are automatically deactivated when their associated home state license is -deactivated by a jurisdiction. - -The tests create their own test data from scratch and clean up after themselves. -""" +""" +Smoke tests for license deactivation privilege functionality. + +This module contains end-to-end tests for the license deactivation workflow where privileges +are automatically deactivated when their associated home state license is +deactivated by a jurisdiction. + +The tests create their own test data from scratch and clean up after themselves. +"""
39-47: Convert function docstring to reST format.The function docstring should use reST format for consistency.
Apply this diff:
- """ - Get provider details from the staff API endpoint. - - :param staff_headers: Authentication headers for staff user - :param compact: The compact abbreviation - :param provider_id: The provider's ID - :returns: Provider details from the API including privileges list - :raises SmokeTestFailureException: If the API request fails - """ + """ + Get provider details from the staff API endpoint. + + :param staff_headers: Authentication headers for staff user + :param compact: The compact abbreviation + :param provider_id: The provider's ID + :return: Provider details from the API including privileges list + :raises SmokeTestFailureException: If the API request fails + """
141-157: Convert function docstring to reST format.The function docstring should follow reST format for consistency.
Apply this diff:
- """ - The test validates that when a home state license is deactivated, - any privileges associated with that license are automatically - deactivated as well. - - This test performs the following steps: - - 1. Upload an active license and wait for provider creation - 2. Update provider registration details - 3. Create a test privilege record - 4. Upload the same license with inactive status - 5. Validate that the privilege is automatically deactivated - 6. Clean up all test data - - :raises SmokeTestFailureException: If any step of the workflow fails - """ + """ + The test validates that when a home state license is deactivated, + any privileges associated with that license are automatically + deactivated as well. + + This test performs the following steps: + + 1. Upload an active license and wait for provider creation + 2. Update provider registration details + 3. Create a test privilege record + 4. Upload the same license with inactive status + 5. Validate that the privilege is automatically deactivated + 6. Clean up all test data + + :raises SmokeTestFailureException: If any step of the workflow fails + """
282-285: Convert function docstring to reST format.The function docstring should follow reST format for consistency.
Apply this diff:
- """ - Run the complete suite of license deactivation privilege smoke tests. - """ + """ + Run the complete suite of license deactivation privilege smoke tests. + """
62-87: Reduce function complexity by refactoring parameters and docstring format.The function has too many parameters (7) and the docstring needs reST format conversion. Consider using a dataclass or configuration object to group related parameters.
Option 1: Create a configuration dataclass
from dataclasses import dataclass @dataclass class PrivilegeValidationConfig: provider_id: str compact: str license_jurisdiction: str license_type: str max_wait_time: int = 120 check_interval: int = 15 def validate_privilege_deactivation(staff_headers: dict, config: PrivilegeValidationConfig): """ Validate that privilege is deactivated due to license deactivation. This function polls the API to check if a privilege has been automatically deactivated after its associated license was deactivated. It retries multiple times within the specified time window. :param staff_headers: Authentication headers for staff user :param config: Configuration object with validation parameters :return: The matching privilege record if validation succeeds :raises SmokeTestFailureException: If privilege is not properly deactivated within the time limit """Option 2: Convert docstring to reST format (minimal change)
- :returns: The matching privilege record if validation succeeds + :return: The matching privilege record if validation succeeds
🧹 Nitpick comments (4)
backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py (4)
141-281: Consider breaking down the large test function into smaller, focused functions.The test function is quite large (140 lines) and handles multiple responsibilities. Consider extracting helper methods for better maintainability and readability.
Example refactoring:
def _setup_test_staff_user(self) -> tuple[str, str, dict]: """Set up test staff user and return credentials.""" # Lines 165-174 def _upload_initial_active_license(self, staff_headers: dict) -> str: """Upload active license and return provider_id.""" # Lines 177-200 def _create_and_verify_test_privilege(self, staff_headers: dict, provider_id: str): """Create test privilege and verify it's active.""" # Lines 202-234 def _trigger_license_deactivation(self, staff_headers: dict): """Upload inactive license to trigger deactivation.""" # Lines 236-251 def _validate_privilege_deactivation_result(self, staff_headers: dict, provider_id: str): """Validate that privilege was properly deactivated.""" # Lines 253-267
199-199: Document the rationale for extended timeout value.The 11-minute timeout (660 seconds) seems unusually long. Consider adding a comment explaining why this extended timeout is necessary.
Apply this diff:
- max_wait_time=660, # 11 minutes (to account for message batch windows) + max_wait_time=660, # 11 minutes - extended timeout to account for async message processing batch windows and event propagation delays
262-262: Use consistent timeout configuration.Consider extracting the timeout value to a constant since it's used in multiple places (lines 199 and 262).
Add this constant at the top with other test configuration:
+# Timeout configuration +ASYNC_PROCESSING_TIMEOUT = 660 # 11 minutes for async message processing and event propagationThen update the usage:
- max_wait_time=660, # 11 minutes (to account for message batch windows) + max_wait_time=ASYNC_PROCESSING_TIMEOUT,- max_wait_time=660, # 11 minutes (to account for message batch windows) + max_wait_time=ASYNC_PROCESSING_TIMEOUT,
292-294: Improve error handling and logging.The current error handling re-raises all exceptions without additional context. Consider logging more details for debugging purposes.
Apply this diff:
- except Exception as e: - logger.error(f'License deactivation privilege smoke tests failed: {str(e)}') - raise + except Exception as e: + logger.error(f'License deactivation privilege smoke tests failed: {str(e)}', exc_info=True) + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py
[refactor] 62-62: Too many arguments (7/5)
(R0913)
[refactor] 62-62: Too many positional arguments (7/5)
(R0917)
[refactor] 62-62: Too many local variables (17/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py(2 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py
[error] 387-387: Too many positional arguments for function call
(E1121)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (3)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py (2)
2-2: LGTM: Import addition is necessary and well-placed.The
dateimport is needed for the new test method and correctly extends the existing datetime import.
340-391: Excellent test coverage for expired license edge case.This test properly verifies that expired licenses don't trigger deactivation events, which is the correct behavior since expired licenses are already effectively inactive. The test:
- Uses clear, descriptive naming
- Follows established test patterns and data setup
- Properly mocks event dependencies
- Correctly verifies the absence of event emission
- Uses appropriate date comparisons (expired date vs. current mock date)
The static analysis hint about "too many positional arguments" on line 387 is a false positive - the function call follows the same pattern used consistently throughout all other tests in this file.
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (1)
5-5: LGTM: Schema import follows existing pattern.The import of
LicenseDeactivationDetailSchemais consistent with other schema imports in the file.
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (1)
49-80: Consider refactoring to reduce parameter count and add input validation.The implementation is functionally correct and follows the established patterns, but has some areas for improvement:
- Parameter count: The method has 6 parameters which exceeds typical guidelines
- Input validation: No validation of input parameters before use
Option 1: Use keyword-only arguments to improve readability:
def generate_license_deactivation_event( - self, source: str, compact: str, jurisdiction: str, provider_id: UUID, license_type: str + self, source: str, *, compact: str, jurisdiction: str, provider_id: UUID, license_type: str ) -> dict:Option 2: Add input validation:
def generate_license_deactivation_event( self, source: str, compact: str, jurisdiction: str, provider_id: UUID, license_type: str ) -> dict: """ Generate a license deactivation event entry for use with batch writers. :param source: The source of the event :param compact: The compact abbreviation :param jurisdiction: The jurisdiction where the license was deactivated :param provider_id: The provider's unique identifier :param license_type: The type of license that was deactivated :returns: Event entry dict that can be used with EventBatchWriter """ + if not all([source, compact, jurisdiction, license_type]): + raise ValueError("All string parameters must be non-empty") + if not isinstance(provider_id, UUID): + raise TypeError("provider_id must be a UUID instance") + event_detail = {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py(2 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
[refactor] 49-49: Too many arguments (6/5)
(R0913)
[refactor] 49-49: Too many positional arguments (6/5)
(R0917)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (1)
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (1)
2-2: LGTM! Imports are appropriate for the new functionality.The UUID and LicenseDeactivationDetailSchema imports are correctly added to support the new method's requirements.
Also applies to: 6-6
backend/compact-connect/lambdas/python/data-events/handlers/license_deactivation_events.py
Outdated
Show resolved
Hide resolved
...ct-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
Show resolved
Hide resolved
landonshumway-ia
force-pushed
the
feat/propagate-deactivation
branch
from
631ca24 to
1b3417c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py (3)
38-47: Convert docstring to reST format.Per the project's documentation standards, please use reST format for the docstring.
def get_provider_details_from_api(staff_headers: dict, compact: str, provider_id: str): - """ - Get provider details from the staff API endpoint. + """Get provider details from the staff API endpoint. :param staff_headers: Authentication headers for staff user :param compact: The compact abbreviation :param provider_id: The provider's ID :returns: Provider details from the API including privileges list :raises SmokeTestFailureException: If the API request fails """
62-87: Convert docstring to reST format and acknowledge appropriate design.The retry logic and parameter count are appropriate for smoke tests that prioritize narrative readability. Please update the docstring to reST format.
def validate_privilege_deactivation( staff_headers: dict, provider_id: str, compact: str, license_jurisdiction: str, license_type: str, max_wait_time: int = 120, check_interval: int = 15, ): - """ - Validate that privilege is deactivated due to license deactivation. + """Validate that privilege is deactivated due to license deactivation. This function polls the API to check if a privilege has been automatically deactivated after its associated license was deactivated. It retries multiple times within the specified time window.
141-157: Convert docstring to reST format - implementation looks excellent.The workflow implementation is well-structured with clear steps and proper cleanup. Please update the docstring to reST format.
def test_license_deactivation_privilege_workflow(): - """ - The test validates that when a home state license is deactivated, + """Test that privileges are automatically deactivated when home state license is deactivated. + + The test validates that when a home state license is deactivated, any privileges associated with that license are automatically deactivated as well.backend/compact-connect/tests/smoke/smoke_common.py (2)
81-87: Convert docstring to reST format.Good improvements with type hints. Please use reST format for the docstring.
def delete_test_staff_user(email: str, user_sub: str, compact: str): - """Deletes a test staff user from Cognito. + """Delete a test staff user from Cognito. :param email: The email address of the staff user to delete :param user_sub: The user's sub ID :param compact: The compact identifier """
102-110: Improve docstring grammar.Good documentation improvements. Minor grammar fix needed.
def create_test_staff_user(*, email: str, compact: str, jurisdiction: str, permissions: dict): - """Creates a test staff user in Cognito, stores their data in DynamoDB, and returns their user sub id. + """Create a test staff user in Cognito, store their data in DynamoDB, and return their user sub ID. :param email: The email address of the staff user to create :param compact: The compact identifier :param jurisdiction: The jurisdiction identifier :param permissions: The permissions dictionary for the user :return: The staff user's sub ID """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backend/compact-connect/common_constructs/queue_event_listener.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_utils.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(4 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py(4 hunks)backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py(2 hunks)backend/compact-connect/lambdas/python/data-events/handlers/license_deactivation_events.py(1 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_license_deactivation_events.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py(2 hunks)backend/compact-connect/lambdas/python/purchases/handlers/privileges.py(2 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py(1 hunks)backend/compact-connect/stacks/event_listener_stack/__init__.py(4 hunks)backend/compact-connect/tests/app/test_event_listener.py(1 hunks)backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py(1 hunks)backend/compact-connect/tests/smoke/smoke_common.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/common_constructs/queue_event_listener.py
🚧 Files skipped from review as they are similar to previous changes (10)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_utils.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py
- backend/compact-connect/lambdas/python/data-events/handlers/license_deactivation_events.py
- backend/compact-connect/lambdas/python/purchases/handlers/privileges.py
- backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
- backend/compact-connect/tests/app/test_event_listener.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.772Z
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#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#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: jsandoval81
PR: csg-org/CompactConnect#873
File: webroot/src/components/LicenseCard/LicenseCard.ts:414-443
Timestamp: 2025-06-24T00:02:39.944Z
Learning: In LicenseCard component's clickUnencumberItem method (webroot/src/components/LicenseCard/LicenseCard.ts), complex event handling for checkbox interactions is intentionally designed to ensure consistent behavior across checkbox input, wrapper label, and outer selection parent elements for custom UI requirements. This complexity should be preserved rather than simplified.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#824
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py:116-201
Timestamp: 2025-05-30T13:48:25.375Z
Learning: In encumbrance handling code, prefer to keep privilege and license encumbrance lifting implementations decoupled rather than extracting shared logic, as requirements between these implementations are likely to change in the future.
backend/compact-connect/lambdas/python/data-events/tests/function/test_license_deactivation_events.py (1)
Learnt from: jusdino
PR: csg-org/CompactConnect#864
File: backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py:866-866
Timestamp: 2025-06-30T17:30:31.212Z
Learning: In the CompactConnect project, the `@sqs_handler` decorator transforms Lambda functions to accept `(event, context)` parameters and automatically handles iteration over `event['Records']`, parsing JSON from each record's body, and managing SQS batch item failures. Functions decorated with `@sqs_handler` have their underlying implementation accept a single `message: dict` parameter, but the decorator makes them callable with standard Lambda `(event, context)` signature. Tests should call these decorated functions with `(event, context)` parameters.
backend/compact-connect/tests/smoke/smoke_common.py (4)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
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#870
File: backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py:128-132
Timestamp: 2025-06-23T21:31:15.787Z
Learning: For smoke tests in the CompactConnect project, broad exception handling (like `except Exception as e:`) is acceptable since these are not runtime code and prioritize simplicity over specific exception handling.
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.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/stacks/api_stack/v1_api/api_model.py:397-401
Timestamp: 2025-04-29T01:59:51.222Z
Learning: In the CompactConnect project, validation constraints like enum values should be defined only in runtime code (Lambda) rather than duplicating them in CDK API schema definitions. This applies to fields like clinicalPrivilegeActionCategory and other similar enumerations.
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (2)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#798
File: backend/compact-connect/lambdas/python/common/cc_common/license_util.py:36-46
Timestamp: 2025-05-14T16:26:16.829Z
Learning: In the `get_valid_license_type_abbreviations` method in `license_util.py`, KeyErrors should not be caught as they indicate an invalid compact value, which is intended to fail explicitly rather than silently returning an empty set.
Learnt from: jusdino
PR: csg-org/CompactConnect#864
File: backend/compact-connect/lambdas/python/common/cc_common/license_util.py:18-37
Timestamp: 2025-06-18T05:57:18.225Z
Learning: In the `get_license_type_by_abbreviation` method in `license_util.py`, KeyError exceptions from invalid compact codes are intentionally caught and re-raised as CCInvalidRequestException with `from e` to provide a consistent interface between invalid compact and invalid abbreviation scenarios, while preserving the original exception information in the chain.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py (3)
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: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))`.
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: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))`.
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: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))`.
backend/compact-connect/stacks/event_listener_stack/__init__.py (1)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py:53-60
Timestamp: 2025-04-29T02:10:38.400Z
Learning: Lambda functions in the provider-data-v1 module that are exposed via API Gateway endpoints (like encumbrance_handler) should only be called through the API, not directly. Therefore, they don't need to handle various body formats as the API Gateway will consistently provide event['body'] as a JSON string.
backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py (9)
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#877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
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#870
File: backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py:128-132
Timestamp: 2025-06-23T21:31:15.787Z
Learning: For smoke tests in the CompactConnect project, broad exception handling (like `except Exception as e:`) is acceptable since these are not runtime code and prioritize simplicity over specific exception handling.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#824
File: backend/compact-connect/tests/app/test_event_listener.py:18-27
Timestamp: 2025-06-04T20:05:18.245Z
Learning: In the CompactConnect test suite, the established pattern is to let file operations in test setup methods raise natural exceptions (FileNotFoundError, JSONDecodeError) rather than adding explicit error handling. This approach maintains consistency across tests and provides sufficient error information for debugging.
Learnt from: jusdino
PR: csg-org/CompactConnect#864
File: backend/compact-connect/lambdas/python/common/cc_common/license_util.py:18-37
Timestamp: 2025-06-18T05:57:18.225Z
Learning: In the `get_license_type_by_abbreviation` method in `license_util.py`, KeyError exceptions from invalid compact codes are intentionally caught and re-raised as CCInvalidRequestException with `from e` to provide a consistent interface between invalid compact and invalid abbreviation scenarios, while preserving the original exception information in the chain.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#798
File: backend/compact-connect/lambdas/python/common/cc_common/license_util.py:36-46
Timestamp: 2025-05-14T16:26:16.829Z
Learning: In the `get_valid_license_type_abbreviations` method in `license_util.py`, KeyErrors should not be caught as they indicate an invalid compact value, which is intended to fail explicitly rather than silently returning an empty set.
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#769
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py:39-41
Timestamp: 2025-04-29T02:09:36.291Z
Learning: In the CompactConnect project, authentication/authorization failures like misconfigured authorizers should be allowed to propagate as 500 errors rather than being caught and converted to 4xx errors, as this is intentional to trigger alerts for critical system issues.
🧬 Code Graph Analysis (4)
backend/compact-connect/tests/smoke/smoke_common.py (1)
backend/compact-connect/tests/smoke/config.py (2)
api_base_url(16-17)provider_user_dynamodb_table(20-21)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (4)
providerId(24-25)providerId(104-105)licenseType(40-41)licenseType(116-117)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (4)
providerId(28-29)providerId(160-161)licenseType(40-41)licenseType(172-173)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
providerId(25-26)providerId(130-131)
backend/compact-connect/stacks/event_listener_stack/__init__.py (3)
backend/compact-connect/stacks/persistent_stack/__init__.py (1)
PersistentStack(39-574)backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(21-155)backend/compact-connect/common_constructs/queue_event_listener.py (1)
QueueEventListener(16-110)
backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py (2)
backend/compact-connect/tests/smoke/smoke_common.py (9)
SmokeTestFailureException(13-17)cleanup_test_provider_records(432-453)create_test_privilege_record(378-429)create_test_staff_user(102-129)delete_test_staff_user(81-99)get_staff_user_auth_headers(174-178)load_smoke_test_env(219-222)upload_license_record(249-301)wait_for_provider_creation(334-375)backend/compact-connect/tests/smoke/config.py (1)
api_base_url(16-17)
🪛 Pylint (3.3.7)
backend/compact-connect/lambdas/python/data-events/tests/function/test_license_deactivation_events.py
[error] 74-74: Too many positional arguments for function call
(E1121)
[error] 118-118: Too many positional arguments for function call
(E1121)
[error] 159-159: Too many positional arguments for function call
(E1121)
[error] 203-203: Too many positional arguments for function call
(E1121)
[error] 254-254: Too many positional arguments for function call
(E1121)
[error] 298-298: Too many positional arguments for function call
(E1121)
[error] 330-330: Too many positional arguments for function call
(E1121)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py
[refactor] 55-55: Too few public methods (0/2)
(R0903)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py
[refactor] 87-87: Too few public methods (0/2)
(R0903)
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
[refactor] 51-51: Too many arguments (6/5)
(R0913)
[refactor] 51-51: Too many positional arguments (6/5)
(R0917)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py
[error] 387-387: Too many positional arguments for function call
(E1121)
backend/compact-connect/stacks/event_listener_stack/__init__.py
[error] 175-182: Missing mandatory keyword argument 'encryption_key' in constructor call
(E1125)
[error] 175-182: Missing mandatory keyword argument 'alarm_topic' in constructor call
(E1125)
backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py
[refactor] 62-62: Too many arguments (7/5)
(R0913)
[refactor] 62-62: Too many positional arguments (7/5)
(R0917)
[refactor] 62-62: Too many local variables (17/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (17)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
10-10: LGTM! Consistent implementation following established patterns.The import and new
LicenseDeactivatedStatusFieldclass follow the same pattern as other status field classes in the file, providing proper validation against the enum values.Also applies to: 87-90
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (1)
55-57: LGTM! Proper event schema implementation.The
LicenseDeactivationDetailSchemacorrectly inherits fromDataEventDetailBaseSchemaand defines the appropriate fields with proper types and constraints, consistent with other event schemas in the file.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py (2)
2-2: LGTM! Necessary import for date handling in tests.
340-391: LGTM! Comprehensive test for expired license deactivation logic.The test properly verifies that license deactivation events are not sent when an expired license is deactivated by the jurisdiction. The test setup, execution, and assertions are all correct and well-structured.
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (2)
8-8: LGTM! Necessary import for license deactivation event validation.
51-81: LGTM! Well-structured event generation method.The method properly validates the event detail using the schema and returns a correctly formatted event entry. The explicit parameter approach avoids potential KeyError issues and ensures type safety.
backend/compact-connect/stacks/event_listener_stack/__init__.py (3)
41-41: LGTM! Proper integration of the new license deactivation listener.
54-54: LGTM! Consistent timeout increase for event listeners.Increasing the timeout to 2 minutes for all event listeners provides consistency and adequate processing time.
Also applies to: 103-103
139-174: LGTM! Proper Lambda function setup and permissions.The license deactivation listener Lambda is correctly configured with proper description, environment variables, permissions, and Nag suppressions following the established pattern.
backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py (1)
282-295: LGTM!The test suite runner is well-implemented with proper error handling and logging.
backend/compact-connect/lambdas/python/data-events/tests/function/test_license_deactivation_events.py (2)
18-41: Well-structured test suite with comprehensive coverage.The test class setup and helper methods are well-implemented. The static analysis warnings about "too many positional arguments" throughout this file are false positives - the
@sqs_handlerdecorator transforms the function to accept(event, context)parameters as documented in the project learnings.
42-333: Excellent test coverage across all scenarios.The test suite comprehensively covers:
- Happy path (active privilege deactivation)
- Edge cases (no matches, multiple matches)
- Boundary conditions (mixed types/jurisdictions)
- Side effects (update record creation)
- Error handling (missing fields)
This provides robust validation of the license deactivation event handler.
backend/compact-connect/tests/smoke/smoke_common.py (5)
249-302: Excellent implementation with comprehensive defaults.The function provides good flexibility with sensible defaults and override capability. The error handling is appropriate.
304-332: LGTM!The implementation correctly returns the first matching provider, with clear documentation about this behavior.
334-376: Well-designed polling implementation.The function provides good defaults (5-minute timeout, 30-second intervals) with clear progress logging and informative error messages.
378-430: Correctly implements privilege record creation.The function properly constructs the DynamoDB record with the expected key format and all required fields for testing license deactivation scenarios.
432-454: Appropriate cleanup implementation.The function correctly queries and deletes all provider records, with acceptable broad exception handling for test cleanup scenarios.
backend/compact-connect/stacks/event_listener_stack/__init__.py
Outdated
Show resolved
Hide resolved
When a home state license is marked as inactive by a jurisdiction (via
jurisdictionUploadedLicenseStatusfield) but is not expired according to thedateOfExpirationfield, all privileges associated with that specific license type and jurisdiction should be automatically deactivated. This feature leverages our existing event-driven architecture to listen for license deactivation events and perform the privilege deactivation.Closes #423
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores