support list of npdb categories for encumbrances #1135
Conversation
WalkthroughFeature-flagged migration from a single clinicalPrivilegeActionCategory to plural clinicalPrivilegeActionCategories across schemas, records, handlers, tests, and events; removed adverse_action_category from several event/public payloads; added ProviderManagement lambdas to ApiLambdaStack and introduced an encumbrance feature flag and environment propagation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as API (Provider)
participant Handler as Encumbrance Handler
participant FF as FeatureFlagClient
participant Schema as AdverseAction Schema
participant DB as Provider DB
participant Bus as Event Bus
participant Worker as Data-Events
participant DC as DataClient
Client->>API: POST /encumbrances
API->>Handler: invoke handler with payload
Handler->>FF: is_feature_enabled("encumbrance-multi-category-flag")
alt Flag ON
Handler->>Schema: build adverseAction with clinicalPrivilegeActionCategories (list)
else Flag OFF
Handler->>Schema: build adverseAction with clinicalPrivilegeActionCategory (string)
end
Handler->>DB: persist adverseAction
Handler->>Bus: publish encumbrance event (adverseActionId) %% adverseActionCategory removed
Bus->>Worker: deliver event
Worker->>FF: is_feature_enabled("encumbrance-multi-category-flag")
Worker->>DC: encumber_* (select encumbranceDetails: categories vs category)
DC->>DB: update privilege/license encumbranceDetails
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
landonshumway-ia
force-pushed
the
feat/encumbrance-multiple-npdb-categories
branch
2 times, most recently
from
b2db442 to
6e22f88
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py (1)
78-79: Validator bug: use enum values, not enum instancesactionAgainst is a String; OneOf should validate against [e.value], not [e].
Apply this diff:
- actionAgainst = String(required=True, allow_none=False, validate=OneOf([e for e in AdverseActionAgainstEnum])) + actionAgainst = String( + required=True, + allow_none=False, + validate=OneOf([e.value for e in AdverseActionAgainstEnum]), + )
🧹 Nitpick comments (18)
backend/compact-connect/stacks/event_listener_stack/__init__.py (1)
38-41: Approve conditional API_BASE_URL propagation; add domain‐format validationThe conditional update is correct and safe, but there’s no existing validation of
api_domain_name. To prevent malformed URLs if a protocol prefix is passed, strip anyhttp:///https://before constructingAPI_BASE_URL.Consider:
if persistent_stack.api_domain_name: + domain = persistent_stack.api_domain_name.removeprefix('https://').removeprefix('http://') self.common_env_vars.update({'API_BASE_URL': f'https://{domain}'})backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (1)
93-99: Consider validating non-empty list in setter.The
clinicalPrivilegeActionCategoriessetter acceptslist[str]but doesn't validate that the list is non-empty. If an empty list is semantically invalid (which seems likely for NPDB categories), consider adding validation.Apply this diff to add validation:
@clinicalPrivilegeActionCategories.setter def clinicalPrivilegeActionCategories(self, value: list[str]) -> None: + if not value: + raise ValueError('clinicalPrivilegeActionCategories must be a non-empty list') self._data['clinicalPrivilegeActionCategories'] = valuebackend/compact-connect/stacks/api_stack/v1_api/api_model.py (3)
417-436: Encumbrance categories: add validation (exclusivity, min_items, unique_items).Currently both fields can be sent and arrays can be empty/duplicated. Recommend:
- Enforce exactly one of single vs. list (one_of).
- Require at least 1 category and unique values.
Apply:
self.api._v1_post_privilege_encumbrance_request_model = self.api.add_model( @@ schema=JsonSchema( type=JsonSchemaType.OBJECT, additional_properties=False, - required=['encumbranceEffectiveDate', 'encumbranceType'], + required=['encumbranceEffectiveDate', 'encumbranceType'], properties={ @@ - 'clinicalPrivilegeActionCategories': JsonSchema( - type=JsonSchemaType.ARRAY, - description='The categories of clinical privilege action', - items=JsonSchema(type=JsonSchemaType.STRING), - ), + 'clinicalPrivilegeActionCategories': JsonSchema( + type=JsonSchemaType.ARRAY, + description='The categories of clinical privilege action', + min_items=1, + unique_items=True, + items=JsonSchema(type=JsonSchemaType.STRING), + ), }, + one_of=[ + JsonSchema(required=['clinicalPrivilegeActionCategory']), + JsonSchema(required=['clinicalPrivilegeActionCategories']), + ], ),
454-473: Mirror validation on license encumbrance request.Same constraints should apply here for consistency.
self.api._v1_post_license_encumbrance_request_model = self.api.add_model( @@ schema=JsonSchema( type=JsonSchemaType.OBJECT, additional_properties=False, - required=['encumbranceEffectiveDate', 'encumbranceType'], + required=['encumbranceEffectiveDate', 'encumbranceType'], properties={ @@ - 'clinicalPrivilegeActionCategories': JsonSchema( - type=JsonSchemaType.ARRAY, - description='The categories of clinical privilege action', - items=JsonSchema(type=JsonSchemaType.STRING), - ), + 'clinicalPrivilegeActionCategories': JsonSchema( + type=JsonSchemaType.ARRAY, + description='The categories of clinical privilege action', + min_items=1, + unique_items=True, + items=JsonSchema(type=JsonSchemaType.STRING), + ), }, + one_of=[ + JsonSchema(required=['clinicalPrivilegeActionCategory']), + JsonSchema(required=['clinicalPrivilegeActionCategories']), + ], ),
1217-1223: Response shape: mark single field as deprecated; keep list as canonical.Minor polish: annotate the single field as deprecated in the response too.
- 'clinicalPrivilegeActionCategory': JsonSchema(type=JsonSchemaType.STRING), + 'clinicalPrivilegeActionCategory': JsonSchema( + type=JsonSchemaType.STRING, + description='(Deprecated) Use clinicalPrivilegeActionCategories' + ),Repeat for both adverseActions response blocks.
Also applies to: 1358-1364
backend/compact-connect/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json (1)
32-41: Schema accepts new plural field; consider validation refinements
- Deprecation notice is clear.
- Consider enforcing at least one category and uniqueness, and allowing either singular or plural during migration via anyOf.
Suggested additions:
"clinicalPrivilegeActionCategories": { "description": "The categories of clinical privilege action", "items": { "type": "string" }, - "type": "array" + "type": "array", + "minItems": 1, + "uniqueItems": true } }, + "anyOf": [ + { "required": ["clinicalPrivilegeActionCategories"] }, + { "required": ["clinicalPrivilegeActionCategory"] } + ], "required": [ "encumbranceEffectiveDate", "encumbranceType" ],Also applies to: 45-45
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
380-387: Avoid repeated remote flag checks inside the loopis_feature_enabled likely makes a network call; calling it per event is wasteful. Cache its result once before iterating.
Apply this diff:
- from cc_common.feature_flag_client import is_feature_enabled - - if is_feature_enabled('encumbrance-multi-category-flag'): + from cc_common.feature_flag_client import is_feature_enabled + # Cache the flag result outside the per-event path + flag_enabled = getattr(ProviderRecordUtility, '_encumbrance_multi_cat_flag', None) + if flag_enabled is None: + flag_enabled = is_feature_enabled('encumbrance-multi-category-flag') + # stash for the duration of this process; safe in Lambda container + ProviderRecordUtility._encumbrance_multi_cat_flag = flag_enabled + + if flag_enabled: if 'clinicalPrivilegeActionCategory' in event['encumbranceDetails']: event['note'] = event['encumbranceDetails'].get('clinicalPrivilegeActionCategory') else: # else we are using the new field, parse the list into a comma-separated string event['note'] = ', '.join(event['encumbranceDetails']['clinicalPrivilegeActionCategories']) else: event['note'] = event['encumbranceDetails']['clinicalPrivilegeActionCategory']backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py (4)
106-122: Ensure unique privilege selection or assert expectationYou select the first NE privilege. If multiple NE privileges can exist, assert exactly one or select deterministically (e.g., by creationDate) to avoid flaky tests.
204-208: Harden error handling on non-200 responsesresponse.json() can raise if body isn’t JSON. Fall back to response.text to preserve the original error.
Apply this diff:
- if response.status_code != 200: - operation = 'lift' if encumbrance_id else 'create' - raise SmokeTestFailureException(f'Failed to {operation} license encumbrance. Response: {response.json()}') + if response.status_code != 200: + operation = 'lift' if encumbrance_id else 'create' + try: + err = response.json() + except Exception: + err = response.text + raise SmokeTestFailureException(f'Failed to {operation} license encumbrance. Response: {err}')- if response.status_code != 200: - operation = 'lift' if encumbrance_id else 'create' - raise SmokeTestFailureException(f'Failed to {operation} privilege encumbrance. Response: {response.json()}') + if response.status_code != 200: + operation = 'lift' if encumbrance_id else 'create' + try: + err = response.json() + except Exception: + err = response.text + raise SmokeTestFailureException(f'Failed to {operation} privilege encumbrance. Response: {err}')Also applies to: 234-238
283-289: Guarantee at least one polling attemptIf max_wait_time < check_interval, max_attempts becomes 0 and the loop never runs. Use ceil division with a floor of 1.
Apply this diff:
- attempts = 0 - max_attempts = max_wait_time // check_interval + attempts = 0 + max_attempts = max(1, (max_wait_time + check_interval - 1) // check_interval)Also applies to: 291-297, 301-307
512-546: Avoid duplicated state checks; reuse helperYou manually re-fetch and check license/provider state here. Consider calling validate_license_encumbered_state and validate_provider_encumbered_state for consistency and less duplication.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py (1)
28-31: Enforce non-empty category list and mutual exclusivityAllowing an empty list weakens prior semantics (singular was required). Add Length(min=1) to the list field to ensure at least one category when the plural is used; keep your exclusivity check.
Apply this diff:
-from marshmallow.validate import OneOf +from marshmallow.validate import OneOf, Length @@ - clinicalPrivilegeActionCategories = List(ClinicalPrivilegeActionCategoryField(), required=False, allow_none=False) + clinicalPrivilegeActionCategories = List( + ClinicalPrivilegeActionCategoryField(), + required=False, + allow_none=False, + validate=Length(min=1), + )Also applies to: 32-47
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py (2)
49-60: Load-time migration is good; also drop deprecated field when both presentIf both fields exist, drop the deprecated one to keep the in-memory object clean.
Apply this diff:
- def migrate_clinical_privilege_action_category_on_load(self, in_data, **_kwargs): + def migrate_clinical_privilege_action_category_on_load(self, in_data, **_kwargs): """Migrate deprecated clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories list when loading from database.""" - # If the deprecated field exists and the new field doesn't, migrate it - if 'clinicalPrivilegeActionCategory' in in_data and 'clinicalPrivilegeActionCategories' not in in_data: - in_data['clinicalPrivilegeActionCategories'] = [in_data['clinicalPrivilegeActionCategory']] - # Remove the deprecated field to avoid having both - del in_data['clinicalPrivilegeActionCategory'] + if 'clinicalPrivilegeActionCategory' in in_data: + if 'clinicalPrivilegeActionCategories' not in in_data: + in_data['clinicalPrivilegeActionCategories'] = [in_data['clinicalPrivilegeActionCategory']] + # Remove the deprecated field to avoid having both + del in_data['clinicalPrivilegeActionCategory'] return in_data
61-66: Pre-dump migration: also remove deprecated field to avoid writing bothYou add the plural but leave the singular; consider removing the singular at dump time for DB cleanliness during the transition.
Apply this diff:
def migrate_clinical_privilege_action_category(self, in_data, **_kwargs): """Migrate deprecated clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories list.""" - # If the deprecated field exists and the new field doesn't, migrate it for backwards compatibility - if 'clinicalPrivilegeActionCategory' in in_data and 'clinicalPrivilegeActionCategories' not in in_data: - in_data['clinicalPrivilegeActionCategories'] = [in_data['clinicalPrivilegeActionCategory']] + # If the deprecated field exists and the new field doesn't, migrate it + if 'clinicalPrivilegeActionCategory' in in_data and 'clinicalPrivilegeActionCategories' not in in_data: + in_data['clinicalPrivilegeActionCategories'] = [in_data['clinicalPrivilegeActionCategory']] + # Remove deprecated field if present to avoid persisting both + if 'clinicalPrivilegeActionCategory' in in_data: + del in_data['clinicalPrivilegeActionCategory'] return in_dataAlso applies to: 76-83
backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (1)
60-66: Add provider_encumbrance_handler log group to API log_groupsYou append log groups for other handlers, but not for provider_encumbrance_handler. Add it once to ensure logs are tracked with the API.
Apply this diff:
# Reference lambda handlers from api_lambda_stack self.get_provider_handler = api_lambda_stack.provider_management_lambdas.get_provider_handler self.query_providers_handler = api_lambda_stack.provider_management_lambdas.query_providers_handler self.get_provider_ssn_handler = api_lambda_stack.provider_management_lambdas.get_provider_ssn_handler self.deactivate_privilege_handler = api_lambda_stack.provider_management_lambdas.deactivate_privilege_handler self.provider_encumbrance_handler = api_lambda_stack.provider_management_lambdas.provider_encumbrance_handler + self.api.log_groups.append(self.provider_encumbrance_handler.log_group)Also applies to: 230-255, 275-317
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (1)
114-120: Comment mismatch with assertions (minor).The comment says “event was published for the affected privilege,” but two events are asserted (for 'ne' and 'ky'). Update the comment to reflect both events.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (2)
356-393: Comment text contradicts assertions (nit).The comment says “deprecated field is not present” but the test asserts it IS present. Update the comment to reflect current migration behavior.
Apply this diff to fix the comment:
- # TODO - remove this assertion as part of https://github.com/csg-org/CompactConnect/issues/1136 # noqa: FIX002 - # Verify that the deprecated field is not present in the stored data + # TODO - remove this assertion as part of https://github.com/csg-org/CompactConnect/issues/1136 # noqa: FIX002 + # Verify that the deprecated field is still present during migration
643-680: Same comment mismatch for license migration test (nit).Adjust the comment to say the deprecated field is still present during migration.
- # TODO - remove this assertion as part of https://github.com/csg-org/CompactConnect/issues/1136 # noqa: FIX002 - # Verify that the deprecated field is not present in the stored data + # TODO - remove this assertion as part of https://github.com/csg-org/CompactConnect/issues/1136 # noqa: FIX002 + # Verify that the deprecated field is still present during migration
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(4 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py(0 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py(2 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(0 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(7 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(4 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_privilege_history.py(3 hunks)backend/compact-connect/stacks/api_lambda_stack/__init__.py(3 hunks)backend/compact-connect/stacks/api_lambda_stack/provider_management.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(6 hunks)backend/compact-connect/stacks/api_stack/v1_api/provider_management.py(8 hunks)backend/compact-connect/stacks/event_listener_stack/__init__.py(1 hunks)backend/compact-connect/stacks/feature_flag_stack/__init__.py(1 hunks)backend/compact-connect/tests/app/test_api/test_provider_management_api.py(23 hunks)backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json(4 hunks)backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_SSN_ANOMALY_DETECTION_ALARM_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_SSN_ENDPOINT_DISABLED_ALARM_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_SSN_READS_RATE_LIMITED_ALARM_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json(4 hunks)backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py(16 hunks)backend/compact-connect/tests/smoke/smoke_common.py(2 hunks)
💤 Files with no reviewable changes (2)
- backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
- backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/stacks/api_stack/v1_api/api_model.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.pybackend/compact-connect/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.jsonbackend/compact-connect/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.jsonbackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
📚 Learning: 2025-09-03T22:29:24.535Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py:0-0
Timestamp: 2025-09-03T22:29:24.535Z
Learning: The EncumbranceEventDetailSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py is used across multiple instances/contexts where adverseActionCategory and adverseActionId fields may be required in some cases and not present in others. This is an intentional design pattern for handling different event variants with a single schema.
Applied to files:
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/stacks/api_stack/v1_api/api_model.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.pybackend/compact-connect/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.jsonbackend/compact-connect/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.jsonbackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
🧬 Code graph analysis (16)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (7)
PrivilegeUpdateData(91-162)compact(28-29)compact(108-109)jurisdiction(32-33)jurisdiction(112-113)encumbranceDetails(151-155)updatedValues(124-125)backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py (1)
encumbrance_handler(42-54)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
serialize_to_database_record(205-208)from_database_record(124-139)to_dict(169-180)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (9)
compact(27-28)compact(31-32)jurisdiction(43-44)jurisdiction(47-48)AdverseActionData(14-147)clinicalPrivilegeActionCategory(84-85)clinicalPrivilegeActionCategory(88-91)clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_default_privilege_update(329-356)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (4)
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py (1)
is_feature_enabled(46-112)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (8)
clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)adverseActionId(126-127)adverseActionId(130-131)clinicalPrivilegeActionCategory(84-85)clinicalPrivilegeActionCategory(88-91)jurisdiction(43-44)jurisdiction(47-48)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
get_adverse_action_by_id(525-535)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCInternalException(44-45)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py (3)
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py (1)
is_feature_enabled(46-112)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (4)
clinicalPrivilegeActionCategory(84-85)clinicalPrivilegeActionCategory(88-91)clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
ClinicalPrivilegeActionCategory(375-388)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (2)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
get(86-87)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
ClinicalPrivilegeActionCategory(375-388)
backend/compact-connect/stacks/api_lambda_stack/__init__.py (2)
backend/compact-connect/common_constructs/ssm_parameter_utility.py (2)
SSMParameterUtility(8-43)load_data_event_bus_from_ssm_parameter(26-43)backend/compact-connect/stacks/api_lambda_stack/provider_management.py (1)
ProviderManagementLambdas(17-329)
backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py (3)
backend/compact-connect/tests/smoke/smoke_common.py (2)
get_provider_user_records(251-264)SmokeTestFailureException(13-17)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (7)
get_privilege_records(481-489)get_specific_license_record(447-462)get_specific_privilege_record(464-479)get_provider_record(79-84)get_provider_record(605-614)get_adverse_action_records_for_license(507-523)get_adverse_action_records_for_privilege(587-603)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
licenseTypeAbbreviation(156-167)to_dict(169-180)
backend/compact-connect/tests/smoke/smoke_common.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
ProviderUserRecords(403-791)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(173-204)backend/compact-connect/tests/smoke/config.py (1)
provider_user_dynamodb_table(40-41)
backend/compact-connect/tests/app/test_api/test_provider_management_api.py (2)
backend/compact-connect/tests/app/test_api/__init__.py (2)
TestApi(11-83)generate_expected_integration_object_for_imported_lambda(48-83)backend/compact-connect/tests/app/base.py (1)
get_resource_properties_by_logical_id(89-96)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (5)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (3)
put_default_adverse_action_record_in_provider_table(154-160)put_default_provider_record_in_provider_table(409-429)put_default_privilege_record_in_provider_table(310-320)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (1)
license_encumbrance_listener(186-243)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(173-204)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (3)
compact(28-29)compact(108-109)encumberedStatus(72-73)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
serialize_to_database_record(205-208)PrivilegeEncumberedStatusEnum(317-321)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (4)
clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)clinicalPrivilegeActionCategory(84-85)clinicalPrivilegeActionCategory(88-91)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
ClinicalPrivilegeActionCategoryField(114-116)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (6)
clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)adverseActionId(126-127)adverseActionId(130-131)clinicalPrivilegeActionCategory(84-85)clinicalPrivilegeActionCategory(88-91)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
ClinicalPrivilegeActionCategoryField(114-116)
backend/compact-connect/stacks/api_lambda_stack/provider_management.py (4)
backend/compact-connect/stacks/persistent_stack/event_bus.py (1)
EventBus(9-25)backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(21-155)backend/compact-connect/stacks/persistent_stack/__init__.py (1)
PersistentStack(38-510)backend/compact-connect/lambdas/python/common/cc_common/config.py (6)
provider_table(88-89)ssn_table(92-93)ssn_index_name(179-180)event_bus_name(84-85)rate_limiting_table(282-283)user_pool_id(191-195)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (4)
clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)clinicalPrivilegeActionCategory(84-85)clinicalPrivilegeActionCategory(88-91)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
ClinicalPrivilegeActionCategoryField(114-116)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (2)
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py (1)
is_feature_enabled(46-112)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (3)
AdverseActionData(14-147)adverseActionId(126-127)adverseActionId(130-131)
backend/compact-connect/stacks/feature_flag_stack/__init__.py (1)
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (2)
FeatureFlagResource(23-72)FeatureFlagEnvironmentName(15-20)
backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (2)
backend/compact-connect/stacks/api_lambda_stack/__init__.py (1)
ApiLambdaStack(15-60)backend/compact-connect/stacks/state_api_stack/v1_api/provider_management.py (1)
_add_get_provider(74-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (39)
backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_SSN_ENDPOINT_DISABLED_ALARM_SCHEMA.json (1)
2-2: LGTM!The alarm description correctly reflects the migration to
ApiLambdaStack/GetProviderSSNHandlerfrom the previousAPIStack/LicenseApistructure.backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_SSN_READS_RATE_LIMITED_ALARM_SCHEMA.json (1)
2-2: LGTM!Consistent with the stack refactoring that moves SSN handlers to
ApiLambdaStack/GetProviderSSNHandler.backend/compact-connect/stacks/api_stack/v1_api/api.py (1)
200-208: LGTM TheProviderManagementconstructor includes theapi_lambda_stackparameter.backend/compact-connect/stacks/feature_flag_stack/__init__.py (1)
116-125: Approve feature flag addition: Verified ‘encumbrance-multi-category-flag’ is referenced in provider-data-v1 handlers and cc_common data model, matching rollout strategy.backend/compact-connect/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json (1)
31-46: LGTM — existing schema enforces exactly one ofclinicalPrivilegeActionCategoryorclinicalPrivilegeActionCategoriesviavalidate_clinical_privilege_action_category_fields.backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py (1)
114-115: Accept dual presence of category fields in record serializationRecord schema’s pre_dump migration intentionally preserves both
clinicalPrivilegeActionCategoriesandclinicalPrivilegeActionCategoryfor backward compatibility, so the test’s expectation is correct.backend/compact-connect/stacks/api_lambda_stack/__init__.py (1)
35-60: LGTM!The integration of
ProviderManagementLambdasis clean and follows the established pattern:
- Data event bus is correctly loaded from SSM parameter to avoid cross-stack references
- The lambda construct is properly initialized with all required dependencies (scope, persistent_stack, data_event_bus)
- The approach is consistent with how
FeatureFlagsLambdasandProviderUsersLambdasare instantiatedbackend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json (1)
631-637: LGTM – Schema supports migration path.The schema correctly adds the new
clinicalPrivilegeActionCategoriesarray field while keeping the deprecatedclinicalPrivilegeActionCategoryfield. Neither is required, which allows for:
- Old clients to continue using the singular field
- New clients to use the plural field
- Gradual migration without breaking existing integrations
This aligns with the feature-flag driven migration strategy described in the PR.
Also applies to: 654-654
backend/compact-connect/tests/smoke/smoke_common.py (2)
33-33: LGTM!Adding
LICENSE_TYPESto the environment follows the established pattern forCOMPACTSandJURISDICTIONSat lines 31-32.
251-264: LGTM – Well-structured helper function.The
get_provider_user_recordsfunction is a clean addition that:
- Mirrors the pattern of
get_all_provider_database_records(lines 238-248)- Returns a typed
ProviderUserRecordsobject for better testability- Includes proper documentation
- Uses consistent query patterns with
KeyConditionExpressionThis will enable more precise smoke test assertions using the typed helper methods from
ProviderUserRecords.backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json (1)
631-637: LGTM – Consistent schema migration.This snapshot follows the same migration pattern as
PROVIDER_USER_RESPONSE_SCHEMA.json:
- Adds
clinicalPrivilegeActionCategoriesarray field (lines 631-637, 1443-1449)- Removes the singular field from required lists (lines 654, 1466)
- Maintains the old field for backward compatibility
The consistency across schemas ensures a smooth migration path.
Also applies to: 654-654, 1443-1449, 1466-1466
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
1430-1442: Feature-flagged encumbranceDetails handling looks goodConditional population with plural/singular fields is correct and consistent with schema changes.
2676-2690: Plural/singular encumbranceDetails for license-driven privilege encumbranceLGTM. Includes licenseJurisdiction and adverseActionId; aligns with EncumbranceDetails schema.
backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py (2)
914-916: Flag mock for multi-category pathMocking is_feature_enabled to True is appropriate to exercise the new path. Remember to remove with the flag.
995-999: Second flag-mocked test signature updateLooks good and consistent with the added decorator.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_privilege_history.py (3)
83-86: Staff test data updated to plural categoriesGood move to exercise multi-category support.
225-227: Flag mock for staff endpointAppropriate to align expectations with multi-category behavior. Remove once flag is retired.
260-261: Assertion updated to comma-separated categoriesMatches the utility behavior (joining categories). Looks correct.
backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py (5)
21-21: Import of provider_user_records util looks goodSwitching tests to use ProviderUserRecords directly is a solid move for determinism and clarity.
340-346: Provider encumbered state validation via provider_user_recordsGood direct validation against provider DynamoDB record.
351-360: Adverse action retrieval via ProviderUserRecordsUsing typed accessors and returning to_dict() for assertions is clean and consistent.
Also applies to: 364-373
374-411: Adverse action vs request verificationSet-based comparison for categories is correct and order-agnostic. Consider normalizing case if upstream ever varies casing; otherwise this is solid.
503-506: Enum values validated
AllencumbranceTypeandclinicalPrivilegeActionCategoriesstrings in the smoke tests exactly match their respective enum definitions.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py (1)
98-103: Response schema fields look alignedExposing clinicalPrivilegeActionCategories with deprecated singular alias is fine for transition. Consider adding a deprecation note in API docs.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py (1)
35-44: Record schema field changes look correctPlural categories with deprecated singular alias is appropriate; validators and required flags match the migration strategy.
backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (1)
109-126: Lambda wiring via ApiLambdaStack looks consistentHandlers are referenced cleanly and API methods use 29s timeouts with existing models/authorizers. Good refactor to centralize Lambda construction/wiring.
Also applies to: 133-151, 185-229
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (4)
198-205: Decorator order and mock args alignment look correct.Patching order matches the function signature (publish_event first, then flag). Good.
222-266: Good coverage for “all privileges already encumbered” with flag ON.AA seeding + assertions for encumbranceDetails using clinicalPrivilegeActionCategories look correct.
251-261: Encumbrance details now use plural categories — LGTM.Asserting clinicalPrivilegeActionCategories list is correct per the new model.
266-273: Flag-OFF variant preserved — LGTM.Signature and decorator order are consistent with the patched mocks.
backend/compact-connect/tests/app/test_api/test_provider_management_api.py (4)
217-221: Handler wiring assertions against api_lambda_stack are correct.Handler names align with PythonFunction(index='handlers/providers.py', handler=...).
234-238: Imported Lambda integration helper usage looks right. Please confirm API Lambda outputs exist.Using ImportValue requires ApiLambdaStack to export Lambda ARNs. Ensure corresponding CfnOutputs exist for:
- GetProviderHandler
- QueryProvidersHandler
- GetProviderSSNHandler
- DeactivatePrivilegeHandler
- ProviderEncumbranceHandler
Would you confirm ApiLambdaStack exports these? If helpful, I can scan the repo and pinpoint missing outputs.
399-475: SSN alarms relocation validated — LGTM.Alarms asserted from api_lambda_stack; 4xx API alarm still in api_stack. Snapshots compare the properties minus actions as intended.
579-586: Encumbrance handler integration (imported lambda) verified — LGTM.Request model capture + integration to provider_encumbrance_handler match the refactor.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (1)
136-178: Privilege update record with plural categories under flag — LGTM.Assertions for updatedValues, effective/create dates, and encumbranceDetails look correct.
backend/compact-connect/stacks/api_lambda_stack/provider_management.py (4)
52-81: GetProvider handler wiring and grants — LGTM.Env, code entry, decrypt/bucket/table grants, and nag suppression look sound.
118-169: SSN handler permissions and alarms — LGTM with a note.Inline policy for PutFunctionConcurrency and alarm wiring look correct. Note: you’re attaching to a shared role (api_query_role). Ensure it’s not reused by other Lambdas to avoid over‑permitting.
266-297: Deactivate privilege handler wiring — LGTM.Grants and event bus permissions look correct.
299-329: Provider encumbrance handler wiring — LGTM.Grants and nag suppression match expected needs.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
Show resolved
Hide resolved
...compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py
Show resolved
Hide resolved
...mpact-connect/tests/resources/snapshots/GET_PROVIDER_SSN_ANOMALY_DETECTION_ALARM_SCHEMA.json
Show resolved
Hide resolved
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)
2629-2632: Docstring types: add type for adverse_action_id and use datetime.dateCurrent docstring omits the type for adverse_action_id. Also prefer datetime.date for clarity.
- :param adverse_action_id: The ID of the adverse action. - :param str license_type_abbreviation: The license type abbreviation. - :param date effective_date: effective date of the encumbrance on the license and therefore privilege. + :param str adverse_action_id: The ID of the adverse action. + :param str license_type_abbreviation: The license type abbreviation. + :param datetime.date effective_date: Effective date of the encumbrance on the license (and therefore privilege).
🧹 Nitpick comments (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
1430-1442: Feature-flagged multi-category support looks goodBranching on encumbrance-multi-category-flag and shaping encumbranceDetails for privilege is correct for the migration. Minor: consider hoisting the import to module scope to avoid repeating inline imports.
Based on learnings
2676-2690: Standardize adverseActionId type in encumbranceDetailsHere adverseActionId uses the string parameter while encumber_privilege uses the UUID from the fetched record. For consistency with schema and downstream consumers, prefer the UUID from the loaded adverse_action object.
- encumbrance_details = { - 'clinicalPrivilegeActionCategories': adverse_action.clinicalPrivilegeActionCategories, - 'licenseJurisdiction': jurisdiction, - 'adverseActionId': adverse_action_id, - } + encumbrance_details = { + 'clinicalPrivilegeActionCategories': adverse_action.clinicalPrivilegeActionCategories, + 'licenseJurisdiction': jurisdiction, + 'adverseActionId': adverse_action.adverseActionId, + } @@ - encumbrance_details = { - 'clinicalPrivilegeActionCategory': adverse_action.clinicalPrivilegeActionCategory, - 'licenseJurisdiction': jurisdiction, - 'adverseActionId': adverse_action_id, - } + encumbrance_details = { + 'clinicalPrivilegeActionCategory': adverse_action.clinicalPrivilegeActionCategory, + 'licenseJurisdiction': jurisdiction, + 'adverseActionId': adverse_action.adverseActionId, + }Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py:60-0
Timestamp: 2025-10-15T19:56:58.007Z
Learning: In the CompactConnect codebase, when migrating from a single field to a list field (e.g., clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories), both fields are intentionally kept as optional during the migration period to support backwards compatibility. Mutual exclusivity validation is not enforced during this phase, as the deprecated field will be removed in a follow-up PR with migration scripts.
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-10-15T19:53:00.390Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.390Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (4)
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py (1)
is_feature_enabled(46-112)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (8)
clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)adverseActionId(126-127)adverseActionId(130-131)clinicalPrivilegeActionCategory(84-85)clinicalPrivilegeActionCategory(88-91)jurisdiction(43-44)jurisdiction(47-48)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
get_adverse_action_by_id(525-535)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCInternalException(44-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
2642-2654: Adverse action lookup path is fineUsing ProviderUserRecords.get_adverse_action_by_id without UUID/string normalization and raising CCInternalException on miss aligns with current behavior.
Based on learnings
jusdino
left a comment
jusdino
left a comment
There was a problem hiding this comment.
Looks good! Just to confirm my understanding:
While the feature flag is inactive
- existing API and DB behavior remain
While the feature flag is active
- We accept either field
- We require that one must be included to create an encumbrance
- We internally translate everything to the new field
- We return the new field
While the feature flag scaffolding remains
- Both fields in schema are present and optional
Once the feature flag scaffolding is removed
- the new field is required
- we perform no more translation
...-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (3)
23-31: Consolidate feature‑flag mocking to a single mechanismYou patch is_feature_enabled at class level and also per‑test elsewhere. This is redundant and can lead to confusion. Keep the class‑level patch with a MagicMock and flip return_value in individual tests that need False.
204-211: Remove redundant is_feature_enabled patch on this testThis test already benefits from the class‑level patch. Dropping the per‑test patch simplifies the signature and avoids double-patching.
Apply this diff:
- @patch('cc_common.feature_flag_client.is_feature_enabled', return_value=True) @patch('cc_common.event_bus_client.EventBusClient._publish_event') - def test_license_encumbrance_listener_handles_all_privileges_already_encumbered( - self, - mock_publish_event, - mock_flag, # noqa: ARG002 - ): + def test_license_encumbrance_listener_handles_all_privileges_already_encumbered( + self, + mock_publish_event, + ):
135-136: Duplicate comment lineRemove the repeated “Now verify the rest with comprehensive assertion”.
- # Now verify the rest with comprehensive assertion - # Now verify the rest with comprehensive assertion + # Now verify the rest with comprehensive assertionbackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (2)
385-389: Comment contradicts assertions — fix wordingThe code asserts the deprecated field exists, but the comment says “not present”. Update the comment to avoid confusion.
- # TODO - remove this assertion as part of https://github.com/csg-org/CompactConnect/issues/1136 # noqa: FIX002 - # Verify that the deprecated field is not present in the stored data + # TODO - remove this assertion as part of https://github.com/csg-org/CompactConnect/issues/1136 # noqa: FIX002 + # Verify that the deprecated field is still present in the stored data during migration
675-679: Comment contradicts assertions — fix wording (license variant)Same issue here; adjust the comment to match the asserts.
- # TODO - remove this assertion as part of https://github.com/csg-org/CompactConnect/issues/1136 # noqa: FIX002 - # Verify that the deprecated field is not present in the stored data + # TODO - remove this assertion as part of https://github.com/csg-org/CompactConnect/issues/1136 # noqa: FIX002 + # Verify that the deprecated field is still present in the stored data during migration
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py(2 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(9 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(8 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py:60-0
Timestamp: 2025-10-15T19:56:58.007Z
Learning: In the CompactConnect codebase, when migrating from a single field to a list field (e.g., clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories), both fields are intentionally kept as optional during the migration period to support backwards compatibility. Mutual exclusivity validation is not enforced during this phase, as the deprecated field will be removed in a follow-up PR with migration scripts.
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py
🧬 Code graph analysis (3)
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (2)
clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (4)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (3)
put_default_adverse_action_record_in_provider_table(154-160)put_default_provider_record_in_provider_table(409-429)put_default_privilege_record_in_provider_table(310-320)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (1)
license_encumbrance_listener(186-243)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(173-204)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
serialize_to_database_record(205-208)PrivilegeEncumberedStatusEnum(317-321)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (7)
PrivilegeUpdateData(91-162)compact(28-29)compact(108-109)jurisdiction(32-33)jurisdiction(112-113)encumbranceDetails(151-155)updatedValues(124-125)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
serialize_to_database_record(205-208)from_database_record(124-139)to_dict(169-180)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (9)
compact(27-28)compact(31-32)jurisdiction(43-44)jurisdiction(47-48)AdverseActionData(14-147)clinicalPrivilegeActionCategory(84-85)clinicalPrivilegeActionCategory(88-91)clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_default_privilege_update(329-356)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (5)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
142-147: Default AA uses plural categories — good migration stepSetting clinicalPrivilegeActionCategories with a single-element list matches the new schema and keeps defaults stable.
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py (1)
96-97: Assertions updated to plural field look correctGetter and snapshot now validate clinicalPrivilegeActionCategories as a list; consistent with schema.
Also applies to: 114-115
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (1)
120-166: Clarify intended publish semantics for already‑encumbered privilegesThis test expects two privilege.encumbrance events even though one privilege was already encumbered. Please confirm this is intentional (audit signal even without state change) and update the handler/docstring if needed so “affected_privileges” semantics and logs (privileges_encumbered=...) remain accurate.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (2)
398-421: 400 on “both fields provided” — confirm alignment with migration policyThese tests enforce a 400 when both clinicalPrivilegeActionCategory and clinicalPrivilegeActionCategories are provided. Prior migration guidance was to keep both optional and not enforce mutual exclusivity until the deprecated field is removed. Confirm this stricter API validation is deliberate and documented for clients.
Based on learnings
Also applies to: 688-711
139-181: EncumbranceDetails plural field usage looks correct under flag-onExpected privilege update record uses clinicalPrivilegeActionCategories with AA id; matches the migration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (1)
360-685: Consider extracting common migration test logic.The privilege migration tests (lines 360-420) and license migration tests (lines 649-710) follow nearly identical patterns:
- Create encumbrance with deprecated field
- Verify both old and new fields are present in database
- Verify error when both fields provided
While test duplication is acceptable, extracting a helper method could reduce maintenance burden:
def _verify_category_field_migration(self, record_type: str, test_record, query_key_prefix: str): """Helper to verify migration from singular to plural category field""" # Query database # Assert both fields present # Assert list contains migrated valueThis would make the tests more concise and easier to update when the migration completes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(9 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py:60-0
Timestamp: 2025-10-15T19:56:58.007Z
Learning: In the CompactConnect codebase, when migrating from a single field to a list field (e.g., clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories), both fields are intentionally kept as optional during the migration period to support backwards compatibility. Mutual exclusivity validation is not enforced during this phase, as the deprecated field will be removed in a follow-up PR with migration scripts.
🧬 Code graph analysis (2)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (4)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (3)
put_default_adverse_action_record_in_provider_table(154-160)put_default_provider_record_in_provider_table(409-429)put_default_privilege_record_in_provider_table(310-320)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
PrivilegeEncumberedStatusEnum(317-321)serialize_to_database_record(205-208)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (1)
license_encumbrance_listener(186-243)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(173-204)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (7)
PrivilegeUpdateData(91-162)compact(28-29)compact(108-109)jurisdiction(32-33)jurisdiction(112-113)encumbranceDetails(151-155)updatedValues(124-125)backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py (1)
encumbrance_handler(42-54)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (9)
compact(27-28)compact(31-32)jurisdiction(43-44)jurisdiction(47-48)AdverseActionData(14-147)clinicalPrivilegeActionCategory(84-85)clinicalPrivilegeActionCategory(88-91)clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_default_privilege_update(329-356)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (5)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (2)
89-93: LGTM! Proper test data setup.Adding adverse action records before testing encumbrance events correctly simulates the expected state where license-level adverse actions exist and trigger privilege encumbrance flows.
Also applies to: 224-228, 291-297, 348-351, 591-594, 737-740, 791-794
256-263: LGTM! Proper verification of field migration.The tests correctly verify the migration from
clinicalPrivilegeActionCategory(singular) toclinicalPrivilegeActionCategories(plural) under different feature flag states:
- Lines 256-263: Verify plural field when flag is enabled
- Lines 326-333: Verify singular field when flag is disabled
This ensures backward compatibility during the migration period.
Also applies to: 326-333
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (3)
100-180: LGTM! Feature-flag-gated privilege encumbrance tests.The tests properly verify encumbrance handling under different feature flag states:
- Lines 100-137: With flag enabled, verifies plural
clinicalPrivilegeActionCategoriesfield- Lines 139-180: Verifies privilege update records with plural field
- Lines 182-228: With flag disabled, verifies singular
clinicalPrivilegeActionCategoryfieldThe
mock_flagparameter is correctly marked as unused since it's only needed for the decorator.
359-420: LGTM! Comprehensive migration and validation tests.These tests properly verify:
- Lines 360-395: Deprecated
clinicalPrivilegeActionCategoryis automatically migrated toclinicalPrivilegeActionCategorieslist, with both fields present in stored data during migration- Lines 397-420: Proper 400 error when both singular and plural fields are provided, preventing ambiguous input
This ensures a smooth migration path and proper error handling.
Based on learnings: The codebase intentionally keeps both fields as optional during migration to support backward compatibility, and mutual exclusivity validation is deferred until the deprecated field is removed in a follow-up PR.
476-477: LGTM! Consistent license encumbrance migration tests.The license encumbrance tests mirror the privilege encumbrance migration pattern:
- Lines 476-514: Verify plural field with flag enabled
- Lines 649-685: Verify field migration
- Lines 687-710: Verify mutual exclusivity validation
The TODO comments appropriately track that these singular field tests will be removed in issue #1136.
Also applies to: 505-506, 649-710
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
Show resolved
Hide resolved
|
@jlkravitz This is ready for your review. Thanks! |
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
Looks good to me! @isabeleliassen This is good to merge.
rather than pass this data through the event bus, this uses the AA id to grab the data directly to load the categories
landonshumway-ia
force-pushed
the
feat/encumbrance-multiple-npdb-categories
branch
from
cdfd125 to
1880cdf
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
backend/compact-connect/stacks/api_lambda_stack/provider_management.py (1)
107-123: Explicit cdk‑nag suppression accepted.Hard‑coded logical ID in appliesTo is intentional in this repo to force review on underlying changes. No action.
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (1)
269-341: Avoid mutating shared MagicMock state; patch the flag per-test.Flip/reset of a class-scoped MagicMock is brittle and can leak across tests on failures/parallelism. Patch the flag on this test instead and drop manual reset.
- @patch('cc_common.event_bus_client.EventBusClient._publish_event') - def test_license_encumbrance_listener_handles_all_privileges_already_encumbered_with_experiment_disabled( - self, - mock_publish_event, - ): - mock_feature_client.return_value = False + @patch('cc_common.feature_flag_client.is_feature_enabled', return_value=False) + @patch('cc_common.event_bus_client.EventBusClient._publish_event') + def test_license_encumbrance_listener_handles_all_privileges_already_encumbered_with_experiment_disabled( + self, + mock_publish_event, + mock_flag, # noqa: ARG002 + ): @@ - # reset the flag client for the other tests - mock_feature_client.return_value = True
🧹 Nitpick comments (20)
backend/compact-connect/stacks/api_stack/v1_api/api_model.py (4)
417-441: Consider adding validation constraints and precedence documentation.The migration from singular to plural field is well-structured for backward compatibility. However, consider these enhancements:
Array validation: The
clinicalPrivilegeActionCategoriesarray has nomaxItemsconstraint or item-level validation. Should the items be constrained to specific enum values, non-empty strings, or have a reasonable max array length?Field precedence: When both
clinicalPrivilegeActionCategoryandclinicalPrivilegeActionCategoriesare provided in a request, which takes precedence? This should be documented in the description or handled with validation logic in the handler.Based on learnings
454-478: Consider adding validation constraints and precedence documentation.Same concerns as the privilege encumbrance model:
Array validation: Add
maxItemsconstraint and consider item-level validation forclinicalPrivilegeActionCategories.Field precedence: Document which field takes precedence when both are provided in a request.
Consistency between privilege and license encumbrance models is good.
Based on learnings
1217-1223: Response schema migration looks good.The addition of
clinicalPrivilegeActionCategoriesto the adverse action response schema is well-structured for the migration. Both fields remain optional, allowing clients to handle either format.For consistency with request models, consider adding a
maxItemsconstraint to the array, even in response schemas, to document expected bounds.Based on learnings
1358-1364: Response schema migration looks good.Consistent implementation with the license adverse actions schema. Both fields remain optional for backward compatibility during the migration period.
Same suggestion as above: consider adding a
maxItemsconstraint for consistency with request models.Based on learnings
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_privilege_history.py (1)
260-260: Expected output format is correct, but consider edge case coverage.The comma-separated format for multiple categories is appropriate. However, consider adding test cases for:
- Single category in the list (to verify no trailing comma)
- Empty list (if allowed)
- Categories with special characters or commas in their names
backend/compact-connect/tests/app/test_api/test_provider_management_api.py (2)
201-203: Good pivot to imported‑lambda pattern.Using api_lambda_stack and its template is correct for imported Lambda integrations. Consider moving these repeated lookups into setUp to DRY tests.
Also applies to: 263-265, 341-343, 404-405, 481-483, 576-578, 637-639, 692-694, 760-762
218-221: Handler name assertions may be brittle.Hard‑coding 'Handler' strings can drift with bundling changes. Prefer asserting by logical ID via get_logical_id(...) or by matching Integration Uri ImportValue instead of Handler literal.
Also applies to: 281-284, 358-361, 485-488, 580-583
backend/compact-connect/stacks/api_lambda_stack/provider_management.py (1)
177-220: Minor: comment vs period mismatch.Comment says “within a day,” but MetricStat uses 3600s period. If intentional for anomaly sensitivity, update comment; otherwise consider aligning periods.
backend/compact-connect/tests/smoke/smoke_common.py (1)
251-265: Add pagination and ConsistentRead to avoid truncated provider snapshots.Current Query returns a single page and may miss items; optionally fail fast if none.
-def get_provider_user_records(compact: str, provider_id: str) -> ProviderUserRecords: +def get_provider_user_records(compact: str, provider_id: str, *, consistent_read: bool = True) -> ProviderUserRecords: """ Get all provider records from DynamoDB and return as ProviderUserRecords utility class. :param compact: The compact identifier :param provider_id: The provider's ID + :param consistent_read: Use strongly consistent reads (default True) :return: ProviderUserRecords instance containing all records for this provider """ - # Query the provider database for all records - query_result = config.provider_user_dynamodb_table.query( - KeyConditionExpression=Key('pk').eq(f'{compact}#PROVIDER#{provider_id}') - ) - - return ProviderUserRecords(query_result['Items']) + items = [] + last_evaluated_key = None + while True: + params = { + 'Select': 'ALL_ATTRIBUTES', + 'KeyConditionExpression': Key('pk').eq(f'{compact}#PROVIDER#{provider_id}'), + 'ConsistentRead': consistent_read, + } + if last_evaluated_key: + params['ExclusiveStartKey'] = last_evaluated_key + resp = config.provider_user_dynamodb_table.query(**params) + items.extend(resp.get('Items', [])) + last_evaluated_key = resp.get('LastEvaluatedKey') + if not last_evaluated_key: + break + if not items: + raise SmokeTestFailureException('Provider not found') + return ProviderUserRecords(items)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (3)
126-133: Don’t mutate Mock call kwargs in-place.Use a shallow copy before pop to avoid side effects on mock internals.
- called_event_batch_writer_1 = call_args_1.pop('event_batch_writer') - called_event_batch_writer_2 = call_args_2.pop('event_batch_writer') + call_args_1 = dict(call_args_1) + call_args_2 = dict(call_args_2) + called_event_batch_writer_1 = call_args_1.pop('event_batch_writer') + called_event_batch_writer_2 = call_args_2.pop('event_batch_writer')
120-166: Tweak comment to match assertions.Docstring says “affected privilege” (singular) but you assert two events. Update wording to avoid confusion.
89-93: Reduce repeated AA setup with a small helper.AA-for-license inserts are duplicated across tests. Consider a local helper (or extend TestDataGenerator) to DRY these lines and keep intent clear.
Also applies to: 224-228, 349-351, 591-595, 737-740, 791-795
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (1)
151-157: Avoid hard-coding 'slp' in SK prefix; use the record’s abbreviation.Improves resilience if defaults change.
- & Key('sk').begins_with( - f'{test_privilege_record.compact}#PROVIDER#privilege/{test_privilege_record.jurisdiction}/slp#UPDATE' - ), + & Key('sk').begins_with( + f'{test_privilege_record.compact}#PROVIDER#privilege/' + f'{test_privilege_record.jurisdiction}/{test_privilege_record.licenseTypeAbbreviation}#UPDATE' + ),backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (2)
82-91: Deprecation note and caller guidance for clinicalPrivilegeActionCategoryGood to keep the singular property during migration. Please document the deprecation and the Optional return to prevent surprises at call sites.
Apply this small docstring addition:
@property def clinicalPrivilegeActionCategory(self) -> str | None: - return self._data.get('clinicalPrivilegeActionCategory') + """Deprecated: prefer clinicalPrivilegeActionCategories. Returns None if unset.""" + return self._data.get('clinicalPrivilegeActionCategory')Optionally verify downstream callers handle None; I can provide a grep script if helpful. Based on learnings.
93-100: DX: accept enums in plural setterSetter only accepts list[str]. To improve ergonomics while keeping storage as strings, accept ClinicalPrivilegeActionCategory items too.
-@clinicalPrivilegeActionCategories.setter -def clinicalPrivilegeActionCategories(self, value: list[str]) -> None: - self._data['clinicalPrivilegeActionCategories'] = value +@clinicalPrivilegeActionCategories.setter +def clinicalPrivilegeActionCategories(self, value: list[str] | list[ClinicalPrivilegeActionCategory]) -> None: + normalized = [ + v.value if isinstance(v, ClinicalPrivilegeActionCategory) else v + for v in value] + self._data['clinicalPrivilegeActionCategories'] = normalizedbackend/compact-connect/tests/smoke/encumbrance_smoke_tests.py (3)
242-260: Reduce flakiness: poll for license/provider encumbered state (eventual consistency)Privilege validation already polls; license/provider checks are single-shot reads and may flake under eventual consistency.
- Mirror the polling approach used in validate_privilege_encumbered_state for:
- validate_license_encumbered_state
- validate_provider_encumbered_state
- Or allow a short retry loop (e.g., 3 attempts with 5s sleep).
Also applies to: 291-298, 337-346
374-411: Tests: support singular fallback for robustnessIf a legacy record sneaks through with the singular field, this matcher will miss it.
Make the matcher tolerate both shapes:
- for adverse_action in adverse_actions: - action_type = adverse_action.get('encumbranceType') - action_categories = set(adverse_action.get('clinicalPrivilegeActionCategories', [])) + for adverse_action in adverse_actions: + action_type = adverse_action.get('encumbranceType') + plural = adverse_action.get('clinicalPrivilegeActionCategories') or [] + # singular fallback for robustness during migration + if not plural and 'clinicalPrivilegeActionCategory' in adverse_action: + plural = [adverse_action['clinicalPrivilegeActionCategory']] + action_categories = set(plural)
512-546: Good assertions; consider asserting that categories persisted as requestedNice state assertions. Optionally assert that at least one adverse action contains exactly the requested categories to fully exercise the write/read path.
I can add a helper usage here to assert category round-trip using verify_license_adverse_action_matches_request.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py (1)
49-60: Migration hooks: allow duplicates? consider validationMigration looks good. If duplicates can be posted, they’ll persist. If that’s undesirable, add a schema-level check to reject duplicates.
Example:
@validates_schema def validate_license_type(self, data, **_kwargs): ... + + # Also ensure categories (if present) are unique +@validates_schema +def validate_categories_unique(self, data, **_kwargs): + cats = data.get('clinicalPrivilegeActionCategories') + if cats is not None and len(cats) != len(set(cats)): + raise ValidationError({'clinicalPrivilegeActionCategories': ['Duplicate entries are not allowed.']})Also applies to: 76-83
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py (1)
32-48: Mutual exclusivity validator: concise and clearExactly-one-of check is good. Consider adding an explicit check that, when plural is provided, it’s not empty (pairs well with Length(min=1)).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
backend/compact-connect/common_constructs/python_common_layer_versions.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(4 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py(0 hunks)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py(2 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py(2 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(0 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(9 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(8 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_privilege_history.py(3 hunks)backend/compact-connect/stacks/api_lambda_stack/__init__.py(2 hunks)backend/compact-connect/stacks/api_lambda_stack/provider_management.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(6 hunks)backend/compact-connect/stacks/api_stack/v1_api/provider_management.py(6 hunks)backend/compact-connect/stacks/event_listener_stack/__init__.py(1 hunks)backend/compact-connect/stacks/feature_flag_stack/__init__.py(1 hunks)backend/compact-connect/tests/app/test_api/test_provider_management_api.py(23 hunks)backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json(4 hunks)backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_SSN_ANOMALY_DETECTION_ALARM_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_SSN_ENDPOINT_DISABLED_ALARM_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_SSN_READS_RATE_LIMITED_ALARM_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json(4 hunks)backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py(16 hunks)backend/compact-connect/tests/smoke/smoke_common.py(2 hunks)
💤 Files with no reviewable changes (2)
- backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
- backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
🚧 Files skipped from review as they are similar to previous changes (12)
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py
- backend/compact-connect/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json
- backend/compact-connect/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
- backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py
- backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_SSN_ANOMALY_DETECTION_ALARM_SCHEMA.json
- backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py
- backend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json
- backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_SSN_ENDPOINT_DISABLED_ALARM_SCHEMA.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
- backend/compact-connect/stacks/api_stack/v1_api/api.py
- backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py:60-0
Timestamp: 2025-10-15T19:56:58.007Z
Learning: In the CompactConnect codebase, when migrating from a single field to a list field (e.g., clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories), both fields are intentionally kept as optional during the migration period to support backwards compatibility. Mutual exclusivity validation is not enforced during this phase, as the deprecated field will be removed in a follow-up PR with migration scripts.
📚 Learning: 2025-10-15T19:56:58.007Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py:60-0
Timestamp: 2025-10-15T19:56:58.007Z
Learning: In the CompactConnect codebase, when migrating from a single field to a list field (e.g., clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories), both fields are intentionally kept as optional during the migration period to support backwards compatibility. Mutual exclusivity validation is not enforced during this phase, as the deprecated field will be removed in a follow-up PR with migration scripts.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.pybackend/compact-connect/stacks/api_stack/v1_api/api_model.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/stacks/api_stack/v1_api/api_model.pybackend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/stacks/api_stack/v1_api/api_model.pybackend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py
📚 Learning: 2025-10-15T19:53:00.390Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.390Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
📚 Learning: 2025-09-03T22:29:24.535Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py:0-0
Timestamp: 2025-09-03T22:29:24.535Z
Learning: The EncumbranceEventDetailSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py is used across multiple instances/contexts where adverseActionCategory and adverseActionId fields may be required in some cases and not present in others. This is an intentional design pattern for handling different event variants with a single schema.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py
📚 Learning: 2025-10-15T21:52:47.370Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1135
File: backend/compact-connect/stacks/api_lambda_stack/provider_management.py:100-0
Timestamp: 2025-10-15T21:52:47.370Z
Learning: In the CompactConnect repository, the team prefers to keep cdk-nag suppressions explicit with hard-coded logical IDs (e.g., 'Resource::<ProviderTableEC5D0597.Arn>/index/*') rather than using generic suppressions. This intentional brittleness ensures that when underlying resources change, the suppressions break and force manual review of the IAM policies.
Applied to files:
backend/compact-connect/stacks/api_lambda_stack/provider_management.py
🧬 Code graph analysis (15)
backend/compact-connect/tests/smoke/smoke_common.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
ProviderUserRecords(403-791)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(173-204)backend/compact-connect/tests/smoke/config.py (1)
provider_user_dynamodb_table(40-41)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (2)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
get(86-87)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
ClinicalPrivilegeActionCategory(375-388)
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (2)
clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)
backend/compact-connect/stacks/event_listener_stack/__init__.py (1)
backend/common-cdk/common_constructs/stack.py (2)
api_domain_name(110-113)common_env_vars(81-89)
backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (2)
backend/compact-connect/stacks/api_lambda_stack/__init__.py (1)
ApiLambdaStack(33-222)backend/compact-connect/stacks/state_api_stack/v1_api/provider_management.py (1)
_add_get_provider(74-106)
backend/compact-connect/stacks/feature_flag_stack/__init__.py (1)
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (2)
FeatureFlagResource(23-72)FeatureFlagEnvironmentName(15-20)
backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(173-204)backend/compact-connect/tests/smoke/smoke_common.py (1)
get_provider_user_records(251-264)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (7)
get_privilege_records(481-489)get_specific_license_record(447-462)get_specific_privilege_record(464-479)get_provider_record(79-84)get_provider_record(605-614)get_adverse_action_records_for_license(507-523)get_adverse_action_records_for_privilege(587-603)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
licenseTypeAbbreviation(156-167)to_dict(169-180)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (2)
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py (1)
is_feature_enabled(46-112)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (3)
AdverseActionData(14-147)adverseActionId(126-127)adverseActionId(130-131)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (4)
clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)clinicalPrivilegeActionCategory(84-85)clinicalPrivilegeActionCategory(88-91)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
ClinicalPrivilegeActionCategoryField(114-116)
backend/compact-connect/tests/app/test_api/test_provider_management_api.py (2)
backend/compact-connect/tests/app/test_api/__init__.py (2)
TestApi(11-83)generate_expected_integration_object_for_imported_lambda(48-83)backend/compact-connect/tests/app/base.py (1)
get_resource_properties_by_logical_id(89-96)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (4)
clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)clinicalPrivilegeActionCategory(84-85)clinicalPrivilegeActionCategory(88-91)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
ClinicalPrivilegeActionCategoryField(114-116)
backend/compact-connect/stacks/api_lambda_stack/__init__.py (1)
backend/compact-connect/stacks/api_lambda_stack/provider_management.py (1)
ProviderManagementLambdas(18-336)
backend/compact-connect/stacks/api_lambda_stack/provider_management.py (4)
backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(19-138)backend/compact-connect/stacks/persistent_stack/__init__.py (1)
PersistentStack(35-480)backend/compact-connect/stacks/api_lambda_stack/__init__.py (1)
ApiLambdaStack(33-222)backend/compact-connect/lambdas/python/common/cc_common/config.py (6)
provider_table(88-89)ssn_table(92-93)ssn_index_name(179-180)event_bus_name(84-85)rate_limiting_table(282-283)user_pool_id(191-195)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (7)
PrivilegeUpdateData(91-162)compact(28-29)compact(108-109)jurisdiction(32-33)jurisdiction(112-113)encumbranceDetails(151-155)updatedValues(124-125)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
serialize_to_database_record(205-208)from_database_record(124-139)to_dict(169-180)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (9)
compact(27-28)compact(31-32)jurisdiction(43-44)jurisdiction(47-48)AdverseActionData(14-147)clinicalPrivilegeActionCategory(84-85)clinicalPrivilegeActionCategory(88-91)clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_default_privilege_update(329-356)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (6)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (3)
put_default_adverse_action_record_in_provider_table(154-160)put_default_provider_record_in_provider_table(409-429)put_default_privilege_record_in_provider_table(310-320)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
PrivilegeEncumberedStatusEnum(317-321)serialize_to_database_record(205-208)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (1)
license_encumbrance_listener(186-243)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(173-204)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (3)
compact(28-29)compact(108-109)encumberedStatus(72-73)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
get_privilege_records(481-489)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (26)
backend/compact-connect/common_constructs/python_common_layer_versions.py (1)
53-53: Verify intentional removal of conditional retention policy.The removal policy is now always
RETAIN, whereas it previously varied based on environment (sandbox vs. non-sandbox). This means sandbox environments will now accumulate layer versions rather than destroying them, potentially accelerating storage consumption toward the 75 GB AWS limit mentioned in the comment above.Please confirm this change was intentional and that the team has considered the storage implications for sandbox environments.
backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_SSN_READS_RATE_LIMITED_ALARM_SCHEMA.json (1)
2-2: Snapshot updates are correct and intentionally reference different stacks by design.The naming difference across the four alarm snapshots reflects intentional architectural design, not inconsistency:
- GET_PROVIDER_SSN_4XX_ALARM_SCHEMA.json: References
APIStack/LicenseApibecause it monitors API Gateway 4xx errors (test comment explicitly states: "still in api_stack")- Other three alarms (anomaly detection, rate-limited, endpoint disabled): Reference
ApiLambdaStack/GetProviderSSNHandlerbecause they monitor Lambda execution metricsAll alarms monitor the same endpoint (
/v1/compacts/{compact}/providers/{providerId}/ssn) but at different infrastructure layers. The reviewed file's snapshot change is correct—the rate-limited alarm should reference the new Lambda handler in ApiLambdaStack. Snapshot tests pass and validate this expected structure.backend/compact-connect/stacks/event_listener_stack/__init__.py (1)
38-41: ****The concern about mutating
common_env_varsis unfounded. Line 80 shows@cached_property, not@property. Unlike@property(which returns a new dict on each access),@cached_propertycaches the result after the first computation and stores it as an instance attribute. Consequently, the.update()call on line 41 mutates the cached dict in-place, and all subsequent spreads of**self.common_env_vars(lines 63, 111, 158) access the same cached dict withAPI_BASE_URLcorrectly included.Likely an incorrect or invalid review comment.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_privilege_history.py (3)
83-83: Test data correctly exercises the new multi-category field.The use of
clinicalPrivilegeActionCategorieswith a list of two categories appropriately tests the new functionality for the staff endpoint.
28-28: Confirm the intentional divergence in field usage across endpoints.The staff endpoint test uses the new
clinicalPrivilegeActionCategories(plural) field, while the provider user and public endpoint tests continue using the singularclinicalPrivilegeActionCategoryfield. This appears to be an intentional phased rollout where the staff endpoint adopts the new multi-category support first, but please confirm this is the expected behavior.Based on learnings
Also applies to: 53-53, 83-83
225-227: **** The test file already has adequate backward compatibility coverage. The five test methods at lines 107–182 (those without explicit feature flag mocks) implicitly test the disabled flag scenario by running against the unmockedis_feature_enabledclient. The single test at line 227 withreturn_value=Trueexplicitly tests the new enabled behavior. This is the correct test structure: legacy behavior is covered implicitly by default unmocked tests, while the new feature behavior is covered by the explicit mock. No additional test is needed.Likely an incorrect or invalid review comment.
backend/compact-connect/stacks/api_lambda_stack/__init__.py (1)
26-26: Import looks good.ProviderManagementLambdas import aligns with usage below; no issues.
backend/compact-connect/tests/app/test_api/test_provider_management_api.py (2)
407-417: Alarm assertions look solid.Correctly scopes SSN alarms to api_lambda_stack and 4XX API alarm to api_stack. Snapshot approach is appropriate.
Also applies to: 424-456, 458-465, 470-474
234-238: Method is properly exposed as a static class member—no AttributeError risk.The verification confirms that
generate_expected_integration_object_for_imported_lambdais defined as a@staticmethodon theTestApiclass (line 48 inbackend/compact-connect/tests/app/test_api/__init__.py), making it correctly accessible viaTestApi.generate_expected_integration_object_for_imported_lambda()at all referenced locations. No action required.backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (3)
15-15: Clean dependency inversion to ApiLambdaStack.Constructor now consumes pre‑bound handlers; reduces duplication and tightens IAM in one place. Nice.
Also applies to: 34-35
60-66: Correct use of pre‑bound handlers.Directly wiring api_lambda_stack.provider_management_lambdas.* keeps API wiring lean.
117-117: Integrations and timeouts are consistent.29s timeout pattern matches rest of API; authorization wiring is unchanged.
Also applies to: 140-140, 215-215, 241-241, 260-260, 284-284, 303-303, 325-325
backend/compact-connect/stacks/api_lambda_stack/provider_management.py (1)
150-163: Concurrency policy scope is tight.Allowing lambda:PutFunctionConcurrency only on self ARN is appropriate. No change needed.
backend/compact-connect/tests/smoke/smoke_common.py (2)
33-33: LGTM on env propagation.Setting LICENSE_TYPES mirrors COMPACTS/JURISDICTIONS and unblocks helpers.
36-36: Import placement is fine for tests.Late import with E402 suppression after sys.path/env setup is appropriate here.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (3)
100-101: Per-test feature flag patching: good.Keeps tests isolated and avoids global state issues.
139-181: EncumbranceDetails list-field assertions look correct.Validates plural category path with effective/create dates.
359-415: Tests correctly validate backward compatibility during migration phase; no changes needed.Both tests properly cover the temporary dual-field window before
clinicalPrivilegeActionCategoryis removed. The widespread presence of the singular field across frontend, handlers, schemas, and API specs is intentional during this migration period. The validation enforcing mutual exclusivity (lines 399-418 snippet) correctly prevents mixed input and protects the transition. Removal of this field is tracked in issue #1136 with follow-up migration scripts.backend/compact-connect/stacks/feature_flag_stack/__init__.py (1)
116-125: Flag resource wiring looks correct.Name, provider reuse, and auto_enable for TEST align with the rollout plan.
Confirm environments that should auto-enable (TEST only vs also SANDBOX).backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py (4)
96-96: Updated getter assertion to plural field is correct.Matches AdverseActionData API.
114-115: Snapshot now expects clinicalPrivilegeActionCategories list.This aligns with the new data shape.
14-32: Serde round-trip stays stable with migrated shape.Good to exclude dynamic dateOfUpdate before equality.
1-9999: No action required—test_adverse_action.py correctly uses the plural field.The test data generator populates
clinicalPrivilegeActionCategories(plural), and all assertions in test_adverse_action.py reference the plural field. The schema's migration logic automatically converts any legacy singular field to the plural form during load, so there is no brittleness risk in this test file. The migration strategy is working as designed per the backwards compatibility approach outlined in learnings.backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
525-535: LGTM: UUID vs str comparison works as intendedKeeping the direct comparison is correct for this codebase; explicit casting breaks mapping per prior context. No change needed.
Based on learnings.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py (1)
98-103: Response shape maintains backward-compat aliasLooks good to expose the plural while keeping the singular optional for migration. No action.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py (1)
37-44: Require at least one category; prevent empty listsAs written, an empty clinicalPrivilegeActionCategories list is allowed, which undermines the requirement that at least one NPDB category is set.
Add a Length(min=1) validator:
-from marshmallow import ValidationError, pre_dump, pre_load, validates_schema +from marshmallow import ValidationError, pre_dump, pre_load, validates_schema from marshmallow.fields import UUID, Date, DateTime, List, String -from marshmallow.validate import OneOf +from marshmallow.validate import OneOf, Length ... - clinicalPrivilegeActionCategories = List(ClinicalPrivilegeActionCategoryField(), required=False, allow_none=False) + clinicalPrivilegeActionCategories = List( + ClinicalPrivilegeActionCategoryField(), + required=False, + allow_none=False, + validate=Length(min=1), + )⛔ Skipped due to learnings
Learnt from: landonshumway-ia PR: csg-org/CompactConnect#1135 File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py:60-0 Timestamp: 2025-10-15T19:56:58.007Z Learning: In the CompactConnect codebase, when migrating from a single field to a list field (e.g., clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories), both fields are intentionally kept as optional during the migration period to support backwards compatibility. Mutual exclusivity validation is not enforced during this phase, as the deprecated field will be removed in a follow-up PR with migration scripts.
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/compact-connect/stacks/api_lambda_stack/provider_management.py (1)
47-57: Critical issue resolved: log groups now correctly registered.The previous critical issue flagging that handler objects were appended instead of their
.log_groupproperty has been fixed. All five handlers now correctly append.log_groupto theapi_lambda_stack.log_groupslist, ensuring the list containsILogGroupentries as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py(3 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_reporting.py(1 hunks)backend/compact-connect/stacks/api_lambda_stack/provider_management.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(1 hunks)backend/compact-connect/tests/app/test_api/test_purchases_api.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_transaction_reporting.py
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py:60-0
Timestamp: 2025-10-15T19:56:58.007Z
Learning: In the CompactConnect codebase, when migrating from a single field to a list field (e.g., clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories), both fields are intentionally kept as optional during the migration period to support backwards compatibility. Mutual exclusivity validation is not enforced during this phase, as the deprecated field will be removed in a follow-up PR with migration scripts.
📚 Learning: 2025-10-15T21:52:47.370Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1135
File: backend/compact-connect/stacks/api_lambda_stack/provider_management.py:100-0
Timestamp: 2025-10-15T21:52:47.370Z
Learning: In the CompactConnect repository, the team prefers to keep cdk-nag suppressions explicit with hard-coded logical IDs (e.g., 'Resource::<ProviderTableEC5D0597.Arn>/index/*') rather than using generic suppressions. This intentional brittleness ensures that when underlying resources change, the suppressions break and force manual review of the IAM policies.
Applied to files:
backend/compact-connect/stacks/api_lambda_stack/provider_management.py
📚 Learning: 2025-10-15T19:56:58.007Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py:60-0
Timestamp: 2025-10-15T19:56:58.007Z
Learning: In the CompactConnect codebase, when migrating from a single field to a list field (e.g., clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories), both fields are intentionally kept as optional during the migration period to support backwards compatibility. Mutual exclusivity validation is not enforced during this phase, as the deprecated field will be removed in a follow-up PR with migration scripts.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py
📚 Learning: 2025-09-03T22:29:24.535Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py:0-0
Timestamp: 2025-09-03T22:29:24.535Z
Learning: The EncumbranceEventDetailSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py is used across multiple instances/contexts where adverseActionCategory and adverseActionId fields may be required in some cases and not present in others. This is an intentional design pattern for handling different event variants with a single schema.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py
🧬 Code graph analysis (2)
backend/compact-connect/stacks/api_lambda_stack/provider_management.py (6)
backend/compact-connect/stacks/persistent_stack/event_bus.py (1)
EventBus(9-25)backend/common-cdk/common_constructs/stack.py (2)
Stack(18-89)common_env_vars(81-89)backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(19-138)backend/compact-connect/stacks/persistent_stack/__init__.py (1)
PersistentStack(35-480)backend/compact-connect/stacks/api_lambda_stack/__init__.py (1)
ApiLambdaStack(33-222)backend/compact-connect/lambdas/python/common/cc_common/config.py (6)
provider_table(88-89)ssn_table(92-93)ssn_index_name(179-180)event_bus_name(84-85)rate_limiting_table(282-283)user_pool_id(191-195)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (4)
clinicalPrivilegeActionCategories(94-95)clinicalPrivilegeActionCategories(98-99)clinicalPrivilegeActionCategory(84-85)clinicalPrivilegeActionCategory(88-91)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
ClinicalPrivilegeActionCategoryField(114-116)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (6)
backend/compact-connect/stacks/api_lambda_stack/provider_management.py (1)
18-336: Well-structured provider management infrastructure.The
ProviderManagementLambdasclass provides a clean consolidation of provider management handlers with:
- Comprehensive environment variable setup (lines 32-45)
- Appropriate IAM grants for each handler's specific needs
- Proper CDK-NAG suppressions with clear reasoning
- Robust monitoring for the sensitive SSN endpoint (lines 177-269) including anomaly detection, rate limiting, and endpoint disable alarms
- Correct integration with the event bus and persistent resources
The encumbrance handler (lines 306-336) correctly wires the new feature with appropriate table and event bus access.
backend/compact-connect/stacks/api_stack/v1_api/api.py (1)
159-167: Clean refactor: handler wiring delegated to api_lambda_stack.The change to pass
api_lambda_stacktoProviderManagementaligns with the new architecture where provider management handlers are pre-constructed inProviderManagementLambdasand exposed through theapi_lambda_stack. This simplifies the API wiring by removing the need to pass individual persistent resources and the event bus.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py (4)
2-4: LGTM!The new imports are correctly used for the list field definition and mutual exclusivity validation.
28-32: LGTM! Empty list validation is now enforced.The
validate=Length(min=1)on line 29 addresses the previous review concern about accepting empty category lists. The field-level validation will catch empty lists before schema-level validation runs.
34-49: LGTM! Validation logic is correct and clear.The mutual exclusivity validation correctly enforces that exactly one of the two fields must be provided, with clear error messages guiding users toward the new plural field.
101-105: Asymmetry is intentional for migration handling—no action requiredThe difference between POST and response schema validation reflects a deliberate migration design:
- POST schema validates that new requests use exactly one field (enforces data quality on write)
- Response schema has no such validation (allows serializing legacy database records during migration period)
Since
adverseActionsdata is populated from database records viato_dict(), the response schema must tolerate whatever state exists in the database—old singular fields, new plural fields, or transitional states. The POST validation ensures new adverse actions conform to the updated schema, while the response schema remains forgiving for backwards compatibility.This design correctly handles the migration referenced in issue #1136.
4 participants
Several states have requested the ability to set multiple NPDB categories when placing an encumbrance on a license or privilege. This adds support for that in a backwards incompatible manner by adding a new field to the adverse action data type and using that field creating these encumbrance records in the system.
Closes #1090
Summary by CodeRabbit