Feat/Make dateOfRenewal field optional for licenses #923
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe changes make the Changes
Sequence Diagram(s)sequenceDiagram
participant Provider
participant API
participant IngestPipeline
participant LicenseSchema
Provider->>API: Submit license record (may omit dateOfRenewal)
API->>LicenseSchema: Validate license (dateOfRenewal optional)
API->>IngestPipeline: Pass license data
IngestPipeline->>IngestPipeline: Detect renewal (based on dateOfExpiration, dateOfRenewal optional)
IngestPipeline-->>API: Return result
API-->>Provider: Response
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
💤 Files with no reviewable changes (7)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ 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: 0
🧹 Nitpick comments (1)
backend/compact-connect/docs/postman/postman-collection.json (1)
80-84: Duplicate literal scope string → extract into single{{scope}}env varThe exact scope string now appears twice (array param + raw URL). If we ever tweak the pattern again, one occurrence might be missed, causing silent auth failures.
Recommend introducing a single environment/collection variable (e.g.{{scope}}) and reference it in both places:- "key": "scope", - "value": "{{jurisdiction}}/{{compact}}.write" + "key": "scope", + "value": "{{scope}}" ... -{{staffUserPoolUrl}}/oauth2/token?grant_type=client_credentials&client_id={{clientId}}&client_secret={{clientSecret}}&scope={{jurisdiction}}/{{compact}}.write +{{staffUserPoolUrl}}/oauth2/token?grant_type=client_credentials&client_id={{clientId}}&client_secret={{clientSecret}}&scope={{scope}}Reduces duplication and prevents drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/compact-connect/docs/README.md(1 hunks)backend/compact-connect/docs/postman/postman-collection.json(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py(4 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/common.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/ingest.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py(4 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py(6 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(0 hunks)backend/compact-connect/tests/resources/snapshots/POST_LICENSES_REQUEST_SCHEMA.json(0 hunks)
💤 Files with no reviewable changes (2)
- backend/compact-connect/tests/resources/snapshots/POST_LICENSES_REQUEST_SCHEMA.json
- backend/compact-connect/stacks/api_stack/v1_api/api_model.py
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#882
File: backend/compact-connect/lambdas/python/migration/compact_configured_states_871/main.py:127-130
Timestamp: 2025-06-26T16:42:00.781Z
Learning: In the compact_configured_states_871 migration, existing jurisdiction configurations that have licenseeRegistrationEnabled: true are migrated with isLive: true for backwards compatibility. This prevents breaking existing live functionality since those states are already operational. The migration preserves the current live state of jurisdictions to maintain service continuity, while new states added after migration can start with isLive: false and be controlled by compact admins.
Learnt from: jsandoval81
PR: csg-org/CompactConnect#922
File: webroot/src/components/UserAccount/UserAccount.ts:0-0
Timestamp: 2025-07-10T19:50:56.706Z
Learning: In the UserAccount component (webroot/src/components/UserAccount/UserAccount.ts), the email field should be disabled for staff users (`isDisabled: this.isStaff`) to maintain existing restrictions, while licensees should be able to change their email address through the new verification process. This is the intended behavior per PR #922 requirements.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/migration/provider_user_pool_migration_551/main.py:35-39
Timestamp: 2025-06-10T03:16:16.896Z
Learning: In the provider user pool migration (provider_user_pool_migration_551), the FilterExpression intentionally only checks for the existence of compactConnectRegisteredEmailAddress. The migration should only remove currentHomeJurisdiction if compactConnectRegisteredEmailAddress is also present, targeting records that went through the old registration process. Records with only currentHomeJurisdiction but no compactConnectRegisteredEmailAddress should be left untouched.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Learnt from: jusdino
PR: csg-org/CompactConnect#864
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:256-262
Timestamp: 2025-06-18T21:57:02.978Z
Learning: The `licenseJurisdiction` field is a required field in the provider data API response from the `/v1/providers/users/me` endpoint, so it can be accessed directly without defensive programming checks.
backend/compact-connect/docs/README.md (4)
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#882
File: backend/compact-connect/lambdas/python/migration/compact_configured_states_871/main.py:127-130
Timestamp: 2025-06-26T16:42:00.781Z
Learning: In the compact_configured_states_871 migration, existing jurisdiction configurations that have licenseeRegistrationEnabled: true are migrated with isLive: true for backwards compatibility. This prevents breaking existing live functionality since those states are already operational. The migration preserves the current live state of jurisdictions to maintain service continuity, while new states added after migration can start with isLive: false and be controlled by compact admins.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Learnt from: jusdino
PR: csg-org/CompactConnect#864
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:256-262
Timestamp: 2025-06-18T21:57:02.978Z
Learning: The `licenseJurisdiction` field is a required field in the provider data API response from the `/v1/providers/users/me` endpoint, so it can be accessed directly without defensive programming checks.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/common.py (1)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py:40-43
Timestamp: 2025-04-29T02:18:11.099Z
Learning: In the CompactConnect codebase, API design follows a pattern where fields are either provided with valid values or omitted entirely. Null values are not allowed, even for optional fields. This applies to Marshmallow schemas where fields are configured with `required=False, allow_none=False`.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/ingest.py (1)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py:40-43
Timestamp: 2025-04-29T02:18:11.099Z
Learning: In the CompactConnect codebase, API design follows a pattern where fields are either provided with valid values or omitted entirely. Null values are not allowed, even for optional fields. This applies to Marshmallow schemas where fields are configured with `required=False, allow_none=False`.
backend/compact-connect/docs/postman/postman-collection.json (2)
undefined
<retrieved_learning>
Learnt from: landonshumway-ia
PR: #798
File: backend/compact-connect/docs/api-specification/latest-oas30.json:55-127
Timestamp: 2025-05-14T17:00:51.255Z
Learning: The mixed format of security scope names in OpenAPI definitions (slash / for compact-level scopes like "aslp/admin" and dot . for jurisdiction-specific scopes like "al/aslp.admin") is defined by AWS Cognito's constraints and is not an issue that needs standardization.
</retrieved_learning>
<retrieved_learning>
Learnt from: landonshumway-ia
PR: #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.
</retrieved_learning>
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (1)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py:40-43
Timestamp: 2025-04-29T02:18:11.099Z
Learning: In the CompactConnect codebase, API design follows a pattern where fields are either provided with valid values or omitted entirely. Null values are not allowed, even for optional fields. This applies to Marshmallow schemas where fields are configured with `required=False, allow_none=False`.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (1)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#824
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:1357-1490
Timestamp: 2025-05-30T13:57:36.286Z
Learning: The `LicenseUtility.get_license_type_by_abbreviation` method returns `None` when given an invalid license type abbreviation, rather than raising an exception like KeyError or ValueError.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py (1)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py:40-43
Timestamp: 2025-04-29T02:18:11.099Z
Learning: In the CompactConnect codebase, API design follows a pattern where fields are either provided with valid values or omitted entirely. Null values are not allowed, even for optional fields. This applies to Marshmallow schemas where fields are configured with `required=False, allow_none=False`.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py (1)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
🧬 Code Graph Analysis (6)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/common.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (1)
dateOfRenewal(76-77)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/ingest.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (1)
dateOfRenewal(76-77)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (1)
dateOfRenewal(76-77)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
dateOfRenewal(48-49)backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
get(77-78)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
UpdateCategory(297-309)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (1)
dateOfRenewal(76-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
🔇 Additional comments (15)
backend/compact-connect/docs/postman/postman-collection.json (1)
81-82: Confirm scope naming matches OAS & Cognito configPer previous guidance (PR #798) the pattern for jurisdiction-level scopes is
<jurisdiction>/<resource>.<verb>.
{{jurisdiction}}/{{compact}}.writefits, assuming{{compact}}resolves to the resource segment (e.g.aslp). Please double-check that:
- The matching resource in Cognito/OAS is indeed
{{compact}}.write, not{{compact}}.adminor similar.- All API requests in the collection that rely on the issued token truly require the
*.writeprivilege; otherwise least-privilege is violated.No action required if both are true.
backend/compact-connect/docs/README.md (1)
39-39: Documentation correctly updated to reflect optional status.The removal of the asterisk (*) from the
dateOfRenewalfield properly reflects that this field is now optional rather than required.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/common.py (1)
33-33: Schema correctly updated to make dateOfRenewal optional.The change from
required=Truetorequired=Falsewhile maintainingallow_none=Falsefollows the established codebase pattern for optional fields.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/ingest.py (1)
85-85: Ingest schema correctly updated for optional dateOfRenewal.The change from
required=Truetorequired=Falsewhile maintainingallow_none=Falseproperly accommodates optional dateOfRenewal in sanitized ingest events.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (1)
150-150: Update record schema correctly aligned with optional dateOfRenewal.The change from
required=Truetorequired=Falsewhile maintainingallow_none=Falseensures consistency with the main license schema updates.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (1)
76-77: Property accessor correctly updated for optional dateOfRenewal.The change from direct dictionary access (
self._data['dateOfRenewal']) to the safe.get()method properly handles the optional nature of the field. The return type annotationdate | Noneaccurately reflects that the field may be absent.backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py (3)
231-231: LGTM: Improved debugging information.The added log message helps with troubleshooting by clearly indicating when no license changes are detected.
274-276: LGTM: Clearer function signature.The updated parameter names (
updated_valuesandremoved_values) better reflect the actual inputs and make the function purpose more explicit.
285-293: LGTM: Sound renewal detection logic for optional dateOfRenewal.The logic change appropriately adapts to the optional nature of
dateOfRenewal. Detecting renewals based solely ondateOfExpirationextension is a reasonable approach that maintains functionality while accommodating states that don't provide renewal dates.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py (4)
56-56: LGTM: Consistent schema update for optional dateOfRenewal.Making
dateOfRenewaloptional inLicensePostRequestSchemaaligns with the PR objective while maintaining theallow_none=Falsepattern consistent with CompactConnect's API design.
100-100: LGTM: Consistent schema update across response schemas.The change in
LicenseUpdatePreviousGeneralResponseSchemamaintains consistency with other schema updates.
162-162: LGTM: Consistent schema update in general response schema.The
LicenseGeneralResponseSchemaupdate maintains consistency across all license-related API schemas.
191-191: LGTM: Complete schema consistency maintained.The final schema update in
LicenseUpdatePreviousResponseSchemacompletes the consistent application of makingdateOfRenewaloptional across all license API schemas.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py (2)
32-32: LGTM: Flexible test helper for optional field scenarios.The added
omit_date_of_renewalparameter enables comprehensive testing of both scenarios, ensuring the optional nature ofdateOfRenewalis properly validated.Also applies to: 46-47
395-494: LGTM: Comprehensive test coverage for optional dateOfRenewal scenarios.The new helper method
_when_test_existing_provider_renewaland separate test cases for renewal with and withoutdateOfRenewalprovide excellent coverage. The test properly validates:
- License renewal detection works with only
dateOfExpirationextension- Response structure correctly omits
dateOfRenewalwhen not provided- History tracking accurately reflects field presence/absence
This ensures robust handling of the optional field across different state data variations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/tests/smoke/license_upload_smoke_tests.py (1)
88-89: Consider adding test coverage for both scenarios with dateOfRenewal present and absent.While removing
dateOfRenewalfrom the test payload correctly validates that the field is now optional, consider adding a second test case or parameterized test to verify that the field still works correctly when provided. This would ensure both scenarios are covered and prevent regressions.Add a test case that includes
dateOfRenewalto validate both optional and present scenarios:# Add after the existing test payload post_body_with_renewal = [ { 'npi': '1111111112', 'licenseNumber': 'A0608337261', 'homeAddressPostalCode': '68001', 'givenName': TEST_PROVIDER_GIVEN_NAME, 'familyName': TEST_PROVIDER_FAMILY_NAME, 'homeAddressStreet1': '123 Fake Street', 'dateOfBirth': '1991-12-10', 'dateOfIssuance': '2024-12-10', 'dateOfRenewal': '2025-12-10', # Include the optional field 'ssn': MOCK_SSN, 'licenseType': 'audiologist', 'dateOfExpiration': '2050-12-10', 'homeAddressState': 'AZ', 'homeAddressCity': 'Omaha', 'compactEligibility': 'eligible', 'licenseStatus': 'active', } ]This would help ensure that making the field optional doesn't break existing functionality when the field is provided.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/tests/smoke/license_upload_smoke_tests.py(1 hunks)backend/compact-connect/tests/smoke/smoke_common.py(0 hunks)
💤 Files with no reviewable changes (1)
- backend/compact-connect/tests/smoke/smoke_common.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#882
File: backend/compact-connect/lambdas/python/migration/compact_configured_states_871/main.py:127-130
Timestamp: 2025-06-26T16:42:00.781Z
Learning: In the compact_configured_states_871 migration, existing jurisdiction configurations that have licenseeRegistrationEnabled: true are migrated with isLive: true for backwards compatibility. This prevents breaking existing live functionality since those states are already operational. The migration preserves the current live state of jurisdictions to maintain service continuity, while new states added after migration can start with isLive: false and be controlled by compact admins.
Learnt from: jsandoval81
PR: csg-org/CompactConnect#922
File: webroot/src/components/UserAccount/UserAccount.ts:0-0
Timestamp: 2025-07-10T19:50:56.706Z
Learning: In the UserAccount component (webroot/src/components/UserAccount/UserAccount.ts), the email field should be disabled for staff users (`isDisabled: this.isStaff`) to maintain existing restrictions, while licensees should be able to change their email address through the new verification process. This is the intended behavior per PR #922 requirements.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/migration/provider_user_pool_migration_551/main.py:35-39
Timestamp: 2025-06-10T03:16:16.896Z
Learning: In the provider user pool migration (provider_user_pool_migration_551), the FilterExpression intentionally only checks for the existence of compactConnectRegisteredEmailAddress. The migration should only remove currentHomeJurisdiction if compactConnectRegisteredEmailAddress is also present, targeting records that went through the old registration process. Records with only currentHomeJurisdiction but no compactConnectRegisteredEmailAddress should be left untouched.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Learnt from: jusdino
PR: csg-org/CompactConnect#864
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:256-262
Timestamp: 2025-06-18T21:57:02.978Z
Learning: The `licenseJurisdiction` field is a required field in the provider data API response from the `/v1/providers/users/me` endpoint, so it can be accessed directly without defensive programming checks.
backend/compact-connect/tests/smoke/license_upload_smoke_tests.py (6)
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#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/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#882
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py:287-359
Timestamp: 2025-07-08T18:40:24.408Z
Learning: In the CompactConnect codebase, landonshumway-ia prefers to avoid extraneous unit tests when existing test coverage is already sufficient to catch bugs. For the get_privilege_purchase_options method's live-jurisdiction filtering logic, the existing tests in the purchases test suite provide adequate coverage without needing additional edge case tests.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/migration/provider_user_pool_migration_551/main.py:35-39
Timestamp: 2025-06-10T03:16:16.896Z
Learning: In the provider user pool migration (provider_user_pool_migration_551), the FilterExpression intentionally only checks for the existence of compactConnectRegisteredEmailAddress. The migration should only remove currentHomeJurisdiction if compactConnectRegisteredEmailAddress is also present, targeting records that went through the old registration process. Records with only currentHomeJurisdiction but no compactConnectRegisteredEmailAddress should be left untouched.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
|
@jlkravitz This is ready for your review. Thanks |
jlkravitz
approved these changes
Jul 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
During onboarding several states, it has been revealed that not all states have a date of renewal for license records. Management has determined to make the 'dateOfRenewal' field optional for states when uploading license data.
This change updates the field in the schemas and documentation from required to optional.
Closes #920
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
dateOfRenewalfield is now optional in CSV field descriptions.Refactor
dateOfRenewalfield is now optional across all relevant endpoints and data models.Tests
dateOfRenewalfield.Chores
dateOfRenewalfield.