Feat/put home jurisdiction updates #796
Conversation
WalkthroughThis update introduces comprehensive support for managing provider home jurisdiction changes in the backend. It adds endpoint, schema, data model, and transactional logic to allow practitioners to manually change their home state, update privileges and eligibility accordingly, and track home jurisdiction history. Extensive refactoring, new tests, and schema updates are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant ProviderUsersHandler
participant DataClient
participant DynamoDB
User->>API: PUT /v1/provider-users/me/home-jurisdiction
API->>ProviderUsersHandler: Lambda invoke
ProviderUsersHandler->>DataClient: update_provider_home_state_jurisdiction(compact, provider_id, selected_jurisdiction)
DataClient->>DynamoDB: Query provider user records
alt License exists in new jurisdiction
DataClient->>DynamoDB: Update provider record with new license
DataClient->>DynamoDB: Update privileges (move or deactivate as needed)
else No license in new jurisdiction
DataClient->>DynamoDB: Deactivate all privileges
DataClient->>DynamoDB: Update provider record (ineligible)
end
DataClient->>DynamoDB: Create provider update record
ProviderUsersHandler-->>API: 200 OK, {"message": "..."}
API-->>User: Response
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ 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 (
|
landonshumway-ia
force-pushed
the
feat/put-home-jurisdiction-updates
branch
3 times, most recently
from
2021f78 to
713e80d
Compare
This refactors the provider-users/me endpoints to all use the same lambda, to simplify the architecture.
This will allow us to track if the provider has moved to a jurisdiction that lacks a valid license.
- deactivating privilege if in same jurisdiction - copying over license data into provider record
landonshumway-ia
force-pushed
the
feat/put-home-jurisdiction-updates
branch
from
713e80d to
02d8fe0
Compare
also reverted change for catching cognito exception
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
backend/compact-connect/tests/smoke/home_jurisdiction_change_smoke_tests.py (1)
1-172: Fix formatting issues and improve error handling.This smoke test provides excellent coverage of the home jurisdiction change flow, but there are formatting issues that need to be addressed.
The formatting issues (whitespace in blank lines, line length violations) identified in past reviews still need to be addressed. Additionally, consider adding more robust error handling around the HTTP requests with retry logic for better test reliability.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py (1)
687-698:⚠️ Potential issueProvider object is overwritten – capture the first provider or avoid shadowing
The variable
test_provider_recordis overwritten by the second call to_when_provider_has_one_license_and_privilege, causing later assertions to potentially reference the wrong provider data. Consider using distinct variable names or reusing the provider ID from the first call when creating additional licenses and privileges.backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py (2)
29-36: 🛠️ Refactor suggestionVerify path matching logic – event["resource"] may not be stable across API types
event["resource"]is only guaranteed for REST-API integrations. For HTTP APIs the field might berequestContext["resourcePath"]or absent altogether. Consider normalizing the path lookup:- resource_path = event.get('resource') + resource_path = event.get('resource') or event['requestContext'].get('resourcePath')
88-94: 🛠️ Refactor suggestionHarden request-body validation
event["body"]can beNone, invalid JSON or missing the required"jurisdiction"key – all will currently raise generic exceptions that become 500s. Guard explicitly and return a 400 viaCCInvalidRequestException.- event_body = json.loads(event['body']) - selected_jurisdiction = event_body['jurisdiction'].lower() + if event.get('body') is None: + raise CCInvalidRequestException('Request body is required') + + try: + event_body = json.loads(event['body']) + except json.JSONDecodeError as exc: + raise CCInvalidRequestException('Malformed JSON in request body') from exc + + selected_jurisdiction = event_body.get('jurisdiction') + if not selected_jurisdiction: + raise CCInvalidRequestException('Field "jurisdiction" is required') + selected_jurisdiction = selected_jurisdiction.lower()backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
268-280:⚠️ Potential issueHandle missing provider record defensively
The method will raise an
IndexErrorif no provider records are found when accessingprovider_user_records[0]. Add a check for empty list before accessing the first element.Apply this fix:
def get_provider_record(self) -> ProviderData: """ Get the provider record from a list of provider records. """ provider_user_records = [ ProviderData.from_database_record(record) for record in ProviderRecordUtility.get_records_of_type(self.provider_records, ProviderRecordType.PROVIDER) ] + if not provider_user_records: + raise CCInternalException('No provider record found for user') if len(provider_user_records) > 1: logger.error('Multiple provider records found', provider_id=provider_user_records[0].providerId) raise CCInternalException('Multiple top-level provider records found for user.') return provider_user_records[0]
🧹 Nitpick comments (3)
backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py (1)
913-1002: Fix formatting issues identified by static analysis.Multiple blank lines contain whitespace which should be removed for code cleanliness. The
resultvariable assignment before return is also unnecessary.Apply this diff to fix the formatting issues:
- # Blank line contains whitespace (multiple locations) + # (remove whitespace from blank lines) - def mock_query(**kwargs): - # Force a small page size to ensure pagination - kwargs['Limit'] = 10 - result = original_query(**kwargs) - return result + def mock_query(**kwargs): + # Force a small page size to ensure pagination + kwargs['Limit'] = 10 + return original_query(**kwargs)🧰 Tools
🪛 Ruff (0.11.9)
916-916: Blank line contains whitespace
Remove whitespace from blank line
(W293)
924-924: Blank line contains whitespace
Remove whitespace from blank line
(W293)
933-933: Blank line contains whitespace
Remove whitespace from blank line
(W293)
954-954: Blank line contains whitespace
Remove whitespace from blank line
(W293)
963-963: Blank line contains whitespace
Remove whitespace from blank line
(W293)
966-966: Blank line contains whitespace
Remove whitespace from blank line
(W293)
971-971: Unnecessary assignment to
resultbeforereturnstatementRemove unnecessary assignment
(RET504)
972-972: Blank line contains whitespace
Remove whitespace from blank line
(W293)
974-974: Blank line contains whitespace
Remove whitespace from blank line
(W293)
978-978: Blank line contains whitespace
Remove whitespace from blank line
(W293)
982-982: Blank line contains whitespace
Remove whitespace from blank line
(W293)
986-986: Blank line contains whitespace
Remove whitespace from blank line
(W293)
992-992: Blank line contains whitespace
Remove whitespace from blank line
(W293)
998-998: Blank line contains whitespace
Remove whitespace from blank line
(W293)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
493-505: Consider extracting transaction batching logic to reduce duplication.The same batching pattern for processing DynamoDB transactions in batches of 100 appears in 4 different methods. This violates the DRY principle.
Consider creating a helper method:
def _execute_transactions_in_batches(self, transactions: list[dict], batch_size: int = 100) -> None: """Execute DynamoDB transactions in batches to respect the 100 item limit.""" for i in range(0, len(transactions), batch_size): batch = transactions[i : i + batch_size] if batch: self.config.dynamodb_client.transact_write_items(TransactItems=batch)Then replace the duplicated code in all 4 locations with:
self._execute_transactions_in_batches(transactions)Also applies to: 595-604, 1744-1750, 1891-1897
1314-1451: Comprehensive implementation of home jurisdiction update logic.The method correctly implements all 6 business rules for updating provider home jurisdiction and handling privileges. The code is well-structured with appropriate helper methods and extensive logging for debugging.
The business logic is complex with many edge cases. Would you like me to help create additional test cases to ensure all scenarios are covered?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(19 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py(5 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py(3 hunks)backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py(12 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py(3 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py(19 hunks)backend/compact-connect/lambdas/python/purchases/handlers/privileges.py(4 hunks)backend/compact-connect/tests/app/test_api/test_provider_users_api.py(6 hunks)backend/compact-connect/tests/smoke/home_jurisdiction_change_smoke_tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/compact-connect/tests/app/test_api/test_provider_users_api.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py
- backend/compact-connect/lambdas/python/purchases/handlers/privileges.py
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py
916-916: Blank line contains whitespace
Remove whitespace from blank line
(W293)
924-924: Blank line contains whitespace
Remove whitespace from blank line
(W293)
933-933: Blank line contains whitespace
Remove whitespace from blank line
(W293)
954-954: Blank line contains whitespace
Remove whitespace from blank line
(W293)
963-963: Blank line contains whitespace
Remove whitespace from blank line
(W293)
966-966: Blank line contains whitespace
Remove whitespace from blank line
(W293)
971-971: Unnecessary assignment to result before return statement
Remove unnecessary assignment
(RET504)
972-972: Blank line contains whitespace
Remove whitespace from blank line
(W293)
974-974: Blank line contains whitespace
Remove whitespace from blank line
(W293)
978-978: Blank line contains whitespace
Remove whitespace from blank line
(W293)
982-982: Blank line contains whitespace
Remove whitespace from blank line
(W293)
986-986: Blank line contains whitespace
Remove whitespace from blank line
(W293)
992-992: Blank line contains whitespace
Remove whitespace from blank line
(W293)
998-998: Blank line contains whitespace
Remove whitespace from blank line
(W293)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (19)
backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py (9)
7-8: LGTM! Good refactoring to use constants.The addition of
DEFAULT_PROVIDER_IDimport supports better test maintainability by replacing hardcoded provider ID strings throughout the tests.
150-154: LGTM! Excellent refactoring to use test data generators.The changes to use
DEFAULT_PROVIDER_IDconstant andgenerate_default_provider()method improve test maintainability and consistency.
201-201: LGTM! Good migration to typed data classes.The refactoring from direct dictionary manipulation to
PrivilegeData.from_database_record()improves type safety and aligns with the broader data model improvements.Also applies to: 215-236
236-236: LGTM! Proper use of data class serialization.Using
serialize_to_database_record()ensures proper data serialization with the typed data class approach.
271-276: LGTM! Consistent use of test data generators.Using
generate_default_providerwithvalue_overridesmaintains consistency with the centralized test data generation approach.
426-450: LGTM! Proper data class usage and accurate comments.The refactoring to use
PrivilegeData.create_new()and the updated comment accurately reflect the test's behavior with 51 jurisdictions creating 102 records.
459-465: LGTM! Cleaner test data generation.Passing all overrides in a single dictionary to
generate_default_provideris cleaner and more maintainable than incremental assignments.
508-543: LGTM! Consistent refactoring pattern.The changes maintain consistency with the PrivilegeData approach and use appropriate helper methods like
put_default_provider_record_in_provider_tablefor cleaner test setup.
914-1001: Excellent test for pagination handling.This test comprehensively validates the fix for the pagination bug where only the last page of results was being returned. The test setup with 30+ records and forced small page size effectively reproduces the pagination scenario and validates the solution.
🧰 Tools
🪛 Ruff (0.11.9)
916-916: Blank line contains whitespace
Remove whitespace from blank line
(W293)
924-924: Blank line contains whitespace
Remove whitespace from blank line
(W293)
933-933: Blank line contains whitespace
Remove whitespace from blank line
(W293)
954-954: Blank line contains whitespace
Remove whitespace from blank line
(W293)
963-963: Blank line contains whitespace
Remove whitespace from blank line
(W293)
966-966: Blank line contains whitespace
Remove whitespace from blank line
(W293)
971-971: Unnecessary assignment to
resultbeforereturnstatementRemove unnecessary assignment
(RET504)
972-972: Blank line contains whitespace
Remove whitespace from blank line
(W293)
974-974: Blank line contains whitespace
Remove whitespace from blank line
(W293)
978-978: Blank line contains whitespace
Remove whitespace from blank line
(W293)
982-982: Blank line contains whitespace
Remove whitespace from blank line
(W293)
986-986: Blank line contains whitespace
Remove whitespace from blank line
(W293)
992-992: Blank line contains whitespace
Remove whitespace from blank line
(W293)
998-998: Blank line contains whitespace
Remove whitespace from blank line
(W293)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py (3)
2-2: LGTM! Proper refactoring to unified handler.The changes to use
provider_users_api_handlerwith properhttpMethodandresourcefields are necessary for the new centralized routing approach.Also applies to: 25-27, 33-38
389-403: LGTM! Well-organized test constants.The constants clearly define test data values and make the tests more readable and maintainable.
404-1331: Excellent comprehensive test coverage for home jurisdiction changes.The
TestPutProviderHomeJurisdictionclass provides thorough coverage of all scenarios including license presence, expiration, encumbrance, and jurisdiction membership. The test structure with helper methods is well-organized and maintainable.backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py (2)
96-111: LGTM! Well-structured handler logic.The jurisdiction validation, user attribute checking, logging, and data client integration are properly implemented. The simple success response is appropriate for this operation.
6-6: LGTM! Necessary import for jurisdiction validation.The
OTHER_JURISDICTIONimport is required for proper jurisdiction validation in the new home jurisdiction update handler.backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (3)
57-64: LGTM! Proper empty check implemented.The method correctly handles the case when no provider records are found by checking if the list is empty before accessing the first element.
82-90: Good enhancement to home jurisdiction filtering logic.The updated logic properly handles the edge case where a provider moves to a non-member jurisdiction by falling back to all licenses when no licenses exist in the selected jurisdiction.
125-161: Well-structured refactor to handle both creation and update scenarios.The method now properly preserves existing provider data when updating, while still supporting new record creation. The logic for handling empty privilege jurisdiction sets is also maintained.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
166-199: Pagination correctly implemented.The method properly accumulates all pages of results in
resp['Items']and returns the complete dataset. The pagination bug mentioned in past reviews has been fixed.
441-446: Excellent handling of privilege reactivation on renewal.The logic correctly removes the
homeJurisdictionChangeStatuswhen a previously deactivated privilege is renewed, allowing it to become active again in the new jurisdiction context.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
6-10: Address unused imports flagged by static analysis.Static analysis indicates that
ActiveInactiveStatusandHomeJurisdictionChangeStatusEnumare imported but unused in this file. Consider removing them if they're truly unused, or add them to__all__if they're part of the public API.🧰 Tools
🪛 Ruff (0.11.9)
7-7:
cc_common.data_model.schema.common.ActiveInactiveStatusimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
9-9:
cc_common.data_model.schema.common.HomeJurisdictionChangeStatusEnumimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
📜 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(19 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py
7-7: cc_common.data_model.schema.common.ActiveInactiveStatus imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
9-9: cc_common.data_model.schema.common.HomeJurisdictionChangeStatusEnum imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (6)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (2)
79-81: LGTM! New property supports home jurisdiction change tracking.The new
homeJurisdictionChangeStatusproperty correctly handles the optional string return type and follows the established pattern of other properties in the class.
134-139: LGTM! New property supports privilege update tracking.The
removedValuesproperty is well-documented and follows the established pattern. The documentation clearly indicates when this property is present (during deactivation updates).backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (4)
166-199: LGTM! Pagination issue previously identified has been resolved.The manual pagination correctly accumulates all results in
resp['Items']and returns the complete dataset. This addresses the pagination bug mentioned in previous reviews.
441-444: Good practice: Tracking field removal in privilege renewals.The logic correctly removes the
homeJurisdictionChangeStatusfield when a privilege is renewed after being deactivated due to a home jurisdiction change, and properly documents this removal in the update record.
1314-1342: Comprehensive business rules documentation for home jurisdiction changes.The docstring clearly outlines the complex business rules for privilege handling during home jurisdiction changes. The logic covers all scenarios including expired licenses, encumbered licenses, and eligibility status.
1428-1432:✅ Verification successful
Verify date comparison logic for license expiration.
The comparison uses
self.config.expiration_resolution_dateto determine if a license is expired. Ensure this configuration value is properly set and represents the current date or evaluation date for expiration checks.
🏁 Script executed:
#!/bin/bash # Description: Find where expiration_resolution_date is defined and used # Expected: Should find the configuration definition and usage patterns echo "Searching for expiration_resolution_date definition..." rg "expiration_resolution_date" --type py -A 3 -B 3 echo "Searching for related date comparison patterns..." rg "dateOfExpiration.*<" --type py -A 2 -B 2Length of output: 6301
🏁 Script executed:
#!/bin/bash # Print the expiration_resolution_date property implementation echo "=== expiration_resolution_date implementation ===" sed -n -n '/def expiration_resolution_date/,/^[[:space:]]*$/p' backend/compact-connect/lambdas/python/common/cc_common/config.pyLength of output: 735
🏁 Script executed:
#!/bin/bash # Locate the definition of current_standard_datetime and expiration_resolution_timezone in config.py echo "=== Searching for current_standard_datetime definition ===" rg "current_standard_datetime" backend/compact-connect/lambdas/python/common/cc_common/config.py -A2 -B2 echo "=== Searching for expiration_resolution_timezone definition ===" rg "expiration_resolution_timezone" backend/compact-connect/lambdas/python/common/cc_common/config.py -A2 -B2Length of output: 1207
🏁 Script executed:
#!/bin/bash # Print the full implementation of current_standard_datetime echo "=== current_standard_datetime implementation ===" rg "def current_standard_datetime" -A 10 backend/compact-connect/lambdas/python/common/cc_common/config.pyLength of output: 602
Expiration check logic is correct and needs no change
I’ve verified thatself.config.expiration_resolution_dateis a dynamically computed date (today in the configured UTC-4 timezone) via:
current_standard_datetime:datetime.now(tz=UTC).replace(microsecond=0)expiration_resolution_timezone:timezone(offset=timedelta(hours=-4))expiration_resolution_date: converts the above to the target zone and takes.date()Since
matching_license_in_current_jurisdiction.dateOfExpirationis also adate, the comparisonif matching_license_in_current_jurisdiction.dateOfExpiration < self.config.expiration_resolution_date: …correctly flags licenses that expired before today. No further action needed.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
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
♻️ Duplicate comments (5)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py (1)
686-697: Provider object is overwritten – capture the first provider or avoid shadowingThe helper
_when_provider_has_one_license_and_privilegecallsput_default_provider_record_in_provider_tableeach time, so:# 1st call – creates provider A (test_provider_record, …) = self._when_provider_has_one_license_and_privilege(...) # 2nd call – creates provider B (overwrites the same variable) (test_provider_record, …) = self._when_provider_has_one_license_and_privilege(...)Later you build the API event from provider B but assert against licenses/privileges belonging to provider A. Unless your generator reuses
providerIds (most don't), these assertions only pass by coincidence.Recommendations:
- Create the provider once and reuse its
providerIdwhen adding licenses or privileges.- Or assign the first provider to a separate variable (e.g.
first_provider_record) so it isn't shadowed.For example, instead of calling the helper twice:
- (test_provider_record, lic1, priv1) = self._when_provider_has_one_license_and_privilege(...) - (test_provider_record, lic2, priv2) = self._when_provider_has_one_license_and_privilege(...) + (first_provider_record, lic1, priv1) = self._when_provider_has_one_license_and_privilege(...) + lic2 = self.test_data_generator.put_default_license_record_in_provider_table( + value_overrides={'providerId': first_provider_record.provider_id, …} + ) + priv2 = self.test_data_generator.put_default_privilege_record_in_provider_table( + value_overrides={'providerId': first_provider_record.provider_id, …} + )This ensures all assertions reference the same real‐world provider.
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
270-281:⚠️ Potential issueHandle missing provider record defensively
The new
get_provider_record()method inProviderUserRecordschecks for multiple records but doesn't handle the case where no provider records are found, which would cause anIndexErroron line 281.def get_provider_record(self) -> ProviderData: """ Get the provider record from a list of provider records. """ provider_user_records = [ ProviderData.from_database_record(record) for record in ProviderRecordUtility.get_records_of_type(self.provider_records, ProviderRecordType.PROVIDER) ] if len(provider_user_records) > 1: logger.error('Multiple provider records found', provider_id=provider_user_records[0].providerId) raise CCInternalException('Multiple top-level provider records found for user.') + if not provider_user_records: + raise CCInternalException('No provider record found for supplied records') return provider_user_records[0]backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_registration.py (1)
390-398:⚠️ Potential issuePotential
AttributeErrorif rollback removed the provider record
get_item(...).get('Item')may returnNonewhen the item is missing. Callingprovider_record.get(...)onNonewould raise anAttributeError, masking the real failure.provider_record = self.config.provider_table.get_item( Key={ 'pk': f'{TEST_COMPACT_ABBR}#PROVIDER#{provider_data["providerId"]}', 'sk': f'{TEST_COMPACT_ABBR}#PROVIDER', } ).get('Item') +self.assertIsNotNone(provider_record, "Provider record unexpectedly missing after rollback") # Verify no registration information was added self.assertIsNone(provider_record.get('compactConnectRegisteredEmailAddress'))backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
1720-1725: Implement rollback for multi-batch transactions.Similar to previous feedback, each 100-item batch is committed independently, so a failure in batch N will leave batches 1…N-1 persisted with no compensating rollback.
Apply the same rollback pattern used in the privilege creation method:
- Track successfully written batches
- Wrap batch processing in try/except blocks
- On exception, invoke rollback logic to reverse processed batches
- Re-raise the original error after rollback
This ensures atomicity across multi-batch operations in both
_deactivate_privileges_for_jurisdiction_changeand_update_privileges_for_jurisdiction_changemethods.Also applies to: 1867-1872
1835-1851: Fix UpdateExpression construction syntax issue.The current code has the same comma placement issue identified in previous reviews. The encumbered status update appends
,encumberedStatus = :encumberedStatuswith a leading comma, then joins parts with spaces, creating malformed expressions.Apply this fix:
- update_expression_parts = [ - 'SET licenseJurisdiction = :licenseJurisdiction, ' - + 'dateOfExpiration = :dateOfExpiration, ' - + 'dateOfUpdate = :dateOfUpdate' - ] - - if is_new_license_encumbered: - update_expression_parts.append(',encumberedStatus = :encumberedStatus') - - # Build the final update expression - update_expression = ' '.join(update_expression_parts) + set_clauses = [ + 'licenseJurisdiction = :licenseJurisdiction', + 'dateOfExpiration = :dateOfExpiration', + 'dateOfUpdate = :dateOfUpdate' + ] + + if is_new_license_encumbered: + set_clauses.append('encumberedStatus = :encumberedStatus') + + # Build the final update expression + update_expression = 'SET ' + ', '.join(set_clauses)
🧹 Nitpick comments (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (2)
241-255: Optimize filter condition evaluation to avoid duplicate object creationThe
get_privilege_recordsmethod callsPrivilegeData.from_database_record(record)twice for each record - once for the filter condition check and once for the return value. This creates unnecessary overhead.def get_privilege_records( self, filter_condition: Callable[[PrivilegeData], bool] | None = None, ) -> list[PrivilegeData]: """ Get all privilege records from a list of provider records. :param filter_condition: An optional filter to apply to the privilege records """ - return [ - PrivilegeData.from_database_record(record) - for record in ProviderRecordUtility.get_records_of_type(self.provider_records, ProviderRecordType.PRIVILEGE) - if (filter_condition is None or filter_condition(PrivilegeData.from_database_record(record))) - ] + privilege_records = [] + for record in ProviderRecordUtility.get_records_of_type(self.provider_records, ProviderRecordType.PRIVILEGE): + privilege_data = PrivilegeData.from_database_record(record) + if filter_condition is None or filter_condition(privilege_data): + privilege_records.append(privilege_data) + return privilege_records
217-226: TODO comment indicates temporary backwards compatibility codeThe TODO comment mentions this backwards compatibility mapping should be removed as part of issue #763. Consider tracking this technical debt to ensure it gets addressed.
Do you want me to create a reminder or tracking issue for removing this backwards compatibility code?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(19 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py(5 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/__init__.py(0 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/__init__.py(0 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/api.py(0 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/record.py(0 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py(4 hunks)backend/compact-connect/lambdas/python/common/common_test/test_constants.py(0 hunks)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py(5 hunks)backend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.json(2 hunks)backend/compact-connect/lambdas/python/common/tests/resources/dynamo/home-jurisdiction-selection.json(0 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_data_model/test_provider_transformations.py(4 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py(5 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py(17 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_registration.py(10 hunks)
💤 Files with no reviewable changes (6)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/init.py
- backend/compact-connect/lambdas/python/common/tests/resources/dynamo/home-jurisdiction-selection.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/api.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/record.py
- backend/compact-connect/lambdas/python/common/common_test/test_constants.py
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.py
- backend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.json
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_data_model/test_provider_transformations.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py
🧰 Additional context used
🧠 Learnings (1)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_registration.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 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))`.
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))`.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (14)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.py (1)
240-240: LGTM! Method signature simplified appropriately.The removal of the
delete_home_jurisdiction_selectionparameter aligns with the broader refactoring to eliminate thehomeJurisdictionSelectiondata model in favor of thecurrentHomeJurisdictionfield.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py (1)
388-398: LGTM! Well-organized test constants and comprehensive test coverage.The new test class
TestPutProviderHomeJurisdictionprovides excellent coverage of home jurisdiction update scenarios, including edge cases like encumbered licenses, expired licenses, and non-member jurisdictions. The constants are well-defined and the test structure is clear.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_registration.py (2)
617-679: LGTM! Comprehensive test for provider update record creation.The new test
test_registration_creates_provider_update_recordprovides excellent coverage for verifying that provider update records are properly created during registration. The test validates both the structure and content of the update record, ensuring data integrity throughout the registration process.
364-377: LGTM! Enhanced rollback testing with realistic error simulation.The use of
ClientErrorfrombotocore.exceptionsto simulate DynamoDB transaction failures is a good practice that makes the test more realistic and robust.backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (3)
64-94: Excellent addition of database query helpers for testing.These new helper methods provide valuable testing utilities for verifying database state after operations. The implementation correctly uses DynamoDB query patterns and proper error handling.
346-378: Good enhancement to support both registered and unregistered provider scenarios.The addition of the
is_registeredparameter allows tests to create providers in different states, which is essential for comprehensive testing of the registration flow.
480-480: Proper alignment with the new provider record utility.The change to use
ProviderRecordUtility.assemble_provider_records_into_api_response_objectaligns with the broader refactoring to typed data classes and centralized record assembly logic.backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (7)
165-197: Pagination implementation is correct.The new
get_provider_user_recordsmethod properly accumulates all paginated results inresp['Items']and returns the complete dataset, avoiding the pagination bug that was previously identified in similar methods.
102-135: Good transition to typed data classes.The method now returns a typed
LicenseDataobject instead of a raw dictionary, improving type safety and code maintainability.
806-829: Well-implemented typed data retrieval method.The new
get_provider_top_level_recordmethod provides clean access to typed provider data with proper error handling and logging.
831-902: Excellent refactoring with typed parameters and improved transaction structure.The updated registration process uses typed data classes and creates proper audit records. The transaction structure ensures data consistency during registration.
1290-1427: Comprehensive home jurisdiction update logic with thorough business rule implementation.The main orchestration method handles complex business scenarios including license availability, eligibility, and encumbrance status. The logic correctly implements the requirements outlined in the method documentation.
1428-1505: Well-structured privilege eligibility checking logic.This helper method cleanly separates the complex decision logic for whether privileges can be moved to a new jurisdiction. The implementation correctly handles various edge cases including missing licenses and compact eligibility.
1507-1577: Proper provider record update handling for both scenarios.Both methods correctly handle provider record updates - one for jurisdiction changes with valid licenses and one for jurisdictions without known licenses. The transaction structure ensures data consistency.
Also applies to: 1579-1645
removed previous migration script
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/compact-connect/lambdas/python/migration/home_jurisdiction_796/main.py (2)
64-84: Consider making batch size configurable and improving error handling.While the current implementation works, consider these improvements:
- Make the batch size configurable via environment variables for flexibility
- The current approach fails the entire migration if any batch fails. Consider tracking failed records and retrying or reporting them separately to allow partial success.
- batch_size = 50 + batch_size = int(os.environ.get('MIGRATION_BATCH_SIZE', '50'))
111-127: Consider adding existence check for provider record.The update operation doesn't verify that the provider record exists. If a provider record is missing, the entire transaction batch will fail.
Add a condition expression to ensure the provider exists:
'Update': { 'TableName': config.provider_table.table_name, 'Key': { 'pk': {'S': provider_pk}, 'sk': {'S': provider_sk}, }, 'UpdateExpression': 'SET currentHomeJurisdiction = :jurisdiction', 'ExpressionAttributeValues': { ':jurisdiction': {'S': selected_jurisdiction}, }, + 'ConditionExpression': 'attribute_exists(pk) AND attribute_exists(sk)', }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/migration/compact_configuration_783/main.py(0 hunks)backend/compact-connect/lambdas/python/migration/home_jurisdiction_796/main.py(1 hunks)backend/compact-connect/stacks/persistent_stack/__init__.py(2 hunks)
💤 Files with no reviewable changes (1)
- backend/compact-connect/lambdas/python/migration/compact_configuration_783/main.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/compact-connect/stacks/persistent_stack/__init__.py (2)
backend/compact-connect/common_constructs/data_migration.py (1)
DataMigration(15-117)backend/compact-connect/common_constructs/stack.py (1)
common_env_vars(81-89)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (2)
backend/compact-connect/stacks/persistent_stack/__init__.py (1)
369-404: Migration resource update looks good!The migration resource has been consistently updated from handling compact configuration to home jurisdiction data. All related references including the construct ID, migration directory, environment variables, permissions, and log groups have been properly updated.
backend/compact-connect/lambdas/python/migration/home_jurisdiction_796/main.py (1)
24-84:✅ Verification successful
Verify migration idempotency.
The migration processes and deletes
homeJurisdictionSelectionrecords. If the migration is run multiple times (e.g., due to deployment rollback/retry), it will find no records on subsequent runs and complete successfully. However, ensure this aligns with your deployment strategy.Run the following script to check if there are any existing
homeJurisdictionSelectionrecords that would be affected by this migration:
🏁 Script executed:
#!/bin/bash # Description: Search for references to homeJurisdictionSelection in the codebase to understand the impact # Look for any remaining references to homeJurisdictionSelection echo "Searching for homeJurisdictionSelection references..." rg -i "homeJurisdictionSelection" -A 3 -B 3 # Check for any test data or fixtures that might create these records echo -e "\nSearching for test data creating homeJurisdictionSelection records..." fd -e json -e yaml -e py | xargs rg -l "homeJurisdictionSelection" 2>/dev/null || echo "No test data found"Length of output: 82096
Migration Idempotency Confirmed
The
do_migrationfunction safely scans for and removes allhomeJurisdictionSelectionrecords. On any subsequent run it will simply find no items and exit without error. No code changes are needed here—just ensure your deployment/rollback process runs this one-time data migration exactly once.
backend/compact-connect/lambdas/python/migration/home_jurisdiction_796/main.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/migration/home_jurisdiction_796/main.py
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
1720-1725:⚠️ Potential issueCritical: Multi-batch transaction rollback still missing
The batching logic still lacks rollback mechanisms for partial failures across multiple batches. This is the same issue identified in previous reviews.
Each batch is committed independently, so if batch N fails, batches 1…N-1 remain committed with no way to undo them. This violates atomicity across the entire operation.
The same rollback pattern used in
_rollback_privilege_transactionsshould be implemented here.
1867-1872:⚠️ Potential issueCritical: Multi-batch transaction rollback still missing
Same issue as the previous batching code - this also needs rollback mechanisms for partial failures across multiple transaction batches.
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
1427-1506: Consider breaking down complex conditional logicThis method contains multiple nested conditions and business rule checks. While the logic appears correct, the complexity could be reduced by extracting some conditions into smaller helper methods.
Consider extracting methods like:
_is_license_eligible_for_privilege_transfer_should_deactivate_privileges_for_license_typeThis would improve readability and testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/compact-connect/docs/design/practitioner-home-state-update-flow-chart.pdfis excluded by!**/*.pdf
📒 Files selected for processing (2)
backend/compact-connect/docs/design/README.md(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(19 hunks)
🧰 Additional context used
🪛 LanguageTool
backend/compact-connect/docs/design/README.md
[style] ~395-~395: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...rivilege Purchase Flow When a provider wants to purchase privileges to practice in addi...
(REP_WANT_TO_VB)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (12)
backend/compact-connect/docs/design/README.md (2)
10-10: Add Privileges entry to Table of Contents
The new “Privileges” section is now referenced in the TOC and the anchor#privilegesmatches the## Privilegesheading below.
380-386: Approve new Privileges section
The “## Privileges” heading and introduction follow the established style (heading, back-to-top link, blank line, body). The anchor link is correct, and the content clearly outlines the purpose of privileges.backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (10)
12-12: LGTM - Good type safety improvementsThe new imports properly support the type safety refactoring with
ProviderUserRecords,HomeJurisdictionChangeStatusEnum, and typed data classes.Also applies to: 20-21, 34-34
102-102: Excellent type safety improvementThe method now returns a proper
LicenseDatainstance instead of a raw dictionary, providing better type safety and validation through thefrom_database_recordmethod.Also applies to: 135-135
165-197: LGTM - Pagination correctly implementedThe pagination logic properly accumulates all pages in
resp['Items']and returns the full accumulated list, avoiding the pagination bug that was previously fixed in other methods.
299-300: Good refactoring to typed objectsThe method now returns a properly typed
PrivilegeDatainstance and uses the newcreate_newpattern for consistent object creation.Also applies to: 330-345
355-356: Well-implemented privilege renewal logic with type safetyThe refactoring to use typed
ProviderDataandPrivilegeDataobjects is excellent. The logic to removehomeJurisdictionChangeStatuswhen renewing previously deactivated privileges (lines 442-443) properly handles the business requirement.Also applies to: 383-383, 397-399, 403-403, 417-444
806-829: Good replacement for boolean check with typed returnReplacing
provider_is_registered_in_compact_connectwithget_provider_top_level_recordthat returns a typedProviderDataobject is a significant improvement. The error handling and logging are appropriate.
831-902: Well-structured registration refactoringThe refactoring to use typed objects and create proper update records with transactional consistency is excellent. The use of
ProviderRecordUtility.populate_provider_recordprovides good separation of concerns.
1835-1851: Excellent UpdateExpression constructionThis properly addresses the previous review feedback about UpdateExpression construction. Building the clauses as a list and joining with commas is much more robust than the previous string concatenation approach.
1291-1318: Well-documented complex business logicThe comprehensive docstring clearly explains the business rules for home jurisdiction changes. The logic handles multiple edge cases appropriately including expired licenses, encumbered licenses, and compact eligibility.
1290-1872: Impressive comprehensive home jurisdiction functionalityThe entire home jurisdiction change implementation is well-architected with proper separation of concerns:
- Clear business rule documentation
- Separate methods for different scenarios (no license vs. with license)
- Proper privilege handling based on license states
- Good logging for traceability
The type safety improvements throughout make the code much more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
1720-1725: Transaction batching needs rollback mechanismThe batching logic correctly respects DynamoDB's 100-item transaction limit. However, like the issue noted in past reviews, there's no rollback mechanism between batches - if a later batch fails, earlier batches remain committed.
Consider implementing rollback for multi-batch transactions by:
- Tracking successfully processed batches
- Wrapping the loop in a try/except block
- On failure, implementing compensating transactions to undo processed batches
Also applies to: 1867-1872
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py (1)
749-865: Consider consolidating or clarifying the purpose of these similar tests.Both test methods cover very similar scenarios - renewing privileges that were deactivated due to home jurisdiction changes. The main difference is that the first test checks the final privilege record state while the second checks the privilege update records.
Consider:
- Consolidating into a single test that verifies both the final state and the update records
- Shortening the method names - the first method name is quite verbose
- Adding comments to clarify the distinct value each test provides
Example consolidation:
- def test_purchase_privileges_removed_privilege_home_jurisdiction_change_deactivation_status_when_renewing_privilege( - self, mock_purchase_client_constructor - ): - def test_purchase_privileges_removes_home_jurisdiction_change_status_when_privilege_renewed( - self, mock_purchase_client_constructor - ): + def test_purchase_privileges_removes_home_jurisdiction_change_status_on_renewal( + self, mock_purchase_client_constructor + ): + """Test that renewing a privilege removes homeJurisdictionChangeStatus and updates records properly."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(19 hunks)backend/compact-connect/lambdas/python/purchases/handlers/privileges.py(4 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/purchases/handlers/privileges.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (21)
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py (7)
99-104: LGTM! Improved test data setup using standardized generators.The migration to using
test_data_generatormethods and updated parameter names for the registration process improves test consistency and maintainability.
276-276: LGTM! Error message updated to match refined validation logic.The updated error message provides a clear, user-friendly response for payment validation failures.
454-454: LGTM! Sort key format updated to match new schema.The updated sort key format aligns with the new privilege record schema structure.
476-481: LGTM! Correctly validates privilege renewal behavior.The updated comment and assertion properly verify that privilege renewal updates existing records rather than creating duplicates, which is the correct behavior.
593-597: LGTM! Mocking updated for new data client API.The updated mocking correctly reflects the new
get_provider_user_recordsmethod and maintains proper test isolation.
748-748: LGTM! Minor formatting improvement.
866-895: LGTM! Well-structured test for encumbrance validation.This test properly validates that encumbered providers cannot purchase privileges. The test setup, execution, and assertions are all correct and cover an important business rule.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (14)
12-12: LGTM: Type-safe imports addedThe new imports support the transition to typed data models and home jurisdiction change functionality. These are necessary and well-organized.
Also applies to: 20-21, 34-34
102-102: Excellent type safety improvementThe method now returns a properly typed
LicenseDataobject instead of a raw dictionary, which improves code maintainability and IDE support.Also applies to: 135-135
165-198: Well-implemented provider records aggregationThe new method correctly handles pagination by accumulating results in
resp['Items']and returning the full dataset. The consistent read default is appropriate for this use case.
299-300: Type safety improvements in privilege generationThe method now uses typed
PrivilegeDataobjects consistently, which improves code safety and maintainability. The parameter and return type annotations provide clear contracts.Also applies to: 312-312, 317-317, 330-345
355-357: Good type safety improvements with complex business logicThe method now uses typed data objects consistently. The logic for handling
homeJurisdictionChangeStatusremoval during privilege renewal is well-documented and appears correct for the business requirements.Also applies to: 383-383, 397-444
519-519: Type safety improvements in rollback logicThe rollback method now uses typed data objects while maintaining the same reliable rollback functionality. This improves code safety without changing behavior.
Also applies to: 521-521, 530-530, 533-533, 563-563
806-829: Excellent method improvement with better return valueThe method now returns the actual
ProviderDataobject instead of just a boolean, which is more useful for callers. The improved error handling and logging add value.
831-902: Excellent rewrite with comprehensive improvementsThe method now uses typed data objects, creates proper audit trails with
ProviderUpdateData, and includes transaction safety with conditional expressions to prevent duplicate registrations. This is a significant improvement in both type safety and functionality.
1290-1426: Comprehensive home jurisdiction update featureThe main method includes well-documented business logic for handling various scenarios when a provider changes home jurisdiction. The privilege handling logic properly considers license expiration, encumbrance, and compact eligibility.
1835-1851: UpdateExpression construction handled correctlyThe method properly builds
set_clausesas a list and joins them with', ', which avoids the comma placement issues mentioned in past reviews. This is the correct approach for dynamic expression building.
1427-1506: Well-structured privilege validation logicThe helper method properly validates whether privileges can be moved to the new jurisdiction, handling cases where no matching license exists or the license is not compact eligible.
1507-1578: Robust provider record update with transaction safetyThe method properly updates the provider record when a valid license exists in the new jurisdiction, using transactions to ensure consistency between the provider record and update history.
1647-1726: Comprehensive privilege deactivation logicThe method properly handles privilege deactivation with appropriate audit trails and transaction safety. The batching respects DynamoDB limits.
1727-1872: Complex but well-implemented privilege update logicThe method handles multiple scenarios including same-jurisdiction privileges, previously deactivated privileges, and license encumbrance status propagation. The logic appears sound for the business requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
1720-1725: Implement rollback for multi-batch transactionsThe batch processing in both
_deactivate_privileges_for_jurisdiction_changeand_update_privileges_for_jurisdiction_changehas the same issue as identified in previous reviews - if a later batch fails, earlier batches remain committed without rollback, risking partial state.Apply the same rollback pattern recommended in the previous review:
- Track successfully processed batches
- Wrap batch processing in try/except for ClientError
- Implement rollback logic to undo processed batches on failure
- Re-raise the original error after rollback
This ensures atomicity across the entire multi-batch operation.
Also applies to: 1867-1872
🧹 Nitpick comments (2)
backend/compact-connect/tests/smoke/home_jurisdiction_change_smoke_tests.py (2)
169-215: Consider enhancing error handling and data validationThe helper function correctly creates a license record for testing purposes, but there are some areas for improvement:
Consider these enhancements:
def add_license_for_provider(provider_record: dict, jurisdiction: str): """ Add a license for a provider in a given jurisdiction """ + if not provider_record.get('licenses'): + raise ValueError("Provider record must contain at least one license") + license_type = provider_record['licenses'][0]['licenseType'] provider_id = provider_record['providerId'] compact = provider_record['compact'] + + # Validate required fields + if not all([license_type, provider_id, compact]): + raise ValueError("Provider record missing required fields") + license_record = { 'pk': f'{compact}#PROVIDER#{provider_id}', - 'sk': f'{compact}#PROVIDER#license/{jurisdiction}/{license_type}#', + 'sk': f'{compact}#PROVIDER#license/{jurisdiction}/slp#', # Use abbreviation consistently # ... rest of the record } + + try: # put the license in for the new jurisdiction logger.info('Adding temp license record', pk=license_record['pk'], sk=license_record['sk']) config.provider_user_dynamodb_table.put_item(Item=license_record) + except Exception as e: + logger.error(f"Failed to add license record: {e}") + raise + # give dynamodb time to propagate time.sleep(1) return license_record
344-350: Consider adding error handling for the main execution blockThe main execution block runs both tests but lacks error handling that could provide better feedback during test failures.
Consider adding error handling:
if __name__ == '__main__': - # Load environment variables from smoke_tests_env.json - load_smoke_test_env() - - # Run test - test_home_jurisdiction_change_inactivates_privileges_when_no_license_in_new_jurisdiction() - test_home_jurisdiction_change_moves_privileges_when_valid_license_in_new_jurisdiction() + try: + # Load environment variables from smoke_tests_env.json + load_smoke_test_env() + + # Run tests + logger.info("Starting home jurisdiction change smoke tests") + test_home_jurisdiction_change_inactivates_privileges_when_no_license_in_new_jurisdiction() + test_home_jurisdiction_change_moves_privileges_when_valid_license_in_new_jurisdiction() + logger.info("All smoke tests completed successfully") + except Exception as e: + logger.error(f"Smoke test failed: {e}") + raise
📜 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/data_client.py(19 hunks)backend/compact-connect/lambdas/python/purchases/handlers/privileges.py(4 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py(6 hunks)backend/compact-connect/tests/smoke/home_jurisdiction_change_smoke_tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/purchases/handlers/privileges.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (17)
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py (8)
99-106: LGTM: Updated to use new data generator methodsThe changes correctly update the test setup to use the new data generator methods and align with the updated data client interface that now separates provider and license record handling.
276-276: LGTM: Error message updated to match refined error handlingThe error message assertion has been updated to reflect the new payment validation error message format in the privilege purchase flow.
454-454: LGTM: Sort key format updated for license type abbreviationsThe privilege record sort key format has been correctly updated to incorporate license type abbreviations (
slpinstead of full license type name), consistent with the new key generation logic.
476-481: LGTM: Improved comment clarity and assertionThe updated comment better explains the renewal behavior (updating existing record vs creating new one) and the assertion correctly verifies only one privilege record exists.
593-597: LGTM: Updated mocking to reflect new data client methodThe mock has been correctly updated to use
get_provider_user_recordsinstead of the deprecatedget_providermethod, aligning with the new data client interface that returns aggregated typed objects.
749-804: LGTM: Comprehensive test for privilege reactivation after home jurisdiction changeThis test thoroughly validates that privileges previously deactivated due to home jurisdiction changes are properly reactivated upon renewal. The test setup correctly creates a privilege with
homeJurisdictionChangeStatus: 'inactive'and verifies it's removed after renewal.
806-864: LGTM: Well-structured test for privilege update record trackingThis test effectively verifies that privilege update records correctly capture the removal of
homeJurisdictionChangeStatuswhen privileges are renewed. The use of the test data generator's query method for privilege update records demonstrates good test design.
866-898: LGTM: Essential test for provider encumbrance validationThis test correctly validates that providers with encumbered status are prevented from purchasing privileges, which is a critical business rule. The error message assertion matches the expected user-facing message for this scenario.
backend/compact-connect/tests/smoke/home_jurisdiction_change_smoke_tests.py (3)
1-23: LGTM: Well-structured smoke test file setupThe file header and imports are well-organized. The shebang, clear comments explaining the purpose, and proper import structure demonstrate good practices for a smoke test script.
24-167: LGTM: Comprehensive test for privilege deactivation scenarioThis test thoroughly validates the complete flow when a provider changes to a jurisdiction without a license:
- Proper setup and state verification before changes
- API call validation with appropriate error handling
- Verification of privilege deactivation and eligibility changes
- Restoration testing to ensure the system can recover
- Final validation of privilege reactivation
The test design follows good practices with clear logging, appropriate delays for eventual consistency, and comprehensive assertions.
217-342: LGTM: Thorough test for privilege movement with valid licenseThis test effectively validates the scenario where a provider has a valid license in the new jurisdiction:
- Proper setup including temporary license creation
- Verification of privilege movement and status updates
- Bidirectional testing (forward and reverse jurisdiction changes)
- Proper cleanup of temporary test data
The test demonstrates good practices with comprehensive state validation and cleanup.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (6)
12-12: LGTM! Type-safe imports addedThe new imports properly support the transition to typed data classes and the home jurisdiction change functionality.
Also applies to: 20-21, 34-34
102-102: Excellent type safety improvementThe method now returns a typed
LicenseDataobject instead of a raw dictionary, improving code maintainability and reducing the risk of runtime errors.Also applies to: 135-135
165-197: Pagination implementation looks correctThe method properly accumulates all paginated results in
resp['Items']and returns the complete dataset viaProviderUserRecords(resp['Items']). This addresses the pagination bug mentioned in previous reviews.
1290-1427: Complex business logic requires verificationThe home jurisdiction change logic implements intricate business rules with multiple edge cases. While the implementation appears comprehensive, the complexity warrants thorough testing to ensure all scenarios are handled correctly.
Key business rules implemented:
- Deactivation when no license exists in new jurisdiction
- Handling of expired and encumbered licenses
- Privilege movement based on compact eligibility
- Proper status tracking with
homeJurisdictionChangeStatusPlease verify this complex business logic with comprehensive testing, including edge cases like:
- Providers with multiple license types
- Mixed states of encumbered/expired licenses
- Jurisdictions with partial license coverage
1835-1851: Excellent improvement in update expression constructionThe new approach properly builds update clauses in a list and joins them with commas, avoiding the syntax issues that could occur with the previous string concatenation approach. This is much more robust and maintainable.
831-902: Excellent refactoring to typed data structuresThe
process_registration_valuesmethod has been comprehensively refactored to use typedProviderDataandLicenseDataobjects, with proper transaction handling and audit trail creation viaProviderUpdateData. This significantly improves type safety and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/compact-connect/docs/api-specification/latest-oas30.json (3)
96-137: Consolidate repeated Cognito scopes
Multiple new administrative scopes (e.g.,ak/aslp.admin,co/aslp.admin, etc.) are added for this endpoint. Rather than duplicating the full list in each path, consider extracting common scopes into a shared security component (e.g.,#/components/securitySchemes/…) or leveraging OpenAPI’s$refto reduce repetition and simplify future maintenance.
1738-1778: AddPUT /v1/provider-users/me/home-jurisdiction
This endpoint correctly introduces the capability to update a provider’s home jurisdiction. A few improvements to consider:
- Add
summaryanddescriptionfields for better autogenerated documentation.- Define possible error responses (
400for invalid jurisdiction,401/403for auth failures).- Ensure consistency with other
meendpoints in terms of response schema and status codes.
3736-3797: DRY: Extract jurisdiction enum
ThecurrentHomeJurisdictionproperty is repeatedly injected into multiple provider-related schemas. To avoid enum drift and ease updates, extract the list of allowed abbreviations into a single reusable component (e.g.,#/components/schemas/JurisdictionAbbreviation) and reference it everywhere.Also applies to: 4882-4942, 6037-6097, 6638-6697
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/docs/api-specification/latest-oas30.json(28 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (3)
backend/compact-connect/docs/api-specification/latest-oas30.json (3)
5-5: Confirm API version bump
Theversionfield has been updated to2025-05-22T19:30:42Z. Please verify that this corresponds to the intended release timestamp and that any client SDKs or documentation generators are aligned with this new version.
78-78: Verify updated schema reference
The request body forPUT /v1/compacts/{compact}now references#/components/schemas/SandboLicenJL55EyhA697p. Ensure the new schema accurately captures the fields required for updating a compact’s configuration and that it’s fully defined (see definition at line6163).
2351-2419: DefineSandboLicenRoFVLhwPvO9Oschema
The new request schema restrictsjurisdictionto the postal abbreviations plus"other". Please confirm that this enum matches the backend validation and consider whether"unknown"should also be accepted at input or handled exclusively by the server.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
1647-1731: Consider implementing batch processing with rollback for scalability.While the current assumption of ≤50 jurisdictions may be reasonable, these methods lack the robust batch processing and rollback mechanisms implemented in
create_provider_privileges. Each privilege requires 2 transaction items, so 50 privileges would use 100 items (the DynamoDB limit).Consider implementing similar batch processing logic:
def _execute_transactions_with_rollback(self, transactions: list[dict], operation_name: str): """Execute transactions in batches with rollback on failure.""" batch_size = 100 processed_transactions = [] try: for i in range(0, len(transactions), batch_size): batch = transactions[i:i + batch_size] self.config.dynamodb_client.transact_write_items(TransactItems=batch) processed_transactions.extend(batch) if len(transactions) > batch_size: logger.info(f'Breaking {operation_name} into multiple transactions') except ClientError as e: logger.error(f'{operation_name} failed, rolling back', error=str(e)) # Implement rollback logic for processed_transactions raise CCAwsServiceException(f'{operation_name} failed') from eThis would provide better scalability and consistency guarantees.
Also applies to: 1733-1883
📜 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(19 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (5)
165-197: LGTM! Pagination issue properly addressed.The new
get_provider_user_recordsmethod correctly accumulates all paginated results inresp['Items']and returns the complete dataset. This properly addresses the pagination bug mentioned in previous reviews where only the last page was being returned.
102-102: Excellent type safety improvements!The migration to typed data classes (
LicenseData,PrivilegeData,ProviderData) across method signatures and return types significantly improves code maintainability and type safety. The consistent application of these types throughout the codebase is well-executed.Also applies to: 299-300, 355-356, 806-806, 834-836
1846-1862: UpdateExpression construction properly fixed.The refactored approach using
set_clauseslist and joining with commas addresses the previous issue with dynamic UpdateExpression construction. This is much more robust than the previous comma concatenation approach and ensures proper expression syntax.
1290-1427: Well-structured business logic for home jurisdiction changes.The
update_provider_home_state_jurisdictionmethod implements comprehensive business rules with clear separation of concerns. The logic flow properly handles various scenarios (no license, expired/encumbered licenses, ineligible licenses) and delegates appropriately to helper methods. The extensive documentation and logging provide good traceability.
831-902: Excellent refactoring of registration process.The updated
process_registration_valuesmethod properly integrates with the typed data model, creates appropriate update records, and maintains transaction consistency. The use ofProviderRecordUtilityfor record population and proper field updates is well-executed.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
This will allow users to move over more than 50 privileges at a time.
Providers need the ability to select a new home jurisdiction in the system should they move. There are many complexities in determining what should happen to privilege records when this happens. This adds a new PUT endpoint that manages that logic.
Closes #467
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests
Chores